Fix locate for punned record pattern #147
Conversation
| @@ -1,4 +1,4 @@ | |||
| $ $MERLIN single complete-prefix -position 11:10 -prefix "Z." \ | |||
| $ $MERLIN single complete-prefix -position 9:10 -prefix "Z." \ | |||
There was a problem hiding this comment.
This is a drive by - while performing this refactor, I noticed that this position was incorrect. Leaving this as it was would still result in the test passing
| @@ -1,6 +1,6 @@ | |||
| Refactor open for record fields | |||
|
|
|||
| $ $MERLIN single refactor-open -action unqualify -position 4:7 <<EOF | |||
| $ $MERLIN single refactor-open -action unqualify -position 4:6 <<EOF | |||
There was a problem hiding this comment.
Same deal as above - this is just a drive-by.
There was a problem hiding this comment.
I can verify that the tests look reasonable, and that the changes make the tests pass.
With some time, I could look through the implementation and agree that it's correct. (I did not do this.)
I have no opinion about the refactor commit or whether the approach taken here is the best approach -- and getting an informed opinion would be hard.
Is this something @voodoos could give more input on? Is this a change that would be welcome upstream?
There was a problem hiding this comment.
I think it's fine to merge this -- though probably better for @voodoos to take a look.
|
Sorry, I won't have time to look at this anytime soon due to my paternity leave. I see you made quite a few changes to mbrowse to handle punnings in lets (and now records), do you think it would make sense to upstream these at some point ? |
In the below code:
If you ask to go to the definition of
a, Merlin will tell you that you're already at the definition point. This is because it chooses to find the location of the patternarather than the labela. This is silly; if you're asking to go to definition here, you clearly want to go to the label definition.There's some prior art here. Last fall, we addressed a similar problem for punned let expressions: #96. When parsing a punned let expression, Merlin's parser inserts an attribute on both the pattern and the expression to label them as coming from a punned let expression. Later, when Merlin chooses a node in the typedtree based on the position of the cursor, it detects the presence of the attribute and chooses the appriopriate node (this happens in
mbrowse.ml).This PR implements a similar strategy, but is slightly different. The AST doesn't have a place to put an attribute on the punned label, so we can only put the attribute on the pattern. This is sufficient, but it made the logic in
mbrowse.mlbe slightly different. As a result, this PR refactors how Merlin selects a node in the typedtree based on cursor position. The refactor is behavior-preserving (except in the presence of the new attribute).Note for reviewing: I recommend going commit-by-commit. The commits are broken up logically, and the tests pass on each one.