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

Typescript forbids safety checks that check equality of values with different generic types #17445

Closed
UppaJung opened this issue Jul 27, 2017 · 28 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@UppaJung
Copy link

TypeScript Version: 2.4.2

When a function is passed two arguments of two different generic types, there exists the possibility that the two generic types are in fact the same type and that the two values are the same value. A function should be able to test for this case and handle it appropriately, especially when ensuring the two values are different is essential to a safety condition. However, typescript forbids us from making this comparison.

Code

// 'Operator '===' cannot be applied to types 'T' and 'U'.'
function test<T, U>(a: T, b: U): void {
  if (a === b) {
    // The caller set T and U to the same type and 'a' and 'b' to the same value.
    // Take apprpriate action
  }
}

Expected behavior:

Allow the comparison, perhaps with a TSLint warning asking me if I'm sure I want to do this.

Actual behavior:

'Operator '===' cannot be applied to types 'T' and 'U'.'

Forcing me to make workaround of

// Workaround using the three-letter profanity eschewed by all righteous TypeScript programmers
function test<T, U>(a: T, b: U): void {
  if (a === (b as any)) {
    // The caller set T and U to the same type and 'a' and 'b' to the same value.
    // Take apprpriate action
  }
}
@DanielRosenwasser
Copy link
Member

This is something we should consider. I would say that the proposed change is the following:

*S* is ***comparable to*** a type *T*, and *T* is ***comparable from*** *S*, if one of the following is true:
  * ...
  * *S* is a type parameter and the constraint of *S* is comparable to *T*.
+ * *T* is a type parameter and *S* is comparable to the constraint of *T*.

@DanielRosenwasser DanielRosenwasser added In Discussion Not yet reached consensus Suggestion An idea for TypeScript labels Aug 2, 2017
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 2.6 milestone Aug 2, 2017
@gcnew
Copy link
Contributor

gcnew commented Aug 2, 2017

Please no. There is a reason why the saying "comparing apples and oranges" exists. T and U are different types and comparison is meaningless in the general case. Casting to any is a sufficient workaround, IMHO.

@kitsonk
Copy link
Contributor

kitsonk commented Aug 2, 2017

Yeah, I agree, unless there is some sort of type relationship between the two, the type system is doing what a type system does, letting you know you might have a logic error, unless you are more specific.

This should be valid though, but isn't:

interface Foo {
    foo: string;
}

interface Bar extends Foo {
    bar?: string;
}

function test<T extends Foo, U extends Bar>(a: T, b: U) {
    if (a === b) { // Operator '===' cannot be applied to types 'T' and 'U'.
        // do something
    }
}

@gcnew
Copy link
Contributor

gcnew commented Aug 2, 2017

I don't think it should be valid. The real type of equality is <T>(x: T, y: T) => boolean. I.e. only arguments of the same type should be compared. That's the reason why in Java the first thing you check is whether the classes of the arguments are the same. If the intention is to check Foo's descendants, then test(a: Foo, b: Foo) would be a better signature.

@gcnew
Copy link
Contributor

gcnew commented Aug 2, 2017

Related: #17404

@kitsonk
Copy link
Contributor

kitsonk commented Aug 2, 2017

That's the reason why in Java the first thing you check is whether the classes of the arguments are the same.

TypeScript is not a nominal typing system. I know you know that. So comparisons to Java are grounded in folly. Types are assignable based on their structure, not their ancestry, so comparisons of two related types can end up with a variable having two different types but still be the same reference, something that is impossible in Java.

True though that test(a: Foo, b: Foo) is better solution to the particular problem here, but to disallow strict equality of references when the types are related would seem contrary to a structural type system to me.

@gcnew
Copy link
Contributor

gcnew commented Aug 2, 2017

In T extends Foo, U extends Bar, you clearly know that U has one property more. If it didn't, they'd be the same type (maybe with different aliases)! But in general, T can be any descendant, including ones not following the Bar line, and in that occasion you'd be comparing apples to oranges.

Another way to look at it is that bounds provide a minimal interface, but the concrete types are "some types" that we know nothing about. On the logical level we shouldn't be comparing two unknown things. And indeed, most of the time it makes no sense.

Do we really want to let the plethora of badly typed programs, for the rare single valid one? E.g. in the simplest case of number:

function testNumber<T extends number, U extends number>(x: T, y: U) {
    return x === y;
}

will always return false unless T and U are actually the same literal type.

PS: Please note that every type parameter can be weakened to the lower bound of {} | null | undefined. That means that any two parameters will be equitable, provided they don't have explicit incompatible bounds.

@kitsonk
Copy link
Contributor

kitsonk commented Aug 2, 2017

function testNumber<T extends number, U extends number>(x: T, y: U) {
    return x === y;
}

will always return false unless T and U are actually the same literal type.

Which the can be, quite easily, so why disallow the comparison? In that case it is entirely valid and entirely sound. Allowing the comparison doesn't make anything less sound, and it is arguable that it is statically identifying an error.

This is totally valid:

function testNumber(x: number, y: number) {
    return x === y;
}

There is a big difference in comparing value/primitive based variables and non-primative/references. The type system doesn't make a huge distinction against them, but there are certainly cases where it is valid to strictly compare two references to see if they are in fact the same object, which is a different operation than comparing if two primitives are the same value.

TypeScript in this case is asserting something is unsound when it actually doesn't know if it is unsound or not. The TypeScript team generally err on the side of pragmatic about uncertainty, only getting in the way when it is certain constructs are errors. I would see an argument if it were some sort of assignment, but a comparison, worst case is unnecessary or illogical versus being wrong and in some cases is totally valid.

@simonbuchan
Copy link

I'm a little confused about "unsound" here: JS doesn't let you override operators, so there's zero chance of a === b causing an error when run, or causing coercion like (all the?) other operatorrs. The only soundness errors TS could give here would be where the types are exclusive: (x: string, y: number) => x === y should probably error with something like 'string' and 'number' can never be equal.

If you wanted, you could make a pragmaticness argument though: (x: { foo: string }, y: { bar: number }) => x === y seems like it should be an error, but { foo: "evil", bar: 666 } is assignable to both x and y.

I would prefer it to be permissive here, but I don't feel too strongly.

@RyanCavanaugh
Copy link
Member

I have a hard time with this. It's tempting to allow any comparison with ===, but it's very easy to write code like

if (x.name === entry.getName) {
  // oops, forgot to call, this code is now unreachable
}

or

if (input.value === 0) { // forgot to use parseInt or unary+

With no information about what T and U are, it seems foolish to allow comparisons between them without some indication from the developer that they intend to do this. What about code like this, where you have two completely isolated domains of values and want help keeping them separate?

function doMap<K, V>(x: MappingThing<K, V>) {
  for (const k of x.keys()) {
    const v = x.getValue(k);
    if (k === v) { // wat, why is this allowed? keys aren't values!
      // ...
    }
  }
}

@simonbuchan
Copy link

Perhaps must share a not-top type? Or one is assignable to the other?

@UppaJung
Copy link
Author

UppaJung commented Aug 9, 2017

@RyanCavanaugh, in the two examples you give the two objects are of different native types and guaranteed to be different. Yielding an error there seems to be the correct behavior. Is it not possible to handle the case of a comparison between two generic types, for which it is unknown whether objects of the same type may be assigned to both and thus unknown in advance whether they are equal, differently?

@kitsonk
Copy link
Contributor

kitsonk commented Aug 9, 2017

I think Ryan is arguing, and I agree with him, 99% of the time you have two generics, they are so unlikely to strict equal each other. Being technically correct and being pragmatic in helping the user are two different things.

If you have real world code, that leverages generics properly, and disallowing === is a hinderance, then it would be best to share it, because the following isn't a valid use of generics:

function test<T, U>(a: T, b: U): void {
  if (a === b) {
    // The caller set T and U to the same type and 'a' and 'b' to the same value.
    // Take apprpriate action
  }
}

In this particular case, because the generics don't actually do anything, aren't really used in the code, etc. it would be a lot better (and ultimately safer) to write it this way:

function test(a: any, b: any): void {
    if (a === b) {
        //
    }
}

@simonbuchan
Copy link

@kitsonk Any case that would hit this issue would probably be some variant of this.

class Component<T = any> {
    parent: null | Component;
    children: Component[];

    props: T; // Makes Component<T> and <U> different types.

    replaceWith<U>(other: Component<U>): Component<U> {
        if (this === other) return other; // '===' cannot be applied to `this` and `Component<U>`
        if (!this.parent) return other;
        return this.parent.replaceChild(this, other);
    }

    replaceChild<U>(existing: Component, replacement: Component<U>): Component<U> {
        const index = this.children.indexOf(existing);
        if (index < 0) {
            throw new RangeError('Invalid child.');
        }
        if (this.children[index] === replacement) { // OK: LHS T is 'any'
            return replacement;
        }
        this.children[index] = replacement;
        this.expensiveUpdateChild(index);
        return replacement;
    }

    expensiveUpdateChild(index: number): void {
        // ...
    }
}

@kitsonk
Copy link
Contributor

kitsonk commented Aug 9, 2017

@simonbuchan yes, you are essentially attempting to work around #6223 (Polymorphic this and Generics). I had the opportunity to speak with the TypeScript team about #6223 and I appreciate it is challenging, but here is a case where the rest of what is logical in the type system is challenged by not being able to effectively deal with Generics and the this type.

@zpdDG4gta8XKpMCd
Copy link

zpdDG4gta8XKpMCd commented Sep 7, 2017

i say asking to enable comparison between 2 generics T and U is due to a bad design in your code, the right solution would be to ask for a comparison function from the outside:

// Right way of doing it

function test<T, U>(a: T, b: U, areEqual: (a: T, b: U) => boolean): void {
  if (areEqual(a, b)) {
    // Since at this point we don't know what T and U are, we can't make any assumptions how they
    // can be compared to each other, that's why we ask the caller to provide the comparison function
    // Take apprpriate action
  }
}

a side note, when you write generic code you can't make any assumptions as to what will be used as type arguments, with that in mind the only way to manipulate those generics is to request necessary operations to be passed from the outside (including but not limiting to areEqual as in my example)

@simonbuchan hardcoding === inside generic code is a very bad idea, because although comparing by reference is a common thing quite sometimes you need to compare:

  • structurally (all properties one-by-one recursively) or
  • partially (by one single property out of many)

@masaeedu
Copy link
Contributor

@gcnew

The real type of equality is <T>(x: T, y: T) => boolean

This doesn't really mean anything, because in this alternative formulation we're simply asking for T to be inferred as A when I pass B extends A and C extends A, instead of being given a type error.

@gcnew
Copy link
Contributor

gcnew commented Sep 16, 2017

@masaeedu Following this logic, all invokations should be allowed with T inferred to {} | null | undefined.

Basically C and B are not related at all. If you already know they are different types, how can the values be equal? o_O

@KiaraGrouwstra
Copy link
Contributor

KiaraGrouwstra commented Sep 17, 2017

I tried a PR at #18530 that uses getApparentType to resolve the constraint of type parameters, so as to error only when they clash (as one might expect in a structural type system). The actual matching logic I've left untouched as isTypeComparableTo.

This is a conservative version of @DanielRosenwasser's proposal to generally add the rule of *T* is a type parameter and *S* is comparable to the constraint of *T*, by having it handled here rather than changing the general logic. (I haven't tried the bigger change -- this smaller change has a relatively limited impact.)

To elaborate on how this would handle the different scenarios here, cutting out most of the code for brevity's sake:
Allowed (values satisfying the type requirements can match):

  • <T, U>(a: T, b: U) (mentioned by @UppaJung in the OP)
  • <T extends Foo, U extends Bar>(a: T, b: U) (mentioned by @kitsonk)
  • <T extends number, U extends number>(x: T, y: U) (mentioned by @gcnew)
  • (x: number, y: number)
  • k === v
    Errors (values satisfying the types are known to clash):
  • x.name === entry.getName (LHS is a string, RHS is a function)
  • input.value === 0 (LHS is a string, RHS is a number)

@RyanCavanaugh has a point on the current behavior potentially preventing unintended behavior in the k === v case (and similarly argued by @gcnew), although it's hard to both prevent that and still allow the other legitimate scenarios. Fortunately, the rest of this behavior seems far less controversial.

I wouldn't say we can read the user's mind to always detect their intention, but the confusion primarily exists when one side of the comparison has no constraint (and therefore could equal things like keys). Without a road of perfection, I'd argue that's the smallest available sacrifice.

@gcnew:

Basically C and B are not related at all. If you already know they are different types, how can the values be equal? o_O

Agreed. That seems orthogonal to the present topic though (that is, type variables not getting resolved to their constraint). Generics aren't the enemy here; I'd rather we improve on both issues than on neither.

@masaeedu
Copy link
Contributor

Following this logic, all invokations should be allowed with T inferred to {} | null | undefined.

I'd agree with {}, but not seeing how passing e.g. 1, 2 would allow you to infer null or undefined, since neither is a common supertype of the arguments. For practical purposes though, when === falls through to {}-based equality, it should be a warning or type error.

