fitzgen requested alexcrichton for a review on PR #9277.
fitzgen requested wasmtime-core-reviewers for a review on PR #9277.
fitzgen opened PR #9277 from fitzgen:refactor-store-trait
to bytecodealliance:main
:
This dates back from the
wasmtime
vswasmtime-runtime
crate split. Now that those crates merged, we can simplify some bits and remove unnecessary abstractions and indirections.
fitzgen updated PR #9277.
fitzgen submitted PR review.
fitzgen created PR review comment:
Note: This can't use the
DerefMut
impl, and must instead explicitly callstore_opaque_mut()
, because mutable borrows are invariant over type parameters.
fitzgen updated PR #9277.
fitzgen edited PR review comment.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
I'm not sure I understand this, can you expand on this a bit? Is this something where we're accidentally handing out
'static
? Or something is acidentally required to be'static
when it shouldn't be? Should the deref impls becomeimpl Deref for dyn (Store + '_)
or something like that?
fitzgen submitted PR review.
fitzgen created PR review comment:
Here is the error, fwiw:
error: lifetime may not live long enough --> crates/wasmtime/src/runtime/vm/table.rs:622:13 | 554 | store: &mut dyn VMStore, | - let's call the lifetime of this reference `'1` ... 622 | store.unwrap_gc_store_mut(), | ^^^^^ requires that `'1` must outlive `'static` | = note: requirement occurs because of a mutable reference to `dyn VMStore` = note: mutable references are invariant over their type parameter = help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance
My understanding is:
- We have a
store: &mut dyn VMStore
- We are implicitly reborrowing it, in an attempt to get a shorter lifetime, by using the
DerefMut
impl at this call site- However, because mutable references are invariant over their type parameter, we don't actually end up with a shorter lifetime, but the same lifetime
- And then for reasons I don't fully understand, that ends up forcing a
'static
constraint. I think because that is the only way that we could unify those two lifetimes? Not sure.FWIW, making the reborrow explicit with
(&mut *store).unwrap_gc_store_mut()
results in the same error message, but pointing at the explicit reborrow instead of the method invocation.I don't know if adding a
T: 'static
on the impl would solve the problem or not. But also we don't require that forStore::new
so I don't think it is really an option.Quite possible I am misunderstanding something here, but at the end of the day: without the
.store_opaque_mut()
we get a compiler error, with it we don't.
fitzgen merged PR #9277.
Last updated: Dec 23 2024 at 12:05 UTC