module: improve named cjs import errors

When trying to import named exports from a CommonJS module an error is
thrown. Unfortunately the V8 error only contains the single line that
causes the error, it is therefore impossible to construct an equivalent
code consisting of default import + object descructuring assignment.

This was the reason why the example code was removed for multi line
import statements in nodejs#35275

To generate a helpful error messages for any case we can parse the file
where the error happens using acorn and construct a valid example code
from the parsed ImportDeclaration. This will work for _any_ valid import
statement.

Since this code is only executed shortly before the node process crashes
anyways performance should not be a concern here.

Fixes: nodejs#35259
Refs: nodejs#35275

Author: ctavan

lib/internal/modules/esm/module_job.js

@@ -2,26 +2,82 @@
 
 const {
   ArrayPrototypeJoin,
+  Number,
   ObjectSetPrototypeOf,
   PromiseAll,
   SafeSet,
   SafePromise,
   StringPrototypeIncludes,
   StringPrototypeMatch,
-  StringPrototypeReplace,
   StringPrototypeSplit,
 } = primordials;
 
 const { ModuleWrap } = internalBinding('module_wrap');
 
 const { decorateErrorStack } = require('internal/util');
+const { fileURLToPath } = require('url');
 const assert = require('internal/assert');
 const resolvedPromise = SafePromise.resolve();
 
 function noop() {}
 
 let hasPausedEntry = false;
 
