2 files changed, 66 insertions, 55 deletions

diff --git a/src/ir/subtype-exprs.h b/src/ir/subtype-exprs.h
index 7ebb0b017..056b373c2 100644
--- a/src/ir/subtype-exprs.h
+++ b/src/ir/subtype-exprs.h
@@ -27,7 +27,7 @@ namespace wasm {
 // Analyze subtyping relationships between expressions. This must CRTP with a
 // class that implements:
 //
-//  * noteSubType(A, B) indicating A must be a subtype of B
+//  * noteSubtype(A, B) indicating A must be a subtype of B
 //  * noteCast(A, B) indicating A is cast to B
 //
 // There must be multiple versions of each of those, supporting A and B being
@@ -35,20 +35,20 @@ namespace wasm {
 // indicating a flexible requirement that depends on the type of that
 // expression. Specifically:
 //
-//  * noteSubType(Type, Type) - A constraint not involving expressions at all,
+//  * noteSubtype(Type, Type) - A constraint not involving expressions at all,
 //                              for example, an element segment's type must be
 //                              a subtype of the corresponding table's.
-//  * noteSubType(HeapType, HeapType) - Ditto, with heap types, for example in a
+//  * noteSubtype(HeapType, HeapType) - Ditto, with heap types, for example in a
 //                                      CallIndirect.
-//  * noteSubType(Type, Expression) - A fixed type must be a subtype of an
+//  * noteSubtype(Type, Expression) - A fixed type must be a subtype of an
 //                                    expression's type, for example, in BrOn
 //                                    (the declared sent type must be a subtype
 //                                    of the block we branch to).
-//  * noteSubType(Expression, Type) - An expression's type must be a subtype of
+//  * noteSubtype(Expression, Type) - An expression's type must be a subtype of
 //                                    a fixed type, for example, a Call operand
 //                                    must be a subtype of the signature's
 //                                    param.
-//  * noteSubType(Expression, Expression) - An expression's type must be a
+//  * noteSubtype(Expression, Expression) - An expression's type must be a
 //                                          subtype of anothers, for example,
 //                                          a block and its last child.
 //
@@ -59,6 +59,17 @@ namespace wasm {
 //  * noteCast(Expression, Expression) - An expression's type is cast to
 //                                       another, for example, in RefCast.
 //
+// The concrete signatures are:
+//
+//      void noteSubtype(Type, Type);
+//      void noteSubtype(HeapType, HeapType);
+//      void noteSubtype(Type, Expression*);
+//      void noteSubtype(Expression*, Type);
+//      void noteSubtype(Expression*, Expression*);
+//      void noteCast(HeapType, HeapType);
+//      void noteCast(Expression*, Type);
+//      void noteCast(Expression*, Expression*);
+//
 // Note that noteCast(Type, Type) and noteCast(Type, Expression) never occur and
 // do not need to be implemented.
 //
diff --git a/src/passes/StringLowering.cpp b/src/passes/StringLowering.cpp
index c70691ea1..c04695c50 100644
--- a/src/passes/StringLowering.cpp
+++ b/src/passes/StringLowering.cpp
@@ -34,6 +34,7 @@
 
 #include "ir/module-utils.h"
 #include "ir/names.h"
+#include "ir/subtype-exprs.h"
 #include "ir/type-updating.h"
 #include "ir/utils.h"
 #include "pass.h"
@@ -199,6 +200,12 @@ struct StringLowering : public StringGathering {
     // Replace string.* etc. operations with imported ones.
     replaceInstructions(module);
 
+    // Replace ref.null types as needed.
+    replaceNulls(module);
+
+    // ReFinalize to apply all the above changes.
+    ReFinalize().run(getPassRunner(), module);
+
     // Disable the feature here after we lowered everything away.
     module->features.disable(FeatureSet::Strings);
   }
@@ -433,65 +440,58 @@ struct StringLowering : public StringGathering {
             WASM_UNREACHABLE("TODO: all string.slice*");
         }
       }
+    };
 
-      // Additional hacks: We fix up a none that should be noext. Before the
-      // lowering we can use none for stringref, but after we must use noext as
-      // the two do not share a bottom type.
-      //
-      // The code here and in the visitors below is course wildly insufficient
-      // (we need selects and blocks and all other joins, and not just nulls,
-      // etc.) but in practice this is enough for now. TODO extend as needed
-      void ensureNullIsExt(Expression* curr) {
-        if (auto* null = curr->dynCast<RefNull>()) {
-          null->finalize(HeapType::noext);
-        }
-      }
+    Replacer replacer(*this);
+    replacer.run(getPassRunner(), module);
+    replacer.walkModuleCode(module);
+  }
 
