Skip to content

Commit

Permalink
#433, make hints spanning multiple modules/declarations
Browse files Browse the repository at this point in the history
  • Loading branch information
ndmitchell committed Feb 7, 2018
1 parent 1af1723 commit 7bb10df
Show file tree
Hide file tree
Showing 4 changed files with 19 additions and 17 deletions.
1 change: 1 addition & 0 deletions CHANGES.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
Changelog for HLint

#433, make hints spanning multiple modules/declarations
#439, add more fixities for new base operators
#437, --json output should be finite
#425, avoid misparsing use of Gtk2Hs `on` function
Expand Down
12 changes: 6 additions & 6 deletions src/Apply.hs
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,12 @@ applyHints cs = applyHintsReal $ map SettingClassify cs
applyHintsReal :: [Setting] -> Hint -> [(Module_, [Comment])] -> [Idea]
applyHintsReal settings hints_ ms = concat $
[ map (classify $ cls ++ mapMaybe readPragma (universeBi m)) $
order "" (hintModule hints settings nm m) `merge`
concat [order (fromNamed d) $ decHints d | d <- moduleDecls m] `merge`
concat [order "" $ hintComment hints settings c | c <- cs]
order [] (hintModule hints settings nm m) `merge`
concat [order [fromNamed d] $ decHints d | d <- moduleDecls m] `merge`
concat [order [] $ hintComment hints settings c | c <- cs]
| (nm,(m,cs)) <- mns
, let decHints = hintDecl hints settings nm m -- partially apply
, let order n = map (\i -> i{ideaModule=moduleName m, ideaDecl=n}) . sortBy (comparing ideaSpan)
, let order n = map (\i -> i{ideaModule=[moduleName m], ideaDecl=n}) . sortBy (comparing ideaSpan)
, let merge = mergeBy (comparing ideaSpan)] ++
[map (classify cls) (hintModules hints settings $ map (second fst) mns)]
where
Expand Down Expand Up @@ -88,6 +88,6 @@ classify xs i = let s = foldl' (f i) (ideaSeverity i) xs in s `seq` i{ideaSever
where
-- figure out if we need to change the severity
f :: Idea -> Severity -> Classify -> Severity
f i r c | classifyHint c ~= ideaHint i && classifyModule c ~= ideaModule i && classifyDecl c ~= ideaDecl i = classifySeverity c
f i r c | classifyHint c ~= [ideaHint i] && classifyModule c ~= ideaModule i && classifyDecl c ~= ideaDecl i = classifySeverity c
| otherwise = r
x ~= y = null x || x == y
x ~= y = null x || x `elem` y
11 changes: 6 additions & 5 deletions src/Idea.hs
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ import Prelude

