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

Do not index occurrences from ppxes #1768

Merged
merged 3 commits into from
May 17, 2024
Merged

Conversation

voodoos
Copy link
Collaborator

@voodoos voodoos commented May 16, 2024

Current version of the indexing iterator actually works on the ppxed-typedtree. This means that we should detect part of the tree that are introduced by PPXes and not index them.

This PR tries to do that in the usual way: it looks for the merlin.hide attribute on some strategic nodes.

@pitag-ha I am still a bit uncertain of which nodes should be considered to look for the merlin.hide attributes ?
Once again, that will only work with ppxes that actually use this attribute... I guess we can have that certainty only with ppxes written using ppxlib ?

We should try to devise a most robust way to distinguished ppxed part of the tree...

cc @Octachron since similar changes should be upstreamed at some point to prevent project-wide occurrences to return location that are actually coming from ppxes.

@voodoos voodoos added this to the 5.0-502 milestone May 16, 2024
voodoos added a commit to voodoos/merlin that referenced this pull request May 16, 2024
@pitag-ha
Copy link
Member

which nodes should be considered to look for the merlin.hide attributes ?

@voodoos and I have already discussed this in private, but in case it's interesting to know what we've discussed: Let's distinguish derivers from other PPXs such as extension node rewriters.

For derivers, we'd only need to look for merlin.hide attrs attached to strucutre/signature items, because Ppxlib wraps the derived code into a structure/signature item with merlin.hide attr. The reason for that is to preserve the location invariants: Without merlin.hide attr, the derived nodes would duplicate the location of a sibling node. All this is only true for Ppxlib derivers. Ppx_deriving doesn't add a merlin.hide attribute, which is why Ppx_deriving derivers have a tendency to mess up Merlin in general.

For other PPXs, e.g. extension node rewriters, it's not Ppxlib itself who adds the merlin.hide attributes, but the PPX author who can do so for certain nodes via Ppxlib's Merlin_helpers.hide_attribute. While it's more likely to add that attribute on expressions or patterns than on other nodes, it can be done on any node allowing an attribute. E.g. ppx_compare does so on a core_type.

So, it's safest here to look for the merlin.hide attribute on any node allowing attributes.

@pitag-ha
Copy link
Member

To my not-very-familiar-with-the-typedtree eyes, this looks good to me up to one doubt I have: The iteration through a node is not necessary to update the typing environment / the scope to what happens in the node, right? I'm asking because, with the PR, the iteration is fully skipped if there's a merlin.hide attribute. So if the iteration was needed to update the scope, then there might be the following problem: Whenever derived code (i.e. code with merlin.hide attribute) shadows the value whose occurrences are being analyzed, there might be false positives of those occurrences in the rest of the file.

@voodoos voodoos merged commit 6b5fe77 into ocaml:master May 17, 2024
7 of 10 checks passed
voodoos added a commit to voodoos/opam-repository that referenced this pull request May 17, 2024
CHANGES:

Fri May 17 19:59:42 CET 2024

  + merlin binary
    - Support for OCaml 5.2 (ocaml/merlin#1757)
    - destruct: Removal of residual patterns (ocaml/merlin#1737, fixes ocaml/merlin#1560)
    - Do not erase fields' names when destructing punned record fields (ocaml/merlin#1734,
      fixes ocaml/merlin#1661)
    - Ignore SIGPIPE in the Merlin server process (ocaml/merlin#1746)
    - Fix lexing of quoted strings in comments (ocaml/merlin#1754, fixes ocaml/merlin#1753)
    - Improve cursor position detection in longidents (ocaml/merlin#1756)
    - Addition of a `merlin-lib.commands` library which disassociates the
      execution of commands from the `new_protocol`, from the binary, allowing
      it to be invoked from other projects (ocaml/merlin#1758)
    - New occurrences backend: Don't index occurrences when `merlin.hide`
      attribute is present. (ocaml/merlin#1768)
    - Use the new `uid_to_decl` table in 5.2's cmt files to get documentation.
      (ocaml/merlin#1773)
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.

2 participants