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

Inherited typing for class property initializers #10570

Open
RyanCavanaugh opened this issue Aug 26, 2016 · 25 comments
Open

Inherited typing for class property initializers #10570

RyanCavanaugh opened this issue Aug 26, 2016 · 25 comments
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Aug 26, 2016

Problem

Initializing a class member with things like { }, null, undefined, or [] has unexpected behavior.

class Base {
  favorites = ["red", "blue"];
}
class Derived extends Base {
  favorites = [];
  constructor() {
    this.favorites.push('green'); // Can't push string onto never[], wat?
  }
}
interface Settings {
  size?: number;
  color?: string;
}
class Base {
  settings: Settings = { size: 42 };
}
class Derived extends Base {
  settings = { };
  constructor() {
    if (big) this.settings = { siz: 100 }; // no error, wat?
  }
}

Solution

New rule: When a class property is initialized with exactly null, undefined, { }, or [], the type of the property is taken from the same property of the inherited type (if one exists), rather than the type of the initializer.

The inherited type is B & I1 & I2 & ... where B is the base class and I1, I2, ... are the implemented interfaces of the class.

Examples

interface Positionable {
  position: string | null;
}
class MyPos implements Positionable {
  position = null;
  setPos(x: string) {
    this.position = x;
  }
  getPos() {
    return this.position.subtr(3); // error detected
  }
}
class Base {
  items = ['one'];
}
class Derived extends Base {
  items = []; // no longer an implicit any
}
var x = new Derived();
x.items.push(10); // Error as expected

Bad Ideas We Thought Were good

image

Contextual typing plays poorly with other behavior such as unit type positions. Consider

enum E { A, B, C }
class Base {
  thing = E.A;
}
class Derived extends Base {
  thing = E.B;
  change() {
    this.thing = E.C; // Error! wat
  }
}

This turns into a big problem because the E.B expression is contextually typed by the unit-like type E.A | E.B | E.C and so acquires the specific type E.B rather than the intended type E! Daniel found this break in Azure.

/cc conspirators @DanielRosenwasser @sandersn

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Aug 26, 2016
@sandersn
Copy link
Member

#6118 is the first failed attempt, which has further discussion of the problems with contextual typing. (All 3 previous attempts were based on contextual typing.)

@DanielRosenwasser
Copy link
Member

I'm actually okay with this idea - it reflects a lot of the same views we're taking with open/expando types.

@RyanCavanaugh
Copy link
Member Author

TODO: Try blindly taking the base class's type and see what happens in the RWC

@sandersn
Copy link
Member

This is basically what the summary of #6118 covers, except that it was for all properties, initialised or not. You intend to try with just properties that are initialised to [], {}, null, undefined, right?

@RyanCavanaugh
Copy link
Member Author

Nope, all properties

@sandersn
Copy link
Member

How is that different from this summary on #6118, then?

@RyanCavanaugh
Copy link
Member Author

RyanCavanaugh commented Sep 19, 2016

Make the declared type of the derived property the type of the base type's property, as if you had written field: typeof super.field = expr), rather than contextually type expr with typeof super.field

@sandersn
Copy link
Member

I just realised I should have linked to #10610. Confusingly, my good/bad/ugly post in #6118 actually refers to an old version of that PR, which does basically what you describe, and as far as I recall, still hits some problem.

@DanielRosenwasser, you actually RWC with #10610. Do you have notes on the problem it hit? I remember it being literal-type related, so maybe it would be different now that we have literal types everywhere.

@RyanCavanaugh
Copy link
Member Author

RyanCavanaugh commented Apr 25, 2017

Problem Scope

The only thing in scope is class property intializers, not method declarations.
Contextual typing of declared method parameters is interesting but a fully separable problem.

Problems Summary

When a base type (either an implemented interface, or an extends'd base class) has a property with the same name, our current behavior when a derived class initialized that type is not good, leading to frequent user confusion.

#10484, #11714, #15191: Initialize union type from string literal

interface I {
  xOrY: 'x' | 'y'
}
// Error, cannot assign "string" to "'x' | 'y'""
class C implements I {
  xOrY = 'x'
}

#1373, #15136: Initialize function-typed field with function expression with unannotated parameter

class Class<T> {
  a: (b: T) => string
}

class Class2 extends Class<number> {
  a = b => 'b' // Error, Parameter 'b' implicitly has an 'any' type.
}

#14684: Implicit any error when initializing with empty array

// (noImplicitAny ON, strictNullChecks OFF)
interface I {
    obj: { x: string[] }
}
// Error: x
class C implements I {
    obj = { x: [] }
}

Optional properties vanish

interface HasOpts {
	opts: {
	  x: number;
	  y?: number;
	}
}
class Foo implements HasOpts {
	opts = { x: 3 };
	bar() {
		// Error, property 'y' is excess
		this.opts = { x: 3, y: 6 };
	}
}

In approximate order of severity based on "hits":

  • Initialization of a string-literal-typed field using a string always fails
  • Initialization of a function-typed field with a function expression doesn't un-implicit-any the parameters (and produces an any parameter)
  • Initialization of an array-typed field with an empty array triggers an implicit any (and produces an any field)
  • Initialization of an object-typed field with an object literal causes unspecified optional properties to disappear from the class property type

Contextual Typing

We very clearly need to be contextually typing initializer expressions with the type implied by the extends or implements clauses.
This is the only plausible way we can fix parameters of function expressions.

Computing Inferred Property Types

What should the type of an initialized property with a matching property in the base type be (when there is no type annotation in the derived class)?
Three appealing options:

  1. The type of the expression
  2. The type of the expression as contextually typed by the base type property
  3. The type of the base type property

Option 1 is Terrible

  1. The type of the expression

Option 1 is what we do today. See above list

Option 2 is Terrible

  1. The type of the expression as contextually typed by the base type property

We have a long-standing tenet that the type of a contextually-typed expression should not be observable. This currently shows up in assignment expressions:

type Obj = { [n: string]: number } 

// OK
var x: Obj = { baz: 100 };
x['bleh'] = 10;

var y = x = { baz: 100 };
y['bleh'] = 10; // Error, but really shouldn't

and

var x: string[] = []; // x: string[]
var y = x; // y: string[]
var z = x = []; // z: implicit any[]

Simply using the contextually-typed type of the initializing expression also gives what appears to be "wrong" behavior for union types:

interface I {
  xOrY: 'x' | 'y'
}
class C implements I {
  // Literals contextually typed by literal unions don't widen
  xOrY = 'x'; // xOrY: 'x'
  changeUp() {
  	this.xOrY = 'y'; // Error, cannot convert 'y' to 'x'
  }
}

Option 3 is a Breaking Change

Breaks

Currently, this code is OK:

class Animal {
    parent = new Animal();
    move() { }
}
class Dog extends Animal {
    parent = new Dog();
    woof() {
        this.parent.woof();
    }
}

If we simply "copy down" the type of Animal.parent, then this.parent.woof() will be invalid.
The user will have to write a type annotation instead:

    parent: Dog = new Dog();

Of course, this code is already suspicious because it's unsound to writes through base class aliases:

class Animal {
    parent = new Animal();
    move() { }
    setParent(c: Cat) {
        this.parent = c;
    }
}

but in general we are not strict about this kind of thing, and in practice this is a difficult "error" to make.

Suspicious Changes?

This code is the same as an earlier example, but the spelling has been changed:

interface I {
    kind: 'widget' | 'gadget'
}
class C implements I {
    kind = 'widget';
}

It would appear that kind should be of type 'widget', not 'widget' | 'gadget'.
Of course, today its type is string (and a compile error), but it's still perhaps unexpected.

@gcnew
Copy link
Contributor

gcnew commented Apr 25, 2017

I'd vote Option 2 for readonly properties and Option 3 for mutable ones.

@RyanCavanaugh
Copy link
Member Author

A desired break not listed in the patterns above: https://stackoverflow.com/questions/44144178/typescript-doesnt-catch-state-type-errors-outside-of-constructor

@olegdunkan
Copy link

olegdunkan commented Jun 26, 2017

Actually it is not an error

y['bleh'] = 10; // Error, but really shouldn't

A bracket notation property access of the form
object [ index ]
where object and index are expressions, is used to access the property with the name computed by the index expression on the given object. A bracket notation property access is processed as follows at compile-time:
...
Otherwise, if index is of type Any, the String or Number primitive type, or an enum type, the property access is of type Any.
spec
I think you meant this:

var foo = y['bleh']; //expect foo to be of number type not any;

@atrauzzi
Copy link

atrauzzi commented Aug 8, 2017

Any update on where this is at? It's causing some very ugly casts just to get things working.

@AnthonyLenglet
Copy link

currently we're fixing this by doing:

interface IStore {
  id: string;
  storeCode: number;
}

export class Store implements IStore {
  public id: IStore['id'] = null;
  public storeCode: IStore['storeCode'] = null;

  constructor(item: IStore) {
    Object.assign(this, item, {});
  }
}

this works but a fix would definitely be really cool to get here !

@shicks
Copy link
Contributor

shicks commented Nov 18, 2019

It seems like #6118 had a mostly reasonable solution, except that it handled multiple inheritance via interfaces incorrectly. Under "The Bad" it listed an example like

abstract class Base {
  abstract x: number|string;
}
interface Contract {
  x: number;
}
class Sub extends Base implements Contract {
  x = 42;
}

as failing because the type of x was taken from Base['x'] only, rather than (Base & Contract)['x'], which would have given the correct contextual type. This still does not address breakages when subtypes have a narrowed "kind":

abstract class Base {
  abstract x: Animal;
}
class Sub extends Base {
  x = new Dog();
  method() { this.x.woof(); }
}

Potentially (as suggested earlier) the use of readonly (and/or as const?) could affect the contextualized type sufficiently to at least make fixes trivial? In the above example, if Base['x'] were readonly then presumably it's safe(r) to infer Sub['x'] as Dog? (Conversely, it's not actually an error but probably should be for only Sub['x'] to be readonly - the same thing might be fine there).

@jonlepage
Copy link

Same here by using ts with jsdoc in js for get ref, Intelisence is broke and lint type error !
With the arrival of jsdoc and ts support, I hoped to be able to stay in js vanilla with full types support !
image

4yr old issue !, i suppose i will need search another architecture approach ! 😢
pity, that restricts some ways to easily architect our projects ! hope a good ts guy can fix this someday !

@jonlepage
Copy link

ok actually this seem a good approach for me, but need using static for avoid import.
image

it's not too ugly and remains readable and accessible.
But I'd rather want do without static and force types in the children class !

const TypesEnum = {
	abc: 'abc',
	abcd: 'abcd',
	abcde: 'abcde',
};

export class Base {
   static TypesEnum = TypesEnum
	/**@type {keyof TypesEnum} - all work fine here, default null for the example purpose */
	_test = null;
	constructor(data) {}
}

export class A extends Base {
   /** @type {Base['_test']} */
	_test = 'abc'; //lint a error and we lost intellisense in Vscode , need force @type!
	constructor(data) {
		super();
	}
}

@paddotk
Copy link

paddotk commented Aug 17, 2022

This issue has been open since 2016, any updates?

@shellscape
Copy link

Checking in on this a year after the last comment.

@aikhan
Copy link

aikhan commented Mar 5, 2024

Checking in after 8 years.

@RyanCavanaugh

@jonlepage
Copy link

Checking in after 8 years.

@RyanCavanaugh

dont worry they are in discutions
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests