Add some tests for invalid subtyping due to non-matching field types #573
Conversation
Specifically around the requirements that both field types have the same mutability and that the storage type must be exactly the same when the field type is mutable.
| (module | ||
| ;; The mutability of fields must be the same. | ||
| (type $c (sub (struct (field (mut (ref null any)))))) | ||
| (type $d (sub $c (struct (field (ref null none))))) |
There was a problem hiding this comment.
Perhaps keep the actual storage types the same in this test (and the next one)?
There was a problem hiding this comment.
Perhaps keep the actual storage types the same in this test (and the next one)?
Whoops, yeah that was the intention, just a copy-paste issue. Will push a fix in a moment.
Just trying to avoid making test files too large, since they become harder to debug the larger they get, and we already have some |
| ;; When fields are const, a subtype's reference fields cannot be supertypes of | ||
| ;; the supertype's fields, they must be subtypes. | ||
| (type $a (sub (struct (field (mut (ref null none)))))) | ||
| (type $b (sub $a (struct (field (mut (ref null any)))))) |
There was a problem hiding this comment.
It looks like this test intends to have constant fields, but in fact it has mutable fields. Do we already test the case where the subtyping is correctly covariant, but the fields are mutable so it is still invalid?
There was a problem hiding this comment.
It looks like this test intends to have constant fields, but in fact it has mutable fields.
Whoops, another copy-paste bug.
Do we already test the case where the subtyping is correctly covariant, but the fields are mutable so it is still invalid?
Wasmtime and wasm-tools incorrectly allowed covariance on mutable fields but was still passing the test suite, which is what motivated me to make this PR and upstream these tests from Wasmtime.
|
Thanks for the approve @tlively! FWIW, I do not have merge permissions on this repo, so I can't merge this PR myself. |
|
@rossberg, what's the preferred workflow here? Do we merge the tests here and then merge gc into wasm-3.0? I know we've landed some tests directly in wasm-3.0 as well. Do we merge those back to the gc repo as well? |
|
I regularly go through the proposal repos and merge them into wasm-3.0, so new commits should eventually make it there. I haven't done any back-merging, because that is way to tedious, and at this point I'm not sure how much it still matters. |
|
Sounds good, I'll go ahead and merge this, then. |
Specifically around the requirements that both field types have the same mutability and that the storage type must be exactly the same when the field type is mutable.
@tlively PTAL, thanks!