In JS, noImplicitThis puts an error on this inside a constructor function
#25979
Comments
sandersn
changed the title
this not properly inferred in @constructor functions.this inside a constructor function
|
Currently this is working as intended. I think we should keep it that way until we find out more about other scenarios we might have missed, and how popular these scenarios are. The intent of I don't expect many pure JS projects to turn on Maybe we're missing some scenario, though. Do you have a mixed TS/JS codebase? If it's a mixed codebase, is it mostly JS? Are you turning on [1] Note that both JS and TS correctly infer the type of var Example2 = function() { this.j = 2 }
var e2 = new Example2()
e2.j // <-- e2.j: numberDespite knowing the type of |
sandersn
added
the
Awaiting More Feedback
|
I'm trying to switch to tsc from closure-compiler for a large jsdoc typed codebase. An example of a large codebase in this style is the closure-library. [I'm trying to do this for a couple reasons, but the biggest is that closure-compiler does not have string literal types and can not specialize the return type of
Yes, and if you tag a function with the jsdoc Maybe a better example to consider: /** @constructor */
function Example() {
// this _should_ throw an error, since Example#method takes a string but doesn't because `this` is any.
this.method(1);
}
/**
* @param {string} a
*/
Example.prototype.method = function(a) {
// tsc correctly infers `this` to `Example`.
};With my PR (#25980), this will correctly throw a Could you clarify what you mean by "contextual type"? |
|
To be clear I'm only talking about constructor functions that are tagged with the jsdoc class tag. If not tagged, I agree that |
|
Thanks. I see the value in typing After some discussion with @mhegazy we decided that // object literal "singleton" with methods
var o = {
state: [1,2,3]
}
o.method = function (x) {
this.state.push(x) // this.state should be visible here
}
o.method(4)
// exports with nested class
var ok = {}
ok.top = function () { }
ok.nestedClass = function (name) {
this.name = name
// this.top shouldn't be visible here!
}
new ok.nestedClass()
module.exports = ok
// class with nested class
function f() {
this.state = [1,2,3]
}
f.nestedClass = function (name) {
// this.state shouldn't be visible
this.name = name // this.name should be a fresh property
}
module.exports = f
//
f.staticMethod = function (name) {
// this.state shouldn't be visible
this.name = name // this.name should be the Function.name of `f`
}
new f.nestedClass() // fine!
f.staticMethod() // `this.name = name` tries to overwrite the name of `f` and fails silentlyWe could try to come up with heuristics to distinguish these 4 cases, but it would be better to require I would like to see tests of three things:
/** @class */
function C() {
if (!(this instanceof C)) {
return new C()
}
this.x = 1
}(I think this is the pattern, but I just wrote it off the top of my head, so it could easily be wrong.)
/** @class */
function C() {
// see the explanation of contextual typing below.
this.functions = [x => x, x => x + 1, x => x - 1]
}The compiler might have trouble figuring out the type of
Re: contextual typing Example.prototype.method = function (a) { this.x }
|
|
@sandersn should we continue discussion in PR #25980? Re:
|
|
Would we need to throw a TS2348 ( Is this what you meant when you said in [1]: "These don't necessarily need to work when marked with @constructor"? Otherwise this would type-check fine, but error at runtime: /** @constructor */
function Example5() {
this.method();
}
Example5.prototype.method = function() {}
Example5(); |
|
Well, what I meant was that it’s acceptable to have type errors inside the callable-constructor function when tagged with |
mhegazy
added
Suggestion
|
@jameskeane and @sandersn we will need to upodate the docs in https://github.com/Microsoft/TypeScript/wiki/Type-Checking-JavaScript-Files |
|
I think the correct update is JSDoc-support-in-Javascript, since that enumerates our tag support. In fact, it currently uses I'll write up a description with the new semantics and send out a PR. |
|
OK, the handbook is updated, and I also updated the wiki until we can publish a new version of the handbook. Afterwards I'll remove the text from the wiki and point to the handbook. |
Previously, `this: this` in constructor functions only when there was an explicit `@constructor` tag on the function. Now, `this: this` for any function that's known to be a constructor function. This improves completions inside constructor functions; also note that previously the compiler *did* type `this: this` inside methods of constructor functions, so this fix makes us more consistent. This is reflected in the large number of baselines that improve. The fix is a simple switch to `isJSConstructor`, which is the standard way to detect constructor functions. I'm not sure why the original PR didn't use this method. I remember discussing this limitation in the original bug, #25979, and I guess I decided that it made sense. But I was heavily primed by the bug's framing of the problem in terms of `noImplicitThis`, which *should* require an explicit `@constructor` tag. With better typing comes better detection of `@readonly` assignment; I had to fix the readonly detection code to use `isJSConstructor` as well.
* Type `this` in more constructor functions Previously, `this: this` in constructor functions only when there was an explicit `@constructor` tag on the function. Now, `this: this` for any function that's known to be a constructor function. This improves completions inside constructor functions; also note that previously the compiler *did* type `this: this` inside methods of constructor functions, so this fix makes us more consistent. This is reflected in the large number of baselines that improve. The fix is a simple switch to `isJSConstructor`, which is the standard way to detect constructor functions. I'm not sure why the original PR didn't use this method. I remember discussing this limitation in the original bug, #25979, and I guess I decided that it made sense. But I was heavily primed by the bug's framing of the problem in terms of `noImplicitThis`, which *should* require an explicit `@constructor` tag. With better typing comes better detection of `@readonly` assignment; I had to fix the readonly detection code to use `isJSConstructor` as well. * Remove `Add @Class tag` fix for noImplicitThis. The new rules mean that it never applies. It's possible that it should apply to functions like ```js function f() { this.init() } ``` In which `init` is never defined, but I think this program is incomplete enough that not offering the fix is fine. * Fix precedence of `@this` Previously, both `@class` and `@this` in a jsdoc would cause the `@this` annotation to be ignored. This became a worse problem with this PR, because `this` is correctly typed even without the annotation. This commit makes sure that `@this` is checked first and used if present.
TypeScript Version: 30c4149
Search Terms:
jsdoc
@constructorthisCode
Compiled with
--allowJs --checkJs --noImplicitThisExpected behavior:
Compiles fine.
Actual behavior:
Playground Link:
https://www.typescriptlang.org/play/#src=%2F**%20%40constructor%20*%2F%20function%20Example()%20%7B%20this.m%20%3D%201%3B%20%7D%0D%0A%2F**%20%40constructor%20*%2F%20var%20Example2%20%3D%20function()%20%7B%20this.j%20%3D%202%3B%20%7D%3B%0D%0A
The text was updated successfully, but these errors were encountered: