Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Illustrate and fix issue #1610 #1611

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

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 Illustrate and fix 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...

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant