Skip to content

(Opinion) hlint should not be enabled by default #1933

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
chris-martin opened this issue Jun 17, 2021 · 18 comments · Fixed by #2458
Closed

(Opinion) hlint should not be enabled by default #1933

chris-martin opened this issue Jun 17, 2021 · 18 comments · Fixed by #2458
Labels
component: hls-hlint-plugin status: in discussion Not actionable, because discussion is still ongoing or there's no decision yet

Comments

@chris-martin
Copy link

chris-martin commented Jun 17, 2021

I'm not sure where exactly the right place to discuss this is, how there should be a consensus, or whether this has already been deliberated on before, but I'd like to toss an opinion out there: hlint is more hindrance than a help.

At present the workaround I have for myself is to add an .hlint.yaml file to every project with the following contents:

[ { "ignore": { "within": '**.*' } } ]

But my concern arises because we're about to teach a beginner workshop where, for the first time, we expect the majority of students to be following along with vscode and haskell-language-server installed. Haskell already offers a tremendous source of distractions from the small carefully-chosen set of core ideas we can try to get across in a few hours, without hlint interjecting its suggestions and bringing up things like eta reduction, tuple sections, etc. while people are still working on understanding types and basic syntax.

For myself, I also find the blue squiggles and additions to the Problems tab in vscode to be superfluous distractions from work. I don't really understand what the intended target audience is for the information that hlint provides, or why this information would need to be omnipresent while we code.

hlint itself acknowledges that...

Some of the hints are subjective [...] occasionally don't always make sense.

But the experience of using vscode and haskell-language-server is that the hints are presented in the absence of this explanation. The messages appear under the "problems" tab as read as imperative commands like "Use fewer brackets". To anyone who doesn't already know better, they appear confident and authoritative in this context.

@jneira
Copy link
Member

jneira commented Jun 17, 2021

Well, that would be a big change and i guess users using hlint will be disturbed setting it off by default.
Dont have numbers to back my arguments but from my experience in the community communication channels (including discord) i think there are:

  • beginners/intermediate users that like hlint cause it suggests widely used idioms (some of them opinionated for sure)
  • advanced users that customize hlint configuration to remove not wanted hints and even add new ones which fit their workflow and help dev teams to stick to specific conventions that escape the type system

I know there are advanced users who dislike hlint hints cause they follow them automatically or dont want to follow it. And agree they can be distracting for beginners who should focus in basic features and the type system.

Otoh you can disable hlint globally for all projects, at least in vscode, i guess it will be possible in other editors too.

//cc @ndmitchell @expipiplus1

@jneira jneira added component: hls-hlint-plugin status: in discussion Not actionable, because discussion is still ongoing or there's no decision yet labels Jun 17, 2021
@pepeiborra
Copy link
Collaborator

FWIW there is already an option to conveniently toggle the hlint diagnostics, and VSCode has strong support for managing option sets at various levels (user, workspace, etc). So you could easily include a .vscode/settings.json file in a project template to ensure that any new projects are created with hlint diagnostics off.

image

@expipiplus1
Copy link
Contributor

From my editor's point of view, this doesn't bother me as I can explicitly
enable HLint now that I've seen this issue; I do wonder how many "hlint hints
don't appear since updating" this repo will get though!

I've never tried to teach true beginners in Haskell, so am not able to speak
about HLint's negative effects there. @chris-martin's concerns do sounds very
valid!

From a selfish point of view though, the more -Wall and HLint active in the
Haskell ecosystem the better. It is annoying to clone someone else's work and
have my editor light up the project with warnings because they didn't develop
with HLint. (The fix is the small distraction of annotating modules or the
project with HLint warding charms).

For all but the very beginner Haskeller, I think HLint is a splendid tool and
well worth being enabled by default. I have no idea how to weigh the desires of
teachers and beginners against other developers though...


For myself, I also find the blue squiggles and additions to the Problems tab in vscode to be superfluous distractions from work. I don't really understand what the intended target audience is for the information that hlint provides, or why this information would need to be omnipresent while we code.

For any advanced Haskellers I suspect that turning off undesired HLint hints
for a project shouldn't be too much of a problem.

FWIW the thing I use HLint most for is automatically applying hints, I no
longer have to burden my text editing with removing extraneous parentheses,
fmap composition etc... I can just edit away knowing that HLint will clean up
after me with just a keypress.

@chris-martin
Copy link
Author

chris-martin commented Jun 17, 2021

FWIW there is already an option to conveniently toggle the hlint diagnostics [...]

image

Oh wonderful, thank you. I find extension settings to be a bit buried in the vscode UI and I often forget they're there :)

A thought -- It's been some years now since I've used an IDE in another language, but I seem to recall seeing these sorts of suggestions in IntelliJ for Java, Python, etc. If anyone knows, maybe it's worth comparing to what the out-of-the-box experience tends to look like in other ecosystems?

@wz1000
Copy link
Collaborator

wz1000 commented Jun 18, 2021

As a compromise that would fix most of my annoyances with hlint, how about not showing any hlints as long as there is any other warning or error in the project? I believe this is the way HIE used to work.

@michaelpj
Copy link
Collaborator

As a compromise that would fix most of my annoyances with hlint, how about not showing any hlints as long as there is any other warning or error in the project?

We already report them at the hint level of severity, deciding to hide (or not) certain kinds of diagnostic under certain conditions seems like a classic example of something that the client should control!

@jneira
Copy link
Member

jneira commented Jun 22, 2021

We already report them at the hint level of severity, deciding to hide (or not) certain kinds of diagnostic under certain conditions seems like a classic example of something that the client should control!

Totally agree the responsability should be in the editor (at least part of it).
Defer (hlint or whatever?) suggestions to not have error or warnings was suggested in #579.
@michaelpj do you know if the lsp spec tell us something about how to control displaying diagnostics by severity?

@michaelpj
Copy link
Collaborator

I didn't mean that the client should tell us which diagnostics to send, but rather that the client should just filter the diagnostics after receiving them, if it wants to. "Hide all hint level diagnostics if there is an error" is something the client can just do on it's own without us!

@chris-martin
Copy link
Author

Please keep in mind that my concern is not whether people who know what they want can customize the editor to do what they want; that is already the case. I would like to protect the unconfident from seeing blue squiggles when they've done nothing wrong.

@expipiplus1
Copy link
Contributor

What would the response be if an issue was opened on VSCode to ignore 'Hint' severity messages by default?

Is this actually a Haskell specific problem considering that many other language servers run linters too. (I don't mean to imply that because everyone else does it it's a good idea to show these by default).

@jneira
Copy link
Member

jneira commented Jun 24, 2021

Please keep in mind that my concern is not whether people who know what they want can customize the editor to do what they want; that is already the case. I would like to protect the unconfident from seeing blue squiggles when they've done nothing wrong.

Well, we are entering the slippery zone of user experience (so psichology) but i think the blue color tries to signal the diagnostic is a recommendation and i would not feel i am doing something "wrong" if i ignore them.
If they would be red or orange i would worry about them (well lot of devs ignore warnings by default too 😛)
That is one of the differences with other tools/languages which consider linting as warnings (or even errors) that should be "fixed".
That additional level of severity lets user be able to ignore only them and keep good errors and warnings from the compiler (or other sources), independently if recommendations comes from hlint or another plugin.

@michaelpj
Copy link
Collaborator

Is this actually a Haskell specific problem considering that many other language servers run linters too.

Exactly. This is IMO a vscode UX issue. In some ways the whole point of the client/server split with lsp servers is that the server just gives the client all the stuff and then it's up to the client to handle everything relating to presentation.


To be clear, I was only objecting to "conditionally don't emit hlint messages when there's an error". Having the hlint plugin default to off seems fine to me.

