[tooling] Remember linked declarations

[voodoos]

This PR generalizes a mechanism that is used to store "value dependencies" in cmt files introduced in 10abdce. It was meant to "track in external tools value declarations between implementations and interfaces".

There are two differences in these changes:

1. We only store the paired uids and not the entire declaration. The declaration can be retrieved from the corresponding cmt_uid_to_decl table (this might require loading a .cmti file when the uid is from an interface).
2. We not only store the paired values, but also modules, modules types, types, etc.

This information will enable external tools to navigate across the declaration graphs of any value, module, type, etc.
Additionally this information can be used to build equivalence classes of declarations. Enabling external tools to perform powerful refactoring by retrieving every usage of every definition whose uids are in the same equivalence class.

This PR compares uids to prevent noisy self-dependencies to be recorded (similarly to what record_value_dependency is doing). Since right now uids are not unique across a compilation unit this means that some links between an implementation uid and an interface uid with the same id are missing. Commit 7f5111b shows the expected test results with #13286 merged.

@alainfrisch I think you were the one to introduce value_dependencies which I remove in ec2cc6c. Do you now if that feature is actively used today ? I made a search on Sherlocode that return very few results. One of them is dead_code_analyser which is only available for OCaml < 4.05. The other looks active and compatible with 5.2: reanalyze. It might be safer to keep both for a while to give time for users to transition ?

cc @lpw25

[Octachron]

@gasche , would you mind having a look at this part of the PR to check if it is understandable? (since the authorship of this part of the PR has become fuzzy between me and @voodoos )

[gasche]

I wonder what is the current exchange rate between Merlin PRs and Pattern-Matching Bug PRs.

I'll have a look.

[gasche]

I had a quick look. My comments:

• I don't know what Directed | Undirected in Cmt_format mean, some documentation comment (with an example!) would help.

• Looking at includemod.ml and includemod.mli, the first time we encounter this idea of "positivity" is in the mark type, which is scarcely documented, so it's a bit cryptic. I learned more from the implementation comment on type positivity in the .ml file, not necessarily because it is very didactic (it focuses on explaining a subtle trick) but because it has meat and something that serves as an example. I wouldn't mind an explanation of this notion of positivity/direction higher in one of the file, with a small example.

• I don't understand why mark and positivity are two separate mechanisms to trace directions. Maybe they evolve differently for good (unexplained) reasons? Maybe they are partly redundant with each other? Who knows.

• Strictly_positive is not documented anywhere that I can see, but my best guess is that it is in fact a combination of Positive and a separate at_toplevel : bool field being true. I suspect that the separate-field presentation would be cleaner.

• (see inline comment about check_modtype_equiv which I found supsicious)

[alainfrisch]

Do you now if that feature is actively used today ?

I confirm we still use it as part of our internal version of dead_code_analyzer (not synced with the public version), but don't let this block progress! I didn't follow developments around Uid, but I guess we should be able to switch to using them instead of declarations.

[Octachron]

@gasche , the positivity/negativity information is the usual concept of positive/negative position for subtyping used for parameter variance. In particular, strict positivity is the same in both case sand denotes the absence of double negations.

For modules and Merlin, this information is useful because it corresponds to the only case where the subtyping relationship is matching implementation item with their inferred types to interface item (compared to the evenly-nested funtor argument cases

module F= functor(_:sig
  module G: functor(_:sig
      val non_strict_pos: int end
    end ) -> sig end
 end) -> struct  let strict_pos = 0 end

)

We have added a new commit which better separates the semantics of the marking condition and the positivity tracking.
Moreover, all information related to "directionality" of the subtyping relationship is now grouped in a single record.

With this refactoring is seemed clearer to simplify the mark argument used in various function into a boolean a remove the mark type from the interface of Includemod. A possible alternative would be to expose the directionality type and its four possible starting values.

[gasche]

I read the new commit that moves "mark" and "positivity" to be in a common submodule, and I like the new result better. Plus you did write some extra documentation, which is nice, thanks! I think that the current state is okay, as far as understandability by others is concerned.

[gasche]

I think you mean [pos] rather than [positivity]. It is unclear to me how the [pos] type "distinguishes between directed and undirected edges". (Or what this means in the first place. I suppose the "edges" are the dependencies, but what does it mean for a dependency to be undirected? Maybe this is better explained somewhere else, but then you could refer to that place.)

Edit: in fact maybe you don't need to document cmt_declaration_dependencies here anymore: now that the pos information is necessarily to correctly "mark" the submodules, it is used for strictly more than that.

[gasche]

I read this sentence as "inside (module types which allows optimizing module type equality checks)", as if certain module types allow this and others don't. I would rephrase this, for example as follows:

The [in_eq] field is [true] when we are checking one direction of module type equality, rather than subtyping.

[gasche]

This function does not strike me as something that clearly should be in this module. (It is not obvious why the notion of direction of module-checking would depend on the notion of dependency edges in the cmt format.) Given that it is only used once that I can see, maybe you could move the logic to the caller, which is still inside includemod.ml but not inside Directionality anymore. Later one maybe we would move the logic further away and change the dependency structures between these modules -- or maybe not.

[Octachron]

With @gasche agreeing with the refactoring inside Includemod, the PR looks fine to me. I propose to move forward and merge this PR (and cherry-pick it to 5.3) once we agree on the names for the edge labels in the dependency table.