feat: update type signature during add argument action #3321
Conversation
|
One point of annoyance I had during implementation: you can't run golden tests without setting It took me a while to realize... is there a way to fix |
| a -> | ||
| TransformT m a | ||
| TransformT m (a, Maybe r) |
There was a problem hiding this comment.
What does this do now? Should the Haddock change to explain the new behaviour?
| pure $ MG xMg (L locMatches matches') originMg | ||
| modifyMgMatchesT mg f = fst <$> modifyMgMatchesT' mg (fmap (, ()) . f) () ((.) pure . const) | ||
|
|
||
| -- | Modify the each LMatch in a MatchGroup |
There was a problem hiding this comment.
| -- | Modify the each LMatch in a MatchGroup | |
| -- | Modify each LMatch in a MatchGroup |
| @@ -972,31 +982,97 @@ suggestAddArgument parsedModule Diagnostic {_message, _range} | |||
| where | |||
| message = unifySpaces _message | |||
|
|
|||
| -- TODO use typ to modify type signature | |||
| -- Given a name for the new binding, add a new pattern to the match in the last position, | |||
| -- returning how many patterns are in this match: | |||
There was a problem hiding this comment.
| -- returning how many patterns are in this match: | |
| -- returning how many patterns there were in this match before the modification: |
There was a problem hiding this comment.
Should it be before or after?
There was a problem hiding this comment.
Before. Revised
| in (L locMatch (Match xMatch ctxMatch (pats <> [newPat]) rhs), length pats) | ||
|
|
||
| -- Attempt to insert a binding pattern into each match for the given LHsDecl. Also return the declaration's name, and | ||
| -- the number of bound patterns in the declaration's matches, if the HsDecl is a fun bind |
There was a problem hiding this comment.
| -- the number of bound patterns in the declaration's matches, if the HsDecl is a fun bind | |
| -- the number of bound patterns in the declaration's matches before the transformation, if the HsDecl is a fun bind |
| appendFinalPatToMatches :: T.Text -> LHsDecl GhcPs -> TransformT (Either ResponseError) (LHsDecl GhcPs, Maybe (GenLocated SrcSpanAnnN RdrName, Int)) | ||
| appendFinalPatToMatches name = \case | ||
| (L locDecl (ValD xVal (FunBind xFunBind idFunBind mg coreFunBind))) -> do | ||
| (mg', numPatsMay) <- modifyMgMatchesT' mg (pure . second Just . addArgToMatch name) Nothing combineMatchNumPats |
There was a problem hiding this comment.
This all feels a bit roundabout. You've taught two functions to pass up the number of matches, and then changed modifyMgMatches to pass a result thorough... but it seems like you could just look for the number of matches independently of the modification, either before or afterwards. Sure, you have to write another function, but I imagine funBindArgCount is quite straightforward code and simplifies all the rest.
There was a problem hiding this comment.
The unfortunate thing is that we don't know which declaration is going to be altered.
Note that we use modifySmallestDeclWithM with this function in addArgumentAction.
An alternative would be to do two calls to modifySmallestDeclWithM (perhaps a variant called viewSmallestDeclWithM that just returns some information on the matching decl). The first would do the modification, the second would extract the information (and then the two maybes would be <|> together).
I don't personally find that clearer... While I do agree that this code is a little odd, I think it's part and parcel of these sorts of transformations :/
I'm happy to switch it up if you still think it'd be clearer.
There was a problem hiding this comment.
Okay, so you do quite a bit of finding this decl. You have to go in and find it here to modify it, then you have to go in again to find the matching signature?
Continuing possibly dumb questions: could we not do something like:
do
decl <- findSmallestDecl
sig <- findSigFor decl
-- return the modified version in case we want to look at them
decl' <- modifyDecl decl
sig' <- modifySig sig
Anyway, I won't nitpick more: I'm happy to go with what you think makes sense as a structure here. It just feels like we've got some pieces of work that are currently entangled and I wonder if they could be disentangled.
There was a problem hiding this comment.
Ah I see your thought. We certainly could do something like this.
However, this not generally how I've seen other AST refactors done with ghc-exactprint. Normally I see modifications done in a lens style Lens ast Decl, and I think it makes some sense to continue to do so.
The API you suggest is actually something I've been considering myself for a while, but the exactprint API is far from stable enough to have such niceties (there would probably be a fair amount of CPP involved in the near future...).
plugins/hls-refactor-plugin/test/data/golden/add-arg/AddArgWithTypeSynSig.hs
Show resolved
Hide resolved
|
What happens if you run this on a local def in a let or where? does it work? Can we have some tests to show whatever happens? And what happens if there's an intervening local def like the lambda case? |
|
There were already tests for where bindings in the tests that existed before support for updating type signatures. However, I've added golden tests because the old tests were crap (I will be refactoring them in a follow up). This plugin currently only looks for top-level bindings. foo = bar
where bar = new_def
-- Hover over new_def...
-- current behavior (only for top levels):
+ add as argument
-- future behavior:
+ add as argument to foo
+ add as argument to bar |
@michaelpj @pepeiborra This one resolves a lot of the issues you had with the previous MR
It's also hopefully much clearer now, since I spent some time cleaning up also...
Some notes: