dicej opened PR #4442 from component-dynamic-type
to main
:
This addresses #4310, introducing a new
component::values::Val
type for
representing component values dynamically. It also adds acall
method to
component::func::Func
, which takes a slice ofVal
s as parameters and returns
aResult<Val>
representing the result.As an implementation detail, I've also added a
component::values::Type
type,
which is an owned, despecialized version of
wasmtime_environ::component::InterfaceType
. That serves two purposes:
- It allows us to despecialize as the first step, which reduces the number of cases to consider when typechecking and lowering.
- It avoids needing to borrow the store both mutably and immutably when lowering, as we would if we used
InterfaceType
s.Finally, I happened to notice that the
ComponentType::SIZE32
calculation in
expand_record_for_component_type
needed a correction, so I did that.Signed-off-by: Joel Dice <joel.dice@fermyon.com>
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
dicej updated PR #4442 from component-dynamic-type
to main
.
dicej updated PR #4442 from component-dynamic-type
to main
.
dicej updated PR #4442 from component-dynamic-type
to main
.
jameysharp created PR review comment:
I don't love making a function named
_to_str
visible outside the current module. But I see all you have at the call site is a&StoreOpaque
so you can't callto_str
instead. Unless @alexcrichton has a better idea I guess this is fine.
jameysharp submitted PR review.
jameysharp created PR review comment:
Would it be reasonable to use
FlagsSize::from_count
everywhere instead of usingceiling_divide
directly?
jameysharp submitted PR review.
jameysharp created PR review comment:
If you got
count
fromType::Flags
here instead of fromVal::Flags
, I don't see that you'd need to store a count inVal::Flags
at all. It looks like everywhere that you need the count, you have the type available.I guess you did it this way to force the host application to specify how many flags they expect to be passing in as parameters, so you can verify in
typecheck
that their expectation was correct—and so you can inform the host application about how many flags they received.I go back and forth on whether that kind of checking is worth doing in this sort of dynamic call situation. @alexcrichton, what's your take?
jameysharp created PR review comment:
This is totally a nitpick but would you mind putting these cases in the same order as the enums they correspond to? I see
Type
andInterfaceType
have the primitive types in the same order, so this list is the only one that doesn't match.
jameysharp created PR review comment:
I think you can use
Iterator::try_for_each
here for a little more clarity.
dicej submitted PR review.
dicej created PR review comment:
Yeah, that felt awkward to me, too. Perhaps we could rename it to something like
to_str_with_store_opaque
? I haven't yet wrapped my head around all the different flavors of store, so perhaps there's a better option.
dicej updated PR #4442 from component-dynamic-type
to main
.
alexcrichton submitted PR review.
dicej updated PR #4442 from component-dynamic-type
to main
.
dicej updated PR #4442 from component-dynamic-type
to main
.
dicej updated PR #4442 from component-dynamic-type
to main
.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
While you're here and doing this could this also add variants for lifting
Vec<T>
fromWasmSlice<T>
and such?The macro could possibly look something like:
forward_lifts! { WasmStr => { Box<str>, Rc<str>, Arc<str>, String } WasmList<T> => { Box<[T]>, ...} }
(or similar)
alexcrichton created PR review comment:
Idiomatically we try to avoid
as
casts where possible since in isolation they could indicate that a lossy conversion is happening. Could the casts here be replaced withi32::from(*value)
? That should signify that no data is lost.
alexcrichton created PR review comment:
I think this
+ index
may not be right since each of thevalues
may take up more than oneValRaw
slot.It might make more sense for this function to take
&mut std::slice::Iter<'_, MaybeUninit<ValRaw>>
instead of the slice/offset it currently has perhaps?
alexcrichton created PR review comment:
I worry with
impl Iterator
that this will excessively monomorphize into callers, so could this use something concrete like&mut &[ValRaw]
or&mut std::slice::Iter<'_, ValRaw>
?
alexcrichton created PR review comment:
Could this have a preceding
debug_assert!
about alignment?
alexcrichton submitted PR review.
alexcrichton created PR review comment:
This seems like it may not be right for multiple arguments since the offset may always be zero?
alexcrichton created PR review comment:
I think this could delegate to
<str as Lower>::store
?
alexcrichton created PR review comment:
This is another case that I think is not quite right for subtle reasons. The main one is that unused
ValRaw
entries (and upper bits which are unused here) need to all be set to zero. Otherwise this runs the risk of leaking undefined values into wasm modules.Additionally this is another location that needs to inform the caller that a nonzero amount of
ValRaw
slots are consumed. Givenvalue
here you may also consume more slots thanvalue
takes up because of how variants are "flattened"
alexcrichton created PR review comment:
I think this is actually subtly incorrect because it would get the alignment incorrect for the call to
realloc
. The type of the list element may need to get passed in here.
alexcrichton created PR review comment:
I think that this may lead to unfortunately surprising results in that a string type is only considered equivalent if it comes from the same
types
origin but it's actually unique across all components. I think that this is sufficient for a "succeed fast" test but this may need to be expanded for deep equality checking as well.
alexcrichton created PR review comment:
GIven how many types are in this module and how
Type
isn't the only export I think it may actually make sense to have this be apub mod types
and don't reexportType
from inside. Otherwise external users can't name types likeField
and such.
alexcrichton created PR review comment:
I think these methods may make more sense on
List
(in the values module) along the lines of:impl List { pub fn new(elements: Box<[Val]>, ty: types::List) -> Result<List> { // ... } }
(using the different structure of types I mentioned above).
Then there could be
impl From<List> for Val
to convert to aVal
as necessary.
alexcrichton created PR review comment:
This doesn't necessarily have to be handled here, but I think this typecheck will eventually want to be more recursive for better error mesages. Right now I think you could get
expected record, got record
which doesn't describe which record fields mismatch or similar.
alexcrichton created PR review comment:
This is a bit bike-sheddy, but instead of
pub struct Handle<T> { ... } pub struct TupleIndex(TypeTupleIndex); pub enum Type { // ... Tuple(Handle<TupleIndex>), // ... }
could this instead be:
struct Handle<T> { ... } pub struct Tuple(Handle<TypeTupleIndex>); pub enum Type { // ... Tuple(Tuple), // ... }
That I think will allow us to be a bit more flexible in the future with internal representations ideally.
alexcrichton created PR review comment:
I think it would still be worth filling this in for other reasons though. For example all of the parameters to a host function are lifted which is where these could show up inline.
Eventually we'll want an equivalent to
crate::Linker::func_new
withincomponent::Linker
although it doesn't have to be added as part of that PR. When that's added this'll get exercised I believe.
alexcrichton created PR review comment:
One nice part of perhaps using
MaybeUninit<ValRaw>
is that you could delegate to theLower for T
implementations? Or otherwise I think it would be worthwhile figuring out how to delegate to that for primitives and strings/lists perhaps (may require an unsafe method or two to extractMaybeUninit<[ValRaw; 2]>
from&mut std::slice::Iter<'_, MaybeUninit<ValRaw>>
though.
alexcrichton created PR review comment:
I think that this is going to need special logic to skip extra fields within
src
to ensure thatflatten_count
entries are always popped off that iterator.
alexcrichton created PR review comment:
Is this still necessary? This seems like a bit of an odd query that I'm not sure what it would be used for
alexcrichton created PR review comment:
I think this organization also helps method discovery since today there's lots of impls for
Handle<T>
for differentT
but forTuple
there will be just the methods related to tuples.
dicej created PR review comment:
@alexcrichton Is there a convenient way to make a
&mut std::slice::Iter<'_, &mut MaybeUninit<ValRaw>>
from a&mut MaybeUninit<[ValRaw; N]
? I could do something like(0..N).map(|i| map_maybe_uninit!(dst[i])
, but I don't think that's what you meant.
dicej submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Not easily, but you can transmute
&mut MaybeUninit<[ValRaw; N]>
to&mut [MaybeUninit<ValRaw>; N]
"safely" and then.iter()
from there
dicej submitted PR review.
dicej created PR review comment:
I'm not sure I understand this.
Type::String
doesn't have aHandle<T>
, so this code wouldn't be reached in that case. Am I misunderstanding what you meant by "string type"?
dicej created PR review comment:
That's what I originally considered, but I ended up using these methods on
Type
instead in the interest of ergonomics. You can see them used in the dynamic.rs tests, along withType::nested
. This avoids forcing the programmer to write a bunch ofif let Type::List(ty) = ty { List::new(elements, ty) } else { unreachable!() }
.This is the ergonomic challenge I was referring to earlier in the discussion about whether
Val
s need to be associated with a specificType
. I'm certainly willing to reconsider; I just want to understand how you expect this API to be used vs how I'm using it in the dynamic.rs tests. Do you not expect real-world code to need a bunch ofif let ... { ... } else { unreachable!() }
when buildingVal
s fromType
s in the scheme you outlined?
dicej submitted PR review.
dicej submitted PR review.
dicej created PR review comment:
Please see my comment above about ergonomics and how it is used in the dynamic.rs tests.
dicej submitted PR review.
dicej created PR review comment:
Any e.g. record field mismatch would have been caught already in
Type::new_record
before we got this far. The only time you'd seeexpected record, got record
would be if you usedVal
s from one component in another, or if your component had two identicalrecord
s (i.e. same field names and types) and you mixed up which one you created theVal
for. Either way, more recursion wouldn't necessarily help. This is somewhat analogous to whenrustc
reports a type mismatch betweenfoo::Bar
andfoo::Bar
because you're using two different versions of thefoo
crate. It's even worse here since types don't really have names in the component model.But yes, I agree we should give a friendlier diagnostic here, e.g. hinting based on whether
Handle::index
orHandle::types
are different, or both.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Ah sorry bad example, but instead I think this would consider
(u32, u32)
not an equivalent type if it comes from two different components.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Ah ok thanks for pointing that out! I think this may be a case where I'm used to slightly more verbose Rust-based idioms perhaps? For example the
Type::new_list
API as-is can return two kinds of errors (not a list type or element of the wrong type) where what I would otherwise expect is to split those two error cases across two functions. Instead of:let input = ty.new_list(Box::new([ Val::U32(32343), Val::U32(79023439), Val::U32(2084037802), ]))?;
I'd expect:
let input = ty.unwrap_list().new(Box::new([ Val::U32(32343), Val::U32(79023439), Val::U32(2084037802), ]))?;
(or similar)
I don't mean to say we should go all the way to writing a bunch of wordy
if let
but I think helper methods can help scope precisely what errors are coming out of each function. In this caseunwrap_list
would be a clear indication of "I expect this type to be a list" and thenew
error would be clearly "I expect each element to be of this type".
dicej updated PR #4442 from component-dynamic-type
to main
.
dicej created PR review comment:
@alexcrichton any further concerns about this method? I'm going to mark this "resolved" for now, but feel free to unresolve it.
dicej submitted PR review.
dicej updated PR #4442 from component-dynamic-type
to main
.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Could this be replaced with a
debug_assert!
that thememory
is the same as the store that this slice originally came from?
alexcrichton submitted PR review.
alexcrichton created PR review comment:
This is a minor nit, but idiomatically I'd expect that
values::List::new
would also exist as a function and this function would simply bevalues::List::new(values, self)
(or something like that). This is just random API additions though so happy to defer to future PRs.
alexcrichton created PR review comment:
Would it be possible to avoid this and only have the version that takes
StoreOpaque
? I'm otherwise worried about making this method too general because the indexes are only guaranteed to be valid for the original memory slice and if one day we remove the bounds checks in release mode (which we should be able to do now I'm just being conservative) it would be memory unsafe to pass in an arbitrary slice here.
alexcrichton created PR review comment:
Is
Lift + Lower
needed here? Ideally I think justLift
would suffice for a trait bound
alexcrichton created PR review comment:
Unfortunately we can't rely on the
.len()
reported from anExactSizeIterator
with unsafe Rust. Given that we have an unsafe guarantee that aVal
is always valid with respect to its type I think that the length will need to be checked after the fields are collected rather than before.
dicej submitted PR review.
dicej created PR review comment:
This is what I get when I remove the
+ Lower
:error[E0277]: the trait bound `T: component::func::typed::Lower` is not satisfied --> crates/wasmtime/src/component/func/typed.rs:536:31 | 536 | unsafe impl <T: Lift> Lift for $a { | ^^^^ the trait `component::func::typed::Lower` is not implemented for `T` ... 550 | / forward_list_lifts! { 551 | | Box<[T]>, 552 | | std::rc::Rc<[T]>, 553 | | std::sync::Arc<[T]>, 554 | | Vec<T>, 555 | | } | |_- in this macro invocation | note: required because of the requirements on the impl of `component::func::typed::Lower` for `[T]` --> crates/wasmtime/src/component/func/typed.rs:1006:16 | 1006 | unsafe impl<T> Lower for [T] | ^^^^^ ^^^ note: required because of the requirements on the impl of `component::func::typed::ComponentType` for `Box<[T]>` --> crates/wasmtime/src/component/func/typed.rs:475:37 | 475 | unsafe impl <$($generics)*> ComponentType for $a { | ^^^^^^^^^^^^^ ... 504 | / forward_impls! { 505 | | (T: Lower + ?Sized) &'_ T => T, 506 | | (T: Lower + ?Sized) Box<T> => T, | | ^^^^^^ 507 | | (T: Lower + ?Sized) std::rc::Rc<T> => T, ... | 510 | | (T: Lower) Vec<T> => [T], 511 | | } | |_- in this macro invocation note: required by a bound in `component::func::typed::Lift` --> crates/wasmtime/src/component/func/typed.rs:444:32 | 444 | pub unsafe trait Lift: Sized + ComponentType { | ^^^^^^^^^^^^^ required by this bound in `component::func::typed::Lift` = note: this error originates in the macro `forward_list_lifts` (in Nightly builds, run with -Z macro-backtrace for more info) help: consider restricting type parameter `T` | 536 | unsafe impl <T: component::func::typed::Lower Lift> Lift for $a { | +++++++++++++++++++++++++++++ ...
alexcrichton created PR review comment:
Oh that bug is here where right now there's
<T: Lower> ComponentType for Box<T>
where that should beT: ComponentType
. I was trying to be "too clever" by shoving it all into one macro but I think it can be split by refactoring the forward macro into multiple macros
alexcrichton submitted PR review.
dicej updated PR #4442 from component-dynamic-type
to main
.
dicej submitted PR review.
dicej created PR review comment:
I've improved the error message so you'll never get
expected record, got record
. I attempted to go much further than that and recursively compare the types, but reusingtyped::typecheck_tuple
etc. turned out to be difficult in a dynamic context, and I didn't want to duplicate them either.Also, I'm having trouble finding guidance in https://github.com/WebAssembly/component-model about when interface types are to be considered equal and when not, so I'm not sure what the correct logic is anyway. Are types considered equal if they are structurally identical?
dicej submitted PR review.
dicej created PR review comment:
I've address this for tuple, option, and expected. Not sure if I should do the same for e.g. record, though. See my comment in the other conversation about type equality.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Idiomatically I think it's fine to call these constructors
new
rather thantry_new
sincetry_*
is typically only used if the base name is already a method elsewhere (and otherwise there's noOption::new
)
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Would it be possible to move
lift
/load
to theVal
module and keep all the internals of all theVal
private (e.g. notpub(crate)
)?
alexcrichton created PR review comment:
I think that the tuple/option/expected bits are probably not going to be necessary in the long-term if the original issue here is fixed. I think this is fine to leave as a FIXME with a follow-up issue for now though.
Otherwise though the current logic in tuple/option/expected I would expect to be baked in here or otherwise into some trait impl on
T
perhaps. I don't think we'd want to duplicate things and have some usingPartialEq
and some using manual impls.
alexcrichton created PR review comment:
I don't think we'll get a ton of guidance from the component model itself, but for now from Wasmtime's perspective given how trampolines and lifting/lowering/etc are all implemented we require exact structural equality for the embedder API. I think the intention is that subtyping would eventually be used but we don't have a great means of implementing that right now.
dicej updated PR #4442 from component-dynamic-type
to main
.
alexcrichton merged PR #4442.
Last updated: Dec 23 2024 at 12:05 UTC