Reduce use of partial functions from HLS

[michaelpj]

HLS has quite a lot of use of partial functions. I'm sure many of these are benign, but we do occasionally get users reporting crashes with such (usually totally useless) errors.

Gradually reducing these is an easy, helpful thing for someone to do. Good new contributor issue!

[pepeiborra]

Could you share a few examples?

[michaelpj]

30-odd usages of head, 15 or so foldl1s, 10 or so !!s.

I think many of these are fine, but therefore usually trivial to avoid. It would be nice to have a blanket "no partial functions" rule.

[michaelpj]

30-ish fromJust...

[jhrcek]

I volunteer to work on this, as I wanted to contribute to hls in more substantial way.
Having looked at some of the partial functions usages, many of them require some domain-specific knowledge, e.g. about the Shake build stuff etc. For example I'm not at all sure about these:

haskell-language-server/ghcide/src/Development/IDE/Core/Shake.hs

Lines 854 to 867 in 070f16f

	use :: IdeRule k v
	=> k -> NormalizedFilePath -> Action (Maybe v)
	use key file = head <$> uses key [file]
	-- | 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]
	-- | 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]

Can we prove these will never fail? What should the functions do in case of empty lisis? Should they maybe accept non-empty lists as inputs (as in https://www.parsonsmatt.org/2017/10/11/type_safety_back_and_forth.html)?

My preliminary plan is this:

1. Come up with a list of partial functions that we'd like to eventually replace with something better.
   Apart from the above mentioned head, foldl1, (!!), fromJust i noticed quite a few Map.!, IntMap.!, Array.!, last
   Can you think of others? Let's keep expanding the list in the description of this issue!
2. replace the obvious ones
3. when it's impossible to "prove" that given partial function will always succeeds, replace it with some variant that gives more informative exception (e.g. headNote from safe). This way we at least get more informative errors when one of these fails and we'll know from user reports which ones actually occur and should be fixed. Wdyt?

[jneira]

The plan sounds really great to me, given at minimum we will have more info about the error.
Many thanks for willing to contribute

[michaelpj]

The plan sounds good to me too. A 4th bonus step would be to promote the .hlint.yaml that we have for ghcide to the top level, and set it to ban the various partial functions as we eliminate them.

Re: usesWithStale, I believe it could be simplified to take a NonEmpty NormalizedFilePath and return a NonEmpty list of results, which would allow avoiding the head. Or it could even work over an arbitrary Traversable if you wanted to get fancy, and then you could use Identity for the singleton cases you see there.

That's just from looking at the definitions of the functions! So I think you can probably get quite far without too much domain knowledge.

[pepeiborra]

Re: usesWithStale, I believe it could be simplified to take a NonEmpty NormalizedFilePath and return a NonEmpty list of results, which would allow avoiding the head. Or it could even work over an arbitrary Traversable if you wanted to get fancy, and then you could use Identity for the singleton cases you see there.

usesWithStale doesn't use head so I assume you are referring to useWithStale. The use of head there is completely safe and cannot fail, by local equational reasoning on the call to usesWithStale [file]. So I don't really see the motivation fo the suggested change. Am I missing something?

[pepeiborra]

when it's impossible to "prove" that given partial function will always succeeds, replace it with some variant that gives more informative exception (e.g. headNote from safe). This way we at least get more informative errors when one of these fails and we'll know from user reports which ones actually occur and should be fixed. Wdyt?

Let's make sure to validate these changes before applying them: headNote is not better than head nowadays. Same goes for other partial functions like fromJust, !!, etc.

[michaelpj]

usesWithStale doesn't use head so I assume you are referring to useWithStale. The use of head there is completely safe and cannot fail, by local equational reasoning on the call to usesWithStale [file]. So I don't really see the motivation fo the suggested change. Am I missing something?

Correct, it can't fail. However,

• That could change, and we wouldn't notice.
• Having exceptions to a "no head rule" makes it much harder to apply.
• Using NonEmpty here removes no expressiveness and removes the need to do any reasoning at all (thanks typesystem).

So I agree it's safe, but if it's trivial to rewrite it so we don't even need to think about whether it's safe, that seems like a win to me.

[pepeiborra]

30-odd usages of head, 15 or so foldl1s, 10 or so !!s.

I think many of these are fine, but therefore usually trivial to avoid. It would be nice to have a blanket "no partial functions" rule.

I was actually asking for examples of user reported crashes due to these partial functions. There haven't been that many as far as I can remember, and most if not all of them in the code action providers. Having a catalogue of those would help keep this effort productive.

[michaelpj]

I don't have a catalogue. Someone reported one on IRC yesterday, which is what sent me down this path. They got no stack trace, which of course makes it hard to know which example to blame...

Personally, I think it's cheap enough to just get rid of all uses, and then we just never have to worry about this again. Sometimes I feel like "it's okay for me to use head, I'm sure that the lists won't be empty" is the "it's okay to for me to use null, I have good null-checking" of Haskell programmers. You will fall on the scissors eventually, better just not to do it.

[pepeiborra]

usesWithStale doesn't use head so I assume you are referring to useWithStale. The use of head there is completely safe and cannot fail, by local equational reasoning on the call to usesWithStale [file]. So I don't really see the motivation fo the suggested change. Am I missing something?

Correct, it can't fail. However,

• That could change, and we wouldn't notice.
• Having exceptions to a "no head rule" makes it much harder to apply.
• Using NonEmpty here removes no expressiveness and removes the need to do any reasoning at all (thanks typesystem).

So I agree it's safe, but if it's trivial to rewrite it so we don't even need to think about whether it's safe, that seems like a win to me.

Using NonEmpty does remove expressiveness, as usesWithStale [] becomes no longer possible. So we will need to write lots of new code to handle the empty list case, which will result in reduced clarity.

[michaelpj]

Using NonEmpty does remove expressiveness, as usesWithStale [] becomes no longer possible

Do we want to call usesWithStale []? If that matters to you, then we can generalize it over Traversable and use empty or nonempty lists as we like.

[michaelpj]

Ah yes, this was what came out of the report on IRC: #2518

[pepeiborra]

If we do this, let's make sure to ban all partial functions using hlint as suggested by Michael, otherwise they will get reintroduced by new code and the effort of rewriting all the existing safe partial functions will be in vain.

Reviewing the plan with this in mind:

1. Install a project-wide hlint CI pass to disallow partial functions, and introduce temporary exceptions to get through CI.
2. Replace all of them gradually, removing the hlint exceptions as we go.

[ttylec]

I was actually asking for examples of user reported crashes due to these partial functions. There haven't been that many as far as I can remember, and most if not all of them in the code action providers. Having a catalogue of those would help keep this effort productive.

It was me who reported on irc and made mentioned. Just wanted to add that from the user point of view it is important to avoid crashes, because vscode is pretty bad at informing that there was a crash (and where). Since vscode+hls works in most cases perfectly out-of-the-box is a great feature for beginner haskellers. But then, when such crash happen, it is extremely hard for such person to identify what happened.

[Anton-Latukha]

ban all partial functions using hlint

On #2537 hlint would be able also to give a warning into PR review, as shown there.

So different levels of hlint notifications are now being possible to be used. So for example, we can allow something, if people may really need it.

[codygman]

Great idea!

Long ago I ran into an issue that could have been from a partial function or with this change would have given a better error in #1618

[michaelpj]

Another victim: #2639 (comment)

[pepeiborra]

Another victim: #2639 (comment)

In all fairness, that call to head is probably in the test body itself

[michaelpj]

Yes: let's not use partial functions in tests either!

[michaelpj]

fromJust in the wild: #2672 (comment)

[codygman]

Edit: This error happened running /.haskell-language-server-wrapper from console but doesn't seem to happen when called from my editor (because there is an env I suppose). I usually run in console first to make sure things run successfully, so the partial function broke that test. Things seem fine with hls from within my editor though.

I'm trying to update our work codebase to hls 1.6.1.0 and I got an error that's not helpful:

Message: 
  Maybe.fromJust: Nothing
  CallStack (from HasCallStack):
  error, called at libraries/base/Data/Maybe.hs:148:21 in base:Data.Maybe
  fromJust, called at src/Development/IDE/Core/Rules.hs:842:51 in
  ghcide-1.6.0.0-inplace:Development.IDE.Core.Rules

Going to the source you can see it's because I don't have an env somehow (maybe it failed) and it was trying to display the template Haskell warning:

getModSummaryRule :: Rules ()
getModSummaryRule = do
    env <- lspEnv <$> getShakeExtrasRules
    displayItOnce <- liftIO $ once $ LSP.runLspT (fromJust env) displayTHWarning

But because of the partial function, if I hadn't known how to track this down, I never would have known about the template Haskell warning.

[Anton-Latukha]

Nota bene: As I was around this theme & two times promised to attend to a particular reduction of the head in the main doc retrieval path (#2539). HLS was always retrieving documentation doing the head [res]. Removing it also uncovered a bunch of other stuff around that code.

head removal is not the main agenda, PR is part of work towards provisioning argument documentation, the main agenda there is to provide argument documentation in a form that is neat for further processing.

[hasufell]

IMO, partial functions are fine, if there is sufficient local proof that it is total in the given context (e.g. the input to a partial function is controlled by the outer function itself). #2672 (comment) is an example where no such local proof is possible and the callstack is actually really long. So the caller cannot make any reasonable assumption about env never being Nothing.

On the other hand, I don't think a function like this needs any adjustment:

foo :: ShortByteString -> ShortByteString
foo sbs
  | null sbs = sbs
  | last sbs == _space = "lol"
  | otherwise = "shrug"

So I'm not too thrilled about enforcing hlint compliance. It's a nuisance.

[ttylec]

I agree it is a nuisance but I would like to give here a different perspective: if hls crash for a beginner user, they are basically clueless what happened. Even if able to find Output in VSCode, finding the reason why hls crashed is hard (it is not necessarily the very last line in the output); and if the line contains just Prelude.head: empty list finding a workaround for such user is impossible.

So it's a nuisance for developer, but if the things go wrong way, for a user it is a deal breaker.

[michaelpj]

I think we need a hlint ratchet, even if it's going to be very tedious to whitelist all the existing usages. At the moment it's way too easy for people to introduce new partial function usage, even when it's very easy to avoid. We need something to slow that down.

[michaelpj]

Ratchet started here: #2974

[michaelpj]

Ratchet committed, you can now use .hlint.yaml as a hit list of things to fix :)

[josephcsible]

I don't think a function like this needs any adjustment:

foo :: ShortByteString -> ShortByteString
foo sbs
  | null sbs = sbs
  | last sbs == _space = "lol"
  | otherwise = "shrug"

So I'm not too thrilled about enforcing hlint compliance. It's a nuisance.

Why wouldn't you want to replace that with this?

foo :: ShortByteString -> ShortByteString
foo sbs = case unsnoc sbs of
  Nothing -> sbs
  Just (_, x)
    | x == _space -> "lol"
    | otherwise -> "shrug"

[hasufell]

I don't think a function like this needs any adjustment:

foo :: ShortByteString -> ShortByteString
foo sbs
  | null sbs = sbs
  | last sbs == _space = "lol"
  | otherwise = "shrug"

So I'm not too thrilled about enforcing hlint compliance. It's a nuisance.

Why wouldn't you want to replace that with this?

foo :: ShortByteString -> ShortByteString
foo sbs = case unsnoc sbs of
  Nothing -> sbs
  Just (_, x)
    | x == _space -> "lol"
    | otherwise -> "shrug"

Because that easily gets ridiculous if you want to look at the first 8 characters. I have a lot of those examples from the filepath package, but I'm not gonna list all of them here.