-      bool isExt(Type type) {
-        return type.isRef() && type.getHeapType() == HeapType::ext;
+  // A ref.null of none needs to be noext if it is going to a location of type
+  // stringref.
+  void replaceNulls(Module* module) {
+    // Use SubtypingDiscoverer to find when a ref.null of none flows into a
+    // place that has been changed from stringref to externref.
+    struct NullFixer
+      : public WalkerPass<
+          ControlFlowWalker<NullFixer, SubtypingDiscoverer<NullFixer>>> {
+      // Hooks for SubtypingDiscoverer.
+      void noteSubtype(Type, Type) {
+        // Nothing to do for pure types.
       }
-
-      void visitIf(If* curr) {
-        // If the if outputs an ext, fix up the arms to contain proper nulls for
-        // that type.
-        if (isExt(curr->type)) {
-          ensureNullIsExt(curr->ifTrue);
-          ensureNullIsExt(curr->ifFalse);
-        }
+      void noteSubtype(HeapType, HeapType) {
+        // Nothing to do for pure types.
       }
-
-      void visitCall(Call* curr) {
-        auto targetSig =
-          getModule()->getFunction(curr->target)->type.getSignature();
-        for (Index i = 0; i < curr->operands.size(); i++) {
-          if (isExt(targetSig.params[i])) {
-            ensureNullIsExt(curr->operands[i]);
-          }
-        }
+      void noteSubtype(Type, Expression*) {
+        // Nothing to do for a subtype of an expression.
       }
-
-      void visitStructNew(StructNew* curr) {
-        if (curr->type == Type::unreachable || curr->operands.empty()) {
-          return;
-        }
-
-        // If we write a none into an ext field, fix that.
-        auto& fields = curr->type.getHeapType().getStruct().fields;
-        assert(curr->operands.size() == fields.size());
-        for (Index i = 0; i < fields.size(); i++) {
-          if (isExt(fields[i].type)) {
-            ensureNullIsExt(curr->operands[i]);
+      void noteSubtype(Expression* a, Type b) {
+        // This is the case we care about: if |a| is a null that must be a
+        // subtype of ext then we fix that up.
+        if (b.isRef() && b.getHeapType().getTop() == HeapType::ext) {
+          if (auto* null = a->dynCast<RefNull>()) {
+            null->finalize(HeapType::noext);
           }
         }
       }
+      void noteSubtype(Expression* a, Expression* b) {
+        // Only the type matters of the place we assign to.
+        noteSubtype(a, b->type);
+      }
+      void noteCast(HeapType, HeapType) {
+        // Casts do not concern us.
+      }
+      void noteCast(Expression*, Type) {
+        // Casts do not concern us.
+      }
+      void noteCast(Expression*, Expression*) {
+        // Casts do not concern us.
+      }
     };
 
-    Replacer replacer(*this);
-    replacer.run(getPassRunner(), module);
-    replacer.walkModuleCode(module);
-
-    // ReFinalize to apply changes to parents.
-    ReFinalize().run(getPassRunner(), module);
+    NullFixer fixer;
+    fixer.run(getPassRunner(), module);
+    fixer.walkModuleCode(module);
   }
 };