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

feat: new no-new-native-nonconstructor rule #16368

Merged
merged 15 commits into from Nov 6, 2022

Conversation

sosukesuzuki
Copy link
Contributor

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[x] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Fixes #16322

Adds new rule no-new-nonconstructor.

Is there anything you'd like reviewers to focus on?

question: Should we deprecate no-new-symbol in this PR? or in other PR?

@eslint-github-bot eslint-github-bot bot added triage An ESLint team member will look at this issue soon feature This change adds a new feature to ESLint labels Sep 30, 2022
@netlify
Copy link

netlify bot commented Sep 30, 2022

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit c9f11ed
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/63674a108a0aff000869538a
😎 Deploy Preview https://deploy-preview-16368--docs-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@@ -0,0 +1,64 @@
/**
* @fileoverview Rule to disallow use of the new operator with global no constructor functions
* @author Sosuke Suzuki
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a question, most of the code for this rule is the same as no-new-symbol. Who should we mention as a an author?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that would be nice.


docs: {
description: "Disallow `new` operators with global no constructor functions",
recommended: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be recommended?

Copy link
Member

Choose a reason for hiding this comment

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

No. We can't set to recommended until we do a major release. We can switch it during the next major release.

@sosukesuzuki sosukesuzuki marked this pull request as ready for review September 30, 2022 18:49
@ljharb
Copy link
Sponsor Contributor

ljharb commented Sep 30, 2022

The naming of this rule doesn't just imply inclusion of primitive constructors that throw, but also all HTML element constructors, as well as %TypedArray% (altho that one's hard to lint for) - but also any function that's not newable, which includes arrows, async, generators, async generators, concise object methods, and class methods.

Is that broad scope intended?

@sosukesuzuki
Copy link
Contributor Author

Is that broad scope intended?

No. Only built-in functions such as Symbol and BigInt are intended. We need a better name. Like no-new-builtin-nonconstructor?

@ljharb
Copy link
Sponsor Contributor

ljharb commented Oct 1, 2022

In the future, when there's other constructors you can't new, would they be included by implication? Is it just primitive constructors? What if there's a future Decimal, or Record or Tuple, that have the same semantics?

If not, it seems like a no-new-bigint rule would be better. If so, then it's important to define exactly what the criteria is, and the name should be informed by that.

@mdjermanovic
Copy link
Member

The criteria is: ES global variable that is a function (noncallable globals are covered by the no-obj-calls rule) and starts with an uppercase letter ( = looks like a constructor) but can't be used with new. At the moment, that would be Symbol and BigInt. In the future, new ES globals that satisfy the criteria would be added. It wouldn't be restricted to globals that relate to primitive types.

@ljharb
Copy link
Sponsor Contributor

ljharb commented Oct 3, 2022

and "can't be used with new" means that you get an exception when you invoke it with new, i gather?

what about HTMLElement and all the other HTML element global constructors that can't be used with new?

@mdjermanovic
Copy link
Member

and "can't be used with new" means that you get an exception when you invoke it with new, i gather?

Yes, exactly.

what about HTMLElement and all the other HTML element global constructors that can't be used with new?

This rule checks only globals from the ECMAScript specification, currently Symbol and BigInt, similar to the no-obj-calls rule which checks only Math, JSON, Reflect and Atomics.

@sosukesuzuki
Copy link
Contributor Author

I don't think no-new-noconstructor is a bad name. It is similar to no-obj-calls.

I think it is sufficient to specify the criteria in the documentation.

@nzakas
Copy link
Member

nzakas commented Oct 25, 2022

We could go with no-new-native-nonconstructor instead?

@sosukesuzuki sosukesuzuki changed the title feat: new no-new-nonconstructor rule feat: new no-new-native-nonconstructor rule Oct 26, 2022
@mdjermanovic mdjermanovic added rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Oct 26, 2022
docs/src/rules/no-new-native-nonconstructor.md Outdated Show resolved Hide resolved
docs/src/rules/no-new-native-nonconstructor.md Outdated Show resolved Hide resolved
docs/src/rules/no-new-native-nonconstructor.md Outdated Show resolved Hide resolved
docs/src/rules/no-new-native-nonconstructor.md Outdated Show resolved Hide resolved
docs/src/rules/no-new-native-nonconstructor.md Outdated Show resolved Hide resolved
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@@ -698,5 +698,19 @@
"logo": "https://eslint.org/apple-touch-icon.png",
"title": "Interesting bugs caught by no-constant-binary-expression - ESLint - Pluggable JavaScript Linter",
"description": "A pluggable and configurable linter tool for identifying and reporting on patterns in JavaScript. Maintain your code quality with ease."
},
"https://tc39.es/ecma262/#sec-symbol-object": {
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
"https://tc39.es/ecma262/#sec-symbol-object": {
"https://tc39.es/ecma262/#sec-symbol-constructor": {

This should fix the docs site build.

docs/src/_data/further_reading_links.json Outdated Show resolved Hide resolved
docs/src/rules/no-new-native-nonconstructor.md Outdated Show resolved Hide resolved
docs/src/rules/no-new-native-nonconstructor.md Outdated Show resolved Hide resolved
docs/src/rules/no-new-native-nonconstructor.md Outdated Show resolved Hide resolved
lib/rules/no-new-native-nonconstructor.js Outdated Show resolved Hide resolved
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

I'll leave this open for a few days in case someone else wants to review it before merging.

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

The code looks good. I just left some suggestions to improve the documentation.

docs/src/rules/no-new-native-nonconstructor.md Outdated Show resolved Hide resolved
docs/src/rules/no-new-native-nonconstructor.md Outdated Show resolved Hide resolved
docs/src/rules/no-new-native-nonconstructor.md Outdated Show resolved Hide resolved
docs/src/rules/no-new-native-nonconstructor.md Outdated Show resolved Hide resolved
Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>
nzakas
nzakas approved these changes Nov 1, 2022
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@nzakas
Copy link
Member

nzakas commented Nov 1, 2022

@mdjermanovic because this is meant to replace no-new-symbol, so we also want to deprecate no-new-symbol or otherwise indicate the relationship between these two rules somehow?

@mdjermanovic
Copy link
Member

We should deprecate no-new-symbol, but since it's in eslint:recommended then maybe better to deprecate it in the next major version?

Copy link
Contributor

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@snitin315
Copy link
Contributor

We should deprecate no-new-symbol, but since it's in eslint:recommended then maybe better to deprecate it in the next major version?

I feel we can deprecate in minor versions, and in the major version remove no-new-symbol in favor of no-new-native-nonconstructor. Deprecations are meant to warn users about a change beforehand.

btmills
btmills approved these changes Nov 6, 2022
Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

I just resolved a merge conflict in the further reading links. Another entry had been added since this branch was last updated to main, so I just needed to add an extra }, line. I'll merge this once CI passes.

@snitin315 in most cases we could deprecate a rule in a minor release, but we're unable to add no-new-native-nonconstructor to eslint:recommended until the next major release, so resolving the deprecation warning would require users to manually override eslint:recommended even if they previously had no customizations. I will, however, call out the replacement in the release notes.

@btmills btmills merged commit f14587c into eslint:main Nov 6, 2022
21 checks passed
@sosukesuzuki sosukesuzuki deleted the add-no-new-noconstructor branch November 6, 2022 06:50
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Nov 10, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.26.0` -> `8.27.0`](https://renovatebot.com/diffs/npm/eslint/8.26.0/8.27.0) |

---

### Release Notes

<details>
<summary>eslint/eslint</summary>

### [`v8.27.0`](https://github.com/eslint/eslint/releases/tag/v8.27.0)

[Compare Source](eslint/eslint@v8.26.0...v8.27.0)

#### Features

-   [`f14587c`](eslint/eslint@f14587c) feat: new `no-new-native-nonconstructor` rule ([#&#8203;16368](eslint/eslint#16368)) (Sosuke Suzuki)
-   [`978799b`](eslint/eslint@978799b) feat: add new rule `no-empty-static-block` ([#&#8203;16325](eslint/eslint#16325)) (Sosuke Suzuki)
-   [`69216ee`](eslint/eslint@69216ee) feat: no-empty suggest to add comment in empty BlockStatement ([#&#8203;16470](eslint/eslint#16470)) (Nitin Kumar)
-   [`319f0a5`](eslint/eslint@319f0a5) feat: use `context.languageOptions.ecmaVersion` in core rules ([#&#8203;16458](eslint/eslint#16458)) (Milos Djermanovic)

#### Bug Fixes

-   [`c3ce521`](eslint/eslint@c3ce521) fix: Ensure unmatched glob patterns throw an error ([#&#8203;16462](eslint/eslint#16462)) (Nicholas C. Zakas)
-   [`886a038`](eslint/eslint@886a038) fix: handle files with unspecified path in `getRulesMetaForResults` ([#&#8203;16437](eslint/eslint#16437)) (Francesco Trotta)

#### Documentation

-   [`ce93b42`](eslint/eslint@ce93b42) docs: Stylelint property-no-unknown ([#&#8203;16497](eslint/eslint#16497)) (Nick Schonning)
-   [`d2cecb4`](eslint/eslint@d2cecb4) docs: Stylelint declaration-block-no-shorthand-property-overrides ([#&#8203;16498](eslint/eslint#16498)) (Nick Schonning)
-   [`0a92805`](eslint/eslint@0a92805) docs: stylelint color-hex-case ([#&#8203;16496](eslint/eslint#16496)) (Nick Schonning)
-   [`74a5af4`](eslint/eslint@74a5af4) docs: fix stylelint error ([#&#8203;16491](eslint/eslint#16491)) (Milos Djermanovic)
-   [`324db1a`](eslint/eslint@324db1a) docs: explicit stylelint color related rules ([#&#8203;16465](eslint/eslint#16465)) (Nick Schonning)
-   [`94dc4f1`](eslint/eslint@94dc4f1) docs: use Stylelint for HTML files ([#&#8203;16468](eslint/eslint#16468)) (Nick Schonning)
-   [`cc6128d`](eslint/eslint@cc6128d) docs: enable stylelint declaration-block-no-duplicate-properties ([#&#8203;16466](eslint/eslint#16466)) (Nick Schonning)
-   [`d03a8bf`](eslint/eslint@d03a8bf) docs: Add heading to justification explanation ([#&#8203;16430](eslint/eslint#16430)) (Maritaria)
-   [`8a15968`](eslint/eslint@8a15968) docs: add Stylelint configuration and cleanup ([#&#8203;16379](eslint/eslint#16379)) (Nick Schonning)
-   [`9b0a469`](eslint/eslint@9b0a469) docs: note commit messages don't support scope ([#&#8203;16435](eslint/eslint#16435)) (Andy Edwards)
-   [`1581405`](eslint/eslint@1581405) docs: improve context.getScope() docs ([#&#8203;16417](eslint/eslint#16417)) (Ben Perlmutter)
-   [`b797149`](eslint/eslint@b797149) docs: update formatters template ([#&#8203;16454](eslint/eslint#16454)) (Milos Djermanovic)
-   [`5ac4de9`](eslint/eslint@5ac4de9) docs: fix link to formatters on the Core Concepts page ([#&#8203;16455](eslint/eslint#16455)) (Vladislav)
-   [`33313ef`](eslint/eslint@33313ef) docs: core-concepts: fix link to semi rule ([#&#8203;16453](eslint/eslint#16453)) (coderaiser)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

 **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xOS4wIiwidXBkYXRlZEluVmVyIjoiMzQuMjEuMiJ9-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1628
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion contributor pool feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Rule: no-new-bigint
6 participants