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.

Author: kripken

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;

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

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);

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) {

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)
+      )
+    )
+  )
+)
+

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)
+    )
+  )
+)
+