take ownership of prepareStackTrace for upstream v8 changes

[devsnek]

These changes exist for a few reasons:

• We have a double-decoration problem where people need to use process.binding('util') to stop node from decorating errors if they want to do their own thing.
• V8 is making upstream changes to Error.prepareStackTrace which scope it per-context and can't land those until we fix how we handle Error.prepareStackTrace in repl
• Error stack decoration was not very consistent, as it pinged around between two methods in C++ and JS that had to be called in the correct order

Closes #21958

/cc @BridgeAR @joyeecheung @bmeck @jdalton @mmarchini @ljharb

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

[nodejs-github-bot]

@devsnek build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1393/pipeline

[devsnek]

I'm also unsure of how this should be semver classified.

[refack]

1st commit looks good with nits, and style conformance.

[refack]

4th commit looks good with nits.

[refack]

+1 I like the idea of owning _setupPrepareStackTrace. Two comments:
Can this change be made semver-minor (modulo Error.prepareStackTrace behaviour), at least to the extent the we generate the same stack trace as the current?
IMHO first commit can be its own PR, and land sooner.

[devsnek]

@nodejs/assert can someone take a look at how this effects getErrMessage?

[refack]

LGTM

[jdalton]

I've compiled the PR but haven't had a chance to check it against my use case. I should be able to in the next couple days though.

Update:

It looks like I can work with this change. It doesn't re-decorate decorated stacks so for me this should be OK.

[devsnek]

@nodejs/assert another ping... can someone take a look at what changes need to be made to assertion error decoration

[refack]

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

[Trott]

@BridgeAR Any chance you have some time to look at this proposed change?

[BridgeAR]

Nit: We do not really use multi line template strings in /lib. It would be nice to replace this using string concat.

[devsnek]

why

[refack]

why

That one of my favorite questions ;)
Sometimes the answer is "tradition", but in this case I think it's readability, as it breaks the indentation.
There also might be some weirdness in Windows where EOL is \r\n if you set git to autocrlf=true, and that changes the strings. _{I'm so so sorry and a little ashamed for my prefered platform, but then again macOS has .DS_Store files so...}

[devsnek]

take your vote i guess

    return `${resourceName}:${lineNumber}
${sourceLine}
${' '.repeat(startPosition)}${'^'.repeat(endPosition - startPosition)}

${errorString}
    at ${trace.join('\n    at ')}`;

vs

    return `${resourceName}:${lineNumber}\n${sourceLine}\n` +
      ' '.repeat(startPosition) + '^'.repeat(endPosition - startPosition) +
      `\n\n${errorString}\n    at ${trace.join('\n    at ')}`;

[joyeecheung]

It's probably going to look a bit better if you break at the end of every \n..

[devsnek]

@joyeecheung like this?

    return `${resourceName}:${lineNumber}\n${sourceLine}\n` +
      ' '.repeat(startPosition) + '^'.repeat(endPosition - startPosition) +
      '\n' +
      '\n' +
      errorString +
      '\n' +
      `    at ${trace.join('\n    at ')}`;

[refack]

Maybe (just to reduce the '\n' kruft):

const numberOfCarrots = endPosition - startPosition;
const traceLines = [
  `${resourceName}:${lineNumber}`,
  `${sourceLine}`,
  ' '.repeat(startPosition) + '^'.repeat(numberOfCarrots),
  '',
  errorString,
  ...(trace.map((t) => `    at ${t}`)),
];
return traceLines.join('\n');

(carrot is funnier than carat)

Also could concat the trace more elegantly:

  errorString,
];
traceLines = traceLines.concat(trace.map((t) => `    at ${t}`))
return traceLines.join('\n');

[devsnek]

all of these are equally unreadable and the last one is creating an array just for the purpose of joining it...

i understand the indentation hazard but that's why we have tests right?

[refack]

all of these are equally unreadable and the last one is creating an array just for the purpose of joining it...

No strong argument from me... Trying to make the best of a complicated situation.
_{as for the array, it's probably going to be the last code run by the process, so..}

[BridgeAR]

@devsnek it has been something that we tried to stick to in the whole code base. Not everyone is aware how multi line template strings actually work as people forget that leading spaces actually count. This was an issue in multiple PRs so far. So it's not only about the readability but about correctness as well (not in this specific PR but as a general issue).

Sadly there's no eslint rule to only forbid multi line template strings. We should probably write one to enforce this.

[BridgeAR]

I have to compile this on my own and play around with it. If I understand this correct it will from now on always attach the source to the stack (if the user did not override the function). That is great! It would be nice if the error decoration could be expanded to cover multiple line statements fully and not only print the first line of the statement. Besides that it seems like the position indicator is not always correct (I checked some error output), so that should be looked at again.

It does not directly seem to impact getErrMessage() if the user does not override the prepareStackTrace() function. I did not yet test this but I guess if the user overrides it, the getErrMessage() function would fail? That would be pretty bad and we should try to find away around it.

We definitely have to think about changing the simple assert error message again before this lands since it would otherwise print redundant information.

@devsnek please correct me if I am wrong with anything I said here.

[devsnek]

@BridgeAR i think removing it entirely would make sense with this change.

Before	After

[nodejs-github-bot]

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

[devsnek]

@joyeecheung does the current init code here make sense?

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[devsnek]

Landed in b046bd1