-- | An idea suggest by a 'Hint'.
data Idea = Idea
{ideaModule :: String -- ^ The module the idea applies to, may be @\"\"@ if the module cannot be determined or is a result of cross-module hints.
,ideaDecl :: String -- ^ The declaration the idea applies to, typically the function name, but may be a type name.
{ideaModule :: [String] -- ^ The modules the idea is for, usually a singleton.
,ideaDecl :: [String] -- ^ The declarations the idea is for, usually a singleton, typically the function name, but may be a type name.
,ideaSeverity :: Severity -- ^ The severity of the idea, e.g. 'Warning'.
,ideaHint :: String -- ^ The name of the hint that generated the idea, e.g. @\"Use reverse\"@.
,ideaSpan :: SrcSpan -- ^ The source code the idea relates to.
Expand All @@ -40,7 +40,8 @@ data Idea = Idea
-- 2) I want to control the format so it's slightly human readable as well
showIdeaJson :: Idea -> String
showIdeaJson idea@Idea{ideaSpan=srcSpan@SrcSpan{..}, ..} = dict
,("decl", str ideaDecl)
[("module", list $ map str ideaModule)
,("decl", list $ map str ideaDecl)
,("severity", str $ show ideaSeverity)
,("hint", str ideaHint)
,("file", str srcSpanFilename)
Expand Down Expand Up @@ -86,8 +87,8 @@ showEx tt Idea{..} = unlines $
where xs = lines $ tt x


rawIdea = Idea "" ""
rawIdeaN a b c d e f = Idea "" "" a b c d e f []
rawIdea = Idea [] []
rawIdeaN a b c d e f = Idea [] [] a b c d e f []

idea severity hint from to = rawIdea severity hint (srcInfoSpan $ ann from) (f from) (Just $ f to) []
where f = trimStart . prettyPrint
Expand Down
12 changes: 6 additions & 6 deletions tests/json.test
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ FILE tests/json-one.hs
foo = (+1)
bar x = foo x
OUTPUT
[{"module":"Main","decl":"bar","severity":"Warning","hint":"Eta reduce","file":"tests/json-one.hs","startLine":2,"startColumn":1,"endLine":2,"endColumn":14,"from":"bar x = foo x","to":"bar = foo","note":[],"refactorings":"[]"}]
[{"module":["Main"],"decl":["bar"],"severity":"Warning","hint":"Eta reduce","file":"tests/json-one.hs","startLine":2,"startColumn":1,"endLine":2,"endColumn":14,"from":"bar x = foo x","to":"bar = foo","note":[],"refactorings":"[]"}]

---------------------------------------------------------------------
RUN tests/json-two.hs --json
Expand All @@ -20,21 +20,21 @@ foo = (+1)
bar x = foo x
baz = getLine >>= return . map toUpper
OUTPUT
[{"module":"Main","decl":"bar","severity":"Warning","hint":"Eta reduce","file":"tests/json-two.hs","startLine":2,"startColumn":1,"endLine":2,"endColumn":14,"from":"bar x = foo x","to":"bar = foo","note":[],"refactorings":"[]"}
,{"module":"Main","decl":"baz","severity":"Suggestion","hint":"Use <$>","file":"tests/json-two.hs","startLine":3,"startColumn":7,"endLine":3,"endColumn":39,"from":"getLine >>= return . map toUpper","to":"map toUpper <$> getLine","note":[],"refactorings":"[Replace {rtype = Expr, pos = SrcSpan {startLine = 3, startCol = 7, endLine = 3, endCol = 39}, subts = [(\"f\",SrcSpan {startLine = 3, startCol = 28, endLine = 3, endCol = 39}),(\"m\",SrcSpan {startLine = 3, startCol = 7, endLine = 3, endCol = 14})], orig = \"f <$> m\"}]"}]
[{"module":["Main"],"decl":["bar"],"severity":"Warning","hint":"Eta reduce","file":"tests/json-two.hs","startLine":2,"startColumn":1,"endLine":2,"endColumn":14,"from":"bar x = foo x","to":"bar = foo","note":[],"refactorings":"[]"}
,{"module":["Main"],"decl":["baz"],"severity":"Suggestion","hint":"Use <$>","file":"tests/json-two.hs","startLine":3,"startColumn":7,"endLine":3,"endColumn":39,"from":"getLine >>= return . map toUpper","to":"map toUpper <$> getLine","note":[],"refactorings":"[Replace {rtype = Expr, pos = SrcSpan {startLine = 3, startCol = 7, endLine = 3, endCol = 39}, subts = [(\"f\",SrcSpan {startLine = 3, startCol = 28, endLine = 3, endCol = 39}),(\"m\",SrcSpan {startLine = 3, startCol = 7, endLine = 3, endCol = 14})], orig = \"f <$> m\"}]"}]

---------------------------------------------------------------------
RUN tests/json-parse-error.hs --json
FILE tests/json-parse-error.hs
©
OUTPUT
[{"module":"","decl":"","severity":"Error","hint":"Parse error","file":"tests/json-parse-error.hs","startLine":1,"startColumn":1,"endLine":1,"endColumn":1,"from":" \u00a9\n \n> \n","to":null,"note":[],"refactorings":"[]"}]
[{"module":[],"decl":[],"severity":"Error","hint":"Parse error","file":"tests/json-parse-error.hs","startLine":1,"startColumn":1,"endLine":1,"endColumn":1,"from":" \u00a9\n \n> \n","to":null,"note":[],"refactorings":"[]"}]

---------------------------------------------------------------------
RUN tests/json-note.hs --json
FILE tests/json-note.hs
foo = any (a ==)
bar = foldl (&&) True
OUTPUT
[{"module":"Main","decl":"foo","severity":"Warning","hint":"Use elem","file":"tests/json-note.hs","startLine":1,"startColumn":7,"endLine":1,"endColumn":17,"from":"any (a ==)","to":"elem a","note":["requires a valid `Eq` instance for `a`"],"refactorings":"[Replace {rtype = Expr, pos = SrcSpan {startLine = 1, startCol = 7, endLine = 1, endCol = 17}, subts = [(\"a\",SrcSpan {startLine = 1, startCol = 12, endLine = 1, endCol = 13})], orig = \"elem a\"}]"}
,{"module":"Main","decl":"bar","severity":"Warning","hint":"Use and","file":"tests/json-note.hs","startLine":2,"startColumn":7,"endLine":2,"endColumn":22,"from":"foldl (&&) True","to":"and","note":["increases laziness"],"refactorings":"[Replace {rtype = Expr, pos = SrcSpan {startLine = 2, startCol = 7, endLine = 2, endCol = 22}, subts = [], orig = \"and\"}]"}]
[{"module":["Main"],"decl":["foo"],"severity":"Warning","hint":"Use elem","file":"tests/json-note.hs","startLine":1,"startColumn":7,"endLine":1,"endColumn":17,"from":"any (a ==)","to":"elem a","note":["requires a valid `Eq` instance for `a`"],"refactorings":"[Replace {rtype = Expr, pos = SrcSpan {startLine = 1, startCol = 7, endLine = 1, endCol = 17}, subts = [(\"a\",SrcSpan {startLine = 1, startCol = 12, endLine = 1, endCol = 13})], orig = \"elem a\"}]"}
,{"module":["Main"],"decl":["bar"],"severity":"Warning","hint":"Use and","file":"tests/json-note.hs","startLine":2,"startColumn":7,"endLine":2,"endColumn":22,"from":"foldl (&&) True","to":"and","note":["increases laziness"],"refactorings":"[Replace {rtype = Expr, pos = SrcSpan {startLine = 2, startCol = 7, endLine = 2, endCol = 22}, subts = [], orig = \"and\"}]"}]

24 comments on commit 7bb10df

@rikvdkleij
Copy link

Choose a reason for hiding this comment

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

@ndmitchell this is a breaking API change. The Intellij Haskell plugin uses the json API to display hlint suggestions and to apply them. This change broke the Hlint integration because it expects not a list for the module value.

@rikvdkleij
Copy link

Choose a reason for hiding this comment

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

@ndmitchell sorry, I see, it is indicated as breaking API change....

@ndmitchell
Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks for the ping @rikvdkleij - had I realised it would break a real user I might not have bothered, or at least made it gracefully fall back in the case when there was only a single point. Is it worth doing that now? Given Intellij is so tightly bound to precise details is it worth restricting to a hard 0.1 upper bound? I don't often break the API, so you usually get ~30 releases between 0.1 marks. Let me know how I can make it easier, or at least less surprising.

@rikvdkleij
Copy link

Choose a reason for hiding this comment

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

@ndmitchell Thanks for your helpful reply!

Is it worth doing that now?

No, it does not really help because it will still raise an error when more than one point.

Given Intellij is so tightly bound to precise details is it worth restricting to a hard 0.1 upper bound?

That would mean I have set upper bound for each Stackage LTS version or GHC version.
Currently the plugin builds the Hlint version of the Stackage LTS version which is used by the project to be sure that Hlint always is built without errors.
So yeah, I think setting an upper bound is the best solution to prevent this kind of error. And when I create new release of plugin I check if there is a new Hlint version.

@ndmitchell
Copy link
Owner Author

Choose a reason for hiding this comment

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

Currently the plugin builds the Hlint version of the Stackage LTS version which is used by the project to be sure that Hlint always is built without errors.

So you mean if I am developing a project with Stackage 11.1, then HLint will come from 11.1 too? There's no need to do that, you should be able to pick an HLint from whatever Stackage IntelliJ is used to, and then you could upgrade HLint only when you rerelease IntelliJ. That would guarantee everything always matched.

@rikvdkleij
Copy link

Choose a reason for hiding this comment

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

So you mean if I am developing a project with Stackage 11.1, then HLint will come from 11.1 too?

Yes.

There's no need to do that, you should be able to pick an HLint from whatever Stackage IntelliJ is used to, and then you could upgrade HLint only when you rerelease IntelliJ. That would guarantee everything always matched.

But how do I know that that specific HLint version is built for every Stackage version (and so also GHC version)?

@ndmitchell
Copy link
Owner Author

Choose a reason for hiding this comment

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

But how do I know that that specific HLint version is built for every Stackage version (and so also GHC version)?

Not quite sure what you mean. You can take an HLint, any HLint version, and run it on any code/GHC. HLint does not require to be compatible with your project, or with your GHC version - it doesn't use the GHC API. Even if you code won't compile, HLint can still run on it. It's just a parser and some guesses.

@rikvdkleij
Copy link

Choose a reason for hiding this comment

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

You can take an HLint, any HLint version, and run it on any code/GHC. HLint does not require to be compatible with your project, or with your GHC version - it doesn't use the GHC API. Even if you code won't compile, HLint can still run on it. It's just a parser and some guesses.

Okay, I understand but in the past I had some compatibility problems with building specific version of HLint against any Stackage LTS version. But I will take a look again. I guess I have to help the Stack build to set the right dependencies if that HLint version is depending on library versions which are not in that Stackage LTS version.

@ndmitchell
Copy link
Owner Author

Choose a reason for hiding this comment

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

I test HLint with every Stackage version, and HLint itself is in Stackage, so it should be robust - please report any issues you find.

@rikvdkleij
Copy link

Choose a reason for hiding this comment

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

I tried to get HLint 2.1.1 build with Stackage LTS version 6.35 (GHC 7.10.3) but after finding the right dependencies and building happy 1.19.5, I got this compilation error in HLint:

 [47 of 52] Compiling Test.Annotations ( src/Test/Annotations.hs, .stack-work/dist/x86_64-osx/Cabal-1.22.5.0/build/Test/Annotations.o )

    /private/var/folders/fg/3071_gkd5p92vmb9884sxtt00000gn/T/stack11871/hlint-2.1.1/src/Test/Annotations.hs:55:51:
        Couldn't match expected type ‘Either
                                        l0 (Either SomeException [Idea] -> [Idea])’
                    with actual type ‘[t0]’
        In the first argument of ‘fromRight’, namely ‘[]’
        In the expression: fromRight [] ideas
        In a stmt of a list comprehension:
          i@(Idea {..}) <- fromRight [] ideas

Build command:
stack build hlint-2.1.1 aeson-1.3.0.0 haskell-src-exts-1.20.2 haskell-src-exts-util-0.2.2 semigroups-0.18.4 th-abstraction-0.2.6.0 text-1.2.3.0 transformers-compat-0.5.1.4 transformers-0.5.5.0

@ndmitchell
Copy link
Owner Author

Choose a reason for hiding this comment

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

I meant:

stack build hlint --resolver=lts-6.35

That should work indefinitely, and I just tested it now.

@rikvdkleij
Copy link

Choose a reason for hiding this comment

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

But that is building HLint version 1.9.35. That is how IntelliJ plugin currently works.

@ndmitchell
Copy link
Owner Author

Choose a reason for hiding this comment

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

I am confused... I thought you built with whatever stack the user project was? Whereas I'm suggesting you pick a single resolver and only upgrade HLint when you make a new release of IntelliJ.

@rikvdkleij
Copy link

Choose a reason for hiding this comment

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

Maybe some context. An IntelliJ user expects that everything works out-of-the-box :-) He has an Haskell Stack project with in his stack.yaml resolver 6.35. IntellIJ wants to build HLint and does not want to install HLint globally because maybe user has also projects of other LTS or GHC versions. So IntelliJ is using LTS/GHC version of project and builds it for that project. This version of HLint is only "visible" for that project.

Also, if I would pick a resolver, it could happen that plugin has to download and install another GHC version "somewhere".

@rikvdkleij
Copy link

Choose a reason for hiding this comment

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

Maybe there is no other choice then let the user install HLint (globally). But then plugin has to "force" a specific version range of HLint.

@ndmitchell
Copy link
Owner Author

Choose a reason for hiding this comment

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

Working out of the box seems good.

An alternative would be that IntelliJ creates its own project, using its own resolver, and installs hlint to its own IntelliJ specific place. E.g. IntelliJ 3.2 ships with knowledge of LTS-6.35, so IntelliJ does stack build hlint --resolver=LTS-6.35 --stack-root=IntelliJ-3.2, to put HLint in a local-to-IntelliJ-3.2 directory, so no global install, but IntelliJ has strongly locked down the HLint it gets to use.

@rikvdkleij
Copy link

Choose a reason for hiding this comment

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

because maybe user has also projects of other LTS or GHC versions.

Okay, this not the case for HLint but still I do not want to change which binaries are on the user his PATH.

@ndmitchell
Copy link
Owner Author

Choose a reason for hiding this comment

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

Okay, this not the case for HLint but still I do not want to change which binaries are on the user his PATH.

Doing a stack build won't change what is on the users path - stack builds are sealed - no global changes.

@rikvdkleij
Copy link

Choose a reason for hiding this comment

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

Doing a stack build won't change what is on the users path - stack builds are sealed - no global changes.

Yes, exactly :-)

