Validate supertypes after isorecursive canonicalization (#4506)

Validating nominally declared supertypes depends on being able to test type
equality, which in turn depends on the compared types having already been
canonicalized. Previously we validated supertypes before canonicalization, so
validation would fail in cases where it should have succeeded. Fix the bug by
canonicalizing first. This means that the global type store can now end up
holding invalid types, but those types will never be exposed to the user, so
that's not a huge problem.

Also fix an unrelated bug that was preventing the test from passing, which is
that supertypes were being hashed and compared without the special logic for
detecting self-references. This bug preventing the equivalent recursion groups
in the test from being deduplicated.

Author: tlively

src/wasm/wasm-type.cpp

@@ -2444,7 +2444,10 @@ size_t RecGroupHasher::hash(const TypeInfo& info) const {
 
 size_t RecGroupHasher::hash(const HeapTypeInfo& info) const {
   assert(info.isFinalized);
-  size_t digest = wasm::hash(uintptr_t(info.supertype));
+  size_t digest = wasm::hash(bool(info.supertype));
+  if (info.supertype) {
+    hash_combine(digest, hash(HeapType(uintptr_t(info.supertype))));
+  }
   wasm::rehash(digest, info.kind);
   switch (info.kind) {
     case HeapTypeInfo::BasicKind:
@@ -2568,9 +2571,16 @@ bool RecGroupEquator::eq(const TypeInfo& a, const TypeInfo& b) const {
 }
 
 bool RecGroupEquator::eq(const HeapTypeInfo& a, const HeapTypeInfo& b) const {
-  if (a.supertype != b.supertype) {
+  if (bool(a.supertype) != bool(b.supertype)) {
     return false;
   }
+  if (a.supertype) {
+    HeapType superA(uintptr_t(a.supertype));
+    HeapType superB(uintptr_t(b.supertype));
+    if (!eq(superA, superB)) {
+      return false;
+    }
+  }
   if (a.kind != b.kind) {
     return false;
   }
@@ -3550,7 +3560,11 @@ void canonicalizeEquirecursive(CanonicalizationState& state) {
 }
 
 std::optional<TypeBuilder::Error>
-validateStructuralSubtyping(const std::vector<HeapType>& types) {
+validateSubtyping(const std::vector<HeapType>& types) {
+  if (getTypeSystem() == TypeSystem::Equirecursive) {
+    // Subtyping is not explicitly declared, so nothing to check.
+    return {};
+  }
   for (size_t i = 0; i < types.size(); ++i) {
     HeapType type = types[i];
     if (type.isBasic()) {
@@ -3623,11 +3637,6 @@ canonicalizeNominal(CanonicalizationState& state) {
     checked.insert(path.begin(), path.end());
   }
 
-  // Check that the declared supertypes are valid.
-  if (auto error = validateStructuralSubtyping(state.results)) {
-    return {*error};
-  }
-
   return {};
 }
 
@@ -3700,11 +3709,6 @@ std::optional<TypeBuilder::Error> canonicalizeIsorecursive(
     return *error;
   }
 
-  // Check that the declared supertypes are structurally valid.
-  if (auto error = validateStructuralSubtyping(state.results)) {
-    return {*error};
-  }
-
   // Now that we know everything is valid, start canonicalizing recursion
   // groups. Before canonicalizing each group, update all the HeapType use sites
   // within it to make sure it only refers to other canonical groups or to
@@ -3839,6 +3843,11 @@ TypeBuilder::BuildResult TypeBuilder::build() {
             << getMillis(afterStructureCanonicalization, end) << " ms\n";
 #endif
 
+  // Check that the declared supertypes are structurally valid.
+  if (auto error = validateSubtyping(state.results)) {
+    return {*error};
+  }
+
   // Note built signature types. See comment in `HeapType::HeapType(Signature)`.
   for (auto type : state.results) {
     if (type.isSignature() && (getTypeSystem() == TypeSystem::Nominal)) {

test/gtest/type-builder.cpp

@@ -431,3 +431,29 @@ TEST_F(IsorecursiveTest, HeapTypeConstructors) {
   EXPECT_EQ(struct_, struct2);
   EXPECT_EQ(array, array2);
 }
+
+TEST_F(IsorecursiveTest, CanonicalizeTypesBeforeSubtyping) {
+  TypeBuilder builder(6);
+  // A rec group
+  builder.createRecGroup(0, 2);
+  builder[0] = Struct{};
+  builder[1] = Struct{};
+  builder[1].subTypeOf(builder[0]);
+
+  // The same rec group again
+  builder.createRecGroup(2, 2);
+  builder[2] = Struct{};
+  builder[3] = Struct{};
+  builder[3].subTypeOf(builder[2]);
+
+  // This subtyping only validates if the previous two groups are deduplicated
+  // before checking subtype validity.
+  builder[4] =
+    Struct({Field(builder.getTempRefType(builder[0], Nullable), Immutable)});
+  builder[5] =
+    Struct({Field(builder.getTempRefType(builder[3], Nullable), Immutable)});
+  builder[5].subTypeOf(builder[4]);
+
+  auto result = builder.build();
+  EXPECT_TRUE(result);
+}