Type this in more constructor functions
#39447
Conversation
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.
src/compiler/checker.ts
Outdated
| getJSDocClassTag(container)) { | ||
| const classType = (getDeclaredTypeOfSymbol(getMergedSymbol(container.symbol)) as InterfaceType).thisType!; | ||
| return getFlowTypeOfReference(node, classType); | ||
| else if (isJSConstructor(container)) { |
There was a problem hiding this comment.
this is the real change of the PR; the rest just dedupes getFlowTypeOfReference calls to the bottom of the function.
|
woops, missed some fourslash tests since those don't get baseline updated. |
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.
|
The fourslash tests point out a codefix that doesn't apply anymore; with this change I'm not sure if this contradicts the original spirit of #25979, in which we decided that It's possible to get back the original (Edit: Or the effort of writing a new error message.) |
|
Now master is broken with some lint. I'll merge it in again once it's fixed. |
|
@rachelgshaffer you might be interested in this as well, especially now that it changes a codefix. |
|
In fact, it looks like I imagined requesting reviewers at all, because the reviewer list is broken. @andrewbranch @elibarzilay can you take a look? |
|
Since it only affects JS files, for non-checkJS compiles, nobody will notice anything. For checkJS compiles, which are a lot rarer, I think the improvement is worth it. Also, the However, it's a good idea to check this on the user tests... @typescript-bot user test this |
Ah, I didn’t realize that. That makes it seem like a smaller change then. |
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
Briefly: yes, it is pretty breaky. I'm going to see what the breaks look like before merging. |
|
acorn: acorn's distributed code uses a weird pattern that aliases the prototype like so let pp$1 = Parser.prototype
pp$1.method = function() { ... }Then any method with a this-property assignment, which is most of them, treats the method as a fresh constructor function instead. It's fine that we don't understand that pattern, but this problem will happen anywhere that we misunderstand a method as a constructor function. bluebird: manual calling of a function with function succeed() {
return finallyHandler.call(this, this.promise._target()._settledValue());
}
function finallyHandler(reasonOrValue) {
var promise = this.promise;
var handler = this.handler;
....chrome-devtools-frontend: BUG: aliases of this don't register as this-property assignments, and this now causes lots of error if you have at least one non-aliased this-property assignment: function Display(place, doc, input) {
var d = this;
this.input = input; // ok
d.scrollbarFiller = elt("div", null, "CodeMirror-scrollbar-filler"); // error!
...puppeteer: side note: puppeteer's user test is broken because of a master → main switch. |
|
The /**
* @class
* @this {{ e: 1, m: 1 }}
*/
function C() {
this.e = this.m + 1
}I think it's pretty easy to fix, though. |
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-bot user test this |
|
The follow-redirects: the inheritance pattern Minimatch.prototype.make = make
function make() { ... }npmlog: just a few more errors in instance methods that we mistakenly think are constructor functions. We don't bind instance methods, but these functions now have errors as well. uglify-js: when your OO metaprogramming has functions named |
|
So, to summarise: There are lots of new errors, mostly in patterns we already didn't understand, or attempt to understand. Before, I think this is fine because the typical case of |
* upstream/master: (75 commits) Insert auto-imports in sorted order (microsoft#39394) LEGO: check in for master to temporary branch. Better checking of @param/@Property tags (microsoft#39487) fix(25155): add space before optional parameters/properties (microsoft#38798) Add regression test for microsoft#38834 (microsoft#39479) Fixes searches for symbols exported using export * as (microsoft#39507) fix(39421): omit prefix text for rest binding element (microsoft#39433) fix(39440): show QF for abstract classes with methods which include 'this' parameter (microsoft#39465) Remove unnecessary assert (microsoft#39483) LEGO: check in for master to temporary branch. Update user baselines (microsoft#39220) Type `this` in more constructor functions (microsoft#39447) LEGO: check in for master to temporary branch. LEGO: check in for master to temporary branch. Properly handle rest parameters in function declarations with @type annotations (microsoft#39473) Ensure type/namespaceish statics are included in the list of namespace merge members (microsoft#38920) Fix getTypeAtLocation for dotted implements clauses (microsoft#39363) Add workflow_dispatch to our nightly publish script. (microsoft#39485) Fix crash in decorator metadata calculation when serializing template literal type nodes (microsoft#39481) Fix test semantic merge conflict between microsoft#39348 and microsoft#39130 (microsoft#39478) ... # Conflicts: # src/compiler/scanner.ts
Previously,
this: thisin constructor functions only when there was an explicit@constructortag on the function. Now,this: thisfor 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: thisinside 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@constructortag.@readonlyassignment; I had to fix the readonly detection code to useisJSConstructoras well.@classand@thisin a jsdoc would cause the@thisannotation to be ignored. This became a worse problem with this PR, becausethisis correctly typed even without the annotation. This PR now makes sure that@thisis checked first and used if present.Fixes #18171