Add -as-argument-for parameter
#1841
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently broken since the `Persistent_env` refactor doesn't have quite the right abstractions to keep us from calling `Env.read_sign_of_cmi` with something we're not going to bind in the environment
Having it now with a weird type is better than having several weird lines now (or, that is, soon, when more functions use `print_global_line`).
lukemaurer
changed the base branch from
main
to
lmaurer/layered-persistent_env
This both makes `test.ml` _far_ easier to maintain and will also make testing native compilation easy without copying everything. Pulled from the 'instantiate' branch, commit fab77b5.
Pulled from the 'instantiate' branch, commit 454de0d.
Pulled from 'instantiate' branch, commit a9f9196.
(Which became a compilation error in the big rebase.)
Looks like something messed with the line endings in error messages.
This makes the code output for Closure not complete gibberish. Unfortunately, it's fatally flawed, as it causes the .cmx for an argument module to be full of zeros (which will immediately be mutated to their actual values, but Closure isn't aware of this). This probably isn't worth actually fixing, since (a) we don't plan to keep Closure around much longer in ocaml-jst, and (b) this whole double-module-block design probably needs to be revisited.
lukemaurer
force-pushed
the
as-argument-for
branch
from
85599a3 to
987a2bd
Compare
|
This probably needs reworking. The implementation currently turns every argument module into a two-field record, the first field of which is the usual module structure and the second field of which is the record with the exact type for the module. This isn't great: everything that accesses a module needs to go through an indirection. That's not as bad as it sounds because (a) we optimize and (b) the mapping of Mark suggested eons ago that we could take the coercion from the full module to its argument type and write the Closure code for it to a special field of the record (we'd just need to write a lambda that applies |
Previously, an argument module would have a special format for its module block, with exactly two fields: the module block that would normally be there, and a second module block whose type matches the parameter exactly. Effectively, this commit unboxes the first field, and also instead of storing the argument form of the module directly, it stores the coercion from the module to its parameter type as a function. The downside of this is that there won't be a fixed location for the argument form of the module, since it will be recomputed each time the module is used as an argument, but in practice this shouldn't matter often since the computation will happen at compile time and the only circumstance in which sharing would matter would be if the parameter is used as a first-class module. Storing the argument form directly in this way could be tricky in cases where the coercion is trivial, since then the module block will have a circular reference to itself as one of its own fields. We could solve this by, rather than passing the module block itself, rebuilding a copy of the module block and passing that to the coercion.
This is simpler and guarantees we never waste space by constructing the argument block more than once. It's still a bit wasteful if the coercion is trivial (i.e., if the module's type has all the same values as the parameter it implements) but this seems unlikely to matter in practice.
lukemaurer
force-pushed
the
as-argument-for
branch
from
6c9fecd to
6d22bc4
Compare
lukemaurer
added a commit
that referenced
this pull request
Jul 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This was not in fact merged. Please burn every reference to this PR and salt the earth. The new PR is #2177.
Depends on #1764.
Add the ability to compile a module declaring that it is suitable to pass as the argument to a module taking a given parameter. Essentially the module is now checked against two different signatures (its own .mli and the parameter, which of course must be a supertype).