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

Allow expressions in class extends clauses #3516

Merged
merged 18 commits into from
Jun 17, 2015
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
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
291 changes: 169 additions & 122 deletions src/compiler/checker.ts

Large diffs are not rendered by default.

5 changes: 5 additions & 0 deletions src/compiler/diagnosticInformationMap.generated.ts
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,11 @@ namespace ts {
Cannot_find_namespace_0: { code: 2503, category: DiagnosticCategory.Error, key: "Cannot find namespace '{0}'." },
No_best_common_type_exists_among_yield_expressions: { code: 2504, category: DiagnosticCategory.Error, key: "No best common type exists among yield expressions." },
A_generator_cannot_have_a_void_type_annotation: { code: 2505, category: DiagnosticCategory.Error, key: "A generator cannot have a 'void' type annotation." },
_0_is_referenced_directly_or_indirectly_in_its_own_base_expression: { code: 2506, category: DiagnosticCategory.Error, key: "'{0}' is referenced directly or indirectly in its own base expression." },
Base_expression_is_not_of_a_constructor_function_type: { code: 2507, category: DiagnosticCategory.Error, key: "Base expression is not of a constructor function type." },
No_base_constructor_has_the_specified_number_of_type_arguments: { code: 2508, category: DiagnosticCategory.Error, key: "No base constructor has the specified number of type arguments." },
Base_constructor_does_not_return_a_class_or_interface_type: { code: 2509, category: DiagnosticCategory.Error, key: "Base constructor does not return a class or interface type." },
Base_constructors_must_all_have_the_same_return_type: { code: 2510, category: DiagnosticCategory.Error, key: "Base constructors must all have the same return type." },
Import_declaration_0_is_using_private_name_1: { code: 4000, category: DiagnosticCategory.Error, key: "Import declaration '{0}' is using private name '{1}'." },
Type_parameter_0_of_exported_class_has_or_is_using_private_name_1: { code: 4002, category: DiagnosticCategory.Error, key: "Type parameter '{0}' of exported class has or is using private name '{1}'." },
Type_parameter_0_of_exported_interface_has_or_is_using_private_name_1: { code: 4004, category: DiagnosticCategory.Error, key: "Type parameter '{0}' of exported interface has or is using private name '{1}'." },
Expand Down
20 changes: 20 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -1525,6 +1525,26 @@
"category": "Error",
"code": 2505
},
"'{0}' is referenced directly or indirectly in its own base expression.": {
"category": "Error",
"code": 2506
},
"Base expression is not of a constructor function type.": {
"category": "Error",
"code": 2507
},
"No base constructor has the specified number of type arguments.": {
"category": "Error",
"code": 2508
},
"Base constructor does not return a class or interface type.": {
"category": "Error",
"code": 2509
},
"Base constructors must all have the same return type.": {
"category": "Error",
"code": 2510
},
Copy link
Contributor

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.

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, at least for the first and the third message.


"Import declaration '{0}' is using private name '{1}'.": {
"category": "Error",
Expand Down
6 changes: 2 additions & 4 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't these still need to be deferred until after getDeclaredTypeOfSymbol?

Copy link
Contributor

Choose a reason for hiding this comment

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

In other words, why remove InterfaceWithBaseTypes?

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, 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 {
Expand Down
13 changes: 10 additions & 3 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -920,6 +919,8 @@ namespace ts {
return node === (<ComputedPropertyName>parent).expression;
case SyntaxKind.Decorator:
return true;
case SyntaxKind.ExpressionWithTypeArguments:
return isClassExtendsExpressionWithTypeArguments(parent);
Copy link
Contributor

Choose a reason for hiding this comment

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

&& node === (<ExpressionWithTypeArguments>parent).expression

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah.

default:
if (isExpression(parent)) {
return true;
Expand Down Expand Up @@ -1913,6 +1914,12 @@ namespace ts {
return token >= SyntaxKind.FirstAssignment && token <= SyntaxKind.LastAssignment;
}

export function isClassExtendsExpressionWithTypeArguments(node: Node): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The 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 ExpressionWithTypeArguments instead of Node, you can just call it isInClassExtendsClause.

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 need the function to take type Node and check the kind. Can't think of a better name.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 isExpressionWithTypeArgumentsInClassExtendsClause.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 {
Expand Down
5 changes: 4 additions & 1 deletion src/harness/typeWriter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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);

Expand Down
3 changes: 2 additions & 1 deletion src/services/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5811,7 +5811,8 @@ namespace ts {
node = node.parent;
}

return node.parent.kind === SyntaxKind.TypeReference || node.parent.kind === SyntaxKind.ExpressionWithTypeArguments;
return node.parent.kind === SyntaxKind.TypeReference ||
(node.parent.kind === SyntaxKind.ExpressionWithTypeArguments && !isClassExtendsExpressionWithTypeArguments(<ExpressionWithTypeArguments>node.parent));
}

function isNamespaceReference(node: Node): boolean {
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/aliasUsageInAccessorsOfClass.types
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,9 @@ import Backbone = require("aliasUsage1_backbone");

export class VisualizationModel extends Backbone.Model {
>VisualizationModel : VisualizationModel
>Backbone.Model : any
>Backbone.Model : Backbone.Model
>Backbone : typeof Backbone
>Model : Backbone.Model
>Model : typeof Backbone.Model

// interesting stuff here
}
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/aliasUsageInArray.types
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,9 @@ import Backbone = require("aliasUsageInArray_backbone");

export class VisualizationModel extends Backbone.Model {
>VisualizationModel : VisualizationModel
>Backbone.Model : any
>Backbone.Model : Backbone.Model
>Backbone : typeof Backbone
>Model : Backbone.Model
>Model : typeof Backbone.Model

// interesting stuff here
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@ import Backbone = require("aliasUsageInFunctionExpression_backbone");

export class VisualizationModel extends Backbone.Model {
>VisualizationModel : VisualizationModel
>Backbone.Model : any
>Backbone.Model : Backbone.Model
>Backbone : typeof Backbone
>Model : Backbone.Model
>Model : typeof Backbone.Model

// interesting stuff here
}
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/aliasUsageInGenericFunction.types
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,9 @@ import Backbone = require("aliasUsageInGenericFunction_backbone");

export class VisualizationModel extends Backbone.Model {
>VisualizationModel : VisualizationModel
>Backbone.Model : any
>Backbone.Model : Backbone.Model
>Backbone : typeof Backbone
>Model : Backbone.Model
>Model : typeof Backbone.Model

// interesting stuff here
}
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/aliasUsageInIndexerOfClass.types
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,9 @@ import Backbone = require("aliasUsageInIndexerOfClass_backbone");

export class VisualizationModel extends Backbone.Model {
>VisualizationModel : VisualizationModel
>Backbone.Model : any
>Backbone.Model : Backbone.Model
>Backbone : typeof Backbone
>Model : Backbone.Model
>Model : typeof Backbone.Model

// interesting stuff here
}
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/aliasUsageInObjectLiteral.types
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,9 @@ import Backbone = require("aliasUsageInObjectLiteral_backbone");

export class VisualizationModel extends Backbone.Model {
>VisualizationModel : VisualizationModel
>Backbone.Model : any
>Backbone.Model : Backbone.Model
>Backbone : typeof Backbone
>Model : Backbone.Model
>Model : typeof Backbone.Model

// interesting stuff here
}
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/aliasUsageInOrExpression.types
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,9 @@ import Backbone = require("aliasUsageInOrExpression_backbone");

export class VisualizationModel extends Backbone.Model {
>VisualizationModel : VisualizationModel
>Backbone.Model : any
>Backbone.Model : Backbone.Model
>Backbone : typeof Backbone
>Model : Backbone.Model
>Model : typeof Backbone.Model

// interesting stuff here
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class C<T extends IHasVisualizationModel> {
}
class D extends C<IHasVisualizationModel> {
>D : D
>C : C<T>
>C : C<IHasVisualizationModel>
>IHasVisualizationModel : IHasVisualizationModel

x = moduleA;
Expand All @@ -46,9 +46,9 @@ import Backbone = require("aliasUsageInTypeArgumentOfExtendsClause_backbone");

export class VisualizationModel extends Backbone.Model {
>VisualizationModel : VisualizationModel
>Backbone.Model : any
>Backbone.Model : Backbone.Model
>Backbone : typeof Backbone
>Model : Backbone.Model
>Model : typeof Backbone.Model

// interesting stuff here
}
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/aliasUsageInVarAssignment.types
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,9 @@ import Backbone = require("aliasUsageInVarAssignment_backbone");

export class VisualizationModel extends Backbone.Model {
>VisualizationModel : VisualizationModel
>Backbone.Model : any
>Backbone.Model : Backbone.Model
>Backbone : typeof Backbone
>Model : Backbone.Model
>Model : typeof Backbone.Model

// interesting stuff here
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ class List<T> {
class DerivedList<U> extends List<U> {
>DerivedList : DerivedList<U>
>U : U
>List : List<T>
>List : List<U>
>U : U

foo: U;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
class C<T1> extends CBase<T1> {
>C : C<T1>
>T1 : T1
>CBase : CBase<T2>
>CBase : CBase<T1>
>T1 : T1

public works() {
Expand Down Expand Up @@ -35,7 +35,7 @@ class C<T1> extends CBase<T1> {
class CBase<T2> extends CBaseBase<Wrapper<T2>> {
>CBase : CBase<T2>
>T2 : T2
>CBaseBase : CBaseBase<T3>
>CBaseBase : CBaseBase<Wrapper<T2>>
>Wrapper : Wrapper<T5>
>T2 : T2

Expand Down
40 changes: 0 additions & 40 deletions tests/baselines/reference/checkForObjectTooStrict.errors.txt

This file was deleted.

42 changes: 42 additions & 0 deletions tests/baselines/reference/checkForObjectTooStrict.symbols
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))

}

}

44 changes: 44 additions & 0 deletions tests/baselines/reference/checkForObjectTooStrict.types
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

}

}

Loading