Improve distinct rec groups error message (#8522)

Refactor ErrorReason to be a std::variant that can carry extra
information beyond an error code. Use this expanded capability to report
the missing features for the error about distinct rec groups being
identical after binary writing.

Fixes #8519.

Author: tlively

src/binaryen-c.cpp

@@ -6541,19 +6541,19 @@ ExpressionRunnerRunAndDispose(ExpressionRunnerRef runner,
 
 TypeBuilderErrorReason TypeBuilderErrorReasonSelfSupertype() {
   return static_cast<TypeBuilderErrorReason>(
-    TypeBuilder::ErrorReason::SelfSupertype);
+    TypeBuilder::ErrorReasonKind::SelfSupertype);
 }
 TypeBuilderErrorReason TypeBuilderErrorReasonInvalidSupertype() {
   return static_cast<TypeBuilderErrorReason>(
-    TypeBuilder::ErrorReason::InvalidSupertype);
+    TypeBuilder::ErrorReasonKind::InvalidSupertype);
 }
 TypeBuilderErrorReason TypeBuilderErrorReasonForwardSupertypeReference() {
   return static_cast<TypeBuilderErrorReason>(
-    TypeBuilder::ErrorReason::ForwardSupertypeReference);
+    TypeBuilder::ErrorReasonKind::ForwardSupertypeReference);
 }
 TypeBuilderErrorReason TypeBuilderErrorReasonForwardChildReference() {
   return static_cast<TypeBuilderErrorReason>(
-    TypeBuilder::ErrorReason::ForwardChildReference);
+    TypeBuilder::ErrorReasonKind::ForwardChildReference);
 }
 
 TypeBuilderRef TypeBuilderCreate(BinaryenIndex size) {
@@ -6652,7 +6652,7 @@ bool TypeBuilderBuildAndDispose(TypeBuilderRef builder,
       *errorIndex = err->index;
     }
     if (errorReason) {
-      *errorReason = static_cast<TypeBuilderErrorReason>(err->reason);
+      *errorReason = static_cast<TypeBuilderErrorReason>(err->reason.getKind());
     }
     delete B;
     return false;

src/wasm-type.h

@@ -894,7 +894,7 @@ struct TypeBuilder {
   void setOpen(size_t i, bool open = true);
   void setShared(size_t i, Shareability share = Shared);
 
-  enum class ErrorReason {
+  enum class ErrorReasonKind {
     // There is a cycle in the supertype relation.
     SelfSupertype,
     // The declared supertype of a type is invalid.
@@ -934,6 +934,27 @@ struct TypeBuilder {
     RecGroupCollision,
   };
 
+  struct RecGroupCollision {
+    FeatureSet missingFeatures;
+    bool operator==(const RecGroupCollision& other) const {
+      return missingFeatures == other.missingFeatures;
+    }
+  };
+
+  struct ErrorReason : std::variant<ErrorReasonKind, RecGroupCollision> {
+    using variant::variant;
+
+    ErrorReasonKind getKind() const {
+      if (auto* kind = std::get_if<ErrorReasonKind>(this)) {
+        return *kind;
+      }
+      return ErrorReasonKind::RecGroupCollision;
+    }
+
+    bool operator==(ErrorReasonKind kind) const { return getKind() == kind; }
+    bool operator!=(ErrorReasonKind kind) const { return getKind() != kind; }
+  };
+
   struct Error {
     // The index of the type causing the failure.
     size_t index;
@@ -1082,7 +1103,7 @@ std::ostream& operator<<(std::ostream&, Continuation);
 std::ostream& operator<<(std::ostream&, Field);
 std::ostream& operator<<(std::ostream&, Struct);
 std::ostream& operator<<(std::ostream&, Array);
-std::ostream& operator<<(std::ostream&, TypeBuilder::ErrorReason);
+std::ostream& operator<<(std::ostream&, const TypeBuilder::ErrorReason&);
 
 // Inline some nontrivial methods here for performance reasons.
 

src/wasm/wasm-type.cpp

@@ -1503,49 +1503,64 @@ std::ostream& operator<<(std::ostream& os, Struct struct_) {
 std::ostream& operator<<(std::ostream& os, Array array) {
   return TypePrinter(os).print(array);
 }
-std::ostream& operator<<(std::ostream& os, TypeBuilder::ErrorReason reason) {
+std::ostream& operator<<(std::ostream& os,
+                         TypeBuilder::ErrorReasonKind reason) {
   switch (reason) {
-    case TypeBuilder::ErrorReason::SelfSupertype:
+    case TypeBuilder::ErrorReasonKind::SelfSupertype:
       return os << "Heap type is a supertype of itself";
-    case TypeBuilder::ErrorReason::InvalidSupertype:
+    case TypeBuilder::ErrorReasonKind::InvalidSupertype:
       return os << "Heap type has an invalid supertype";
-    case TypeBuilder::ErrorReason::ForwardSupertypeReference:
+    case TypeBuilder::ErrorReasonKind::ForwardSupertypeReference:
       return os << "Heap type has an undeclared supertype";
-    case TypeBuilder::ErrorReason::ForwardChildReference:
+    case TypeBuilder::ErrorReasonKind::ForwardChildReference:
       return os << "Heap type has an undeclared child";
-    case TypeBuilder::ErrorReason::InvalidFuncType:
+    case TypeBuilder::ErrorReasonKind::InvalidFuncType:
       return os << "Continuation has invalid function type";
-    case TypeBuilder::ErrorReason::InvalidSharedType:
+    case TypeBuilder::ErrorReasonKind::InvalidSharedType:
       return os << "Shared types require shared-everything";
-    case TypeBuilder::ErrorReason::InvalidWaitQueue:
+    case TypeBuilder::ErrorReasonKind::InvalidWaitQueue:
       return os << "Waitqueues require shared-everything";
-    case TypeBuilder::ErrorReason::InvalidStringType:
+    case TypeBuilder::ErrorReasonKind::InvalidStringType:
       return os << "String types require strings feature";
-    case TypeBuilder::ErrorReason::InvalidUnsharedField:
+    case TypeBuilder::ErrorReasonKind::InvalidUnsharedField:
       return os << "Heap type has an invalid unshared field";
-    case TypeBuilder::ErrorReason::NonStructDescribes:
+    case TypeBuilder::ErrorReasonKind::NonStructDescribes:
       return os << "Describes clause on a non-struct type";
-    case TypeBuilder::ErrorReason::ForwardDescribesReference:
+    case TypeBuilder::ErrorReasonKind::ForwardDescribesReference:
       return os << "Describes clause is a forward reference";
-    case TypeBuilder::ErrorReason::MismatchedDescribes:
+    case TypeBuilder::ErrorReasonKind::MismatchedDescribes:
       return os << "Described type is not a matching descriptor";
-    case TypeBuilder::ErrorReason::NonStructDescriptor:
+    case TypeBuilder::ErrorReasonKind::NonStructDescriptor:
       return os << "Descriptor clause on a non-struct type";
-    case TypeBuilder::ErrorReason::MismatchedDescriptor:
+    case TypeBuilder::ErrorReasonKind::MismatchedDescriptor:
       return os << "Descriptor type does not describe heap type";
-    case TypeBuilder::ErrorReason::InvalidUnsharedDescriptor:
+    case TypeBuilder::ErrorReasonKind::InvalidUnsharedDescriptor:
       return os << "Heap type has an invalid unshared descriptor";
-    case TypeBuilder::ErrorReason::InvalidUnsharedDescribes:
+    case TypeBuilder::ErrorReasonKind::InvalidUnsharedDescribes:
       return os << "Heap type describes an invalid unshared type";
-    case TypeBuilder::ErrorReason::RequiresCustomDescriptors:
+    case TypeBuilder::ErrorReasonKind::RequiresCustomDescriptors:
       return os << "custom descriptors required but not enabled";
-    case TypeBuilder::ErrorReason::RecGroupCollision:
+    case TypeBuilder::ErrorReasonKind::RecGroupCollision:
       return os
              << "distinct rec groups would be identical after binary writing";
   }
   WASM_UNREACHABLE("Unexpected error reason");
 }
 
+std::ostream& operator<<(std::ostream& os,
+                         const TypeBuilder::ErrorReason& reason) {
+  os << reason.getKind();
+  if (auto* collision = std::get_if<TypeBuilder::RecGroupCollision>(&reason)) {
+    if (collision->missingFeatures != FeatureSet(FeatureSet::None)) {
+      os << " (to resolve this, use";
+      collision->missingFeatures.iterFeatures(
+        [&](auto feat) { os << " --enable-" << FeatureSet::toString(feat); });
+      os << ")";
+    }
+  }
+  return os;
+}
+
 unsigned Field::getByteSize() const {
   if (type != Type::i32) {
     return type.getByteSize();
@@ -2413,13 +2428,13 @@ validateType(Type type, FeatureSet feats, bool isShared) {
   if (type.isRef()) {
     auto heapType = type.getHeapType();
     if (isShared && !heapType.isShared()) {
-      return TypeBuilder::ErrorReason::InvalidUnsharedField;
+      return TypeBuilder::ErrorReasonKind::InvalidUnsharedField;
     }
     if (heapType.isShared() && !feats.hasSharedEverything()) {
-      return TypeBuilder::ErrorReason::InvalidSharedType;
+      return TypeBuilder::ErrorReasonKind::InvalidSharedType;
     }
     if (heapType.isString() && !feats.hasStrings()) {
-      return TypeBuilder::ErrorReason::InvalidStringType;
+      return TypeBuilder::ErrorReasonKind::InvalidStringType;
     }
   }
   return std::nullopt;
@@ -2433,7 +2448,7 @@ validateStruct(const Struct& struct_, FeatureSet feats, bool isShared) {
     }
     if (field.packedType == Field::PackedType::WaitQueue &&
         !feats.hasSharedEverything()) {
-      return TypeBuilder::ErrorReason::InvalidWaitQueue;
+      return TypeBuilder::ErrorReasonKind::InvalidWaitQueue;
     }
   }
   return std::nullopt;
@@ -2464,10 +2479,10 @@ validateSignature(Signature sig, FeatureSet feats, bool isShared) {
 std::optional<TypeBuilder::ErrorReason>
 validateContinuation(Continuation cont, FeatureSet feats, bool isShared) {
   if (!cont.type.isSignature()) {
-    return TypeBuilder::ErrorReason::InvalidFuncType;
+    return TypeBuilder::ErrorReasonKind::InvalidFuncType;
   }
   if (isShared != cont.type.isShared()) {
-    return TypeBuilder::ErrorReason::InvalidFuncType;
+    return TypeBuilder::ErrorReasonKind::InvalidFuncType;
   }
   return std::nullopt;
 }
@@ -2480,48 +2495,48 @@ validateTypeInfo(HeapTypeInfo& info,
     // The supertype must be canonical (i.e. defined in a previous rec group)
     // or have already been defined in this rec group.
     if (super->isTemp && !seenTypes.count(HeapType(uintptr_t(super)))) {
-      return TypeBuilder::ErrorReason::ForwardSupertypeReference;
+      return TypeBuilder::ErrorReasonKind::ForwardSupertypeReference;
     }
     // The supertype must have a valid structure.
     if (!isValidSupertype(info, *super)) {
-      return TypeBuilder::ErrorReason::InvalidSupertype;
+      return TypeBuilder::ErrorReasonKind::InvalidSupertype;
     }
   }
   if (auto* desc = info.described) {
     if (!features.hasCustomDescriptors()) {
-      return TypeBuilder::ErrorReason::RequiresCustomDescriptors;
+      return TypeBuilder::ErrorReasonKind::RequiresCustomDescriptors;
     }
     if (info.kind != HeapTypeKind::Struct) {
-      return TypeBuilder::ErrorReason::NonStructDescribes;
+      return TypeBuilder::ErrorReasonKind::NonStructDescribes;
     }
     assert(desc->isTemp && "unexpected canonical described type");
     if (!seenTypes.count(HeapType(uintptr_t(desc)))) {
-      return TypeBuilder::ErrorReason::ForwardDescribesReference;
+      return TypeBuilder::ErrorReasonKind::ForwardDescribesReference;
     }
     if (desc->descriptor != &info) {
-      return TypeBuilder::ErrorReason::MismatchedDescribes;
+      return TypeBuilder::ErrorReasonKind::MismatchedDescribes;
     }
   }
   if (auto* desc = info.descriptor) {
     if (!features.hasCustomDescriptors()) {
-      return TypeBuilder::ErrorReason::RequiresCustomDescriptors;
+      return TypeBuilder::ErrorReasonKind::RequiresCustomDescriptors;
     }
     if (info.kind != HeapTypeKind::Struct) {
-      return TypeBuilder::ErrorReason::NonStructDescriptor;
+      return TypeBuilder::ErrorReasonKind::NonStructDescriptor;
     }
     if (desc->described != &info) {
-      return TypeBuilder::ErrorReason::MismatchedDescriptor;
+      return TypeBuilder::ErrorReasonKind::MismatchedDescriptor;
     }
   }
   if (info.share == Shared) {
     if (!features.hasSharedEverything()) {
-      return TypeBuilder::ErrorReason::InvalidSharedType;
+      return TypeBuilder::ErrorReasonKind::InvalidSharedType;
     }
     if (info.described && info.described->share != Shared) {
-      return TypeBuilder::ErrorReason::InvalidUnsharedDescribes;
+      return TypeBuilder::ErrorReasonKind::InvalidUnsharedDescribes;
     }
     if (info.descriptor && info.descriptor->share != Shared) {
-      return TypeBuilder::ErrorReason::InvalidUnsharedDescriptor;
+      return TypeBuilder::ErrorReasonKind::InvalidUnsharedDescriptor;
     }
   }
   bool isShared = info.share == Shared;
@@ -2636,7 +2651,7 @@ buildRecGroup(std::unique_ptr<RecGroupInfo>&& groupInfo,
     for (auto child : type.getHeapTypeChildren()) {
       if (isTemp(child) && !seenTypes.count(child)) {
         return {TypeBuilder::Error{
-          i, TypeBuilder::ErrorReason::ForwardChildReference}};
+          i, TypeBuilder::ErrorReasonKind::ForwardChildReference}};
       }
     }
   }
@@ -2746,8 +2761,26 @@ TypeBuilder::BuildResult TypeBuilder::build() {
       auto group = (*built)[0].getRecGroup();
       auto uniqueGroup = impl->unique.insertOrGet(group);
       if (group != uniqueGroup) {
+        // There is a conflict. Find the set of missing featuers that would
+        // resolve the conflict if enabled.
+        FeatureSet missingFeatures = FeatureSet::None;
+        FeatureSet potential = FeatureSet::GC | FeatureSet::CustomDescriptors;
+        std::vector<HeapType> builtTypes = *built;
+        std::vector<HeapType> otherTypes(uniqueGroup.begin(),
+                                         uniqueGroup.end());
+        for (uint32_t x = 1; x & FeatureSet::All; x <<= 1) {
+          FeatureSet f(x);
+          if ((f & potential) && !impl->features.has(f)) {
+            // We have a potential missing feature. Check whether enabling it
+            // allows us to differentiate the rec groups.
+            if (RecGroupShape(builtTypes, impl->features | f) !=
+                RecGroupShape(otherTypes, impl->features | f)) {
+              missingFeatures |= f;
+            }
+          }
+        }
         return {TypeBuilder::Error{
-          groupStart, TypeBuilder::ErrorReason::RecGroupCollision}};
+          groupStart, TypeBuilder::RecGroupCollision{missingFeatures}}};
       }
     }
 

test/gtest/type-builder.cpp

@@ -215,7 +215,8 @@ TEST_F(TypeTest, DirectSelfSupertype) {
 
   const auto* error = result.getError();
   ASSERT_TRUE(error);
-  EXPECT_EQ(error->reason, TypeBuilder::ErrorReason::ForwardSupertypeReference);
+  EXPECT_EQ(error->reason,
+            TypeBuilder::ErrorReasonKind::ForwardSupertypeReference);
   EXPECT_EQ(error->index, 0u);
 }
 
@@ -233,7 +234,8 @@ TEST_F(TypeTest, IndirectSelfSupertype) {
 
   const auto* error = result.getError();
   ASSERT_TRUE(error);
-  EXPECT_EQ(error->reason, TypeBuilder::ErrorReason::ForwardSupertypeReference);
+  EXPECT_EQ(error->reason,
+            TypeBuilder::ErrorReasonKind::ForwardSupertypeReference);
   EXPECT_EQ(error->index, 0u);
 }
 
@@ -249,7 +251,7 @@ TEST_F(TypeTest, InvalidSupertype) {
 
   const auto* error = result.getError();
   ASSERT_TRUE(error);
-  EXPECT_EQ(error->reason, TypeBuilder::ErrorReason::InvalidSupertype);
+  EXPECT_EQ(error->reason, TypeBuilder::ErrorReasonKind::InvalidSupertype);
   EXPECT_EQ(error->index, 1u);
 }
 
@@ -265,7 +267,7 @@ TEST_F(TypeTest, InvalidFinalSupertype) {
 
   const auto* error = result.getError();
   ASSERT_TRUE(error);
-  EXPECT_EQ(error->reason, TypeBuilder::ErrorReason::InvalidSupertype);
+  EXPECT_EQ(error->reason, TypeBuilder::ErrorReasonKind::InvalidSupertype);
   EXPECT_EQ(error->index, 1u);
 }
 
@@ -282,7 +284,7 @@ TEST_F(TypeTest, InvalidSharedSupertype) {
 
   const auto* error = result.getError();
   ASSERT_TRUE(error);
-  EXPECT_EQ(error->reason, TypeBuilder::ErrorReason::InvalidSupertype);
+  EXPECT_EQ(error->reason, TypeBuilder::ErrorReasonKind::InvalidSupertype);
   EXPECT_EQ(error->index, 1u);
 }
 
@@ -299,7 +301,7 @@ TEST_F(TypeTest, InvalidUnsharedSupertype) {
 
   const auto* error = result.getError();
   ASSERT_TRUE(error);
-  EXPECT_EQ(error->reason, TypeBuilder::ErrorReason::InvalidSupertype);
+  EXPECT_EQ(error->reason, TypeBuilder::ErrorReasonKind::InvalidSupertype);
   EXPECT_EQ(error->index, 1u);
 }
 
@@ -319,7 +321,7 @@ TEST_F(TypeTest, ForwardReferencedChild) {
 
   const auto* error = result.getError();
   ASSERT_TRUE(error);
-  EXPECT_EQ(error->reason, TypeBuilder::ErrorReason::ForwardChildReference);
+  EXPECT_EQ(error->reason, TypeBuilder::ErrorReasonKind::ForwardChildReference);
   EXPECT_EQ(error->index, 1u);
 }
 

test/lit/validation/type-collision-exact.wast

@@ -1,6 +1,6 @@
 ;; RUN: not wasm-opt %s -all --disable-custom-descriptors 2>&1 | filecheck %s
 
-;; CHECK: error: invalid type: distinct rec groups would be identical after binary writing
+;; CHECK: error: invalid type: distinct rec groups would be identical after binary writing (to resolve this, use --enable-custom-descriptors)
 
 (module
   (type $foo (struct))

test/lit/validation/type-collision-funcref.wast

@@ -1,6 +1,6 @@
 ;; RUN: not wasm-opt %s -all --disable-gc 2>&1 | filecheck %s
 
-;; CHECK: error: invalid type: distinct rec groups would be identical after binary writing
+;; CHECK: error: invalid type: distinct rec groups would be identical after binary writing (to resolve this, use --enable-gc)
 
 (module
   (type $foo (func))

test/lit/validation/type-collision-null.wast

@@ -1,6 +1,6 @@
 ;; RUN: not wasm-opt %s -all --disable-gc 2>&1 | filecheck %s
 
-;; CHECK: error: invalid type: distinct rec groups would be identical after binary writing
+;; CHECK: error: invalid type: distinct rec groups would be identical after binary writing (to resolve this, use --enable-gc)
 
 (module
   (type $A (func (param externref)))