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.

Sign up for GitHub

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

Contextually type implemented properties #6118

Closed
wants to merge 19 commits into from
Closed

Contextually type implemented properties #6118

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
c63b031
Add tests and one baseline
sandersn Dec 11, 2015
f81cac5
Contextually type implemented properties.
sandersn Dec 16, 2015
c097494
Add property widening + multiple inheritance cases
sandersn Dec 16, 2015
6388186
Accept baselines
sandersn Dec 16, 2015
6bf5c3f
Accept baseline change to bestCommonTypeOfTuple2
sandersn Dec 16, 2015
055ae8c
Contextually type undefined/null/[]/{}
sandersn Dec 21, 2015
52d1be1
Accept new baselines
sandersn Jan 7, 2016
e70dd7f
Update missed fourslash test
sandersn Jan 7, 2016
4523557
Use `getContextualType`, not `TypeMapper`
sandersn Jan 7, 2016
16a3c00
Add a few `return undefined`
sandersn Jan 7, 2016
bed2c6c
Fix check for context-sensitive function-like
sandersn Feb 2, 2016
6e25554
Merge branch 'master' into contextually-type-implemented-properties
sandersn Feb 2, 2016
dec4d6d
Update baselines for new tests
sandersn Feb 2, 2016
8f79de1
Fix bad merge in checker and rebaseline other tests
sandersn Feb 2, 2016
8ac5082
Merge branch 'master' into contextually-type-implemented-properties
sandersn Apr 20, 2016
635471d
Fix test breaks after merge
sandersn Apr 21, 2016
c1d0d18
Use whole contextual type for object literals
sandersn Apr 21, 2016
872f848
Add tests and fix exposed bugs
sandersn Apr 22, 2016
526a09b
Improve handling of conflicting inherited properties
sandersn Apr 22, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2593,6 +2593,13 @@ namespace ts {
}
}

if (declaration.kind === SyntaxKind.PropertyDeclaration) {
const type = getTypeOfBasePropertyDeclaration(declaration);
if (type) {
return type;
}
}

