`NoUnused.Parameters`: don't flag named patterns

[lue-bird]

Unit patterns are ignored

testUnit : () -> Bool
testUnit =
    \() -> True

but named patterns are flagged and replaced with _

type Tag
    = Tag

testTag : Tag -> Bool
testTag =
    \Tag -> True

or

type Tagged a
    = Tagged a

testTagged : Tagged a -> Bool
testTagged =
    \(Tagged _) -> True

Named patterns are flagged if all inner patterns are _.

This seems to be a deliberate choice. However,

1. named patterns like these are no different from unit patterns.
2. often, spelling out a Tag can help to understand the code better:

   Result.mapError
   (\NoGoatFound ->
       ...
   )

   Providing the option to ignore named patterns in this rule could be one solution.

[jfmengels]

Hey :wave:

This is interesting. I agree that it is not necessary to remove these. I know there are some rules that only check whether a pattern is equal to _ to know whether it can be simplified/ignored, but I can't remember which ones, and therefore have trouble figuring out what the side-effects of this would be (not that we couldn't fix it in those rules later, but we might have trouble detecting them).

@sparksp any opinions on this?

[jfmengels]

Do we agree that named patterns should likely still be reported for named functions?

I get the feeling that it's more likely for a lambda to have an extra parameter because the function that uses it requires a certain shape. But for named functions, the shape of the function is a bit less important, and therefore having a (Tagged _) remain there for no reason is likely something that should be simplified.

[jfmengels]

Hmm, it becomes a bit tricky when encountering things like the following in named functions.

foo (Singular _, Pair _ _) thing =
    thing

Should we report Singular _ and Pair _ _, or should we keep them as is? Maybe stylistically it is fine to keep them, but I believe that in this case we should let the user know the argument is not used.

I think we have multiple choices:

• A. Report named patterns anyway (contrary to the original request)

• B. Keep the named pattern but only in some cases, such as keeping it if it's the one at the root of the argument (parentheses not included)

  • foo (A _) = x - Don't report
  • foo (A _, B _) = x - Report and suggest (not autofix) to turn both patterns by _, which will turn them into ( _, _ ) and in a later fix into _
  • foo (A (B _)) = x - :man_shrugging:

• C. Don't report named patterns, but if the entire parameter does not introduce any variables AND is not _ (or (_)) AND does not have any otherwise simplifiable patterns, then report it.

  • foo (A _, B (C _) (D x) ) = x - Don't report because there's a variable introduced in there somewhere
  • foo (A _, B (C _) (D _) ) = x - Report because no variables were introduced, meaning the parameter is entirely unused, and the signature of the function should probably be simplified.

@sparksp @lue-bird What do you think?

[lue-bird]

Do we agree that named patterns should likely still be reported for named functions?

Good point! Yes, using them in named functions seems useless. However...

let
    handleError NoGoatFound =
        ...
in
Result.mapError handleError

isn't really that different from the lambda. The argument is nothing more than a named unit.

it becomes a bit tricky when encountering things like the following in named functions

I don't really see a difference between A, A _, A B, ( A, B ) or A ( B, C ). At least they are all useful in certain contexts:

let
    handleError (AnimalMissing Goat) =
        ...
in
Result.mapError handleError

Maybe we do what I suggested

Providing the option to ignore named patterns in this rule

but like this:

|> NoUnused.Parameters.keepNamedPatterns
    [ "AnimalMissing", "Goat" ]

to not overcomplicate things.

• Stupid named patterns like

  (Singular _, Pair _ _)

  will still be reported.

• AnimalMissing and Goat won't be reported even in named functions

I'd for example want Toop.TX _ _ ... values reported but not Goat.

[jfmengels]

I don't believe that's the right way to go about it, because I don't think it especially makes sense to ignore this by virtue of what type is destructured, ignoring by location or context would make a lot more sense. What I mean is that if you like this style of writing code, then you'll likely add all your types in that configuration, and if you don't, you'll put an empty list.

[sparksp]

In the particular case of AnimalMissing Goat as a function argument, this must mean that Goat and AnimalMissing a are the only variants of their types; with the proposed change to ignore single variant types both would be allowed.

Do you have any other concrete examples to consider?

