-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Allow expressions in class extends clauses #3516
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
Changes from 7 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 |
---|---|---|
|
@@ -426,7 +426,7 @@ module 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 @@ module 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 @@ module 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 @@ module 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; | ||
|
@@ -1878,6 +1879,12 @@ module 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 |
---|---|---|
|
@@ -25,7 +25,7 @@ class C<T extends IHasVisualizationModel> { | |
} | ||
class D extends C<IHasVisualizationModel> { | ||
>D : D | ||
>C : C<T> | ||
>C : typeof C | ||
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 best way to handle all these changes is to tweak typeWriter to produce results like what we used to get. Then, we can actually review the handful of files that have relevant changes. Then, once we can see that nothing else changed unintentionally, we can remove that tweak in a followup change and then update all the baselines. With several hundred files changed, github won't show all the changes. And it becomes very difficult to ascertain if something else might have broke as part of this. We've done this numerous times as we've changed hte compiler. For example, even when moving over from the old compiler to the new one, we tweaked things in the new typewriter to produce the same output as hte previous one. 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've added a workaround to reduce the number of changes, but it isn't possible to completely eliminate the differences. Should be a lot better now, though. |
||
>IHasVisualizationModel : IHasVisualizationModel | ||
|
||
x = moduleA; | ||
|
@@ -46,9 +46,9 @@ import Backbone = require("aliasUsageInTypeArgumentOfExtendsClause_backbone"); | |
|
||
export class VisualizationModel extends Backbone.Model { | ||
>VisualizationModel : VisualizationModel | ||
>Backbone.Model : any | ||
>Backbone.Model : typeof Backbone.Model | ||
>Backbone : typeof Backbone | ||
>Model : Backbone.Model | ||
>Model : typeof Backbone.Model | ||
|
||
// interesting stuff here | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,5 +4,5 @@ declare class A { } | |
|
||
declare class B extends A { } | ||
>B : B | ||
>A : A | ||
>A : typeof A | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,5 +7,5 @@ declare module foo { | |
|
||
class B extends A { } | ||
>B : B | ||
>A : A | ||
>A : typeof A | ||
} |
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.