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

Class inheritance should not bypass generic parameter contravariance #53798

Closed
Feavy opened this issue Apr 16, 2023 · 11 comments
Closed

Class inheritance should not bypass generic parameter contravariance #53798

Feavy opened this issue Apr 16, 2023 · 11 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@Feavy
Copy link

Feavy commented Apr 16, 2023

Bug Report

πŸ”Ž Search Terms

contravariance bypass inheritance variance annotation

πŸ•— Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about variance.

⏯ Playground Link

Playground link with relevant code

πŸ’» Code

In this example we use contravariance to make Comparator<Animal> assignable to Comparator<Dog> : something than can compare animals can compare dogs.

class Animal { }

class Dog extends Animal {
  wouf = true;
}

class Comparator<in T> {
  public compare(a: T, b: T): boolean {
    return true;
  }
}

class DogComparator extends Comparator<Dog> {
  compareDogs = true;
}

// [ok] an Animal comparator can compare Dogs, so then it is assignable to Comparator<Dog>
const okCompareDogs: Comparator<Dog> = new Comparator<Animal>();

// [error as expected] a Dog comparator cannot compare Animals as it may require specific props of Dog for comparison 
const okCompareAnimals: Comparator<Animal> = new Comparator<Dog>();

// [should be an error] It should be the same here, the fact that DogComparator is a subclass of Comparator<Dog> should not bypass contravariance
// DogComparator which extends Comparator<Dog> is still not a subclass of Comparator<Animal> as Animal is not a subclass of Dog
const nokCompareAnimals: Comparator<Animal> = new DogComparator();

πŸ™ Actual behavior

The following line is accepted by the compiler:

const nokCompareAnimals: Comparator<Animal> = new DogComparator();

I think the problem is that as DogComparator is a subclass of Comparator, the compiler only checks for
(a: Dog, b: Dog) => boolean to be assignable to (a: Animal, b: Animal) => void, regardless of contravariance annotation put on the generic type.

πŸ™‚ Expected behavior

The nok line above should give a compilation error as contravariance on Comparator's generic parameter should prevent DogComparator (which extends Comparator<Dog>) from being treated as a Comparator<Animal>.

Even if (a: Dog, b: Dog) => boolean is assignable to (a: Animal, b: Animal) => void, there should be a second contraint to check if Animal is a subtype of Dog (which will give the error) because of contravariance on the generic parameter.

For information this is the case in Kotlin: see this playground sample

Thanks πŸ˜ƒ

@Feavy Feavy changed the title Class inheritance should not bypass generic paramater contravariance Class inheritance should not bypass generic parameter contravariance Apr 16, 2023
@whzx5byb
Copy link

I believe this is essentially the same problem described at #48240 (comment).

In your example the generic parameter T in class Comparator<in T> is still NOT really contravariant even you use the in annotation, because ts will use structural relations when comparing between different generic types (Comparator vs DogComparator), and T will be treated as bivariant.

@Feavy
Copy link
Author

Feavy commented Apr 16, 2023

Okay I understand the problem.

But in any case, when a parameter is marked with the in keyword, then it should be contravariant, not "contravariant but not really". For me this feature is unfinished.

@fatcerberus
Copy link

fatcerberus commented Apr 16, 2023

While I’m inclined to agree this is surprising, it’s also a fundamental design limitation because the type system is primarily structural. If contravariance isn’t witnessed in the structure of the type then you’re going to lose that information when using a type identity that doesn’t include the in hint.

From what I understand variance annotations were added more for the benefit of the implementer than the consumer, for exactly this reason. So in T should be read as β€œT is at least contravariant, but may be more permissive”, because that’s how the compiler will read it too.

@fatcerberus
Copy link

Put another way: If you write in T, you’re merely asserting that Foo<T> is assignable to Foo<U> where U is a subtype of T, and this is what the compiler will check for when you write the type. If the actual definition of the type says the type parameter is bivariant instead of only contra-, the constraint is still met.

@RyanCavanaugh
Copy link
Member

This is the intentional behavior. Variance annotations only apply when you're comparing two instantiations of the same type; this is the only place it even makes sense to think about doing it in the first place.

POLA is ungeneralizable, but to me this isn't surprising at all. TypeScript is a structural type system and DogComparator -> Comparator<Animal> is a structural operation (since it's not guaranteed to be a correct operation to upcast DogComparator to Comparator<Dog>), and the structural comparison succeeds on account of method parameter bivariance. That's what should happen, and that's what does happen.

The only plausible diff I could imagine here would be erroring on the in annotation since it only subsets the actual variance, but it's unclear how that's a real improvement. Variance annotations are an advanced feature and it's assumed you understand what you are doing and what the trade-offs are.

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Apr 17, 2023
@Feavy
Copy link
Author

Feavy commented Apr 17, 2023

Well, then indeed I have to admit I'm facing the limits of TypeScript's design, which makes things like that being accepted although they don't make any sense.

Anyway, thank you all for your answers.

@fatcerberus
Copy link

fatcerberus commented Apr 17, 2023

POLA is ungeneralizable, but to me this isn't surprising at all.

It's maybe a bit surprising if one is expecting the variance annotation to be part of structure (i.e. being a property of the type parameter T itself, which seems like a reasonable assumption to me; there are many situations where TS works directly with unbound type parameters), but in reality it's a property of a type identity. So there's some potential for an impedance mismatch there.

@Feavy
Copy link
Author

Feavy commented Apr 18, 2023

By the way I tried the code snippet given in the Why are function parameters bivariant? FAQ section and it actually gives a compile error (which is very welcome πŸ˜€) playground example

Is this section outdated?

@fatcerberus
Copy link

fatcerberus commented Apr 18, 2023

Yeah, I've brought it up before that that section is outdated. πŸ˜“ It used to be that all function types were bivariant but strictFunctionTypes limits it to only methods. The exclusion for methods is very intentional:

https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-6.html#strict-function-types

The stricter checking applies to all function types, except those originating in method or constructor declarations. Methods are excluded specifically to ensure generic classes and interfaces (such as Array<T>) continue to mostly relate covariantly.

If methods were strictly checked, Array<string> wouldn't be assignable to Array<string | number> because it has its type parameter in contravariant positions... which could be argued to be a good thing (arrays are, in fact, mutable), but the reality is it would break a ton of actual code--including even innocent cases like const x: Array<string | number> = [ "foo", "bar" ]--so that wasn't done.

But of course this implies that if you really do want the stricter checking for your methods, you have the option of using an interface with function-typed properties instead of methods.

@Feavy
Copy link
Author

Feavy commented Apr 18, 2023

Alright thank you! It is clearer for me now.

@typescript-bot
Copy link
Collaborator

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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

5 participants