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

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

Merged
merged 1 commit into from Jan 11, 2022

Conversation

captbaritone
Copy link
Contributor

@captbaritone captbaritone commented Jan 4, 2022

Fixes #15467 by defining isConstant with inBooleanPosition set to false as meaning:

For both string and number, if coerced to that type, the value will be constant.

Prerequisites checklist

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

[x] Bug fix (template)
[x] Changes an existing rule (template)

What changes did you make? (Give an overview)

Make no-constant-condition more correct. See #15467 for more information.

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

Would ESLint treat this as a breaking change? It will remove some false positives, but also -- in a few rare cases -- introduce a few false negatives.

@eslint-github-bot eslint-github-bot bot added triage bug labels Jan 4, 2022
@@ -262,8 +277,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

@captbaritone captbaritone Jan 4, 2022

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.

@@ -358,18 +371,6 @@ ruleTester.run("no-constant-condition", rule, {
code: "if(''+[]) {}",
errors: [{ messageId: "unexpected", type: "BinaryExpression" }]
},
{
code: "if([a]==[a]) {}",
Copy link
Contributor Author

@captbaritone captbaritone Jan 4, 2022

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

@mdjermanovic mdjermanovic Jan 6, 2022

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

@captbaritone captbaritone Jan 6, 2022

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

@mdjermanovic mdjermanovic Jan 7, 2022

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

@captbaritone captbaritone Jan 8, 2022

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

@mdjermanovic mdjermanovic Jan 9, 2022

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

@captbaritone captbaritone Jan 4, 2022

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

@captbaritone captbaritone Jan 4, 2022

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.

@mdjermanovic mdjermanovic added accepted rule and removed triage labels Jan 6, 2022
Fixes eslint#15467 by defining `isConstant` with `inBooleanPosition` set to `false` as meaning:

> For both string and number, if coerced to that type, the value will be constant.
Copy link
Member

@mdjermanovic mdjermanovic left a comment

LGTM, thanks! I'll leave this open for a few days in case anyone else wants to review it before merging.

nzakas
nzakas approved these changes Jan 11, 2022
Copy link
Member

@nzakas nzakas left a comment

LGTM

@nzakas nzakas merged commit 03ac8cf into eslint:main Jan 11, 2022
14 checks passed
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jul 11, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age label Jul 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted archived due to age bug rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants