errors: print original exception context

[bcoe]

When --enable-source-maps is set, the error context displayed
above the stack trace now shows original source rather than
transpiled.

When --enable-source-maps is set, annotate the top of the stack trace with the original source, rather than the ugly transpiled source:

before:

  if ((obj === null || obj === void 0 ? void 0 : (_obj$a = obj.a) === null || _obj$a === void 0 ? void 0 : _obj$a.b) === 22) throw Error('an exception');

after:

/Users/bencoe/oss/node-1/test/fixtures/source-map/babel-throw-original.js:18
  if (obj?.a?.b === 22) throw Error('an exception');

Open questions

The logic in node_errors.cc is a little magical to me, specifically I'm trying to figure out how it's placing the ^ under the throw, rather than under the Error (which is where the stack trace points).

@addaleax I know you've also worked in this file, any thoughts (maybe this is an issue we don't care about).

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[devsnek]

I'm trying to figure out how it's placing the ^ under the throw, rather than under the Error

As a general rule, v8::Message is generated from the location of throw, and the error's stack is generated from the location of the error. The v8::Message might sometimes point to the error location instead if it failed to figure out the throw location.

[bcoe]

@devsnek do you think we should be clever, and even just grep for the throw and move the pointer accordingly? might work for 80% of cases.

[devsnek]

@bcoe i think its okay to use the error position. alternatively, you could store the message position info on the error in c++ (like how we store decorations) so its available in the js prepareStackTrace callback.

[bnoordhuis]

As a general observation, having an environment-wide flag that toggles the behavior doesn't seem like a good idea. Wouldn't you want to distinguish on whether the script contains a source map URL or not?

You can query the v8::Message object to find out if it has one:

const bool has_source_map_url =
    !message->GetScriptOrigin().SourceMapUrl().IsEmpty();

[bcoe]

@bnoordhuis const bool has_source_map_url = !message->GetScriptOrigin().SourceMapUrl().IsEmpty(); works great, I didn't have a clue this was a possibility.

It does, however, present the issue that if you haven't run a file with --enable-source-maps, you will not get the context added above your stack trace (as the source-map will still be found by v8).

Should we check both values (whether source maps are enabled, and whether the script has a source map), and only annotate the error in prepareStackTrace for files with source maps?

[bcoe]

@bnoordhuis using your suggestion as a starting point, we now only apply the new getErrorSource logic, if:

1. source maps are enabled, and.
2. the file has a source map.

[addaleax]

@addaleax I know you've also worked in this file, any thoughts (maybe this is an issue we don't care about).

Not really any thoughts beyond that this seems useful and I’m happy to see it :)

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/31484/

[BridgeAR]

LGTM. Just left a suggestion to improve unicode support and maybe another test file was meant to be added?

[bcoe]

@BridgeAR I opted for using fixture based testing, my thinking being that it's actually a pretty elegant way to assert against the whole stack output.

tldr; those are tabs in source_map_reference_error_tabs.out:

> const text = fs.readFileSync('test/message/source_map_reference_error_tabs.out', 'utf8')
undefined
> text
'*tabs.coffee:26\n' +
  '\talert "I knew it!"\n' +
  '\t^\n' +

[bcoe]

@BridgeAR can you recommend an error message we could throw that would exercise your unicode improvements, I think this would be a great addition to the test suite.

Well I'm extending on the test suite, any thoughts on a transpiler I haven't used yet 😆

We've used babel, TypeScript, uglify, CoffeeScript, Istanbul.

[BridgeAR]

@bcoe I mostly use the following characters to verify unicode support:

• あ - a full width character with a length of one.
• 🐕 - a full width character with a length of two.
• 𐐷 - a half width character with the length of two.
• '\u0301', '0x200D', '\u200E' - zero width characters.

full width = ' ' // Two spaces
half width = ' '
zero width = ''

Using these in the error message should be sufficient.

[bcoe]

@BridgeAR take a look 👍

I noticed that it's actually more accurate with source maps than without now (the message displayed for ("あ 🐕 🐕", throw Error("an error")); points to the E with source maps, " " without source maps).

[bnoordhuis]

LGTM % comments. Nice work, @bcoe.

[BridgeAR]

Awesome work! Still LGTM, just left two nits.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/31545/

[bcoe]

Landed in 458677f

[codebytere]

@bcoe would you mind opening a manual backport for this into v14.x-staging? it's a conflict in a few files. Feel free to update the label if you don't think it should go back!