Skip to content

fix: Make field types invariant for soundness #2539

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

Merged
merged 1 commit into from
Oct 20, 2022
Merged

Conversation

dcodeIO
Copy link
Member

@dcodeIO dcodeIO commented Oct 19, 2022

Discovered in #2537: #2158 introduced unsoundness by making class fields covariant, which is also incompatible with Wasm GC where mutable fields are required to be invariant.

As Wasm GC states:

Structure types support width and depth subtyping

  • struct <fieldtype1>* <fieldtype1'>* <: struct <fieldtype2>*
    • iff (<fieldtype1> <: <fieldtype2>)*

Field types are covariant if they are immutable, invariant otherwise

  • const <valtype1> <: const <valtype2>
    • iff <valtype1> <: <valtype2>
  • var <valtype> <: var <valtype>
  • Note: mutable fields are not subtypes of immutable ones, so const really means constant, not read-only

Example of the problem:

class Animal {
  sibling: Animal;
}
class Cat extends Animal {
  sibling: Cat; // unsound
}
class Dog extends Animal {
  sibling: Dog; // unsound
}

(<Animal>new Cat()).sibling = new Dog(); // Cat with Dog sibling is allowed by covariance but incorrect

This PR makes field types invariant (again), but keeps the ability to redeclare fields as long as types are invariant.

  • I've read the contributing guidelines
  • I've added my name and email to the NOTICE file

@dcodeIO dcodeIO requested a review from MaxGraey October 19, 2022 21:16
@dcodeIO dcodeIO merged commit ee6a165 into main Oct 20, 2022
@HerrCai0907 HerrCai0907 deleted the field-invariance branch October 17, 2023 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants