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

static property inheritance complaining when it shouldn't #4628

Open
benlesh opened this issue Sep 3, 2015 · 28 comments · May be fixed by #39699
Open

static property inheritance complaining when it shouldn't #4628

benlesh opened this issue Sep 3, 2015 · 28 comments · May be fixed by #39699
Labels
Committed The team has roadmapped this issue Suggestion An idea for TypeScript
Milestone

Comments

@benlesh
Copy link

benlesh commented Sep 3, 2015

I've seen that you've already closed many issues on this. But I'm reporting another one in hopes to wear you down ;)

It seems I can't change the signature of a static function in a derived class. The fact that the subclass is at all aware of the static functions on the super class is really strange.

I developed in C# for years, so I know what you were trying to do... but given that you don't support a new keyword like C# does, I think this behavior is really wrong. If you're looking to target being a superset of ES6/ES7, you need to correct this behavior ASAP. Generally in JavaScript, static properties are not copied to subclasses unless the copy is explicit.

I ran into this issue with RxJS Next, where I Subject must inherit from Observable, but they both need different static create() signatures. So I end up hacking around and fighting TypeScript.

@mhegazy
Copy link
Contributor

mhegazy commented Sep 3, 2015

The intuition we tried to match is that a specialized class can stand in for its base; which is a general intuition that an OOP developer would expect.

can you expand more on your scenario, and assuming a base class is needed but not expected to be generic enough to handle the child?

@benlesh
Copy link
Author

benlesh commented Sep 3, 2015

The issue is specifically where Observable.create and Subject.create need to have different signatures.

As you can see, I've found a workaround, but it's not ideal.

@benlesh
Copy link
Author

benlesh commented Sep 3, 2015

I guess ES6 does pull the static functions over. But I'm able to redefine them at will in ES6. TypeScript complains. It's problematic for functions that are idiomatic in many JavaScript libraries like static create functions.

@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Sep 3, 2015
@RyanCavanaugh
Copy link
Member

Summary of discussion from the Slog today:

  • Static inheritance is part of the ES6 spec, but...
  • Some people use the static side of classes (i.e. constructor functions with static properties) polymorphically, and some don't
  • The first group wants this check as much as they want general substitutability checks for the instance side of classes
  • The second group doesn't care at all if their static members align or not
  • The first group should have their substitutability failures detected at the use sites, which is worse than at declaration sites, but still (mostly) works
  • The second group is just out of luck in our current design

Conclusion: Try removing the check for assignability of the static side of classes and see what kind of errors go away (mostly in the baselines, since we mostly have clean code in our real-world-code baselines). Proceed depending on those results.

The Committed tag here is tentative.

@SinoBoeckmann
Copy link

So... and now there a plans to change this? I mean, this is pretty old now? Plans for a survey or something?

@RyanCavanaugh RyanCavanaugh removed their assignment Oct 17, 2019
@benwerner01
Copy link

benwerner01 commented Feb 7, 2020

Static factory methods are a useful design paradigm, for instance when object creation relies on data that needs to be asynchronously fetched a static create() method can encapsulate the data fetching, and return the object type (as such fetches cannot be conducted directly in the constructor). This behaviour also aligns with the OOP principles in the popular object oriented languages C# and Java. Requiring that in inheritance each static method with the same name must have compatible types restricts this significantly (for create(), this would enforce the use of the same parameters). Is there an argument for maintaining this restriction?

For example, consider the following:

class Entity {
  id: string;
  name: string;
  constructor(id: string, name: string) {
    this.name = name;
    this.id  = id;
  }

  static async create = (name: string): Promise<Entity> => {
    const { id } = await db.createEntity(name);
    return new Entity(id, name);
  }
}

class Bob extends Entity {
  age: number;
  constructor(id: string, age: number) {
    super(id, 'Bob');
    this.age = age;
  }

