-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Allow expressions in class extends clauses #3516
Changes from 13 commits
dfa1494
80ea687
956d73e
cc81cc7
367e257
d575259
186f525
e305de1
d09d61f
de8eb22
38e3d9f
2c57776
f6bcf70
d71af8a
471f6e0
efcccaa
26fd879
5b9a1b5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1669,10 +1669,8 @@ namespace ts { | |
typeParameters: TypeParameter[]; // Type parameters (undefined if non-generic) | ||
outerTypeParameters: TypeParameter[]; // Outer type parameters (undefined if none) | ||
localTypeParameters: TypeParameter[]; // Local type parameters (undefined if none) | ||
} | ||
|
||
export interface InterfaceTypeWithBaseTypes extends InterfaceType { | ||
baseTypes: ObjectType[]; | ||
resolvedBaseConstructorType?: Type; // Resolved base constructor type of class | ||
resolvedBaseTypes: ObjectType[]; // Resolved base types | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't these still need to be deferred until after getDeclaredTypeOfSymbol? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In other words, why remove InterfaceWithBaseTypes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, they're still deferred. The pattern we use elsewhere for single deferred values is a resolvedXXX property guarded by a getXXX function that returns the value. In cases where we defer the computation of multiple properties we use an XXXWithYYY type and return the object itself. |
||
} | ||
|
||
export interface InterfaceTypeWithDeclaredMembers extends InterfaceType { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -426,7 +426,7 @@ namespace ts { | |
// Specialized signatures can have string literals as their parameters' type names | ||
return node.parent.kind === SyntaxKind.Parameter; | ||
case SyntaxKind.ExpressionWithTypeArguments: | ||
return true; | ||
return !isClassExtendsExpressionWithTypeArguments(node); | ||
|
||
// Identifiers and qualified names may be type nodes, depending on their context. Climb | ||
// above them to find the lowest container | ||
|
@@ -460,7 +460,7 @@ namespace ts { | |
} | ||
switch (parent.kind) { | ||
case SyntaxKind.ExpressionWithTypeArguments: | ||
return true; | ||
return !isClassExtendsExpressionWithTypeArguments(parent); | ||
case SyntaxKind.TypeParameter: | ||
return node === (<TypeParameterDeclaration>parent).constraint; | ||
case SyntaxKind.PropertyDeclaration: | ||
|
@@ -872,7 +872,6 @@ namespace ts { | |
while (node.parent.kind === SyntaxKind.QualifiedName) { | ||
node = node.parent; | ||
} | ||
|
||
return node.parent.kind === SyntaxKind.TypeQuery; | ||
case SyntaxKind.Identifier: | ||
if (node.parent.kind === SyntaxKind.TypeQuery) { | ||
|
@@ -920,6 +919,8 @@ namespace ts { | |
return node === (<ComputedPropertyName>parent).expression; | ||
case SyntaxKind.Decorator: | ||
return true; | ||
case SyntaxKind.ExpressionWithTypeArguments: | ||
return isClassExtendsExpressionWithTypeArguments(parent); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. |
||
default: | ||
if (isExpression(parent)) { | ||
return true; | ||
|
@@ -1913,6 +1914,12 @@ namespace ts { | |
return token >= SyntaxKind.FirstAssignment && token <= SyntaxKind.LastAssignment; | ||
} | ||
|
||
export function isClassExtendsExpressionWithTypeArguments(node: Node): boolean { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you reword this name? It sounds weird in English to have "extends" modify "expression with type arguments". Actually, if you make this take There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I need the function to take type There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The kind can be checked before calling the function. If you keep the check you can call it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Otherwise it sounds like bad English for doesClassExtendExpressionWithTypeArguments. |
||
return node.kind === SyntaxKind.ExpressionWithTypeArguments && | ||
(<HeritageClause>node.parent).token === SyntaxKind.ExtendsKeyword && | ||
node.parent.parent.kind === SyntaxKind.ClassDeclaration; | ||
} | ||
|
||
// Returns false if this heritage clause element's expression contains something unsupported | ||
// (i.e. not a name or dotted name). | ||
export function isSupportedExpressionWithTypeArguments(node: ExpressionWithTypeArguments): boolean { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,7 +41,10 @@ class TypeWriterWalker { | |
var lineAndCharacter = this.currentSourceFile.getLineAndCharacterOfPosition(actualPos); | ||
var sourceText = ts.getTextOfNodeFromSourceText(this.currentSourceFile.text, node); | ||
|
||
var type = this.checker.getTypeAtLocation(node); | ||
// Workaround to ensure we output 'C' instead of 'typeof C' for base class expressions | ||
// var type = this.checker.getTypeAtLocation(node); | ||
var type = node.parent && ts.isClassExtendsExpressionWithTypeArguments(node.parent) && this.checker.getTypeAtLocation(node.parent) || this.checker.getTypeAtLocation(node); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought this was already being done in checker in getTypeOfNode. Is this different? So in general, is an ExpressionWithTypeArguments a type node? I'm still not clear on that. And do all components of our compiler agree about the answer? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the extends clause of a class an ExpressionWithTypeArguments is considered neither a type node nor an expression node, but the contained expression is considered an expression and the contained type arguments (if any) are considered type nodes. The getTypeAtLocation function returns the base class instance type for such a node (which matches what we previously did). In other contexts, an ExpressionWithTypeArguments and its contained nodes are all considered type nodes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it. Thanks for clarifying. |
||
|
||
ts.Debug.assert(type !== undefined, "type doesn't exist"); | ||
var symbol = this.checker.getSymbolAtLocation(node); | ||
|
||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
=== tests/cases/compiler/checkForObjectTooStrict.ts === | ||
module Foo { | ||
>Foo : Symbol(Foo, Decl(checkForObjectTooStrict.ts, 0, 0)) | ||
|
||
export class Object { | ||
>Object : Symbol(Object, Decl(checkForObjectTooStrict.ts, 0, 12)) | ||
|
||
} | ||
|
||
} | ||
|
||
|
||
|
||
class Bar extends Foo.Object { // should work | ||
>Bar : Symbol(Bar, Decl(checkForObjectTooStrict.ts, 6, 1)) | ||
>Foo.Object : Symbol(Foo.Object, Decl(checkForObjectTooStrict.ts, 0, 12)) | ||
>Foo : Symbol(Foo, Decl(checkForObjectTooStrict.ts, 0, 0)) | ||
>Object : Symbol(Foo.Object, Decl(checkForObjectTooStrict.ts, 0, 12)) | ||
|
||
constructor () { | ||
|
||
super(); | ||
>super : Symbol(Foo.Object, Decl(checkForObjectTooStrict.ts, 0, 12)) | ||
|
||
} | ||
|
||
} | ||
|
||
|
||
class Baz extends Object { | ||
>Baz : Symbol(Baz, Decl(checkForObjectTooStrict.ts, 18, 1)) | ||
>Object : Symbol(Object, Decl(lib.d.ts, 92, 1), Decl(lib.d.ts, 223, 11)) | ||
|
||
constructor () { // ERROR, as expected | ||
|
||
super(); | ||
>super : Symbol(ObjectConstructor, Decl(lib.d.ts, 124, 1)) | ||
|
||
} | ||
|
||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
=== tests/cases/compiler/checkForObjectTooStrict.ts === | ||
module Foo { | ||
>Foo : typeof Foo | ||
|
||
export class Object { | ||
>Object : Object | ||
|
||
} | ||
|
||
} | ||
|
||
|
||
|
||
class Bar extends Foo.Object { // should work | ||
>Bar : Bar | ||
>Foo.Object : Foo.Object | ||
>Foo : typeof Foo | ||
>Object : typeof Foo.Object | ||
|
||
constructor () { | ||
|
||
super(); | ||
>super() : void | ||
>super : typeof Foo.Object | ||
|
||
} | ||
|
||
} | ||
|
||
|
||
class Baz extends Object { | ||
>Baz : Baz | ||
>Object : Object | ||
|
||
constructor () { // ERROR, as expected | ||
|
||
super(); | ||
>super() : void | ||
>super : ObjectConstructor | ||
|
||
} | ||
|
||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be helpful in these 4 error messages to mention what type the expression resolved to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, at least for the first and the third message.