Skip to content

Fixed multiple prologue directives with parameter properties #48687

New issue

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

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

Already on GitHub? # to your account

Conversation

JoshuaKGoldberg
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg commented Apr 13, 2022

Two issues:

  • Parameter properties should be inserted after any prologue directives
  • Prologue directives were getting copied over twice when there wasn't a super()

Fixes #48671

@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Apr 13, 2022
@@ -35405,7 +35405,7 @@ namespace ts {
superCallStatement = statement;
break;
}
if (!isPrologueDirective(statement) && nodeImmediatelyReferencesSuperOrThis(statement)) {
if (nodeImmediatelyReferencesSuperOrThis(statement)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change isn't necessary for the PR; I just had only noticed the check is unnecessary after #29374 was merged 😄

const indexAfterLastPrologueStatement = factory.copyPrologue(body.statements, statements, /*ensureUseStrict*/ false, visitor);
const superStatementIndex = findSuperStatementIndex(body.statements, indexAfterLastPrologueStatement);
const prologueStatementCount = factory.copyPrologue(body.statements, statements, /*ensureUseStrict*/ false, visitor);
const superStatementIndex = findSuperStatementIndex(body.statements, prologueStatementCount);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm nitpicking the variable name here. Both variable names are correct; prologueStatementCount just makes more sense to me -- especially in the context of the new array spread below.

@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as ready for review April 13, 2022 23:44
@jakebailey jakebailey merged commit fa2515e into microsoft:main Apr 14, 2022
@jakebailey
Copy link
Member

Thanks for fixing this!

@jakebailey
Copy link
Member

@typescript-bot cherry-pick this to release-4.6

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 20, 2022

Heya @jakebailey, I've started to run the task to cherry-pick this into release-4.6 on this PR at 76f482f. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

Hey @jakebailey, I couldn't open a PR with the cherry-pick. (You can check the log here). You may need to squash and pick this PR into release-4.6 manually.

jakebailey pushed a commit to jakebailey/TypeScript that referenced this pull request Apr 20, 2022
@jakebailey
Copy link
Member

Sent #48782 manually. This PR is needed for the other PRs thanks to this PR affecting the base ts.ts transform.

Jack-Works pushed a commit to Jack-Works/TypeScript that referenced this pull request Apr 22, 2022
DanielRosenwasser pushed a commit that referenced this pull request May 3, 2022
…#48782)

Co-authored-by: Josh Goldberg <me@joshuakgoldberg.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No-op string in constructor is repeated and reordered when constructor assignment is used
3 participants