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

Add new ESLint rule: prefer-primordials #35448

Closed
wants to merge 1 commit into from
Closed

Add new ESLint rule: prefer-primordials #35448

wants to merge 1 commit into from

Conversation

Leko
Copy link
Contributor

@Leko Leko commented Oct 1, 2020

Fixed #35449

  • Added a new rule for checking access built-in objects
  • Fixed lint errors of the rule

Rule option

A rule option has a required key and optional keys

key name type required description example values
name string ✔︎ Name of object Number, Symbol, Array
ignore string[] - List of properties to exclude from the lint targets stackTraceLimit
into string - Wrapping up in the specified namespace when reporting errors Number (ex. If this rule find code that is using parseInt globally, it will report const { NumberParseInt } = primordials; ...)

Ignored properties of Error

I marked some of properties of Error as ignored.

Error.stackTraceLimit, Error.captureStackTrace

A lot of tests are failed when I replace these props with primordials.

Error.prepareStackTrace

View failed tests output
=== release test-repl-pretty-custom-stack ===                                 
Path: parallel/test-repl-pretty-custom-stack
/Users/leko/ghq/github.com/Leko/node/test/parallel/test-repl-pretty-custom-stack.js:44
  throw e;
  ^

[AssertionError [ERR_ASSERTION] [ERR_ASSERTION]: Expected values to be strictly equal:

  • actual - expected

  • 'Uncaught Error: Whoops!\n' +

  • ' at REPL1::\n' +

  • ' at d (REPL1::)\n' +

  • ' at c (REPL1::)\n' +

  • ' at b (REPL1::)\n' +

  • ' at a (REPL1::)\n'

  • 'Uncaught Error: Whoops!--->\n' +
  • 'REPL1::--->\n' +
  • 'd (REPL1::)--->\n' +
  • 'c (REPL1::)--->\n' +
  • 'b (REPL1::)--->\n' +
  • 'a (REPL1::)\n'--->
    node:internal/main/runmain_module:17:47--->
    Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:72:12)--->
    Function.Module._load (node:internal/modules/cjs/loader:778:14)--->
    Module.load (node:internal/modules/cjs/loader:937:32)--->
    Object.Module._extensions..js (node:internal/modules/cjs/loader:1101:10)--->
    Module._compile (node:internal/modules/cjs/loader:1072:30)--->
    Object. (/Users/leko/ghq/github.com/Leko/node/test/parallel/test-repl-pretty-custom-stack.js:74:7)--->
    Array.forEach ()--->
    run (/Users/leko/ghq/github.com/Leko/node/test/parallel/test-repl-pretty-custom-stack.js:27:10)] {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: 'Uncaught Error: Whoops!\n' +
    ' at REPL1:
    :\n' +
    ' at d (REPL1:
    :)\n' +
    ' at c (REPL1:
    :)\n' +
    ' at b (REPL1:
    :)\n' +
    ' at a (REPL1:
    :)\n',
    expected: 'Uncaught Error: Whoops!--->\n' +
    'REPL1:
    :--->\n' +
    'd (REPL1:
    :)--->\n' +
    'c (REPL1:
    :)--->\n' +
    'b (REPL1:
    :)--->\n' +
    'a (REPL1:
    :_)\n',
    operator: 'strictEqual'
    }

Error.isPrototypeOf

View failed tests output
=== release test-assert-async ===                                             
Path: parallel/test-assert-async
node:internal/process/promises:218
          triggerUncaughtException(err, true /* fromPromise */);
          ^

AssertionError [ERR_ASSERTION]: TypeError is not instance of AssertionError
at Object.handler2 (/Users/leko/ghq/github.com/Leko/node/test/parallel/test-assert-async.js:202:5)
at Object. (/Users/leko/ghq/github.com/Leko/node/test/common/index.js:361:15)
at expectedException (node:assert:656:26)
at expectsError (node:assert:781:3)
at Function.rejects (node:assert:834:3)
at async Promise.all (index 20) {
generatedMessage: false,
code: 'ERR_ASSERTION',
actual: false,
expected: true,
operator: '=='
}

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

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Oct 1, 2020
@targos
Copy link
Member

targos commented Oct 4, 2020

I'm +1 on this. It should be doable after #35499

@Leko
Copy link
Contributor Author

Leko commented Oct 6, 2020

@targos Thank you for the information! I'll check your PR and mark this PR as ready for review when #35499 is merged.

