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

New rule: No unused tuple values #16

Open jfmengels opened 4 years ago

jfmengels commented 4 years ago

What the rule should do:

Report unused values in tuples

What problems does it solve:

It will help remove and uncover more unused code.

Example of things the rule would report:

The second value in the tuple is never used, so we might as well remove it.

let
    a : ( Int, Int )
    a = ( 1, 2 )
    --       ^ reported here

    first = Tuple.first a

    ( otherFirst, _ ) = a
in
first + otherFirst

Same for 3-tuples. Also type aliases to such tuples should also be checked.

Example of things the rule would not report:

let
    a : ( Int, Int )
    a = ( 1, 2 )

    first = Tuple.first a

    ( _, second ) = a
in
first + second

I am looking for:

Arkham commented 4 years ago

I'd be up for trying to implement this! My plan is to:

I've got two questions:

  1. anything else you'd recommend looking at?
  2. what about autofixing for this rule? I couldn't think of a way to do it cleanly..
jfmengels commented 4 years ago

Awesome!

I would recommend starting as small as possible. Try to report something, then to remove all false positives raised by it. Then try to report some more and to remove false positives again, and so on until you're out of ideas or when it becomes too difficult. In practice, we could integrate and publish this with even the smallest amount of reports that brings value and doesn't have false positives.

I think the easiest would be look at whether elements that are explicitly tuple values have one member that is never being accessed. If you lose the trail of how a tuple is being used because the code is becoming too complex, don't report it. False positives are a bane for users - because it hinders them from writing code they want/need to - but false negatives not so much, especially for such a rule.

I would recommend against copying the tests from NoUnused.Patterns. Take inspiration but no more. Start with a couple tests that you think are feasible and focus on those. elm-review rules are perfect for a TDD approach. I recommend reading about the Review.Test module and the advice it gives.

I would not worry about autofixing this rule. If you do want to do that, I think you can only do it cleanly for simple cases. A few that I could see:

let
  -- usage of a literal tuple
  ( a, b ) = ( 1, 2 )
in
b
-->
let
  b = 2
in
b

Unsure about this one, but this is something that you could do.

let
  ( a, b ) = someValue
in
b
-->
let
  -- Wrap with parens just in case. elm-format will remove it when possible
  b = Tuple.second (someValue)
in
b

I don't see a clean way of doing it for more complex things, like the return type of a function. It's fine to let the user handle some of the issues. It's better to let the user handle fixing the problem rather than the tool resolve it in a partial, bad or questionable way.

I hope this helps. Good luck and have fun!