  // TS Error: Types of property 'create' are incompatible
  static async create = (age: number): Promise<Bob> => {
    const { id } = await db.createBob(age);
    return new Bob(id, age);
  }
}

@marijnh
Copy link

marijnh commented Apr 20, 2020

I agree that this should at least be a strictness option that can be turned off. In some circumstances people might actually use constructor values and their static methods polymorphically, but I don't think that's a widely understood expectation, and this restriction gets in the way of entirely reasonable code such as static constructor methods with the same name but different parameter types.

@Sayan751
Copy link

Sayan751 commented Sep 7, 2020

Recently ran into this as well.

type Class<T> = {
    readonly prototype: T;
    new(...args: any[]): T;
};

class Foo {
    public static create<TFoo extends Foo = Foo>(this: Class<TFoo>, model: Partial<TFoo>): TFoo {
        return new this(model.propertyA);
    }

    public constructor(public propertyA?: string) { }
}

class Bar extends Foo {

    public static create<TBar extends Bar = Bar>(this: Class<TBar>,model: Partial<TBar>): TBar {
        return new this(
            model.propertyB,
            model.propertyA,
        );
    }

    public constructor(
        public propertyB: number,
        public propertyA?: string,
    ) {
        super(propertyA);
    }
}

Intuitively speaking, as Bar#create breaks the prototypal "link", I find this restriction bit of an annoyance. Refer this playground link.

Contextually, I also tried to use the InstanceType utility class to find a workaround. But w/o much luck. Here is the playground link for that.

Due to the lack of better example, I would like to point out that with C#, similar behavior is possible (just to justify the pattern/example).

using System;

namespace trash_console
{
  class Program
  {
    static void Main(string[] args)
    {
      Bar.Create();
      Foo.Create();
      Console.ReadLine();

      // output:
      //
      // Foo ctor
      // Bar ctor
      // Foo ctor
    }
  }

  internal class Foo
  {
    public static Foo Create()
    {
      return new Foo();
    }

    public Foo()
    {
      Console.WriteLine("Foo ctor");
    }
  }

  internal class Bar : Foo
  {
    public new static Bar Create()
    {
      return new Bar();
    }

    public Bar()
    {
      Console.WriteLine("Bar ctor");
    }
  }
}

@shicks
Copy link
Contributor

shicks commented Sep 21, 2020

This is causing a number of problems trying to migrate Closure Library to TypeScript. In particular, many of our legacy classes make common use of the pattern

class Component {}
namespace Component {
  export enum EventType { FOO }
}

class Button extends Component {} // TS2417
namespace Button {
  export enum EventType { BAR }
}

But this complains about class-side inheritance since the two EventType enums are incompatible.

In actuality, subclass constructors are not generally substitutable for one another, so enforcing it at the class level seems misguided. In particular, all the existing enforcement does nothing to warn about the following broken code:

class Component {
  private readonly x = 1;
  constructor(readonly componentType: string) {}

  static create(): Component {
    return new this('generic');
  }
}

class Button extends Component {
  private readonly y = 2;
  constructor(readonly size: number) {
    super('button');
    if (typeof size !== 'number') throw Error('oops!');
  }
}

Button.create(); // NOTE: no error, but throws

The correct solution would be to to (ideally) ban unannotated use of this in static methods of (non-final) classes and require explicit specification:

class Component {
  static create<T extends typeof Component>(this: T) { ... }
}

Button.create(); // Error: Button not assignable to typeof Component

This triggers an error at the call site of Button.create() since TypeScript can already tell that Button is not assignable to typeof Component. Given this, it makes sense to loosen up on the requirement that overridden static properties must be compatible. TypeScript's structural typing is smart enough to know when it matters.

EDIT: clarified the code examples a little

@Luxcium
Copy link

Luxcium commented Dec 18, 2020

Class static side 'typeof Maybelist' incorrectly extends base class static side 'typeof Monad'.