If you already know they are different types, how can the values be equal? o_O

They are different types, but types are a synthetic construction in a type system with structural subtyping; the same value may inhabit any number of types.

@DanielRosenwasser
Copy link
Member

To quote myself from #18530 (comment)

in #18394 (comment) we decided that we don't think users will benefit as much from the sort of change involved here. We believe most instances of comparisons between two independent generics are likely to be mistakes, and using a type assertion as a workaround is an acceptable tradeoff.

@DanielRosenwasser DanielRosenwasser added the Working as Intended The behavior described is the intended behavior; this is not a bug label Sep 19, 2017
@DanielRosenwasser DanielRosenwasser removed In Discussion Not yet reached consensus Suggestion An idea for TypeScript labels Sep 19, 2017
@DanielRosenwasser DanielRosenwasser removed this from the TypeScript 2.6 milestone Sep 19, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Oct 4, 2017

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

@mindplay-dk
Copy link

FWIW, I frequently find this feature to be incredibly annoying - consider this simple example:

function equal<A, B = A>(a: A, b: B) {
    return a === b;
}

I have something like that (but not exactly like that) in a real project with much more complex generic typings, where two types can be either the same (by default) or optionally may be different.

I think, in part, what bugs me is the error message:

Operator '===' cannot be applied to types 'A' and 'B'.

This is first of all not true - this being Javascript, you absolutely can apply the === operator to objects of different types.

The reason this message is even more annoying though, is that you don't even know if these type-arguments are of the same type or not - with type B defaulting to the same as type A in this example, it's more than just possible, it's actually quite likely.

And the work-around is quite confusing:

function equal<A, B>(a: A, b: B) {
    return a === b as any; // huh?
}

Just raises questions for the person reading the code. "why a type-cast to any?" Most times when you see as any it's being used by beginners to shut the compiler up - in strict mode especially, where, once you get proficient with the type-system, there is hardly a valid use-case for as any that can't be fixed by properly working through your types, and the use of it becomes a giant red flag in code-reviews etc.

At the very least, the error message ought to provide a more meaningful explanation - don't tell me the operator "cannot be applied", it absolutely can. You could say something like "should not be applied" or "may not be applicable to types A and B", but it really is quite confusing to be told that the === operator doesn't work on differing types, this being JS, where it actually can applied just fine.

But ideally you should provide a more idiomatic means of dealing with this scenario - perhaps some means of annotating a type argument as being comparable to another? I don't know how you'd describe that feature, to be honeste.

But as any really seems like an ugly work-around - because that's usually all as any ever is.

@RyanCavanaugh
Copy link
Member

@mindplay-dk I like to recommend people upcast when they're doing something non-suspicious:

function equal<A, B>(a: A, b: B) {
    return a === b as {}; // nothing up my sleeve
}

as {} is a lot less scary because you can be sure you're not accidently spreading an infectious any through your code

@RyanCavanaugh
Copy link
Member

Also, the implementation signature and the visible signature need not be exactly the same. You can write

function equal<A, B extends A = A>(a: A, b: B): boolean;
function equal(a: {}, b: {}) {
    return a === b;
}

if you want to do janky stuff in the function body.

This is first of all not true - this being Javascript, you absolutely can apply the === operator to objects of different types.

I mean, the entire point of TypeScript is to forbid things which are legal JavaScript but not what the programmer intended. It's allowable to write window === new ArrayBuffer() but that doesn't make any sense and it's good for us to disallow it.

@KiaraGrouwstra
Copy link
Contributor

with strictNullChecks, the above signature disallows this:

let maybeNumber: number | undefined;
equal(1, maybeNumber);

I tend to skip the generics for functions like this, as they tend to give false positives.

@RyanCavanaugh
Copy link
Member

I'm also not clear on what "this function takes two arguments that might be the same but might not be" means. Isn't that just...?

function equal(a: any, b: any) {
    return a === b;
}

@mindplay-dk
Copy link

I'm also not clear on what "this function takes two arguments that might be the same but might not be" means.

I'm over-simplifying, it's far more complex in my real use-case :-)

Anyhow, the visible vs implementation signature work-around - nice! I hadn't thought of that. It's much less a work-around than as {}, I think, and won't raise any confusion/questions. I'll take it! thanks! :-)

@microsoft microsoft locked and limited conversation to collaborators Jul 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests