Correctly track thisContainer for this-property-assignments in JS nested containers #22779
Conversation
Previously it would update on every block, not just those that could bind `this`. In addition, if the constructor symbol still can't be found, then no binding happens. This is usually OK because people don't add new properties in methods too often.
| // Extend that DOM! | ||
| Event.prototype.removeChildren = function () { | ||
| ~~~~~ | ||
| !!! error TS2304: Cannot find name 'Event'. |
There was a problem hiding this comment.
Maybe add @lib: dom to this test?
src/compiler/binder.ts
Outdated
| @@ -479,11 +479,9 @@ namespace ts { | |||
| // and block-container. Then after we pop out of processing the children, we restore | |||
| // these saved values. | |||
| const saveContainer = container; | |||
| const saveContainerContainer = containerContainer; | |||
| const saveContainerContainer = thisParentContainer; | |||
| // @noEmit: true | ||
| // @Filename: bluebird.js | ||
|
|
||
| !function outer (f) { return f }( |
There was a problem hiding this comment.
Could this be made simpler? Why are we applying identity and then negating it?
There was a problem hiding this comment.
(I'm doing some digging to find out why all these pieces appear to be required.)
There was a problem hiding this comment.
Here's a simpler repro that fails on batch compilation:
function Async() {
this._trampolineEnabled = true;
}
Async.prototype.disableTrampolineIfNecessary = function dtin(b) {
if (b) {
this._trampolineEnabled = false;
}
};I was testing on the language service, and I can't get this repro to crash consistently like I can the more complex one -- you just have to request diagnostics for that one.
| this._trampolineEnabled = false; | ||
| } | ||
| }; | ||
| var a = new Async() |
There was a problem hiding this comment.
Are these 3 lines necessary to the test?
There was a problem hiding this comment.
No, this really only checks (1) lack of crashes and (2) symbol correctness. Removed.
| // ArrowFunction isn't treated as a this-container | ||
| (args) => { | ||
| this.args = args | ||
| this.newProperty = 1 |
There was a problem hiding this comment.
Include a test that new Installer().newProperty is number | undefined?
Also, fix indents
There was a problem hiding this comment.
Hm. Turns out newProperty: number | undefined but args: number even when it's initialized with this.args = 0. I'll have to find out why.
There was a problem hiding this comment.
Ah. The assignment in loadArgMetadata acts as a second declaration, so we union the types together. This is not ideal — we shouldn't redeclare properties that we know are in the constructor — but I'll fix it separately.
There was a problem hiding this comment.
Opened #22792 to track this; there are couple of things to consider before changing this.
| // @checkJs: true | ||
| // @noEmit: true | ||
| // @Filename: npm-install.js | ||
| function Installer (where, dryrun, args, opts) { |
There was a problem hiding this comment.
Would be simpler to just write this.args = 0 and take no parameters?
|
@sandersn please port to release-2.8 as well. |
|
@mhegazy @andy-ms The break introduced in #22643 is not in 2.8, so this fix does not apply to 2.8. To port to 2.8, I'd have to port #22643 too, and it's a complex fix to a subtle problem. (The problem is that there are too many symbols created from this-assignments, which breaks rename and find-all-refs.) |
src/compiler/binder.ts
Outdated
| @@ -479,7 +479,7 @@ namespace ts { | |||
| // and block-container. Then after we pop out of processing the children, we restore | |||
| // these saved values. | |||
| const saveContainer = container; | |||
| const saveContainerContainer = thisParentContainer; | |||
| const saveThisContainer = thisParentContainer; | |||
There was a problem hiding this comment.
Nit: saveThisParentContainer, since the thisContainer would just be container usually.
|
thanks for the clarification. if the break is not in 2.8 we do not need to port anything. |
Previously thisContainer (previously called containerContainer) would update on every block, not just those that could bind
this.In addition, if the constructor symbol still can't be found, then no binding happens. This is usually OK because people don't add new properties in methods too often.
Fixes #22774