Skip to content

Refactor/cleanup usages of MakeCallWithNoExplicitArgument helper #78273

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 28, 2025
Merged

Refactor/cleanup usages of MakeCallWithNoExplicitArgument helper #78273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 5 additions & 19 deletions src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_ForEachStatement.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,8 +180,8 @@ private BoundStatement RewriteForEachEnumerator(
}

// ((C)(x)).GetEnumerator(); OR (x).GetEnumerator(); OR async variants (which fill-in arguments for optional parameters)
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 •
edited by AlekseyTs
Loading

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.


// E e = ((C)(x)).GetEnumerator();
BoundStatement enumeratorVarDecl = MakeLocalDeclaration(forEachSyntax, enumeratorVar, enumeratorVarInitValue);
Expand Down Expand Up @@ -214,11 +214,10 @@ private BoundStatement RewriteForEachEnumerator(
// }

var rewrittenBodyBlock = CreateBlockDeclaringIterationVariables(iterationVariables, iterationVarDecl, rewrittenBody, forEachSyntax);
BoundExpression rewrittenCondition = SynthesizeCall(
BoundExpression rewrittenCondition = MakeCall(
methodArgumentInfo: enumeratorInfo.MoveNextInfo,
syntax: forEachSyntax,
receiver: boundEnumeratorVar,
allowExtensionAndOptionalParameters: isAsync,
expression: boundEnumeratorVar,
firstRewrittenArgument: null);

var disposalFinallyBlock = GetDisposalFinallyBlock(forEachSyntax, enumeratorInfo, enumeratorType, boundEnumeratorVar, out var hasAsyncDisposal);
Expand Down Expand Up @@ -369,7 +368,7 @@ private bool TryGetDisposeMethod(SyntaxNode forEachSyntax, ForEachEnumeratorInfo
}

// ((IDisposable)e).Dispose() or e.Dispose() or await ((IAsyncDisposable)e).DisposeAsync() or await e.DisposeAsync()
disposeCall = MakeCallWithNoExplicitArgument(disposeInfo, forEachSyntax, receiver, firstRewrittenArgument: null);
disposeCall = MakeCall(disposeInfo, forEachSyntax, receiver, firstRewrittenArgument: null);

BoundStatement disposeCallStatement;
var disposeAwaitableInfoOpt = enumeratorInfo.DisposeAwaitableInfo;
Expand Down Expand Up @@ -535,19 +534,6 @@ private BoundExpression ConvertReceiverForInvocation(CSharpSyntaxNode syntax, Bo
return receiver;
}

private BoundExpression SynthesizeCall(MethodArgumentInfo methodArgumentInfo, CSharpSyntaxNode syntax, BoundExpression? receiver, bool allowExtensionAndOptionalParameters, BoundExpression? firstRewrittenArgument)
{
if (allowExtensionAndOptionalParameters)
{
// Generate a call with zero explicit arguments, but with implicit arguments for optional and params parameters.
return MakeCallWithNoExplicitArgument(methodArgumentInfo, syntax, receiver, firstRewrittenArgument: firstRewrittenArgument);
}

// Generate a call with literally zero arguments
Debug.Assert(methodArgumentInfo.Arguments.IsEmpty);
return BoundCall.Synthesized(syntax, receiver, initialBindingReceiverIsSubjectToCloning: ThreeState.Unknown, methodArgumentInfo.Method, arguments: ImmutableArray<BoundExpression>.Empty);
}

/// <summary>
/// Lower a foreach loop that will enumerate a collection via indexing.
///
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_UsingStatement.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,7 @@ private BoundExpression GenerateDisposeCall(
disposeInfo = MethodArgumentInfo.CreateParameterlessMethod(disposeMethod);
}

disposeCall = MakeCallWithNoExplicitArgument(disposeInfo, resourceSyntax, disposedExpression, firstRewrittenArgument: null);
disposeCall = MakeCall(disposeInfo, resourceSyntax, disposedExpression, firstRewrittenArgument: null);

if (awaitOpt is object)
{
Expand All @@ -489,7 +489,7 @@ private BoundExpression GenerateDisposeCall(
/// Synthesize a call `expression.Method()`, but with some extra smarts to handle extension methods. This call expects that the
/// receiver parameter has already been visited.
/// </summary>
private BoundExpression MakeCallWithNoExplicitArgument(MethodArgumentInfo methodArgumentInfo, SyntaxNode syntax, BoundExpression? expression, BoundExpression? firstRewrittenArgument)
private BoundExpression MakeCall(MethodArgumentInfo methodArgumentInfo, SyntaxNode syntax, BoundExpression? expression, BoundExpression? firstRewrittenArgument)
{
MethodSymbol method = methodArgumentInfo.Method;

Expand Down