Conversation
WardBrian
left a comment
WardBrian
left a comment
There was a problem hiding this comment.
Thanks @voodoos, excited to get jumps on these!
First, I'm not sure what is is necessary to actually try out this branch in my local dev environment (pinning it doesn't seem to affect the behavior I end up seeing up in ocaml-lsp/eglot), so this is based purely on reading the test output.
- The test added in #1917 has slightly different behavior in this branch, it seems like it is pickier about exactly where the cursor is placed. This may be intentional, but thought it was worth flagging.
- #1917 also resolved (largely by accident) a FIXME in
tests/test-dirs/locate/context-detection/cd-test.t/run.t, which this branch doesn't (even though I would expect it to, since it is based on record lookup and thefind_record_fieldfunction here should find it, no?) - Because this still falls back to reconstruct_identifier, maybe it is worth including the two non-heuristic parts of #1917 (the first two bullet points)?
| } | ||
|
|
||
| $ $MERLIN single locate -look-for ml -position 6:15 \ | ||
| $ $MERLIN single locate -look-for ml -position 5:16 \ |
There was a problem hiding this comment.
It looks like this is more conservative than #1917, which (correctly IMO) identified this as the operator at 2:4
Thanks for the quick review ! I will have a look at the cases you mentioned. We probably also should gather "namespaces" at the same time, and merge this with the existing Context processing. We could also try to use the upstream longident parser now that it correctly registers locations to also improve the reconstruct identifier heuristic itself. |
2 participants
Now that all the locations of a Longident's components are preserved in the Typedtree, there are many cases where the dreaded
reconstruct_identifierheuristic is needed. Reconstruct identifier was re-parsing the longidents, and many issues stemmed from that process, especially in the handling of custom operators which are tricky to parse right out of context.In this PR we directly lookup longidents in the typedtree when possible and split them according to the cursor location. This is much more robust and works in most cases. In some recovery cases though, the longident is unavailable due to a type-error, we when fallback to the old logic.
This might evolve before that PR is merged, but right now only Locate use the new backend.
This replaces #1917 and should fix #1915