Case split plugin

[Aster89]

This PR is for introducing the so called case-split plugin, as requested in #5013.

(In the following, me is myself and we is myself, @fendor, @MangoIV, and @AndreasPK.)

The change as of now needs lots of refinements (obviously beside getting rid of all the trace* calls I've peppered the code with, and beside squashing all commits together), especially these:

• currently, it's not clear to me how to affect the maximum number of patterns that are inserted by the plugin without also affecting the maximum number of uncovered patterns in the diagnostic error message;
  • ideally the two numbers should be independent, i.e., the diagnostic error message would still show maxUncoveredPatterns uncovered patterns, but the plugin would nonetheless insert _all_¹ uncovered patterns;
  • we agreed a good first strategy is to have the plugin honor maxUncoveredPatterns, because it's the least invasive approach, it doesn't require any additional code, one can only improve from there, and nothing prevents the user from triggering the plugin more than once to uncover more and more patterns (as noticed by Andreas, this could actually be the gist of some solution to the whole problem, i.e. trigger the plugin repeatedly until no uncovered pattern remains);
• as shown by several tests (e.g. T16.hs), the indentation of the patterns inserted by the plugins is sometimes too much, although correct,
  • and this is because I haven't fully grasped how deltaPos and related abstractions work; need to chat with @alanz about this;
• a few tests are not passing, testifying some decisions I haven't taken yet/use cases I haven't really a clear idea about:
  • I would assume the plugin should work for \case just as it does for case (see T12.hs), but maybe this is as much a generalization as having the plugin work with function definitions, so I guess we can drop this test, or maybe assume it as failing and link a new enhancement request to it?
  • The plugin works even if the expression being scrutinized is _, but the test doesn't pass (see T18.hs); I still don't know why;
  • how should the plugin behave when the type is not known, e.g. see T19.hs? I think it should just not offer any action.
  • when two case expressions, both incomplete, are nested (e.g. T14.hs), the plugin should offer an action for each incomplete case trigger for the innermost case expression which the cursor is on;
• the arrow should be -> or its unicode counterpart, honoring the -XUnicodeSyntax flag or previous patterns.

---

¹ Not really all, we don't want to split an all Ints, for instance, nor on an actual data with 100 ctors, presumably. Or do we?

[dschrempf]

Everybody is eagerly anticipating this change, thank you!

Given the many WIP commits, I just wanted to ask if you could sqaush or reword them. I guess you had this in mind anyways.

[Aster89]

Everybody is eagerly anticipating this change, thank you!

Glad to hear that!

Given the many WIP commits, I just wanted to ask if you could sqaush or reword them. I guess you had this in mind anyways.

Yep, see 3rd paragraph at the top :P

[dschrempf]

Ah, the second sub-clause :-P my attention had already shifted before that one, I apologize :-)! Thanks!

[MangoIV]

First pass. I think after you clean up the main logic a bit more and add some documentation there, I can take another look. :)

Very good work, looking forward to having this in HLS!

[MangoIV]

another small round :)

[MangoIV]

now you can reinstate mapMaybe again, to get rid of the fromJust.

let extractDiagAndAlts diag dsMsg = 
      (fdLspDiagnostic diag, dsMsgToPmAlts dsMsg)
pure $
   mapMaybe ( \diag -> extractDiagAndAlts diag <$> getMaybeDsMsg diag ) fileDiags

so now you can skip the bimap in the next step.

[Aster89]

I see the advantage this approach causes in "the next step".

But if I have to be honest, I find the last line of your snippet fairly harder to read, because the majority of the line is a lambda, rather than a "vocabulary" word, and I always have to read it all, and reason about it, to remind myself/re-understand what it does.

On the contrary, the mapMaybe sequence takes me less time to understand once I know the in and out types. I mean, if I know the type in is [(a, Maybe b)] and type out is [(a, b)], I know exactly what it takes to go from the former to the latter, and I easily believe that mapMaybe sequence is doing the right job, even without verifying it by hand, because it's just a combination of two existing simple abstractions: the fact that they typecheck reassures me. Unlike the lambda, which contains "hand-written" logic, and so I feel obliged to check that it is doing indeed the right choice.

And even without mapMaybe sequence, if I can rely on individual steps built on top of filter/map/isJust/fromJust, that still makes it more readable.

But I suppose this is just the way I imagine code in front of my eyes vs how you do it?

[MangoIV]

You can use for instance ExceptT for now. Is it clear why that's better?
And if you want to, you can try

newtype ExceptCT e m a 
  = MkExceptC {unExceptCT :: forall b. (e -> m b) -> (a -> m b) -> m b} 

and see if that's faster. But that doesn't have to be done now.

[MangoIV]

you can avoid having to write zipWith3 and get cleaner code in one go, by applying the review comment that I gave you previously.

[Aster89]

You mean this comment I assume. I had initially done that - before trying to NonEmpty all lists - but at some point I fell back to a 3 way zip. Can't remember why. I'll review that once more.

[Aster89]

tl;dr

In hindsight, I don't think I understood your original suggestion.

How I interpreted your original suggestion

The original code you commented was this:

              then zipWith3 (.)
                            (replicate (length missing - 1) addSemiCol ++ [id])
                            (repeat $ setDP 1 defaultIndent)

and your comment was

couldn't we make this a bit more regular by applying the transformation in the first argument to the second argument before passing it to zipWith ($)?

I didn't quite understand what first and second refer to, so I looked at the target (use zipWith ($)) and thought of a way of rearranging the arguments to do that, and came up with this:

              then zipWith ($)
                           (fmap (repeat (setDP 1 defaultIndent) .) $ replicate (length missing - 1) addSemiCol ++ [id])

which is applying the transformation in the third argument to the second argument before passing it to zipWith ($). I thought ok, maybe Magnus was in a rush, counted the arguments of zipWith3 (.) rather then the arguments of zipWith3, and did the typo of swapping "first" and "second".

That interpretation doesn't work on the present code

But this interpretation I gave builds on the fact that the 3rd argument to zipWith3 is a repeat.

I don't quite remember how, but the current code now doesn't have this, so I must assume that I've misunderstood your original suggestion.

Can you clarify?