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

Refactor Type_enclosing and make use of Context information #1108

Merged
merged 42 commits into from
Jun 4, 2020

Conversation

voodoos
Copy link
Collaborator

@voodoos voodoos commented Mar 4, 2020

This PR:

  • Extracts the module Context from analysis/locate.ml
  • Extracts some of the Type_enclosing logic from query_commands.ml
  • Makes Context provide richer infos
  • Use that information in Type_enclosing

This fixes the bug highlighted in #1104
Should also fix #864

@trefis
Copy link
Contributor

trefis commented Mar 4, 2020

Actually, can we split this PR in two?
Let's just keep the extraction commits (the first two) that are obviously good and mergeable, and open a separate one for the use of context in type_enclosing.

Thanks!

@voodoos
Copy link
Collaborator Author

voodoos commented Mar 25, 2020

Status of this PR

It is still partial, and when context information is not satisfying it fallback to the old behaviour: try to type in the various namespaces until success. Tests need to be more exhaustive but I believe it should give identical or better results than master. For example it does fix #1104

The weird transition from 405 to 406

As we discussed with @trefis and @let-def, something happened between these two versions.
With this PR the tests context.t and let.t, which were added recently as issues, are indeed behaving as expected for ocaml <= 405.
However past version 406 they do not behave as expected any more.

Todo

  • Solve the discrepancy between 405 and 406 (may be in another PR)
  • Use context information to type partial paths (for now it falls back to previous behaviour)
  • Remove some debug prints

@voodoos voodoos force-pushed the refactor branch 5 times, most recently from 64da4fd to 766423a Compare March 30, 2020 13:50
@voodoos
Copy link
Collaborator Author

voodoos commented Mar 30, 2020

What changed since last comment:

  • Added more tests
  • Cleaner Type_enclosing signature
  • Refactored type_in_env and removed some need for the fallbacks thanks to better context longident analysis (fallback stil present in the code).
  • Test cons.t is now correct from version 406
  • Record labels are now correctly typed
  • Fixed a bug in locate (527f4a7)
  • Tests let.t still wrong

@voodoos
Copy link
Collaborator Author

voodoos commented May 29, 2020

@trefis do you think this is ready for merging ?

It opens the door to more work, but this can be the subject of other PR's:

  • Solve the discrepancy between 405 and 406 (may be in another PR)
  • cons.t test still wrong from 406.
  • when cursor at (M.|A 3), the enclosing node returned is const 3, thus
    breaking the context inference (but the fallback catch that for now...)

These are not regressions but already present issues.
The last one is the one that prevent us for having a fully context-based type inference. (For now the old fallback behaviour is still in place.)

Copy link
Contributor

@trefis trefis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last request before this can be merged: there seems to be a lot of whitespace changes in areas of the code / comments that you otherwise do not touch.
Could these be reverted?

@voodoos voodoos requested a review from trefis June 4, 2020 08:47
Copy link
Contributor

@trefis trefis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will merge once CI comes back green.

@trefis trefis merged commit 45cc5ba into ocaml:master Jun 4, 2020
voodoos added a commit to voodoos/opam-repository that referenced this pull request Jun 10, 2020
CHANGES:

Tue Jun  9 15:13:42 CET 2020

  + ocaml support
    - alerts are no-more ignored and are reported as warnings (ocaml/merlin#1138)
  + merlin binary
    - fix completion of names containing `-` (ocaml/merlin#1142)
    - fix several type-enclosing bugs by performing context-analysis (ocaml/merlin#1108)
    - lsp: add deprecation flag to outline items (ocaml/merlin#1087)
    - lsp: add go-to typedef (`Locate_type`) (ocaml/merlin#1067)
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-type-enclosing picks up variable from scope instead of record field
2 participants