Rule suggestion: Ban comparisons of non-primitives

[benny-medflyt]

The comparison operators ===, >, <, >=, <= can be used to compare primitive values, and this is usually good and fine.

But when they are used to compare between non-primitives (objects, arrays, functions, etc..) they compare by reference, instead of by value. This is often done accidentally, when the programmer really meant to compare by value (using a ".equals" method, or a deep-equal library).

I propose a rule that bans all comparisons where the operands are a non-primitive type. This will catch accidents where the programmer made a mistake and meant to use ".equals" or a deep-equal library. Many code bases never have the need to compare for reference-equality, and for those that do, it is usually pretty rare, and can be done with a user-defined function such as "isSameObject(a, b)" that makes it much more explicit what check is being performed.

Questions:
How should any be treated?
How should generic parameters be treated?

[ajafff]

How should any be treated?

Should be allowed, because it can be anything.

How should generic parameters be treated?

Get the constraint of the type parameter and apply the same logic as for all other types.

<=, >=, < and > should allow Date objects.

Is it an error if you compare Map | string === Map, that is one side could be a primitive, the other side is definitely never a primitive?

[benny-medflyt]

How should any be treated?

Should be allowed, because it can be anything.

Sounds good 👍

How should generic parameters be treated?

Get the constraint of the type parameter and apply the same logic as for all other types.

I'm not sure I understand?

What about this function? Allowed or not? Or would each call site be checked individually?

function foo<T>(x: T, y: T): boolean {
    return x === y;
}

<=, >=, < and > should allow Date objects.

Perhaps, I personally don't mind either way. But I think there is an argument to be made not to have any special cases. Dates can be compared using getTime(). Date I think is the most common mistake where people compare them with ===, and it is very easy to make the mistake of changing a working ">" comparison to a broken "===" comparison (which of course this new rule would catch, but still there is an argument to be made in favor of encouraging consistency: always use getTime())

Is it an error if you compare Map | string === Map, that is one side could be a primitive, the other side is definitely never a primitive?

I would say that this should be an error.

[ajafff]

Let's take your example from above:

function foo<T>(x: T, y: T): boolean {
    return x === y;
}

T has no constraint, it gets an implicit constraint of any. Therefore we treat it as any.
If you have T extends number we treat it as number, in this case we allow the comparison.
If you have T extends object we treat it as reference type, i.e. add an error on the comparison.

[benny-medflyt]

I'm not sure I understand/agree with your analysis.

According to that logic if we have a function:

function foo<S, T>(x: S, y: T): boolean {
    return x === y;
}

Then x and y are to be treated as any, and so we should be allowed to compare them. But the TypeScript compiler itself rejects this function. (Operator '===' cannot be applied to types 'S' and 'T'.)

[benny-medflyt]

Also: Think about how this rule should work with switch statements

[AndreasGassmann]

Has there been any progress regarding this issue? I currently ran into this problem when switching from numbers to BigNumbers. We changed all the definitions, but in some cases we forgot to update the comparisons because they were valid.

A small example:

const a = new BigNumber('1')
const b = new BigNumber('2')
if (a > b) {
  console.log('one') // not printed
}
if (b < a) {
  console.log('two') // not printed
}
if (b === a) {
  console.log('three') // not printed
}
if (b !== a) {
  console.log('four') // printed
}

All of those were valid. I would expect tslint to give me a warning in case one and two. Probably also for three and four. They are obviously valid, but in many cases they might not have been what the developer wanted.

I would actually like the rule to only allow < and > comparisons for numbers. Even for strings it doesn't make sense most of the time in my opinion.

[AndreasGassmann]

I started working on this last month. Hopefully this week I'll have time to go over the code again and submit a PR.

I'm currently struggling with some basic questions:

• The greater-than / less-than operators should only be allowed in combination with numbers. I can't think of any edge cases where this could be a problem. Please correct me if I'm wrong.
• However when comparing equality, then I'm not quite sure how to handle it. On the one hand, I would like this to give an error:

if (new BigNumber(1) === new BigNumber(1)) {} // Error
if (new Date() === new Date()) {} // Error

But if you have two objects and would like to compare if it's the same reference, then that would not be possible:

let obj1 = {}
let obj2 = obj1;

if (obj1 === obj2) {} // Error, but it probably shouldn't?

I don't see any good way around that. It should probably be configurable. And in case of the "strict" mode, you can then do comparisons using Object.is(obj1, obj2) instead of using ===

Any thoughts on that?

[benny-medflyt]

Here is my opinion:

Ordering operators (>, >=, <, <=)

Should only be allowed in one of the following cases:

• Both operands are of type number
• Both operands are of type string (I personally think that comparing strings is a legitimate use case)

Regarding Date: JavaScript does correctly allow to compare dates, but I don't think it's necessary to support this. If the programmer wants to compare dates, she can use the getTime function: date1.getTime() < date2.getTime()

Equal (===) and not-equal (!==) operators

Should only be allowed in one of the following cases:

• Both operands are of type number
• Both operands are of type string
• Both operands are of type boolean
• One operand is of type null (the other operand may be anything)
• One operand is of type undefined (the other operand may be anything)

Comparing objects

Regarding your question of comparisons involving objects: As you see, I don't think it should be allowed at all. And it should not be configurable. The entire point of this tslint rule is to catch such uses. If you plan on doing lots of comparisons with objects, then simply don't enable this rule.

And if you still do want to enable this tslint rule and need to compare objects in certain rare circumstances, then, as you say, you can use the Object.is function. Or even better, use a more type-safe version such as:

function sameObject<T>(a: T, b:T): boolean {
    // tslint:disable-next-line:no-object-comparisons
    return a === b;
}

[benny-medflyt]

And here is another open question where I don't have an opinion or answer:

Generics

How should generics be handled? For instance, the following code:

function checkFirstEqual<T>(a: T[], b: T[]): boolean {
    const a1 = a[0];
    const b1 = b[0];
    return a1 === b1; // Legal or compile error?
}

console.log(checkFirstEqual([1,2,3], [4, 5, 6])); // Legal or compile error?
console.log(checkFirstEqual([{foo:1}], [{bar:2}])); // Legal or compile error?

[AndreasGassmann]

@benny-medflyt I submitted a PR. I would be happy to get some feedback. #4519