Case split plugin - #5014
Conversation
|
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. |
Glad to hear that!
Yep, see 3rd paragraph at the top :P |
|
Ah, the second sub-clause :-P my attention had already shifted before that one, I apologize :-)! Thanks! |
5456d10 to
038348b
Compare
MangoIV
left a comment
There was a problem hiding this comment.
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!
82aad14 to
c7f9b61
Compare
There was a problem hiding this comment.
Very nice, this looks like it is making great progress!
I reviewed the plugin, I have mostly comments regarding readability and suggestions for improvements that hopefully improve the maintainability in the long run.
Some things are nitpicks, and you should feel free to ignore them.
I think we need one Note that outlines what the implementation strategy of the case split plugin is (e.g., reads the structured error message, etc...) so that it is easier to tell what the individual steps for case splitting are.
| getActionByTitle :: T.Text -> [CodeAction] -> Session CodeAction | ||
| getActionByTitle title actions = | ||
| case find (\a -> a ^. L.title == title) actions of | ||
| Just a -> pure a | ||
| Nothing -> liftIO $ assertFailure $ "Action " <> show title <> " not found in " <> show [a ^. L.title | a <- actions] |
There was a problem hiding this comment.
There is inspectCodeAction which might be what you are looking for here, should also be helpful to get rid of the _CACodeAction
| caseSplitPluginCodeActionTitle :: Text | ||
| caseSplitPluginCodeActionTitle = "Add placeholders for the first `-fmax-uncovered-patterns` missing patterns" |
There was a problem hiding this comment.
I am not a huge fan of the title, as it exposes implementation details... But I don't have a better idea for now
There was a problem hiding this comment.
Do I understand that you're alluding to the fact I mention -fmax-uncovered-patterns? I'd argue that this is rightfully documenting a current limitation of the plugin, rather than exposing an implementation detail.
There was a problem hiding this comment.
If you case split on the non exhausted patterns, how soon would you like to know its current limitations?
Like this, it is the second thing you learn, and you learn that there is a flag called -fmax-uncovered-patterns. As a user, what are you supposed to do about this?
I just don't think we need to say in the code action title, why we are only completing -fmax-uncovered-patterns cases, as it doesn't mean anything to beginners, and is maybe not even observable in many applications.
There was a problem hiding this comment.
If you case split on the non exhausted patterns, how soon would you like to know its current limitations?
I would say "soon", personally.
As a user, what are you supposed to do about this?
"Supposed to", nothing, but at least I'm given pointers to look for if I care about it.
I just don't think we need to say in the code action title, why we are
Well, technically I'm not telling why. I'm just saying what's the number of completions they get, to avoid surprising them when they don't see them all.
But I guess this is already in the land of preference/opinions? 😄
There was a problem hiding this comment.
Yes, my opinion is, mentioning the option (not even the number of patterns you are going to match) doesn't add anything worthwhile to the average users. But I am not going to fight over it.
@MangoIV what do you think?
There was a problem hiding this comment.
I don't think it's crucial information in general here but I see an issue in users reporting bugs upstream because they think this is not a known caveat. I don't know where else to put the information though except perhaps in the documentation / on the website of HLS itself?
There was a problem hiding this comment.
The documentation on the website feels like the appropriate place. (Should be added regardless)
I expect users are going to report it either way, as it doesn't really mean anything to them.
There was a problem hiding this comment.
The documentation on the website feels like the appropriate place.
Which website?
Are you by any chance referring to the bullet list here?
(Should be added regardless)
Before GSoC submission deadline, or can I do it afterwards?
Since
But I am not going to fight over it.
and
I see an issue in users reporting bugs upstream because they think this is not a known caveat
I'll keep the message as is. And take note (of changing this String) in the stretch goals.
| -- | ||
| -- | ||
| -- Refer to test cases to see practical examples. | ||
| appendMissingPats :: Maybe Int -> MatchGroup GhcPs (LHsExpr GhcPs) -> NonEmpty (LMatch GhcPs (LHsExpr GhcPs)) -> Maybe (MatchGroup GhcPs (LHsExpr GhcPs)) |
There was a problem hiding this comment.
If I understand correctly, this is doing the heavy lifting of actually pushing the new matches to the AST.
Afaict, there are four ways we support grafting:
- LambdaCase with braces
- LambdaCase without braces
- case with braces
- case without braces
I am not sure merging these cases altogether is the best strategy.
What would the code look like if we differentiated between these cases explicitly? More duplication, but maybe also simpler?
This PR is for (re-¹)introducing the so called case-split plugin, as requested in #5013, and it is part of my project for GSoC 2026.
To see the full summary of the project, click here. The summary also contains a list of stretch goals, some of which have been addressed already.
The work I've done and that I plan to submit via the present pull request, has consisted mainly of:
ghc-exactprintfor correctly layout out the patterns to be inserted,The result of such work is a plugin providing a code action for adding missing patterns to a
case/\caseexpression that has a non-exhaustive list of patterns.For instance, the plugin turns this
into this
¹ Earlier attempts
Indeed, there were some earlier attempts. See the summary for some pointers.