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

align ClassStaticBlockDeclaration with IIFE in CFA #44969

Merged
merged 6 commits into from
Mar 23, 2022

Conversation

Zzzen
Copy link
Contributor

@Zzzen Zzzen commented Jul 10, 2021

Fixes #44949
Fixes #45932

@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Jul 10, 2021
!hasSyntacticModifier(node, ModifierFlags.Async) &&
!(node as FunctionLikeDeclaration).asteriskToken &&
!!getImmediatelyInvokedFunctionExpression(node)) ||
node.kind === SyntaxKind.ClassStaticBlockDeclaration;
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 am not sure how to format these lines, so here is how prettier handles it.

Copy link
Member

Choose a reason for hiding this comment

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

I would have done it slightly differently, but prettier's format is fine.

@sandersn sandersn added this to Not started in PR Backlog Jul 15, 2021
PR Backlog automation moved this from Not started to Needs merge Aug 2, 2021
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Looks good but one naming suggestion and it would be nice to have a second opinion on the semantics from @rbuckton

@@ -657,19 +657,23 @@ namespace ts {
const saveExceptionTarget = currentExceptionTarget;
const saveActiveLabelList = activeLabelList;
const saveHasExplicitReturn = hasExplicitReturn;
const isIIFE = containerFlags & ContainerFlags.IsFunctionExpression && !hasSyntacticModifier(node, ModifierFlags.Async) &&
!(node as FunctionLikeDeclaration).asteriskToken && !!getImmediatelyInvokedFunctionExpression(node);
const isIIFELike =
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const isIIFELike =
const isImmediatelyInvoked =

(or isImmediate for even more old-school flavor)

!hasSyntacticModifier(node, ModifierFlags.Async) &&
!(node as FunctionLikeDeclaration).asteriskToken &&
!!getImmediatelyInvokedFunctionExpression(node)) ||
node.kind === SyntaxKind.ClassStaticBlockDeclaration;
Copy link
Member

Choose a reason for hiding this comment

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

I would have done it slightly differently, but prettier's format is fine.

@fwienber
Copy link

Hi! I just left a new test case at the issue which you might want to check! Would be awesome if that case was covered by your fix, too!

@sandersn sandersn moved this from Needs merge to Waiting on reviewers in PR Backlog Feb 18, 2022
@fwienber
Copy link

Anything missing here but the last review? I'd love to retest this in the nightly build as soon as it is merged!

@Zzzen
Copy link
Contributor Author

Zzzen commented Mar 22, 2022

@rbuckton please take a look.

PR Backlog automation moved this from Waiting on reviewers to Needs merge Mar 23, 2022
@rbuckton
Copy link
Member

I'll have the bot run some extensive tests to make sure there's no other side effects from this change.

@rbuckton
Copy link
Member

@typescript-bot run dt
@typescript-bot test this
@typescript-bot user test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 23, 2022

Heya @rbuckton, I've started to run the parallelized Definitely Typed test suite on this PR at ecb3adb. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 23, 2022

Heya @rbuckton, I've started to run the parallelized community code test suite on this PR at ecb3adb. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 23, 2022

Heya @rbuckton, I've started to run the extended test suite on this PR at ecb3adb. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

@rbuckton
Copy link
Member

None of the reported test failures look to be related to this change. I think this is good to merge.

@rbuckton rbuckton merged commit 20c01cd into microsoft:main Mar 23, 2022
PR Backlog automation moved this from Needs merge to Done Mar 23, 2022
@fwienber
Copy link

Sorry to be unfamiliar with your process, but could you tell me how I can see which milestone / release this fix will be in?
It does not seem to be included in the 4.7.0 Milestone (yet?).

@fwienber
Copy link

And also not part of the latest nightly build (if I didn't do something wrong when trying to test with the nightly)...

@Zzzen
Copy link
Contributor Author

Zzzen commented Mar 29, 2022

Did you try 4.7.0-dev.20220329?

@fwienber
Copy link

Oh, my fault. Has arrived in nightly and works perfectly!
Can't wait for the release (I also found the 4.7 release plan in the meantime)!

@fwienber
Copy link

My reproducer is now also fixed in TypeScript playground when choosing "nightly" ☺

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
PR Backlog
  
Done
5 participants