Skip to content

Extensions: pattern-based constructs #78480

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

Merged
merged 11 commits into from
May 14, 2025
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
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++) // Tracked by https://github.com/dotnet/roslyn/issues/76130: Test this code path with new extensions
Copy link
Member Author

Choose a reason for hiding this comment

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

📝 Covered by Deconstruct_01 and Deconstruct_02

for (int i = (deconstructMethod.IsExtensionMethod ? 1 : 0); i < parameters.Length; i++)
{
if (parameters[i].RefKind != RefKind.Out)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ private BoundInterpolatedString BindUnconvertedInterpolatedExpressionToFactory(
SyntaxNode syntax = unconvertedSource.Syntax;
ImmutableArray<BoundExpression> expressions = makeInterpolatedStringFactoryArguments(syntax, parts, diagnostics);

BoundExpression construction = MakeInvocationExpression(
BoundExpression construction = MakeInvocationExpression( // Tracked by https://github.com/dotnet/roslyn/issues/76130 : test this scenario with a delegate-returning property (should be blocked by virtue of allowFieldsAndProperties: false)
syntax,
new BoundTypeExpression(syntax, null, factoryType) { WasCompilerGenerated = true },
factoryMethod,
Expand Down
13 changes: 7 additions & 6 deletions src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -137,11 +137,11 @@ internal BoundExpression MakeInvocationExpression(
}

BoundExpression result = BindInvocationExpression(
node, node, methodName, boundExpression, analyzedArguments, diagnostics, queryClause,
ignoreNormalFormIfHasValidParamsParameter: ignoreNormalFormIfHasValidParamsParameter,
node, node, methodName, boundExpression, analyzedArguments, diagnostics, acceptOnlyMethods: !allowFieldsAndProperties,
queryClause, ignoreNormalFormIfHasValidParamsParameter: ignoreNormalFormIfHasValidParamsParameter,
disallowExpandedNonArrayParams: disallowExpandedNonArrayParams);

// Query operator can't be called dynamically.
// Query operator can't be called dynamically.
if (queryClause != null && result.Kind == BoundKind.DynamicInvocation)
{
// the error has already been reported by BindInvocationExpression
Expand Down Expand Up @@ -245,7 +245,7 @@ BoundExpression bindArgumentsAndInvocation(InvocationExpressionSyntax node, Boun
boundExpression = CheckValue(boundExpression, BindValueKind.RValueOrMethodGroup, diagnostics);
string name = boundExpression.Kind == BoundKind.MethodGroup ? GetName(node.Expression) : null;
BindArgumentsAndNames(node.ArgumentList, diagnostics, analyzedArguments, allowArglist: true);
return BindInvocationExpression(node, node.Expression, name, boundExpression, analyzedArguments, diagnostics);
return BindInvocationExpression(node, node.Expression, name, boundExpression, analyzedArguments, diagnostics, acceptOnlyMethods: false);
}

static bool receiverIsInvocation(InvocationExpressionSyntax node, out InvocationExpressionSyntax nested)
Expand Down Expand Up @@ -324,6 +324,7 @@ private BoundExpression BindInvocationExpression(
BoundExpression boundExpression,
AnalyzedArguments analyzedArguments,
BindingDiagnosticBag diagnostics,
bool acceptOnlyMethods,
CSharpSyntaxNode queryClause = null,
bool ignoreNormalFormIfHasValidParamsParameter = false,
bool disallowExpandedNonArrayParams = false)
Expand Down Expand Up @@ -356,7 +357,7 @@ private BoundExpression BindInvocationExpression(
ignoreNormalFormIfHasValidParamsParameter: ignoreNormalFormIfHasValidParamsParameter,
disallowExpandedNonArrayParams: disallowExpandedNonArrayParams,
anyApplicableCandidates: out _,
acceptOnlyMethods: false);
acceptOnlyMethods: acceptOnlyMethods);
Copy link
Contributor

@AlekseyTs AlekseyTs May 13, 2025

Choose a reason for hiding this comment

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

acceptOnlyMethods: acceptOnlyMethods);

Are we testing all call sites that pass true? #Closed

}
else if ((object)(delegateType = GetDelegateType(boundExpression)) != null)
{
Expand Down Expand Up @@ -727,7 +728,7 @@ private BoundExpression BindMethodGroupInvocation(
Debug.Assert(extensionMemberAccess.Kind != BoundKind.MethodGroup);

extensionMemberAccess = CheckValue(extensionMemberAccess, BindValueKind.RValue, diagnostics);
BoundExpression extensionMemberInvocation = BindInvocationExpression(syntax, expression, methodName: null, extensionMemberAccess, analyzedArguments, diagnostics);
BoundExpression extensionMemberInvocation = BindInvocationExpression(syntax, expression, methodName: null, extensionMemberAccess, analyzedArguments, diagnostics, acceptOnlyMethods: false);
anyApplicableCandidates = !extensionMemberInvocation.HasAnyErrors;
return extensionMemberInvocation;
}
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs
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; // Tracked by https://github.com/dotnet/roslyn/issues/76130: Test this code path with new extensions
int skippedExtensionParameters = deconstructMethod?.IsExtensionMethod == true ? 1 : 0;
Copy link
Member Author

Choose a reason for hiding this comment

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

📝 Covered by PositionalPattern_01

for (int i = 0; i < node.Subpatterns.Count; i++)
{
var subPattern = node.Subpatterns[i];
Expand Down
10 changes: 7 additions & 3 deletions src/Compilers/CSharp/Portable/Binder/RefSafetyAnalysis.cs
Original file line number Diff line number Diff line change
Expand Up @@ -627,7 +627,7 @@ static SafeContext getDeclarationValEscape(BoundTypeExpression typeExpression, S
// and so the ref safety of the pattern is equivalent to a `Deconstruct(out var ...)` invocation
// where "safe-context inference of declaration expressions" would have the same effect.
if (node.DeconstructMethod is { } m &&
tryGetThisParameter(m)?.EffectiveScope == ScopedKind.None)
tryGetReceiverParameter(m)?.EffectiveScope == ScopedKind.None)
{
using (new PatternInput(this, _localScopeDepth))
{
Expand All @@ -637,12 +637,16 @@ static SafeContext getDeclarationValEscape(BoundTypeExpression typeExpression, S

return base.VisitRecursivePattern(node);

static ParameterSymbol? tryGetThisParameter(MethodSymbol method)
static ParameterSymbol? tryGetReceiverParameter(MethodSymbol method)
{
if (method.IsExtensionMethod) // Tracked by https://github.com/dotnet/roslyn/issues/76130: Test this code path with new extensions
if (method.IsExtensionMethod)
{
return method.Parameters is [{ } firstParameter, ..] ? firstParameter : null;
}
else if (method.GetIsNewExtensionMember())
Copy link
Member Author

Choose a reason for hiding this comment

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

📝 Covered by Deconstruction_UnscopedRef_ExtensionMethod and Deconstruction_ScopedRef_ExtensionMethod

{
return method.ContainingType.ExtensionParameter;
Copy link
Contributor

@AlekseyTs AlekseyTs May 9, 2025

Choose a reason for hiding this comment

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

return method.ContainingType.ExtensionParameter;

It is not clear why do we want to have this parameter at all. How does this align with the language spec? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I understand the question.
From a ref analysis perspective, it should be equivalent to deconstruct via invocation, deconstruction or recursive pattern. So all three need to account for scoped on the receiver parameter. Here's we're looking at a skeleton signature, so this is the parameter we need to check for presence or absence of scoped. See Deconstruction_ScopedRef_ExtensionMethod.
I can adjust the name of the local function (maybe "tryGetReceiverParameter") if that's what bothers you.

}

return method.TryGetThisParameter(out var thisParameter) ? thisParameter : null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -652,7 +652,7 @@ private void MakeExplicitParameterTypeInferences(Binder binder, BoundExpression
}
else if (IsUnfixedTypeParameter(target) && !target.NullableAnnotation.IsAnnotated() && kind is ExactOrBoundsKind.LowerBound)
{
var ordinal = GetOrdinal((TypeParameterSymbol)target.Type); // Tracked by https://github.com/dotnet/roslyn/issues/76130 : test nullability scenario where the override of ordinals matters
Copy link
Member Author

Choose a reason for hiding this comment

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

📝 Covered by Nullability_Method_04

var ordinal = GetOrdinal((TypeParameterSymbol)target.Type);
_nullableAnnotationLowerBounds[ordinal] = _nullableAnnotationLowerBounds[ordinal].Join(argumentType.NullableAnnotation);
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -547,7 +547,7 @@
<value>Inconsistent accessibility: indexer return type '{1}' is less accessible than indexer '{0}'</value>
</data>
<data name="ERR_BadVisIndexerParam" xml:space="preserve">
<value>Inconsistent accessibility: parameter type '{1}' is less accessible than indexer '{0}'</value>
<value>Inconsistent accessibility: parameter type '{1}' is less accessible than indexer or property '{0}'</value>
</data>
<data name="ERR_BadVisOpReturn" xml:space="preserve">
<value>Inconsistent accessibility: return type '{1}' is less accessible than operator '{0}'</value>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ protected BoundExpression LowerEvaluation(BoundDagEvaluation evaluation)
PropertySymbol property = p.Property;
var outputTemp = new BoundDagTemp(p.Syntax, property.Type, p);
BoundExpression output = _tempAllocator.GetTemp(outputTemp);
input = _factory.ConvertReceiverForExtensionMemberIfNeeded(property, input);
return _factory.AssignmentExpression(output, _localRewriter.MakePropertyAccess(_factory.Syntax, input, property, LookupResultKind.Viable, property.Type, isLeftOfAssignment: false));
}

Expand All @@ -169,7 +170,7 @@ void addArg(RefKind refKind, BoundExpression expression)

Debug.Assert(method.Name == WellKnownMemberNames.DeconstructMethodName);
int extensionExtra;
if (method.IsStatic) // Tracked by https://github.com/dotnet/roslyn/issues/76130: Test this code path with new extensions
if (method.IsStatic)
{
Debug.Assert(method.IsExtensionMethod);
receiver = _factory.Type(method.ContainingType);
Expand All @@ -190,6 +191,7 @@ void addArg(RefKind refKind, BoundExpression expression)
addArg(RefKind.Out, _tempAllocator.GetTemp(outputTemp));
}

receiver = _factory.ConvertReceiverForExtensionMemberIfNeeded(method, receiver);
return _factory.Call(receiver, method, refKindBuilder.ToImmutableAndFree(), argBuilder.ToImmutableAndFree());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,7 @@ private BoundStatement InitializeFixedStatementGetPinnable(
}

// .GetPinnable()
callReceiver = factory.ConvertReceiverForExtensionMemberIfNeeded(getPinnableMethod, callReceiver);
var getPinnableCall = getPinnableMethod.IsStatic ?
factory.Call(null, getPinnableMethod, callReceiver) :
factory.Call(callReceiver, getPinnableMethod);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -698,9 +698,11 @@ private BoundExpression MakeObjectInitializerMemberAccess(
#if DEBUG
var discardedUseSiteInfo = CompoundUseSiteInfo<AssemblySymbol>.Discarded;
Debug.Assert(_compilation.Conversions.ClassifyConversionFromType(rewrittenReceiver.Type, memberSymbol.ContainingType, isChecked: false, ref discardedUseSiteInfo).IsImplicit ||
(memberSymbol.GetIsNewExtensionMember() && !memberSymbol.IsStatic && ConversionsBase.IsValidExtensionMethodThisArgConversion(_compilation.Conversions.ClassifyConversionFromType(rewrittenReceiver.Type, memberSymbol.ContainingType.ExtensionParameter!.Type, isChecked: false, ref discardedUseSiteInfo))) ||
_compilation.Conversions.HasImplicitConversionToOrImplementsVarianceCompatibleInterface(rewrittenReceiver.Type, memberSymbol.ContainingType, ref discardedUseSiteInfo, out _));
// It is possible there are use site diagnostics from the above, but none that we need report as we aren't generating code for the conversion
#endif
rewrittenReceiver = _factory.ConvertReceiverForExtensionMemberIfNeeded(memberSymbol, rewrittenReceiver);

switch (memberSymbol.Kind)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,24 @@ public BoundExpression AssignmentExpression(BoundExpression left, BoundExpressio
return AssignmentExpression(Syntax, left, right, isRef: isRef, wasCompilerGenerated: true);
}

public BoundExpression ConvertReceiverForExtensionMemberIfNeeded(Symbol member, BoundExpression receiver)
{
if (member.GetIsNewExtensionMember())
Copy link
Contributor

@AlekseyTs AlekseyTs May 9, 2025

Choose a reason for hiding this comment

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

member

Should we check !IsStatic? #Closed

{
Debug.Assert(!member.IsStatic);
ParameterSymbol? extensionParameter = member.ContainingType.ExtensionParameter;
Debug.Assert(extensionParameter is not null);
#if DEBUG
var discardedUseSiteInfo = CompoundUseSiteInfo<AssemblySymbol>.Discarded;
Debug.Assert(Conversions.IsValidExtensionMethodThisArgConversion(this.Compilation.Conversions.ClassifyConversionFromType(receiver.Type, extensionParameter.Type, isChecked: false, ref discardedUseSiteInfo)));
#endif

return this.Convert(extensionParameter.Type, receiver);
}

return receiver;
}

/// <summary>
/// Creates a general assignment that might be instrumented.
/// </summary>
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.de.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.es.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.fr.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.it.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ja.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ko.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.pl.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.pt-BR.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading