Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New rule: No unused tuple values #16

Open
jfmengels opened this issue Sep 25, 2020 · 2 comments
Open

New rule: No unused tuple values #16

jfmengels opened this issue Sep 25, 2020 · 2 comments
Labels
hacktoberfest help wanted Extra attention is needed

Comments

@jfmengels
Copy link
Owner

jfmengels commented Sep 25, 2020

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:

  • A good name. NoUnused.TupleValues?
  • Someone to implement it (I can provide help and guidance)
@jfmengels jfmengels added enhancement New feature or request help wanted Extra attention is needed hacktoberfest and removed enhancement New feature or request labels Sep 25, 2020
@Arkham
Copy link

Arkham commented Oct 12, 2020

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
Copy link
Owner Author

jfmengels commented Oct 12, 2020

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants