Open michaelpj opened 2 years ago
Could you share a few examples?
30-odd usages of head
, 15 or so foldl1
s, 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.
30-ish fromJust
...
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: https://github.com/haskell/haskell-language-server/blob/070f16fbc21be5c9c889e09fe78ec2f2e3d90ed9/ghcide/src/Development/IDE/Core/Shake.hs#L854-L867 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:
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!The plan sounds really great to me, given at minimum we will have more info about the error. Many thanks for willing to contribute
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.
Re:
usesWithStale
, I believe it could be simplified to take aNonEmpty NormalizedFilePath
and return aNonEmpty
list of results, which would allow avoiding thehead
. Or it could even work over an arbitraryTraversable
if you wanted to get fancy, and then you could useIdentity
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?
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.
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,
head
rule" makes it much harder to apply.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.
30-odd usages of
head
, 15 or sofoldl1
s, 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.
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.
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.
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.
Ah yes, this was what came out of the report on IRC: https://github.com/haskell/haskell-language-server/pull/2518
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:
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.
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.
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
Another victim: https://github.com/haskell/haskell-language-server/pull/2639#issuecomment-1022727133
In all fairness, that call to head is probably in the test body itself
Yes: let's not use partial functions in tests either!
fromJust
in the wild: https://github.com/haskell/haskell-language-server/issues/2672#issue-1120471115
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.
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.
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). https://github.com/haskell/haskell-language-server/issues/2672#issuecomment-1044260987 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.
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.
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.
Ratchet started here: https://github.com/haskell/haskell-language-server/pull/2974
Ratchet committed, you can now use .hlint.yaml
as a hit list of things to fix :)
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"
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.
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!