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

src: fix undefined script name in error source #56502

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Jan 7, 2025

If an error is not caught, the file name and source code line where the error was thrown is printed. However, for unnamed eval scripts, a placeholder instead of undefined should be print:

undefined:3
  throw new Error('error in unnamed script');
  ^

Error: error in unnamed script
    at eval (eval at <anonymous> (/dev/nodejs/node/test/fixtures/errors/throw_in_eval_unnamed.js:5:1), <anonymous>:3:9)

This PR prints a placeholder <anonymous_script> for such errors:

<anonymous_script>:3
  throw new Error('error in anonymous script');

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jan 7, 2025
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

LGTM with a suggestion.

const char* filename_string = *filename;
std::string filename_string;
if (message->GetScriptResourceName()->IsUndefined()) {
filename_string = "<unnamed_script>";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
filename_string = "<unnamed_script>";
filename_string = "<anonymous>";

Which is what Chrome DevTools use, or I think <anonymous_script> is somewhat easier to understand. It would be even clearer if it's <eval_script> but I suspect this can be hit in other non-eval cases so can be tricky.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't use <anonymous> because it has been used to refer to anonymous functions in the error stacks just below the sources. I think it could be confused with anonymous scripts and functions.

<anonymous_script> looks good to me.

Copy link

codecov bot commented Jan 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.12%. Comparing base (52c6449) to head (996e2c3).
Report is 14 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #56502      +/-   ##
==========================================
- Coverage   89.12%   89.12%   -0.01%     
==========================================
  Files         662      662              
  Lines      191555   191560       +5     
  Branches    36856    36854       -2     
==========================================
- Hits       170730   170721       -9     
- Misses      13696    13701       +5     
- Partials     7129     7138       +9     
Files with missing lines Coverage Δ
src/node_errors.cc 64.67% <100.00%> (+1.17%) ⬆️

... and 28 files with indirect coverage changes

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM with @joyeecheung's suggestion

@legendecas legendecas force-pushed the errors/eval-undefined branch from 3f9f17c to 996e2c3 Compare January 7, 2025 16:39
@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 8, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 8, 2025
@nodejs-github-bot
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants