Overhaul of module-type-of and shadowing #1081

jonludlam · 2024-02-08T23:29:13Z

This PR contains a relatively big overhaul of how we do module-type-of expansions in particular, but also how we reuse previously calculated expansions for other module type expressions too. In doing so some other changes were required:

Remove the booleans mark_substituted and add_canonical from the resolution and expansion functions
Separate the concepts of shadowed and hidden identifiers
Add removed_items into Lang.Signature.t so it can be persisted instead of having to be recalculated
Simplify the representation of sig ... end with .. (not strictly required but it's another potential source of exponential size problems)

Note that, despite the name of the branch, this hasn't fixed the issue that the module type of expressions aren't quite right, though their expansions are correct. A little bit more logic is required to assess whether we need to show an ascription operation.

This PR includes the content of PRs #1079 and #1078 .

src/model/lang.ml

src/xref2/cpath.ml

src/xref2/subst.ml

src/xref2/tools.ml

src/model/names.ml

src/loader/cmt.ml

src/loader/ident_env.cppo.ml

src/model/names.ml

src/xref2/link.ml

src/xref2/tools.ml

test/xref2/expansion.t/run.t

src/xref2/tools.ml

jonludlam · 2024-03-12T13:37:47Z

I think I've resolved the issues we found during review, and also a couple of others:

I've fixed the infinite loop that @lukemaurer found in async_kernel
I've removed all of the MTOInvalidated stuff in subst as this is no longer needed.

panglesd

Although my expertise on this area is not very high, I approve this PR! It looks good to me.
It fixes a problem with module type expansions, and the other modifications in this PR are also good improvements to the codebase.
Thanks @jonludlam for the work!

panglesd · 2024-04-02T08:15:51Z

test/generators/html/Ocamlary.html

-      <span> = <a href="Ocamlary-Dep13-class-c.html">Dep13.c</a></span>
+      <span> = 
+       <a href="Ocamlary-Dep11-module-type-S-class-c.html">Dep13.c</a>
+      </span>


It's correct but slightly odd to link from Dep13.c to Dep11.S.c when there is a Dep13 page containing c...

Since the change to compile where we incrementally build up the environment rather than populating it before processing anything, we have expansions for all module type expressions in our env. These expansions are calculated in the context of the original expression, so any `module type of` expressions will have their expansions filled in. This means that any subsequent expression that uses a signature containing a `module type of` expression will have access to that expansion. Criticially this includes the situation where we are modifying the signature via a `with` clause. For example, given the following signature: module type S = sig module X : sig type t end module type Y = module type of struct include X end end module T : S module X2 : sig type t = int type u end module type T = module type of T with module X := X2 by the time we get to the line `module type T ... ` we've already processed module `T` and put it in the environment, complete with expansion. This expansion has, in turn, an expansion for `module type Y`, which looks like this: sig type t = X.t end We then do the destructive substitution removing `X` from the signature of `T` and replacing all instances of it with `X2` in our expansion. This includes both the `X` in `module type Y = module type of struct include X end` and the `X` in its expansion `type t = X.t`. Thus our expansion of `T` looks like: sig module type Y = module type of struct include X2 end end and the expansion of Y is: sig type t = X2.t end Note that this is _inconsistent_, because if we were to really calculate the expansion of `module type of struct include X2 end`, we'd get: sig type t = X2.t type u = X2.u end but the expansion of `Y` actually agrees with what OCaml thinks it should be. The consequence of this is that we no longer need to calculate the expansions of all `module type of` expressions before doing anything else. This commit removes the code that did that and also removes the problematic issue that keeping these explicit `module type of` expansions meant having expansions in the 'unexpanded module type' type, which was leading to the sort of size/space explosions that are relatively easy to trigger. What this commit does _not_ address is the inconsistency mentioned above. This was a pre-existing problem and should be addressed - most likely by adjusting the description to be something like: module type Y = module type of struct include (X2 <: S.X) end Using the notation for transparent ascription mentioned in a Jane Street blog post [here](https://blog.janestreet.com/plans-for-ocaml-408/). Note that the path `S.X` is actually projecting from a module type rather than a module, and isn't actually a valid path!

This commit preserves shadowed names across transitions to and from component.ml types from Lang types.

The Names module now distinguishes between an element that's hidden because it was found in between (**/**) pairs, or contains a double underscore, and elements that are shadowed because they came from an `include` and were subsequently redeclared. This change is required by the change to the shadowing logic, as we still need to be able to lookup hidden items by name.

This allows us to reuse computed signatures in the `With` case where we need to keep track of removed items.

Now that the removed items are stored we no longer need to recalcuate signatures, and can simply use the precalculated ones.

We were already rendering it with `sig .. end` and this change means there's less danger of accidentally storing multiple copies of a signature in an odoc file.

These were causing us to have to recalculate resolved paths and expansions just because one of these booleans was flipped. The first, mark_substituted, is required to give a slightly better result when dealing with opaque module types, but removing it didn't seem to change anything at all in the docs of `core` or its dependencies. The second we handle by stripping off the canonical constructor where we don't want it.

It would lead to an infinite loop. Also adds a test for the problem.

1. Recognise string names that are shadowed as such 2. Don't do shadow substitutions inside include declarations 3. Ensure shadowed names are globally unique

Also contains a fix for source-id - currently a source id requires a container page as parent. This was previously 'working' by creating a parent page called "./" which is obviously not great. This change requires that the source id have a non-empty parent, which we may want to change at some point.

jonludlam mentioned this pull request Feb 17, 2024

Failed to resolve module path identifier #1083

Open

jonludlam commented Feb 19, 2024

View reviewed changes

src/model/lang.ml Outdated Show resolved Hide resolved

jonludlam commented Feb 19, 2024

View reviewed changes

src/xref2/cpath.ml Show resolved Hide resolved

jonludlam commented Feb 19, 2024

View reviewed changes

src/xref2/subst.ml Outdated Show resolved Hide resolved

jonludlam commented Feb 19, 2024

View reviewed changes

src/xref2/tools.ml Outdated Show resolved Hide resolved

jonludlam commented Feb 19, 2024

View reviewed changes

src/xref2/tools.ml Outdated Show resolved Hide resolved

jonludlam commented Feb 19, 2024

View reviewed changes

src/xref2/tools.ml Show resolved Hide resolved

jonludlam commented Feb 19, 2024

View reviewed changes

src/model/names.ml Show resolved Hide resolved