hlint plugin

[alanz]

Use the same approach as for DAML, which re-uses the parsed AST.

From IRC:

<cocreature> alanz: our dlint integration takes the AST instead of reparsing the module which is a bit more efficient, see https://github.com/digital-asset/daml/blob/master/compiler/damlc/daml-ide-core/src/Development/IDE/Core/Rules/Daml.hs#L1081

[alanz]

@cocreature @ndmitchell

I am trying the DAML approach for hls, but getting a mismatch on ghc-lib equivalents for the GHC types. Does this approach require the ghc-lib flag to be activated on ghcide? And what are the implications of doing that? Can hlint be run on straight GHC modules?

[ndmitchell]

HLint only supports one major version of GHC at a time, namely 8.8 at the moment. For every other version it uses ghc-lib-8.8. For DAML there is only one version of GHC and it's not a problem. My guess is that for hls we'll have to use the AST when you are on 8.8, and on older versions, try passing equivalent flags to the parse bit of hlint itself. I'm not sure if there are any negative interactions of putting ghc and ghc-lib in the same process - @cocreature and @shayne-fletcher-da will know.

[shayne-fletcher]

In general using ghc and ghc-lib in the same process we expect to work. Note though, that usage of ghc-lib in that context will require to use package import syntax. HLint does not use package import syntax; it calls onto either ghc or ghc-lib depending on the compiler it's built with.

[alanz]

HLint does not use package import syntax; it calls onto either ghc or ghc-lib depending on the compiler it's built with.

So perhaps the hlint plugin for hls should be in a sublibrary, with the appropriate conditionals in the cabal definition to achieve the same effect. Would that work?

[shayne-fletcher]

That sounds eminently plausible.

Here's how it's done in hlint.cabal:

• Define a flag

flag ghc-lib
  default: False
  manual: True
  description: Force dependency on ghc-lib-parser even if GHC API in the ghc package is supported

• Then in the library stanza

    if !flag(ghc-lib) && impl(ghc >= 8.8.0) && impl(ghc < 8.9.0)
        build-depends:
          ghc == 8.8.*,
          ghc-boot-th,
          ghc-boot
    else
        build-depends:
          ghc-lib-parser == 8.8.*

[jneira]

I am taking a look to the draft pr, trying to apply the changes related with ghc-lib but, after setting in scope last versions of hlint and ghc-lib i am getting this error:

src\Ide\Plugin\Hlint.hs:110:11: error:
    • Couldn't match type ‘ghc-8.8.3:GHC.ParsedModule’
                     with ‘ParsedModule’
      NB: ‘ParsedModule’
            is defined in ‘GHC’ in package ‘ghc-lib-8.10.1.20200523’
          ‘ghc-8.8.3:GHC.ParsedModule’
            is defined in ‘GHC’ in package ‘ghc-8.8.3’
        arising from a use of ‘use_’
    • In a stmt of a 'do' block: pm <- use_ GetParsedModule file
      In the expression:
        do pm <- use_ GetParsedModule file
           let anns = pm_annotations pm
           let modu = pm_parsed_source pm
           (classify, hint) <- useNoFile_ GetHlintSettings
           ....
      In the second argument of ‘($)’, namely
        ‘\ GetHlintDiagnostics file
           -> do pm <- use_ GetParsedModule file
                 let ...
                 ....’
    |
110 |     pm <- use_ GetParsedModule file
    |           ^^^^^^^^^^^^^^^^^^^^^^^^^

GetParsedModule is defined in ghcide, where a fixed version of ghc is being used (afaik) so it does not mix well with the hlint use of ghc-lib
So, the options i can see (corrections welcome) could be:

• Not using GetParsedModule and lose the ghcide rule (not good)
• Use a suitable hlint version for each ghc fixed version
  • >= 3.0 for ghc-8.10
  • >=2.x for ghc-8.8
  • ? for ghc-8.6

[ndmitchell]

Integrating old HLint is a guaranteed way to get old bugs cropping up, which is lots of support burden.

Therefore, I'd recommend the approach of using createModuleEx taking the GHC AST on ghc-8.10, and otherwise using parseModuleEx taking the input string. Better to share the AST, and likely to reduce the potential bugs (as things like the extensions will be precisely known), but otherwise it degrades reasonably.

If you want to go with only one code path (which isn't unreasonable) I'd always do parseModuleEx.

[jneira]

@ndmitchell thanks, wise notes, as usual

[jneira]

A new draft pr based on #43: #166

[alanz]

@jneira what is the status of this?

[jneira]

I am trying to add the apply-refact part, but i think i`ll have to change the diagnostics one: see #166

[ndmitchell]

A suggestion: it would be cool if in addition to fixing the hints, there was an option to ignore that specific hint for this module, by inserting a {- HLINT ignore -} directive.