Public type mismatch between collectHeapTypeInfo and getPublicHeapTypes

[kripken]

Testcase:

(module
 (type $1 (sub (struct (field (mut (ref null $1))))))
 (rec
  (type $5 (sub (struct (field (ref func)))))
  (type $6 (sub $1 (struct (field (mut (ref null $1))))))
 )
 (rec
  (type $7 (sub (struct (field (ref $1)))))
  (type $8 (sub (func)))
 )
 (type $9 (sub $6 (struct (field (mut (ref null $1))))))

 (export "func" (func $1))

 (func $1 (result (ref null $8))
  (local $l (ref $9))
  (drop
   (struct.get $5 0
    (struct.new $5
     (ref.func $1)
    )
   )
  )
  (ref.null $8)
 )
)

$ bin/wasm-opt a.wat -all --closed-world --type-refining
Fatal: Internal GlobalTypeRewriter build error: Heap type has an invalid supertype at index 1

Looking into this, it seems like the two APIs mentioned in the title disagree. The pass uses the latter, while GlobalTypeRewriter uses the former, and they disagree on whether $1 is public or not, leading to that error. Here is how I checked that:

diff --git a/src/ir/type-updating.cpp b/src/ir/type-updating.cpp
index 8b74ebfcd..9906615a6 100644
--- a/src/ir/type-updating.cpp
+++ b/src/ir/type-updating.cpp
@@ -38,20 +38,34 @@ GlobalTypeRewriter::TypeMap GlobalTypeRewriter::rebuildTypes(
   const std::vector<HeapType>& additionalPrivateTypes) {
   // Find the heap types that are not publicly observable. Even in a closed
   // world scenario, don't modify public types because we assume that they may
   // be reflected on or used for linking. Figure out where each private type
   // will be located in the builder.
   auto typeInfo = ModuleUtils::collectHeapTypeInfo(
     wasm,
     ModuleUtils::TypeInclusion::UsedIRTypes,
     ModuleUtils::VisibilityHandling::FindVisibility);
 
+  for (auto& [t, info] : typeInfo) {
+    std::cout << "collectHeapTypeInfo: " << wasm.typeNames[t].name << ": " << (info.visibility == ModuleUtils::Visibility::Private ? "private" : "public") << '\n';
+  }
+
+  {
+    auto publicTypes = ModuleUtils::getPublicHeapTypes(wasm);
+    std::unordered_set<HeapType> publicTypesSet(publicTypes.begin(),
+                                                publicTypes.end());
+    for (auto t : publicTypes) {
+      std::cout << "getPublicHeapTypes says this is public: " << wasm.typeNames[t].name << '\n';
+    }
+  }
+
+  assert(additionalPrivateTypes.empty());
   std::unordered_set<HeapType> additionalSet(additionalPrivateTypes.begin(),
                                              additionalPrivateTypes.end());
 
   std::vector<std::pair<HeapType, SmallVector<HeapType, 1>>> privateSupertypes;
   privateSupertypes.reserve(typeInfo.size());
   for (auto& [type, info] : typeInfo) {
     if (info.visibility != ModuleUtils::Visibility::Private &&
         !additionalSet.count(type)) {
       continue;
     }

This prints out

collectHeapTypeInfo: (null Name): public
collectHeapTypeInfo: 9: private
collectHeapTypeInfo: 5: private
collectHeapTypeInfo: 8: public
collectHeapTypeInfo: 1: private
collectHeapTypeInfo: 6: private
getPublicHeapTypes says this is public: (null Name)
getPublicHeapTypes says this is public: 8
getPublicHeapTypes says this is public: 1
getPublicHeapTypes says this is public: 7

So $1 is mismatched.

@tlively in your refactoring of those APIs, I thought one was built on top of the other - how can they disagree?

[tlively]

Public visibility is currently propagated only through types that have been collected by collectHeapTypes. getPublicHeapTypes uses TypeInclusion::BinaryTypes, so it collects type $7 and propagates public visibility through type $7 from type $8 to type $1. When GlobalTypeRewriter calls collectHeapTypes it uses TypeInclusion::UsedIRTypes, so type $7 is not collected and public visibility never propagates through it to type $1.

I think the best fix would be to have public visibility propagate through all types, even if they are not part of the collected output.