Address PR comments

Author: stevenfontanella

scripts/test/shared.py

@@ -443,7 +443,6 @@ def get_tests(test_dir, extensions=[], recursive=False):
     'imports.wast',  # Missing validation of missing function on instantiation
     'proposals/threads/imports.wast',  # Missing memory type validation on instantiation
     'linking.wast',  # Missing function type validation on instantiation
-    # 'memory.wast',   # Requires wast `module definition` support
     'proposals/threads/memory.wast',  # Missing memory type validation on instantiation
     'memory64-imports.wast',  # Missing validation on instantiation
     'annotations.wast',  # String annotations IDs should be allowed

scripts/test/support.py

@@ -151,14 +151,12 @@ def to_end(j):
             break
         i = to_end(start + 1)
         chunk = wast[start:i]
-        if QUOTED.match(chunk):
+        if QUOTED.match(chunk) or MODULE_DEFINITION_OR_INSTANCE.match(chunk):
             # There may be assertions after this quoted module, but we aren't
             # returning the module, so we need to skip the assertions as well.
             ignoring_quoted = True
             continue
         if chunk.startswith('(module'):
-            if MODULE_DEFINITION_OR_INSTANCE.match(chunk):
-                continue
             ignoring_quoted = False
             ret += [(chunk, [])]
         elif chunk.startswith('(assert_invalid'):

src/parser/wast-parser.cpp

@@ -91,6 +91,7 @@ Result<Action> action(Lexer& in) {
 // (module id binary string*)
 // (module id quote string*)
 // (module definition id? ...)
+// (module definition id? binary ...)
 Result<WASTModule> wastModule(Lexer& in, bool maybeInvalid = false) {
   Lexer reset = in;
   if (!in.takeSExprStart("module"sv)) {
@@ -122,7 +123,7 @@ Result<WASTModule> wastModule(Lexer& in, bool maybeInvalid = false) {
       }
     }
     std::string mod(reset.next().substr(0, in.getPos() - reset.getPos()));
-    return QuotedModule{!isDefinition, QuotedModuleType::Text, mod};
+    return WASTModule{isDefinition, QuotedModule{QuotedModuleType::Text, mod}};
   } else {
     // In this case the module is mostly valid WAT, unless it is a module
     // definition in which case it will begin with (module definition ...)
@@ -146,8 +147,7 @@ Result<WASTModule> wastModule(Lexer& in, bool maybeInvalid = false) {
       return in.err("expected end of module");
     }
 
-    wasm->isDefinition = isDefinition;
-    return wasm;
+    return WASTModule{isDefinition, wasm};
   }
 
   // We have a quote or binary module. Collect its contents.
@@ -160,7 +160,7 @@ Result<WASTModule> wastModule(Lexer& in, bool maybeInvalid = false) {
     return in.err("expected end of module");
   }
 
-  return QuotedModule{isDefinition, type, ss.str()};
+  return WASTModule{isDefinition, QuotedModule{type, ss.str()}};
 }
 
 Result<NaNKind> nan(Lexer& in) {
@@ -558,7 +558,7 @@ Result<WASTScript> wast(Lexer& in) {
         // No, that wasn't the problem. Return the original error.
         return Err{err->msg};
       }
-      cmds.push_back({WASTModule{std::move(wasm)}, line});
+      cmds.push_back({WASTModule{/*isDefinition=*/false, std::move(wasm)}, line});
       return cmds;
     }
     CHECK_ERR(cmd);

src/parser/wat-parser.h

@@ -93,12 +93,14 @@ struct AssertAction {
 enum class QuotedModuleType { Text, Binary };
 
 struct QuotedModule {
-  bool isDefinition = false;
   QuotedModuleType type;
   std::string module;
 };
 
-using WASTModule = std::variant<QuotedModule, std::shared_ptr<Module>>;
+struct WASTModule {
+  bool isDefinition = false;
+  std::variant<QuotedModule, std::shared_ptr<Module>> module;
+};
 
 enum class ModuleAssertionType { Trap, Malformed, Invalid, Unlinkable };
 

src/tools/wasm-shell.cpp

@@ -39,13 +39,13 @@ using namespace wasm;
 using namespace wasm::WATParser;
 
 struct Shell {
-  // Keyed by module name
+  // Keyed by module name.
   std::map<Name, std::shared_ptr<Module>> modules;
 
-  // Keyed by instance name
+  // Keyed by instance name.
   std::map<Name, std::shared_ptr<ShellExternalInterface>> interfaces;
   std::map<Name, std::shared_ptr<ModuleRunner>> instances;
-  // used for imports, keyed by instance name
+  // Used for imports, keyed by instance name.
   std::map<Name, std::shared_ptr<ModuleRunner>> linkedInstances;
 
   Name lastModule;
@@ -100,12 +100,11 @@ struct Shell {
 
   Result<std::shared_ptr<Module>> makeModule(WASTModule& mod) {
     std::shared_ptr<Module> wasm;
-    if (auto* quoted = std::get_if<QuotedModule>(&mod)) {
+    if (auto* quoted = std::get_if<QuotedModule>(&mod.module)) {
       wasm = std::make_shared<Module>();
       switch (quoted->type) {
         case QuotedModuleType::Text: {
           CHECK_ERR(parseModule(*wasm, quoted->module));
-          wasm->isDefinition = quoted->isDefinition;
           break;
         }
         case QuotedModuleType::Binary: {
@@ -119,11 +118,10 @@ struct Shell {
             p.dump(ss);
             return Err{ss.str()};
           }
-          wasm->isDefinition = quoted->isDefinition;
           break;
         }
       }
-    } else if (auto* ptr = std::get_if<std::shared_ptr<Module>>(&mod)) {
+    } else if (auto* ptr = std::get_if<std::shared_ptr<Module>>(&mod.module)) {
       wasm = *ptr;
     } else {
       WASM_UNREACHABLE("unexpected module kind");
@@ -173,7 +171,7 @@ struct Shell {
 
     lastModule = wasm->name;
     modules[lastModule] = wasm;
-    if (!wasm->isDefinition) {
+    if (!mod.isDefinition) {
       CHECK_ERR(instantiate(*wasm, wasm->name));
     }
 
@@ -530,7 +528,7 @@ struct Shell {
 
     // print_* functions are handled separately, no need to define here.
 
-    WASTModule mod = std::move(spectest);
+    WASTModule mod = {/*isDefinition=*/false, spectest};
     auto added = addModule(mod);
     if (added.getErr()) {
       WASM_UNREACHABLE("error building spectest module");

src/tools/wasm2js.cpp

@@ -788,7 +788,10 @@ void AssertionEmitter::emit() {
     Name testFuncName("check" + std::to_string(i));
     auto& cmd = script[i].cmd;
     if (auto* mod = std::get_if<WASTModule>(&cmd)) {
-      if (auto* w = std::get_if<std::shared_ptr<Module>>(mod)) {
+      if (mod->isDefinition) {
+        Fatal() << "Module definition is not supported on line " << script[i].line;
+      }
+      if (auto* w = std::get_if<std::shared_ptr<Module>>(&mod->module)) {
         wasm = *w;
         // We have already done the parse, but we still do this to apply the
         // features from the command line.
@@ -951,7 +954,10 @@ int main(int argc, const char* argv[]) {
         Fatal() << "expected module";
       }
       if (auto* mod = std::get_if<WASTModule>(&(*script)[0].cmd)) {
-        if (auto* w = std::get_if<std::shared_ptr<Module>>(mod)) {
+        if (mod->isDefinition) {
+          Fatal() << "module definition is not supported";
+        }
+        if (auto* w = std::get_if<std::shared_ptr<Module>>(&mod->module)) {
           wasm = *w;
           // This isn't actually before the parse, but we can't apply the
           // feature options any earlier. FIXME.

src/wasm.h

@@ -2519,8 +2519,6 @@ class Module {
 
   Name start;
 
-  bool isDefinition = false;
-
   std::vector<CustomSection> customSections;
 
   // Optional user section IR representation.