Skip to content
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

Fix hlint parsing file with disabled extension #2671

Merged
merged 5 commits into from
Feb 2, 2022

Conversation

eddiemundo
Copy link
Collaborator

@eddiemundo eddiemundo commented Feb 1, 2022

Seems to fix #1279

All I did was not use the extensions that hlint turns on by default, and only use extensions found via the mod summary. The mod summary seems to also include extensions listed under the extensions stanza in the cabal file.

Not fully certain this doesn't break somebody's workflow.

If someone turns on an extension in the hlint.yaml and expects hlint to always consider it on I think that will conflict with this fix because I don't know how to disambiguate between an extension that is turned on by default by hlint and an extension turned on by the user in the hlint.yaml.

@eddiemundo eddiemundo requested a review from jneira as a code owner February 1, 2022 03:00
Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

This seems sensible to me. My understanding is that the reason for listing extensions in .hlint.yaml is that hlint can't otherwise know what extensions GHC would in fact be using to compile that file. But here we really do know, which seems superior.

However, this reasoning isn't completely obvious, s ocould you add a comment? Perhaps write a doc comment for getExtensions that explains which extensions we get?

Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

looks great, thanks!

## Known Differences from the `hlint` executable

- The `hlint` executable by default turns on many extensions when parsing a file because it is not certain about the exact extensions that apply to the file (they may come from project files). This differs from HLS which uses only the extensions the file needs to parse the file. Hence it is possible for the `hlint` executable to report a parse error on a file, but the `hlint` plugin to work just fine on the same file. This does mean that the turning on/off of extensions in the hlint config may be ignored by the `hlint` plugin.
- Hlint restrictions do not work (yet). This [PR](https://github.com/ndmitchell/hlint/pull/1340) should enable that functionality, but this requires a newer version of hlint to be used in HLS.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should maybe be in a "known limitations" section on the features page too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I'll add that.

debugm $ "hlint:getIdeas:setExtensions:" ++ show hlintExts
return $ flags { enabledExtensions = hlintExts }

getExtensions :: ParseFlags -> NormalizedFilePath -> Action [Extension]
getExtensions pflags nfp = do
-- Gets extensions from ModSummary dynflags for the file.
Copy link
Collaborator

Choose a reason for hiding this comment

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

great!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Argh, there's a somewhat important typo. It's supposed to be, "this is used when HLINT_ON_GHC_LIB is defined" instead of "is not". Gonna add it to the features.md known limitations PR.

@jneira jneira added the merge me Label to trigger pull request merge label Feb 2, 2022
@mergify mergify bot merged commit 7f00abb into haskell:master Feb 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HLint fails to parse code that is invalid in disabled extensions
4 participants