Reduce usages of Prelude/Data.Text.head #2519

jhrcek · 2021-12-21T13:42:17Z

First batch of fixes to #2514

This is going to be harder than I thought :P
There are more heads to come, but opening this just to get initial feedback on the general approach.

Also (due to my ocd) I had to fix bunch of hlint warnings which lead to meaningful simplifications.

Vague questions: how are you guys dealing with running tests locally?
I'm getting bunch of fd:28: hPutBuf: resource vanished (Broken pipe) when running cabal test locally..

jhrcek · 2021-12-21T13:46:14Z

plugins/hls-tactics-plugin/src/Wingman/Judgements.hs

@@ -34,6 +34,9 @@ buildHypothesis
  where
    go (occName -> occ, t)
      | Just ty <- t
+      -- TODO what's the intention behind this check?
+      -- can it be replace with something like `not . isSymOcc`?
+      -- https://hackage.haskell.org/package/ghc-9.2.1/docs/GHC-Plugins.html#v:isSymOcc


Probably question for @isovector
My intention is to get rid of this usage of head

This is checking that the bindings in scope were created in source Haskell. GHC makes lots of bindings whose name start with $ or other odd things.

You could replace it with maybe False isAlpha $ headMay $ occNameString occ

ghcide/src/Development/IDE/Plugin/CodeAction.hs

jneira · 2021-12-21T14:02:38Z

ghcide/src/Development/IDE/GHC/Compat/Units.hs

@@ -324,7 +325,7 @@ moduleUnit =
    Module.moduleUnitId
 #endif

-filterInplaceUnits :: [UnitId] -> [PackageFlag] -> ([UnitId], [PackageFlag])
+filterInplaceUnits :: NE.NonEmpty UnitId -> [PackageFlag] -> ([UnitId], [PackageFlag])


I would use types unqualified, hope it is the pattern for other data types

Ditto, not sure this one should change.

jneira · 2021-12-21T14:04:28Z

I'm getting bunch of fd:28: hPutBuf: resource vanished (Broken pipe) when running cabal test locally..

I got them in windows long time ago, what is your setup?

we have some issues about, in nix and windows
https://github.com/haskell/haskell-language-server/issues?q=is%3Aissue+is%3Aopen+resource+vanished+%28Broken+pipe%29

but lately I don't get them in windows 10

pepeiborra

Most test suites need TASTY_NUM_THREADS=1 to override the default behaviour that runs tests concurrently. The ghcide testsuite sets this implicitly I believe, but others don't.

If that's not enough, just copy the command line from CI

pepeiborra · 2021-12-21T13:57:18Z

ghcide/bench/lib/Experiments.hs

-        let DocumentPositions{..} = head docs
+        let DocumentPositions{..} = headNote "Experiments.runBench" docs


I don't think that the safe package is best practice anymore. It was created before we had HasCallStack but nowadays head will produce a better error message

Yes, and I think if we're going to do this we should replace it with a meaningful error, not just the position where it occurred (although that's somewhat useful).

We also could be more lenient about partial functions in things like tests and benchmarks.

We don't want to have any errors escaping out in this way in ghcide.

But this particular call site is in the benchmark suite which is allowed to error out, and a position is more helpful than any description in that case.

pepeiborra · 2021-12-21T13:58:14Z

ghcide/src/Development/IDE/Core/Shake.hs

