8000 Rule proposal: disallow comparing non-numeric values with >, < operators · Issue #9335 · typescript-eslint/typescript-eslint · GitHub
[go: up one dir, main page]
Skip to content

Rule proposal: disallow comparing non-numeric values with >, < operators #9335

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
Closed
6 tasks done
osdiab opened this issue Jun 11, 2024 · 12 comments
Closed
6 tasks done

Rule proposal: disallow comparing non-numeric values with >, < operators #9335

osdiab opened this issue Jun 11, 2024 · 12 comments
Labels
enhancement: new plugin rule New rule request for eslint-plugin locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@osdiab
Copy link
osdiab commented Jun 11, 2024

Before You File a Proposal Please Confirm You Have Done The Following...

My proposal is suitable for this project

  • My proposal specifically checks TypeScript syntax, or it proposes a check that requires type information to be accurate.
  • My proposal is not a "formatting rule"; meaning it does not just enforce how code is formatted (whitespace, brace placement, etc).
  • I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Description

I ran into a bug where because numbers were passed from an API as strings, and we needed to compare two of those numbers, we ended up not actually comparing the numbers numerically but instead comparing them alphabetically. It would have been useful for us to have a rule banning code like:

const x = "hello" > "goodbye"

since in reality whenever we compare strings we usually use localeCompare() anyway.

Fail Cases

const shouldFail1 = "hello" > "goodbye";

const a = "hello";
const b = "goodbye";
const shouldFail2 = a < b;

Pass Cases

const shouldPass1 = 1 < 2;
const shouldPass2 = 2n > 1n;

const a = 1n;
const b = 2n;
const shouldPass3 = a > b;


### Additional Info

_No response_
@osdiab osdiab added enhancement: new plugin rule New rule request for eslint-plugin package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Jun 11, 2024
@bradzacher
Copy link
Member

I ran into a bug where because numbers were passed from an API as strings

By "bug" do you mean to say that the API types were wrong (eg it sent a string but the types said number) or that the API types were right but the code was wrong (eg it sent a string, the types said string, and the code compared anyways).

I don't think that such a rule truly makes sense in all cases. In a codebase you might want to mix-and-match depending on what type of string you're dealing with.
For example - ordering a set of string keys using logical comparison is fine and faster than doing a localeCompare. Or sometimes you will just want a truly stable sort rather than a locale-dependent one - for example if you want a consistent ordering across locales.

There's not going to be a hard-and-fast rule for always or never using logical comparisons on strings. Sometimes one may make sense, and sometimes another would. Without a consistent case a rule is difficult - because it will have many known false positives that need to be ignored with a disable comment.

Which is the same thing I posted in this related slash almost the same issue - #5647 (comment). This request is a subset of that one.

Thats my take - so I'm a -1 on this as a rule within our project. Would like to hear what the rest of the team thinks.

@kirkwaiblinger
Copy link
Member

I ran into a bug where because numbers were passed from an API as strings

By "bug" do you mean to say that the API types were wrong (eg it sent a string but the types said number) or that the API types were right but the code was wrong (eg it sent a string, the types said string, and the code compared anyways).

+1 on this question being important.

I'm definitely somewhat sympathetic to the idea of not comparing strings with a < b. However, I'm not aware of a built-in method equivalent, i.e. something that would look like a.asciiCompare(b). Is there one? If there isn't, I think this would be a pretty tough sell.

From the title, which says "non-numeric", I think of Date objects as well, which

  • do compare sanely with date1 < date 2
  • do not compare sanely with date1 === date2
  • Have an easy built-in alternative if you want to avoid using <: date1.getTime() < date2.getTime()

@osdiab
Copy link
Author
osdiab commented Jun 12, 2024

As in the API was correctly returning strings (because precision is important to preserve) but the code was incorrectly comparing them alphabetically.

I do not believe a rule is only justified if all possible code in all possible projects would benefit from it. A rule like this doesn't need to be in the recommended rule set.

In my case I would rather that my linter catches this sort of logical error, and in the case where it is desired, just disable the rule for that line (or use localeCompare, or just make a helper that does alphabetic comparison that turns off the rule for a single line internally). And in turn, eliminate the risk of this type of bug cropping up for my team in the future.

@osdiab
Copy link
Author
osdiab commented Jun 12, 2024 •
edited
Loading

As for whether or not to disallow date comparisons, that is not as much of an issue for me (though the equality one is relevant) - but if it is for someone I don't see why having granularity by type be a configurable option would be a problem, potentially even differentiating equality vs greater/less than comparisons.

That said maybe handling === with objects in a separate rule might make more sense, as that other issue originally requested.

Personally, I am totally fine with reserving > and < comparison entirely to numeric types for my projects, since it is exceedingly rare in my company's code to ever use it for anything else. But it would be sufficient for my needs to just disallow it for strings if other types are controversial.

@bradzacher
Copy link
Member

I do not believe a rule is only justified if all possible code in all possible projects would benefit from it.

The criteria for a new rule to be added now a days isn't just "would someone find this useful" and instead "would a large portion of our users find this useful".
So we lean on our experience as a maintenance team to discuss and decide if a rule meets our bar.

Why do we have a higher bar now? Well at this point we have well over a hundred rules. We have a massive surface area to maintain and each rule comes with a cost.
We as volunteer maintainers simply don't have the bandwidth to do it all so we need to be selective.

Not that even if our eventual verdict is no - that doesn't mean this rule shouldn't exist at all! Just that we don't think it should live within our project.

But this is all part of the discussion that we need to have to come to a decision.

@Josh-Cena
Copy link
Member

What about a restrict-comparison-operands rule? Could allow strings but ban {} < {}, for example. Strings could be intentional but there are cases where TS allows probably unsound things like comparing "[object Object]".

@bradzacher
Copy link
Member

@Josh-Cena that would be covered by the broader #5647

@Josh-Cena
Copy link
Member
Josh-Cena commented Jun 12, 2024 •
edited
Loading

So, we have another four-way scenario here with overlapping concerns.

  1. object === object: could be intentional
  2. object < object: probably unsafe
  3. string === string: definitely intentional
  4. string < string: could be intentional

#5647 wants 1 and 2, but we have a lot of doubts on 1. This one wants 4 but the impact is not general enough so I'm suggesting it be merged with 2. 2 is the only case where we definitely want to report, but all other cases are in the gray area.

Alternatively restrict-comparison-operands can check === too 🤷‍♂️

@kirkwaiblinger
Copy link
Member

TIL about TS allowing { a: 0 } < { a: 0 }. +1 to banning that (in a way that doesn't ban Dates by default) . If we do, seems like we could have an option (disabled by default) to ban string1 < string2 for small marginal cost as well as one to ban date1 < date2.

+1 to not including ===.

@JoshuaKGoldberg
Copy link
Member

Agreement from me on banning the cases that are always wrong, like { a: 0 } < { a: 0 }. 👍 sounds like we have general consensus!

@JoshuaKGoldberg JoshuaKGoldberg added accepting prs Go ahead, send a pull request that resolves this issue and removed triage Waiting for team members to take a look labels Jun 17, 2024
@bradzacher
Copy link
Member
bradzacher commented Jun 17, 2024 •
edited
Loading

@JoshuaKGoldberg if that's the only part of the proposal we're in agreement on then we should close this and instead use and refine #5647

@JoshuaKGoldberg
Copy link
Member

SGTM. Closing now, but someone yell at me if there's anything here that we should look at differently.

@JoshuaKGoldberg JoshuaKGoldberg closed this as not planned Won't fix, can't repro, duplicate, stale Jun 17, 2024
@JoshuaKGoldberg JoshuaKGoldberg removed the accepting prs Go ahead, send a pull request that resolves this issue label Jun 17, 2024
@JoshuaKGoldberg JoshuaKGoldberg mentioned this issue Jun 17, 2024
Open
6 tasks
@github-actions github-actions bot added the locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. label Jun 25, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
enhancement: new plugin rule New rule request for eslint-plugin locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@osdiab @JoshuaKGoldberg @bradzacher @kirkwaiblinger @Josh-Cena
0