Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allowed non-this, non-super code before super call in derived classes with property initializers #29374

Open
wants to merge 26 commits into
base: master
from

Conversation

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Jan 11, 2019

Starts on #8277 by allowing the non-this, non-super code to be root-level statements in the constructor. This will now be allowed:

class Base { }
class Derived extends Base {
    public prop = true;
    constructor(public paramProp = true) {
        console.log("Hello, world!");
        super();
    }
}

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?

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`?
src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/utilities.ts Outdated Show resolved Hide resolved
src/compiler/utilities.ts Outdated Show resolved Hide resolved
@weswigham

This comment has been minimized.

Copy link
Member

weswigham commented Jan 11, 2019

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?

TransformFlags.Super | TransformFlags.ContainsSuper for super at least.

src/compiler/utilities.ts 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.
Copy link
Contributor

ajafff left a comment

This is getting very complex very fast. I wonder if there's an easier way using the control flow graph?

src/compiler/utilities.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/utilities.ts Outdated Show resolved Hide resolved
src/compiler/utilities.ts Outdated Show resolved Hide resolved
src/compiler/utilities.ts 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.
src/compiler/utilities.ts Outdated Show resolved Hide resolved
@RyanCavanaugh

This comment has been minimized.

Copy link
Member

RyanCavanaugh commented Jan 25, 2019

cc @ahejlsberg for review

@JoshuaKGoldberg

This comment has been minimized.

Copy link
Contributor Author

JoshuaKGoldberg commented Apr 2, 2019

Ping @ahejlsberg - is there anything that needs to be done here? It'd be nice to have this in 😄

@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.6.0 milestone Jul 12, 2019
@JoshuaKGoldberg

This comment has been minimized.

Copy link
Contributor Author

JoshuaKGoldberg commented Jul 16, 2019

Correction: ping, @weswigham?

@JoshuaKGoldberg JoshuaKGoldberg changed the title Allowed non-this, non-super code before super call in derived classes Allowed non-this, non-super code before super call in derived classes with property initializers Sep 2, 2019
@JoshuaKGoldberg

This comment has been minimized.

Copy link
Contributor Author

JoshuaKGoldberg commented Feb 15, 2020

Well this is fun: private named instance fields (#30829) did some merge conflict damage here 😅. I'll merge from master best I can now with a gulp baseline-accept to get this cleaned up, attempt to get the tests reasonable, and start pinging folks in earnest this week.

_this = _super.call(this) || this;
return _super.call(this) || this;
Comment on lines -193 to +193

This comment has been minimized.

Copy link
@JoshuaKGoldberg

JoshuaKGoldberg Feb 15, 2020

Author Contributor

Note that this is a behavioral change for when H is used incorrectly as a callable function:

let invalid = H();

...but the new output is a correction, as when H is changed to extend Object, the return does exist.

@@ -42,6 +42,7 @@ var _a;
var x = "p";
var A = /** @class */ (function () {
function A(y) {
this.y = y;

This comment has been minimized.

Copy link
@JoshuaKGoldberg

JoshuaKGoldberg Feb 15, 2020

Author Contributor

😡. Introduced issue thanks to merge conflicts. Will fix...

@JoshuaKGoldberg JoshuaKGoldberg force-pushed the JoshuaKGoldberg:non-this-super-before-super branch 3 times, most recently from c789b9b to 1623935 Feb 16, 2020
@JoshuaKGoldberg JoshuaKGoldberg force-pushed the JoshuaKGoldberg:non-this-super-before-super branch from a23d80f to 9a7bfc6 Feb 16, 2020
@JoshuaKGoldberg JoshuaKGoldberg force-pushed the JoshuaKGoldberg:non-this-super-before-super branch from 9a7bfc6 to 74e6972 Feb 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.