TS problem with Property and subclassing

[pixelzoom]

This is a problem that I'm running into in Geometric Optics. @samreid and I worked on this for awhile, and didn't get anywhere.

The basic idea is that an abstract superclass declares an abstract Property<T>, where T is some base type. Then the subclass defines that Property<S>, where S is a subclass of T.

Here's a minimal example:

class Parent {}

class Child extends Parent {}

abstract class Optics {
  abstract thing: Property<Parent>
}

class SubOptics extends Optics {
  thing: Property<Child>;
  constructor( thing: Property<Child> ) {
    super();
    this.thing = thing;
  }
}

There are no TS errors in the above example. But if we make this change:

class Child extends Parent {
  readonly foo = 5;
}

Then thing in this line:

  thing: Property<Child>;

gets flagged with this error:

TS2416: Property 'thing' in type 'SubOptics' is not assignable to the same property in base type 'Optics'.   
  Type 'Property<Child>' is not assignable to type 'Property<Parent>'.     
    The types of 'tinyProperty.link' are incompatible between these types.       
      Type '(listener: PropertyLinkListener<Child>) => void' is not assignable to type '(listener: PropertyLinkListener<Parent>) => void'.         
        Types of parameters 'listener' and 'listener' are incompatible.           
          Types of parameters 'tinyProperty' and 'tinyProperty' are incompatible.             Type 'IReadOnlyProperty<Child>' is not assignable to type 'IReadOnlyProperty<Parent>'.               
            Types of property 'unlink' are incompatible.
                 Type '(listener: PropertyListener<Child>) => void' is not assignable to type '(listener: PropertyListener<Parent>) => void'.
                   Types of parameters 'listener' and 'listener' are incompatible.                     Type 'PropertyListener<Parent>' is not assignable to type 'PropertyListener<Child>'.
                       Type 'PropertyLazyLinkListener<Parent>' is not assignable to type 'PropertyListener<Child>'.
                         Type 'PropertyLazyLinkListener<Parent>' is not assignable to type 'PropertyLazyLinkListener<Child>'.
                           Type 'Parent' is not assignable to type 'Child'.

[pixelzoom]

Slack discussion with @jonathanolson:

Jonathan Olson 2:16 PM Ahhh, I think it's because of the link and we'd need the covariant/contravariant type control. If you could set a Property<Child> to Property<Parent>, then something else could call set( something: Property ) on it that could break it. read-only should theoretically be super safe, but writable ones would suffer from that type issue

I think Java allowed that in its generics, and failed during runtime if you did it

Chris Malley 2:19 PM I’m not clear on why/where this is would make it possible to set Property<Child> to Property<Parent>.

Jonathan Olson 2:20 PM Do you mean what the potential break would be type-wise or runtime-wise?

Chris Malley 2:21 PM Seems detectable type-wise. If I have foo: Child and try to do foo = new Parent() isn’t that type-wise? Why is it different for Property?

Jonathan Olson 2:21 PM

// class A, class B extends A, class C extends A

const a: Property<A> = new Property<B>( new B() );
a.value = new C();
// Now a Property<B> would point to a C

Chris Malley 2:22 PM I see. And this is due to the declaration in the base class: abstract thing: Property<Parent> And the error manifests only when the “shape” of Child becomes different than Parent, by adding something to Child’s API.

Jonathan Olson 2:23 PM Otherwise don't they have the same structural type, thus are assignable?

Chris Malley 2:23 PM Right. TS doesn’t care at all about the class hierarchy.

Jonathan Olson 2:23 PM yup Ahh, link DOES have the issue potentially too, breakable wait, only breakable by types

Chris Malley 2:25 PM So I guess I’m stuck having to parameterize the base class, like:

class Parent {}

class Child extends Parent {
  readonly foo = 5;
}

abstract class Optics<T> {
  abstract thing: Property<T>
}

class SubOptics extends Optics<Child> {
  thing: Property<Child>;
  constructor( thing: Property<Child> ) {
    super();
    this.thing = thing;
  }
}

Jonathan Olson 2:25 PM Perhaps there's a way of saying a IReadOnlyProperty<Child> IS a IReadOnlyProperty<Parent> for typescript

I'll investigate better ways, I'm sure this comes up

Chris Malley 2:25 PM Sure is a common OO pattern.

Jonathan Olson 2:26 PM Looking into it

Chris Malley 2:49 PM What’s interesting to me in Geometric Optics is that my use case is DerivedProperty, not Property. So there is zero chance that someone can set the value to something incorrect, because it’s not settable. But I’m being bound by the same constraints as Property, which is settable.

[samreid]

Here's a self-contained synthetic case that reproduces the problem above without relying on anything in axon. It shows that TypeScript is revealing a real type error, not that there is a fundamental problem with axon or DerivedProperty.

type FakeListener<T> = ( value: T ) => void;

class FakeProperty<T> {

  listeners: FakeListener<T>[] = [];

  addElement( t: T ) {
    this.listeners.forEach( listener => listener( t ) );
  }
}

class Parent {}

class Child extends Parent {
  speak() {
    return 'hello';
  }
}

abstract class Optics {
  abstract thing: FakeProperty<Parent>
}

class SubOptics extends Optics {
  thing: FakeProperty<Child>; // TYPE ERROR

  constructor( thing: FakeProperty<Child> ) {
    super();
    this.thing = thing;
  }
}

const t = new SubOptics( new FakeProperty<Child>() );
t.thing.listeners.push( ( child: Child ) => {
  child.speak();
} );