@rikvdkleij
Copy link

Choose a reason for hiding this comment

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

An alternative would be that IntelliJ creates its own project, using its own resolver, and installs hlint to its own IntelliJ specific place. E.g. IntelliJ 3.2 ships with knowledge of LTS-6.35, so IntelliJ does stack build hlint --resolver=LTS-6.35 --stack-root=IntelliJ-3.2, to put HLint in a local-to-IntelliJ-3.2 directory, so no global install, but IntelliJ has strongly locked down the HLint it gets to use.

That sounds good. Could only happen that IntelliJ will trigger a GHC install but that is not really a big problem if you have good Internet connection.

@rikvdkleij
Copy link

Choose a reason for hiding this comment

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

@ndmitchell Many thanks for thinking along!!

Maybe I will do the same for external dependencies as stylish-haskell and hindent. Intero not because that one is really GHC specific.

@ndmitchell
Copy link
Owner Author

Choose a reason for hiding this comment

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

Makes a lot of sense, and indeed making Intero the exception. Thanks for maintaining and improving IntelliJ - I know it's a lot of work, but it is valuable! Do shout in future if there is something that HLint can make easier.

@rikvdkleij
Copy link

Choose a reason for hiding this comment

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

Thanks for maintaining and improving IntelliJ - I know it's a lot of work,

Thanks! Yes, it is 😄

Other question. Can I do the same for the Hoogle (executable) as for HLint? According to the dependencies it should be but just to be sure.

@ndmitchell
Copy link
Owner Author

Choose a reason for hiding this comment

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

Yep, Hoogle will work exactly the same way.

Please sign in to comment.