jfmengels / elm-review-unused

Provides elm-review rules to detect unused elements in your Elm project
https://package.elm-lang.org/packages/jfmengels/elm-review-unused/latest/
BSD 3-Clause "New" or "Revised" License
23 stars 12 forks source link

"NoUnused.CustomTypeConstructorArgs" is giving a false positive on "Argument is never extracted and therefore never used." #68

Open wolfadex opened 2 years ago

wolfadex commented 2 years ago

Describe the bug The rule incorrectly says that the second String is unused despite it being used in an equality check. This originally came from a fork of https://github.com/jxxcarlson/auth-starter-lamdera/blob/master/src/Credentials.elm, where the second String is being generated and is used as part of an authentication system.

NoUnused.CustomTypeConstructorArgs: Argument is never extracted and therefore
never used.

14| type Creds
15|     = V1 String String
                    ^^^^^^

This argument is never used. You should either use it somewhere, or remove it at
the location I pointed at

SSCCE (Short Self-Contained Correct Example) Steps to reproduce the behavior:

  1. Copy https://ellie-app.com/j78yjmQrHr2a1 into a project with elm-review setup
  2. Make sure elm-review uses jfmengels/elm-review-unused and the config
    config : List Rule
    config =
    [ NoUnused.CustomTypeConstructorArgs.rule
    ]
  3. Run npx elm-review
  4. See false positive

Expected behavior This shouldn't be reported as unused.

Additional context Add any other context about the problem here.

jfmengels commented 2 years ago

Hey @wolfadex

This is a known false positive, as written in the rule's documentation. You're not the first on to come across this, so while it's not common, it does happen from time to time.

There are two complementary things that we could do:

  1. Write in the rule's report that this could be a false negative, and indicate what the user could change their code to to help the linter know that the value is used, mainly doing explicit destructuring and value comparisons. Since this could be quite a large explanation, we could probably put it in the rule's documentation, and have a warning with a link in the error message.

  2. Have the rule try and detect whether the value is used in a comparison. This is going to be quite complex, because that's a more complex analysis that we haven't really done with elm-review so far (but that could be useful for other things). Unfortunately, that will likely still not cover everything if we use libraries like assoc-list as elm-review is currently not capable of of looking at the code in dependencies. Therefore, I think that 1. would still be useful.