@@ -853,18 +854,18 @@ defineNoDiagnostics op = defineEarlyCutoff $ RuleNoDiagnostics $ \k v -> (Nothin
 -- | Request a Rule result if available
 use :: IdeRule k v
    => k -> NormalizedFilePath -> Action (Maybe v)
-use key file = head <$> uses key [file]
+use key file = headNote "Development.IDE.Core.Shake.use" <$> uses key [file]


As I commented on the issue, I think you could solve all of these at a stroke by making the underlying uses functions work on NonEmpty, which I think would be fine.

Yes, I support the generalization to any Traversable to use NonEmpty here

pepeiborra · 2021-12-21T13:58:22Z

ghcide/src/Development/IDE/Core/Shake.hs


 -- | Request a Rule result, it not available return the last computed result, if any, which may be stale
 useWithStale :: IdeRule k v
    => k -> NormalizedFilePath -> Action (Maybe (v, PositionMapping))
-useWithStale key file = head <$> usesWithStale key [file]
+useWithStale key file = headNote "Development.IDE.Core.Shake.useWithStale" <$> usesWithStale key [file]


pepeiborra · 2021-12-21T13:58:29Z

ghcide/src/Development/IDE/Core/Shake.hs


 -- | Request a Rule result, it not available return the last computed result which may be stale.
 --   Errors out if none available.
 useWithStale_ :: IdeRule k v
    => k -> NormalizedFilePath -> Action (v, PositionMapping)
-useWithStale_ key file = head <$> usesWithStale_ key [file]
+useWithStale_ key file = headNote "Development.IDE.Core.Shake.useWithStale_" <$> usesWithStale_ key [file]


pepeiborra · 2021-12-21T13:58:36Z

ghcide/src/Development/IDE/Core/Shake.hs

@@ -932,7 +933,7 @@ useNoFile :: IdeRule k v => k -> Action (Maybe v)
 useNoFile key = use key emptyFilePath

 use_ :: IdeRule k v => k -> NormalizedFilePath -> Action v
-use_ key file = head <$> uses_ key [file]
+use_ key file = headNote "Development.IDE.Core.Shake.use_" <$> uses_ key [file]


pepeiborra · 2021-12-21T14:03:41Z

ghcide/src/Development/IDE/Plugin/CodeAction/ExactPrint.hs

      unless (null $ tail lies) $
-        addTrailingCommaT (head lies) -- Why we need this?
+        addTrailingCommaT (headNote "Development.IDE.Plugin.CodeAction.ExactPrint.extendHiding2" lies) -- Why we need this?


pepeiborra · 2021-12-21T14:03:49Z

ghcide/src/Development/IDE/Plugin/Completions.hs

@@ -210,7 +211,7 @@ extendImportHandler :: CommandFunction IdeState ExtendImport
 extendImportHandler ideState edit@ExtendImport {..} = do
  res <- liftIO $ runMaybeT $ extendImportHandler' ideState edit
  whenJust res $ \(nfp, wedit@WorkspaceEdit {_changes}) -> do
-    let (_, List (head -> TextEdit {_range})) = fromJust $ _changes >>= listToMaybe . toList
+    let (_, List (headNote "Development.IDE.Plugin.Completions.extendImportHandler" -> TextEdit {_range})) = fromJust $ _changes >>= listToMaybe . toList


I think this should just do nothing if the match fails. It's already inside a whenJust for the response coming in.

pepeiborra · 2021-12-21T14:04:21Z

ghcide/src/Development/IDE/Spans/Documentation.hs

@@ -63,7 +64,9 @@ lookupKind env mod =
    fmap (fromRight Nothing) . catchSrcErrors (hsc_dflags env) "span" . lookupName env mod

 getDocumentationTryGhc :: HscEnv -> Module -> Name -> IO SpanDoc
-getDocumentationTryGhc env mod n = head <$> getDocumentationsTryGhc env mod [n]
+getDocumentationTryGhc env mod n =
+    headNote "Development.IDE.Spans.Documentation.getDocumentationTryGhc"


This seems potentially kind of bad! I think we should do something here. Just return a Maybe?

I think this is similar to uses in that we are local equational reasoning suffices to prove the absence of errors. You could generalise getDocumentationsTryGhc to take Traversable and use a NonEmpty list in getDocumentationTryGhc

pepeiborra · 2021-12-21T14:05:25Z

ghcide/test/exe/Main.hs

@@ -4886,7 +4886,9 @@ projectCompletionTests =
              "import ALocal"
            ]
        compls <- getCompletions doc (Position 1 13)
-        let item = head $ filter ((== "ALocalModule") . (^. Lens.label)) compls
+        item <- case find (\c -> c ^. Lens.label == "ALocalModule") compls of
+            Nothing -> liftIO . assertFailure $ "No completion with label ALocalModule found in : " <> show compls


Do we enable -fno-ignore-assert ? I don't remember

pepeiborra · 2021-12-21T14:06:13Z

plugins/hls-tactics-plugin/src/Wingman/CaseSplit.hs

-   in fmap wildify . head . drop 5 $ iterated
+   in fmap wildify . (\xs -> atNote "Wingman.CaseSplit.iterateSplit" xs 5) $ iterated


I would just use !!

Right, this one is a bit funny because we know the list is infinite. Hmm.

Anton-Latukha · 2021-12-21T14:46:30Z

Nota bene: I addressed:

haskell-language-server/ghcide/src/Development/IDE/Spans/Documentation.hs

Lines 64 to 67 in bd0046b

    
           getDocumentationTryGhc :: HscEnv -> Module -> Name -> IO SpanDoc 
        
           getDocumentationTryGhc env mod n = head <$> getDocumentationsTryGhc env mod [n]

In #2349 - would merge it in some short order.

michaelpj

Generally looks good!

In future it would make this sort of thing easier to review if you don't also apply hlint suggestions. It's tempting, but reviewer bandwith is a bottleneck for us. You can always do a trivial "fix hlint suggestions PR" :)

Pepe, I'm interpreting your "Remove" comments as "go back to head here, because due to the implicit HasCallStack it is equally informative to headNote", is that right?

michaelpj · 2021-12-21T14:23:12Z

ghcide/bench/lib/Experiments.hs

-        let DocumentPositions{..} = head docs
+        let DocumentPositions{..} = headNote "Experiments.runBench" docs


Yes, and I think if we're going to do this we should replace it with a meaningful error, not just the position where it occurred (although that's somewhat useful).

We also could be more lenient about partial functions in things like tests and benchmarks.

michaelpj · 2021-12-21T14:24:10Z

ghcide/src/Development/IDE/Core/Shake.hs

