Loading settings differs between library and executable

[tfausak]

trafficstars

First, some pointers. I discovered this issue while investigating a bug against Splint, a GHC plugin that uses HLint: https://github.com/tfausak/splint/issues/9. I think this issue is related to both #143 and #98.

The shortest way to state this problem is that I think calling autoSettings (or argsSettings) behaves differently than calling hlint. In particular any arguments specified in the configuration file seem to be ignored. For example, consider the following Haskell module:

-- Main.hs
import Text.RawString.QQ -- from raw-strings-qq
main = print [r||]

Clearly it needs the QuasiQuotes language extension in order to compile. No problem, you can add that to your HLint configuration. To wit:

# .hlint.yaml
- arguments: [ -XQuasiQuotes ]

And indeed everything works fine that way. You can run hlint through your shell, or you can run Language.Haskell.HLint.hlint and it will succeed.

The problem comes when you try to split apart reading the settings from linting the file. I'm not terribly familiar with HLint's internals, but it felt right to call autoSettings followed by parseModuleEx to roughly recreate HLint's command-line interface. Unfortunately that does not work:

>>> (parseFlags, _, _) <- autoSettings
>>> result <- parseModuleEx parseFlags "Main.hs" Nothing
>>> :sprint result
result = Left (ParseError _ _ _)

You can see that it fails because it tries to parse the module without quasi-quotes enabled. Explicitly enabling quasi-quotes works fine:

>>> (parseFlags, _, _) <- argsSettings [ "-XQuasiQuotes" ]
>>> result <- parseModuleEx parseFlags "Main.hs" Nothing
>>> :sprint result
result = Right (hlint-3.1.6:GHC.All.ModuleEx _ (_,_))

Clearly one solution to this problem is to simply call hlint and avoid loading the settings separately. For my use case that's not ideal, since loading the settings takes a small but appreciable amount of time. I'd prefer to load the settings once, and then lint a bunch of files with those settings.

So ultimately my question is this: Is it possible to load HLint's settings properly using its exposed API? If not, could autoSettings (really argsSettings) be made to work the same as hlint? Thanks!

[ndmitchell]

In the time before, HLint had a HLint.hs file which configured things. And that was meant to be the sum-total of all configuration, and it explicitly imported the configuration in the HLint directory. It seems that in the transition to the new way of doing things (a user .hlint.yaml plus a default one supplied by HLint) the behaviour has gone a bit wrong. I see both bugs and design issues here, so we can definitely clean it up, but it's not totally obvious what to fix to do so.

The autoSettings is defined as:

autoSettings = do
    (fixities, classify, hints) <- findSettings (readSettingsFile Nothing) Nothing
    pure (parseFlagsAddFixities fixities defaultParseFlags, classify, hints)

Looking at the functions involved:

• readSettingsFile still talks about HLint.* files ending in .hs. Those won't work. So the documentation and implementation of this function is wrong. It also talks about strings, when the settings file had absolutely better be a .yaml file FilePath.
• findSettings should probably "find" the built in hlint.yaml, plus the user one, but it seems to only grab one. That seems wrong.
• The rest of the code seems reasonable.

So I think we need a simplified readSettingsFile, a defaultHLintSettings (to find the one in .hlint.yaml) and a userHLintSettings (to find the one relative to the users path). Does that seem plausible?

[tfausak]

Ah, that explains the HLint.hs references in the documentation! I was wondering.

Your analysis makes sense to me, and your proposed solution seems plausible.

The only other thing I have to add is that the hlint executable eventually uses readAllSettings to, uh, read all the settings:

https://github.com/ndmitchell/hlint/blob/283a97ddbb1231e4d508a57bd738610c85d5d28a/src/HLint.hs#L126

[tgiannak]

Given that the changes to address this properly seem to be on the larger side, would it be reasonable to implement a mitigation in the meantime?

For example, modifying splitSettings to preserve the Restrict hints? E.g.,

--- a/src/Language/Haskell/HLint.hs
+++ b/src/Language/Haskell/HLint.hs
@@ -49,6 +49,7 @@ import Data.Maybe
 import System.FilePath
 import Data.Functor
 import Prelude
+import qualified Hint.Restrict as Restrict


 -- | Get the Cabal configured data directory of HLint.
@@ -116,7 +117,9 @@ splitSettings :: [Setting] -> ([FixityInfo], [Classify], Hint)
 splitSettings xs =
     ([x | Infix x <- xs]
     ,[x | SettingClassify x <- xs]
-    ,H.resolveHints $ [Right x | SettingMatchExp x <- xs] ++ map Left enumerate)
+    ,H.resolveHints ([Right x | SettingMatchExp x <- xs] ++ map Left enumerate)
+     <> mempty { hintModule = Restrict.restrictHint . (xs++)}
+    )

[ndmitchell]

@tgiannak - that seems a clear improvement, so happy to take a patch with that change for now.

[tgiannak]

@ndmitchell I opened https://github.com/ndmitchell/hlint/pull/1340 with the change, but it looks like you need to approve the GitHub workflows to run.

[ndmitchell]

Thanks @tgiannak - hopefully this improves things in the meantime.

[tgiannak]

Would it help to include the patch for branch-3.2 as well? I think that haskell-language-server is still being built against 3.2 for GHC 8.10.

[ndmitchell]

I guess thats up to Haskell Language server folks. By and large, the fewer releases to branch-3.2 I have to do, the happier I am. I would strongly encourage projects to drop old versions, rather than inflict pain upon all dependencies for users on ancient versions. But if there is a specific request, I'd probably make such a release.

[tgiannak]

I'll see if I can figure out how to improve the version bounds issues keeping it on 3.2 first, then. Thanks!

[ndmitchell]

I believe the last patch I did for the 3.2 branch was to help with an aeson compatibility issue, but due to other unrelated issues it turned out not to be useful. @jneira will know the details.