[GC] Fix TypeRefining on StructGets without content but with a reachable ref (#7138)

If we see a StructGet with no content (the type it reads from has no writes)
then we can make it unreachable. The old code literally just changed the type
to unreachable, which would later work out with refinalization - but only if
the StructGet's ref was unreachable. But it is possible for this situation to
occur without that, and if so, this hit the validation error "can't have an
unreachable node without an unreachable child".

To fix this, merge all code paths that handle "impossible" situations, which
simplifies things, and add this situation.

This uncovered an existing bug where we noted default values of refs, but
not non-refs (which could lead us to think that a field of a struct that only
was ever created by struct.new_default, was never created at all). Fixed as
well.

2 files changed, 172 insertions, 62 deletions

diff --git a/src/passes/TypeRefining.cpp b/src/passes/TypeRefining.cpp
index ee12c388d..c37a105ec 100644
--- a/src/passes/TypeRefining.cpp
+++ b/src/passes/TypeRefining.cpp
@@ -58,11 +58,13 @@ struct FieldInfoScanner
 
   void
   noteDefault(Type fieldType, HeapType type, Index index, FieldInfo& info) {
-    // Default values do not affect what the heap type of a field can be turned
-    // into. Note them, however, as they force us to keep the type nullable.
+    // Default values must be noted, so that we know there is content there.
     if (fieldType.isRef()) {
-      info.note(Type(fieldType.getHeapType().getBottom(), Nullable));
+      // All we need to note here is nullability (the field must remain
+      // nullable), but not anything else about the type.
+      fieldType = Type(fieldType.getHeapType().getBottom(), Nullable);
     }
+    info.note(fieldType);
   }
 
   void noteCopy(HeapType type, Index index, FieldInfo& info) {
@@ -270,71 +272,49 @@ struct TypeRefining : public Pass {
           return;
         }
 
-        if (curr->ref->type.isNull()) {
-          // This get will trap. In theory we could leave this for later
-          // optimizations to do, but we must actually handle it here, because
-          // of the situation where this get's type is refined, and the input
-          // type is the result of a refining:
-          //
-          //   (struct.get $A    ;; should be refined to something
-          //     (struct.get $B  ;; just refined to nullref
-          //
-          // If the input has become a nullref then we can't just return out of
-          // this function, as we'd be leaving a struct.get of $A with the
-          // wrong type. But we can't find the right type since in Binaryen IR
-          // we use the ref's type to see what is being read, and that just
-          // turned into nullref. To avoid that corner case, just turn this code
-          // into unreachable code now, and the later refinalize will turn all
-          // the parents unreachable, avoiding any type-checking problems.
-          Builder builder(*getModule());
-          replaceCurrent(builder.makeSequence(builder.makeDrop(curr->ref),
-                                              builder.makeUnreachable()));
-          return;
+        Type newFieldType;
+        if (!curr->ref->type.isNull()) {
+          auto oldType = curr->ref->type.getHeapType();
+          newFieldType = parent.finalInfos[oldType][curr->index].getLUB();
         }
 
-        auto oldType = curr->ref->type.getHeapType();
-        auto newFieldType = parent.finalInfos[oldType][curr->index].getLUB();
-        if (Type::isSubType(newFieldType, curr->type)) {
-          // This is the normal situation, where the new type is a refinement of
-          // the old type. Apply that type so that the type of the struct.get
-          // matches what is in the refined field. ReFinalize will later
-          // propagate this to parents.
-          //
-          // Note that ReFinalize will also apply the type of the field itself
-          // to a struct.get, so our doing it here in this pass is usually
-          // redundant. But ReFinalize also updates other types while doing so,
-          // which can cause a problem:
-          //
-          //  (struct.get $A
-          //    (block (result (ref null $A))
-          //      (ref.null any)
-          //    )
-          //  )
-          //
-          // Here ReFinalize will turn the block's result into a bottom type,
-          // which means it won't know a type for the struct.get at that point.
-          // Doing it in this pass avoids that issue, as we have all the
-          // necessary information. (ReFinalize will still get into the
-          // situation where it doesn't know how to update the type of the
-          // struct.get, but it will just leave the existing type - it assumes
-          // no update is needed - which will be correct, since we've updated it
-          // ourselves here, before.)
-          curr->type = newFieldType;
-        } else {
-          // This instruction is invalid, so it must be the result of the
-          // situation described above: we ignored the read during our
-          // inference, and optimized accordingly, and so now we must remove it
-          // to keep the module validating. It doesn't matter what we emit here,
-          // since there are no struct.new or struct.sets for this type, so this
-          // code is logically unreachable.
-          //
-          // Note that we emit an unreachable here, which changes the type, and
-          // so we should refinalize. However, we will be refinalizing later
-          // anyhow in updateTypes, so there is no need.
+        if (curr->ref->type.isNull() || newFieldType == Type::unreachable ||
+            !Type::isSubType(newFieldType, curr->type)) {
+          // This get will trap, or cannot be reached: either the ref is null,
+          // or the field is never written any contents, or the contents we see
+          // are invalid (they passed through some fallthrough that will trap at
+          // runtime). Emit unreachable code here.
           Builder builder(*getModule());
           replaceCurrent(builder.makeSequence(builder.makeDrop(curr->ref),
                                               builder.makeUnreachable()));
+          return;
         }
+
+        // This is the normal situation, where the new type is a refinement of
+        // the old type. Apply that type so that the type of the struct.get
+        // matches what is in the refined field. ReFinalize will later
+        // propagate this to parents.
+        //
+        // Note that ReFinalize will also apply the type of the field itself
+        // to a struct.get, so our doing it here in this pass is usually
+        // redundant. But ReFinalize also updates other types while doing so,
+        // which can cause a problem:
+        //
+        //  (struct.get $A
+        //    (block (result (ref null $A))
+        //      (ref.null any)
+        //    )
+        //  )
+        //
+        // Here ReFinalize will turn the block's result into a bottom type,
+        // which means it won't know a type for the struct.get at that point.
+        // Doing it in this pass avoids that issue, as we have all the
+        // necessary information. (ReFinalize will still get into the
+        // situation where it doesn't know how to update the type of the
+        // struct.get, but it will just leave the existing type - it assumes
+        // no update is needed - which will be correct, since we've updated it
+        // ourselves here, before.)
+        curr->type = newFieldType;
       }
     };
 
diff --git a/test/lit/passes/type-refining.wast b/test/lit/passes/type-refining.wast
index 91f61bc15..fad016e1d 100644
--- a/test/lit/passes/type-refining.wast
+++ b/test/lit/passes/type-refining.wast
@@ -1574,3 +1574,133 @@
   (ref.null $8)
  )
 )
+
+;; Test for a bug where we made a struct.get unreachable because it was reading
+;; a field that had no writes, but in a situation where it is invalid for the
+;; struct.get to be unreachable.
+(module
+ ;; CHECK:      (type $never (sub (struct (field i32))))
+ (type $never (sub (struct (field i32))))
+
+ ;; CHECK:      (rec
+ ;; CHECK-NEXT:  (type $optimizable (struct (field (mut nullfuncref))))
+ (type $optimizable (struct (field (mut (ref null func)))))
+
+ ;; CHECK:       (type $2 (func))
+
+ ;; CHECK:      (global $never (ref null $never) (ref.null none))
+ (global $never (export "never") (ref null $never)
+   ;; Make the type $never public (if it were private, we'd optimize in a
+   ;; different way that avoids the bug that this tests for).
+   (ref.null $never)
+ )
+
+ ;; CHECK:      (export "never" (global $never))
+
+ ;; CHECK:      (func $setup (type $2)
+ ;; CHECK-NEXT:  (drop
+ ;; CHECK-NEXT:   (struct.new $optimizable
+ ;; CHECK-NEXT:    (ref.null nofunc)
+ ;; CHECK-NEXT:   )
+ ;; CHECK-NEXT:  )
+ ;; CHECK-NEXT:  (drop
+ ;; CHECK-NEXT:   (block
+ ;; CHECK-NEXT:    (drop
+ ;; CHECK-NEXT:     (block (result (ref none))
+ ;; CHECK-NEXT:      (ref.as_non_null
+ ;; CHECK-NEXT:       (ref.null none)
+ ;; CHECK-NEXT:      )
+ ;; CHECK-NEXT:     )
+ ;; CHECK-NEXT:    )
+ ;; CHECK-NEXT:    (unreachable)
+ ;; CHECK-NEXT:   )
+ ;; CHECK-NEXT:  )
+ ;; CHECK-NEXT: )
+ (func $setup
+  ;; A struct.new, so that we have a field to refine (which avoids the pass
+  ;; early-exiting).
+  (drop
+   (struct.new $optimizable
+    (ref.null func)
+   )
+  )
+  ;; A struct.get that reads a $never, but the actual fallthrough value is none.
+  ;; We never create this type, so the field has no possible content, and we can
+  ;; replace this with an unreachable.
+  (drop
+   (struct.get $never 0
+    (block (result (ref $never))
+     (ref.as_non_null
+      (ref.null none)
+     )
+    )
+   )
+  )
+ )
+)
+
+;; Test that we note default values.
+(module
+ (rec
+  ;; CHECK:      (rec
+  ;; CHECK-NEXT:  (type $A (sub (struct (field i32))))
+  (type $A (sub (struct (field i32))))
+  ;; CHECK:       (type $B (sub (struct (field i32))))
+  (type $B (sub (struct (field i32))))
+ )
+ ;; CHECK:      (type $2 (func (param (ref null $A) (ref null $B))))
+
+ ;; CHECK:      (type $optimizable (sub (struct (field (ref $2)))))
+ (type $optimizable (sub (struct (field funcref))))
+
+ ;; CHECK:      (elem declare func $test)
+
+ ;; CHECK:      (export "test" (func $test))
+
+ ;; CHECK:      (func $test (type $2) (param $x (ref null $A)) (param $y (ref null $B))
+ ;; CHECK-NEXT:  (drop
+ ;; CHECK-NEXT:   (struct.new $optimizable
+ ;; CHECK-NEXT:    (ref.func $test)
+ ;; CHECK-NEXT:   )
+ ;; CHECK-NEXT:  )
+ ;; CHECK-NEXT:  (drop
+ ;; CHECK-NEXT:   (struct.get $A 0
+ ;; CHECK-NEXT:    (struct.new $A
+ ;; CHECK-NEXT:     (i32.const 0)
+ ;; CHECK-NEXT:    )
+ ;; CHECK-NEXT:   )
+ ;; CHECK-NEXT:  )
+ ;; CHECK-NEXT:  (drop
+ ;; CHECK-NEXT:   (struct.get $B 0
+ ;; CHECK-NEXT:    (struct.new_default $B)
+ ;; CHECK-NEXT:   )
+ ;; CHECK-NEXT:  )
+ ;; CHECK-NEXT: )
+ (func $test (export "test") (param $x (ref null $A)) (param $y (ref null $B))
+  ;; Use $A, $B as params of this export, so they are public.
+
+  ;; Make something for the pass to do, to avoid early-exit.
+  (drop
+   (struct.new $optimizable
+    (ref.func $test)
+   )
+  )
+
+  ;; Get from a struct.new. We have nothing to optimize here. (In particular, we
+  ;; cannot make this unreachable, as there is a value in the field, 0.)
+  (drop
+   (struct.get $A 0
+    (struct.new $A
+     (i32.const 0)
+    )
+   )
+  )
+
+  ;; As above. Now the value in the field comes from a default value.
+  (drop
+   (struct.get $B 0
+    (struct.new_default $B)
+   )
+  )
+ )
+)