Class static side 'typeof Maybelist' incorrectly extends base class static side 'typeof Monad'.
  Types of property 'fromValueOf' are incompatible.
    Type '<xTVal>(value: Functor<xTVal[]>) => Maybelist<xTVal, xTVal[]>' is not assignable to type '<yTVal>(value: Functor<yTVal>) => Monad<yTVal>'.
      Types of parameters 'value' and 'value' are incompatible.
        Type 'Functor<yTVal>' is not assignable to type 'Functor<unknown[]>'.
          Type 'yTVal' is not assignable to type 'unknown[]'.ts(2417)

It is not clear to me why the error is on the class not on the static method I would like to disable this error I have other static methods in the same class that would benefit from this type check so I would like to be able to disable it at the static method level instead of the class level or the project level but anything will be appreciated since it will fix a 5 year old issue (#39699)

Screenshot_src_classes_monad
Click! to enlarge

Left pane: public static fromValueOf in class Maybelist uses generic xTVal line 24

// Line 24 in class Maybelist<Val = unknown, MLVal extends Array<Val> = Val[ ]> extends Monad<MLVal> :
  public static fromValueOf<xTVal>(value: Functor<xTVal[]>): Maybelist<xTVal> {
    return Maybelist.of<xTVal>(
      (Monad.from<xTVal[]>(value.clone).fork as unknown) as xTVal,
    );
  }
/* [...] */

// could have been simplified like this:
  public static fromValueOf<xTVal>(value: Functor<xTVal[]>): Maybelist<xTVal> {
    return Maybelist.of<xTVal>(value.clone);
  }
/* [...] */

Right pane: public static fromValueOf in class Monad uses generic yTVal line 15

// Line 15 in class Monad<MVal = unknown> extends Chain<MVal> implements IMonad<MVal> :
   public static fromValueOf<yTVal>(value: Functor<yTVal>): Monad<yTVal> {
    return Monad.from<yTVal>(value.clone);
  }
/* [...] */

Playground

Link to playground simplified version

Class static side 'typeof Maybelist' incorrectly extends base class static side 'typeof Monad'.
  Types of property 'fromValueOf' are incompatible.
    Type '<xTVal>(value: Monad<xTVal[]>) => Maybelist<xTVal, xTVal[]>' is not assignable to type '<yTVal>(value: Monad<yTVal>) => Monad<yTVal>'.
      Types of parameters 'value' and 'value' are incompatible.
        Type 'Monad<yTVal>' is not assignable to type 'Monad<unknown[]>'.
          Type 'yTVal' is not assignable to type 'unknown[]'.(2417)

I don't know what @sandersn will think of that... I am working on this thing for too long... probably I should just switch back to JavaScript (People who know me know I am addicted to TypeScript and I won't go back to JavaScript)

@justinfagnani
Copy link

@realh: "If they were instance methods, yes that would cause problems whether you were using JS or TS, but as they're static methods you're always going to qualify them when you call them eg GtkSettings.install_property(pspec)"

This is not always true. My team has a lot of code where static members are the implementation of a static interface that is accessed generically on constructor instances.

This is a simple example of configuration statics that cause problems because static declarations are allowed to narrow the static type of a supertype. Once the type is narrowed, a further subclass can't widen it again without a cast:

class A {
  static p: string | string[];
}

class B extends A {
  static p = 'B';
}

// Error
// Class static side 'typeof C' incorrectly extends base class static side 'typeof B'.
//  Types of property 'p' are incompatible.
//    Type 'string[]' is not assignable to type 'string'.(2417)
class C extends B {
  static p = ['B'];
}

function getP(ctor: typeof A): string {
  return typeof ctor.p === 'string' ? ctor.p : ctor.p.join(',');
}

What I think I want here is that without an explicit type annotation, B.p kept the type of B's superclass. I'm admittedly unsure what other problems that might cause.

@shicks
Copy link
Contributor

shicks commented Feb 11, 2021

