Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

repl: improve error output #22436

Closed
wants to merge 4 commits into from
Closed

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Aug 21, 2018

  1. Currently extra properties on an error will be ignored, if thrown.
    This information will from now on be visible.
  2. In case someone threw a non error object it would have resulted in
    [object Object]. Instead, the full object will now be visible.
  3. Some cases were not detected properly as error before and "Thrown: "
    was visible before. That is now fixed.
  4. Refactor ERR_SCRIPT_EXECUTION_INTERRUPTED stack handling.
  5. Error objects contained the domain properties due to the internal
    handling. Those are now removed as they do not provide a benefit for
    the user.

Refs: #20253

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

1) Currently extra properties on an error will be ignored, if thrown.
   This information will from now on be visible.
2) In case someone threw a non error object it would have resulted in
   `[object Object]`. Instead, the full object will now be visible.
3) Some cases were not detected properly as error before and "Thrown: "
   was visible before. That is now fixed.
The stack was removed later on instead of never being attached in
the first place.
@nodejs-github-bot nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Aug 21, 2018
@BridgeAR
Copy link
Member Author

@nodejs/repl PTAL

@devsnek
Copy link
Member

devsnek commented Aug 21, 2018

how does someone know if an error was thrown or logged, if they aren't prefixed with Thrown

@BridgeAR
Copy link
Member Author

@devsnek that's just how the repl worked so far. I see your point but I guess that's a discussion for a different PR?

lib/repl.js Outdated

if (internalUtil.isError(e)) {
if (e.stack) {
if (e instanceof SyntaxError) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error could be a syntax error from another realm (so not instanceof SyntaxError). Since we've already confirmed it is an error object above with isError(e) we could just check that the name is SyntaxError.

@BridgeAR
Copy link
Member Author

Comment addressed.

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

PTAL

Error.stackTraceLimit = 0;
const err = new ERR_SCRIPT_EXECUTION_INTERRUPTED();
Error.stackTraceLimit = tmp;
reject(err);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since .stack creation is deferred until the .stack is accessed does toggling the Error.stackTraceLimit have the effect wanted here (making an empty stack)? It looks like stackTraceLimit is reset before the lazy .stack is generated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stack size is set on error creation time. Otherwise the test would fail as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good to know!

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 25, 2018
@BridgeAR
Copy link
Member Author

BridgeAR commented Aug 26, 2018

BridgeAR added a commit to BridgeAR/node that referenced this pull request Aug 27, 2018
1) Currently extra properties on an error will be ignored, if thrown.
   This information will from now on be visible.
2) In case someone threw a non error object it would have resulted in
   `[object Object]`. Instead, the full object will now be visible.
3) Some cases were not detected properly as error before and "Thrown: "
   was visible before. That is now fixed.

PR-URL: nodejs#22436
Refs: nodejs#20253
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Aug 27, 2018
The stack was removed later on instead of never being attached in
the first place.

PR-URL: nodejs#22436
Refs: nodejs#20253
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR
Copy link
Member Author

Landed in ccb303e, 2eef077 🎉

@BridgeAR BridgeAR closed this Aug 27, 2018
@addaleax
Copy link
Member

@BridgeAR Can you backport to v10.x-staging? It applies cleanly but there’s a failing test.

@targos targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 28, 2018
@targos
Copy link
Member

targos commented Sep 2, 2018

Ping

targos pushed a commit that referenced this pull request Sep 23, 2018
1) Currently extra properties on an error will be ignored, if thrown.
   This information will from now on be visible.
2) In case someone threw a non error object it would have resulted in
   `[object Object]`. Instead, the full object will now be visible.
3) Some cases were not detected properly as error before and "Thrown: "
   was visible before. That is now fixed.

PR-URL: #22436
Refs: #20253
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Sep 23, 2018
The stack was removed later on instead of never being attached in
the first place.

PR-URL: #22436
Refs: #20253
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR deleted the better-repl-errors branch January 20, 2020 11:35
addaleax added a commit to addaleax/node that referenced this pull request Apr 12, 2021
The REPL implementation would strip away the first and last character
of a formatted error message if it ended with `]` (but with the
obviously missing check for a starting `]`), leading to things like
`Uncaught rror: foo[a` being printed for input like `Error: foo[a]`.

Refs: nodejs#22436
jasnell pushed a commit that referenced this pull request Apr 13, 2021
The REPL implementation would strip away the first and last character
of a formatted error message if it ended with `]` (but with the
obviously missing check for a starting `]`), leading to things like
`Uncaught rror: foo[a` being printed for input like `Error: foo[a]`.

Refs: #22436

PR-URL: #38209
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
BethGriggs pushed a commit that referenced this pull request Apr 15, 2021
The REPL implementation would strip away the first and last character
of a formatted error message if it ended with `]` (but with the
obviously missing check for a starting `]`), leading to things like
`Uncaught rror: foo[a` being printed for input like `Error: foo[a]`.

Refs: #22436

PR-URL: #38209
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Apr 29, 2021
The REPL implementation would strip away the first and last character
of a formatted error message if it ended with `]` (but with the
obviously missing check for a starting `]`), leading to things like
`Uncaught rror: foo[a` being printed for input like `Error: foo[a]`.

Refs: #22436

PR-URL: #38209
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
richardlau pushed a commit that referenced this pull request Dec 14, 2021
The REPL implementation would strip away the first and last character
of a formatted error message if it ended with `]` (but with the
obviously missing check for a starting `]`), leading to things like
`Uncaught rror: foo[a` being printed for input like `Error: foo[a]`.

Refs: #22436

PR-URL: #38209
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants