src, util: move error decoration to native

[devsnek]

Fixes: #19783
Fixes: #19763

This moves the burden of decorating error stacks to TryCatch, which is always used in this case, so we know the stacks will always be decorated. I'm going to benchmark this soonish, and a bunch of message tests have to be updated

/cc @nodejs/diagnostics @nodejs/performance

Checklist

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

[mcollina]

There is a spurious package-lock file added to this PR.

[targos]

Can be simplified to:

err_obj->SetPrivate(env->context(),
                    env->error_decorated_private_symbol(),
                    v8::True(env->isolate())).FromJust();

[jdalton]

Does this decorate an error if the .stack has already been generated?

Is there a way to opt-out of an error being decorated so that this won't clobber existing decorating user-land may do to stacks?

[devsnek]

@jdalton I'm like 97% sure this pr has so far only changed observable behaviour for modules, not scripts. that being said if anyone feels like throwing test cases at it that would be cool

[BridgeAR]

This needs a rebase.

[devsnek]

anyone have any opinions on something like --no-decorate-traces

[jdalton]

anyone have any opinions on something like --no-decorate-traces

A global flag is nice, but a programmatic way to opt-out per error would be better.

For example:

foo.mjs

const e = new Error
e.stack = "DECORATED\n" + e.stack
throw e

Will produce:

(node:91873) ExperimentalWarning: The ESM module loader is experimental.
file:///Users/jdalton/projects/foo.mjs:4
throw e
^

DECORATED
Error
    at file:///Users/jdalton/projects/foo.mjs:1:11
    at ModuleJob.run (internal/modules/esm/module_job.js:104:14)

Which is decorating an already decorated error.

If there was an API to specify that an error is already decorated that would be great. The expected output of opting-out of the built-in decorator is

DECORATED
Error
    at file:///Users/jdalton/projects/foo.mjs:1:11
    at ModuleJob.run (internal/modules/esm/module_job.js:104:14)

Something like util.skipNodeErrorDecorator(e), util.setErrorDecorator(e, decorator), or a symbol like util.inspect.custom to allow customization are possible approaches.

[devsnek]

@jdalton funny you ask for a programmatic way to opt out, as i've just found out over the last few hours the only proper way for node to do this is via Error.prepareStackTrace. i'm hesitant because of that to continue with this pr, but if people feel like the ecosystem will survive node setting Error.prepareStackTrace i'd like to continue. it would have the added benefit of moving node's error decoration to js which should speed things up. its also worth noting that at the current time node doesn't really support opting out of decoration. currently opting out looks like this: https://github.com/devsnek/node-noasi/blob/master/index.js#L45

[jdalton]

funny you ask for a programmatic way to opt out, as i've just found out over the last few hours the only proper way for node to do this is via Error.prepareStackTrace.

I'm aware of Error.prepareStackTrace() but not on how that would be a more proper way for Node to opt-out. Can you go into that more.

currently opting out looks like this: node-noasi/index.js#L45

Correct. A more legit way would be great!

[devsnek]

@jdalton assuming that the v8 embedder has set Error.prepareStackTrace, a user can just change the override while their stack trace is being generated. this is already a pretty common practice in userland.

[jdalton]

assuming that the v8 embeddeder has set Error.prepareStackTrace, a user can just change the override while their stack trace is being generated.

Ah ya, though that's more of a global solution, so not great for per error opt-outs (package use).

[devsnek]

i've also mentioned this in the error stacks proposal for tc39: tc39/proposal-error-stacks#20 (comment) and i opened a feature request for something from v8's c++ api: https://bugs.chromium.org/p/v8/issues/detail?id=7637

[devsnek]

i think i'll just close this, those two linked issues are plenty for tracking

[devsnek]

@ChALkeR would it be possible to run a gzemnid for specifically code that does Object.defineProperty(Error, 'prepareStackTrace', ...)? (I don't care about Error.prepareStackTrace = ...)