-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Conversation
Treat class extends clause as expression position in services.ts
@@ -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 comment
The 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 comment
The 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.
Conflicts: src/compiler/checker.ts tests/baselines/reference/strictModeReservedWordInClassDeclaration.errors.txt
@mhegazy @JsonFreeman @CyrusNajmabadi I'd like to get this one in pretty soon. I will be putting up a separate CR with the change to |
return signatures; | ||
} | ||
|
||
// The base constructor of a class can resolve 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.
Can/should this be a JSDoc-style comment?
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.
Agreed.
I can take a look later today, or tonight. |
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 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?
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.
In other words, why remove InterfaceWithBaseTypes?
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, 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.
if (!(staticBaseType.symbol && staticBaseType.symbol.flags & SymbolFlags.Class)) { | ||
// When the static base type is a "class-like" constructor function (but not actually a class), we verify | ||
// that all instantiated base constructor signatures return the same type. | ||
let constructors = getInstantiatedConstructorsForTypeArguments(staticBaseType, baseTypeNode.typeArguments); |
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.
This was just computed above on line 10591. Is it possible to reuse it?
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.
Well, here we're looking at the instantiated constructors. We could potentially cache the uninstantiated constructors, but I don't think it is worth the effort.
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.
Oh true, I missed that.
👍 |
Allow expressions in class extends clauses
Hey there! Is this still a thing? I just tried to extend a Map getting the "calling constructor without new is forbidden" error! Here is a fiddle. Or did i miss something in the many discussions? Thanks in advance. |
@Andy1605 this is a bug in your runtime; see https://bugs.chromium.org/p/v8/issues/detail?id=3101 and https://bugs.chromium.org/p/v8/issues/detail?id=3622 and google/traceur-compiler#1413 (comment) |
Tested it with chrome 50.0.2661.66 beta-m (64-bit), Cyberfox 45.0.2 (64-bit) and with Opera 36.0.2130.46 (32 bit - Chrome engine, bad example), Edge 25.10586.0.0. Same result. |
Is there a plausible workaround for this problem on TypeScript end? This is game changing for us! |
@Andy1605 what is the proposed output that would make something like this work without engine support? |
@mhegazy Hmm... would it make sense, if there would be a possibility to suppress the "super() call must be there" error? Then one could at least try to write a workaround. Smth like creating an instance of the subclassed type and merging it back. I am lost here as well. |
@RyanCavanaugh that chromium issue seems fixed now (https://bugs.chromium.org/p/v8/issues/detail?id=3101) Both in Chrome and Chrome canary I can do the following in the chrome console without errors:
However when I execute the compiled typescript output I get a similar error as @Andy1605:
|
This PR allows the
extends
clause of a class to specify an arbitrary expression that computes a constructor function. The PR also relaxes the strict requirement thatextends
specify a class type and instead allows expressions of "class-like" constructor function types. This means that built-in types can now be extended in class declarations.The
extends
clause of a class previously required a type reference to be specified. It now accepts an expression optionally followed by a type argument list. The type of the expression must be a constructor function type with at least one construct signature that has the same number of type parameters as the number of type arguments specified in theextends
clause. The return type of the matching construct signature(s) is the base type from which the class instance type inherits. Effectively, this allows both real classes and "class-like" expressions to be specified in theextends
clause.Some examples:
Fixes #511.