-
Notifications
You must be signed in to change notification settings - Fork 26
Supporting vtables for closures #872
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
base: main
Are you sure you want to change the base?
Conversation
TODO: fix the `translate_vtable_shim` part, then remove the "//@ known-failure" in the test `simple/dyn-fn` example
|
@protz Could you please help us approving the CI procedure? |
|
@sonmarcho @Nadrieril would you mind approving the CI for us? |
|
Done |
charon/tests/ui/simple/dyn-fn.out
Outdated
| thread 'rustc' panicked at src/bin/charon-driver/translate/translate_trait_objects.rs:941:17: | ||
| internal error: entered unreachable code | ||
| note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace | ||
| error: Thread panicked when extracting item `test_crate::gives_fn::{closure#0}`. | ||
| --> tests/ui/simple/dyn-fn.rs:8:15 | ||
| | | ||
| 8 | takes_fn(&|counter| { | ||
| | ^^^^^^^^^ | ||
|
|
||
| ERROR Charon failed to translate this code (1 errors) |
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.
This is not acceptable in a PR. If a PR regresses a test like this you must indicate it in the PR description and explain why you think it is justified to accept this regression. In this case it's a panic, which at the very least should be changed to emit e rpoper error message (but most likely should be fixed instead).
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.
Sorry for that. This panic occurs because Fn* Traits are builtin traits, which are marked opaque and not translated by default. I fixed this and now charon can translate closures.
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.
However, a new panic appears when translating dyn-fn.rs. The error message is as follows:
error: Trying to generate a vtable shim for a non-vtable-safe method
--> tests/ui/simple/dyn-fn.rs:8:15
|
8 | takes_fn(&|counter| {
| _______________^
9 | | *counter += 1;
10 | | true
11 | | })
| |_____^
This panic happened in function translate_vtable_shim, which assumes that the input impl_func_def is FullDefKind::AssocFun. However, when translating a closure, impl_func_def as a FullDefKind::Closure is passed. So the new problem is that FullDefKind::Closure does not have fields vtable_sig and sig, which are offered by FullDefKind::AssocFun. Should we fcollect these information in charon, or in hax?
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.
I see, this should be added in hax
charon/src/bin/charon-driver/translate/translate_trait_objects.rs
Outdated
Show resolved
Hide resolved
charon/src/bin/charon-driver/translate/translate_trait_objects.rs
Outdated
Show resolved
Hide resolved
charon/src/bin/charon-driver/translate/translate_trait_objects.rs
Outdated
Show resolved
Hide resolved
charon/src/bin/charon-driver/translate/translate_trait_objects.rs
Outdated
Show resolved
Hide resolved
charon/src/bin/charon-driver/translate/translate_trait_objects.rs
Outdated
Show resolved
Hide resolved
| fn generate_closure_method_shim_ref( | ||
| &mut self, | ||
| span: Span, | ||
| impl_def: &hax::FullDef, | ||
| closure_kind: ClosureKind, | ||
| ) -> Result<ConstantExprKind, Error> { |
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.
translate_fn_ptr doesn't work here?
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.
I'd prefer this method to be in translate_closures also
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.
translate_fn_ptrdoesn't work here?
We tried translate_fn_ptr and pass the impl_def of closure. However, error occurs when executing ``translate_fn_ptr->translate_fun_item` -> `translate_generic_args`, reporting “Unexpected error: could not find" type variables `closurekind` and `upvars`.
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.
I'd prefer this method to be in translate_closures also
moved into translate_closures
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.
I looked into it, that's an inconsistency in hax actually. I fixed it in latest hax, translate_fn_ptr looks like it works now (the generics still need to be fixed up though of course).
charon/src/bin/charon-driver/translate/translate_trait_objects.rs
Outdated
Show resolved
Hide resolved
|
This overall looks good, the one issue is the new panic. In the future please don't submit PRs with such regressions without explaining them. Also just so you know the PR description was not useful information: you don't need to say what you did, I will read that in the code; instead a PR description is useful for context and for why you did things a certain way. FYI LLMs are terrible at generating useful PR descriptions as far as I've seen, please avoid that in the future. |
| }); | ||
| ConstantExprKind::Ref(global) | ||
| } else if self | ||
| .translate_vtable_struct_ref(span, impl_expr.r#trait.hax_skip_binder_ref())? |
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.
You can't skip binders like that, this will result in errors if the binder is not trivial.. Please use translate_region_binder then erase the resulting charon::RegionBinder.
| let shim_id: FunDeclId = self.register_item( | ||
| span, | ||
| impl_def.this(), | ||
| TransItemSourceKind::VTableMethod(TraitImplSource::Closure(closure_kind)), | ||
| ); | ||
|
|
||
| let mut generics = Box::new(self.the_only_binder().params.identity_args()); |
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.
Please use the appropriate translate_closure_bound_ref_etc method here instead of most of this function
This PR adds support for vtable instantiation for closures.
The implementation involves two main steps:
Matching a closure implementation
Instantiating the corresponding vtable
Step 1 is implemented in
translate_body.rs, while Step 2 is implemented intranslate_trait_objects.rs.The modifications in each file are described below.
translate_rvalueSupports registering closure trait implementations. For example, in the test file
dyn-fn.rs, the closure|counter| { ... }is matched in thehax::ImplExprAtom::Builtincase and registered as aVTableInstance.add_method_to_vtable_defAdds additional checks for
FnOncewhen constructing the method field in theFnOncevtable, due to the incompatibility ofFnOnce::call_oncewith dyn-compatibility.get_vtable_instance_infoSupports retrieving the vtable struct reference and the corresponding trait declaration reference for closures, based on their kind (Fn, FnMut, or FnOnce).
add_supertraits_to_vtable_valueSupports filling vtable fields with built-in supertraits. For closures supertraits, they are translated as VTableInstance with
TraitImplSource::Closure(kind), generating a vtable according to the closurekind. For other built-in supertraits, they are translated as VTableInstance withTraitImplSource::Normal, whose trait reference itself is used as the impl reference (e.g.@2 := &core::marker::MetaSized::{vtable}).gen_vtable_instance_init_bodySupports generating the vtable body for closures. Closure implementations are treated as trait implementations with no associated items and a single specialized closure method (
callorcall_mut).call_onceis ignored due to the incompatibility mentioned above. Based on this design, the function retrieves(trait_pred, items)for closures and usesmk_fieldto add the specialized closure method.generate_closure_method_shim_refImplements the generation of the closure method shim reference used in the
mk_fieldingen_vtable_instance_init_body.