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

module: support eval with ts syntax detection #56285

Merged

Conversation

marco-ippolito
Copy link
Member

@marco-ippolito marco-ippolito commented Dec 17, 2024

Refs: nodejs/typescript#17
Previous attempt: #56273

What is the problem?

Before this PR when --experimental-strip-types was enabled, --eval would always parse the input as typescript.
If unflagged, the typescript parser would throw different errors on invalid syntax so unflagging would become a breaking change.

With this PR when running --eval and --experimental-strip-types is enabled, if parsing the code fails we try again with typescript parser.
If it fails again we throw the original error, adding the typescript parser message.

In this way the error is the original error and it's not a breaking change.

Example:


With `--experimental-strip-types`:


marcoippolito@marcos-MBP node % ./node --experimental-strip-types -e "enum Foo{}"    
file:///Users/marcoippolito/Documents/projects/forks/node/[eval1]:1
enum Foo{}
^^^^

  x TypeScript enum is not supported in strip-only mode
   ,----
 1 | enum Foo{}
   : ^^^^^^^^^^
   `----


SyntaxError: Unexpected reserved word
    at compileSourceTextModule (node:internal/modules/esm/utils:338:16)
    at ModuleLoader.eval (node:internal/modules/esm/loader:218:18)
    at node:internal/process/execution:322:29
    at asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:98:11)
    at Object.runEntryPointWithESMLoader (node:internal/modules/run_main:120:19)
    at evalTypeScriptModuleEntryPoint (node:internal/process/execution:318:47)
    at node:internal/main/eval_string:32:3

Node.js v24.0.0-pre




Without `--experimental-strip-types`:


marcoippolito@marcos-MBP node % ./node  -e "enum Foo{}"
file:///Users/marcoippolito/Documents/projects/forks/node/[eval1]:1
enum Foo{}
^^^^

SyntaxError: Unexpected reserved word
    at compileSourceTextModule (node:internal/modules/esm/utils:338:16)
    at ModuleLoader.eval (node:internal/modules/esm/loader:218:18)
    at node:internal/process/execution:73:24
    at asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:98:11)
    at Object.runEntryPointWithESMLoader (node:internal/modules/run_main:120:19)
    at evalModuleEntryPoint (node:internal/process/execution:72:47)
    at evalTypeScriptModuleEntryPoint (node:internal/process/execution:309:12)
    at node:internal/main/eval_string:32:3

Node.js v24.0.0-pre

This PR also add two new --input-type:

  • module-typescript
  • commonjs-typescript

So that if the syntax is known we can reduce the overhead of multiple parsing.
If the -typescript input is passed we can throw ERR_INVALID_TYPESCRIPT_SYNTAX safely

the temporary side effect is to remove the workers ability to eval typescript code, which is currently untested (and it never was tested)

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 17, 2024

Review requested:

  • @nodejs/loaders
  • @nodejs/typescript

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Dec 17, 2024
@marco-ippolito marco-ippolito force-pushed the test/wrap-in-syntax-error branch 6 times, most recently from 7229405 to f0f0e3c Compare December 17, 2024 14:44
@marco-ippolito marco-ippolito added module Issues and PRs related to the module subsystem. strip-types Issues or PRs related to strip-types support labels Dec 17, 2024
@marco-ippolito marco-ippolito force-pushed the test/wrap-in-syntax-error branch from f0f0e3c to 728a1fd Compare December 17, 2024 14:51
@marco-ippolito marco-ippolito marked this pull request as ready for review December 17, 2024 14:52
@marco-ippolito marco-ippolito force-pushed the test/wrap-in-syntax-error branch 4 times, most recently from 3485014 to 1dd1cb5 Compare December 17, 2024 15:16
doc/api/cli.md Outdated Show resolved Hide resolved
doc/api/cli.md Outdated Show resolved Hide resolved
doc/api/cli.md Outdated Show resolved Hide resolved
@marco-ippolito marco-ippolito force-pushed the test/wrap-in-syntax-error branch from 2f87a9c to aeb77e9 Compare December 17, 2024 15:29
Copy link
Member

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

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

Looking good! I like this approach, it feels like a good way to unflag strip-types and provide convenience to the vast majority of users, while providing an option to be explicit for edge cases.

@joyeecheung should review the additions to lib/internal/process/execution.js, I feel like I’ve worked in that file in the past and she’s had notes for me.

doc/api/cli.md Show resolved Hide resolved
doc/api/cli.md Outdated Show resolved Hide resolved
doc/api/cli.md Outdated Show resolved Hide resolved
doc/api/cli.md Outdated Show resolved Hide resolved
lib/internal/process/execution.js Show resolved Hide resolved
lib/internal/process/execution.js Show resolved Hide resolved
Copy link

codecov bot commented Dec 17, 2024

Codecov Report

Attention: Patch coverage is 96.92737% with 11 lines in your changes missing coverage. Please review.

Project coverage is 88.54%. Comparing base (a9e65f6) to head (ec519b5).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/process/execution.js 97.18% 8 Missing ⚠️
src/node_options.cc 40.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #56285      +/-   ##
==========================================
+ Coverage   88.53%   88.54%   +0.01%     
==========================================
  Files         657      657              
  Lines      190395   190700     +305     
  Branches    36555    36590      +35     
==========================================
+ Hits       168567   168864     +297     
- Misses      15002    15014      +12     
+ Partials     6826     6822       -4     
Files with missing lines Coverage Δ
lib/internal/main/eval_string.js 84.61% <100.00%> (+4.95%) ⬆️
lib/internal/modules/cjs/loader.js 98.11% <ø> (-0.01%) ⬇️
lib/internal/modules/esm/loader.js 97.87% <100.00%> (+0.06%) ⬆️
lib/internal/modules/esm/translators.js 91.45% <ø> (-0.05%) ⬇️
lib/internal/modules/typescript.js 100.00% <100.00%> (ø)
src/node_options.cc 87.92% <40.00%> (-0.10%) ⬇️
lib/internal/process/execution.js 97.81% <97.18%> (-0.98%) ⬇️

... and 26 files with indirect coverage changes

@marco-ippolito marco-ippolito force-pushed the test/wrap-in-syntax-error branch from 78e6970 to 38d78e4 Compare December 19, 2024 19:58
@marco-ippolito
Copy link
Member Author

marco-ippolito commented Dec 19, 2024

I also tried locally to unflag --experimental-strip-types.

The only --eval related failures are:

/test/message/eval_messages.js
/test/message/stdin_messages.js

Those are snapshots tests but I have no idea on how regenerate them, from what I can tell the difference is in the error stacktrace.
@nodejs/repl may have more infos

@GeoffreyBooth
Copy link
Member

Those are snapshots tests but I have no idea on how regenerate them, from what I can tell the difference is in the error stacktrace.

See #47707

@marco-ippolito marco-ippolito force-pushed the test/wrap-in-syntax-error branch from 38d78e4 to ec519b5 Compare December 24, 2024 08:26
@marco-ippolito marco-ippolito added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Dec 24, 2024
@marco-ippolito marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 24, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 24, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@marco-ippolito marco-ippolito added the review wanted PRs that need reviews. label Dec 24, 2024
Copy link
Member

@pmarchini pmarchini left a comment

Choose a reason for hiding this comment

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

LGTM

@marco-ippolito marco-ippolito added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 24, 2024
@marco-ippolito marco-ippolito added dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. labels Dec 24, 2024
doc/api/cli.md Show resolved Hide resolved
} catch (tsError) {
// If its not an error, or it's not an invalid typescript syntax error, rethrow it.
if (!isError(tsError) || tsError?.code !== 'ERR_INVALID_TYPESCRIPT_SYNTAX') {
throw tsError;
Copy link
Member

Choose a reason for hiding this comment

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

What I found with syntax detection is that if you rethrow an exception in JavaScript, the first line (the source of the error) changes to Node internals rather than being the actual line of code the error originated from. The only way to prevent this was to do everything in C++. There might not be a test for this specifically for eval input; there should be, and there was for file input. I went down the same road of implementing the whole thing in JavaScript and stumbling onto this and then needing to redo it in C++ with the help of @targos and @joyeecheung.

Copy link
Member Author

@marco-ippolito marco-ippolito Dec 24, 2024

Choose a reason for hiding this comment

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

I mean it's eval does it really matter that we adding a line in the stacktrace? We don't have this problem for files.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no difference:

marcoippolito@marcos-MacBook-Pro node % node -e "throw new Error('yo')"                                                
[eval]:1
throw new Error('yo')
^

Error: yo
    at [eval]:1:7
    at runScriptInThisContext (node:internal/vm:209:10)
    at node:internal/process/execution:118:14
    at [eval]-wrapper:6:24
    at runScript (node:internal/process/execution:101:62)
    at evalScript (node:internal/process/execution:136:3)
    at node:internal/main/eval_string:55:3

Node.js v22.10.0
marcoippolito@marcos-MacBook-Pro node % ./node --experimental-strip-types -e "throw new Error('yo')"
[eval]:1
throw new Error('yo')
^

Error: yo
    at [eval]:1:7
    at runScriptInThisContext (node:internal/vm:209:10)
    at node:internal/process/execution:478:12
    at [eval]-wrapper:6:24
    at runScriptInContext (node:internal/process/execution:476:60)
    at evalTypeScript (node:internal/process/execution:316:3)
    at node:internal/main/eval_string:71:3

Node.js v24.0.0-pre

@marco-ippolito marco-ippolito added the baking-for-lts PRs that need to wait before landing in a LTS release. label Dec 24, 2024
@marco-ippolito marco-ippolito added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 24, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 24, 2024
@nodejs-github-bot nodejs-github-bot merged commit da3f388 into nodejs:main Dec 24, 2024
85 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in da3f388

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. baking-for-lts PRs that need to wait before landing in a LTS release. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. lib / src Issues and PRs related to general changes in the lib or src directory. module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. review wanted PRs that need reviews. strip-types Issues or PRs related to strip-types support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants