Apply some hlint suggestions, silence some others. #1227

peterwicksstringfield · 2021-01-18T19:16:24Z

These suggested changes come from running hlint on the entire code base, using the configuration at ghcide/.hlint.yaml

~/haskell-language-server$ hlint --report --with-group=extra --hint=ghcide/.hlint.yaml --ignore-glob='test/testdata/**' --ignore-glob='test/wrapper/testdata/**' --ignore-glob='ghcide/test/data/**' --ignore-glob='plugins/hls-eval-plugin/test/testdata/**' .

If we plan to expand hlint coverage across the code base we will need to apply or silence all of these at some point.

cf. #693

plugins/hls-eval-plugin/src/Ide/Plugin/Eval/Code.hs

peterwicksstringfield · 2021-01-18T19:21:42Z

plugins/hls-eval-plugin/src/Ide/Plugin/Eval/Parse/Token.hs

@@ -145,6 +145,8 @@ tokens = concatMap (\(l, vs) -> map (Located l) vs) . zip [0 ..] . reverse . snd
      Right (st', tokens') -> (st', tokens' : tokens)
      Left err -> error $ unwords ["Tokens.next failed to parse", ln, err]

+{- HLINT ignore tokens "Use zipFrom" -}
+
 -- | Parse a line of input
 aline :: State -> TParser
 aline InCode          = optionStart <|> multi <|> singleOpen <|> codeLine


Line 142.

I don't think this is an improvement but we can do whatever.

Found: zip [0 .. ] Perhaps: zipFrom 0```

peterwicksstringfield · 2021-01-18T19:23:08Z

plugins/hls-splice-plugin/hls-splice-plugin.cabal

@@ -18,6 +18,7 @@ library
  build-depends:    aeson
                  , base       >=4.12 && <5
                  , containers
+                  , extra
                  , foldl
                  , haskell-lsp
                  , hls-plugin-api


Alternatively: don't incur a dependency just to satisfy hlint's obsession with eitherM. But the rest of hls already depends on extra so why not?

HLint only suggests extra functions if you pass the appropriate option, so if you don't want to use more option, then don't pass that.

peterwicksstringfield · 2021-01-18T19:24:43Z

plugins/tactics/src/Ide/Plugin/Tactic.hs

-    False -> pure []
+  if xopt ext dflags
+    then tp dflags plId uri range jdg
+    else pure []


The use of "case ... True ... False" is consistent and obviously deliberate in the tactics plugin. Should we just ignore the "use if then else" suggestion for this module and leave the code alone?

(I don't care either way, I basically made an arbitrary decision just to get the PR started.)

wz1000 · 2021-01-18T19:26:09Z

Please don't change anything in hie-compat, we want the source to be as close to the original GHC version as possible.

peterwicksstringfield · 2021-01-18T19:26:16Z

plugins/tactics/src/Ide/Plugin/Tactic/Context.hs

@@ -33,6 +36,9 @@ mkContext locals tcg = Context
  }


Right now this function is one operation per line. The suggested changes would combine some of the lines, which seems worse.

plugins/tactics/src/Ide/Plugin/Tactic/Context.hs:(29,22)-(31,30): Suggestion: Use <=< Found: (getFunBindId =<<) . fmap unLoc . bagToList Perhaps: (getFunBindId Control.Monad.<=< (fmap unLoc . bagToList))```

Maybe

((getFunBindId . unLoc) <=< bagToList)

is better?
I'm not super inclined to this though...

I could move these:

{- HLINT ignore mkContext "Redundant fmap" -} {- HLINT ignore mkContext "Use <=<" -}

to the .hlint.yaml?

I think Redundant fmap rule may be useful for some cases... but for <=<, I agree that would be better.

I moved the fish recommendation ignore to the hlint.yaml and left the fmap recommendation in the module.

plugins/tactics/test/AutoTupleSpec.hs

test/utils/Test/Hls/Util.hs

peterwicksstringfield · 2021-01-20T03:36:47Z

The failing build is this thing:

...During interactive linking, GHCi couldn't find the following symbol:\n--   interactive_Ghci1_asPrint_closure\n-- This may be due to you not asking GHCi to load extra object files,\n-- archives or DLLs needed by your current session.  Restart GHCi, specifying\n-- the missing library ...

Ailrun · 2021-01-20T03:54:16Z

@peterwicksstringfield It's eval plugin's non-deterministic bug, and it may be gone after a few trials.

Ailrun · 2021-02-15T04:20:53Z

@peterwicksstringfield Could you rebase this? Now the eval subtlety has (presumably) fixed, and it will be better to merge (or close) this PR before sending a style formatter PR.

peterwicksstringfield · 2021-02-15T15:40:13Z

Ailrun: happy too. Wasn't sure what the state of the this PR was and wasn't sure how to ask.

Will push in an hour or so.

Ailrun · 2021-02-15T22:15:40Z

plugins/hls-eval-plugin/src/Ide/Plugin/Eval/Types.hs

@@ -55,6 +55,8 @@ type Loc = Located Line

 type Line = Int

+{- HLINT ignore locate "Use zipWithFrom" -}
+


If we do not use the rule for code like this, then will we ever use this rule?
If we won't, I think it's better to put this in the hlint setting.

You're right. I'll just apply this hint too.

Ailrun · 2021-02-15T22:19:40Z

plugins/hls-tactics-plugin/src/Ide/Plugin/Tactic/Judgements.hs

@@ -89,6 +89,8 @@ withNewGoal :: a -> Judgement' a -> Judgement' a
 withNewGoal t = field @"_jGoal" .~ t


+{- HLINT ignore introducing "Use zipFrom" -}
+


Same for here

Ailrun · 2021-02-15T22:20:22Z

plugins/hls-tactics-plugin/src/Ide/Plugin/Tactic/Judgements.hs

@@ -218,6 +218,8 @@ filterSameTypeFromOtherPositions dcon pos jdg =
   in disallowing Shadowed (M.keys to_remove) jdg


+{- HLINT ignore getAncestry "Replace case with maybe" -}
+


Can I ask why this is disabled?

I don't remember what I was thinking. The version with maybe looks fine to me now. Fixed.

Ailrun · 2021-02-17T19:14:54Z

stack.yaml

@@ -1,4 +1,4 @@
-resolver: lts-14.27 # Last 8.6.5
+resolver: nightly-2020-12-09


I wonder why did you update this?

Merge error. I very carefully checked to make sure that that wasn't included before I pushed. Apparently I checked wrong.

Ailrun

LGTM

peterwicksstringfield · 2021-02-17T21:45:45Z

Thanks for the help. :)

Ailrun · 2021-02-18T06:29:25Z

Could you rebase this PR?

peterwicksstringfield · 2021-02-19T02:21:30Z

Meanwhile, on the other side of the code base:
529e4dd
"... + {- HLINT ignore "Use zipFrom" -} ..."

