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

Compiler should complain about implicit return value of child class constructor, if it does not satisfy child class type #13819

Open
RobinBuschmann opened this issue Feb 1, 2017 · 9 comments
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript

Comments

@RobinBuschmann
Copy link

RobinBuschmann commented Feb 1, 2017

TypeScript Version:  2.1.5

Since TypeScript 2.1 (due to #7574) the return value of a constructor call is the return value of the super constructor. So that

class Child extends Base {}

is compiled to

function Child() {
     return _super !== null && _super.apply(this, arguments) || this;
}

Actual behavior:
The actual problem is, that the implementation of Child(see example below) will be compiled without errors, despite of the fact that the instance (child) does not satisfy its type Child, because someMethod does not exist on child.

class Base {
    hello = 'base';
    constructor() {
        return {
            hello: 'world'
        };
    }
}

class Child extends Base {  // <-- compiler does not complain
    someMethod() {}
}

const child = new Child();
child.someMethod();  // <-- compiler does NOT complain, despite of "someMethod" does not exist on {hello: 'world'}

Expected behavior:
The compiler should complain about the implicit return value of the child class constructor, if this value is not a type of the child class. So that either an explicitly defined return value for the child class constructor should be forced like...

class Child extends Base { 

   constructor() {
     super();
     return { 
       someMethod() {},
       /* ... inherited members */
     }
   }
   someMethod() {}
}

or the return value has to be extended (const base = super()) to fulfill the type Child or all members of the child class have to be optional.

@mhegazy
Copy link
Contributor

mhegazy commented Feb 1, 2017

The actual problem is, that the implementation of Child(see example below) will be compiled without errors, despite of the fact that the instance (child) does not satisfy its type Child, because someMethod does not exist on child.

But the problem is not in Child, it is in Base, and Base gets an error reported correctly. So is the request to report another error in Child as well?

@RobinBuschmann
Copy link
Author

But the problem is not in Child, it is in Base, and Base gets an error reported correctly. So is the request to report another error in Child as well?

Ah sry, that was not clear enough. The issue is not about the missing someBaseMethod of the Base class, but the missing someMethod member of the Child instance . I've adjusted the example and removed someBaseMethod. I hope this clarifies the actual problem.

@DanielRosenwasser DanielRosenwasser added the Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. label Feb 1, 2017
@DanielRosenwasser
Copy link
Member

I think this was a problem that we understood, but because of the existing difficulties in modeling the issue and the relative rarity that we anticipated, we decided to be lax.

I don't think I know of a specific mechanism to describe this behavior except for a new encoding on construct signatures. As a strawman, something like this:

interface FooStatic {
    new(): explicit return Foo
}

But even this suffers the issue that it doesn't communicate whether or not the returned object's prototype was adjusted appropriately.

@DanielRosenwasser
Copy link
Member

CC @justinfagnani for any ideas

@RobinBuschmann
Copy link
Author

But wouldn't be a compilation error sufficient enough? Since tsc already throws in this case:

class Foo {
    bar: string;
    baz: number;
    constructor() {
        return {
            bar: 'hey', // <-- baz is missing
        };
    }
}

I would expect that the compiler also throws when the described issue occurs.

@justinfagnani
Copy link

justinfagnani commented Feb 2, 2017

I think the problem is that constructors are typed to return their containing classes instance type, and not the this type. Methods that return this seem to work fine:

(ignore the runtime infinite loop in the constructor :) )

export class A {
  constructor() {
    return new A(); // no error
  }
  foo(): this {
    return new A(); // error: Type 'A' is not assignable to type 'this'.
  }
}

If constructors had to return a this, then it would force authors to either cast and say "trust me", or to do a more correct thing:

export class A {
  constructor() {
    return Object.create(new.target.prototype);
  }
}

^ I see that Object.create can already correctly infer the type for uses like Object.create(A.prototype), but I'm not sure how hard it would be to get it to understand that Object.create(new.target.prototype) has a this type.

@justinfagnani
Copy link

I wanted to add some more realistic use-cases for returning something other than this to see how it would type check:

1: Singleton / cached instances. Here's some attempt to make singletons to work with subclassing, assuming subclasses cooperate by not double-initializing...

const cache = new WeakMap();
export class A {
  constructor() {
    const cached = cache.get(new.target);
    if (cached != null) {
      return cached as this;
    }
    cache.set(new.target, this);
  }
}

2: "Constructor call trick" this is what HTMLElement does internally to upgrade custom elements during parsing. This is basically working around the fact that you can't Function.call a class constructor.

let instance: A|null;
export class A {
  constructor() {
    if (instance != null) {
      const i = instance;
      instance = null;
      return i as this; // we know it's the right type
    }
  }
}
export type Constructor<T> = new(...args: any[]) => T;

// upgrades an existing object o to be of type constructor
export function upgrade<T extends Constructor<A>>(o: any, constructor: T): T {
  instance = o;
  Object.setPrototypeOf(o, constructor.prototype);
  const upgraded = new constructor();
  console.assert(o === upgraded);
  return o;
}

@justinfagnani
Copy link

I see that 2.2rc supports new.target, but it's not inferred as a this type. Filed #13849

@justinfagnani
Copy link

@DanielRosenwasser do you think my diagnosis is sound here? Should the issue be changed to require a this type when returning from a constructor?

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature and removed Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. labels Mar 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

5 participants