ocaml / merlin

Context sensitive completion for OCaml in Vim and Emacs
https://ocaml.github.io/merlin/
MIT License
1.59k stars 233 forks source link

Do not index occurrences from ppxes #1768

Closed voodoos closed 6 months ago

voodoos commented 6 months ago

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.

pitag-ha commented 6 months ago

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 commented 6 months ago

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.