Skip to content
This repository has been archived by the owner on Jan 2, 2021. It is now read-only.

Remove Strict from the language extensions used for code actions #638

Merged
merged 1 commit into from
Jun 17, 2020

Conversation

tek
Copy link
Contributor

@tek tek commented Jun 13, 2020

This code action uses an unreliable heuristic, i.e. substring matching
for all existing language extensions.
The "quickfix" LSP action chooses an arbitrary code action from the
list, presumably the first in the list.

As an example, the redundant import

import Data.Map.Strict

will trigger the suggestion to add the language extension "Strict".
Moving this suggestion to the end of the list reduces the likelihood
for false positives.


Since selection of the quickfix action happens outside of ghcide, there's probably a better solution.
Any suggestions? A better workaround could be to check for the language extension's name's presence in the offending code.

I added a test but it fails and I don't see the definitions of getCodeActions and executeCodeAction. Some advice would be appreciated!

@pepeiborra
Copy link
Collaborator

I agree that the suggest extension logic is a bit flaky, it's the way it is until GHC gives us structured error messages (there are already 2 GHC proposals under way).

Lowering the priority will help the 20% of cases where the suggestion is wrong, but unhelp the other 80% of cases where the suggestion is right.

Based on my experience that Strict is the extension that triggers this most commonly (one of the few extensions that has a single word name?) I would prefer a fix that removes Strict from the list of ghcExtensions.

Those functions come from the lsp-test package

@tek
Copy link
Contributor Author

tek commented Jun 14, 2020

I agree that the suggest extension logic is a bit flaky, it's the way it is until GHC gives us structured error messages (there are already 2 GHC proposals under way).

awesome!

Lowering the priority will help the 20% of cases where the suggestion is wrong, but unhelp the other 80% of cases where the suggestion is right.

Based on my experience that Strict is the extension that triggers this most commonly (one of the few extensions that has a single word name?) I would prefer a fix that removes Strict from the list of ghcExtensions.

Sounds good to me!

Those functions come from the lsp-test package

thanks!

@tek tek force-pushed the deprioritize-suggest-extension branch from 39aea52 to ab229a5 Compare June 14, 2020 13:15
@tek tek changed the title [RFC] Deprioritize the "suggest language extension" code action Remove Strict from the language extensions used for code actions Jun 14, 2020
@tek
Copy link
Contributor Author

tek commented Jun 14, 2020

@pepeiborra you fine with it like this?

Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@tek
Copy link
Contributor Author

tek commented Jun 14, 2020

awesome! 🚀

ghcExtensions = Map.fromList . map ( ( T.pack . flagSpecName ) &&& flagSpecFlag ) $ xFlags
ghcExtensions = Map.fromList . filter notStrictFlag . map ( ( T.pack . flagSpecName ) &&& flagSpecFlag ) $ xFlags
where
notStrictFlag (name, _) = name /= "Strict"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add a link to the issue you initially raised here. It's one odd and unexpected exception, so having the backstory is always useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ndmitchell done!
there's no issue though, only this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - this PR is fine.

Since the code action for language extension suggestions uses substring
matching, the presence of the literal name of an extension can trigger
a false positive.

`Strict` is an identifier that occurs frequently in imports, causing
the extension to be suggested rather than the removal of a redundant
import.
@tek tek force-pushed the deprioritize-suggest-extension branch from ab229a5 to bb4abdd Compare June 14, 2020 16:40
Copy link
Collaborator

@cocreature cocreature left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks a lot!

@cocreature cocreature merged commit 0d806c3 into haskell:master Jun 17, 2020
@tek tek deleted the deprioritize-suggest-extension branch June 17, 2020 12:50
pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
…askell/ghcide#638)

Since the code action for language extension suggestions uses substring
matching, the presence of the literal name of an extension can trigger
a false positive.

`Strict` is an identifier that occurs frequently in imports, causing
the extension to be suggested rather than the removal of a redundant
import.
pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
…askell/ghcide#638)

Since the code action for language extension suggestions uses substring
matching, the presence of the literal name of an extension can trigger
a false positive.

`Strict` is an identifier that occurs frequently in imports, causing
the extension to be suggested rather than the removal of a redundant
import.
pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
…askell/ghcide#638)

Since the code action for language extension suggestions uses substring
matching, the presence of the literal name of an extension can trigger
a false positive.

`Strict` is an identifier that occurs frequently in imports, causing
the extension to be suggested rather than the removal of a redundant
import.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants