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

Rule Change: Detect comparison to always new objects in no-constant-binary-expression #15357

Closed
1 task done
captbaritone opened this issue Nov 25, 2021 · 7 comments
Closed
1 task done
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects

Comments

@captbaritone
Copy link
Contributor

captbaritone commented Nov 25, 2021

What rule do you want to change?

no-constant-binary-expression

What change to do you want to make?

Generate more warnings

How do you think the change should be implemented?

A new default behavior

Example code

function isEmpty(arr) {
  return arr === [];
}

function isDefault(obj) {
  return obj === {value: 'default'};
}

What does the rule currently do for this code?

Does not warn

What will the rule do after it's changed?

Warn

Participation

  • I am willing to submit a pull request to implement this change.

Additional comments

A === comparison between an any value and a newly constructed object is guaranteed to always be false and can be trivially detected based on syntax. This type of programming error can be easy to make for programmers coming from compare-by-value languages.

I originally proposed this as a new rule in #15222 (you can see more context there) and it was rejected. However, now that I've realized we are calling my other proposed rule (#15222) no-constant-binary-expression I'm thinking that it makes sense to include this type of detection in no-constant-binary-expression.

In short: foo === {} or foo === [] or foo === (() => {}) are all statically provably constant binary expressions, and thus would make sense to flag as part of the new rule no-constant-binary-expression.

Thoughts?

@captbaritone captbaritone added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules labels Nov 25, 2021
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Nov 25, 2021
@nzakas
Copy link
Member

nzakas commented Nov 27, 2021

Can’t argue with that logic. 👍

@nzakas nzakas moved this from Needs Triage to Feedback Needed in Triage Nov 27, 2021
@captbaritone
Copy link
Contributor Author

Curious what, if any, further input is needed here?

@nzakas
Copy link
Member

nzakas commented Dec 2, 2021

We just need to wait for others to review

@eslint/eslint-tsc thoughts?

@btmills
Copy link
Member

btmills commented Dec 2, 2021

Logic follows, and it fits the rule, so 👍

@mdjermanovic
Copy link
Member

Makes sense to me 👍

@nzakas nzakas added the accepted There is consensus among the team that this change meets the criteria for inclusion label Dec 3, 2021
@nzakas nzakas moved this from Feedback Needed to Ready to Implement in Triage Dec 3, 2021
@nzakas
Copy link
Member

nzakas commented Dec 3, 2021

@captbaritone you are good to go here.

@captbaritone
Copy link
Contributor Author

Thanks everyone! I've incorporated this change into #15296.

Triage automation moved this from Ready to Implement to Complete Dec 3, 2021
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jun 2, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jun 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
Archived in project
Triage
Complete
Development

No branches or pull requests

4 participants