8000 Rule proposal: prevent comparing object types · Issue #5647 · typescript-eslint/typescript-eslint · GitHub
[go: up one dir, main page]
Skip to content

Rule proposal: prevent comparing object types #5647

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
Open
6 tasks done
longngn opened this issue Sep 14, 2022 · 12 comments
Open
6 tasks done

Rule proposal: prevent comparing object types #5647

longngn opened this issue Sep 14, 2022 · 12 comments
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: new plugin rule New rule request for eslint-plugin package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@longngn
Copy link
longngn commented Sep 14, 2022

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

Many developers mistakenly use === or !== to compare objects whereas they intend to use isEqual or .equals. The same for strings where most will want to use localeCompare instead of operators <, <=, >=, >.

I would like to port strict-comparisons rule from TSLint to here: https://palantir.github.io/tslint/rules/strict-comparisons/

Fail Cases

const s1 = 'a'
const s2 = 'b'
if (a < b) {}

const o1 = {}
const o2 = {}
if (o1 === o2) {}

Pass Cases

const s1 = 'a'
const s2 = 'b'
if (a.localeCompare(b) < 0) {}

const o1 = {}
const o2 = {}
if (o1 === o2) {}

Additional Info

No response
@longngn longngn 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 Sep 14, 2022
@longngn longngn mentioned this issue Sep 14, 2022
Closed
3 tasks
@bradzacher
Copy link
Member

I don't think I agree with the premise of both of these checks.

Comparing strings using > and < is fine for a lot of cases if all you want is a consistent ordering for a list of strings. If you're instead looking for a "locale correct" ordering of strings, then it would be better to use localeCompare.
But there are a number of cases where it's completely fine to use one or the other - so a rule that always enforces localeCompare doesn't seem entirely correct.

For objects similarly - there are many cases where comparing by reference is what you really want to do. One case in point is when you've got readonly object types - comparing by reference is exactly what you want to do.
Even with mutable objects, there are a lot of cases where a reference check is more than adequate in determining some degree of equality.

The point I'm getting at is that the code that this rule enforces is not always going to be the correct thing to do - not just at a "some people mightn't like this", but even within a single codebase you might need to both use and not use the rule.

Looking at the history - the rule was a very late addition to tslint, which is why it's not covered in our rule alternative guide. This is the first request for the rule in 3 years, so I don't think it was a highly used rule, which doesn't indicate that it is useful for the broader TS community.

I'd be interested in hearing @JoshuaKGoldberg's thoughts given he was involved in the original TSLint implementation.

@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for team members to take a look labels Sep 14, 2022
@JoshuaKGoldberg
Copy link
Member

Yeah I'm not too motivated to add a rule that bans string <s or object ===s. We don't want to be in the business of yelling at people to use more verbose code when it doesn't likely matter. In retrospect from TSLint, I don't think those features of the rule were particularly useful for almost any users. +1 to this not being a particularly useful rule.

The one case for this rule that I can see being useful is object greater than / less than equality:

https://www.typescriptlang.org/play?#code/MYewdgzgLgBCBGArApsKBGGBeGBvAvgNwBQoksCKaATNnkcQJYBmMAFJahjADxxJdqASnpA

const object1 = {};
const object2 = {};
if (object1 < object2) {}

Maybe we could accept a trimmed down version of the rule that just flags object greater-than / less-than checks?

@bradzacher
Copy link
Member

One big issue I see is also the most common mistake we see when people look at TS types.

{} does not mean "an object" - it means "any non-null value".
Put another way:

function compare(a: {}, b: {}) {
  return a < b;
}

compare({}, {}); // obviously bad
compare(() => {}, () => {}); // obviously bad

compare(1, 2); // completely fine
compare('a', 'b'); // mostly fine

playground

So the whole premise of disallowing (type {}) === (type {}) doesn't actually make sense in TS because it doesn't mean "an object".

Going one step further to prove my point - even a type that looks like an object still doesn't mean an object:

function compare(a: {toString(): string}, b: {toString(): string}) {
  return a < b;
}

compare({}, {}); // obviously bad
compare(() => {}, () => {}); // obviously bad

compare(1, 2); // completely fine
compare('a', 'b'); // mostly fine

playground

So really the only way this rule is safe and correct is if we ensure that for a < b, both a and b are not super-types of number. Which is a difficult thing to exactly prove given TS doesn't expose any type relationship APIs.

So I'm not sure there's any part of the rule that we are strongly in favour of.

@JoshuaKGoldberg
Copy link
Member

🙃 another victim of microsoft/TypeScript#9879. Marking as blocked until that one's resolved.

@JoshuaKGoldberg JoshuaKGoldberg added blocked by external API Blocked by a tool we depend on exposing an API, such as TypeScript's Type Relationship API and removed awaiting response Issues waiting for a reply from the OP or another party labels Sep 14, 2022
@neongreen
Copy link

FWIW I'd love to have this rule available for my own code. I can think of situations where I might want reference equality, but those are rare and I'd very much rather see a warning for it that I'd have to explicitly turn off.

I'm not saying that people should be pushed towards enabling this rule — far from it. But I would appreciate having it.

@JoshuaKGoldberg JoshuaKGoldberg mentioned this issue Oct 11, 2023
Open
@mike-hogan
Copy link

Here is why I would like something like what this feature request is driving at.

I use tiny types all the time:

export interface ValueType<T> {
    value: T;
}

export interface CustomerId extends ValueType<string> {
    _type: 'customer.id';
}

export function customerId(value: string): CustomerId {
    return {
        _type: 'customer.id',
        value,
    };
}

const id1 = customerId("1")
const id2 = customerId("2")

if(id1 === id2) // I want to guard against this

if(id.value === id.value) // in favour of this

@zackdotcomputer
Copy link

I agree that this rule should not be applied by default, but I think there are valid, common mistakes that catch folks out and that having the ability to enable this rule would be helpful for preventing inconsistencies and errors.

Specifically, we were just caught out by the gotcha around Date === Date checks. So I went looking for the rule to enable to ban Date === Date checks to prevent members of our team from making that error in the future. It would be useful if I could put a warning on this check and others like it without having to make a custom rule.

@pbklink
Copy link
pbklink commented Mar 15, 2024 •
edited
Loading

Have to convert some code which previously used number to Decimal (decimal.js). Unfortunately the equivalence and comparisons all still happily compile but are now wrong. So need to carefully go through all the code to fix. This rule would have really helped.

This rule would in general be extremely helpful when working with Decimal like types as it is so easy to accidentally enter (say) x > y instead of x.greaterThan(y), because you are mentally still dealing with numbers.

Maybe in the options for the rule, you can specify which types the rule should be used on. I would include Date, Decimal and some custom objects with are date or number like.

(PS: I also need to fix the assignment by reference instead of value difference. However I am sure that asking for such a rule would be just plain rude 😃)

@Josh-Cena Josh-Cena changed the title Rule proposal: Add strict-comparisons rule Rule proposal: prevent comparing object types Jun 5, 2024
@bradzacher bradzacher mentioned this issue Jun 11, 2024
Closed
6 tasks
@JoshuaKGoldberg
Copy link
Member

See also discussion in #9335.

@kirkwaiblinger
Copy link
Member

See also discussion in #9335.

Coming from that issue, sounds like we have consensus to proceed with banning the { a: 0 } < { a: 0 } case, at a minimum. Marking accepting PRs for that subset of the proposal.

Special-casing/options around Dates/strings do not seem to have sufficient consensus to proceed at this time (alas!).

It also sounds like we have consensus amongst team members to reject the object === object check being part of the initial implementation of this rule

Note - the Date/string/object === object cases may be still reasonable to evaluate for community engagement once the rule exists in its minimal version, but let's bring that minimal version into existence first.

If other team members disagree with this path forward, please yell! 🙂

@kirkwaiblinger kirkwaiblinger added the accepting prs Go ahead, send a pull request that resolves this issue label Jun 21, 2024
@Josh-Cena
Copy link
Member
Josh-Cena commented Jun 22, 2024 •
edited
Loading

My thinking is that the rule should by default check all cases, with allowedForEquality and allowedForOrdering as options. Sticking a significant part of the rule behind an option seems weird.

@JoshuaKGoldberg
Copy link
Member

Now that v8 is released, per #7936, we can mark this issue as (now fully) unblocked! 🚀

@JoshuaKGoldberg JoshuaKGoldberg removed the blocked by external API Blocked by a tool we depend on exposing an API, such as TypeScript's Type Relationship API label Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: new plugin rule New rule request for eslint-plugin package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
9 participants
@mike-hogan @zackdotcomputer @neongreen @JoshuaKGoldberg @bradzacher @pbklink @longngn @kirkwaiblinger @Josh-Cena
0