-
Notifications
You must be signed in to change notification settings - Fork 86
Syntactic support for instance names as identifiers #1873
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
Merged
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
To support parameterised libraries, it will help to sharpen the distinction between `Compilation_unit.Name.t` and `Compilation_unit.t`. In particular, a `CU.Name.t` will mean the name of a .cmi and a `CU.t` will mean the name of a .cmo/x. Accordingly, the `cmi_name` field in `Cmi_format` changes from `CU.t` to `CU.Name.t`. There is never a `CU.t` for a parameter module since it has no .cmo/x, so for these, the `CU.t` is removed from the `Cmi_format` altogether. A non-parameter .cmi still needs it, however, since we need to store the pack prefix for the implementation .cmo/x. (We don't support pack prefixes for parameter modules.) Accordingly, the `cmi_kind` field now has two variants: ``` type kind = | Normal of { cmi_impl : CU.t } | Parameter ``` As it happens, all this forces through a related change in `Import_info.t` so that we can store import info for a parameter module, which has a CRC but no compilation unit. Since the format of import info for interfaces and implementations is diverging, a split API is introduced to `Import_info`. (This is the same API that ocaml-flambda#1746 adds, but here it's optional and only used in the few places that need it.)
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.
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
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.
Declares that the module being compiled has a parameter, whose type is given by an .mli. Given `-parameter Param`, we require that: * An interface `param.mli` is in the path and was compiled with `-as-parameter` * All subsequent modules that refer to this module will also be compiled with `-parameter Param` Once support for parameterised libraries is complete, it will also be possible for a reference to pass an argument rather than forwarding `Param` along.
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`).
The library implements a simple little subsitution calculus based on dependent contextual modal type theory. The library is a functor parameterised over an identifiable and will calculate what a module's remaining parameters will look like when that module is referred to with some of its parameters given. Since parameters can have parameters, the correct behaviour here isn't obvious.
The profiler seems worth at least gesturing toward keeping current. I'm assuming we're just leaving upstream `objinfo.ml` to rot.
We're not sure how shapes are going to work anyway, so adding `Shape.of_global_name` seems premature.
lukemaurer
added a commit
that referenced
this pull request
Jul 18, 2024
The failure was in a backtrace with line numbers in the compiler. Fairly certain a better fix would be `git rm`.
Emacs helpfully removes the whitespace errors on the assumption that ocamlformat would never want to add them. This assumption is wrong (see comment in file).
riaqn
approved these changes
Sep 27, 2024
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.
LGTM - some small comments.
We will move the syntax to parsetree
in a future PR.
lukemaurer
added a commit
that referenced
this pull request
Oct 23, 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.
Depends on #1872.
Currently the only support is as a handwritten extension like
which isn't great, but initially we plan to support creating and referring to instances only using Dune, so this syntax will only see the light of day in source files generated by Dune and not meant for human consumption.