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

"ABScene<X extends A & B> extends Scene<X>" should not be assignable to Scene<A> #40624

Closed
Gittenburg opened this issue Sep 18, 2020 · 4 comments
Labels
Duplicate An existing issue was already created

Comments

@Gittenburg
Copy link

TypeScript Version: 4.1.0-dev.20200917

Search Terms:

generics, generic constraints, extends, intersection, intersect, assignable, type safety, compatible

Code

class Scene<X> {
   constructor(public x: X){}
   something(): number {return 0}
}

interface A {
    a: string
}
interface B {
    b: string
}

class ABScene<X extends A & B> extends Scene<X> {
    something(){
        return this.x.a.length + this.x.b.length;
    }
}

const a: Scene<A> = new ABScene<A & B>({a: 'a', b: 'b'}); // no error?
// This circumvents the constraint that X must extend A & B.

a.x = {a: 'a'}
// We can now assign just A.

a.something()
// The method fails because it tries to access b which is not defined anymore.

Expected behavior:

There should be an error that type 'A' does not satisfy the constraint 'A & B'.

Actual behavior:

It compiles with no error.

I guess the compiler forgets the generic constraint of the subclass.

Playground Link:

Playground Link

Related Issues:
#31006

@Gittenburg Gittenburg changed the title "ABScene<X extends A & B> extends Scene<X>" should not be assignable to Scene<A|B> "ABScene<X extends A & B> extends Scene<X>" should not be assignable to Scene<A> Sep 18, 2020
@MartinJohns
Copy link
Contributor

MartinJohns commented Sep 18, 2020

const a: Scene<A> = new ABScene<A & B>({a: 'a', b: 'b'}); // no error?
// This circumvents the constraint that X must extend A & B.

But there is no such constraint. Nothing in the type ABScene<A & B> causes incompatibility to the type Scene<A> - they're structurally the same.

Type arguments themselves are not considered for structural comparison:

type Example<A> { value: string }

// Works just fine, because both types are structurally compatible.
const exampleA: Example<string> = { value: '' };
const exampleB: Example<number> = exampleA;

My guess would be that this is a design limitation and you should design your class differently / immutable, but maybe the TypeScript team can shed more lights.

@Gittenburg
Copy link
Author

Gittenburg commented Sep 18, 2020

Well the constraint does exist for the ABScene class (the following assignment fails if we do not cast it to Scene<A>). My point is that casting it to Scene<A> looses the constraint and thereby the type safety within the class.

But yeah, I also guess that this isn't a bug and just an inconvenient side-effect of structural comparison, which is a damn shame, because this pattern provides a very convenient way to build a powerful composable class hierarchy. So my two questions are:

  • Is there a convenient way to achieve similar flexibility in a truly type safe manner with TypeScript?
  • Is it thinkable that TypeScript could be more clever about class comparisons and respect the semantics of generic constraints?

@andrewbranch
Copy link
Member

This is a soundness hole, but it’s really much simpler than your example: playground.

The hole is that properties (and index signatures, as in the array example) are compared covariantly, when really that’s only sound if they’re readonly. Properties that are both readable and writable ought to be compared invariantly, if we’re being as strict as possible. This turns out not to cause problems for people all that often in practice, but it’s a pretty popular request nonetheless. I’d point you to #1394 and #18770 (there are probably more).

I do want to emphasize that @MartinJohns is correct about the fact that type parameters only affect assignability by merit of where they’re structurally used, but that this is not “an inconvenient side-effect of structural comparison”—the strict variance you’re looking for is totally compatible with structural typing. In fact, we really like how variance arises from structure, and it does work in a strict way when it comes to function types [playground]:

declare class Box<Read, Write> {
  get: () => Read;
  set: (x: Write) => void;
}

declare let b1: Box<string, number>;
declare let b2: Box<unknown, 2>;

// This is allowed
b2 = b1;
b2.get();  // because here, we know this is a string, and unknown includes strings, so it's safe
b2.set(2); // and here, we know we could actually pass any number, but limiting us to 2 is safe
// so b1's contract is fulfilled by b2

// This is not allowed
declare let b3: Box<"hello", number | Date>;
b3 = b1;
// ~~~~
b3.get();           // because this says we'd get 'hello', when b1.get() can return any string
b3.set(new Date()); // and this says we can pass in a Date, even though b1.set can only take a number

Because the variance of Read and Write is determined by where they’re used in Box’s type definitions, we correctly identify that Read is covariant and Write is contravariant. In order to get an instantiation of Box that’s a supertype of Box<string, number>, we’d need Read to be instantiated with a supertype of string (e.g. unknown), whereas we’d need Write to be instantiated with a subtype of number (e.g. 2). The fact that this all works out without any special variance annotations is pretty nice, and it’s way better than assuming/enforcing that all type parameters be compared invariantly, which is technically what your example is suggesting.

Practically, I would echo @MartinJohns’s advice that you could embrace immutability (you could add readonly to x and it would save you here), as this problem really only happens when you combine mutation with passing/assigning objects to variables of other types. I know it’s a contrived example, but you should also never write an explicit type annotation like this when you can avoid it:

// Bad:
const a: Scene<A> = new ABScene<A & B>({a: 'a', b: 'b'});
// Much better:
const a = new ABScene<A & B>({a: 'a', b: 'b'});

@andrewbranch andrewbranch added the Duplicate An existing issue was already created label Sep 18, 2020
@Gittenburg
Copy link
Author

Thank you so much for your elaborate explanation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

3 participants