Fix locate in let punnings#2066
Conversation
voodoos
force-pushed
the
locate-in-punning
branch
from
22e7942 to
31b48aa
Compare
There was a problem hiding this comment.
I have mixed feeling because this makes the behavior of occurrences (and maybe other queries? we should think about it!) less intuitive. I do think locate is the most important of them so this is an improvement, just one that is not entirely satisfying.
| > x | ||
| > EOF | ||
|
|
||
| FIXME: Should locate to the `x` in `f x`, not say that we are at the definition |
There was a problem hiding this comment.
This is not a fixme anymore, tight ?
| FIXME: Should locate to the `x` in `f x`, not say that we are at the definition | |
| Should locate to the `x` in `f x`, not say that we are at the definition |
|
@panglesd found a much more annoying regression, this also changes the type returned by type-enclosing on the punned value. He his going to try a better fix. |
panglesd
force-pushed
the
locate-in-punning
branch
from
31b48aa to
0a38fec
Compare
|
Thanks for the review! Yes, this is annoying especially for type-enclosing: We want the type of the pattern to be displayed. I've changed the PR in the following way:
This way, we can select the "expression" node in case of locate query, and the pattern node otherwise. The "oneliner" fix is not anymore a oneliner, but I think it is more correct and hopefully the tie-breaking mechanism can be useful in other contexts. |
voodoos
left a comment
voodoos
left a comment
There was a problem hiding this comment.
Thanks, that looks much better !
The next step is to check which other places could benefit from this (aka record labels and labelled arguments).
|
|
||
| let compare_locations pos l1 l2 = | ||
| module Tie_breaker = struct | ||
| type tie_break = [ `Prefer_first | `Prefer_second ] |
There was a problem hiding this comment.
Did we really need polymorphic variant here?
Is that just for avoiding repeating the path Mbrowse.Tie_breaking... ?
There was a problem hiding this comment.
No I don't think we need it, and the path is probably not needed anyway with type-directed disambiguation of constructors. I think I started with it before I defined the type and it stuck.
I will turn it into regular variants
| module Tie_breaker : sig | ||
| (** Some nodes in the typedtree may share the same, non-ghost, location, while | ||
| being different. One example of this is let-punning: | ||
|
|
There was a problem hiding this comment.
For science, can we have more than two overlapped nodes ?
There was a problem hiding this comment.
I... don't think so, unless you take ppx rewriting into account.
But maybe it can, I don't know. For instance, I think defining an object defines internally: an object, a type and a class type? Whose locations might coincide? (completely outside of my comfort zone).
Anyway, the tie-breaker compares nodes two-by-two, carrying the winner to the next match, until there is no challenger left. That is less expressive than taking all overlapping nodes, but I think it is expressive enough.
| ( Context.inspect_browse_tree | ||
| ~disambiguate:Mbrowse.Tie_breaker.prefer_expression ~cursor:pos lid | ||
| [ browse ], | ||
| is_label ) |
There was a problem hiding this comment.
Nitpick: maybe this would be more readable by defining disambiguate before the match expression ? Or even the complete fst of the pair.
Include let punnings, named argument punnings and fields punning
Change the order in which we fold the let operators, to give priority to the expression in the "locate" query
Changing the selected node for a better locate experience, made a worse experience for other queries such as type-enclosing.
and fix a typo in the docstring
panglesd
force-pushed
the
locate-in-punning
branch
from
27cb84d to
8bbda06
Compare
3 participants
This fixes an issue with the locate query reported in #1796.
(Sorry this is quite verbose for such a small PR but it is also here to help me be corrected if I have assumed something wrong)
In a punned let binding, both the pattern and the expression have the same location. None of them is ghost. I think that somehow breaks the assumption that "sibling nodes have disjoint locations".
During a locate query, the first thing is to load the environment at the point of the cursor. The lookup of "nodes at cursor position" will suppose the invariant mentioned above to return a list all nodes the cursor is in (of which we take the first one, the "deepest").
Since two siblings (the pattern and expression) have the same location, the order in which they are considered matter: when the first one is found, we never look at the other siblings, and "enter" it to continue the search.
In the current implementation, patterns are looked first, and we end up with the environment at the pattern to do our "locate". And so, the unhelpful "Already at definition point".
This PR fixes this by folding let operators in a different order, making the expressions side be entered first.
Having the behaviour depend on the order of the fold is slightly brittle, but I think this PR is still fine to solve #1796 without refactoring a huge part to solve a bigger underlying issue ("siblings might have the same location"). But I'm new to the codebase so I might miss something!
(I'm happy to implement a better fix, but then I'd like to discuss with more experienced Merlin disciple first)