feat!: add two more cases to `no-implicit-coercion` #17832

gurgunday · 2023-12-09T16:00:54Z

Prerequisites checklist

I have read the contributing guidelines.

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

Changes an existing rule
Add autofix to a rule

What changes did you make? (Give an overview)

Hey everyone, I realized the no-implicit-coercion had some missing cases:

Subtracting 0 from an expression (exp - 0) — I can't image why anyone would do that other than to convert to a number, in fact I know that some use it within Fastify repos
Using the subtraction operator twice (-(-exp)) — this is something that would be pretty hard to come by I guess 🤣, but should still be invalid

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

My first ESLint PR, so not sure if everything is a-ok

linux-foundation-easycla · 2023-12-09T16:00:58Z

The committers listed above are authorized under a signed CLA.

✅ login: gurgunday / name: Gürgün Dayıoğlu (0541585, 86fb26d, 8da4742, bb1ac23, b0001d9, fe7e048, 5531723, 6814836, 9c74329, 7476cd9, 4458a1c, e0c8555, b344795, 2a9b163, ef0ed3e, 4f59c87, be63839, e00f85c)

eslint-github-bot · 2023-12-09T16:00:59Z

Hi @gurgunday!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.

The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
There should be a space following the initial tag and colon, for example 'feat: Message'.
The first letter of the tag should be in lowercase

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

netlify · 2023-12-09T16:01:54Z

✅ Deploy Preview for docs-eslint canceled.

Name	Link
🔨 Latest commit	`e00f85c`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/658b699c58e6f10008c83302

kecrily

Can you put these two cases in the documentation as well? The documentation is at
docs/src/rules/no-implicit-coercion.md

gurgunday · 2023-12-09T17:11:49Z

Done

kecrily

LGTM, a good catch. Thank you

fasttime · 2023-12-12T21:17:07Z

I believe that there should be a way to explicitly disable reporting on exp - 0 and -(-exp) using the allow option, as it can be done with the other expressions.

The full list of options is here:

eslint/lib/rules/no-implicit-coercion.js

Line 15 in e52b98b

const ALLOWABLE_OPERATORS = ["~", "!!", "+", "*"];

gurgunday · 2023-12-12T23:42:58Z

I believe that there should be a way to explicitly disable reporting on exp - 0 and -(-exp) using the allow option, as it can be done with the other expressions.

Nice catch, added

mdjermanovic · 2023-12-13T13:06:18Z

I think the main question is whether the current behavior of not reporting these patterns is a bug.

If it's a bug, then we should fix it, and we could release this change as a semver-minor bug fix that results in ESLint reporting more errors.

If it isn't a bug, but rather an enhancement, then we can't change the default behavior in a minor version. These checks could only be added in a minor version if they're behind an option (or options) that is disabled by default, or in a major version. Also, per our rules policies, stylistic rules are frozen except for bug fixes.

Since this is a stylistic rule and these particular patterns are not listed in the documentation, I don't think we can consider this a bug fix. Curious what others think about this.

gurgunday · 2023-12-13T13:30:03Z

To me it's clearly a bug or at least a big inconsistency

exp - 0's raison d'etre is number conversion, just like exp * 1, which is currently reported

and -(-exp) is no different than !(!exp) or !!exp, which again is currently reported — in fact both operators - and ! do the exact same thing in terms of conversion to a certain type and negation

Edit: Also, I thought stylistic meant modifications that don’t alter the AST, at least that’s how Prettier defines it

kecrily · 2023-12-13T14:57:35Z

I'm more inclined to think this is a error. Because the implicit type conversion here is no different from what is listed in the current document. It doesn't add new functionality, it just fixes missing cases.

nzakas · 2023-12-13T16:28:50Z

@gurgunday thanks for the submission. Because you didn't fill out an issue template (as mentioned in the PR template) describing whether you think this is a bug or a feature request, it makes it difficult to evaluate as either. In the future, please be sure to include a template, or file an issue first, to help us better evaluate your submission.

As mentioned, no-implicit-coercion is a stylistic rule, meaning that it reflects a coding preference rather than a potential problem, which we have frozen since 2020. As such, we only make changes that are verified bugs.

Given that the PR already is created and has tests, I'm willing to treat this as a bug.

nzakas · 2023-12-13T16:30:40Z

lib/rules/no-implicit-coercion.js

@@ -12,7 +12,7 @@ const astUtils = require("./utils/ast-utils");
 //------------------------------------------------------------------------------

 const INDEX_OF_PATTERN = /^(?:i|lastI)ndexOf$/u;
-const ALLOWABLE_OPERATORS = ["~", "!!", "+", "*"];
+const ALLOWABLE_OPERATORS = ["~", "!!", "+", "--", "-", "*"];


I think this means -(-expr) correct? If so, I think this is misleading since this looks like the decrement operator (--) which does change the value. I think we need to rethink how this is represented.

I see, it does look like that, yeah

I could use - for both but they are different operators (unary negation and subtraction)

Maybe "-(-)"?

We could use "- -" with a space in between (as in - -1) to separate the - tokens. It's unfortunate that those exceptions to the rule are expressed in terms of operators and not operations, so there is no way to differentiate among +foo and "" + foo and foo += "" when defining exceptions. But since there is already "!!" as a combination of two operators, "- -" should make sense, too, I guess.

I agree with what you said

lib/rules/no-implicit-coercion.js

fasttime · 2023-12-16T10:11:53Z

I am now looking into the behavior of the rule with the other numeric expressions +foo and foo * 1 and there are some exceptions:

/* eslint no-implicit-coercion: "error" */

+foo;                // error
+123;                // ok
+Number(foo);        // ok
+parseInt(foo);      // ok
+parseFloat(foo);    // ok

foo * 1;             // error
123 * 1;             // ok
Number(foo) * 1;     // ok
parseInt(foo) * 1;   // ok
parseFloat(foo) * 1; // ok

Playground link

I can think that + in front of a number literal is useful if you want to stress that it's a positive value. Also, it wouldn't be okay to suggest replacing +123 with Number(123). Same thing with the other exceptions: Number(foo) * 1 should not be replaced with Number(Number(foo)).

What the rule is doing here is calling isNumeric to check if the node is one of the exceptions above:

eslint/lib/rules/no-implicit-coercion.js

Lines 97 to 106 in e52b98b

    
           function isNumeric(node) { 
        
               return ( 
        
                   node.type === "Literal" && typeof node.value === "number" || 
        
                   node.type === "CallExpression" && ( 
        
                       node.callee.name === "Number" || 
        
                       node.callee.name === "parseInt" || 
        
                       node.callee.name === "parseFloat" 
        
                   ) 
        
               ); 
        
           }

Shall we do the same thing with the new cases introduced in this PR? Then, for example, we would no longer flag expressions like - -123, parseInt(foo) - 0, etc.

mdjermanovic · 2023-12-19T09:25:52Z

I still think that enabling these checks by default is a breaking change, so marking this as feat!: sounds good to me. We're about to start with v9 prereleases, so this is going to be merged for v9 anyway.

mdjermanovic · 2023-12-19T10:09:24Z

A note to other reviewers: I believe this should be retagged as fix: before merging as per #17832 (comment).

Per our semver policies, a bug fix in a rule that results in ESLint reporting more linting errors is a semver-minor change, so we're using the feat tag for this kind of changes.

nzakas · 2023-12-21T23:33:06Z

Based on the discussion, marking this as accepted and breaking.

docs/src/rules/no-implicit-coercion.md

mdjermanovic · 2023-12-23T11:17:49Z

Autofixing -(-exp) to Number(exp) could change the runtime behavior of code if exp evaluates to a BigInt value.

const exp = 1n;

-(-exp); // 1n

Number(exp); // 1

This is an edge case, but maybe because of this -(-exp) shouldn't be autofixable?

gurgunday · 2023-12-23T13:34:13Z

I agree, but isn’t this the case with all Number autofixes?

I now believe we should remove it though

mdjermanovic · 2023-12-23T16:27:11Z

I agree, but isn’t this the case with all Number autofixes?

I believe we considered it acceptable for +exp and exp * 1 because they throw an error if exp evaluates to a BigInt value, though a change from throwing to not throwing an error is also a change in the runtime behavior so we could reconsider that as well.

I now believe we should remove it though

I think it's fine to introduce the -(-exp) check if it isn't autofixable as there's will be an option to disable it. We should probably also document when not to use it.

Curious what others think about this.

fasttime · 2023-12-23T17:25:54Z

I believe we considered it acceptable for +exp and exp * 1 because they throw an error if exp evaluates to a BigInt value, though a change from throwing to not throwing an error is also a change in the runtime behavior so we could reconsider that as well.

My first thought was that the BigInt case had never been considered because the rule itself predates the introduction of BigInts to JavaScript in ES2020.

eslint/docs/src/_data/rule_versions.json

Line 133 in 299bfae

"no-implicit-coercion": "1.0.0-rc-2",

I agree that the current autofix behavior for numbers should be rediscussed. Maybe in another issue?

mdjermanovic · 2023-12-23T17:42:58Z

I checked the BigInt issues and PRs now, and it was mentioned in #11983 (comment) but there was no further discussion.

I agree that we could discuss the current behavior in a separate issue.

As for the new check -(-exp), seems better to not make it autofixable?

mdjermanovic · 2023-12-26T13:34:15Z

lib/rules/no-implicit-coercion.js

+                // -(-foo)
+                operatorAllowed = options.allow.includes("- -");
+                if (!operatorAllowed && options.number && node.operator === "-" && node.argument.type === "UnaryExpression" && node.argument.operator === "-" && !isNumeric(node.argument.argument)) {
+                    report(node);


The message for this error now becomes "use `undefined` instead".

I think it's fine to keep the recommendation in the message, we can just make the error non-fixable (the third argument passed to report() should be false).

Oh, apologies, hadn't properly tested my changes

Fixing it

Not sure about this: should I revert b344795 and 2a9b163 too?

Seemed like those were autofix tests so I removed the one for - -

Not sure about this: should I revert b344795 and 2a9b163 too?

Seemed like those were autofix tests so I removed the one for - -

Yes, we should test whether the rule reports an error for -(-foo).

This reverts commit e0c8555.

This reverts commit 2a9b163.

mdjermanovic

LGTM, thanks!

* main: docs: Migrate to v9.0.0 (eslint#17905) docs: Switch to flat config by default (eslint#17840) docs: Update Create a Plugin for flat config (eslint#17826) feat!: add two more cases to `no-implicit-coercion` (eslint#17832) docs: Switch shareable config docs to use flat config (eslint#17827) chore: remove creating an unused instance of Linter in tests (eslint#17902) feat!: Switch Linter to flat config by default (eslint#17851)

fasttime · 2024-01-11T15:46:46Z

I've created a new issue to discuss about the autofix for BigInt values and other edge cases: #17983.

add two more cases

0541585

gurgunday requested a review from a team as a code owner December 9, 2023 16:00

gurgunday changed the title ~~no-implicit-coercion: add two more cases~~ feat: no-implicit-coercion: add two more cases Dec 9, 2023

eslint-github-bot bot added the feature This change adds a new feature to ESLint label Dec 9, 2023

gurgunday changed the title ~~feat: no-implicit-coercion: add two more cases~~ feat: add two more cases to no-implicit-coercion Dec 9, 2023

fix comment

86fb26d

kecrily reviewed Dec 9, 2023

View reviewed changes

docs

8da4742

kecrily approved these changes Dec 9, 2023

View reviewed changes

mdjermanovic added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules labels Dec 11, 2023

add allow

bb1ac23

nzakas reviewed Dec 13, 2023

View reviewed changes

gurgunday added 3 commits December 13, 2023 17:47

change operator

b0001d9

change docs

fe7e048

use - -

5531723

fasttime reviewed Dec 14, 2023

View reviewed changes

lib/rules/no-implicit-coercion.js Outdated Show resolved Hide resolved

lib/rules/no-implicit-coercion.js Outdated Show resolved Hide resolved

add tests

6814836

gurgunday requested a review from fasttime December 15, 2023 16:18

nzakas added accepted There is consensus among the team that this change meets the criteria for inclusion breaking This change is backwards-incompatible and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Dec 21, 2023

mdjermanovic changed the title ~~feat: add two more cases to no-implicit-coercion~~ feat!: add two more cases to no-implicit-coercion Dec 21, 2023

mdjermanovic reviewed Dec 22, 2023

View reviewed changes

docs/src/rules/no-implicit-coercion.md Show resolved Hide resolved

add examples

4458a1c

mdjermanovic added the contributor pool label Dec 23, 2023

gurgunday added 3 commits December 24, 2023 23:20

Refactor implicit coercion check in no-implicit-coercion rule

e0c8555

fix test

b344795

fix test

2a9b163

mdjermanovic reviewed Dec 26, 2023

View reviewed changes

gurgunday added 4 commits December 26, 2023 16:38

Revert "Refactor implicit coercion check in no-implicit-coercion rule"

ef0ed3e

This reverts commit e0c8555.

make non-fixable

4f59c87

Revert "fix test"

be63839

This reverts commit 2a9b163.

add back test

e00f85c

mdjermanovic approved these changes Dec 27, 2023

View reviewed changes

mdjermanovic merged commit 0b21e1f into eslint:main Dec 27, 2023
17 checks passed

fasttime mentioned this pull request Jan 11, 2024

Bug: edge cases for no-implicit-coercion autofix #17983

Closed

1 task

gurgunday deleted the no-coercion branch January 11, 2024 16:37

rwlxxvii mentioned this pull request May 17, 2024

[Snyk] Upgrade eslint from 9.0.0 to 9.1.1 rwlxxvii/containers#114

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: add two more cases to `no-implicit-coercion` #17832

feat!: add two more cases to `no-implicit-coercion` #17832

gurgunday commented Dec 9, 2023

linux-foundation-easycla bot commented Dec 9, 2023 •

edited

eslint-github-bot bot commented Dec 9, 2023

netlify bot commented Dec 9, 2023 •

edited

kecrily left a comment

gurgunday commented Dec 9, 2023

kecrily left a comment

fasttime commented Dec 12, 2023

gurgunday commented Dec 12, 2023

mdjermanovic commented Dec 13, 2023

gurgunday commented Dec 13, 2023 •

edited

kecrily commented Dec 13, 2023

nzakas commented Dec 13, 2023

nzakas Dec 13, 2023

gurgunday Dec 13, 2023

fasttime Dec 13, 2023 •

edited

gurgunday Dec 13, 2023

fasttime commented Dec 16, 2023

mdjermanovic commented Dec 19, 2023

mdjermanovic commented Dec 19, 2023

nzakas commented Dec 21, 2023

mdjermanovic commented Dec 23, 2023

gurgunday commented Dec 23, 2023

mdjermanovic commented Dec 23, 2023 •

edited

fasttime commented Dec 23, 2023

mdjermanovic commented Dec 23, 2023

mdjermanovic Dec 26, 2023

gurgunday Dec 26, 2023

gurgunday Dec 26, 2023 •

edited

mdjermanovic Dec 26, 2023

mdjermanovic left a comment

fasttime commented Jan 11, 2024

feat!: add two more cases to no-implicit-coercion #17832

feat!: add two more cases to no-implicit-coercion #17832

Conversation

gurgunday commented Dec 9, 2023

Prerequisites checklist

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

What changes did you make? (Give an overview)

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

linux-foundation-easycla bot commented Dec 9, 2023 • edited

eslint-github-bot bot commented Dec 9, 2023

netlify bot commented Dec 9, 2023 • edited

✅ Deploy Preview for docs-eslint canceled.

kecrily left a comment

Choose a reason for hiding this comment

gurgunday commented Dec 9, 2023

kecrily left a comment

Choose a reason for hiding this comment

fasttime commented Dec 12, 2023

gurgunday commented Dec 12, 2023

mdjermanovic commented Dec 13, 2023

gurgunday commented Dec 13, 2023 • edited

kecrily commented Dec 13, 2023

nzakas commented Dec 13, 2023

nzakas Dec 13, 2023

Choose a reason for hiding this comment

gurgunday Dec 13, 2023

Choose a reason for hiding this comment

fasttime Dec 13, 2023 • edited

Choose a reason for hiding this comment

gurgunday Dec 13, 2023

Choose a reason for hiding this comment

fasttime commented Dec 16, 2023

mdjermanovic commented Dec 19, 2023

mdjermanovic commented Dec 19, 2023

nzakas commented Dec 21, 2023

mdjermanovic commented Dec 23, 2023

gurgunday commented Dec 23, 2023

mdjermanovic commented Dec 23, 2023 • edited

fasttime commented Dec 23, 2023

mdjermanovic commented Dec 23, 2023

mdjermanovic Dec 26, 2023

Choose a reason for hiding this comment

gurgunday Dec 26, 2023

Choose a reason for hiding this comment

gurgunday Dec 26, 2023 • edited

Choose a reason for hiding this comment

mdjermanovic Dec 26, 2023

Choose a reason for hiding this comment

mdjermanovic left a comment

Choose a reason for hiding this comment

fasttime commented Jan 11, 2024

feat!: add two more cases to `no-implicit-coercion` #17832

feat!: add two more cases to `no-implicit-coercion` #17832

linux-foundation-easycla bot commented Dec 9, 2023 •

edited

netlify bot commented Dec 9, 2023 •

edited

gurgunday commented Dec 13, 2023 •

edited

fasttime Dec 13, 2023 •

edited

mdjermanovic commented Dec 23, 2023 •

edited

gurgunday Dec 26, 2023 •

edited