Remove closed world validation checks (#7019)

These were added to avoid common problems with closed world mode, but
in practice they are causing more harm than good, forcing users to work
around them. In the meantime (until #6965), remove this validation to unblock
current toolchain makers.

Fix GlobalTypeOptimization and AbstractTypeRefining on issues that this
uncovers: without this validation, it is possible to run them on more wasm
files than before, hence these were not previously detected. They are
bundled in this PR because their tests cannot validate before this PR.

8 files changed, 113 insertions, 183 deletions

diff --git a/src/pass.h b/src/pass.h
index 8d81fce8e..daed8b6ab 100644
--- a/src/pass.h
+++ b/src/pass.h
@@ -212,14 +212,6 @@ struct PassOptions {
   // but we also want to keep types of things on the boundary unchanged. For
   // example, we should not change an exported function's signature, as the
   // outside may need that type to properly call the export.
-  //
-  //   * Since the goal of closedWorld is to optimize types aggressively but
-  //     types on the module boundary cannot be changed, we assume the producer
-  //     has made a mistake and we consider it a validation error if any user
-  //     defined types besides the types of imported or exported functions
-  //     themselves appear on the module boundary. For example, no user defined
-  //     struct type may be a parameter or result of an exported function. This
-  //     error may be relaxed or made more configurable in the future.
   bool closedWorld = false;
   // Whether to try to preserve debug info through, which are special calls.
   bool debugInfo = false;
diff --git a/src/passes/AbstractTypeRefining.cpp b/src/passes/AbstractTypeRefining.cpp
index 313ad5f4d..509448851 100644
--- a/src/passes/AbstractTypeRefining.cpp
+++ b/src/passes/AbstractTypeRefining.cpp
@@ -106,6 +106,16 @@ struct AbstractTypeRefining : public Pass {
       }
     }
 
+    // Assume all public types are created, which makes them non-abstract and
+    // hence ignored below.
+    // TODO: In principle we could assume such types are not created outside the
+    //       module, given closed world, but we'd also need to make sure that
+    //       we don't need to make any changes to public types that refer to
+    //       them.
+    for (auto type : ModuleUtils::getPublicHeapTypes(*module)) {
+      createdTypes.insert(type);
+    }
+
     SubTypes subTypes(*module);
 
     // Compute createdTypesOrSubTypes by starting with the created types and
diff --git a/src/passes/GlobalTypeOptimization.cpp b/src/passes/GlobalTypeOptimization.cpp
index 1f0da2595..74107f9cd 100644
--- a/src/passes/GlobalTypeOptimization.cpp
+++ b/src/passes/GlobalTypeOptimization.cpp
@@ -198,6 +198,20 @@ struct GlobalTypeOptimization : public Pass {
           continue;
         }
 
+        // The propagation analysis ensures we update immutability in all
+        // supers and subs in concert, but it does not take into account
+        // visibility, so do that here: we can only become immutable if the
+        // parent can as well.
+        auto super = type.getDeclaredSuperType();
+        if (super && !canBecomeImmutable.count(*super)) {
+          // No entry in canBecomeImmutable means nothing in the parent can
+          // become immutable. We don't need to check the specific field index,
+          // because visibility affects them all equally (i.e., if it is public
+          // then no field can be changed, and if it is private then this field
+          // can be changed, and perhaps more).
+          continue;
+        }
+
         // No set exists. Mark it as something we can make immutable.
         auto& vec = canBecomeImmutable[type];
         vec.resize(i + 1);
diff --git a/src/wasm/wasm-validator.cpp b/src/wasm/wasm-validator.cpp
index 0184e3284..d1ca5220c 100644
--- a/src/wasm/wasm-validator.cpp
+++ b/src/wasm/wasm-validator.cpp
@@ -63,7 +63,6 @@ struct ValidationInfo {
   bool validateWeb;
   bool validateGlobally;
   bool quiet;
-  bool closedWorld;
 
   std::atomic<bool> valid;
 
@@ -4135,46 +4134,6 @@ static void validateFeatures(Module& module, ValidationInfo& info) {
   }
 }
 
-static void validateClosedWorldInterface(Module& module, ValidationInfo& info) {
-  // Error if there are any publicly exposed heap types beyond the types of
-  // publicly exposed functions. Note that we must include all types in the rec
-  // groups that are used, as if a type if public then all types in its rec
-  // group are as well.
-  std::unordered_set<RecGroup> publicRecGroups;
-  ModuleUtils::iterImportedFunctions(module, [&](Function* func) {
-    publicRecGroups.insert(func->type.getRecGroup());
-  });
-  for (auto& ex : module.exports) {
-    if (ex->kind == ExternalKind::Function) {
-      publicRecGroups.insert(module.getFunction(ex->value)->type.getRecGroup());
-    }
-  }
-
-  std::unordered_set<HeapType> publicTypes;
-  for (auto& group : publicRecGroups) {
-    for (auto type : group) {
-      publicTypes.insert(type);
-    }
-  }
-
-  // Ignorable public types are public, but we can ignore them for purposes of
-  // erroring here: It is always ok that they are public.
-  auto ignorable = getIgnorablePublicTypes();
-
-  for (auto type : ModuleUtils::getPublicHeapTypes(module)) {
-    if (!publicTypes.count(type) && !ignorable.count(type)) {
-      auto name = type.toString();
-      if (auto it = module.typeNames.find(type); it != module.typeNames.end()) {
-        name = it->second.name.toString();
-      }
-      info.fail("publicly exposed type disallowed with a closed world: $" +
-                  name,
-                type,
-                nullptr);
-    }
-  }
-}
-
 // TODO: If we want the validator to be part of libwasm rather than libpasses,
 // then Using PassRunner::getPassDebug causes a circular dependence. We should
 // fix that, perhaps by moving some of the pass infrastructure into libsupport.
@@ -4183,7 +4142,6 @@ bool WasmValidator::validate(Module& module, Flags flags) {
   info.validateWeb = (flags & Web) != 0;
   info.validateGlobally = (flags & Globally) != 0;
   info.quiet = (flags & Quiet) != 0;
-  info.closedWorld = (flags & ClosedWorld) != 0;
 
   // Parallel function validation.
   PassRunner runner(&module);
@@ -4210,9 +4168,6 @@ bool WasmValidator::validate(Module& module, Flags flags) {
     validateStart(module, info);
     validateModuleMaps(module, info);
     validateFeatures(module, info);
-    if (info.closedWorld) {
-      validateClosedWorldInterface(module, info);
-    }
   }
 
   // Validate additional internal IR details when in pass-debug mode.
@@ -4231,11 +4186,7 @@ bool WasmValidator::validate(Module& module, Flags flags) {
 }
 
 bool WasmValidator::validate(Module& module, const PassOptions& options) {
-  Flags flags = options.validateGlobally ? Globally : Minimal;
-  if (options.closedWorld) {
-    flags |= ClosedWorld;
-  }
-  return validate(module, flags);
+  return validate(module, options.validateGlobally ? Globally : Minimal);
 }
 
 bool WasmValidator::validate(Function* func, Module& module, Flags flags) {
diff --git a/test/lit/passes/abstract-type-refining.wast b/test/lit/passes/abstract-type-refining.wast
index 814f5c1f0..e887894ea 100644
--- a/test/lit/passes/abstract-type-refining.wast
+++ b/test/lit/passes/abstract-type-refining.wast
@@ -1304,3 +1304,55 @@
     )
   )
 )
+
+;; $A is never created, but $B is, so all appearances of $A, like in the cast
+;; and the struct field, could be replaced by $B, except that $A is a public type,
+;; so might be created outside the module.
+(module
+  (rec
+    ;; YESTNH:      (rec
+    ;; YESTNH-NEXT:  (type $A (sub (struct (field (ref null $A)))))
+    ;; NO_TNH:      (rec
+    ;; NO_TNH-NEXT:  (type $A (sub (struct (field (ref null $A)))))
+    (type $A (sub (struct (field (ref null $A)))))
+
+    ;; YESTNH:       (type $B (sub $A (struct (field (ref null $A)))))
+    ;; NO_TNH:       (type $B (sub $A (struct (field (ref null $A)))))
+    (type $B (sub $A (struct (field (ref null $A)))))
+  )
+
+  ;; YESTNH:      (type $2 (func (param anyref)))
+
+  ;; YESTNH:      (global $global (ref $B) (struct.new_default $B))
+  ;; NO_TNH:      (type $2 (func (param anyref)))
+
+  ;; NO_TNH:      (global $global (ref $B) (struct.new_default $B))
+  (global $global (ref $B) (struct.new_default $B))
+
+  ;; YESTNH:      (export "global" (global $global))
+  ;; NO_TNH:      (export "global" (global $global))
+  (export "global" (global $global))
+
+  ;; YESTNH:      (func $new (type $2) (param $x anyref)
+  ;; YESTNH-NEXT:  (drop
+  ;; YESTNH-NEXT:   (ref.cast (ref $A)
+  ;; YESTNH-NEXT:    (local.get $x)
+  ;; YESTNH-NEXT:   )
+  ;; YESTNH-NEXT:  )
+  ;; YESTNH-NEXT: )
+  ;; NO_TNH:      (func $new (type $2) (param $x anyref)
+  ;; NO_TNH-NEXT:  (drop
+  ;; NO_TNH-NEXT:   (ref.cast (ref $A)
+  ;; NO_TNH-NEXT:    (local.get $x)
+  ;; NO_TNH-NEXT:   )
+  ;; NO_TNH-NEXT:  )
+  ;; NO_TNH-NEXT: )
+  (func $new (param $x anyref)
+    (drop
+      (ref.cast (ref $A)
+        (local.get $x)
+      )
+    )
+  )
+)
+
diff --git a/test/lit/passes/gto-mutability.wast b/test/lit/passes/gto-mutability.wast
index 13b416b1b..3b01572c8 100644
--- a/test/lit/passes/gto-mutability.wast
+++ b/test/lit/passes/gto-mutability.wast
@@ -652,3 +652,39 @@
     )
   )
 )
+
+;; The parent is public, which prevents us from making any field immutable in
+;; the child.
+(module
+  ;; CHECK:      (type $parent (sub (struct (field (mut i32)))))
+  (type $parent (sub (struct (field (mut i32)))))
+  ;; CHECK:      (type $1 (func))
+
+  ;; CHECK:      (type $child (sub $parent (struct (field (mut i32)))))
+  (type $child (sub $parent (struct (field (mut i32)))))
+
+  ;; CHECK:      (global $global (ref $parent) (struct.new $parent
+  ;; CHECK-NEXT:  (i32.const 0)
+  ;; CHECK-NEXT: ))
+  (global $global (ref $parent) (struct.new $parent
+    (i32.const 0)
+  ))
+
+  ;; Make the parent public by exporting the global.
+  ;; CHECK:      (export "global" (global $global))
+  (export "global" (global $global))
+
+  ;; CHECK:      (func $func (type $1)
+  ;; CHECK-NEXT:  (drop
+  ;; CHECK-NEXT:   (struct.new_default $child)
+  ;; CHECK-NEXT:  )
+  ;; CHECK-NEXT: )
+  (func $func
+    ;; Create the child so the type is used. No sets to the fields exist, so
+    ;; in theory all fields could be immutable.
+    (drop
+      (struct.new_default $child)
+    )
+  )
+)
+
diff --git a/test/lit/validation/closed-world-interface-intrinsics.wast b/test/lit/validation/closed-world-interface-intrinsics.wast
deleted file mode 100644
index 50151b8c0..000000000
--- a/test/lit/validation/closed-world-interface-intrinsics.wast
+++ /dev/null
@@ -1,46 +0,0 @@
-;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited.
-
-;; RUN: foreach %s %t wasm-opt -all --closed-world -S -o - | filecheck %s
-
-;; Test that we do not error on call.without.effects despite it being an import.
-;; call.without.effects does not make the types in it public, and so it can
-;; validate with --closed-world.
-
-(module
-  ;; CHECK:      (type $struct (struct (field i32)))
-  (type $struct (struct i32))
-
-  ;; CHECK:      (type $1 (func (param (ref $struct) funcref)))
-
-  ;; CHECK:      (type $2 (func))
-
-  ;; CHECK:      (type $3 (func (param (ref $struct))))
-
-  ;; CHECK:      (import "binaryen-intrinsics" "call.without.effects" (func $cwe (type $1) (param (ref $struct) funcref)))
-  (import "binaryen-intrinsics" "call.without.effects" (func $cwe (param (ref $struct)) (param funcref)))
-
-  ;; CHECK:      (elem declare func $func)
-
-  ;; CHECK:      (func $test (type $2)
-  ;; CHECK-NEXT:  (call $cwe
-  ;; CHECK-NEXT:   (struct.new $struct
-  ;; CHECK-NEXT:    (i32.const 100)
-  ;; CHECK-NEXT:   )
-  ;; CHECK-NEXT:   (ref.func $func)
-  ;; CHECK-NEXT:  )
-  ;; CHECK-NEXT: )
-  (func $test
-    (call $cwe
-      (struct.new $struct
-        (i32.const 100)
-      )
-      (ref.func $func)
-    )
-  )
-
-  ;; CHECK:      (func $func (type $3) (param $ref (ref $struct))
-  ;; CHECK-NEXT:  (nop)
-  ;; CHECK-NEXT: )
-  (func $func (param $ref (ref $struct))
-  )
-)
diff --git a/test/lit/validation/closed-world-interface.wast b/test/lit/validation/closed-world-interface.wast
deleted file mode 100644
index b4fe965cf..000000000
--- a/test/lit/validation/closed-world-interface.wast
+++ /dev/null
@@ -1,79 +0,0 @@
-;; Test that we error out on nontrivial public types with --closed-world
-
-;; RUN: not wasm-opt -all --closed-world %s 2>&1 | filecheck %s
-
-
-;; This is pulled in by a global.
-;; CHECK:      publicly exposed type disallowed with a closed world: $array, on
-;; CHECK-NEXT: (array (mut i32))
-
-;; This is pulled in only by a global, so it is disallowed even though it is a function type.
-;; CHECK:      publicly exposed type disallowed with a closed world: $private, on
-;; CHECK-NEXT: (func (param v128))
-
-;; This is referred to by the type of a function export, but is still not allowed.
-;; CHECK:      publicly exposed type disallowed with a closed world: $struct, on
-;; CHECK-NEXT: (struct)
-
-(module
-  (type $struct (struct))
-  (type $array (array (mut i32)))
-
-  (type $void (func))
-  (type $abstract (func (param anyref)))
-  (type $concrete (func (param (ref null $struct))))
-
-  (rec
-    (type $exported-pair-0 (func (param (ref $exported-pair-1))))
-    (type $exported-pair-1 (func (param (ref $exported-pair-0))))
-  )
-  (rec
-    ;; This is on an exported function.
-    (type $partial-pair-0 (func))
-    ;; The latter type types are not public, but allowed to be because the
-    ;; entire rec group is allowed due to the first.
-    (type $partial-pair-1 (func))
-    ;; Test a non-function type.
-    (type $partial-pair-2 (struct))
-  )
-
-  (type $private (func (param v128)))
-
-  ;; Ok even though it is an import instead of an export.
-  (func $1 (import "env" "test5") (type $exported-pair-1))
-
-  (func $2 (export "test1") (type $void)
-    (unreachable)
-  )
-
-  ;; Ok because it only refers to basic heap types
-  (func $3 (export "test2") (type $abstract)
-    (unreachable)
-  )
-
-  ;; Not ok because it refers to $struct.
-  (func $4 (export "test3") (type $concrete)
-    (unreachable)
-  )
-
-  ;; Ok even though it is in a rec group because the rest of the group and the
-  ;; types this refers to are on the boundary as well.
-  (func $5 (export "test4") (type $exported-pair-0)
-    (unreachable)
-  )
-
-  ;; Ok, and we also allow the other type in the group.
-  (func $6 (export "test6") (type $partial-pair-0)
-    (unreachable)
-  )
-
-  ;; Not ok.
-  (global $1 (export "g1") (ref null $array) (ref.null none))
-
-  ;; Ok because this type is on the boundary anyway.
-  (global $2 (export "g2") (ref null $void) (ref.null func))
-
-  ;; Not ok even though it is a function type because it is not otherwise on the
-  ;; boundary.
-  (global $3 (export "g3") (ref null $private) (ref.null func))
-)