Skip to content

Are hlint's severities a bit too high in general? #1549

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

Closed
michaelpj opened this issue Dec 15, 2023 · 9 comments
Closed

Are hlint's severities a bit too high in general? #1549

michaelpj opened this issue Dec 15, 2023 · 9 comments

Comments

@michaelpj
Copy link

HLS has historically reported all hlint diagnostics at "information" level. There is a proposal to instead respect hlint's severities. This seems nice, but putting hlint's diagnostics next to GHC's makes the severities for hlint seem a bit high overall.

I don't have a clear rubric, but it seems to me roughly that GHC's view of severities is something more like:

  • Info: take it or leave it
  • Warning: you're almost certainly doing something wrong here
  • Error: so bad I can't continue

Whereas hlint's view seems more like:

  • Info: take it or leave it
  • Warning: many people would prefer the alternative
  • Error: almost certainly something wrong

... and then there are custom errors, which I think people probably want to be severe.

Having written this down, I now think that perhaps the best thing to do would be to:

  • Map hlint "error"s to "error"s (I'm tempted to say to "warning", but I think this might confuse people who have added custom error rules?)
  • Map hlint "warning" and "hint"s both to "info"

Any thoughts?

@michaelpj
Copy link
Author

Alternatively, we take hlint's severities as they are, and direct people to adjust them here when they complain 🤷

@googleson78
Copy link
Contributor

A tangent that it is somewhat unrelated, but also sounds like it could be relevant to the discussion here:

I would also like to add that having hlint's diagnostic override a lot of other things that HLS displays, and having them pop up first, is quite annoying, given that in 99.9% of the cases I would prefer to see the compiler's output.

This is especially bad for students/new learners, where I've seen an hlint diagnostic hide or make very hard to see a type error produced by the compiler, which will cause their code to not compile.

@michaelpj
Copy link
Author

That sounds like a HLS issue, maybe make an issue there? That sounds like it's a client behaviour, and it seems surprising that clients would let a non-error diagnostic appear before an error diagnostic.

Of course, if we changed to take more of hlint's diagnostics then that problem would probably get worse, since hlint "error"s would compete with GHC's errors.

@IAmPara0x
Copy link

IAmPara0x commented Dec 15, 2023

Question: Are their any hlint lint that has severity of "error" by default?

If no then the only case, HLS will show error is when someone explicitly add a "error" lint to .hlint.yaml . Then I think it's reasonable for HLS to show that diagnostic as error else it may cause a confusion like: haskell/haskell-language-server#3881?

Though I do think hlint's default suggestions/hints are too strong to be considered as warnings. If we moved all the default hints to Suggestion, then HLS will only provide warning and error when user explicitly adds it in the hlint config.

Any thoughts on this?

@michaelpj
Copy link
Author

Question: Are their any hlint lint that has severity of "error" by default?

Yes, lots, e.g. https://github.com/ndmitchell/hlint/blob/master/data/hlint.yaml#L816

If it was only user-defined ones then I think we could get away with preserving the "error" status. But tbh looking at that I'm not sure I feel great about presenting "use readTVarIO instead of atomically . readTVar" to the user as an "error".

@michaelpj
Copy link
Author

Here's a concrete (but perhaps unpleasant proposal): align hlint's interpretation of severities with GHC's. That means:

  • No error level hints in the default set at all, unless there's an incredibly good case that they're wrong
    • Users can add error level hints if they want, if they really want them to show up as errors
    • Everything now an error would become a warning
  • Consider demoting some warnings to suggestions/info

@ndmitchell
Copy link
Owner

There were 8 error level severities (basically the ones I forgot to downgrade, or got added by other people). Now there are 2, and both are a result of people doing something crazy.

So if you want to map warning to info, and leave error alone, that should work fine now (and even with 8, they were fairly uncommon warnings around STM and Lens).

@michaelpj
Copy link
Author

Okay, that sounds good. Let's see if people complain :)

@michaelpj
Copy link
Author

I'm going to close this as "basically done". I don't know if you want to write some guidance about how people should choose severities for hints somewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants