Skip to content

Conversation

@voodoos
Copy link
Collaborator

@voodoos voodoos commented May 22, 2023

This PR add tests illustrating issue #1610: locate not jumping correctly with path such as M(C).t.
There are two things leading to this:

  1. reconstruct_identifer looks broken on module paths
  2. Locate used Longident.parse to parse which is also broken on module path (and infix operators).

This PR improve 2 by using Parser.parse_longident. This make locate work when the path M(C).t is manually inputted by the user. This also fixes another issue related to infix operators.

I will keep that PR as a draft until 1 is also fixes (and thus the issue).

@voodoos voodoos changed the title Illustrate issue #1610 May 23, 2023
@voodoos voodoos marked this pull request as ready for review May 23, 2023 13:06
@voodoos voodoos requested review from let-def and removed request for let-def May 23, 2023 13:12
@voodoos
Copy link
Collaborator Author

voodoos commented May 23, 2023

@let-def I had to modify reconstruct_identifierfor that patch.

The issue was that "papply" components in paths where dropped leading to the impossibility to jump to t in N(F).t.
I decided to go with a quick, but kind-of dirty trick: application are considered a single component. A path M.N(F).t would be reconstructed as ["M";"N(F)";"t"].

This does fix the issue but prevent users from jumping to N of F.

I though this wasn't supported before anyway, but I think I am wrong about that, that's why I cancelled my request for review. I guess there is no working around a cleaner implementation without breaking existing behavior...

voodoos added 6 commits May 24, 2023 10:52
This fixes locate when a prefix is given, but reconstruct_identifier is still not giving the correct answer.

This also fix another issue with infix operators.
 about locating infix operators
This treat an application as a single components so it is not a satisfing long-term solution.

A better approach would be to change the return type of [reconstruct_identifier] to account for module application.

This fixes ocaml#1610
@voodoos voodoos force-pushed the issue-1610-jump-type-in-functor branch from 91507bd to 0d8bdd9 Compare May 24, 2023 08:57
@voodoos
Copy link
Collaborator Author

voodoos commented May 24, 2023

@let-def I tried to get reconstruct_identifier to work with Papply components in 0d8bdd9 but it's not very satisfying. I guess we should really have a different output type than a flat list ? Have you thought about it already ? I am surprised that this was not handled so far.

In any case, the current hacky solution works on the test suite but it has an unexpected side-effect:
When looking fo the type of t in M(N).t reconstruct identifier now correctly returns the list of reconstructed identifiers: ["M(N).t"] instead of an empty list. However parsing that expression fails: Parser_raw.parse_expression mistakenly returns Pexp_construct and Pexp_field nodes and so type-enclosing is wrong. Do you now how I could fix this ?

@voodoos voodoos mentioned this pull request May 24, 2023
@voodoos voodoos marked this pull request as draft July 4, 2023 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

1 participant