[lue-bird]

My exact use-case is this: The fix for the elm-review rule (NoSinglePatternCase) can be configured what kind of fix to propose in given situations (the original is similar but not the same):

type FixByDestructuringIn
    = FixByDestructuringInExistingLets
    | FixByDestructuringTheArgument
        { argumentAlsoUsedElsewhere :
            OnlyFixInSeparateLetLeftFixOr
                DestructureUsingAs
        , notDestructable :
            OnlyFixInSeparateLetLeftFixOr
                DestructureInExistingLets
        }

type DestructureUsingAs = DestructureUsingAs
type DestructureInExistingLets = DestructureInExistingLets

if we have an error:

notDestructable
    |> onlyCreatingSeparateLetLeftFixOr
        (\DestructureInExistingLets ->
            fixInExistingLets
        )

argumentAlsoUsedElsewhere
    |> onlyCreatingSeparateLetLeftFixOr
        (\DestructureUsingAs ->
            destructureAsFix varInCaseOf
        )

onlyCreatingSeparateLetLeftFixOr :
    (fix -> List Fix)
    -> OnlyFixInSeparateLetLeftFixOr fix
    -> List Fix

[jfmengels]

I'm sorry @lue-bird, but I don't follow the use-case or what you want to do with it. Can you explain what kind of code you have before the rule, the code you have after and why it bothers you that the named pattern disappears?

[sparksp]

I think that allowing/ignoring single variant types will fix this for @lue-bird. That will stop DestructureUsingAs and DestructureInExistingLets from getting reported.

[jfmengels]

So is the suggestion to have an option to the rule to allow or not allow this:

foo =
    \(Singular _) ->
        x

and to, regardless of the chosen option, to always report the following?

foo =
    \(Singular _, Pair _ _) ->
        x

which would then be simplified to \_ -> x.

---

I'm still having trouble understanding the rationale for the request. I don't understand the examples you gave @lue-bird (is it implementation of your rule, or related to what the rule will look at?). So if you don't mind, let's go back to the drawing board.

The reason people use () as an argument is usually for indicating the lack of any data or for having computations be lazy. The reason we don't report () (and I encourage it) is so that people reading the code don't have to wonder what the _ could potentially stand for.

About things like type Tag = Tag. If it's used as a phantom type, we won't have a value to destructure. If it's used as a real value, then I have to admit I'm having trouble seeing the use of such a type. If the type is meant to grow later (type Tag = Tag | Tag2), then I do some see the value of not reporting it, because foo = \Tag -> x will then cause a compiler error whereas foo = \_ -> x, and I do see value in having that reminder. But if the the type is supposed to stay as it is, then to me the API design is lacking and should be improved.

For type Tag = Tag Value, the type really holds a value. if we see foo = \(Tag _) -> x. then to me this is a sign that foo should be simplified and the argument removed. If the argument is there to fit an API (view : { takeFoo : a -> Int } -> Html msg ; view { takeFoo = foo }, then I'd say it's fair to ignore the argument by changing it to _. If again, the purpose is to let the type grow, then maybe is should stay in a case expression? (Though I know at least Simplify will remove the case expression).

So the only reasonable case I see for not removing a named pattern is for keeping compiler reminders. And that is a fine reason.

Alright, let's say we agreed that we don't want to report named patterns for that reason. Then when we see

foo =
    \(Singular _, Pair _ _) ->
        x

then we shouldn't report this, because any simplification to this will remove compiler reminders.

Similarly, this shouldn't be reported

foo =
    \(Foo Bar) ->
        x

because if we simplify it, then we lose some of the reminders.

And actually, for that reason we also shouldn't change named functions and the following should not be reported.

foo Foo =
  x

The downside of not reporting the above is that it will require more attention from the user to notice that they should remove the entire parameter when the last variable from the pattern is removed and the entire parameter lost its purpose.

I'll have to mull it over, but I am willing to go towards this solution. Let me know if you disagree with something I said, but then I could really use some detailed explanation of your rationale or needs :pray:

[jfmengels]

I have disabled reporting named patterns entirely in 1.1.16. I'll close this issue, but if you have any comments, feel free to continue the discussion or open a new issue.