[tooling] Distinct uids for interfaces

[voodoos]

Uids are supposed to be unique inside a compilation unit. But if we consider that a compilation unit is made of an interface file and an implementation file, then this is not actually true: declarations are numbered with uids starting from 0 in both the interface and the implementation. This can be a source of confusion for external tools such as Merlin or the occurrences indexer.

This PR proposes annotating uids with their origin (Impl of Intf). To make the change less invasive I used a reference that defaults to Impl and is switched to Intf when Typemod.type_interface is called.

Additionally, to make the testsuite more stable, commit 238d54f sorts the declarations in ocamlobjinfo's printing... This is a noisy commit that should make future changes less noisy. Reviewing commit-by-commit should filter the noise.

This change is important for a PR that I plan to open soon that generalizes the cmt_format.cmt_value_dependencies field so that external tools can keep track of dependencies between declarations. This will allow Merlin to jump between ml and mli files, have the project-wide occurrences feature also return declarations, etc.

Pinging the people who were involved in the recent indexing work: @Octachron @gasche
CC @lpw25

[lpw25]

I think it would be better to store this information in the same place we store the current unit name (i.e. in Env). Possibly it would be worth creating a current_unit type containing the name and the intf_or_impl and then have mk take one of those and then have Env.get_current_unit return one of those and replace the vast majority of Env.get_unit_names with Env.get_current_unit. (Also maybe rename Env.get_unit_name to Env.get_current_unit_name for clarity).

[voodoos]

It seems like a reasonable thing to do, but I am going to face circular dependencies issues if the current_unit type is defined in Env and refered to in Shape.Uid. I dug a bit more and there might be an even more appropriate place for that type definition: the Unit_info module introduced by @Octachron. Currently, the unit_name in env is set from the corresponding field of Unit_info.t:

let with_info ~native ~tool_name ~source_file ~output_prefix ~dump_ext k =
  Compmisc.init_path ();
  let target = Unit_info.make ~source_file output_prefix in
  Env.set_unit_name (Unit_info.modname target);
  let env = Compmisc.initial_env() in
  let dump_file = String.concat "." [output_prefix; dump_ext] in
  Compmisc.with_ppf_dump ~file_prefix:dump_file @@ fun ppf_dump ->
  k {
    target;
    env;
    ppf_dump;
    tool_name;
    native;
  }

Maybe we could simply add the impl_or_intf field to the Unit_info.t and store all this in the Env reference: Env.set_unit_info target ?

[lpw25]

Sounds good to me

[voodoos]

I made changes in that direction in e05aae4

[gasche]

Ping @lpw25 : are you planning to do a second review iteration on this PR?

[lpw25]

I didn't really do a first pass, just looked at the part from the PR description that sounded like it could be improved. I'd be happy to review this PR but can't really commit to doing so promptly.

[Octachron]

I have a small interrogation, with the new source_kind this is the third argument that we pass to with_info which is only used to construct an Unit_info.t. Do we want to group together those arguments in a single unit_info argument.

[voodoos]

That seems reasonable, I will give it a go, we can always revert later if we're not satisfied.

[voodoos]

Done in 2f2b546

[Octachron]

I agree that keeping the kind of the current unit is useful information in general, even more so if it helps keeping uid unique.
The PR is ready to merge from my point of view.