Constructor functions as classes #32944
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The original test passes but I haven't run any other tests yet, so I assume the world is now broken.
Instead of overwriting them
1. Mark @class-tagged functions with Class too. 2. Only gather local type parameters of constructor functions. 3. Remove getJSClassType calls with getDeclaredTypeOfSymbol. 4. Add a couple more failing tests. getDeclaredTypeOfClassOrInterface now needs to understand prototype assignment. That's next, I think.
1. Binder marks prototype assignments as Class now. 2. Checker merges prototype assignments using the same merge code as for functions and their declarations. No more intersections. Many fewer failing tests now.
Even if there are no this-property assignments in them. (Then why are you using a class?).
It's probably not needed because now it's just a conditional call to getDeclaredTypeOfSymbol, and I think most callers already know whether they have a JS constructor function beforehand.
Because all the properties are merged during getDeclaredTypeOfSymbol.
Pretty cool!
sandersn
commented
Aug 16, 2019
| // note:we overwrite links because we just cloned the symbol | ||
| links = symbol as TransientSymbol; |
There was a problem hiding this comment.
the code that's now in mergeJSSymbols is used both here, in getTypeOfFuncClassEnumModule, and in getDeclaredTypeOfClassOrInterface.
| if (node && isBinaryExpression(node)) { | ||
| // prototype assignments get the outer type parameters of their constructor function | ||
| const assignmentKind = getAssignmentDeclarationKind(node); | ||
| if (assignmentKind === AssignmentDeclarationKind.Prototype || assignmentKind === AssignmentDeclarationKind.PrototypeProperty) { |
There was a problem hiding this comment.
Object.defineProperty has a number of other limitations and bugs that make it not worthwhile to support yet.
| @@ -6157,6 +6152,16 @@ namespace ts { | |||
| function getOuterTypeParameters(node: Node, includeThisTypes?: boolean): TypeParameter[] | undefined { | |||
| while (true) { | |||
| node = node.parent; // TODO: GH#18217 Use SourceFile kind check instead | |||
| if (node && isBinaryExpression(node)) { | |||
There was a problem hiding this comment.
prototype assignments jump from the assignment location to the location of the constructor function and continues walking up the tree looking for type parameters. It does not fork and continue looking from the site of the assignment.
In a hilarious tweet, I provided some code where this difference could be observed, but it was clearly insane. I think this is a good solution, because it makes prototype assignments act as if they are effectively nested inside the constructor function, in the same way that methods are nested inside classes in modern JS.
| @@ -6244,7 +6250,7 @@ namespace ts { | |||
| const constraint = getBaseConstraintOfType(type); | |||
| return !!constraint && isValidBaseType(constraint) && isMixinConstructorType(constraint); | |||
| } | |||
| return isJSConstructorType(type); | |||
There was a problem hiding this comment.
The first case of this function now covers constructor functions.
| @@ -7504,12 +7512,18 @@ namespace ts { | |||
| // will never be observed because a qualified name can't reference signatures. | |||
| if (symbol.flags & (SymbolFlags.Function | SymbolFlags.Method)) { | |||
| type.callSignatures = getSignaturesOfSymbol(symbol); | |||
| type.constructSignatures = filter(type.callSignatures, sig => isJSConstructor(sig.declaration)); | |||
There was a problem hiding this comment.
Construct signatures are now constructed just below here, for symbols marked Class | Function only, instead of running this on all functions.
| @@ -8744,7 +8758,6 @@ namespace ts { | |||
| let type = signature.target ? instantiateType(getReturnTypeOfSignature(signature.target), signature.mapper!) : | |||
| signature.unionSignatures ? getUnionType(map(signature.unionSignatures, getReturnTypeOfSignature), UnionReduction.Subtype) : | |||
| getReturnTypeFromAnnotation(signature.declaration!) || | |||
| isJSConstructor(signature.declaration) && getJSClassType(getSymbolOfNode(signature.declaration!)) || | |||
There was a problem hiding this comment.
this special case also moves into the Class|Function case of resolveAnonymousTypeMembers, where the correct answer is now just classType.
Note that call signatures of constructor functions now correctly fall through to getReturnTypeFromBody, where before they also claimed to return the class type from getJSClassType.
| return getDeclaredTypeOfClassOrInterface(getSymbolOfNode(parent as ClassLikeDeclaration | InterfaceDeclaration)).thisType!; | ||
| } | ||
| } | ||
|
|
||
| // inside x.prototype = { ... } |
There was a problem hiding this comment.
getThisType and checkThisExpression should use the same code to look up the class' thisType, but do not. In particular, checkThisExpression's code, while more complete, is less efficient than this code since it uses checkExpressionCached instead of just grabbing symbol.parent. Since it's a separate change, I'll fix it in a followup PR.
| const targetReturnType = isResolvingReturnTypeOfSignature(target) ? anyType : (target.declaration && isJSConstructor(target.declaration)) ? | ||
| getJSClassType(target.declaration.symbol)! : getReturnTypeOfSignature(target); | ||
| const targetReturnType = isResolvingReturnTypeOfSignature(target) ? anyType | ||
| : target.declaration && isJSConstructor(target.declaration) ? getDeclaredTypeOfClassOrInterface(target.declaration.symbol) |
There was a problem hiding this comment.
this allows a constructor function to be assignable BOTH to new (x: number) => C and (x: number) => void. I left it in because it seems ... useful for JS, even if it's not particularly safe.
| @@ -22826,39 +22852,35 @@ namespace ts { | |||
|
|
|||
| // If the symbol of the node has members, treat it like a constructor. | |||
| const symbol = getSymbolOfNode(func); | |||
| return !!symbol && (symbol.members !== undefined || symbol.exports !== undefined && symbol.exports.get("prototype" as __String) !== undefined); | |||
There was a problem hiding this comment.
this diff sucks! isJSConstructor is now simpler -- it no longer has to check symbol.exports.get("prototype") since the symbols are merged. And isJSConstructorType is deleted.
sandersn
requested review from
weswigham,
andrewbranch,
orta and
PranavSenthilnathan
|
@andrewbranch @orta @PranavSenthilnathan I thought you might be interested in this too. |
|
@typescript-bot perf test this |
|
Heya @sandersn, I've started to run the perf test suite on this PR at b2e72cf. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
@sandersn Here they are:Comparison Report - master..32944
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
weswigham
reviewed
Aug 16, 2019
src/compiler/binder.ts
Outdated
| @@ -2558,11 +2571,18 @@ namespace ts { | |||
| node.left.parent = node; | |||
| node.right.parent = node; | |||
| const lhs = node.left as PropertyAccessEntityNameExpression; | |||
| bindPropertyAssignment(lhs.expression, lhs, /*isPrototypeProperty*/ false); | |||
| const constructorSymbol = lookupSymbolForPropertyAccess(lhs.expression); | |||
| if (constructorSymbol) { | |||
There was a problem hiding this comment.
I think this binding should be unconditional, so a merge cross-file can occur. Eg,
// file1.js
function C() {
this.a = 2;
}
// file2.js
C.prototype.foo = function() { return this.a; };There was a problem hiding this comment.
This is pretty complicated because of things like
Outer.Inner = function() { this.a = 1 }
Outer.Inner.prototype = { ... }Probably needs an iterated declareSymbol just like declarePossiblyMissingNamespace.
There was a problem hiding this comment.
Yep, that sounds about right.
weswigham
reviewed
Aug 16, 2019
src/compiler/binder.ts
Outdated
| } | ||
|
|
||
| function bindObjectDefinePrototypeProperty(node: BindableObjectDefinePropertyCall) { | ||
| const namespaceSymbol = lookupSymbolForPropertyAccess((node.arguments[0] as PropertyAccessExpression).expression as EntityNameExpression); | ||
| if (namespaceSymbol) { | ||
| addDeclarationToSymbol(namespaceSymbol, namespaceSymbol.valueDeclaration, SymbolFlags.Class); |
There was a problem hiding this comment.
As I originally suspected, bindPotentiallyMissingNamespaces overlaps a lot with lookupSymbolForPropertyAccess+addDeclarationToSymbol, so the right thing is to put it there. Couple of caveats:
- bindObjectDefinePrototypeProperty doesn't call bindPropertyAssignment, so cross-file merges don't work for it currently anyway. I want to merge this PR soon, so I filed JS: Object.defineProperty "undefined namespaces" don't merge cross-file #32979 to track adding that (and testing it, which is the bigger task.)
- The symbol binding is still technically conditional, but works in all the cases that cross-file binding does today.
- I'm still working on making it elegant, which is why I didn't do it in the first place. Currently I'm passing two booleans, isPrototypeProperty and isClass, which Seems Bad. I'll push a commit after I figure out whether an enum can work well here.
sandersn
commented
Aug 16, 2019
src/compiler/binder.ts
Outdated
| @@ -227,7 +227,8 @@ namespace ts { | |||
| symbol.flags |= symbolFlags; | |||
|
|
|||
| node.symbol = symbol; | |||
| symbol.declarations = append(symbol.declarations, node); | |||
| // TODO: This is probably too slow to run on every call | |||
There was a problem hiding this comment.
perf numbers seem fine, so I should remove this TODO
weswigham
reviewed
Aug 16, 2019
| let inferred: Type | undefined; | ||
| if (isJSConstructor(symbol.valueDeclaration)) { | ||
| inferred = getInferredClassType(symbol); | ||
| function mergeJSSymbols(target: Symbol, source: Symbol | undefined) { |
There was a problem hiding this comment.
If mergeJSSymbols is called twice with the same pair of symbols, you'll get a new output symbol - I think the result of the merge needs to be stored, eg, on the symbol links of the target.
There was a problem hiding this comment.
It's not future-proof, but we both looked at the callers of mergeJSSymbols and they both cache immediately after calling it.
Also remove old TODOs
weswigham
approved these changes
Aug 19, 2019
timsuchanek
pushed a commit
to timsuchanek/TypeScript
that referenced
this pull request
Sep 11, 2019
* Initial implementation The original test passes but I haven't run any other tests yet, so I assume the world is now broken. * Append constructor function construct sigs Instead of overwriting them * Grab bag of improvements. 1. Mark @class-tagged functions with Class too. 2. Only gather local type parameters of constructor functions. 3. Remove getJSClassType calls with getDeclaredTypeOfSymbol. 4. Add a couple more failing tests. getDeclaredTypeOfClassOrInterface now needs to understand prototype assignment. That's next, I think. * Prototype assignments work now 1. Binder marks prototype assignments as Class now. 2. Checker merges prototype assignments using the same merge code as for functions and their declarations. No more intersections. Many fewer failing tests now. * Mark prototype-property assignments as Class Even if there are no this-property assignments in them. (Then why are you using a class?). * Simplify getJSClassType, remove calls to its guts It's probably not needed because now it's just a conditional call to getDeclaredTypeOfSymbol, and I think most callers already know whether they have a JS constructor function beforehand. * isJSDocConstructor doesn't need to check prototype anymore Because all the properties are merged during getDeclaredTypeOfSymbol. * outer type parameter lookup follow prototype assignment * this-type and -expression support in ctor funcs Pretty cool! * Fix remaining tests * Fix minor lint * Delete now-unused code * Add class flag to nested class declarations Also remove old TODOs
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In JS, constructor functions are now classes. That means:
thistypes work.And it only adds about 40 lines of code!
Fixes #26833
Fixes #23007