// Use the type of the initializer expression if one is present
if (declaration.initializer) {
return checkExpressionCached(declaration.initializer);
Expand Down Expand Up @@ -2895,6 +2902,34 @@ namespace ts {
return unknownType;
}

function getTypeOfBasePropertyDeclaration(declaration: VariableLikeDeclaration) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change to declaration: PropertyDeclaration.

if (declaration.parent.kind === SyntaxKind.ClassDeclaration) {
const property = getPropertyFromBaseInterfaces(<ClassLikeDeclaration>declaration.parent, declaration.symbol.name);
if (property) {
return getTypeOfSymbol(property);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use an explicit return?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}

function getPropertyFromBaseInterfaces(declaration: ClassLikeDeclaration, propertyName: string): Symbol {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why only base interfaces? Shouldn't we look at the base class for a contextual type?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that a derived class property would override the base class property (making their types unrelated), but on playing with it in the playground, it looks like that's not the case -- it has to be a subclass of the base class' property's type. I'll add code to check the base class too.

const implementedTypeNodes = getClassImplementsHeritageClauseElements(declaration);
if (implementedTypeNodes) {
for (const typeRefNode of implementedTypeNodes) {
const t = getTypeFromTypeReference(typeRefNode);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't going to work properly for generic types, this gets you the uninstantiated base type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, nevermind about the uninstantiated base type.

if (t !== unknownType) {
for (const property of getPropertiesOfType(t)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the loop when you could use getPropertyOfType or getPropertyOfObjectType?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know about those. Thanks.

if (property.valueDeclaration.flags & NodeFlags.Private) {
continue;
}
if (property.name === propertyName) {
return property;
}
}
}
}
}
}

function getTargetType(type: ObjectType): Type {
return type.flags & TypeFlags.Reference ? (<TypeReference>type).target : type;
}
Expand Down Expand Up @@ -7214,6 +7249,12 @@ namespace ts {
return type;
}
}
if (declaration.kind === SyntaxKind.PropertyDeclaration) {
const type = getTypeOfBasePropertyDeclaration(declaration);
if (type) {
return type;
}
}
if (isBindingPattern(declaration.name)) {
return getTypeFromBindingPattern(<BindingPattern>declaration.name, /*includePatternInType*/ true);
}
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/bestCommonTypeOfTuple2.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class F extends C { f }
class C1 implements base1 { i = "foo"; c }
>C1 : C1
>base1 : base1
>i : string
>i : any
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a regression. "foo" should be contextually typed with any, but since that doesn't affect the type of "foo", i should still infer its type as string.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After lots of discussion with you and @RyanCavanaugh, I propose a more complex rule that would revert this test change [1]:

  1. If a property has a type annotation, use that.
  2. If a property has an inherited type and no initialiser, use that.
  3. If a property has an inherited type and an initialiser, contextually type the initialiser with the inherited type, then use the type of the initialiser.

This makes the following examples correct:

interface Comparer {
  compare(x: any, y: any): number;
}
class NumCmp implements Comparer {
  compare = (x: number, y: number) => x - y;
}
let cmp = new NumCmp();
cmp.compare("foo", "bar") // ERROR here

interface Long {
  length: number;
}
class Cat implements Long {
  length = undefined;
}
let longcat = new Cat();
longcat.length = "error here"; // ERROR

[1] Note that the behaviour that caused the change is technically correct but it is a breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After more discussion, we decided to drop rule (2) and additionally special-case contextual typing of null, undefined, [] and {} so that undefined/null in these types takes the contextual type rather than undefined.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that seems reasonable; so that means a property with no initializer implicitly get the any type?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is the case like

class Base {
  f(n: number): number;
}
class Derived extends Base {
  f(n);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So n is any, and f returns any?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you plan to do when inferentially typing undefined or null? Will you leave it alone, to be later added as an inference candidate if it aligns with a type parameter? Or will you fix the type parameter, and then use that inference result to contextually type the undefined?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give an example? The case I can come up with isn't very interesting -- its behaviour wouldn't change because the contextual type wouldn't change anything.

let f: (n: number) => number;
let g: <T>(t: T) => void;
f(undefined); // first argument: number
f(undefined); // first argument: undefined which widens to any, so t is any.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the example @JsonFreeman is thinking of:

declare function f<T>(x: T, y: T): T;

const a = f(100, undefined);
const b = f(undefined, 200);

What will happen here? What will we infer for the types of a and b?

Though he should correct me if I'm wrong.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah something like that is what I meant. In @DanielRosenwasser's example, a and b should both be number. So for null and undefined, the contextual type should only take effect if you are not in an inference context (otherwise the undefined in the examples would be contextually typed by T).

>"foo" : string
>c : any

Expand Down
41 changes: 41 additions & 0 deletions tests/baselines/reference/implementedPropertyContextualTyping1.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
//// [implementedPropertyContextualTyping1.ts]
interface Event {
time: number
}
interface Base {
superHandle: (e: Event) => number;
}
interface Listener extends Base {
handle: (e: Event) => void;
}
interface Ringer {
ring: (times: number) => void;
}

class Alarm implements Listener, Ringer {
handle = e => {
let n: number = e.time;
}
superHandle = e => {
return e.time;
}
ring = times => {
let m: number = times + 1;
}
}

//// [implementedPropertyContextualTyping1.js]
var Alarm = (function () {
function Alarm() {
this.handle = function (e) {
var n = e.time;
};
this.superHandle = function (e) {
return e.time;
};
this.ring = function (times) {
var m = times + 1;
};
}
return Alarm;
}());
65 changes: 65 additions & 0 deletions tests/baselines/reference/implementedPropertyContextualTyping1.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
=== tests/cases/conformance/expressions/contextualTyping/implementedPropertyContextualTyping1.ts ===
interface Event {
>Event : Symbol(Event, Decl(implementedPropertyContextualTyping1.ts, 0, 0))

time: number
>time : Symbol(time, Decl(implementedPropertyContextualTyping1.ts, 0, 17))
}
interface Base {
>Base : Symbol(Base, Decl(implementedPropertyContextualTyping1.ts, 2, 1))

superHandle: (e: Event) => number;
>superHandle : Symbol(superHandle, Decl(implementedPropertyContextualTyping1.ts, 3, 16))
>e : Symbol(e, Decl(implementedPropertyContextualTyping1.ts, 4, 15))
>Event : Symbol(Event, Decl(implementedPropertyContextualTyping1.ts, 0, 0))
}
interface Listener extends Base {
>Listener : Symbol(Listener, Decl(implementedPropertyContextualTyping1.ts, 5, 1))
>Base : Symbol(Base, Decl(implementedPropertyContextualTyping1.ts, 2, 1))

handle: (e: Event) => void;
>handle : Symbol(handle, Decl(implementedPropertyContextualTyping1.ts, 6, 33))
>e : Symbol(e, Decl(implementedPropertyContextualTyping1.ts, 7, 10))
>Event : Symbol(Event, Decl(implementedPropertyContextualTyping1.ts, 0, 0))
}
interface Ringer {
>Ringer : Symbol(Ringer, Decl(implementedPropertyContextualTyping1.ts, 8, 1))

ring: (times: number) => void;
>ring : Symbol(ring, Decl(implementedPropertyContextualTyping1.ts, 9, 18))
>times : Symbol(times, Decl(implementedPropertyContextualTyping1.ts, 10, 8))
}

class Alarm implements Listener, Ringer {
>Alarm : Symbol(Alarm, Decl(implementedPropertyContextualTyping1.ts, 11, 1))
>Listener : Symbol(Listener, Decl(implementedPropertyContextualTyping1.ts, 5, 1))
>Ringer : Symbol(Ringer, Decl(implementedPropertyContextualTyping1.ts, 8, 1))

handle = e => {
>handle : Symbol(handle, Decl(implementedPropertyContextualTyping1.ts, 13, 41))
>e : Symbol(e, Decl(implementedPropertyContextualTyping1.ts, 14, 9))

let n: number = e.time;
>n : Symbol(n, Decl(implementedPropertyContextualTyping1.ts, 15, 5))
>e.time : Symbol(Event.time, Decl(implementedPropertyContextualTyping1.ts, 0, 17))
>e : Symbol(e, Decl(implementedPropertyContextualTyping1.ts, 14, 9))
>time : Symbol(Event.time, Decl(implementedPropertyContextualTyping1.ts, 0, 17))
}
superHandle = e => {
>superHandle : Symbol(superHandle, Decl(implementedPropertyContextualTyping1.ts, 16, 2))
>e : Symbol(e, Decl(implementedPropertyContextualTyping1.ts, 17, 14))

return e.time;
>e.time : Symbol(Event.time, Decl(implementedPropertyContextualTyping1.ts, 0, 17))
>e : Symbol(e, Decl(implementedPropertyContextualTyping1.ts, 17, 14))
>time : Symbol(Event.time, Decl(implementedPropertyContextualTyping1.ts, 0, 17))
}
ring = times => {
>ring : Symbol(ring, Decl(implementedPropertyContextualTyping1.ts, 19, 2))
>times : Symbol(times, Decl(implementedPropertyContextualTyping1.ts, 20, 7))

let m: number = times + 1;
>m : Symbol(m, Decl(implementedPropertyContextualTyping1.ts, 21, 5))
>times : Symbol(times, Decl(implementedPropertyContextualTyping1.ts, 20, 7))
}
}
70 changes: 70 additions & 0 deletions tests/baselines/reference/implementedPropertyContextualTyping1.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
=== tests/cases/conformance/expressions/contextualTyping/implementedPropertyContextualTyping1.ts ===
interface Event {
>Event : Event

time: number
>time : number
}
interface Base {
>Base : Base

superHandle: (e: Event) => number;
>superHandle : (e: Event) => number
>e : Event
>Event : Event
}
interface Listener extends Base {
>Listener : Listener
>Base : Base

handle: (e: Event) => void;
>handle : (e: Event) => void
>e : Event
>Event : Event
}
interface Ringer {
>Ringer : Ringer

ring: (times: number) => void;
>ring : (times: number) => void
>times : number
}

class Alarm implements Listener, Ringer {
>Alarm : Alarm
>Listener : Listener
>Ringer : Ringer

handle = e => {
>handle : (e: Event) => void
>e => { let n: number = e.time; } : (e: Event) => void
>e : Event

let n: number = e.time;
>n : number
>e.time : number
>e : Event
>time : number
}
superHandle = e => {
>superHandle : (e: Event) => number
>e => { return e.time; } : (e: Event) => number
>e : Event

return e.time;
>e.time : number
>e : Event
>time : number
}
ring = times => {
>ring : (times: number) => void
>times => { let m: number = times + 1; } : (times: number) => void
>times : number

let m: number = times + 1;
>m : number
>times + 1 : number
>times : number
>1 : number
}
}
15 changes: 15 additions & 0 deletions tests/baselines/reference/implementedPropertyContextualTyping2.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
tests/cases/conformance/expressions/contextualTyping/implementedPropertyContextualTyping2.ts(9,1): error TS2322: Type 'string' is not assignable to type 'number'.


==== tests/cases/conformance/expressions/contextualTyping/implementedPropertyContextualTyping2.ts (1 errors) ====
interface Long {
length: number;
}

class Cat implements Long {
length = undefined;
}
const longCat = new Cat();
longCat.length = "wat";
~~~~~~~~~~~~~~
!!! error TS2322: Type 'string' is not assignable to type 'number'.
20 changes: 20 additions & 0 deletions tests/baselines/reference/implementedPropertyContextualTyping2.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
//// [implementedPropertyContextualTyping2.ts]
interface Long {
length: number;
}

class Cat implements Long {
length = undefined;
}
const longCat = new Cat();
longCat.length = "wat";

//// [implementedPropertyContextualTyping2.js]
var Cat = (function () {
function Cat() {
this.length = undefined;
}
return Cat;
}());
var longCat = new Cat();
longCat.length = "wat";
24 changes: 24 additions & 0 deletions tests/cases/conformance/expressions/contextualTyping/implementedPropertyContextualTyping1.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
interface Event {
time: number
}
interface Base {
superHandle: (e: Event) => number;
}
interface Listener extends Base {
handle: (e: Event) => void;
}
interface Ringer {
ring: (times: number) => void;
}

class Alarm implements Listener, Ringer {
handle = e => {
let n: number = e.time;
}
superHandle = e => {
return e.time;
}
ring = times => {
let m: number = times + 1;
}
}
9 changes: 9 additions & 0 deletions tests/cases/conformance/expressions/contextualTyping/implementedPropertyContextualTyping2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
interface Long {
length: number;
}

class Cat implements Long {
length = undefined;
}
const longCat = new Cat();
longCat.length = "wat";