Rebased. Dropped use of zipFrom and zipFromWith. Moved the zipFrom and zipFromWith ignores to .hlint.yaml. Dropped all changes to hls-tactics-plugin/**.

jneira · 2021-02-19T06:28:20Z

Lets try merge this asap to avoid more conflicts

jhrcek · 2021-02-19T08:15:02Z

FYI this commit seems to have broken the install script.
Doing stack ./install.hs hls-8.8.4 at the root of this repo works with previous commit, but not after this was merged.

There's no clear error message:

$ stack ./install.hs hls-8.8.4
--  While building package hls-install-0.8.0.0 (scroll up to its section to see the error) using:
      /home/jhrcek/.stack/setup-exe-cache/x86_64-linux-tinfo6/Cabal-simple_mPHDZzAJ_2.4.0.1_ghc-8.6.5 --builddir=.stack-work/dist/x86_64-linux-tinfo6/Cabal-2.4.0.1 build lib:hls-install --ghc-options " -fdiagnostics-color=always"
    Process exited with code: ExitFailure 1

jneira · 2021-02-19T10:01:18Z

install/src/Env.hs

@@ -54,7 +54,7 @@ findInstalledGhcs = do
    -- sort by version to make it coherent with getHlsVersions
    $ sortBy (comparing fst)
    -- nub by version. knownGhcs takes precedence.
-    $ nubBy ((==) `on` fst)
+    $ nubOrdBy (compare `on` fst)


Maybe this change has broken the install script??? It shouldn't but is the unique direct change in that component.
@jhrcek please, could you try the install reverting this change manually?

If the change broke it, hlint suggestion was very wrong 🤔

@jneira Just tried with latest master and I had to revert both this change (nubOrdBy back to nubBy) AND one more change done in install/src/Print.hs below. After reverting those two changes it started working again.

I don't know if there's some kind of implicit import going on or something ,but those functions come from extra and I don't see them imported in either of the 2 modules.

No implicit imports afaik, fortunately 🙂
The fix would be add those imports probably

@jhrcek Thanks for reporting it, would you like to open a pr adding the appropiate imports? i'll do myself in other case

Sure, I'll do it later today.

I guess this part of the code isn't tested by the CI then? Sorry about that.

@jneira thanks for the merge!
@jhrcek thanks for fixing it!

(I did not except this PR to be so fiddly and hard to merge.)

yeah, the install script is not being tested, but it does not change frequently so 🤷

peterwicksstringfield commented Jan 18, 2021

View reviewed changes

plugins/hls-eval-plugin/src/Ide/Plugin/Eval/Code.hs Outdated Show resolved Hide resolved

peterwicksstringfield commented Jan 18, 2021

View reviewed changes

plugins/tactics/test/AutoTupleSpec.hs Outdated Show resolved Hide resolved

peterwicksstringfield commented Jan 18, 2021

View reviewed changes

test/utils/Test/Hls/Util.hs Outdated Show resolved Hide resolved

peterwicksstringfield force-pushed the harder_hlint_fixes branch 2 times, most recently from 50fc3af to 1e2fd7a Compare January 18, 2021 22:28

jneira added the pr: needs rebase label Jan 19, 2021

peterwicksstringfield marked this pull request as ready for review January 24, 2021 20:00

jneira removed the pr: needs rebase label Jan 25, 2021

jneira added the pr: needs rebase label Feb 10, 2021

peterwicksstringfield force-pushed the harder_hlint_fixes branch 2 times, most recently from 1872dc4 to 6576637 Compare February 15, 2021 20:50

Ailrun reviewed Feb 15, 2021

View reviewed changes

Ailrun removed the pr: needs rebase label Feb 16, 2021

Ailrun reviewed Feb 17, 2021

View reviewed changes

Ailrun approved these changes Feb 17, 2021

View reviewed changes

Ailrun added merge me Label to trigger pull request merge pr: needs rebase labels Feb 18, 2021

peterwicksstringfield force-pushed the harder_hlint_fixes branch from c28e00a to 8877b7c Compare February 19, 2021 02:21

Hlint hints.

00fd613

peterwicksstringfield force-pushed the harder_hlint_fixes branch from 8877b7c to 00fd613 Compare February 19, 2021 04:04

Merge branch 'master' into harder_hlint_fixes

a95d81b

jneira removed the pr: needs rebase label Feb 19, 2021

mergify bot merged commit 2bc6310 into haskell:master Feb 19, 2021

jneira reviewed Feb 19, 2021

View reviewed changes

jhrcek mentioned this pull request Feb 19, 2021

Fix install script after hlint fixes #1400

Merged

peterwicksstringfield deleted the harder_hlint_fixes branch February 20, 2021 01:55

		@@ -55,6 +55,8 @@ type Loc = Located Line

		type Line = Int

		{- HLINT ignore locate "Use zipWithFrom" -}

		@@ -89,6 +89,8 @@ withNewGoal :: a -> Judgement' a -> Judgement' a
		withNewGoal t = field @"_jGoal" .~ t


		{- HLINT ignore introducing "Use zipFrom" -}

		@@ -218,6 +218,8 @@ filterSameTypeFromOtherPositions dcon pos jdg =
		in disallowing Shadowed (M.keys to_remove) jdg


		{- HLINT ignore getAncestry "Replace case with maybe" -}

		@@ -1,4 +1,4 @@
		resolver: lts-14.27 # Last 8.6.5
		resolver: nightly-2020-12-09

Apply some hlint suggestions, silence some others. #1227

Apply some hlint suggestions, silence some others. #1227

Conversation

peterwicksstringfield commented Jan 18, 2021 • edited Loading

peterwicksstringfield Jan 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterwicksstringfield Jan 18, 2021 • edited Loading

Choose a reason for hiding this comment

wz1000 commented Jan 18, 2021

peterwicksstringfield Jan 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ailrun Feb 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterwicksstringfield commented Jan 20, 2021 • edited Loading

Ailrun commented Jan 20, 2021

Ailrun commented Feb 15, 2021

peterwicksstringfield commented Feb 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ailrun left a comment

Choose a reason for hiding this comment

peterwicksstringfield commented Feb 17, 2021

Ailrun commented Feb 18, 2021

peterwicksstringfield commented Feb 19, 2021

jneira commented Feb 19, 2021

jhrcek commented Feb 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jneira Feb 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterwicksstringfield commented Jan 18, 2021 •

edited

Loading

peterwicksstringfield Jan 18, 2021 •

edited

Loading

peterwicksstringfield Jan 18, 2021 •

edited

Loading

peterwicksstringfield Jan 18, 2021 •

edited

Loading

Ailrun Feb 16, 2021 •

edited

Loading

peterwicksstringfield commented Jan 20, 2021 •

edited

Loading

jhrcek commented Feb 19, 2021 •

edited

Loading

jneira Feb 19, 2021 •

edited

Loading