Rule enhancement: Don't do unnecessary destructuring

[jfmengels]

What the rule should do:

What problems does it solve:

Example of things the rule would report:

view (Model model) =
   div []
    [ viewA (Model model)
    , viewB (Model model)
    ]
-->
view model =
   div []
    [ viewA model
    , viewB model
    ]

Example of things the rule would not report:

view (Model model) =
   div []
    [ viewA (Model model)
    , viewB model -- using the extracted model
    , viewC model.x -- also using the extracted model
    ]

Should this be part of the Simplify rule or should it be a new rule?

I think it could be part of the Simplify rule.

I am looking for:

Someone to work on this :)

[lue-bird]

I'd use it if it existed, but...

In practice, rules handling patterns in this form like below are pretty useless because

• the rule can't provide a good default name for the undestructured
  • → doing the refactor is dumb work but still takes effort
    • revisit in the future when fixes can take user input? 👀
  • → deciding on a name isn't always easy and needs context
  • → the rule might be seen as annoying
• adding the variant name isn't hugely verbose
• variant names etc. add some descriptive names
• in code that's changing a lot, you might still want to quickly be able to access the value(s)
  • → the rule might be seen as annoying
• false negatives for records without types: "How many fields does the original record have?"

  \{ field } -> f { field = field }
  

type Id = IdUnsafe Int

toString2 combine (IdUnsafe aInt) (IdUnsafe bInt) =
    combine (IdUnsafe aInt |> toString) (IdUnsafe bInt |> toString)

orElse alternative maybe =
    case maybe of
        Nothing ->
            alternative
        Just content ->
            Just content

tupleCompare ( first, second ) =
    compair ( first, second ) Descending

attempts at making names

toString2 combine idHoldingAInt?? idHoldingBInt?? =
    combine (idHoldingAInt?? |> toString) (idHoldingBInt?? |> toString)

orElse alternative maybe =
    case maybe of
        Nothing ->
            alternative
        maybeHoldingContent?? ->
            maybeHoldingContent??

\firstSecond?? -> compair firstSecond?? Descending

Where you can already feel that generic names like idHoldingAInt don't cut down on verbosity while being awkward to read.

My subjective conclusion is that the small readability benefits are not worth at the moment the work of creating this rule

[jfmengels]

I think my proposal wasn't very clear, or not precise enough. I should have added a more explicit description.

The idea of the change is only to remove the destructuring of single-record variants if the unwrapped value gets wrapped again everywhere that it is used. The example I gave is basically the only one I wanted to cover.

Taking back my original example

view (Model model) = -- Model is unwrapped into model
   div []
    [ viewA (Model model)  -- model in wrapped again
    , viewB (Model model) -- model in wrapped again
    ]
--> we can simplify it to this:
view model =
   div []
    [ viewA model
    , viewB model
    ]

If we had an alias to the argument, then we could use that name instead

view ((Model m) as model) =
   div []
    [ viewA (Model m)
    , viewB (Model m)
    ]
-->
view model =
   div []
    [ viewA model
    , viewB model
    ]

I agree that it could be interesting to do this for tuples, but as you say, that requires giving it a name. That said, maybe the name could end up being bad in the example I gave :thinking:

[lue-bird]

Tho your example could (?) end up being simplified by NoUnusedPatterns

view ((Model m) as model) =
   div []
    [ viewA (Model m)
    , viewB (Model m)
    ]
-->
view ((Model m) as _) =
   div []
    [ viewA (Model m)
    , viewB (Model m)
    ]
-->
view (Model m) =
   div []
    [ viewA (Model m)
    , viewB (Model m)
    ]

But only suggesting a fix when you have an alias makes sense. Maybe we can extend it to

view model =
  let
    (Model m) = model
  in
  div []
    [ viewA (Model m)
    , viewB (Model m)
    ]
-->
view model =
   div []
    [ viewA model
    , viewB model
    ]

Might be a little harder to implement.

What I don't yet understand: Do we want to always report an error to folks, saying they should add an alias if none is present? That doesn't really seem like a good solution

• folks have to explicitly navigate to the reported section, add the alias
• maybe (?) other rules like NoPatternAs or NoUnused.Pattern will jump in before Simplify gets it
• when they want the same name, like (Model model) as model, could it trip up some logic? At the very least this won't compile (which would go against your idea of compiler first, then elm-review)

[lue-bird]

Funny edge case: a type with phantom parameters should not be reported by this check.

E.g. this code I found in elm-review is not a no-op:

|> (\(ModuleRuleSchema moduleVisitorSchema) -> ModuleRuleSchema moduleVisitorSchema)

similar example

List.foldl
    (\addVisitors (ModuleRuleSchema moduleVisitorSchema) ->
        addVisitors (ModuleRuleSchema moduleVisitorSchema)
    )
    ...

[jfmengels]

That's right :+1: This means we have to make sure that either:

• The type does not have type variables
• The type's type variable in this context is unbound, which I think is do-able in the context of a argument function declaration

If we don't know either, then we should not simplify this. But I think this will be enough to report most cases (since most types don't have type variables anyway).