@Leko Leko changed the title Add new custom ESLint rule: prefer-primordials Add new ESLint rule: prefer-primordials Oct 10, 2020
@Leko Leko marked this pull request as ready for review October 11, 2020 04:38
@Leko Leko requested a review from targos October 11, 2020 04:38
@Leko Leko added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 11, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 11, 2020
@nodejs-github-bot

This comment has been minimized.

@Leko Leko requested a review from targos October 13, 2020 18:27
@codecov-io
Copy link

codecov-io commented Oct 18, 2020

Codecov Report

Merging #35448 into master will decrease coverage by 0.47%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #35448      +/-   ##
==========================================
- Coverage   96.87%   96.40%   -0.48%     
==========================================
  Files         212      220       +8     
  Lines       69641    73735    +4094     
==========================================
+ Hits        67466    71085    +3619     
- Misses       2175     2650     +475     
Impacted Files Coverage Δ
lib/internal/crypto/scrypt.js 28.33% <0.00%> (-71.67%) ⬇️
lib/internal/crypto/diffiehellman.js 92.47% <0.00%> (-6.50%) ⬇️
lib/internal/crypto/keys.js 94.99% <0.00%> (-4.22%) ⬇️
lib/internal/crypto/util.js 96.79% <0.00%> (-3.21%) ⬇️
lib/internal/crypto/sig.js 97.54% <0.00%> (-2.46%) ⬇️
lib/internal/source_map/prepare_stack_trace.js 94.95% <0.00%> (-1.69%) ⬇️
lib/internal/crypto/keygen.js 99.20% <0.00%> (-0.49%) ⬇️
lib/internal/modules/cjs/loader.js 94.39% <0.00%> (-0.28%) ⬇️
lib/_http_server.js 98.34% <0.00%> (-0.19%) ⬇️
lib/fs.js 94.13% <0.00%> (-0.13%) ⬇️
... and 57 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8b950a...a2f6630. Read the comment docs.

@Leko Leko added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 18, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 18, 2020
@nodejs-github-bot

This comment has been minimized.

@Leko Leko added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 19, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 19, 2020
@nodejs-github-bot

This comment has been minimized.

Copy link
Contributor

@DerekNonGeneric DerekNonGeneric left a comment

Choose a reason for hiding this comment

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

Just wanted to say thanks for doing this. Definitely +1, but don't have time for a proper review at the moment. Very happy to see this!

lib/internal/http.js Outdated Show resolved Hide resolved
@Leko Leko added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 19, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 19, 2020
@nodejs-github-bot

This comment has been minimized.

Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

LGTM based on the added unit tests and changes in lib. I am not familiar with writing ESLint rules.

@Leko
Copy link
Contributor Author

Leko commented Oct 27, 2020

@nodejs/collaborators Could you help me with reviewing this PR?

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

LGTM, let's land this! 🎉

@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 6, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 6, 2020
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

I added a new custom ESLint rule to fix these problems.

We have a lot of replaceable codes with primordials.
Accessing built-in objects is restricted by existing rule
(no-restricted-globals), but accessing property in the built-in objects
is not restricted right now. We manually review codes that can be
replaced by primordials, but there's a lot of code that actually needs
to be fixed. We have often made pull requests to replace the primordials
with.

Restrict accessing global built-in objects such as `Promise`.
Restrict calling static methods such as `Array.from` or `Symbol.for`.
Don't restrict prototype methods to prevent false-positive.
@Leko
Copy link
Contributor Author

Leko commented Nov 7, 2020

I found a new lint error while landing. I fixed the error.
Object.freeze in the #35915

@Leko Leko added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 7, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 7, 2020
@nodejs-github-bot
Copy link
Collaborator

@Leko
Copy link
Contributor Author

Leko commented Nov 7, 2020

Landed in cef1444

@Leko Leko closed this Nov 7, 2020
@Leko Leko deleted the improve-primordials-linter branch November 7, 2020 10:29
Leko added a commit that referenced this pull request Nov 7, 2020
I added a new custom ESLint rule to fix these problems.

We have a lot of replaceable codes with primordials.
Accessing built-in objects is restricted by existing rule
(no-restricted-globals), but accessing property in the built-in objects
is not restricted right now. We manually review codes that can be
replaced by primordials, but there's a lot of code that actually needs
to be fixed. We have often made pull requests to replace the primordials
with.

Restrict accessing global built-in objects such as `Promise`.
Restrict calling static methods such as `Array.from` or `Symbol.for`.
Don't restrict prototype methods to prevent false-positive.

PR-URL: #35448
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
Int16Array,
Int32Array,
Int8Array,
JSON,
Copy link
Contributor

Choose a reason for hiding this comment

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

JSON, Math and Reflect don’t actually exist on primordials, only their methods.


I’m fixing that in #36016.

ExE-Boss added a commit to ExE-Boss/node that referenced this pull request Nov 8, 2020
nodejs-github-bot pushed a commit that referenced this pull request Nov 9, 2020
Refs: #35448
Refs: #36003
Refs: https://tc39.es/ecma262/#sec-%typedarray%-intrinsic-object

Co-authored-by: Antoine du Hamel <[email protected]>

PR-URL: #36016
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
danielleadams pushed a commit that referenced this pull request Nov 9, 2020
I added a new custom ESLint rule to fix these problems.

We have a lot of replaceable codes with primordials.
Accessing built-in objects is restricted by existing rule
(no-restricted-globals), but accessing property in the built-in objects
is not restricted right now. We manually review codes that can be
replaced by primordials, but there's a lot of code that actually needs
to be fixed. We have often made pull requests to replace the primordials
with.

Restrict accessing global built-in objects such as `Promise`.
Restrict calling static methods such as `Array.from` or `Symbol.for`.
Don't restrict prototype methods to prevent false-positive.

PR-URL: #35448
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
danielleadams pushed a commit that referenced this pull request Nov 9, 2020
Refs: #35448
Refs: #36003
Refs: https://tc39.es/ecma262/#sec-%typedarray%-intrinsic-object

Co-authored-by: Antoine du Hamel <[email protected]>

PR-URL: #36016
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
@danielleadams danielleadams mentioned this pull request Nov 9, 2020
targos added a commit that referenced this pull request May 16, 2021
I added a new custom ESLint rule to fix these problems.

We have a lot of replaceable codes with primordials.
Accessing built-in objects is restricted by existing rule
(no-restricted-globals), but accessing property in the built-in objects
is not restricted right now. We manually review codes that can be
replaced by primordials, but there's a lot of code that actually needs
to be fixed. We have often made pull requests to replace the primordials
with.

Restrict accessing global built-in objects such as `Promise`.
Restrict calling static methods such as `Array.from` or `Symbol.for`.
Don't restrict prototype methods to prevent false-positive.

PR-URL: #35448
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
targos added a commit that referenced this pull request May 16, 2021
I added a new custom ESLint rule to fix these problems.

We have a lot of replaceable codes with primordials.
Accessing built-in objects is restricted by existing rule
(no-restricted-globals), but accessing property in the built-in objects
is not restricted right now. We manually review codes that can be
replaced by primordials, but there's a lot of code that actually needs
to be fixed. We have often made pull requests to replace the primordials
with.

Restrict accessing global built-in objects such as `Promise`.
Restrict calling static methods such as `Array.from` or `Symbol.for`.
Don't restrict prototype methods to prevent false-positive.

PR-URL: #35448
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
targos pushed a commit that referenced this pull request May 16, 2021
Refs: #35448
Refs: #36003
Refs: https://tc39.es/ecma262/#sec-%typedarray%-intrinsic-object

Co-authored-by: Antoine du Hamel <[email protected]>

PR-URL: #36016
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
targos pushed a commit that referenced this pull request Jun 11, 2021
I added a new custom ESLint rule to fix these problems.

We have a lot of replaceable codes with primordials.
Accessing built-in objects is restricted by existing rule
(no-restricted-globals), but accessing property in the built-in objects
is not restricted right now. We manually review codes that can be
replaced by primordials, but there's a lot of code that actually needs
to be fixed. We have often made pull requests to replace the primordials
with.

Restrict accessing global built-in objects such as `Promise`.
Restrict calling static methods such as `Array.from` or `Symbol.for`.
Don't restrict prototype methods to prevent false-positive.

PR-URL: #35448
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
targos pushed a commit that referenced this pull request Jun 11, 2021
Refs: #35448
Refs: #36003
Refs: https://tc39.es/ecma262/#sec-%typedarray%-intrinsic-object

Co-authored-by: Antoine du Hamel <[email protected]>

PR-URL: #36016
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

primordials access check can be improved