FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Allowed non-this, non-super code before super call in derived classes with property initializers by JoshuaKGoldberg · Pull Request #29374 · microsoft/TypeScript · GitHub

/ TypeScript Public

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

Merged
Ron Buckton (rbuckton) merged 61 commits into
microsoft:mainfrom
JoshuaKGoldberg:non-this-super-before-super
Jan 14, 2022
ConversationCommits (61)ChecksFiles changed
Merged

Allowed non-this, non-super code before super call in derived classes with property initializers#29374
Ron Buckton (rbuckton) merged 61 commits into
microsoft:mainfrom
JoshuaKGoldberg:non-this-super-before-super

Conversation

Josh Goldberg ✨ (JoshuaKGoldberg) commented Jan 11, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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?

Edit 2/28/2020: 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, and gulp runtests passes locally (on Windows). I'll try a Mac to see if there's some odd encoding/whitespace behavior with the failing test... ✔️

Edit 3/11/2021: It seems this is fairly close to merging.

Edit 1/13/2022: I bought this PR a birthday cake for its third birthday. https://twitter.com/JoshuaKGoldberg/status/1481654056422567944

Allowed non-this, non-super code before super call in derived classes
6845db9 
Fixes microsoft#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`?
Klaus Meinhardt (ajafff) suggested changes Jan 11, 2019
View reviewed changes
Comment thread src/compiler/checker.ts Outdated
Klaus Meinhardt (ajafff) suggested changes Jan 11, 2019
View reviewed changes
Comment thread src/compiler/utilities.ts Outdated
Comment thread src/compiler/utilities.ts Outdated

Wesley Wigham (weswigham) commented Jan 11, 2019

Copy link
Copy Markdown
Member

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.

Klaus Meinhardt (ajafff) reviewed Jan 11, 2019
View reviewed changes
Comment thread src/compiler/utilities.ts Outdated
(node|statement)RefersToSuperOrThis wasn't checking root level scope …
00b4a2b 
…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.
Klaus Meinhardt (ajafff) suggested changes Jan 12, 2019
View reviewed changes

Klaus Meinhardt (ajafff) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread src/compiler/utilities.ts Outdated
Comment thread src/compiler/checker.ts Outdated
Comment thread src/compiler/utilities.ts Outdated
Comment thread src/compiler/utilities.ts Outdated
Comment thread src/compiler/utilities.ts Outdated
Merged
Closed
Josh Goldberg added 3 commits January 15, 2019 14:40
Limited super calls to root-level statements in constructor bodies
ccd52fc 
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.
Merge branch 'master'
da71ed9
Fixed error number following 'master' merge
b4a9ac2
Klaus Meinhardt (ajafff) suggested changes Jan 18, 2019
View reviewed changes
Comment thread src/compiler/utilities.ts Outdated

Ryan Cavanaugh (RyanCavanaugh) commented Jan 25, 2019

Copy link
Copy Markdown
Member

cc Anders Hejlsberg (@ahejlsberg) for review

Max Graey (MaxGraey) mentioned this pull request Jan 30, 2019
Merged
Closed

Josh Goldberg ✨ (JoshuaKGoldberg) commented Apr 2, 2019

Copy link
Copy Markdown
Contributor Author

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

Josh Goldberg ✨ (JoshuaKGoldberg) commented Jul 16, 2019

Copy link
Copy Markdown
Contributor Author

Correction: ping, Wesley Wigham (@weswigham)?

Paulo Koch (pkoch) commented Jan 14, 2022

Copy link
Copy Markdown

Ron Buckton (@rbuckton) Is there anything else that Josh Goldberg ✨ (@JoshuaKGoldberg) can do to move this forward?

Ron Buckton (rbuckton) commented Jan 14, 2022

Copy link
Copy Markdown
Contributor

TypeScript Bot (@typescript-bot) user test this inline

TypeScript Bot (typescript-bot) commented Jan 14, 2022

Copy link
Copy Markdown
Contributor

Heya Ron Buckton (@rbuckton), I'm starting to run the inline community code test suite on this PR at 1b3dd6d. Hold tight - I'll update this comment with the log link once the build has been queued.

Ron Buckton (rbuckton) commented Jan 14, 2022

Copy link
Copy Markdown
Contributor

Ron Buckton (@rbuckton) Is there anything else that Josh Goldberg ✨ (@JoshuaKGoldberg) can do to move this forward?

Nope, it looks ready to go actually, and just in time for 4.6 beta.

Ron Buckton (rbuckton) merged commit b7fee7f into microsoft:main Jan 14, 2022
Josh Goldberg ✨ (JoshuaKGoldberg) deleted the non-this-super-before-super branch January 14, 2022 14:31

Josh Goldberg ✨ (JoshuaKGoldberg) commented Jan 14, 2022

Copy link
Copy Markdown
Contributor Author

Fantastic, thanks so much for the reviews & merge Ron Buckton (@rbuckton)!

If any issues come out of this change I'm available to try to fix, if that's helpful.

Naveed Ahmad Hematmal (NaveedAhmadHematmal) commented Jan 14, 2022

Copy link
Copy Markdown

The longest-living PR I have ever seen. 😳

Luke Scott (lukescott) commented Jan 20, 2022 •
edited
Loading

Copy link
Copy Markdown

YES!!! Thank you!🎉 🎉 🎉

Callum (callumok2004) commented Jan 22, 2022

Copy link
Copy Markdown

🥳🎉🎉

Closed
5 tasks
Kitson Kelly (kitsonk) mentioned this pull request Feb 14, 2022
Merged
This was referenced Mar 3, 2022
Open
Merged

iJungleboy (iJungleboy) commented Mar 7, 2022

Copy link
Copy Markdown

Amazing work and perseverance. Awesome!

anth.dev (EstopaceMA) commented Mar 18, 2022

Copy link
Copy Markdown

Man! Awesome. The perseverance! 🎉

John Reilly (johnnyreilly) mentioned this pull request Apr 1, 2022
Closed
This was referenced Apr 13, 2022
Closed
Merged
P Sz (p-szm) mentioned this pull request Apr 19, 2022
Closed
John Lusty (jlusty) mentioned this pull request Apr 19, 2022
Closed
Agnès Toulet (AgnesToulet) mentioned this pull request Jun 2, 2022
Merged

Vladyslav V (vladboss61) commented Jul 7, 2022

Copy link
Copy Markdown

Does it mean that now super is going to be invoked the first time never mind where he is located in the constructor?

Eemeli Aro (eemeli) added a commit to messageformat/messageformat that referenced this pull request Jul 16, 2022
fix(mf2): Use private member for ResolvedMessage cache, thanks to mic…
0fbf543 
…rosoft/TypeScript#29374

The minimum TypeScript dependency is lifted to 4.6.2

Justin Fortier (jfortier-haptiq) commented Jul 28, 2022

Copy link
Copy Markdown

Congrats, apparently the cake did it's job! 🎉

Guillaume Bersac (gbersac) commented Jul 28, 2022

Copy link
Copy Markdown

Guy you've made it reddit programmer humor front page! https://www.reddit.com/r/ProgrammerHumor/comments/waa0lz/if_youre_ever_frustrated_that_your_github_prs/

taylus commented Jul 28, 2022

Copy link
Copy Markdown

grats on the PR

upq commented Jul 28, 2022

Copy link
Copy Markdown

Congrats !! Happy for you!
Might as well be the most famous PR yet ... from branding perspective I recommend you naming the PR
Suggestion: Des Supa

Microsoft (microsoft) locked as resolved and limited conversation to collaborators Jul 28, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

Wesley Wigham (weswigham) weswigham approved these changes
+2 more reviewers
Klaus Meinhardt (ajafff) ajafff requested changes
Ron Buckton (rbuckton) rbuckton approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

Ron Buckton (rbuckton)

Labels

For Uncommitted Bug PR for untriaged, rejected, closed or missing bug

Projects

PR Backlog
Archived in project

Milestone

TypeScript 3.7.1

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL