esm: improve check for ESM syntax

GeoffreyBooth · targos · commit c76eb2797124 · 2023-10-23T08:19:34.000+02:00
PR-URL: #50127 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -90,6 +90,7 @@ const {
   makeContextifyScript,
   runScriptInThisContext,
 } = require('internal/vm');
+const { containsModuleSyntax } = internalBinding('contextify');
 
 const assert = require('internal/assert');
 const fs = require('fs');
@@ -104,7 +105,6 @@ const {
 const {
   getCjsConditions,
   initializeCjsConditions,
-  hasEsmSyntax,
   loadBuiltinModule,
   makeRequireFunction,
   normalizeReferrerURL,
@@ -1315,7 +1315,7 @@ function wrapSafe(filename, content, cjsModuleInstance, codeCache) {
   } catch (err) {
     if (process.mainModule === cjsModuleInstance) {
       const { enrichCJSError } = require('internal/modules/esm/translators');
-      enrichCJSError(err, content);
+      enrichCJSError(err, content, filename);
     }
     throw err;
   }
@@ -1400,10 +1400,11 @@ Module._extensions['.js'] = function(module, filename) {
     const pkg = packageJsonReader.readPackageScope(filename) || { __proto__: null };
     // Function require shouldn't be used in ES modules.
     if (pkg.data?.type === 'module') {
+      // This is an error path because `require` of a `.js` file in a `"type": "module"` scope is not allowed.
       const parent = moduleParentCache.get(module);
       const parentPath = parent?.filename;
       const packageJsonPath = path.resolve(pkg.path, 'package.json');
-      const usesEsm = hasEsmSyntax(content);
+      const usesEsm = containsModuleSyntax(content, filename);
       const err = new ERR_REQUIRE_ESM(filename, usesEsm, parentPath,
                                       packageJsonPath);
       // Attempt to reconstruct the parent require frame.
diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js
@@ -30,11 +30,11 @@ function lazyTypes() {
   return _TYPES = require('internal/util/types');
 }
 
+const { containsModuleSyntax } = internalBinding('contextify');
 const assert = require('internal/assert');
 const { readFileSync } = require('fs');
 const { dirname, extname, isAbsolute } = require('path');
 const {
-  hasEsmSyntax,
   loadBuiltinModule,
   stripBOM,
 } = require('internal/modules/helpers');
@@ -166,11 +166,11 @@ translators.set('module', async function moduleStrategy(url, source, isMain) {
  * Provide a more informative error for CommonJS imports.
  * @param {Error | any} err
  * @param {string} [content] Content of the file, if known.
- * @param {string} [filename] Useful only if `content` is unknown.
+ * @param {string} [filename] The filename of the erroring module.
  */
 function enrichCJSError(err, content, filename) {
   if (err != null && ObjectGetPrototypeOf(err) === SyntaxErrorPrototype &&
-      hasEsmSyntax(content || readFileSync(filename, 'utf-8'))) {
+      containsModuleSyntax(content, filename)) {
     // Emit the warning synchronously because we are in the middle of handling
     // a SyntaxError that will throw and likely terminate the process before an
     // asynchronous warning would be emitted.
@@ -217,7 +217,7 @@ function loadCJSModule(module, source, url, filename) {
       importModuleDynamically,        // importModuleDynamically
     ).function;
   } catch (err) {
-    enrichCJSError(err, source, url);
+    enrichCJSError(err, source, filename);
     throw err;
   }
 
@@ -344,7 +344,7 @@ translators.set('commonjs', async function commonjsStrategy(url, source,
       assert(module === CJSModule._cache[filename]);
       CJSModule._load(filename);
     } catch (err) {
-      enrichCJSError(err, source, url);
+      enrichCJSError(err, source, filename);
       throw err;
     }
   } : loadCJSModule;
diff --git a/lib/internal/modules/helpers.js b/lib/internal/modules/helpers.js
@@ -3,7 +3,6 @@
 const {
   ArrayPrototypeForEach,
   ArrayPrototypeJoin,
-  ArrayPrototypeSome,
   ObjectDefineProperty,
   ObjectPrototypeHasOwnProperty,
   SafeMap,
@@ -299,32 +298,10 @@ function normalizeReferrerURL(referrer) {
   return new URL(referrer).href;
 }
 
-/**
- * For error messages only, check if ESM syntax is in use.
- * @param {string} code
- */
-function hasEsmSyntax(code) {
-  debug('Checking for ESM syntax');
-  const parser = require('internal/deps/acorn/acorn/dist/acorn').Parser;
-  let root;
-  try {
-    root = parser.parse(code, { sourceType: 'module', ecmaVersion: 'latest' });
-  } catch {
-    return false;
-  }
-
-  return ArrayPrototypeSome(root.body, (stmt) =>
-    stmt.type === 'ExportDefaultDeclaration' ||
-    stmt.type === 'ExportNamedDeclaration' ||
-    stmt.type === 'ImportDeclaration' ||
-    stmt.type === 'ExportAllDeclaration');
-}
-
 module.exports = {
   addBuiltinLibsToObject,
   getCjsConditions,
   initializeCjsConditions,
-  hasEsmSyntax,
   loadBuiltinModule,
   makeRequireFunction,
   normalizeReferrerURL,
diff --git a/src/node_contextify.cc b/src/node_contextify.cc
@@ -318,13 +318,15 @@ void ContextifyContext::CreatePerIsolateProperties(
   SetMethod(isolate, target, "makeContext", MakeContext);
   SetMethod(isolate, target, "isContext", IsContext);
   SetMethod(isolate, target, "compileFunction", CompileFunction);
+  SetMethod(isolate, target, "containsModuleSyntax", ContainsModuleSyntax);
 }
 
 void ContextifyContext::RegisterExternalReferences(
     ExternalReferenceRegistry* registry) {
   registry->Register(MakeContext);
   registry->Register(IsContext);
   registry->Register(CompileFunction);
+  registry->Register(ContainsModuleSyntax);
   registry->Register(PropertyGetterCallback);
   registry->Register(PropertySetterCallback);
   registry->Register(PropertyDescriptorCallback);
@@ -1204,33 +1206,18 @@ void ContextifyContext::CompileFunction(
       data + cached_data_buf->ByteOffset(), cached_data_buf->ByteLength());
   }
 
-  // Set host_defined_options
   Local<PrimitiveArray> host_defined_options =
-      PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength);
-  host_defined_options->Set(
-      isolate, loader::HostDefinedOptions::kID, id_symbol);
-
-  ScriptOrigin origin(isolate,
-                      filename,
-                      line_offset,       // line offset
-                      column_offset,     // column offset
-                      true,              // is cross origin
-                      -1,                // script id
-                      Local<Value>(),    // source map URL
-                      false,             // is opaque (?)
-                      false,             // is WASM
-                      false,             // is ES Module
-                      host_defined_options);
-
-  ScriptCompiler::Source source(code, origin, cached_data);
-  ScriptCompiler::CompileOptions options;
-  if (source.GetCachedData() == nullptr) {
-    options = ScriptCompiler::kNoCompileOptions;
-  } else {
-    options = ScriptCompiler::kConsumeCodeCache;
-  }
+      GetHostDefinedOptions(isolate, id_symbol);
+  ScriptCompiler::Source source =
+      GetCommonJSSourceInstance(isolate,
+                                code,
+                                filename,
+                                line_offset,
+                                column_offset,
+                                host_defined_options,
+                                cached_data);
+  ScriptCompiler::CompileOptions options = GetCompileOptions(source);
 
-  TryCatchScope try_catch(env);
   Context::Scope scope(parsing_context);
 
   // Read context extensions from buffer
@@ -1255,9 +1242,83 @@ void ContextifyContext::CompileFunction(
     }
   }
 
+  TryCatchScope try_catch(env);
+  Local<Object> result = CompileFunctionAndCacheResult(env,
+                                                       parsing_context,
+                                                       &source,
+                                                       params,
+                                                       context_extensions,
+                                                       options,
+                                                       produce_cached_data,
+                                                       id_symbol,
+                                                       try_catch);
+
+  if (try_catch.HasCaught() && !try_catch.HasTerminated()) {
+    try_catch.ReThrow();
+    return;
+  }
+
+  if (result.IsEmpty()) {
+    return;
+  }
+  args.GetReturnValue().Set(result);
+}
+
+Local<PrimitiveArray> ContextifyContext::GetHostDefinedOptions(
+    Isolate* isolate, Local<Symbol> id_symbol) {
+  Local<PrimitiveArray> host_defined_options =
+      PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength);
+  host_defined_options->Set(
+      isolate, loader::HostDefinedOptions::kID, id_symbol);
+  return host_defined_options;
+}
+
+ScriptCompiler::Source ContextifyContext::GetCommonJSSourceInstance(
+    Isolate* isolate,
+    Local<String> code,
+    Local<String> filename,
+    int line_offset,
+    int column_offset,
+    Local<PrimitiveArray> host_defined_options,
+    ScriptCompiler::CachedData* cached_data) {
+  ScriptOrigin origin(isolate,
+                      filename,
+                      line_offset,     // line offset
+                      column_offset,   // column offset
+                      true,            // is cross origin
+                      -1,              // script id
+                      Local<Value>(),  // source map URL
+                      false,           // is opaque (?)
+                      false,           // is WASM
+                      false,           // is ES Module
+                      host_defined_options);
+  return ScriptCompiler::Source(code, origin, cached_data);
+}
+
+ScriptCompiler::CompileOptions ContextifyContext::GetCompileOptions(
+    const ScriptCompiler::Source& source) {
+  ScriptCompiler::CompileOptions options;
+  if (source.GetCachedData() != nullptr) {
+    options = ScriptCompiler::kConsumeCodeCache;
+  } else {
+    options = ScriptCompiler::kNoCompileOptions;
+  }
+  return options;
+}
+
+Local<Object> ContextifyContext::CompileFunctionAndCacheResult(
+    Environment* env,
+    Local<Context> parsing_context,
+    ScriptCompiler::Source* source,
+    std::vector<Local<String>> params,
+    std::vector<Local<Object>> context_extensions,
+    ScriptCompiler::CompileOptions options,
+    bool produce_cached_data,
+    Local<Symbol> id_symbol,
+    const TryCatchScope& try_catch) {
   MaybeLocal<Function> maybe_fn = ScriptCompiler::CompileFunction(
       parsing_context,
-      &source,
+      source,
       params.size(),
       params.data(),
       context_extensions.size(),
@@ -1269,24 +1330,26 @@ void ContextifyContext::CompileFunction(
   if (!maybe_fn.ToLocal(&fn)) {
     if (try_catch.HasCaught() && !try_catch.HasTerminated()) {
       errors::DecorateErrorStack(env, try_catch);
-      try_catch.ReThrow();
+      return Object::New(env->isolate());
     }
-    return;
   }
+
+  Local<Context> context = env->context();
   if (fn->SetPrivate(context, env->host_defined_option_symbol(), id_symbol)
           .IsNothing()) {
-    return;
+    return Object::New(env->isolate());
   }
 
+  Isolate* isolate = env->isolate();
   Local<Object> result = Object::New(isolate);
   if (result->Set(parsing_context, env->function_string(), fn).IsNothing())
-    return;
+    return Object::New(env->isolate());
   if (result
           ->Set(parsing_context,
                 env->source_map_url_string(),
                 fn->GetScriptOrigin().SourceMapUrl())
           .IsNothing())
-    return;
+    return Object::New(env->isolate());
 
   std::unique_ptr<ScriptCompiler::CachedData> new_cached_data;
   if (produce_cached_data) {
@@ -1295,14 +1358,91 @@ void ContextifyContext::CompileFunction(
   if (StoreCodeCacheResult(env,
                            result,
                            options,
-                           source,
+                           *source,
                            produce_cached_data,
                            std::move(new_cached_data))
           .IsNothing()) {
-    return;
+    return Object::New(env->isolate());
   }
 
-  args.GetReturnValue().Set(result);
+  return result;
+}
+
+// When compiling as CommonJS source code that contains ESM syntax, the
+// following error messages are returned:
+// - `import` statements: "Cannot use import statement outside a module"
+// - `export` statements: "Unexpected token 'export'"
+// - `import.meta` references: "Cannot use 'import.meta' outside a module"
+// Dynamic `import()` is permitted in CommonJS, so it does not error.
+// While top-level `await` is not permitted in CommonJS, it returns the same
+// error message as when `await` is used in a sync function, so we don't use it
+// as a disambiguation.
+constexpr std::array<std::string_view, 3> esm_syntax_error_messages = {
+    "Cannot use import statement outside a module",  // `import` statements
+    "Unexpected token 'export'",                     // `export` statements
+    "Cannot use 'import.meta' outside a module"};    // `import.meta` references
+
+void ContextifyContext::ContainsModuleSyntax(
+    const FunctionCallbackInfo<Value>& args) {
+  // Argument 1: source code
+  CHECK(args[0]->IsString());
+  Local<String> code = args[0].As<String>();
+
+  // Argument 2: filename
+  Local<String> filename = String::Empty(args.GetIsolate());
+  if (!args[1]->IsUndefined()) {
+    CHECK(args[1]->IsString());
+    filename = args[1].As<String>();
+  }
+
+  Environment* env = Environment::GetCurrent(args);
+  Isolate* isolate = env->isolate();
+  Local<Context> context = env->context();
+
+  // TODO(geoffreybooth): Centralize this rather than matching the logic in
+  // cjs/loader.js and translators.js
+  Local<String> script_id = String::Concat(
+      isolate, String::NewFromUtf8(isolate, "cjs:").ToLocalChecked(), filename);
+  Local<Symbol> id_symbol = Symbol::New(isolate, script_id);
+
+  Local<PrimitiveArray> host_defined_options =
+      GetHostDefinedOptions(isolate, id_symbol);
+  ScriptCompiler::Source source = GetCommonJSSourceInstance(
+      isolate, code, filename, 0, 0, host_defined_options, nullptr);
+  ScriptCompiler::CompileOptions options = GetCompileOptions(source);
+
+  std::vector<Local<String>> params = {
+      String::NewFromUtf8(isolate, "exports").ToLocalChecked(),
+      String::NewFromUtf8(isolate, "require").ToLocalChecked(),
+      String::NewFromUtf8(isolate, "module").ToLocalChecked(),
+      String::NewFromUtf8(isolate, "__filename").ToLocalChecked(),
+      String::NewFromUtf8(isolate, "__dirname").ToLocalChecked()};
+
+  TryCatchScope try_catch(env);
+
+  ContextifyContext::CompileFunctionAndCacheResult(env,
+                                                   context,
+                                                   &source,
+                                                   params,
+                                                   std::vector<Local<Object>>(),
+                                                   options,
+                                                   true,
+                                                   id_symbol,
+                                                   try_catch);
+
+  bool found_error_message_caused_by_module_syntax = 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;
+        break;
+      }
+    }
+  }
+  args.GetReturnValue().Set(found_error_message_caused_by_module_syntax);
 }
 
 static void StartSigintWatchdog(const FunctionCallbackInfo<Value>& args) {
diff --git a/src/node_contextify.h b/src/node_contextify.h