t.thing.addElement( new Parent() ); // TYPE ERROR

This is because TypeScript correctly identifies type error due to attempted covariant parameter types, which are not type safe. Only contravariant parameter types are type safe. If you add ts-ignore or use any and run the code anyways, you get this error:

Uncaught TypeError: child.speak is not a function

You can read more about covariance, contravariance, bivariance and invariance here: https://en.wikipedia.org/wiki/Covariance_and_contravariance_(computer_science)

Also note in particular the section "Covariant method parameter type" which discusses that some languages like Eiffel purposefully avoid this type check in order to support cases like this.

It also looks like the solution described in https://github.com/microsoft/TypeScript/issues/10717#issuecomment-768569674 matches what I originally discussed with @pixelzoom--adding a type parameter to optic like Optic<T extends OpticsShapes>

[pixelzoom]

Programming TypeScript (Cherney), pages 116-118, has a good summary of variance:

A <: B means "A is a subtype of or the same as type B" A >:B means "A is a supertype of or the same as type B" ... four sorts of variance: Invariance: You want exactly a T Covariance: You want a <: T Contravariance: You want a >: T Bivariance: You're OK with either a <: T or a >: T

... and how TypeScript addresses variance:

... When talking about types, we say that TypeScript shapes (objects and classes) are covariant in their property types. That is, for an object A to be assignable to an object B, each of its properties must be <: its corresponding property in B. ... In TypeScript, every complex type is covarient in its members - object, classes, arrays, and function return types - with one exception: function parameter types, which are contravariant.

[pixelzoom]

In @samreid's synthetic example, this statement in the Optics base class

abstract thing: FakeProperty<Parent>

...sets up the requirement that thing.link signature is:

link( listener: PropertyLinkListener< Parent >, options?: any ): void

The SubOptics subclass of Optic then defines:

thing: FakeProperty<Child>;

...which will have a link signature of:

link( listener: PropertyLinkListener<Parent>, options?: any ): void

Since TS requires function parameters to be contravariant >:, this is where the problem occurs. The superclass requires listener to be PropertyLinkListener<Parent>, but the subclass provides PropertyLinkListener<Child>, and Child <: Parent.

[jonathanolson]

We could presumably lose some type safety AND get this by doing something like link<U extends T>( listener: PropertyLinkListener<U>, options?: any ), however then you could add an overly-specific link listener (e.g. add a Path listener to a Node Property).

[pixelzoom]

Now that I understand this better, I don't think we should try to circumvent TS's variance implementation. I agree with @samreid that the correct approach in geometric-optics would be Optic<T extends OpticsShapes>. I probably won't actually do that in geometric-optics, and will stick with a "one size fits all" OpticsShapes.

So closing this issue.

[jonathanolson]

I'm able to get the following to typecheck:

class Base {
  foo: number;

  constructor() {
    this.foo = 5;
  }
}

class Subtype extends Base {
  bar: string;

  constructor() {
    super();

    this.bar = 'hi';
  }
}

const c: IReadOnlyProperty<Subtype> = new Property<Subtype>( new Subtype() );

const d: IReadOnlyProperty<Base> = c;

console.log( d.value );

It looks like it was a combination of linkAttribute and TinyProperty not using the IProperty interface in its emitter parameters.

[jonathanolson]

@samreid can you review and verify?

[jonathanolson]

I'm adding back in linkAttribute that returns any in another commit.

[samreid]

Good discovery @jonathanolson. The main idea is that IReadOnlyProperty and even IProperty omit the problematic methods with contravariant parts from their interface. Therefore I'm able to put:

class Lens extends Optic {

  readonly shapesProperty: IReadOnlyProperty<LensShapes>;

and it passes the type checker.

From the example in the top comment, if we change abstract thing: Property<Parent> to abstract thing: IProperty<Parent>, everything else works and type checks:

class Parent {}

class Child extends Parent {
  readonly foo = 5;
}

abstract class Optics {
  abstract thing: IProperty<Parent>
}

class SubOptics extends Optics {
  thing: Property<Child>;
  constructor( thing: Property<Child> ) {
    super();
    this.thing = thing;
  }
}

It is sort of "by accident" that IProperty omits the problematic parameters (parameters that break the contravariance rule) from its interface, but it's a convenient accident. If those parts could be removed from the full Property API itself, then we could use Property instead of IProperty here. But I'm not sure if that would be possible/easy.

I think the next step is to discuss with @pixelzoom.

Also assigning to @jonathanolson to discuss the possibility of removing the other problematic parts from the Property API.

[jonathanolson]

If those parts could be removed from the full Property API itself, then we could use Property instead of IProperty here.

But what about cases supporting TinyProperty?

It is sort of "by accident"

Somewhat designed, I want to keep it covariant.

the possibility of removing the other problematic parts from the Property API.

I don't see how that's possible. Property includes a lot of fields we don't want on TinyProperty (and wouldn't want to fake).

[samreid]

Thanks, let's hear from @pixelzoom next, and see if he wants to use this strategy in the Geometric Optics case where this problem was reported.

[pixelzoom]

Reopening https://github.com/phetsims/geometric-optics/issues/273 to investigate whether this works in practice in Geometric Optics.

[pixelzoom]

Working nicely now in Geometric Optics, see https://github.com/phetsims/geometric-optics/issues/273#issuecomment-993826654. Thanks!

Not sure what (if anything) still needs to be done. So assigning to @samreid and @jonathanolson to discuss.

[samreid]

Sounds good, I think this issue can be closed.