ndmitchell / hlint

Haskell source code suggestions
Other
1.47k stars 195 forks source link

Warn on mulitline parenthesized expression #1198

Open michaelpj opened 3 years ago

michaelpj commented 3 years ago

I wanted to write a (possibly overly strict) linting rule for a multline parenthesized expression, i.e. something like

foldl (\acc x -> 
   let something = ...
   in ...) seed xs

The idea is that you probably want to pull this out to a variable, or use $ if possible.

I'm not sure if it's possible to write a rule for this with hlint's rule engine, since the "multi-line" part is really key, and I'm not sure how you'd express that.

googleson78 commented 3 years ago

You have source span info available in hlint, along with parens info, so it should be possible, I think? As long as ghc sets a "proper" source span on parens (i.e. from where the left parens is to where the right parens is)

ndmitchell commented 3 years ago

You can't write this with the matching engine. It should be possible to extend the matching engine with a multiline predicate which tests if the start/end spans aren't on the same line. I'm not totally sure if HLint will see through too many brackets automatically to subvert a bracket-based approach, but might work.

However, as a programmer, I'd really hate it if someone said "you can't put this on more than one line", and about half the time I'd remove the line breaks making the code worse, rather than doing any other refactor. Is this really a good idea? I appreciate it's often not the right thing, but is the solution worse than the problem?

michaelpj commented 3 years ago

I'm not sure if this is a good idea, which is one of the reasons I'd like to try it out and see what it flags! I don't think it would be much more annoying than the "redundant parentheses" hint, which I also often ignore.

ndmitchell commented 3 years ago

Happy to take a patch adding isMultiline as a side condition - probably needs to go about https://github.com/ndmitchell/hlint/blob/master/src/Hint/Match.hs#L209. Whether the bracket stuff fights back is unclear, but I assume if it does a few other tweaks might be sufficient to get around it. Like you could match x as a pure wildcard, demand isParen on it and isMultiline. That should be enough to see if it happens much or has value,

FWIW, I find the redundant parens hint very useful - I consider brackets quite noisy, so eliminating those that are obviously unnecessary for their context helps me read the remaining code. But it's all a matter of personal taste.

zliu41 commented 1 year ago

Mulitline parenthesized expression can be very readable, as long as it is formatted well. E.g., I find the following perfectly readable:

foldr
  (\a acc -> 
   ...
   ...
   ...
  )
  z
  xs

And it is often irritating to have to find a name for the function.

googleson78 commented 1 year ago

While I do agree with @zliu41's sentiment, I also know that there are people who are firmly in the "if it's more than one line put it in a let/where" camp, so I think it wouldn't hurt to have something like this available.