Refactor Persistent_env to separate imports from bound names
#1764
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
With the advent of parameters, we have reasons to refer to global module names besides those names being runtime global variables, or addressable values at all. In addition, we're going to want to load a .cmi but then parameterise it in different ways to get different signatures, each of which will be bound to a separate value. In the end, then, instead of just `pers_struct`, we'll want three different record types, each with an associated cache: * `import`, corresponding directly to a .cmi file, keyed by `Compilation_unit.Name.t` * `pers_name`, corresponding to an `import` with parameters applied to it, keyed by a `Compilation_unit.Name.t` and some parameters * `pers_struct`, corresponding to a persistent name that is actually bound in the environment, with the same key as `pers_name` For this PR, I'm leaving out the second cache as nothing about it is relevant yet, but the split is disruptive enough that even the two are worthwhile. Note that `pers_struct` is currently still keyed by `Compilation_unit.Name.t` since we don't yet have the datatypes for parameterised names. Besides the internal reorganisation of `Persistent_env`, this also changes the API between it and `Env`, offloading some of the work done by the callback, `read_sign_of_cmi`. Significantly, `read_sign_of_cmi` is no longer called _at all_ until something is going to be bound (i.e., become a `pers_struct`). This keeps the `Env.t` completely free of anything it shouldn't know about.
lukemaurer
added a commit
to lukemaurer/flambda-backend
that referenced
this pull request
Aug 31, 2023
I've disabled the check on the output, since currently we get the wrong error message (and the one we get is confusing). This will be much easier to fix when PR ocaml-flambda#1764 is fixed to avoid unhelpful checks on `.mli` files that are loaded directly rather than as part of name resolution.
lukemaurer
added a commit
to lukemaurer/flambda-backend
that referenced
this pull request
Aug 31, 2023
I've disabled the check on the output, since currently we get the wrong error message (and the one we get is confusing). This will be much easier to fix when PR ocaml-flambda#1764 is fixed to avoid unhelpful checks on `.mli` files that are loaded directly rather than as part of name resolution.
This eliminates a silly bit of back-and-forth, and `Persistent_env` is perfectly capable of knowing the shape.
This lets us fulfil the intent that we don't call the callback for things that aren't actually bound in the environment. That way we don't do awkward things like calculate a binding for something that doesn't actually have a binding.
lukemaurer
added a commit
to lukemaurer/flambda-backend
that referenced
this pull request
Sep 27, 2023
I've disabled the check on the output, since currently we get the wrong error message (and the one we get is confusing). This will be much easier to fix when PR ocaml-flambda#1764 is fixed to avoid unhelpful checks on `.mli` files that are loaded directly rather than as part of name resolution.
lukemaurer
added a commit
to lukemaurer/flambda-backend
that referenced
this pull request
Oct 19, 2023
Displaying the correct error message will be easier again after ocaml-flambda#1764
lpw25
approved these changes
Dec 2, 2023
lukemaurer
added a commit
that referenced
this pull request
Dec 8, 2023
* Add `-as-parameter` option This allows compiling an .mli as a _parameter module_ rather than a normal compilation unit. A parameter module defines a module _type_ rather than a module, so it cannot be referred to directly from another module. A forthcoming PR will add the `-parameter P` option which adds the parameter module `P` as a parameter to the current module, which then allows references to `P` in the module. Further PRs will deal with how to actually use a module that takes parameters. For the moment, `-parameter` is unimplemented, so any reference to a parameter module is an error. * Add checks for misuse of `-as-parameter` * Raise error on combination of `-as-parameter` and `-for-pack` * Add test for check for compiling `.ml` of parameter `.mli` I've disabled the check on the output, since currently we get the wrong error message (and the one we get is confusing). This will be much easier to fix when PR #1764 is fixed to avoid unhelpful checks on `.mli` files that are loaded directly rather than as part of name resolution. * Add test of check for `-as-parameter` on implementation * Implement `register_parameter_import` and `is_registered_parameter_import` * Code review * Code review * Fix error message and re-enable test
lpw25
approved these changes
Apr 22, 2024
With module information cached at multiple levels, we need to make sure that the visibility check happens no matter which cache is hit.
lukemaurer
added a commit
to lukemaurer/flambda-backend
that referenced
this pull request
Oct 23, 2024
* Add `-as-parameter` option This allows compiling an .mli as a _parameter module_ rather than a normal compilation unit. A parameter module defines a module _type_ rather than a module, so it cannot be referred to directly from another module. A forthcoming PR will add the `-parameter P` option which adds the parameter module `P` as a parameter to the current module, which then allows references to `P` in the module. Further PRs will deal with how to actually use a module that takes parameters. For the moment, `-parameter` is unimplemented, so any reference to a parameter module is an error. * Add checks for misuse of `-as-parameter` * Raise error on combination of `-as-parameter` and `-for-pack` * Add test for check for compiling `.ml` of parameter `.mli` I've disabled the check on the output, since currently we get the wrong error message (and the one we get is confusing). This will be much easier to fix when PR ocaml-flambda#1764 is fixed to avoid unhelpful checks on `.mli` files that are loaded directly rather than as part of name resolution. * Add test of check for `-as-parameter` on implementation * Implement `register_parameter_import` and `is_registered_parameter_import` * Code review * Code review * Fix error message and re-enable test
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.
Depends on #1753.
With the advent of parameters, we have reasons to refer to global module names besides those names being runtime global variables, or addressable values at all. In addition, we're going to want to load a .cmi but then parameterise it in different ways to get different signatures, each of which will be bound to a separate value.
In the end, then, instead of just
pers_struct, we'll want three different record types, each with an associated cache:import, corresponding directly to a .cmi file, keyed byCompilation_unit.Name.tpers_name, corresponding to animportwith parameters applied to it, keyed by aCompilation_unit.Name.tand some parameterspers_struct, corresponding to a persistent name that is actually bound in the environment, with the same key aspers_nameFor this PR, I'm leaving out the second cache as nothing about it is relevant yet, but the split is disruptive enough that even the two are worthwhile. Note that
pers_structis currently still keyed byCompilation_unit.Name.tsince we don't yet have the datatypes for parameterised names.Besides the internal reorganisation of
Persistent_env, this also changes the API between it andEnv, offloading some of the work done by the callback,read_sign_of_cmi. Significantly,read_sign_of_cmiis no longer called at all until something is going to be bound (i.e., become apers_struct). This keeps theEnv.tcompletely free of anything it shouldn't know about.