Skip to content

Conversation

AlekseyTs
Copy link
Contributor

No description provided.

@AlekseyTs AlekseyTs requested a review from a team as a code owner April 23, 2025 13:48
@ghost ghost added the untriaged Issues and PRs which have not yet been triaged by a lead label Apr 23, 2025
@AlekseyTs
Copy link
Contributor Author

@dotnet/roslyn-compiler Please review

BoundExpression enumeratorVarInitValue = SynthesizeCall(getEnumeratorInfo, forEachSyntax, receiver,
allowExtensionAndOptionalParameters: isAsync || getEnumeratorInfo.Method.IsExtensionMethod || getEnumeratorInfo.Method.GetIsNewExtensionMember(), firstRewrittenArgument: firstRewrittenArgument);
BoundExpression enumeratorVarInitValue = MakeCall(getEnumeratorInfo, forEachSyntax, receiver,
firstRewrittenArgument: firstRewrittenArgument);
Copy link
Member

@jjonescz jjonescz Apr 24, 2025

Choose a reason for hiding this comment

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

If I understand correctly, previously we asserted that there are no arguments if allowExtensionAndOptionalParameters is false and proceeded to emit a call without arguments, now that assert is gone and in Release builds we proceed to emit a call with arguments. Is it worth preserving the assert at least? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it worth preserving the assert at least?

The assert made sense on a code path that wouldn't do the right thing in presence of arguments. i.e. the arguments would be dropped, That code path is gone, now the arguments will never be dropped, IL comparison tests ensure the generated code is what we expect.

@AlekseyTs
Copy link
Contributor Author

@dotnet/roslyn-compiler For the second review

@AlekseyTs AlekseyTs requested a review from a team April 24, 2025 18:39
@AlekseyTs
Copy link
Contributor Author

@dotnet/roslyn-compiler For the second review

1 similar comment
@AlekseyTs
Copy link
Contributor Author

@dotnet/roslyn-compiler For the second review

@AlekseyTs AlekseyTs merged commit b01529a into dotnet:main Apr 28, 2025
24 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Apr 28, 2025
@RikkiGibson RikkiGibson modified the milestones: Next, 18.0 P1 Aug 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants