Jsdoc @constructor - in constructor properly infer this as class instance#25980
Conversation
`c.prototype.method = function() { this }` was already supported.
This commit add support to two more kinds relying on the JSDoc
`@constructor` tag. These are:
1. `/** @constructor */ function Example() { this }`
2. `/** @constructor */ var Example = function() { this }`
C3 and C4 `this` was set as `any`, now it is properly showing as the class type.
|
|
|
This PR doesn't fix the following case, which I need some guidance on: /** @constructor */
var Example3 = module.exports = function() {
this.method();
};
Example3.prototype.method = function() {};The I looked everywhere for something that walks up binary ops and combines the symbols but could not find it. EDIT: looks like a special binding for |
sandersn
left a comment
There was a problem hiding this comment.
I don't think we should take this change until we've discussed and agreed on a solution in the originating bug.
| if (classType) { | ||
| return getFlowTypeOfReference(node, classType); | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think this is the correct fix. The type of this should still be any because without an explicit or contextual type for this, you can still assign to any property on it. Instead, the no-implicit-this error should just be suppressed in javascript. The check at the bottom of checkThisExpression should change to if (!type && noImplicitThis && !isInJavascriptFile(node)) { so that JS-based uses of this in constructor functions won't error.
When calling a JS function explicitly tagged with either `@class` or `@constructor` the checker should throw a TS2348 not callable error.
|
@sandersn I've added a test case for the circular initializer example you wanted. Also, I've included the change that will throw There was a baseline change to I'm running through the user contributed test cases now and will update soon. |
sandersn
left a comment
There was a problem hiding this comment.
- Probably should move the code that requires
@class-marked functions to be called with new. - Some odd indentation.
Otherwise looks good.
| // * /** @constructor */ function [name]() { ... } | ||
| // * /** @constructor */ var x = function() { ... } | ||
| else if ((container.kind === SyntaxKind.FunctionExpression || | ||
| container.kind === SyntaxKind.FunctionDeclaration) && |
There was a problem hiding this comment.
indentation looks weird on this line.
| >new Outer.Pos(1, 'x') : Pos | ||
| >Outer.Pos : typeof Pos | ||
| >Outer : { (element: any, config: any): void; Pos(line: any, ch: any): void; } | ||
| >Outer : { (element: any, config: any): void; } |
There was a problem hiding this comment.
You're right, this is a result of the change in resolveAnonymousTypeMembers. And it breaks intelllisense -- you get no suggestions for Outer.. I haven't figured out why though.
| type.callSignatures = getSignaturesOfSymbol(symbol); | ||
| // If the function is explicitly marked with `@class`, then it must be constructed. | ||
| if (isInJavaScriptFile(symbol.valueDeclaration) && getJSDocClassTag(symbol.valueDeclaration)) { | ||
| type.constructSignatures = getSignaturesOfSymbol(symbol); |
There was a problem hiding this comment.
given the way this breaks intellisense, perhaps a better change would be to change resolveCallExpression to not resolve call signatures on functions with @constructor instead of switching them entirely to be construct signatures. Something like (just before the last return of the function):
if (callSignatures.some(sig => isJavaScriptConstructor(sig.declaration) && !!getJSDocClassTag(sig.declaration!))) {
error(node, Diagnostics.Value_of_type_0_is_not_callable_Did_you_mean_to_include_new, typeToString(funcType));
return resolveErrorCall(node);
}Although there's probably a more elegant way to write it. This undoes the questionable baseline changes that happen with the current code.
This undoes the last commit that sought to change how js functions tagged with `@class` were inferred. For some reason, currently unknown, giving those functions construct signatures causes issues in property assignment/member resolution (as seen in the `typeFromPropertyAssignment12` test case). Instead of changing the signature resolution, the error is explicitly generated in `resolveCallExpression` for those functions.
|
@jameskeane Thanks for the suggestion and implementation! |
|
@sandersn thanks! |
c.prototype.method = function() { this }was already supported.This commit add support to two more kinds relying on the JSDoc
@constructortag. These are:/** @constructor */ function Example() { this }/** @constructor */ var Example = function() { this }Fixes #25979