Search for a symbol in workspace looks into .pp.ml files

[ulugbekna]

Searching for a symbol in workspace uses .pp.ml files, it seems, which users can't actually open.

[tatchi]

Am I right that .pp.ml files are related to ppxes? What should be done in that case?

We can filter out sourcefiles that end with .pp.ml from the result but in that case and given that I understand it correctly, we'll lose symbols from files that are preprocessed by a ppx. Is that acceptable?

[tatchi]

I stumbled upon that PR in the merlin repo which seems to be related to the same issue: ocaml/merlin#1219

The proposed fix looks pretty easy and consist of falling back to a corresponding .ml/.mli file. According to this comment, locations should be preserved.

I got curious and tried that solution. It seems to show good results:

Screen.Recording.2021-11-24.at.06.39.35.mov

[tatchi]

The fix has been merged upstream and is included in merlin 4.5 ocaml/merlin#1219

I know that there's an ongoing effort to unvendor Merlin so I assume that when that's done, we will have an up to date version of Merlin that contains the fix ? cc @rgrinberg

[rgrinberg]

We don't plan to unvendor merlin. Once I will update our fork of it, the fix should be included.

[tmattio]

We don't plan to unvendor merlin.

I thought we agreed that was the plan though?

The fork is making a lot of people's life more difficult, and merlin maintainers have agreed to upstream the patches in principle.

[rgrinberg]

I remember agreeing about reducing the patches but certainly not un vendoring merlin.

The difficulties imposed by the fork can be solved by deleting the vendored directory once all the patches are upstreamed.

[tmattio]

I'm confused. Do you mean that you intend to keep vendoring merlin even if all the patches are upstreamed?

[rgrinberg]

Exactly

[tatchi]

The fix has been merged upstream and is included in merlin 4.5 ocaml/merlin#1219

I think I've been confused. I don't think the upstream change would have fixed that issue (at least without any changes from our side). Anyway, good news is that #671 should fix the issue 🤗

[tatchi]

Closed #671