module: fix detect-module not retrying as esm for cjs-only errors

nodejs · nodejs-github-bot · Mar 12, 2024 · Mar 9, 2024 · Mar 10, 2024 · Mar 10, 2024
commit f44c31ce887b4878825b2cd4cda955dc8d7a698a
diff --git a/src/node_contextify.cc b/src/node_contextify.cc
@@ -1409,6 +1409,25 @@ constexpr std::array<std::string_view, 3> esm_syntax_error_messages = {
     "Unexpected token 'export'",                     // `export` statements
     "Cannot use 'import.meta' outside a module"};    // `import.meta` references
 
+// Another class of error messages that we need to check for are syntax errors
+// where the syntax throws when parsed as CommonJS but succeeds when parsed as
+// ESM. So far, the cases we've found are:
+// - CommonJS module variables (`module`, `exports`, `require`, `__filename`,
+//   `__dirname`): if the user writes code such as `const module =` in the top
+//   level of a CommonJS module, it will throw a syntax error; but the same
+//   code is valid in ESM.
+// - Top-level `await`: if the user writes `await` at the top level of a
+//   CommonJS module, it will throw a syntax error; but the same code is valid
+//   in ESM.
+constexpr std::array<std::string_view, 6> throws_only_in_cjs_error_messages = {
+    "Identifier 'module' has already been declared",
+    "Identifier 'exports' has already been declared",
+    "Identifier 'require' has already been declared",
+    "Identifier '__filename' has already been declared",
+    "Identifier '__dirname' has already been declared",
+    "await is only valid in async functions and "
+    "the top level bodies of modules"};
+
 void ContextifyContext::ContainsModuleSyntax(
     const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
@@ -1476,19 +1495,61 @@ void ContextifyContext::ContainsModuleSyntax(
                                                    id_symbol,
                                                    try_catch);
 
-  bool found_error_message_caused_by_module_syntax = false;
+  bool should_retry_as_esm = false;
   if (try_catch.HasCaught() && !try_catch.HasTerminated()) {
     Utf8Value message_value(env->isolate(), try_catch.Message()->Get());
     auto message = message_value.ToStringView();
 
     for (const auto& error_message : esm_syntax_error_messages) {
       if (message.find(error_message) != std::string_view::npos) {
-        found_error_message_caused_by_module_syntax = true;
+        should_retry_as_esm = true;
+        break;
+      }
+    }
+
+    for (const auto& error_message : throws_only_in_cjs_error_messages) {
+      if (message.find(error_message) != std::string_view::npos) {
+        // Try parsing again where the user's code is wrapped within an async
+        // function. If the new parse succeeds, then the error was caused by
+        // either a top-level declaration of one of the CommonJS module
+        // variables, or a top-level `await`.
+        TryCatchScope second_parse_try_catch(env);
+        Local<String> wrapped_code =
+            String::Concat(isolate,
+                           String::NewFromUtf8(isolate, "(async function() {")
+                               .ToLocalChecked(),
+                           code);
+        wrapped_code = String::Concat(
+            isolate,
+            wrapped_code,
+            String::NewFromUtf8(isolate, "})();").ToLocalChecked());
+        ScriptCompiler::Source wrapped_source =
+            GetCommonJSSourceInstance(isolate,
+                                      wrapped_code,
+                                      filename,
+                                      0,
+                                      0,
+                                      host_defined_options,
+                                      nullptr);
+        ContextifyContext::CompileFunctionAndCacheResult(
+            env,
+            context,
+            &wrapped_source,
+            std::move(params),
+            std::vector<Local<Object>>(),
+            options,
+            true,
+            id_symbol,
+            second_parse_try_catch);
+        if (!second_parse_try_catch.HasCaught() &&
+            !second_parse_try_catch.HasTerminated()) {
+          should_retry_as_esm = true;
+        }
         break;
       }
     }
   }
-  args.GetReturnValue().Set(found_error_message_caused_by_module_syntax);
+  args.GetReturnValue().Set(should_retry_as_esm);
 }
 
 static void CompileFunctionForCJSLoader(

diff --git a/test/es-module/test-esm-detect-ambiguous.mjs b/test/es-module/test-esm-detect-ambiguous.mjs
@@ -234,6 +234,47 @@ describe('--experimental-detect-module', { concurrency: true }, () => {
       });
     }
   });
+
+  // https://github.com/nodejs/node/issues/50917
+  describe('syntax that errors in CommonJS but works in ESM', { concurrency: true }, () => {
+    it('permits top-level `await`', async () => {
+      const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
+        '--experimental-detect-module',
+        '--eval',
+        'await Promise.resolve(); console.log("executed");',
+      ]);
+
+      strictEqual(stderr, '');
+      strictEqual(stdout, 'executed\n');
+      strictEqual(code, 0);
+      strictEqual(signal, null);
+    });
+
+    it('still throws on `await` in an ordinary sync function', async () => {
+      const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
+        '--experimental-detect-module',
+        '--eval',
+        'function fn() { await Promise.resolve(); } fn();',
+      ]);
+
+      match(stderr, /SyntaxError: await is only valid in async function/);
+      strictEqual(stdout, '');
+      strictEqual(code, 1);
+      strictEqual(signal, null);
+    });
+
+    it('permits declaration of CommonJS module variables', async () => {
+      const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
+        '--experimental-detect-module',
+        fixtures.path('es-modules/package-without-type/commonjs-wrapper-variables.js'),
+      ]);
+
+      strictEqual(stderr, '');
+      strictEqual(stdout, 'exports require module __filename __dirname\n');
+      strictEqual(code, 0);
+      strictEqual(signal, null);
+    });
+  });
 });
 
 // Validate temporarily disabling `--abort-on-uncaught-exception`

diff --git a/test/fixtures/es-modules/package-without-type/commonjs-wrapper-variables.js b/test/fixtures/es-modules/package-without-type/commonjs-wrapper-variables.js
@@ -0,0 +1,6 @@
+const exports = "exports";
+const require = "require";
+const module = "module";
+const __filename = "__filename";
+const __dirname = "__dirname";
+console.log(exports, require, module, __filename, __dirname);