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.

Sign up for GitHub

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

Jump to bottom

fix: Prevent false positives with no-constant-condition #15486

Merged
merged 1 commit into from Jan 11, 2022
Merged

fix: Prevent false positives with no-constant-condition #15486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 23 additions & 4 deletions lib/rules/no-constant-condition.js
View file
Open in desktop
Expand Up @@ -124,7 +124,8 @@ module.exports = {
* Checks if a node has a constant truthiness value.
* @param {ASTNode} node The AST node to check.
* @param {boolean} inBooleanPosition `false` if checking branch of a condition.
* `true` in all other cases
* `true` in all other cases. When `false`, checks if -- for both string and
* number -- if coerced to that type, the value will be constant.
* @returns {Bool} true when node's truthiness is constant
* @private
*/
Expand All @@ -138,15 +139,31 @@ module.exports = {
case "Literal":
case "ArrowFunctionExpression":
case "FunctionExpression":
case "ObjectExpression":
return true;
case "ClassExpression":
case "ObjectExpression":

/**
* In theory objects like:
*
* `{toString: () => a}`
* `{valueOf: () => a}`
*
* Or a classes like:
*
* `class { static toString() { return a } }`
* `class { static valueOf() { return a } }`
*
* Are not constant verifiably when `inBooleanPosition` is
* false, but it's an edge case we've opted not to handle.
*/
return true;
case "TemplateLiteral":
return (inBooleanPosition && node.quasis.some(quasi => quasi.value.cooked.length)) ||
node.expressions.every(exp => isConstant(exp, inBooleanPosition));
node.expressions.every(exp => isConstant(exp, false));

case "ArrayExpression": {
if (node.parent.type === "BinaryExpression" && node.parent.operator === "+") {
if (!inBooleanPosition) {
return node.elements.every(element => isConstant(element, false));
}
return true;
Expand Down Expand Up @@ -196,6 +213,8 @@ module.exports = {

case "SequenceExpression":
return isConstant(node.expressions[node.expressions.length - 1], inBooleanPosition);
case "SpreadElement":
return isConstant(node.argument, inBooleanPosition);

// no default
}
Expand Down
30 changes: 14 additions & 16 deletions tests/lib/rules/no-constant-condition.js
View file
Open in desktop
Expand Up @@ -175,7 +175,17 @@ ruleTester.run("no-constant-condition", rule, {
"function* foo() {while (true) {function* foo() {yield;}yield;}}",
"function* foo() { for (let x = yield; x < 10; x++) {yield;}yield;}",
"function* foo() { for (let x = yield; ; x++) { yield; }}",
"if (new Number(x) + 1 === 2) {}"
"if (new Number(x) + 1 === 2) {}",

// #15467
"if([a]==[b]) {}",
"if (+[...a]) {}",
"if (+[...[...a]]) {}",
"if (`${[...a]}`) {}",
"if (`${[a]}`) {}",
"if (+[a]) {}",
"if (0 - [a]) {}",
"if (1 * [a]) {}"
],
invalid: [
{ code: "for(;true;);", errors: [{ messageId: "unexpected", type: "Literal" }] },
Expand Down Expand Up @@ -262,8 +272,6 @@ ruleTester.run("no-constant-condition", rule, {

// #5228 , typeof conditions
{ code: "if(typeof x){}", errors: [{ messageId: "unexpected", type: "UnaryExpression" }] },
{ code: "if(`${typeof x}`){}", errors: [{ messageId: "unexpected", type: "TemplateLiteral" }] },
{ code: "if(`${''}${typeof x}`){}", errors: [{ messageId: "unexpected", type: "TemplateLiteral" }] },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are true errors that we won't catch any more.

{ code: "if(typeof 'abc' === 'string'){}", errors: [{ messageId: "unexpected", type: "BinaryExpression" }] },
{ code: "if(a = typeof b){}", errors: [{ messageId: "unexpected", type: "AssignmentExpression" }] },
{ code: "if(a, typeof b){}", errors: [{ messageId: "unexpected", type: "SequenceExpression" }] },
Expand Down Expand Up @@ -358,18 +366,6 @@ ruleTester.run("no-constant-condition", rule, {
code: "if(''+[]) {}",
errors: [{ messageId: "unexpected", type: "BinaryExpression" }]
},
{
code: "if([a]==[a]) {}",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a true error that we won't catch any more, but it is equivalent to a false positive (if([a]==[b]) {}) which used to trigger.

Copy link
Member

Choose a reason for hiding this comment

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

Is if([a]==[b]) {} a false positive? Both sides are objects, so this should be comparison by reference, and thus always false (even when the elements are same)?

[1]==[1] // 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.

Ah, yes. Sorry, not sure what I was thinking.

That said, [a] cannot be considered "constant" for the purposes of == comparison unless we also know what we are comparing it to (and thus how it will be compared), and currently the rule does not try to determine that information.

[1] == 1
true
[0] == 1
false

So, you're absolutely right that if([a]==[b]) {} is a constant condition, but I still think it's appropriate to treat [a] as not constant when comparing with inBooleanPosition: false.

Copy link
Member

Choose a reason for hiding this comment

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

Will no-constant-binary-expression catch these cases?

  • ArrayExpression === anything
  • ArrayExpression !== anything
  • ArrayExpression == ArrayExpression
  • ArrayExpression != ArrayExpression

I'm asking because these look like realistic mistakes that will be no longer reported by no-constant-condition, so it might be good to special-case them to retain that behavior. But if they will be reported by another rule, then there's no need for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These would get caught by no-constnat-binary-expression:

  • ArrayExpression === anything
  • ArrayExpression !== anything

These would not:

  • ArrayExpression == ArrayExpression
  • ArrayExpression != ArrayExpression

However, I suspect (hope!) that most people would be using some form of the eqeqeq rule to avoid the many many pitfalls and foot-guns exposed by testing for == equality.

Copy link
Member

Choose a reason for hiding this comment

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

These would not:

  • ArrayExpression == ArrayExpression
  • ArrayExpression != ArrayExpression

I've just left a question about this on the no-constant-binary-expression PR: #15296 (comment)

errors: [{ messageId: "unexpected", type: "BinaryExpression" }]
},
{
code: "if([a] - '') {}",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a false positive. If a = 1 the condition is true, if a = 0 it is false.

errors: [{ messageId: "unexpected", type: "BinaryExpression" }]
},
{
code: "if(+[a]) {}",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a false positive. If a = 1 the condition is true, if a = 0 it is false.

errors: [{ messageId: "unexpected", type: "UnaryExpression" }]
},
{
code: "if(+1) {}",
errors: [{ messageId: "unexpected", type: "UnaryExpression" }]
Expand Down Expand Up @@ -397,7 +393,9 @@ ruleTester.run("no-constant-condition", rule, {
// Boxed primitives are always truthy
{ code: "if(new Boolean(foo)) {}", errors: [{ messageId: "unexpected" }] },
{ code: "if(new String(foo)) {}", errors: [{ messageId: "unexpected" }] },
{ code: "if(new Number(foo)) {}", errors: [{ messageId: "unexpected" }] }
{ code: "if(new Number(foo)) {}", errors: [{ messageId: "unexpected" }] },

// Spreading a constant array
{ code: "if(`${[...['a']]}`) {}", errors: [{ messageId: "unexpected" }] }
]
});