@@ -853,18 +854,18 @@ defineNoDiagnostics op = defineEarlyCutoff $ RuleNoDiagnostics $ \k v -> (Nothin
 -- | Request a Rule result if available
 use :: IdeRule k v
    => k -> NormalizedFilePath -> Action (Maybe v)
-use key file = head <$> uses key [file]
+use key file = headNote "Development.IDE.Core.Shake.use" <$> uses key [file]


As I commented on the issue, I think you could solve all of these at a stroke by making the underlying uses functions work on NonEmpty, which I think would be fine.

michaelpj · 2021-12-21T14:26:13Z

ghcide/session-loader/Development/IDE/Session.hs

@@ -758,7 +761,7 @@ getDependencyInfo fs = Map.fromList <$> mapM do_one fs
 -- ID. Therefore we create a fake one and give them all the same unit id.
 removeInplacePackages
    :: UnitId     -- ^ fake uid to use for our internal component
-    -> [UnitId]
+    -> NonEmpty UnitId


Perhaps simpler to just convert to a normal list before calling this - I don't think this function needs to require its argument to be non-empty!

michaelpj · 2021-12-21T14:27:49Z

ghcide/src/Development/IDE/Plugin/CodeAction.hs

@@ -1553,7 +1551,7 @@ rangesForBinding' b (L l (IEThingWith _ thing _  inners labels))
 rangesForBinding' _ _ = []

 -- | 'matchRegex' combined with 'unifySpaces'
-matchRegexUnifySpaces :: T.Text -> T.Text -> Maybe [T.Text]
+matchRegexUnifySpaces :: T.Text -> T.Text -> Maybe (NonEmpty T.Text)


I get why this is the return type, but it's funny to see Maybe (NonEmpty a) instead of [a]!

It is funny, but in my experience it's actually the more convenient/"right" choice, whenever you want to do something special for the empty list case.

michaelpj · 2021-12-21T14:29:33Z

ghcide/src/Development/IDE/GHC/Compat/Units.hs

@@ -324,7 +325,7 @@ moduleUnit =
    Module.moduleUnitId
 #endif

-filterInplaceUnits :: [UnitId] -> [PackageFlag] -> ([UnitId], [PackageFlag])
+filterInplaceUnits :: NE.NonEmpty UnitId -> [PackageFlag] -> ([UnitId], [PackageFlag])


Ditto, not sure this one should change.

michaelpj · 2021-12-21T14:35:26Z

ghcide/src/Development/IDE/Spans/Documentation.hs

@@ -63,7 +64,9 @@ lookupKind env mod =
    fmap (fromRight Nothing) . catchSrcErrors (hsc_dflags env) "span" . lookupName env mod

 getDocumentationTryGhc :: HscEnv -> Module -> Name -> IO SpanDoc
-getDocumentationTryGhc env mod n = head <$> getDocumentationsTryGhc env mod [n]
+getDocumentationTryGhc env mod n =
+    headNote "Development.IDE.Spans.Documentation.getDocumentationTryGhc"


This seems potentially kind of bad! I think we should do something here. Just return a Maybe?

michaelpj · 2021-12-21T14:37:55Z

plugins/hls-floskell-plugin/src/Ide/Plugin/Floskell.hs

-      let gibiansky = head (filter (\s -> styleName s == "gibiansky") styles)
+      let gibiansky = case find (\s -> styleName s == "gibiansky") styles of
+            Just gibStyle -> gibStyle
+            Nothing -> error "Ide.Plugin.Floskell.findConfigOrDefault: Style with name 'gibiansky' not found"


Can we just not set the appStyle if this fails?

michaelpj · 2021-12-21T14:38:51Z

plugins/hls-tactics-plugin/src/Wingman/CaseSplit.hs

-   in fmap wildify . head . drop 5 $ iterated
+   in fmap wildify . (\xs -> atNote "Wingman.CaseSplit.iterateSplit" xs 5) $ iterated


Right, this one is a bit funny because we know the list is infinite. Hmm.

michaelpj · 2021-12-21T14:39:56Z

plugins/hls-tactics-plugin/src/Wingman/Machinery.hs

@@ -342,7 +342,7 @@ lookupNameInContext name = do
 getDefiningType
    :: TacticsM CType
 getDefiningType = do
-  calling_fun_name <- fst . head <$> asks ctxDefiningFuncs
+  calling_fun_name <- asks (fst . headNote "Wingman.Machinery.getDefiningType" . ctxDefiningFuncs)


Just below we do something with failure, can we also use that branch if we fail here?

michaelpj · 2021-12-21T14:42:01Z

plugins/hls-tactics-plugin/src/Wingman/Simplify.hs

-             -- amount of gas we give to auto
+  = -- Do three passes; this should be good enough for the limited
+    -- amount of gas we give to auto
+    (\xs -> atNote "Wingman.Simplify.simplify" xs 3)


Another annoying infinite list.

pepeiborra · 2021-12-21T15:03:12Z

Pepe, I'm interpreting your "Remove" comments as "go back to head here, because due to the implicit HasCallStack it is equally informative to headNote", is that right?

Correct

jhrcek · 2021-12-21T19:10:24Z

Thanks for reviews, I'll address your comments tomorrow.

jhrcek · 2021-12-22T10:56:13Z

I decided to close this PR as it contains too many unrelated changes.
I'll open multiple smaller, easier to review and hopefully less controversial PRs, addressing most of your comments and suggestions.

Reduce usages of Prelude.head

017843c

jhrcek changed the title ~~Reduce usages of Prelude.head~~ Reduce usages of Prelude/Data.Text.head Dec 21, 2021

jhrcek commented Dec 21, 2021

View reviewed changes

ghcide/src/Development/IDE/Plugin/CodeAction.hs Show resolved Hide resolved

Fix function name

9ea5898

jneira reviewed Dec 21, 2021

View reviewed changes

pepeiborra requested changes Dec 21, 2021

View reviewed changes

michaelpj approved these changes Dec 21, 2021

View reviewed changes

This was referenced Dec 22, 2021

Improve action for fixing import typo #2522

Merged

Fix some hlint warnings #2523

Merged

jhrcek closed this Dec 22, 2021

jneira mentioned this pull request Jan 25, 2022

Hlinting partial functions #2634

Merged

jhrcek deleted the jan/reduce-use-of-partial-fns-1 branch January 19, 2024 04:14

		let DocumentPositions{..} = head docs
		let DocumentPositions{..} = headNote "Experiments.runBench" docs

		in fmap wildify . head . drop 5 $ iterated
		in fmap wildify . (\xs -> atNote "Wingman.CaseSplit.iterateSplit" xs 5) $ iterated

Reduce usages of Prelude/Data.Text.head #2519

Reduce usages of Prelude/Data.Text.head #2519

Conversation

jhrcek commented Dec 21, 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 commented Dec 21, 2021 • edited Loading

pepeiborra left a 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

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

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

Anton-Latukha commented Dec 21, 2021 • edited Loading

michaelpj left a 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

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

pepeiborra commented Dec 21, 2021

jhrcek commented Dec 21, 2021

jhrcek commented Dec 22, 2021

jhrcek commented Dec 21, 2021 •

edited

Loading

jneira commented Dec 21, 2021 •

edited

Loading

Anton-Latukha commented Dec 21, 2021 •

edited

Loading