-
Notifications
You must be signed in to change notification settings - Fork 22
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
Engine: rework global name representation #1199
Conversation
9cba2cd
to
69b4062
Compare
1fca9c3
to
f21a709
Compare
I get a crash on the following example: mod a {
#[derive(PartialEq/* , Eq, Copy, Clone, Debug */)]
struct Test();
pub fn g(){super::b::h()}
}
mod b {
pub fn h(){super::a::g()}
} Here is the error message (with a change to make the ident readable):
|
f21a709
to
4030b42
Compare
I pushed a fix to that (removing dummy methods in opaque trait impls), and to another small problem. The next problem I have can be reproduced with: mod a {
struct Test {
r: i32
}
pub fn g(){super::b::h()}
}
mod b {
enum Test {
A(i32)
}
pub fn h(){super::a::g()}
} Open this code snippet in the playground We discussed this and the problem is that the internal representation of paths within modules carries information about which kind of items they represent. So the ident of |
I fixed all the bugs I found. I tested on sandwich which now type-checks when extracted with this branch (it even works without patch). For libcrux we need a few changes to the raw fstar that I have put in cryspen/libcrux#774. |
Libcrux CI succeeded with this PR: https://github.com/cryspen/libcrux/actions/runs/13048916135/job/36405261000. I just have one thread opened about a comment, I think we should merge soon. |
5d4a7ba
to
fca7b74
Compare
I reverted #1075 as part of this branch to avoid conflicts. The corresponding issue will still be fixed thanks to the new naming. |
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.
Generally, the changes look fine to me. Let' merge, but first also ask Lasse to take a quick look if he has time?
@cmester0, this PR is changing the names within hax, and this will probably break things in the Coq or SSProve backends and/or examples. |
9834cdc
to
051c4eb
Compare
The ml-kem job failed in the merge queue. The reason is that the dependency to |
This PR implements #1163.
It is expected most of the issues in #1135.
This PR is about:
Motivation
The previous design for global identifiers assumed every identifiers came from Rust. This was true at that time.
Since then, we shifted from that. The engine now creates names in the following situations:
The assumption that all the identifiers come from Rust is thus now completly broken, hence the need to move out from the current design.
A second motivation is the fact Rust identifiers were coming without any "kind" information: in the engine, we were not able to state whether a given identifier was, say, a struct or a function.
This was a big problem when printing names in the backends: the name of, e.g., a type cannot be printed with the same logic as, e.g., a constant.
Previous design
A concrete identifier in hax was basically a tuple containing:
In the previous design, we added incrementally hooks into that raw rust DefId representation so that we could alter the identifiers. This was providing us a way to create new identifiers.
Printing names
Rust have a very flexible namespacing.
Modules are just one way of doing namespacing: item declaration is allowed in expression bodies, thus items can be nested arbitrarily.
It is possible e.g. to define a module within an anonymous const within a method implementation.
The logic for printing name was very complicated and hard to maintain or to fix.
New design
Frontend
#1198 made the frontend output a definition kind along with any Rust definition identifier. Before that, #1054 added a parent information for each Rust defition identifier.
Together, those two PRs makes the frontend output, for each defintiion identifiers: (1) the full chain of identifiers up to the crate root (2) a precise definition kind, informing us precisely about the definition.
Engine
Representation
Now the engine has a representation for concrete identifiers in three layers:
DefId
type, generated from Rust to OCaml, defined by the frontendExplicit_def_id
: wraps a rust raw identifier, adding a metadata to disambiguate types and constructor (see the documentation of this module for more details)Concrete_ident
: aExplicit_def_id
that can be moved to a fresh module name and/or have a hygenic suffixConcrete_ident
is an type that describes an eventual need of freshness: the underlying explicit def id are never touched. BeforeConcrete_ident
, every identifier comes from Rust.The freshness of concrete identifier is guaranteed lazily: when one request the rendering of a concrete identifier, then the engine will produce a stable but fresh name.
View
Working with raw rust identifier is difficult, espcially for rendering identifiers as string in the backends.
Rust represents identifiers as a crate and a path. Each chunk of the path of an Rust identifier is roughly a level of nest in Rust. The path lacks informations about definition kinds.
There is two kinds of nesting for items.
Instead, the view transform each path as a list of smaller relational paths. For instance, consider the following piece of code:
Here, the Rust raw definition identifier of
LocalStruct
is roughlya::my_crate::<Impl 0>::assoc_fn::LocalStruct::field
.The view for
LocalStruct
looks like:Such a hierachical path approach makes printing names much easier under the following constraints:
Progress
kind
field toDefIdContents
#1198Explicit_def_id
)Concrete_ident_view_types
)Concrete_ident_view
)Concrete_ident
>Fresh_module
)concrete_ident
#793 and [META] Engine: enhancements to the concrete ident view API #973)Concrete_ident
)tests
folderexamples
foldermod
andfn
s #1136v_...
instead oft_...
#1156t_
in rec bundle #1169rec
qualifier in a mutually recursive top-levellet
#1158hax_lib::fstar::before
in recursive bundles #1179Review status
@karthikbhargavan and @maximebuyse, you can already take a look at the all modified OCaml modules in the subdirectoy
engine/lib/concrete_idents
.