Allowed non-this, non-super code before super call in derived classes with property initializers #29374
Conversation
Fixes #8277. It feels wrong to put a new `forEachChild` loop in the checker, though in the vast majority of user files this will be a very quick one. Is there a better way to check for a reference to `super` or `this`?
|
tests/baselines/reference/derivedClassSuperProperties.errors.txt
Outdated
Show resolved
Hide resolved
…boundaries
```ts
function () {
return this;
}
```
It was immediately going to `ts.forEachChild` so the statement itself wasn't being counted as a new `this` scope.
|
This is getting very complex very fast. I wonder if there's an easier way using the control flow graph? |
tests/baselines/reference/derivedClassSuperProperties.errors.txt
Outdated
Show resolved
Hide resolved
As per discussion in the issue, it would be ideal to consider any block that always ends up calling to super() the equivalent of a root-level super() statement. This would be valid:
```ts
foo = 1;
constructor() {
condition() ? super(1) : super(0);
this.foo;
}
```
...as it would compile to the equivalent of:
```ts
function () {
condition() ? super(1) : super(0);
this.foo = 1;
this.foo;
}
That change would a bit more intense and I'm very timid, so leaving it out of this PR. In the meantime the requirement is that the super() statement must itself be root-level.
|
cc @ahejlsberg for review |
|
Ping @ahejlsberg - is there anything that needs to be done here? It'd be nice to have this in |
|
Correction: ping, @weswigham? |
|
@weswigham I believe I've addressed all the discussions on this PR, and it should be ready for re-review when you have a chance please! |
|
Ping @rbuckton - now that 3.9 is out, I think it'd be lovely to get this into 4.0. Do you know if you'll be able to review this? (please |
|
Was chatting with @ahejlsberg about this about 2 weeks ago - I recall agreeing that it'd be useful for 4.0, short of maybe some style nits. As a related change, he was also looking into ]our btw, one thing I'd recommend is reviewing with whitespace changes off (https://github.com/microsoft/TypeScript/pull/29374/files?w=1) |
| prop = 1; | ||
| constructor() { | ||
| Math.random() | ||
| ? super(1) |
rbuckton
Aug 28, 2020
Member
Perhaps for a future PR, but it would be nice to actually support this. Its technically possible:
// ts
class Derived extends Base {
prop = 1;
constructor() {
Math.random() ? super(1) : super(0);
}
}
// es2015
class Derived extends Base {
constructor() {
// wrap `super` and initialization in an arrow
const _super = (...args) => {
super(...args);
this.prop = 1;
return this;
};
// class body
Math.random() ? _super(1) : _super(0);
}
}
// es5
var Derived = /** @class */ (function (_super) {
__extends(Derived, _super);
function Derived() {
var _this = this;
// wrap `super` and initialization in a function expression
var _super_1 = function() {
_this = _super.apply(_this, arguments) || _this;
_this.prop = 1;
return _this;
};
// class body
Math.random() ? _super_1(1) : _super_1(0);
}
return Derived;
}(Base));| case SyntaxKind.MethodDeclaration: | ||
| case SyntaxKind.GetAccessor: | ||
| case SyntaxKind.SetAccessor: | ||
| // Object properties can have computed names; only method-like bodies start a new scope |
rbuckton
Dec 1, 2020
Member
Do we need to be concerned about parameter initializers?
class C extends Base {
constructor(public x: number, y = () => this.x) {
super();
}
}
JoshuaKGoldberg
Dec 2, 2020
Author
Contributor
I don't believe we do? In the above, the () => this.x is the new scope, which should be covered by SyntaxKind.ArrowFunction.
...nes/reference/derivedClassConstructorWithExplicitReturns01.sourcemap.txt
Outdated
Show resolved
Hide resolved
...elines/reference/emitSuperCallBeforeEmitParameterPropertyDeclaration1.js
Outdated
Show resolved
Hide resolved
tests/baselines/reference/emitSuperCallBeforeEmitPropertyDeclaration1.js
Outdated
Show resolved
Hide resolved
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
tests/baselines/reference/parameterInitializerBeforeDestructuringEmit.js
Outdated
Show resolved
Hide resolved
|
Ping @rbuckton - is there anything else you'd like changed?
cc @DanielRosenwasser; I'm hesitant to mark this as fixing the linked issue, as it only partially does that. Edge case in the tooling maybe? |
Starts on #8277 by allowing the non-
this, non-supercode to be root-level statements in the constructor. This will now be allowed:It feels wrong to put a newforEachChildloop in the checker, though in the vast majority of user files this will be a very quick one. Is there a better way to check for a reference tosuperorthis?Edit 2/28: I've mostly resolved the merge conflicts introduced by both
✔️
#private fields &useDefineForClassFields, but I'm not confident my approach is still a valid one. I'd greatly appreciate it if someone could confirm I'm on the right track!Oh, andgulp runtestspasses locally (on Windows). I'll try a Mac to see if there's some odd encoding/whitespace behavior with the failing test...