error message regression in node 13 vs 12

[ljharb]

With this file, tmp, with +x permissions:

#!/usr/bin/env node

'syntax error

node tmp in node 12 / nvm run 12 tmp begets:

$PWD/tmp:3
'syntax error
^^^^^^^^^^^^^

SyntaxError: Invalid or unexpected token
    at Module._compile (internal/modules/cjs/loader.js:895:18)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:995:10)
    at Module.load (internal/modules/cjs/loader.js:815:32)
    at Function.Module._load (internal/modules/cjs/loader.js:727:14)
    at Function.Module.runMain (internal/modules/cjs/loader.js:1047:10)
    at internal/main/run_main_module.js:17:11

node tmp in node 13.6 / nvm run 13 tmp begets:

$PWD/tmp:1
#!/usr/bin/env node
^

SyntaxError: Invalid or unexpected token
    at wrapSafe (internal/modules/cjs/loader.js:1060:16)
    at Module._compile (internal/modules/cjs/loader.js:1108:27)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1164:10)
    at Module.load (internal/modules/cjs/loader.js:993:32)
    at Function.Module._load (internal/modules/cjs/loader.js:892:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
    at internal/main/run_main_module.js:17:47

The error message in node 13 was highly confusing and I had no idea where the syntax error was in the actual file (which was obviously much larger) until I thought to test it in node 12.

[ljharb]

cc @nodejs/modules-active-members in case it has to do with loader changes, and in case this can be fixed before it's backported to node 12.

[jkrems]

Does this happen without the shebang as well or is this specific to it?

Previously reported issue: nodejs/modules#464

[devsnek]

This bug can reproduced in pure V8. I'd suggest reporting it upstream.

[GeoffreyBooth]

This bug can reproduced in pure V8. I'd suggest reporting it upstream.

Out of curiosity, how do you test that?

[ljharb]

I'll file it upstream (testing details would help), but upgrading v8 in node 12 seems like a problem since it will break the usefulness of error messages in an LTS node, so upstream or not, it seems like something we'd have to address :-/

[jkrems]

> vm.createScript('#!/usr/bin/env node\n\n\'syntax error').runInThisContext()
evalmachine.<anonymous>:1
#!/usr/bin/env node
^

Uncaught SyntaxError: Invalid or unexpected token
> vm.createScript('#!/usr/bin/env node\n\n42-').runInThisContext()
evalmachine.<anonymous>:3
42-


Uncaught SyntaxError: Unexpected end of input

Looks like it's specific to certain kinds of SyntaxError only?

[devsnek]

@jkrems The issues are actually unrelated. Note where the arrow is pointing @SMotaal's issue.

@GeoffreyBooth I have a local checkout and build of V8, which includes a command-line debugging tool called d8. https://v8.dev/docs/d8

[ljharb]

Filed https://bugs.chromium.org/p/v8/issues/detail?id=10110, it'd be great if folks helped flesh it out.

[ljharb]

@devsnek given that the node 13 error comes from at wrapSafe (internal/modules/cjs/loader.js:1060:16), doesn't that imply it's an issue in node's CJS loader and not in v8?

[jkrems]

@ljharb Since the same (bad) behavior can be seen without wrapSafe, I think there's definitely still a V8 bug. Once the V8 behavior is fixed, we could see if there's a secondary bug in node as well.

[ljharb]

The v8 behavior seems to be fixed (according to the v8 issue)! I'm not sure which version of v8 it will land in, nor which version of node. It'd be great if someone in @nodejs/v8 could update this issue :-)

[jkrems]

It landed after the branch cut for 8.1 which means so far it only landed in 8.2 releases. Not sure how likely it is to be backported to 7.8 or 7.9 unless we actively pursue it.

[mmarchini]

Requested backport to 8.1 on the upstream bug. I think it's unlikely they'll backport to 7.9 (and even more unliekly to 7.8), so we should look into floating patches to fix it on v12 and v13.

[mmarchini]

Just a heads up: with the upgrade of V8 on master, this bug resurfaced. I'm trying to get it back ported upstream so we don't have to float another patch. If we get close to the release date for v14.x and it's not backported upstream yet, I'll float the patch again. I'll reopen it just to keep track, but it shouldn't affect any of the existing release lines.

[targos]

I assume this is resolved now.