+function extractExample(file, lineNumber) {
+  const { readFileSync } = require('fs');
+  const { parse } = require('internal/deps/acorn/acorn/dist/acorn');
+  const { findNodeAt } = require('internal/deps/acorn/acorn-walk/dist/walk');
+
+  const code = readFileSync(file, { encoding: 'utf8' });
+  const parsed = parse(code, {
+    sourceType: 'module',
+    locations: true,
+  });
+  let start = 0;
+  let node;
+  do {
+    node = findNodeAt(parsed, start);
+    start = node.node.end + 1;
+
+    if (node.node.loc.end.line < lineNumber) {
+      continue;
+    }
+
+    if (node.node.type !== 'ImportDeclaration') {
+      continue;
+    }
+
+    const defaultSpecifier = node.node.specifiers.find(
+      (specifier) => specifier.type === 'ImportDefaultSpecifier'
+    );
+    const defaultImport = defaultSpecifier ?
+      defaultSpecifier.local.name :
+      'pkg';
+
+    const joinString = ', ';
+    let totalLength = 0;
+    const imports = node.node.specifiers
+      .filter((specifier) => specifier.type === 'ImportSpecifier')
+      .map((specifier) => {
+        const statement =
+          specifier.local.name === specifier.imported.name ?
+            `${specifier.imported.name}` :
+            `${specifier.imported.name}: ${specifier.local.name}`;
+        totalLength += statement.length + joinString.length;
+        return statement;
+      });
+
+    const boilerplate = `const {  } = ${defaultImport};`;
+    const destructuringAssignment = totalLength > 80 - boilerplate.length ?
+      `\n${imports.map((i) => `  ${i}`).join(',\n')}\n` :
+      ` ${imports.join(joinString)} `;
+
+    return `\n\nimport ${defaultImport} from '${node.node.source.value}';\n` +
+      `const {${destructuringAssignment}} = ${defaultImport};\n`;
+  } while (node === undefined || node.node.loc.start.line <= lineNumber);
+  return '';
+}
+
 /* A ModuleJob tracks the loading of a single Module, and the ModuleJobs of
  * its dependencies, over time. */
 class ModuleJob {
@@ -109,21 +165,17 @@ class ModuleJob {
             await this.loader.resolve(childSpecifier, parentFileUrl);
         const format = await this.loader.getFormat(childFileURL);
         if (format === 'commonjs') {
-          const importStatement = splitStack[1];
-          // TODO(@ctavan): The original error stack only provides the single
-          // line which causes the error. For multi-line import statements we
-          // cannot generate an equivalent object descructuring assignment by
-          // just parsing the error stack.
-          const oneLineNamedImports = StringPrototypeMatch(importStatement, /{.*}/);
-          const destructuringAssignment = oneLineNamedImports &&
-              StringPrototypeReplace(oneLineNamedImports, /\s+as\s+/g, ': ');
+          const [, fileUrl, lineNumber] =
+                StringPrototypeMatch(parentFileUrl, /^(.*):([0-9]+)$/);
+          const example = extractExample(
+            fileURLToPath(fileUrl),
+            Number(lineNumber),
+          );
           e.message = `Named export '${name}' not found. The requested module` +
             ` '${childSpecifier}' is a CommonJS module, which may not support` +
             ' all module.exports as named exports.\nCommonJS modules can ' +
             'always be imported via the default export, for example using:' +
-            `\n\nimport pkg from '${childSpecifier}';\n${
-              destructuringAssignment ?
-                `const ${destructuringAssignment} = pkg;\n` : ''}`;
+            example;
           const newStack = StringPrototypeSplit(e.stack, '\n');
           newStack[3] = `SyntaxError: ${e.message}`;
           e.stack = ArrayPrototypeJoin(newStack, '\n');

test/es-module/test-esm-cjs-named-error.mjs

@@ -3,21 +3,26 @@ import { rejects } from 'assert';
 
 const fixtureBase = '../fixtures/es-modules/package-cjs-named-error';
 
-const errTemplate = (specifier, name, namedImports) =>
+const errTemplate = (specifier, name, namedImports, defaultName) =>
   `Named export '${name}' not found. The requested module` +
   ` '${specifier}' is a CommonJS module, which may not support ` +
   'all module.exports as named exports.\nCommonJS modules can ' +
   'always be imported via the default export, for example using:' +
-  `\n\nimport pkg from '${specifier}';\n` + (namedImports ?
-    `const ${namedImports} = pkg;\n` : '');
+  `\n\nimport ${defaultName || 'pkg'} from '${specifier}';\n` + (namedImports ?
+    `const ${namedImports} = ${defaultName || 'pkg'};\n` : '');
 
-const expectedWithoutExample = errTemplate('./fail.cjs', 'comeOn');
+const expectedMultiLine = errTemplate('./fail.cjs', 'comeOn',
+                                      '{ comeOn, everybody }');
 
 const expectedRelative = errTemplate('./fail.cjs', 'comeOn', '{ comeOn }');
 
 const expectedRenamed = errTemplate('./fail.cjs', 'comeOn',
                                     '{ comeOn: comeOnRenamed }');
 
+const expectedDefaultRenamed =
+  errTemplate('./fail.cjs', 'everybody', '{ comeOn: comeOnRenamed, everybody }',
+              'abc');
+
 const expectedPackageHack =
     errTemplate('./json-hack/fail.js', 'comeOn', '{ comeOn }');
 
@@ -44,11 +49,18 @@ rejects(async () => {
   message: expectedRenamed
 }, 'should correctly format named imports with renames');
 
+rejects(async () => {
+  await import(`${fixtureBase}/default-and-renamed-import.mjs`);
+}, {
+  name: 'SyntaxError',
+  message: expectedDefaultRenamed
+}, 'should correctly format hybrid default and named imports with renames');
+
 rejects(async () => {
   await import(`${fixtureBase}/multi-line.mjs`);
 }, {
   name: 'SyntaxError',
-  message: expectedWithoutExample,
+  message: expectedMultiLine,
 }, 'should correctly format named imports across multiple lines');
 
 rejects(async () => {

test/fixtures/es-modules/package-cjs-named-error/default-and-renamed-import.mjs

@@ -0,0 +1,4 @@
+import abc, {
+  comeOn as comeOnRenamed,
+  everybody,
+} from './fail.cjs';