In fact, I think I'd be in favour. Expert users will know it exists and go find it. What about beginners?

  • Having it off by default means that they may not get hlint even if they would like it, and it may take some time for them to explore the options and find that they can turn it on.
  • Having it on by default is potentially confusing (I totally agree with Chris about stuff like the eta-reduction hint, tbh), and can impede learning and getting on with all the rest of Haskell.

Overall "beginners miss out on hlint oh no" seems less bad to me.

@michaelpj
Copy link
Collaborator

In fact, thinking about it: if we can get our option-discovery good enough, I'd be quite in favour of stripping down the default features quite a lot. We also provide a lot of code actions and code lenses by default, and it might be good to cut that down.

Consider e.g. the "make imports explicit" lens. I can imagine it's quite overwhelming for users to see every import line annotated with this massive code lens!

@anka-213
Copy link
Contributor

Regarding the "make imports explicit" lens: We might want to add a heuristic that when more than n identifiers are imported from a specific module, don't show the full list but instead just the number of imported identifiers or something. And maybe don't make the whole thing explicit by just clicking on it? If there are 50+ identifiers imported from a specific module, I'm not sure if making it explicit would improve readability.

I've seen a lot of beginners accidentally clicking on that code-lens.

(This is kind of off topic though)

@jneira
Copy link
Member

jneira commented Jun 25, 2021

I think one of the problems is hlint suggestions are controversial. If we could agree in a minimal set of suggestions which fits most uses, from beginners to advanced ones maybe the recommendations and code refactors will be less disturbed.
Not sure if that agreement is even possible though 🤔
But well, if is has been possible apply hlint to ghc itself, maybe it would be. 🙂
Would love to know the opinion about this discussion from @ndmitchell

@ndmitchell
Copy link
Collaborator

The problem with defaulting features to off (e.g. hlint, code lenses on imports) is that it's approximately equivalent to deleting these features. We should assume that most people won't reconfigure their editor, which means most things should be on by default (because they are useful) or deleted (because they only impact a few users and aren't worth the hassle).

I think a perfectly reasonable code action on any HLint suggestion would be "disable all hlint warnings" which would drop a suitable disable-everything .hlint.yaml, or change the config (no idea which is the better idea). That way users can very quickly disable it. Similarly, I think having a code action on the first import which is annotated saying "disable these annotations" would be good too.

I also agree that whether hints are displayed or not should be very much in the editors choice. If they want to hide hints while warnings/errors exist, that should be an editor configuration. If they want to disable hints entirely, that should also be an editor choice.

While HLint suggestions aren't universally applicable, the default set has been much reduced from the past. Most things are mostly a good idea. Not universally, but mostly.

@jneira
Copy link
Member

jneira commented Jul 13, 2021

  • The control on what and when diagnostics are shown per severity and other criteria should be more a client responsability than a server one
  • Disable the extension in the actual context would be close to remove the extension (the rest of the extensions are enabled by default and there is no a hls plugins marketplace, plugin suggestions in place or other builtin discovery mechanism as other ides)
  • We should make easier disable hints at project/module level (in addition of the existing configuration option to disable the plugin): see Add code action to suppress specific hlint warnings #600
    • We could add Disable hint in the module, Disable hint in the project and Disable all hints in the project. All of them could be implemented creating/updating a project level .hlint.yaml (to implement them evenly)

So if everyone agree i think we could close this when the last action was done (and optionally when we check editors has the required control over diagnostics)

@jneira jneira linked a pull request Dec 9, 2021 that will close this issue
@jneira
Copy link
Member

jneira commented Dec 9, 2021

#2458 will allow to disable specific hints in a module.
I think we can close this, as we will not disable the plugin by default, there are existing ways to disable it and a issue tracking how to make easier ignore hints (#600)

@mergify mergify bot closed this as completed in #2458 Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: hls-hlint-plugin status: in discussion Not actionable, because discussion is still ongoing or there's no decision yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants