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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions plugins/hls-hlint-plugin/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,9 @@
This is typically done through an [HLint configuration file](https://github.com/ndmitchell/hlint#customizing-the-hints).
You can also change the behavior of HLint by adding a list of flags to `haskell.plugin.hlint.config.flags`
if your configuration is in a non-standard location or you want to change settings globally.

## 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.


22 changes: 15 additions & 7 deletions plugins/hls-hlint-plugin/src/Ide/Plugin/Hlint.hs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ import Development.IDE.Core.Shake (getDiagnost
import qualified Refact.Apply as Refact

#ifdef HLINT_ON_GHC_LIB
import Data.List (nub)
import Development.IDE.GHC.Compat (BufSpan,
DynFlags,
WarningFlag (Opt_WarnUnrecognisedPragmas),
Expand Down Expand Up @@ -284,16 +283,26 @@ getIdeas nfp = do
Just <$> liftIO (parseModuleEx flags' fp contents')

setExtensions flags = do
hlintExts <- getExtensions flags nfp
hlintExts <- getExtensions nfp
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.

-- Previously this would concatenate extensions from both hlint's parsedFlags
-- and the ModSummary dynflags. However using the parsedFlags extensions
-- can sometimes interfere with the hlint parsing of the file.
-- See https://github.com/haskell/haskell-language-server/issues/1279
--
-- Note: this is used when HLINT_ON_GHC_LIB is not defined. We seem to need
-- these extensions to construct dynflags to parse the file again. Therefore
-- using hlint default extensions doesn't seem to be a problem when
-- HLINT_ON_GHC_LIB is not defined because we don't parse the file again.
getExtensions :: NormalizedFilePath -> Action [Extension]
getExtensions nfp = do
dflags <- getFlags
let hscExts = EnumSet.toList (extensionFlags dflags)
let hscExts' = mapMaybe (GhclibParserEx.readExtension . show) hscExts
let hlintExts = nub $ enabledExtensions pflags ++ hscExts'
let hlintExts = hscExts'
return hlintExts
where getFlags :: Action DynFlags
getFlags = do
Expand Down Expand Up @@ -538,8 +547,7 @@ applyHint ide nfp mhint =
liftIO $ withSystemTempFile (takeFileName fp) $ \temp h -> do
hClose h
writeFileUTF8NoNewLineTranslation temp oldContent
(pflags, _, _) <- runAction' $ useNoFile_ GetHlintSettings
eddiemundo marked this conversation as resolved.
Show resolved Hide resolved
exts <- runAction' $ getExtensions pflags nfp
exts <- runAction' $ getExtensions nfp
-- We have to reparse extensions to remove the invalid ones
let (enabled, disabled, _invalid) = Refact.parseExtensions $ map show exts
let refactExts = map show $ enabled ++ disabled
Expand Down
6 changes: 2 additions & 4 deletions plugins/hls-hlint-plugin/test/Main.hs
Original file line number Diff line number Diff line change
Expand Up @@ -134,8 +134,7 @@ suggestionsTests =
changeDoc doc [change']
testHlintDiagnostics doc

, knownBrokenForHlintOnGhcLib "hlint doesn't take in account cpp flag as ghc -D argument" $
testCase "[#554] hlint diagnostics works with CPP via ghc -XCPP argument" $ runHlintSession "cpp" $ do
, testCase "[#554] hlint diagnostics works with CPP via ghc -XCPP argument" $ runHlintSession "cpp" $ do
doc <- openDoc "CppCond.hs" "haskell"
testHlintDiagnostics doc

Expand Down Expand Up @@ -212,8 +211,7 @@ suggestionsTests =
length diags @?= 1
unusedExt ^. L.code @?= Just (InR "refact:Unused LANGUAGE pragma")

, knownBrokenForHlintOnGhcLib "[#1279] hlint uses a fixed set of extensions" $
testCase "hlint should not activate extensions like PatternSynonyms" $ runHlintSession "" $ do
, testCase "[#1279] hlint should not activate extensions like PatternSynonyms" $ runHlintSession "" $ do
doc <- openDoc "PatternKeyword.hs" "haskell"

waitForAllProgressDone
Expand Down