I'd argue that the extends-time enforcement is the fundamental problem. The check ultimately needs to be at the usage site. In your example, the call to getP(C) will typecheck just fine because typeof C is a subtype of typeof A, but this is not just because C is a subtype of A (subtypeness is not invariant across typeof, though that's a partial requirement): it's also necessary that the static side be completely compatible. That's when the type checker should enforce the static side types. And if D extends A is never passed as typeof A then it can define p to be whatever type it wants. (For subtyping compatibility, it's also necessary that the constructor arguments be compatible; if that's not needed for some application, a structural type or interface like {p: string|string[]} could be more broadly useful.

@jerrys-msft
Copy link
Member

Yeah, this seems fatally broken to me, especially since it applies to private statics as well. And I can see no advantage to the current behavior that could possibly outweigh the loss of the factory method pattern.

@shicks
Copy link
Contributor

shicks commented Mar 5, 2021

There's also unfortunately no remotely reasonable way to work around this. Any suppression has to happen at the level of the entire class, which loses a lot of type checking.

@Arturokin
Copy link

Any update on this? i'm stuck, i want to have different parameters on parent static function and in child static function, i don't see necessary that Typescript does this with static functions

@cataclym
Copy link

cataclym commented Jan 6, 2022

Please, we need an update on this

@richardgirges
Copy link

richardgirges commented Mar 3, 2022

At the very least, is there any chance we can get the error to appear at the static method level, as opposed to the class? This would allow us to suppress it with // @ts-expect-error without having to suppress errors on the entire class.

@benlesh
Copy link
Author

benlesh commented Mar 4, 2022

FWIW: I'm going to stop watching this, but I'll leave it open for others. I'm okay with the current behavior because I try to avoid inheritance like the plague now. ... and static methods, TBH. haha. Partially because of this issue.

@shicks
Copy link
Contributor

shicks commented Jun 6, 2022

I still believe this is worth fixing (see my earlier comments), but I've come up with a free-after-typechecking workaround that at least unblocks me, so I thought I'd share it here:

type OmitStatics<T, S extends string> =
    T extends {new(...args: infer A): infer R} ?
        {new(...args: A): R}&Omit<T, S> :
        Omit<T, S>;

Example usage:

class Base {}
namespace Base {
  export enum EventType { A }
}
class Sub extends (Base as OmitStatics<typeof Base, 'EventType'>) {}
namespace Sub {
  export enum EventType { B }
}

TypeScript fully understands the correct class relationship, but it doesn't complain about the otherwise-incompatible override anymore.

Warning: nothing prevents you from calling an incompatibly-overridden static method unsafely (e.g. Base's static foo() calls this.bar(42) but Sub incompatibly overrides static bar(arg: string)). If you want that extra protection, you could take a broader approach by eliminating all statics from the base class (this has the added "benefit" of enforcing that statics are only called directly on the classes they're defined on, which I'd consider best practice as long as you're not relying on the class-side dynamism):

type OmitAllStatics<T extends {new(...args: any[]): any, prototype: any}> =
    T extends {new(...args: infer A): infer R, prototype: infer P} ?
        {new(...args: A): R, prototype: P} :
        never;
class Sub extends (Base as OmitAllStatics<typeof Base>) { /* ... */ }

Playground examples

@zimtsui
Copy link

zimtsui commented Jul 27, 2022

In functional type theory, a type is defined as a set of object along with a set of operation. It's analogous to an algebraic structure except it doesn't have to comply with closure axiom.

The operations above may have the type as both its input or output.

However, in TypeScript, method call is written as a.f() rather than f(a). This syntax cannot describe those operations which have another 3-party type as their input.

For example, there is a type A, which have 2 operations:

  • capture an object's state as a snapshot.
  • restore as an object from a snapshot.
capture :: A -> Snapshot
restore :: Snapshot -> A

In TypeScript, you can capture like a.capture(). But the restore operation's input is not A, you cannot write somthing like XXX.restore(), so you have to write it as a static method:

class A {
	public capture(): Snapshot;
	public static restore(s: Snapshot): A;
}
const a = A.restore(snapshot);

Therefore, the semantic meaning of static methods of a class is the algebra-like operations as well.

This is why many non-functional OOP language all deicide to inherit static members automatically.

@fatcerberus
Copy link

It's perhaps worth noting that constructor is already exempt from substitutability checks, and in JS the constructor function is the class, which means in practice there's a good chance a subclass can't stand in for its parent class anyway and at that point enforcing that all the static members of the subclass are compatible with the ones in the base class doesn't actually accomplish much.

@steveetm
Copy link

steveetm commented Mar 1, 2023

Is the following the same or a bug?

interface BaseEntity {
    id: number;
}

interface ExtendedEntity extends BaseEntity {
     someextendedprop: number;
}

class Base<T extends BaseEntity> {
        public static async bulkCreate<E extends BaseEntity, M extends Base<E>>(
            this: { new(): M },
        ): Promise<M[]> {
            return [new this];
        }
        doSomething():T {
            return {} as T;
        }
};


class Sub<T extends ExtendedEntity> extends Base<T> {
    public static async bulkCreate<E extends ExtendedEntity,M extends Sub<E>>(
        this: { new(): M },
    ): Promise<M[]> {
        return [new this];
    }
};

Strange, as if I remove doSomething tha static complain on class disappears. But I don't really understand what is the problem, the subclass returns instances which are compatible with base class instances.

  • If I have only the static declaration, everything is ok.
  • If I have only the member functions (as generic), everything is ok.
  • If I have both :
Class static side 'typeof Sub' incorrectly extends base class static side 'typeof Base'.
  Types of property 'bulkCreate' are incompatible.
    Type '<E extends ExtendedEntity, M extends Sub<E>>(this: new () => M) => Promise<M[]>' is not assignable to type '<E extends BaseEntity, M extends Base<E>>(this: new () => M) => Promise<M[]>'.
      The 'this' types of each signature are incompatible.
        Type 'new () => M' is not assignable to type 'new () => Sub<ExtendedEntity>'.
          Type 'M' is not assignable to type 'Sub<ExtendedEntity>'.
            Type 'Base<E>' is not assignable to type 'Sub<ExtendedEntity>'.
              The types returned by 'doSomething()' are incompatible between these types.
                Type 'E' is not assignable to type 'ExtendedEntity'.
                  Property 'someextendedprop' is missing in type 'BaseEntity' but required in type 'ExtendedEntity'.(2417)

@shicks
Copy link
Contributor

shicks commented Mar 1, 2023

Yes, this is the same issue. The declared type of Sub.bulkCreate is not a subtype of the declared type of Base.bulkCreate. TypeScript is expecting the static methods to follow Liskov substitution, so that you could write

Sub.bulkCreate.call(Base);

But as you've written your Sub.bulkCreate signature, you've constrained the input type M to be narrower in the subclass than in the base class (it requires this to be Sub<ExtendedEntity>, rather than a Base<BaseEntity>). This is somewhat confusing for two reasons: (1) because function parameters are contravariant, so (arg: number) => void is actually a supertype of (arg: unknown) => void, even though number is a subtype of unknown. But also (2) TypeScript plays a little fast and loose with this rule by allowing methods (i.e. using method shorthand rather than a property with a colon) to break this rule, so a lot of people get away with breaking it without realizing what they're doing.

The reason the error goes away when you remove doSomething is because TypeScript types classes structurally: so without that method the template parameter no longer shows up anywhere in the type, so that Sub<A> === Base<B> for all A and B, as far as the type checker is concerned (and Sub doesn't add any other properties, either). Removing the extra property from ExtendedEntity would also fix the error, since as you've written it Sub<A> === Base<A> already.

Back to the original issue, there should be nothing wrong with what you've written. The type checker can see that typeof Sub is not assignable to typeof Base, and can enforce this just fine. If you wanted to make it assignable, you can, but it should let you choose to have it not be assignable (which it's not). You can use the workaround I posted earlier to remove bulkCreate from Base in the extends clause, but we shouldn't have to do that.

@steveetm
Copy link

steveetm commented Mar 1, 2023

@shicks reading your workaround again after this explanation just really made it super clear, I can't upvote this enough. I switch would be nice to to disable this check(or make it appear on the function and just ignore it there), but I really think this could work well for more complex cases we use. Thanks!

*update: Still have an issue with generic classes as now the generic properties simply grabbed from Base, not Base<T>. But trying to figure out something.

pyoor added a commit to pyoor/webidl2.js that referenced this issue Apr 17, 2023
This is also due to microsoft/TypeScript#4628
which prevents changing the signature of static methods on inherited
classes.
pyoor added a commit to pyoor/webidl2.js that referenced this issue Jun 14, 2023
This is also due to microsoft/TypeScript#4628
which prevents changing the signature of static methods on inherited
classes.
saschanaz added a commit to w3c/webidl2.js that referenced this issue Jun 21, 2023
)

* feat: allow adding additional members to production parse methods

* feat: add 'extensions' option for extending existing productions

* fix: remove unnecessary spread operator

Co-authored-by: Kagami Sascha Rosylight <saschanaz@outlook.com>

* refactor: rename extension 'callback-interface' to callbackInterface

* test: improve extension parsing tests

* docs: fix up jsdoc definition for ParserOptions

* test: remove use strict

* test: merge extension test into custom-production

* test: replace customProduction with top-level CustomAttribute

* test: remove extension argument from collection utility

* docs: normalize use of Token import

* test: fix import of expect function

* docs: mark args as any

This is also due to microsoft/TypeScript#4628
which prevents changing the signature of static methods on inherited
classes.

* docs: fix path to container.js

* refactor: remove unnecessary spread operator

* docs: fix jsdoc types

Co-authored-by: Kagami Sascha Rosylight <saschanaz@outlook.com>

* docs: fix jsdoc types

Co-authored-by: Kagami Sascha Rosylight <saschanaz@outlook.com>

* fix: remove iheritance attribute from CallbackInterface

---------

Co-authored-by: Kagami Sascha Rosylight <saschanaz@outlook.com>
@hazae41
Copy link

hazae41 commented Aug 27, 2023

How about a new keyword override in the child class, that would explicitly tell the compiler to fuck off?

