Skip source occurrences under [@merlin.hide] (#1456)

[klakplok]

Fixes #1456.

In rendered source pages, ppx-generated code can produce spurious source links, even on tokens that aren't identifiers at all. For instance on Core.Char the type keyword, t and char are clickable and jump to unrelated definitions.

The cause is that derivers sometimes emit code that borrows the source location of the type declaration it's derived from (or parts of it). Those locations are not marked ghost, so odoc collects occurrences for them, and emits links to their definitions. The result on the example above is even more confusing because these links are nested, triggering the auto-closing semantics of links by the browser.

In this PR, we honour the [@merlin.hide] attribute: ppx_deriving wraps its output in include struct ... end [@@merlin.hide], so we prune those subtrees when collecting occurrences. AFAIU, this is the same signal Merlin and ppxlib use to skip generated code. We do this in addition to ghost locations (who mark single nodes, not subtrees), not as a replacement.

Comes with a regression test for the raw mechanism, not the end-to-end behaviour using ppx deriver to limit dependencies. I initially wrote such an end-to-end test, but it was difficult to make it compatible with both plain OCaml and OxCaml CI environments so I dropped it. I did check manually that the patch works on both an OCaml switch (5.4) with ppx_deriving and an OxCaml switch (5.2.0) with ppx_jane.

Disclosure: this PR was made with AI assistance. The investigation and the chosen solution are mine; the regression test was written by Claude and the fix itself is a mix. I'm also new to this codebase, so there may well be a more obvious or idiomatic fix I've missed, happy to be pointed at one.

I don't think this is urgent, I did this as an exercise to dive into the codebase, but since source display is enabled on ocaml.org, it's probably worth doing if my fix does not look too stupid.

[panglesd]

Thanks for the PR!

I wonder why we need both merlin.hide and ghost locations. But if both merlin and ppx_deriving use them, and ppx_deriving does not use ghost locations, clearly we should support them!

I'll review the implementation soon.

[klakplok]

I wonder why we need both merlin.hide and ghost locations. But if both merlin and ppx_deriving use them, and ppx_deriving does not use ghost locations, clearly we should support them!

The current way of marking and then detecting ghost generated code in the OCaml pipeline is not very satisfying indeed, this patch does not really try to fix the situation, it just attempts to mimic Merlin's behaviour and follow ppxlib's check semantics to properly interprete the current output of derivers.

Both markings have a different scope: marking a node as ghost only marks the node itself, whereas marking a node as merlin.hide marks the node and its descendants. They also don't mean the exact same thing: one says "this is generated code" the other says "this should be ignored my merlin". For our purpose, the meaning is the same though: ignore.

In theory, only ghost should suffice, if ppx properly marked all generated code as ghost and produced consistent locations. Because it is more precise, ghost is also mode powerful, for instance you could have a ppx turning [%time x] into let t0 = time () in let res = x in print (time () -. t0) ; res, and in that case it's valid to set all the code as ghost, except x that could be kept non ghost and with its original location. Marking the generated code as merlin.hide does not allow for such a non ghost leaf inside a ghost tree.

In practice some derivers generate code that is marked non ghost. They also reuse code locations from various sub-nodes of the input and don't follow proper nesting. Both are not on purpose and should be considered violations of the ppxlib good practices [1]. However, deriving will wrap the code they generate in a merlin.hide node, so that's acceptable, as the good practices (that are enforced by a check in ppxlib) also supports merlin.hide.

[1]: see the manual for ppxlib section on good practices

[panglesd]

Looks good to me. (And thanks for the explanation!)

(If you want to improve the code, maybe you could split the "iterator that does not iterate on merlin.hide" part, and the "iterator that calls the Analysis module" part:

let rec restricted_iterator next_iterator = 
  let module_declaration iterator ({ Typedtree.md_attributes; _ } as md) =
    if not_hidden md_attributes then next_iterator.module_declaration iterator md
  in
  ...

let our_iterator = <the iterator that was defined before>

let final_iterator = restricted_iterator our_iterator

Not sure it is worth it, just a remark!)

[klakplok]

If you want to improve the code, maybe you could split the "iterator that does not iterate on merlin.hide" part, and the "iterator that calls the Analysis module" part.

Right, I'll look into this. Ideally, this code would eventually be shared with Merlin somehow, so splitting may help.