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.