-
Notifications
You must be signed in to change notification settings - Fork 89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Overhaul of module-type-of and shadowing #1081
Conversation
00e6024
to
9546508
Compare
I think I've resolved the issues we found during review, and also a couple of others:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
<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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's correct but slightly odd to link from Dep13.c
to Dep11.S.c
when there is a Dep13
page containing c
...
c9d234c
to
0dfa632
Compare
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.
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:
mark_substituted
andadd_canonical
from the resolution and expansion functionsremoved_items
intoLang.Signature.t
so it can be persisted instead of having to be recalculatedsig ... 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 .