@Gnuxie
Copy link

Gnuxie commented Feb 15, 2024

So from what I can gather, the reason this issue is dead in the water is because of these concerns.

However, the inaction has come at the cost that you cannot share the name of your static factory methods. As I understand this exact same restriction isn't applied to constructors, as the issue would appear far more frequently. Which makes sense because it would make using constructors completely intolerable. Well this is an intolerable issue and you simply cannot use static factory methods that will clash. The type gymnast workarounds also all suck, and no one should reasonably incorporate them into their code.

The workaround I wanted to use is probably something like this: pay for the pain in verbose names, use the class name again in your static factory methods such that FooBar.factory = FooBar.FooBarFactory. However, even this sucks. If I wanted the factory to have a short name, for example having a Result static factory on an Error type to wrap them in a Result type, then you're all out of luck. There are for sure other valid examples where shorter and consistent names are desired.

What makes it acceptable for this issue to be parked? The hope that very few users come across it without quickly finding a suitable workaround, but there does seem to be a lot of confidence in that. Meanwhile I'll compromise too by using different names.

@christianschmitz
Copy link

christianschmitz commented Mar 6, 2024

FWIW: I'm going to stop watching this, but I'll leave it open for others. I'm okay with the current behavior because I try to avoid inheritance like the plague now. ... and static methods, TBH. haha. Partially because of this issue.

Good point: "prefer composition over inheritance...". Hitting this issue in Typescript can help you decide in favor of composition (without necessarily completely avoiding inheritance for an entire codebase)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Committed The team has roadmapped this issue Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.