Autofix heuristics for NoUnused.Parameters

[gampleman]

Currently NoUnused.Parameters doesn't auto fix issues in named functions. The docs say:

We're only offering fixes for lambdas here because we believe unused parameters in functions are a code smell that should be refactored.

Unfortunately, neither the docs nor the error message really expand on the rationale here. So let me take a stab:

I think the idea here is that a better fix for this is not passing in the unused variable at call sites.

I'll proceed with the rest of this issue with this assumption.

Now whether this is true depends on a few situations. The first classification I think is to figure out if the callsite is a direct or indirect callsite.

A direct callsite is one where the function is called directly, typically in a fully applied fashion.

target a = 3

myFun a b =
  let 

     foo = target a

  in 
  doSomething foo b

An indirect callsite is one where the function is stored in a datastructure:

type alias Process =
   { start : Int
   , next : Int -> Int
   }

process1 : Process
process1 = 
   { start = 0, next = target }

eval : Process -> Int 
eval {start, next }
  next start

results = List.map eval [process1]

I think if we found all the callsites to be direct, then it would make sense to suggest an auto fix to remove the arguments at all of the callsites (and then other rules might cleanup those callsites, etc).

Indirect callsites complicate the picture, as we often need to evaluate all the possible arguments for each potential callsite can only really remove the argument iff all potential arguments for each potential callsite ignore that argument.

Now I'm not sure how feasible that analysis actually is. Finding direct callsites is straightforward, and I suppose it could be relatively straightforward to find clear cut cases (i.e. when a function is only ever referred to when being fully evaluated, that it is only called via direct callsites). The dificulty of finding all the indirect callsites would probably involve some flow analysis, which may well be untractable or even just impractical.

---

To summarise though, I think the most actionable idea here is to just add more auto fixes for named functions that only ever have direct callsites by eliminating the variable from both the signature and from all of the callsites.

[jfmengels]

Hey
I think this is a good idea 👍
It's a lot of additional analysis but I think it's worth having.
There's currently a limitation in elm-review that fixes are for a single file only, meaning that we wouldn't be able to apply a fix to functions that are exposed.

I think we can and should focus on direct call sites, because indirect call sites seem a lot more complex to fix. So what I'm thinking is that we find that argument n°2 of a function is unused, then we look for all the places where we find that function. And if in all those places at least 2 arguments (including <| and |>), and remove that additional parameter (and eventual pipe).

But this would be quite a bit of work, especially considering that this rule IIRC does not currently do any work of that kind.

[jfmengels]

Additional thought: if the unused parameter is for a non-exposed function, then it makes sense to report the issue when analyzing the module, and trying to remove the argument in all the other call sites.

When the function is exposed however, it could make sense to delay the report until we've analyzed the rest of the project, to see whether we can automate removing the argument from all other call sites.

That said, this last piece doesn't make much sense until we have multi-file fixes.