Skip to content

Extensions: Review/adjust call sites of MethodSymbol.IsExtensionMethod API #77820

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
Mar 26, 2025
Merged

Extensions: Review/adjust call sites of MethodSymbol.IsExtensionMethod API #77820

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
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1708,7 +1708,7 @@ internal bool TryGetCollectionIterationType(SyntaxNode syntax, TypeSymbol collec
out iterationType,
builder: out var builder);
// Collection expression target types require instance method GetEnumerator.
if (result && builder.ViaExtensionMethod)
if (result && builder.ViaExtensionMethod) // PROTOTYPE: Add test coverage for new extensions
{
iterationType = default;
return false;
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Deconstruct.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -683,7 +683,7 @@ private BoundExpression MakeDeconstructInvocationExpression(
// This prevents, for example, an unused params parameter after the out parameters.
var deconstructMethod = ((BoundCall)result).Method;
var parameters = deconstructMethod.Parameters;
for (int i = (deconstructMethod.IsExtensionMethod ? 1 : 0); i < parameters.Length; i++)
for (int i = (deconstructMethod.IsExtensionMethod ? 1 : 0); i < parameters.Length; i++) // PROTOTYPE: Test this code path with new extensions
{
if (parameters[i].RefKind != RefKind.Out)
{
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1058,7 +1058,7 @@ deconstructMethod is null &&
if (deconstructMethod is null)
hasErrors = true;

int skippedExtensionParameters = deconstructMethod?.IsExtensionMethod == true ? 1 : 0;
int skippedExtensionParameters = deconstructMethod?.IsExtensionMethod == true ? 1 : 0; // PROTOTYPE: Test this code path with new extensions
for (int i = 0; i < node.Subpatterns.Count; i++)
{
var subPattern = node.Subpatterns[i];
Expand Down
107 changes: 74 additions & 33 deletions src/Compilers/CSharp/Portable/Binder/ForEachLoopBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,12 +236,20 @@ private BoundForEachStatement BindForEachPartsWorker(BindingDiagnosticBag diagno
{
originalBinder.CheckImplicitThisCopyInReadOnlyMember(collectionExpr, getEnumeratorMethod, diagnostics);

if (getEnumeratorMethod.IsExtensionMethod && !hasErrors)
if (!hasErrors)
{
var messageId = IsAsync ? MessageID.IDS_FeatureExtensionGetAsyncEnumerator : MessageID.IDS_FeatureExtensionGetEnumerator;
messageId.CheckFeatureAvailability(diagnostics, Compilation, collectionExpr.Syntax.Location);
if (getEnumeratorMethod.IsExtensionMethod)
{
var messageId = IsAsync ? MessageID.IDS_FeatureExtensionGetAsyncEnumerator : MessageID.IDS_FeatureExtensionGetEnumerator;
messageId.CheckFeatureAvailability(diagnostics, Compilation, collectionExpr.Syntax.Location);

if (getEnumeratorMethod.ParameterRefKinds is { IsDefault: false } refKinds && refKinds[0] == RefKind.Ref)
if (getEnumeratorMethod.ParameterRefKinds is { IsDefault: false } refKinds && refKinds[0] == RefKind.Ref)
{
Error(diagnostics, ErrorCode.ERR_RefLvalueExpected, collectionExpr.Syntax);
hasErrors = true;
}
}
else if (getEnumeratorMethod.GetIsNewExtensionMember() && getEnumeratorMethod.ContainingType.ExtensionParameter.RefKind == RefKind.Ref) // PROTOTYPE: add test coverage for 'ref readonly' and 'in'
{
Error(diagnostics, ErrorCode.ERR_RefLvalueExpected, collectionExpr.Syntax);
hasErrors = true;
Expand Down Expand Up @@ -570,7 +578,8 @@ private BoundForEachStatement BindForEachPartsWorker(BindingDiagnosticBag diagno
(collectionConversionClassification.IsImplicit &&
(IsIEnumerable(builder.CollectionType) ||
IsIEnumerableT(builder.CollectionType.OriginalDefinition, IsAsync, Compilation) ||
builder.GetEnumeratorInfo.Method.IsExtensionMethod)) ||
builder.GetEnumeratorInfo.Method.IsExtensionMethod ||
builder.GetEnumeratorInfo.Method.GetIsNewExtensionMember())) ||
// For compat behavior, we can enumerate over System.String even if it's not IEnumerable. That will
// result in an explicit reference conversion in the bound nodes, but that conversion won't be emitted.
(collectionConversionClassification.Kind == ConversionKind.ExplicitReference && collectionExpr.Type.SpecialType == SpecialType.System_String));
Expand Down Expand Up @@ -861,9 +870,9 @@ private EnumeratorResult GetEnumeratorInfoCore(SyntaxNode syntax, SyntaxNode col

#if DEBUG
Debug.Assert(span == originalSpan);
Debug.Assert(!builder.ViaExtensionMethod || builder.GetEnumeratorInfo.Method.IsExtensionMethod);
Debug.Assert(!builder.ViaExtensionMethod || builder.GetEnumeratorInfo.Method.IsExtensionMethod || builder.GetEnumeratorInfo.Method.GetIsNewExtensionMember());
#endif
if (!builder.ViaExtensionMethod &&
if (!builder.ViaExtensionMethod && // PROTOTYPE: Add test coverage for new extensions
((result is EnumeratorResult.Succeeded && builder.ElementTypeWithAnnotations.Equals(elementField.TypeWithAnnotations, TypeCompareKind.AllIgnoreOptions) &&
builder.CurrentPropertyGetter?.RefKind == (wellKnownSpan == WellKnownType.System_ReadOnlySpan_T ? RefKind.RefReadOnly : RefKind.Ref)) ||
result is EnumeratorResult.FailedAndReported))
Expand Down Expand Up @@ -908,7 +917,7 @@ private EnumeratorResult GetEnumeratorInfoCore(SyntaxNode syntax, SyntaxNode col
#if DEBUG
Debug.Assert(collectionExpr == originalCollectionExpr ||
(originalCollectionExpr.Type?.IsNullableType() == true && originalCollectionExpr.Type.StrippedType().Equals(collectionExpr.Type, TypeCompareKind.AllIgnoreOptions)));
Debug.Assert(!builder.ViaExtensionMethod || builder.GetEnumeratorInfo.Method.IsExtensionMethod);
Debug.Assert(!builder.ViaExtensionMethod || builder.GetEnumeratorInfo.Method.IsExtensionMethod || builder.GetEnumeratorInfo.Method.GetIsNewExtensionMember());
#endif

return result;
Expand Down Expand Up @@ -1019,12 +1028,26 @@ EnumeratorResult createPatternBasedEnumeratorResult(ref ForEachEnumeratorInfo.Bu
{
Debug.Assert((object)builder.GetEnumeratorInfo != null);

Debug.Assert(!(viaExtensionMethod && builder.GetEnumeratorInfo.Method.Parameters.IsDefaultOrEmpty));
Debug.Assert(!(viaExtensionMethod && builder.GetEnumeratorInfo.Method.IsExtensionMethod && builder.GetEnumeratorInfo.Method.Parameters.IsDefaultOrEmpty));
Debug.Assert(!(viaExtensionMethod && !builder.GetEnumeratorInfo.Method.IsExtensionMethod && !builder.GetEnumeratorInfo.Method.GetIsNewExtensionMember()));

builder.ViaExtensionMethod = viaExtensionMethod;
builder.CollectionType = viaExtensionMethod
? builder.GetEnumeratorInfo.Method.Parameters[0].Type
: collectionExpr.Type;

if (viaExtensionMethod)
{
if (builder.GetEnumeratorInfo.Method.IsExtensionMethod)
{
builder.CollectionType = builder.GetEnumeratorInfo.Method.Parameters[0].Type;
}
else
{
builder.CollectionType = builder.GetEnumeratorInfo.Method.ContainingType.ExtensionParameter.Type;
}
}
else
{
builder.CollectionType = collectionExpr.Type;
}

if (SatisfiesForEachPattern(syntax, collectionSyntax, ref builder, isAsync, diagnostics))
{
Expand Down Expand Up @@ -1200,7 +1223,7 @@ private void GetDisposalInfoForEnumerator(SyntaxNode syntax, ref ForEachEnumerat
MethodSymbol patternDisposeMethod = TryFindDisposePatternMethod(receiver, syntax, isAsync, patternDiagnostics, out bool expanded);
if (patternDisposeMethod is object)
{
Debug.Assert(!patternDisposeMethod.IsExtensionMethod);
Debug.Assert(!patternDisposeMethod.IsExtensionMethod && !patternDisposeMethod.GetIsNewExtensionMember());
Debug.Assert(patternDisposeMethod.ParameterRefKinds.IsDefaultOrEmpty ||
patternDisposeMethod.ParameterRefKinds.All(static refKind => refKind is RefKind.None or RefKind.In or RefKind.RefReadOnlyParameter));

Expand Down Expand Up @@ -1522,6 +1545,8 @@ private MethodArgumentInfo FindForEachPatternMethodViaExtension(SyntaxNode synta
{
var result = overloadResolutionResult.ValidResult.Member;

Debug.Assert(result.IsExtensionMethod || result.GetIsNewExtensionMember());

if (result.CallsAreOmitted(syntax.SyntaxTree))
{
// Calls to this method are omitted in the current syntax tree, i.e it is either a partial method with no implementation part OR a conditional method whose condition is not true in this source file.
Expand All @@ -1531,28 +1556,44 @@ private MethodArgumentInfo FindForEachPatternMethodViaExtension(SyntaxNode synta
return null;
}

CompoundUseSiteInfo<AssemblySymbol> useSiteInfo = GetNewCompoundUseSiteInfo(diagnostics);
var collectionConversion = this.Conversions.ClassifyConversionFromExpression(collectionExpr, result.Parameters[0].Type, isChecked: CheckOverflowAtRuntime, ref useSiteInfo);
diagnostics.Add(syntax, useSiteInfo);
MethodArgumentInfo info;
bool expanded = overloadResolutionResult.ValidResult.Result.Kind == MemberResolutionKind.ApplicableInExpandedForm;

// Unconditionally convert here, to match what we set the ConvertedExpression to in the main BoundForEachStatement node.
Debug.Assert(!collectionConversion.IsUserDefined);
collectionExpr = new BoundConversion(
collectionExpr.Syntax,
collectionExpr,
collectionConversion,
@checked: CheckOverflowAtRuntime,
explicitCastInCode: false,
conversionGroupOpt: null,
ConstantValue.NotAvailable,
result.Parameters[0].Type);
if (result.IsExtensionMethod)
{
CompoundUseSiteInfo<AssemblySymbol> useSiteInfo = GetNewCompoundUseSiteInfo(diagnostics);
var collectionConversion = this.Conversions.ClassifyConversionFromExpression(collectionExpr, result.Parameters[0].Type, isChecked: CheckOverflowAtRuntime, ref useSiteInfo);
diagnostics.Add(syntax, useSiteInfo);

// Unconditionally convert here, to match what we set the ConvertedExpression to in the main BoundForEachStatement node.
Debug.Assert(!collectionConversion.IsUserDefined);
collectionExpr = new BoundConversion(
collectionExpr.Syntax,
collectionExpr,
collectionConversion,
@checked: CheckOverflowAtRuntime,
explicitCastInCode: false,
conversionGroupOpt: null,
ConstantValue.NotAvailable,
result.Parameters[0].Type);

info = BindDefaultArguments(
result,
collectionExpr,
expanded: expanded,
collectionExpr.Syntax,
diagnostics);
}
else
{
info = BindDefaultArguments(
result,
extensionReceiverOpt: null,
expanded: expanded,
collectionExpr.Syntax,
diagnostics);
}

var info = BindDefaultArguments(
result,
collectionExpr,
expanded: overloadResolutionResult.ValidResult.Result.Kind == MemberResolutionKind.ApplicableInExpandedForm,
collectionExpr.Syntax,
diagnostics);
methodGroupResolutionResult.Free();
analyzedArguments.Free();
return info;
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/RefSafetyAnalysis.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,7 +604,7 @@ static SafeContext getDeclarationValEscape(BoundTypeExpression typeExpression, S

static ParameterSymbol? tryGetThisParameter(MethodSymbol method)
{
if (method.IsExtensionMethod)
if (method.IsExtensionMethod) // PROTOTYPE: Test this code path with new extensions
{
return method.Parameters is [{ } firstParameter, ..] ? firstParameter : null;
}
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/WithUsingNamespacesAndTypesBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ private static bool IsValidLookupCandidateInUsings(Symbol symbol)

// lookup via "using static" ignores extension methods and non-static methods
case SymbolKind.Method:
if (!symbol.IsStatic || ((MethodSymbol)symbol).IsExtensionMethod)
if (!symbol.IsStatic || ((MethodSymbol)symbol).IsExtensionMethod) // PROTOTYPE: Test this code path with new extensions
{
return false;
}
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1635,7 +1635,7 @@ public override BoundNode VisitDelegateCreationExpression(BoundDelegateCreationE
static bool ignoreReceiver(MethodSymbol method)
{
// static methods that aren't extensions get an implicit `this` receiver that should be ignored
return method.IsStatic && !method.IsExtensionMethod;
return method.IsStatic && !method.IsExtensionMethod; // PROTOTYPE: Test this code path with new extensions
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.DebugVerifier.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ private void VerifyExpression(BoundExpression expression, bool overrideSkippedEx
private void VisitForEachEnumeratorInfo(ForEachEnumeratorInfo enumeratorInfo)
{
Visit(enumeratorInfo.DisposeAwaitableInfo);
if (enumeratorInfo.GetEnumeratorInfo.Method.IsExtensionMethod)
if (enumeratorInfo.GetEnumeratorInfo.Method.IsExtensionMethod) // PROTOTYPE: Test this code path with new extensions
{
foreach (var arg in enumeratorInfo.GetEnumeratorInfo.Arguments)
{
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10975,7 +10975,7 @@ private void VisitForEachExpression(

MethodSymbol? reinferredGetEnumeratorMethod = null;

if (enumeratorInfoOpt?.GetEnumeratorInfo is { Method: { IsExtensionMethod: true, Parameters: var parameters } } enumeratorMethodInfo)
if (enumeratorInfoOpt?.GetEnumeratorInfo is { Method: { IsExtensionMethod: true, Parameters: var parameters } } enumeratorMethodInfo) // PROTOTYPE: Test this code path with new extensions
{
// this is case 7
// We do not need to do this same analysis for non-extension methods because they do not have generic parameters that
Expand Down Expand Up @@ -11042,7 +11042,7 @@ private void VisitForEachExpression(
useLegacyWarnings: false,
AssignmentKind.Assignment);

bool reportedDiagnostic = enumeratorInfoOpt?.GetEnumeratorInfo.Method is { IsExtensionMethod: true }
bool reportedDiagnostic = enumeratorInfoOpt?.GetEnumeratorInfo.Method is { IsExtensionMethod: true } // PROTOTYPE: Test this code path with new extensions
? false
: CheckPossibleNullReceiver(expr);

Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.PatternLocalRewriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ void addArg(RefKind refKind, BoundExpression expression)

Debug.Assert(method.Name == WellKnownMemberNames.DeconstructMethodName);
int extensionExtra;
if (method.IsStatic)
if (method.IsStatic) // PROTOTYPE: Test this code path with new extensions
{
Debug.Assert(method.IsExtensionMethod);
receiver = _factory.Type(method.ContainingType);
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Call.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ private void InterceptCallAndAdjustArguments(
// When the original call is to an instance method, and the interceptor is an extension method,
// we need to take special care to intercept with the extension method as though it is being called in reduced form.
Debug.Assert(receiverOpt is not BoundTypeExpression || method.IsStatic);
var needToReduce = receiverOpt is not (null or BoundTypeExpression) && interceptor.IsExtensionMethod;
var needToReduce = receiverOpt is not (null or BoundTypeExpression) && interceptor.IsExtensionMethod; // PROTOTYPE: Test this code path with new extensions
var symbolForCompare = needToReduce ? ReducedExtensionMethodSymbol.Create(interceptor, receiverOpt!.Type, _compilation, out _) : interceptor; // PROTOTYPE test interceptors

if (!MemberSignatureComparer.InterceptorsComparer.Equals(method, symbolForCompare))
Expand Down Expand Up @@ -245,7 +245,7 @@ private void InterceptCallAndAdjustArguments(
break;
}

if (invokedAsExtensionMethod && interceptor.IsStatic && !interceptor.IsExtensionMethod)
if (invokedAsExtensionMethod && interceptor.IsStatic && !interceptor.IsExtensionMethod) // PROTOTYPE: Test this code path with new extensions
{
// Special case when intercepting an extension method call in reduced form with a non-extension.
this._diagnostics.Add(ErrorCode.ERR_InterceptorMustHaveMatchingThisParameter, attributeLocation, method.Parameters[0], method);
Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -181,7 +181,7 @@ 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, firstRewrittenArgument: firstRewrittenArgument);
allowExtensionAndOptionalParameters: isAsync || getEnumeratorInfo.Method.IsExtensionMethod || getEnumeratorInfo.Method.GetIsNewExtensionMember(), firstRewrittenArgument: firstRewrittenArgument);
Copy link
Member

Choose a reason for hiding this comment

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

Does this change have any effect? It looks like SynthesizeCall only checks IsExtensionMethod anyway.

Copy link
Contributor Author

@AlekseyTs AlekseyTs Mar 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, this change has an effect. I am not sure what check you are referring to, but this change gets us into the if (allowExtensionAndOptionalParameters) on line 540, which was the intent. Otherwise ```Debug.Assert(methodArgumentInfo.Arguments.IsEmpty);```` fails, which indicates that we are about to do a wrong thing.


// E e = ((C)(x)).GetEnumerator();
BoundStatement enumeratorVarDecl = MakeLocalDeclaration(forEachSyntax, enumeratorVar, enumeratorVarInitValue);
Expand Down
3 changes: 1 addition & 2 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceComplexParameterSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1588,9 +1588,8 @@ void validateParamsType(BindingDiagnosticBag diagnostics)

Debug.Assert(!addMethods.IsDefaultOrEmpty);

if (addMethods[0].IsStatic) // No need to check other methods, extensions are never mixed with instance methods
if (addMethods[0].IsExtensionMethod || addMethods[0].GetIsNewExtensionMember()) // No need to check other methods, extensions are never mixed with instance methods
{
Debug.Assert(addMethods[0].IsExtensionMethod);
diagnostics.Add(ErrorCode.ERR_ParamsCollectionExtensionAddMethod, syntax, Type);
return;
}
Expand Down
Loading