No stack trace with missing async keyword and dynamic imports

[sbrl]

• Version: v13.9.0
• Platform: Linux PC_NAME 5.3.0-40-generic Presentation about named exports in core modules#32~18.04.1-Ubuntu SMP Mon Feb 3 14:05:59 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
• Subsystem:

What steps will reproduce the bug?

Consider the following code:

a.mjs

(async () => {
    "use strict";
    
	let b = await import("./b.mjs");
	await b.default();
})();

b.mjs

import fs from 'fs';

function bug() {
	// The something doesn't have to exist
	console.log(await fs.promises.readFile("/proc/cpuinfo", "utf-8"));
}
export default async function () {
	await bug();
}

Explanation

There is obviously a bug in b.mjs above. The bug() function is missing the async keyword`. However, when I attempt to execute the above, I get the following error:

(node:30870) UnhandledPromiseRejectionWarning: SyntaxError: Unexpected reserved word
    at Loader.moduleStrategy (internal/modules/esm/translators.js:81:18)
    at async link (internal/modules/esm/module_job.js:37:21)
(node:30870) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:30870) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

This isn't particularly helpful. Now, consider the following snippet:

c.mjs:

import fs from 'fs';

function bug() {
	// The something doesn't have to exist
	console.log(await fs.promises.readFile("/proc/cpuinfo", "utf-8"));
}

(async () => {
    "use strict";
    
    await bug();
})();

There's bug in this one too, but executing it yields a very different and much more helpful error:

file:///tmp/c.mjs:5
	console.log(await fs.promises.readFile("/proc/cpuinfo", "utf-8"));
	            ^^^^^

SyntaxError: Unexpected reserved word
    at Loader.moduleStrategy (internal/modules/esm/translators.js:81:18)
    at async link (internal/modules/esm/module_job.js:37:21)

Much better. Node gives us a helping hand by telling us where the error occurred.

How often does it reproduce? Is there a required condition?

This bug in the error message only appears to occur when a module is dynamically imported with import("./path/to/file.mjs"). Specifically, the first error message is missing this bit:

file:///tmp/c.mjs:5
	console.log(await fs.promises.readFile("/proc/cpuinfo", "utf-8"));
	            ^^^^^

...obviously the filepath and line number would be different for b.mjs is this bit was added.

What is the expected behavior?

The first error message should look like the second.

When dynamically importing a module that is missing the async keyword on a method, the error message should tell me where the error occurred.

What do you see instead?

The bit there it tells me where the "unexpected reserved word" was found is missing. See above for examples of what's gone wrong.

Additional information

If possible, when it's the await keyword that was detected as unexpected, the error message should reflect this more closely. For example, it might be helpful to say `SyntaxError: Unexpected reserved word "await" (did you forget the "async" keyword?)" or something like that.

[MylesBorins]

/cc @nodejs/modules

[coreyfarrell]

See also #49441

[devsnek]

this is a very general issue with how our error stack decoration works, and not specific to the esm loader. We can fix this in two different ways:

• Patch ModuleWrap constructor to call stack decoration util
• Move stack decoration to our prepareStackTrace handler (make all errors decorated)

[sbrl]

Ah, I see @devsnek. Thanks for the explanation! I decided to report this issue because with a larger codebase it's actually a pretty significant time waster. For my PhD I have a dynamic import system for a number of subcommands that do different things in a Node.js application. It's becoming a serious challenge to figure out where I've made a mistake.....

I can grant private access to this codebase if this would help fixing the bug.

[NyanHelsing]

@sbrl Just want to note that adding a linter may help in this case by allowing you to locate errors and get that more useful stack trace without needing to actually execute the code.

[sbrl]

@ExProbitasFiducia Right. I'm actually already using a linter, but it doesn't detect issues with async / await for some reason. I'm using Atom.

[NyanHelsing]

Shouldn't really matter which editor you have, the linter should perform the same even if you execute it from the cli.

I'm a fan of eslint and they do have a rule for async/await - https://eslint.org/docs/rules/require-await

Im not sure if this is useful, or related for you but I thought it was some neat info; there is a class of bugs associated with writing an async function that never uses the await keyword. I had an issue once where the await was in a conditional and I remember that was a pretty tricky thing to track down.

[ljharb]

require-await is a terrible rule; it's perfectly valid to have an async function without using await. The issue is usually caused by forgetting to await a promise, which that rule won't actually help you with if you already have an await statement in your async function.

[NyanHelsing]

That's the way I understand async is supposed to work. As I mentioned I've found that there are circumstances in which an async function doesn't await a value causes problem. I hope you don't run into it but that rule prevents it from happening anyway...

[ljharb]

Again, the issue there is that you're not awaiting a promise, which is unrelated to the presence or absence of await statements in a function.