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 record fields #15

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

New rule: No unused record fields #15

jfmengels opened this issue Sep 25, 2020 · 0 comments
Labels
enhancement New feature or request hacktoberfest help wanted Extra attention is needed

Comments

@jfmengels
Copy link
Owner

jfmengels commented Sep 25, 2020

What the rule should do:

Report unused fields in records, records defined in type aliases and in records that are arguments to custom types constructors.

What problems does it solve:

It will help remove and uncover more unused code.

Example of things the rule would report:

In the following example, the unused field is not used and should be reported.

someOperation : { x : Int, unused : Int } -> Int
--           reported here ^^^^^^
someOperation record =
  record.x

The advice would be to use { a | x : Int } or to remove the unused field.

In the following example, the unused field is not used in any places where we declare Record to be used

type alias Record = { w : Int, x : Int, y : Int, z : Int, unused : Int }
--                                          reported here ^^^^^^
someOperation : Record -> Int
someOperation ({x} as record) = 
  let
    {y} = record
  in
  .w record + x + y + record.z

The same result should appear if the record was wrapped in a custom type constructor.

type Record = Record { x : Int, y : Int, z : Int, unused : Int }
--                                  reported here ^^^^^^
someOperation : Record -> Int
someOperation ((Record {x}) as fullRecord) = 
  let
    (Record record) = fullRecord
    {y} = record
  in
  x + y + record.z

Example of things the rule would not report:

  • When all fields are used.
  • Type aliases exposed as part of the package
  • Records in custom type constructors exposed as part of the package
  • Fields in records used as

Notes:

I think this can be quite a tricky rule to implement, and it will likely require many, many iterations to get rid of all false positives and false negatives. If we have to choose between one, we should choose false negatives, as that will avoid putting unnecessary annoyances on the user (especially since we can't disable rules locally).

I imagine we could potentially have a more aggressive version of the rule, under a different name or with a configuration flag, that is more aggressive. That would allow people to do some more manual removals when they fancy to, and allow us to experiment and improve the rule.

Things start to get tricky when you pass in records to other functions, because you will have to dig more into whether something is used or not.

I am looking for:

  • A good name. I'm hesitating between NoUnused.Fields and NoUnused.RecordFields. I prefer the former, but I wonder if the additional explicitness in the latter makes it any clearer.
  • Someone to implement it (I can provide help and guidance)

I think the first step for writing this rule would be handling the first example, and then seeing what false positives come out of that and fix them. We can deal with record aliases and ones in custom types later.

I don't know how things will work when type annotations are missing, so let's skip the annotated values for the initial phase.

@jfmengels jfmengels added enhancement New feature or request help wanted Extra attention is needed hacktoberfest labels Sep 25, 2020
jfmengels added a commit that referenced this issue Mar 25, 2021
Fixes #15
Avoids a false positive between imported types and imported
custom type constructors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request hacktoberfest help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant