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

Locate infix operators #1445

Closed
wants to merge 3 commits into from
Closed

Locate infix operators #1445

wants to merge 3 commits into from

Conversation

voodoos
Copy link
Collaborator

@voodoos voodoos commented Mar 18, 2022

Two Three years later

This fixes #949

In #949 the issue was identified as Longident.parse returning wrong lids for infix operators. A PR was merged in the compiler to expose the true longident parser.

In fact this PR relies on Longident.unflatten.

~~This PR uses that parser to parse longidents produced by reconstruct_identifier.
However for usages of an infix operator the reconstructed identifier does not have surrounding parenthesis and the parser fails on these.

I chose to simply rerun the parser with added parenthesis if it fails, and fallback on Longident.parse if it fails again.
We probably could have a more deterministic style here, but I am not sure if that is worth the effort... @trefis ?~~

@bbatsov
Copy link

bbatsov commented Jul 25, 2022

Two Three years later

Better late than never. ;-)

@voodoos
Copy link
Collaborator Author

voodoos commented Jul 26, 2022

:-)
There are a still few things I want to change before considering reviewing and merging that PR.

@voodoos voodoos marked this pull request as draft July 26, 2022 21:02
@voodoos voodoos force-pushed the fix-949 branch 2 times, most recently from acfd919 to e297d87 Compare November 25, 2022 14:31
@voodoos voodoos marked this pull request as ready for review November 25, 2022 14:31
@voodoos voodoos added this to the Next release milestone Feb 1, 2023
@voodoos
Copy link
Collaborator Author

voodoos commented Feb 21, 2023

Update: this PR has not been merged yet because we discovered some issues with the current approach. More work is needed.

@voodoos
Copy link
Collaborator Author

voodoos commented May 24, 2023

Closing in favor of #1612

@voodoos voodoos closed this May 24, 2023
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.

merlin-locate does not work on +.
2 participants