Skip to content
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.

Sign up for GitHub

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

Fix wasmtime::FieldType::matches for subtyping and mutability #9724

Merged
merged 1 commit into from
Dec 3, 2024
Merged

Fix wasmtime::FieldType::matches for subtyping and mutability #9724

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions crates/wasmtime/src/runtime/type_registry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1235,11 +1235,16 @@ impl TypeRegistry {
}

/// Is type `sub` a subtype of `sup`?
#[inline]
pub fn is_subtype(&self, sub: VMSharedTypeIndex, sup: VMSharedTypeIndex) -> bool {
if sub == sup {
return true;
}

self.is_subtype_slow(sub, sup)
}

fn is_subtype_slow(&self, sub: VMSharedTypeIndex, sup: VMSharedTypeIndex) -> bool {
// Do the O(1) subtype checking trick:
//
// In a type system with single inheritance, the subtyping relationships
Expand Down
46 changes: 29 additions & 17 deletions crates/wasmtime/src/runtime/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1362,7 +1362,14 @@ impl StorageType {
/// Panics if either type is associated with a different engine from the
/// other.
pub fn eq(a: &Self, b: &Self) -> bool {
a.matches(b) && b.matches(a)
match (a, b) {
(StorageType::I8, StorageType::I8) => true,
(StorageType::I8, _) => false,
(StorageType::I16, StorageType::I16) => true,
(StorageType::I16, _) => false,
(StorageType::ValType(a), StorageType::ValType(b)) => ValType::eq(a, b),
(StorageType::ValType(_), _) => false,
}
}

pub(crate) fn comes_from_same_engine(&self, engine: &Engine) -> bool {
Expand Down Expand Up @@ -1457,8 +1464,21 @@ impl FieldType {
/// Panics if either type is associated with a different engine from the
/// other.
pub fn matches(&self, other: &Self) -> bool {
(other.mutability == Mutability::Var || self.mutability == Mutability::Const)
&& self.element_type.matches(&other.element_type)
// Our storage type must match `other`'s storage type and either
//
// 1. Both field types are immutable, or
//
// 2. Both field types are mutable and `other`'s storage type must match
// ours, i.e. the storage types are exactly the same.
use Mutability as M;
match (self.mutability, other.mutability) {
// Case 1
(M::Const, M::Const) => self.element_type.matches(&other.element_type),
// Case 2
(M::Var, M::Var) => StorageType::eq(&self.element_type, &other.element_type),
// Does not match.
_ => false,
}
}

/// Is field type `a` precisely equal to field type `b`?
Expand Down Expand Up @@ -1676,13 +1696,9 @@ impl StructType {
pub fn matches(&self, other: &StructType) -> bool {
assert!(self.comes_from_same_engine(other.engine()));

// Avoid matching on structure for subtyping checks when we have
// precisely the same type.
if self.type_index() == other.type_index() {
return true;
}

Self::fields_match(self.fields(), other.fields())
self.engine()
.signatures()
.is_subtype(self.type_index(), other.type_index())
}

fn fields_match(
Expand Down Expand Up @@ -1940,13 +1956,9 @@ impl ArrayType {
pub fn matches(&self, other: &ArrayType) -> bool {
assert!(self.comes_from_same_engine(other.engine()));

// Avoid matching on structure for subtyping checks when we have
// precisely the same type.
if self.type_index() == other.type_index() {
return true;
}

self.field_type().matches(&other.field_type())
self.engine()
.signatures()
.is_subtype(self.type_index(), other.type_index())
}

/// Is array type `a` precisely equal to array type `b`?
Expand Down
121 changes: 87 additions & 34 deletions tests/all/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,13 @@ fn field(heap_ty: HeapType) -> FieldType {
)
}

fn imm_field(heap_ty: HeapType) -> FieldType {
FieldType::new(
Mutability::Const,
StorageType::ValType(RefType::new(true, heap_ty).into()),
)
}

fn valty(heap_ty: HeapType) -> ValType {
ValType::Ref(RefType::new(true, heap_ty))
}
Expand Down Expand Up @@ -81,25 +88,57 @@ fn basic_struct_types() -> Result<()> {
fn struct_type_matches() -> Result<()> {
let engine = Engine::default();

let super_ty = StructType::new(&engine, [field(HeapType::Func)])?;
let super_ty = StructType::with_finality_and_supertype(
&engine,
Finality::NonFinal,
None,
[imm_field(HeapType::Func)],
)?;

// Depth.
let sub_ty = StructType::new(&engine, [field(HeapType::NoFunc)])?;
let sub_ty = StructType::with_finality_and_supertype(
&engine,
Finality::Final,
Some(&super_ty),
[imm_field(HeapType::NoFunc)],
)?;
assert!(sub_ty.matches(&super_ty));
let not_sub_ty = StructType::new(&engine, [imm_field(HeapType::NoFunc)])?;
assert!(!not_sub_ty.matches(&super_ty));

// Width.
let sub_ty = StructType::new(&engine, [field(HeapType::Func), field(HeapType::Extern)])?;
let sub_ty = StructType::with_finality_and_supertype(
&engine,
Finality::Final,
Some(&super_ty),
[imm_field(HeapType::Func), imm_field(HeapType::Extern)],
)?;
assert!(sub_ty.matches(&super_ty));
let not_sub_ty = StructType::new(
&engine,
[imm_field(HeapType::Func), imm_field(HeapType::Extern)],
)?;
assert!(!not_sub_ty.matches(&super_ty));

// Depth and width.
let sub_ty = StructType::new(&engine, [field(HeapType::NoFunc), field(HeapType::Extern)])?;
let sub_ty = StructType::with_finality_and_supertype(
&engine,
Finality::Final,
Some(&super_ty),
[imm_field(HeapType::NoFunc), imm_field(HeapType::Extern)],
)?;
assert!(sub_ty.matches(&super_ty));
let not_sub_ty = StructType::new(
&engine,
[imm_field(HeapType::NoFunc), imm_field(HeapType::Extern)],
)?;
assert!(!not_sub_ty.matches(&super_ty));

// Not depth.
// Unrelated structs.
let not_sub_ty = StructType::new(&engine, [imm_field(HeapType::Extern)])?;
assert!(!not_sub_ty.matches(&super_ty));
let not_sub_ty = StructType::new(&engine, [field(HeapType::Extern)])?;
assert!(!not_sub_ty.matches(&super_ty));

// Not width.
let not_sub_ty = StructType::new(&engine, [])?;
assert!(!not_sub_ty.matches(&super_ty));

Expand All @@ -109,29 +148,41 @@ fn struct_type_matches() -> Result<()> {
#[test]
fn struct_subtyping_fields_must_match() -> Result<()> {
let engine = Engine::default();

let a = StructType::with_finality_and_supertype(
&engine,
Finality::NonFinal,
None,
[field(HeapType::Any)],
[imm_field(HeapType::Any)],
)?;

for (expected, fields) in [
// Missing field.
(false, vec![]),
// Non-matching field.
(false, vec![field(HeapType::Extern)]),
// Exact match is okay.
(true, vec![field(HeapType::Any)]),
// Subtype of the field is okay.
(true, vec![field(HeapType::Eq)]),
// Extra fields are okay.
(true, vec![field(HeapType::Any), field(HeapType::Extern)]),
for (msg, expected, fields) in [
("Missing field", false, vec![]),
(
"Non-matching field",
false,
vec![imm_field(HeapType::Extern)],
),
("Wrong mutability field", false, vec![field(HeapType::Any)]),
("Exact match is okay", true, vec![imm_field(HeapType::Any)]),
(
"Subtype of the field is okay",
true,
vec![imm_field(HeapType::Eq)],
),
(
"Extra fields are okay",
true,
vec![imm_field(HeapType::Any), imm_field(HeapType::Extern)],
),
] {
let actual =
StructType::with_finality_and_supertype(&engine, Finality::NonFinal, Some(&a), fields)
.is_ok();
assert_eq!(expected, actual);
assert_eq!(
expected, actual,
"expected valid? {expected}; actually valid? {actual}; {msg}"
);
}

Ok(())
Expand Down Expand Up @@ -263,16 +314,18 @@ fn array_subtyping_field_must_match() -> Result<()> {
&engine,
Finality::NonFinal,
None,
field(HeapType::Any),
imm_field(HeapType::Any),
)?;

for (expected, field) in [
// Non-matching field.
(false, field(HeapType::Extern)),
(false, imm_field(HeapType::Extern)),
// Wrong mutability field.
(false, field(HeapType::Any)),
// Exact match is okay.
(true, field(HeapType::Any)),
(true, imm_field(HeapType::Any)),
// Subtype of the field is okay.
(true, field(HeapType::Eq)),
(true, imm_field(HeapType::Eq)),
] {
let actual =
ArrayType::with_finality_and_supertype(&engine, Finality::NonFinal, Some(&a), field)
Expand Down Expand Up @@ -322,61 +375,61 @@ fn array_subtyping() -> Result<()> {
&engine,
Finality::NonFinal,
None,
field(HeapType::Any),
imm_field(HeapType::Any),
)?;
let a = ArrayType::with_finality_and_supertype(
&engine,
Finality::NonFinal,
Some(&base),
field(HeapType::Any),
imm_field(HeapType::Any),
)?;
let b = ArrayType::with_finality_and_supertype(
&engine,
Finality::NonFinal,
Some(&base),
field(HeapType::Eq),
imm_field(HeapType::Eq),
)?;
let c = ArrayType::with_finality_and_supertype(
&engine,
Finality::NonFinal,
Some(&a),
field(HeapType::Any),
imm_field(HeapType::Any),
)?;
let d = ArrayType::with_finality_and_supertype(
&engine,
Finality::NonFinal,
Some(&a),
field(HeapType::Eq),
imm_field(HeapType::Eq),
)?;
let e = ArrayType::with_finality_and_supertype(
&engine,
Finality::NonFinal,
Some(&c),
field(HeapType::Any),
imm_field(HeapType::Any),
)?;
let f = ArrayType::with_finality_and_supertype(
&engine,
Finality::NonFinal,
Some(&e),
field(HeapType::Any),
imm_field(HeapType::Any),
)?;
let g = ArrayType::with_finality_and_supertype(
&engine,
Finality::NonFinal,
None,
field(HeapType::Eq),
imm_field(HeapType::Eq),
)?;
let h = ArrayType::with_finality_and_supertype(
&engine,
Finality::NonFinal,
Some(&g),
field(HeapType::Eq),
imm_field(HeapType::Eq),
)?;
let i = ArrayType::with_finality_and_supertype(
&engine,
Finality::NonFinal,
Some(&h),
field(HeapType::Eq),
imm_field(HeapType::Eq),
)?;

for (expected, sub_name, sub, sup_name, sup) in [
Expand Down
Loading