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.

"Overhaul of module-type-of and shadowing"

@jonludlam

CHANGES: ### Highlight - Hierarchical documentation (@jonludlam, @panglesd, @Julow) Pages can now be organized in a directory tree structure. Relative and absolute references are added: `{!./other_page.label}`, `{!//other_page}`. - Improved sidebar and breadcrumbs navigation (@panglesd, @gpetiot) The documentation pages and the libraries of the entire package are shown on the left sidebar. - Added support for images, videos, audio and other assets The syntax is `{image!/reference/to/asset}` or `{image:URL}` for images. The syntax for `{video...}` and `{audio...}` is the same. (@panglesd, @EmileTrotignon, ocaml/odoc#1170, ocaml/odoc#1171, ocaml/odoc#1184, ocaml/odoc#1185) - Search using Sherlodoc (@panglesd, @EmileTrotignon, @Julow) A new search bar that supports full-text and type-based search. ### Added - Experimental driver (@jonludlam, @panglesd) The driver builds the documentation for a collection of Opam packages using the newer Odoc features. It supports linking external packages to ocaml.org and markdown files. This is experimental and will break in the future. - Cross-package references (@panglesd, @Julow) Pages and modules from other packages can be referenced: `{!/otherpackage/page}`, `{!/otherpackage/Module.t}`. - Option to remap links to other packages to ocaml.org or other site. See the `--remap` option of the driver or the `--remap-file` option of `odoc html-generate`. (@jonludlam, ocaml/odoc#1189, ocaml/odoc#1248) - Option to compute occurrences of use of each identifiers The commands `aggregate-occurrences` and `count-occurrences` are added. (@panglesd, ocaml/odoc#976, ocaml/odoc#1076, ocaml/odoc#1206) - Added the `odoc classify` command (@jonludlam, ocaml/odoc#1121) Helps driver detecting which modules belong to which libraries. - Added `--suppress-warnings` to the CLI to remove warnings from a unit, even if they end up being raised in another unit through expansion (@jonludlam, ocaml/odoc#1260) - Add clock emoji before `@since` tag (@yawaramin, ocaml/odoc#1089) - Navigation for the search bar : use '/' to enter search, up and down arrows to select a result, and enter to follow the selected link. (@EmileTrotignon, ocaml/odoc#1088) - Fix a big gap between the preamble and the content of a page (@EmileTrotignon, ocaml/odoc#1147) - Add a marshalled search index consumable by sherlodoc (@EmileTrotignon, @panglesd, ocaml/odoc#1084) - Allow referencing of polymorphic constructors in polymorphic variant type aliases (@panglesd, ocaml/odoc#1115) - Added a home icon in the breacrumbs (@panglesd, ocaml/odoc#1251) It can be disabled with a CLI option. - Add a frontmatter syntax for mld pages (@panglesd, ocaml/odoc#1187, ocaml/odoc#1193, ocaml/odoc#1243, ocaml/odoc#1246, ocaml/odoc#1251) Allows to specify the title of a page, the order of sub-pages and other behaviors in the sidebar. - Added `odoc-md` to process standalone Markdown pages (@jonludlam, ocaml/odoc#1234) ### Changed - The command line interface changed to support the new features. + Packages and libraries: `odoc link` must now be aware of packages and libraries with the `-L libname:path` and `-P pkgname:path` options. The module search path should still be passed with the `-I` option. The current package should be specified with `--current-package=pkgname`. + Hierarchy: `odoc compile` now outputs `.odoc` in the directory tree specified with `--output-dir=DIR` and the parent identifier must be specified with `--parent-id=PARENT`. The option `--source-parent-file` is removed. + Source code: Implementations are compiled with `compile-impl` instead of with `compile`. The options `--cmt=..` and `--source-name=..` are removed. Source code pages are generated with `html-generate-source`. + Assets: The commands `compile-asset`, `html-generate-asset` are added. The option `html-generate --asset` is removed. + Sidebar: The index is built using `compile-index`. The sidebar data is extracted from the index with `sidebar-generate` and passed to `html-generate --sidebar=..`. - The syntax for `@tag` is now delimited (@panglesd, ocaml/odoc#1239) A `@tag` can now be followed by a paragraph or other elements. - Updated colors for code fragments (@EmileTrotignon, ocaml/odoc#1023) - Fixed complexity of looking up `.odoc` files (@panglesd, ocaml/odoc#1075) - Normalize whitespaces in codespans (@gpetiot, ocaml/odoc#1085) A newline followed by any whitespaces is normalized as one space character. - Reduce size of `Odoc_html_frontend` when compiled to javascript (@EmileTrotignon, ocaml/odoc#1072) - Overhaul of module-type-of expansions and shadowing code (@jonludlam, ocaml/odoc#1081) - Output file paths and labels in the man and latex backends changed to avoid name clashes (@Julow, ocaml/odoc#1191) ### Fixed - Fix variant constructors being hidden if they contain hidden types (@jonludlam, ocaml/odoc#1105) - Fix rare assertion failure due to optional parameters (@jonludlam, ocaml/odoc#1272, issue ocaml/odoc#1001) - Fix resolution of module synopses in {!modules} lists that require --open (@jonludlam, ocaml/odoc#1104} - Fix top comment not being taken from includes often enough (@panglesd, ocaml/odoc#1117) - Fixed 404 links from search results (@panglesd, ocaml/odoc#1108) - Fixed title content not being picked up across pages when rendering references (ocaml/odoc#1116, @panglesd) - Fix wrong links to standalone comments in search results (ocaml/odoc#1118, @panglesd) - Remove duplicated or unwanted comments with inline includes (@Julow, ocaml/odoc#1133) - Fix bug where source rendering would cause odoc to fail completely if it encounters invalid syntax (@jonludlam ocaml/odoc#1208) - Add missing parentheses in 'val (let*) : ...' (@Julow, ocaml/odoc#1268) - Fix syntax highlighting not working for very large files (@jonludlam, @Julow, ocaml/odoc#1277)

@jonludlam

CHANGES: - Hierarchical documentation (@jonludlam, @panglesd, @Julow) Pages can now be organized in a directory tree structure. Relative and absolute references are added: `{!./other_page.label}`, `{!//other_page}`. - Improved sidebar and breadcrumbs navigation (@panglesd, @gpetiot) The documentation pages and the libraries of the entire package are shown on the left sidebar. - Added support for images, videos, audio and other assets The syntax is `{image!/reference/to/asset}` or `{image:URL}` for images. The syntax for `{video...}` and `{audio...}` is the same. (@panglesd, @EmileTrotignon, ocaml/odoc#1170, ocaml/odoc#1171, ocaml/odoc#1184, ocaml/odoc#1185) - Search using Sherlodoc (@panglesd, @EmileTrotignon, @Julow) A new search bar that supports full-text and type-based search. - Experimental driver (@jonludlam, @panglesd) The driver builds the documentation for a collection of Opam packages using the newer Odoc features. It supports linking external packages to ocaml.org and markdown files. This is experimental and will break in the future. - Cross-package references (@panglesd, @Julow) Pages and modules from other packages can be referenced: `{!/otherpackage/page}`, `{!/otherpackage/Module.t}`. - Option to remap links to other packages to ocaml.org or other site. See the `--remap` option of the driver or the `--remap-file` option of `odoc html-generate`. (@jonludlam, ocaml/odoc#1189, ocaml/odoc#1248) - Option to compute occurrences of use of each identifiers The commands `aggregate-occurrences` and `count-occurrences` are added. (@panglesd, ocaml/odoc#976, ocaml/odoc#1076, ocaml/odoc#1206) - Added the `odoc classify` command (@jonludlam, ocaml/odoc#1121) Helps driver detecting which modules belong to which libraries. - Added `--suppress-warnings` to the CLI to remove warnings from a unit, even if they end up being raised in another unit through expansion (@jonludlam, ocaml/odoc#1260) - Add clock emoji before `@since` tag (@yawaramin, ocaml/odoc#1089) - Navigation for the search bar : use '/' to enter search, up and down arrows to select a result, and enter to follow the selected link. (@EmileTrotignon, ocaml/odoc#1088) - Fix a big gap between the preamble and the content of a page (@EmileTrotignon, ocaml/odoc#1147) - Add a marshalled search index consumable by sherlodoc (@EmileTrotignon, @panglesd, ocaml/odoc#1084) - Allow referencing of polymorphic constructors in polymorphic variant type aliases (@panglesd, ocaml/odoc#1115) - Added a home icon in the breacrumbs (@panglesd, ocaml/odoc#1251) It can be disabled with a CLI option. - Add a frontmatter syntax for mld pages (@panglesd, ocaml/odoc#1187, ocaml/odoc#1193, ocaml/odoc#1243, ocaml/odoc#1246, ocaml/odoc#1251) Allows to specify the title of a page, the order of sub-pages and other behaviors in the sidebar. - Added `odoc-md` to process standalone Markdown pages (@jonludlam, ocaml/odoc#1234) - The command line interface changed to support the new features. + Packages and libraries: `odoc link` must now be aware of packages and libraries with the `-L libname:path` and `-P pkgname:path` options. The module search path should still be passed with the `-I` option. The current package should be specified with `--current-package=pkgname`. + Hierarchy: `odoc compile` now outputs `.odoc` in the directory tree specified with `--output-dir=DIR` and the parent identifier must be specified with `--parent-id=PARENT`. The option `--source-parent-file` is removed. + Source code: Implementations are compiled with `compile-impl` instead of with `compile`. The options `--cmt=..` and `--source-name=..` are removed. Source code pages are generated with `html-generate-source`. + Assets: The commands `compile-asset`, `html-generate-asset` are added. The option `html-generate --asset` is removed. + Sidebar: The index is built using `compile-index`. The sidebar data is extracted from the index with `sidebar-generate` and passed to `html-generate --sidebar=..`. - The syntax for `@tag` is now delimited (@panglesd, ocaml/odoc#1239) A `@tag` can now be followed by a paragraph or other elements. - Updated colors for code fragments (@EmileTrotignon, ocaml/odoc#1023) - Fixed complexity of looking up `.odoc` files (@panglesd, ocaml/odoc#1075) - Normalize whitespaces in codespans (@gpetiot, ocaml/odoc#1085) A newline followed by any whitespaces is normalized as one space character. - Reduce size of `Odoc_html_frontend` when compiled to javascript (@EmileTrotignon, ocaml/odoc#1072) - Overhaul of module-type-of expansions and shadowing code (@jonludlam, ocaml/odoc#1081) - Output file paths and labels in the man and latex backends changed to avoid name clashes (@Julow, ocaml/odoc#1191) - Fix variant constructors being hidden if they contain hidden types (@jonludlam, ocaml/odoc#1105) - Fix rare assertion failure due to optional parameters (@jonludlam, ocaml/odoc#1272, issue ocaml/odoc#1001) - Fix resolution of module synopses in {!modules} lists that require --open (@jonludlam, ocaml/odoc#1104} - Fix top comment not being taken from includes often enough (@panglesd, ocaml/odoc#1117) - Fixed 404 links from search results (@panglesd, ocaml/odoc#1108) - Fixed title content not being picked up across pages when rendering references (ocaml/odoc#1116, @panglesd) - Fix wrong links to standalone comments in search results (ocaml/odoc#1118, @panglesd) - Remove duplicated or unwanted comments with inline includes (@Julow, ocaml/odoc#1133) - Fix bug where source rendering would cause odoc to fail completely if it encounters invalid syntax (@jonludlam ocaml/odoc#1208) - Add missing parentheses in 'val (let*) : ...' (@Julow, ocaml/odoc#1268) - Fix syntax highlighting not working for very large files (@jonludlam, @Julow, ocaml/odoc#1277)

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