From 56a15944c863c8db45bc3ab699d52e97a7d61330 Mon Sep 17 00:00:00 2001
From: Thomas Lively <tlively@google.com>
Date: Wed, 23 Oct 2024 16:58:33 -0700
Subject: [PATCH] Fix TypeMerging bug with indirectly reachable public types

TypeMerging works by representing the type definition graph as a partitioned DFA
and then refining the partitions to find mergeable types. #7023 was due to a bug
where the DFA included edges from public types to their children, but did not
necessarily include corresponding states for those children.

One way to fix the bug would have been to traverse the type graph, finding all
reachable public types and creating DFA states for them, but that might be
expensive in cases where there are large graphs of public types.

Instead, fix the problem by removing the edges from public types to their
children entirely. Types reachable from public types are also public and
therefore are not eligible to be merged, so these edges were never necessary for
correctness.

Fixes #7023.
---
 src/passes/TypeMerging.cpp        | 18 +++++++++++++-----
 test/lit/passes/type-merging.wast | 19 +++++++++++++++++++
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/src/passes/TypeMerging.cpp b/src/passes/TypeMerging.cpp
index 206addd2fed..22c2352f292 100644
--- a/src/passes/TypeMerging.cpp
+++ b/src/passes/TypeMerging.cpp
@@ -504,11 +504,19 @@ std::vector<HeapType> TypeMerging::getPublicChildren(HeapType type) {
 
 DFA::State<HeapType> TypeMerging::makeDFAState(HeapType type) {
   std::vector<HeapType> succs;
-  for (auto child : type.getHeapTypeChildren()) {
-    // Both private and public heap type children participate in the DFA and are
-    // eligible to be successors.
-    if (!child.isBasic()) {
-      succs.push_back(getMerged(child));
+  // Both private and public heap type children participate in the DFA and are
+  // eligible to be successors, except that public types are terminal states
+  // that do not have successors. This is sufficient because public types are
+  // always in their own singleton partitions, so they already differentiate
+  // types that reach them without needing to consider their children. In the
+  // other direction, including the children is not necessary to differentiate
+  // types reached by the public types because all such reachable types are also
+  // public and not eligible to be merged.
+  if (privateTypes.count(type)) {
+    for (auto child : type.getHeapTypeChildren()) {
+      if (!child.isBasic()) {
+        succs.push_back(getMerged(child));
+      }
     }
   }
   return {type, std::move(succs)};
diff --git a/test/lit/passes/type-merging.wast b/test/lit/passes/type-merging.wast
index b8d60286346..5d0c4bc5e7b 100644
--- a/test/lit/passes/type-merging.wast
+++ b/test/lit/passes/type-merging.wast
@@ -985,6 +985,25 @@
  (global $g2 (ref $C') (struct.new_default $D2'))
 )
 
+;; Regression test for a bug where public types not directly reachable from
+;; private types were included as successors but not given states in the DFA.
+(module
+  ;; CHECK:      (type $public-child (struct))
+  (type $public-child (struct))
+  ;; CHECK:      (type $public (struct (field (ref $public-child))))
+  (type $public (struct (ref $public-child)))
+  ;; CHECK:      (type $private (struct (field (ref $public))))
+  (type $private (struct (ref $public)))
+
+  ;; CHECK:      (global $public (ref null $public) (ref.null none))
+  (global $public (ref null $public) (ref.null none))
+  ;; CHECK:      (global $private (ref null $private) (ref.null none))
+  (global $private (ref null $private) (ref.null none))
+
+  ;; CHECK:      (export "public" (global $public))
+  (export "public" (global $public))
+)
+
 ;; Check that a ref.test inhibits merging (ref.cast is already checked above).
 (module
   ;; CHECK:      (rec