Skip to content

Extensions: account for new extensions in function type scenarios #77755

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 2 commits into from
Mar 24, 2025
Merged

Extensions: account for new extensions in function type scenarios #77755

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0195e15
Extensions: account for new extensions in function type scenarios
jcouv Mar 20, 2025
c73f9d2
Address feedback
jcouv Mar 24, 2025
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
74 changes: 45 additions & 29 deletions src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10988,13 +10988,18 @@ static bool isCandidateUnique(ref MethodSymbol? method, MethodSymbol candidate)
useParams = false;
MethodSymbol? foundMethod = null;
var typeArguments = node.TypeArgumentsOpt;
int arity = typeArguments.IsDefaultOrEmpty ? 0 : typeArguments.Length;

// 1. instance methods
if (node.ResultKind == LookupResultKind.Viable)
{
var methods = ArrayBuilder<MethodSymbol>.GetInstance(capacity: node.Methods.Length);
foreach (var memberMethod in node.Methods)
{
switch (node.ReceiverOpt)
{
case BoundTypeOrValueExpression:
Copy link
Member Author

Choose a reason for hiding this comment

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

📝 Fixed a bug here. The relevant test is FunctionType_ColorColorReceiver_04.

break;
case BoundTypeExpression:
case null: // if `using static Class` is in effect, the receiver is missing
if (!memberMethod.IsStatic) continue;
Expand All @@ -11006,7 +11011,6 @@ static bool isCandidateUnique(ref MethodSymbol? method, MethodSymbol candidate)
break;
}

int arity = typeArguments.IsDefaultOrEmpty ? 0 : typeArguments.Length;
if (memberMethod.Arity != arity)
{
// We have no way of inferring type arguments, so if the given type arguments
Expand Down Expand Up @@ -11047,52 +11051,63 @@ static bool isCandidateUnique(ref MethodSymbol? method, MethodSymbol candidate)
}
}

if (node.ReceiverOpt is not BoundTypeExpression && node.SearchExtensions)
// 2. extensions
if (node.SearchExtensions)
{
var receiver = node.ReceiverOpt!;
var methodGroup = MethodGroup.GetInstance();
Debug.Assert(node.ReceiverOpt!.Type is not null); // extensions are only considered on member access

BoundExpression receiver = node.ReceiverOpt;
LookupOptions options = arity == 0 ? LookupOptions.AllMethodsOnArityZero : LookupOptions.Default;
var singleLookupResults = ArrayBuilder<SingleLookupResult>.GetInstance();
CompoundUseSiteInfo<AssemblySymbol> discardedUseSiteInfo = CompoundUseSiteInfo<AssemblySymbol>.Discarded;

foreach (var scope in new ExtensionScopes(this))
{
methodGroup.Clear();
PopulateExtensionMethodsFromSingleBinder(scope, methodGroup, node.Syntax, receiver, node.Name, typeArguments, BindingDiagnosticBag.Discarded); // PROTOTYPE account for new extension members
var methods = ArrayBuilder<MethodSymbol>.GetInstance(capacity: methodGroup.Methods.Count);
foreach (var extensionMethod in methodGroup.Methods)
{
var substituted = typeArguments.IsDefaultOrEmpty ? extensionMethod : extensionMethod.Construct(typeArguments);

var reduced = substituted.ReduceExtensionMethod(receiver.Type, Compilation, out bool wasFullyInferred);
if (reduced is null)
{
// Extension method was not applicable
continue;
}
singleLookupResults.Clear();
scope.Binder.EnumerateAllExtensionMembersInSingleBinder(singleLookupResults, node.Name, arity, options, originalBinder: this, ref discardedUseSiteInfo, ref discardedUseSiteInfo);

if (!wasFullyInferred)
var methods = ArrayBuilder<MethodSymbol>.GetInstance(capacity: singleLookupResults.Count);
foreach (SingleLookupResult singleLookupResult in singleLookupResults)
{
// Remove static/instance mismatches
Symbol extensionMember = singleLookupResult.Symbol;
bool memberCountsAsStatic = extensionMember is MethodSymbol { IsExtensionMethod: true } ? false : extensionMember.IsStatic;
switch (node.ReceiverOpt)
{
continue;
case BoundTypeOrValueExpression:
break;
case BoundTypeExpression:
if (!memberCountsAsStatic) continue;
break;
default:
if (memberCountsAsStatic) continue;
break;
}

if (!satisfiesConstraintChecks(reduced))
// Note: we only care about methods since we're already decided this is a method group (ie. not resolving to some other kind of extension member)
if (extensionMember is MethodSymbol method)
{
continue;
var substituted = (MethodSymbol?)extensionMember.GetReducedAndFilteredSymbol(typeArguments, receiver.Type, Compilation, checkFullyInferred: true);
if (substituted is not null)
{
methods.Add(substituted);
}
}

methods.Add(reduced);
}

if (!OverloadResolution.FilterMethodsForUniqueSignature(methods, out useParams))
{
singleLookupResults.Free();
methods.Free();
methodGroup.Free();
return null;
}

foreach (var reduced in methods)
foreach (var method in methods)
{
if (!isCandidateUnique(ref foundMethod, reduced))
if (!isCandidateUnique(ref foundMethod, method))
{
singleLookupResults.Free();
methods.Free();
methodGroup.Free();
useParams = false;
return null;
}
Expand All @@ -11102,11 +11117,12 @@ static bool isCandidateUnique(ref MethodSymbol? method, MethodSymbol candidate)

if (foundMethod is not null)
{
methodGroup.Free();
singleLookupResults.Free();
return foundMethod;
}
}
methodGroup.Free();

singleLookupResults.Free();
}

useParams = false;
Expand Down
91 changes: 11 additions & 80 deletions src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4683,92 +4683,23 @@ private static bool AddReducedAndFilteredSymbol(
TypeSymbol receiverType,
CSharpCompilation compilation)
{
if (member is MethodSymbol method)
Symbol? substitutedMember = member.GetReducedAndFilteredSymbol(typeArguments, receiverType, compilation, checkFullyInferred: false);
if (substitutedMember is null)
{
MethodSymbol constructedMethod;
if (!typeArguments.IsDefaultOrEmpty && method.GetMemberArityIncludingExtension() == typeArguments.Length)
{
constructedMethod = method.ConstructIncludingExtension(typeArguments);
Debug.Assert((object)constructedMethod != null);

if (!checkConstraintsIncludingExtension(constructedMethod, compilation, method.ContainingAssembly.CorLibrary.TypeConversions))
{
return false;
}
}
else
{
constructedMethod = method;
}

if ((object)receiverType != null)
{
if (method.IsExtensionMethod)
{
constructedMethod = constructedMethod.ReduceExtensionMethod(receiverType, compilation);
}
else
{
Debug.Assert(method.GetIsNewExtensionMember());
constructedMethod = (MethodSymbol)SourceNamedTypeSymbol.GetCompatibleSubstitutedMember(compilation, constructedMethod, receiverType)!;
}

if ((object)constructedMethod == null)
{
return false;
}
}

// Don't add exact duplicates.
if (filteredMembers.Contains(constructedMethod))
{
return false;
}

members.Add(member);
filteredMembers.Add(constructedMethod);
return true;
}
else if (member is PropertySymbol property)
{
Debug.Assert(receiverType is not null);
Debug.Assert(property.GetIsNewExtensionMember());
var constructedProperty = (PropertySymbol)SourceNamedTypeSymbol.GetCompatibleSubstitutedMember(compilation, property, receiverType)!;

if (constructedProperty is null)
{
return false;
}

members.Add(member);
filteredMembers.Add(constructedProperty);
return true;
return false;
}

throw ExceptionUtilities.UnexpectedValue(member.Kind);

static bool checkConstraintsIncludingExtension(MethodSymbol symbol, CSharpCompilation compilation, TypeConversions conversions)
// Don't add exact duplicates.
if (filteredMembers.Contains(substitutedMember))
{
var constraintArgs = new ConstraintsHelper.CheckConstraintsArgs(compilation, conversions, includeNullability: false,
NoLocation.Singleton, diagnostics: BindingDiagnosticBag.Discarded, template: CompoundUseSiteInfo<AssemblySymbol>.Discarded);

bool success = true;

if (symbol.GetIsNewExtensionMember())
{
NamedTypeSymbol extensionDeclaration = symbol.ContainingType;
success = extensionDeclaration.CheckConstraints(constraintArgs);
}

if (success)
{
success = symbol.CheckConstraints(constraintArgs);
}

return success;
return false;
}
#nullable disable

members.Add(member);
filteredMembers.Add(substitutedMember);
return true;
}
#nullable disable

private static void MergeReducedAndFilteredSymbol(
ArrayBuilder<Symbol> members,
Expand Down
94 changes: 94 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/MemberSymbolExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
using System.Linq;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Text;
using Roslyn.Utilities;

Expand Down Expand Up @@ -154,6 +155,99 @@ internal static TMember ConstructIncludingExtension<TMember>(this TMember member
throw ExceptionUtilities.UnexpectedValue(member);
}

// For lookup APIs in the semantic model, we can return symbols that aren't fully inferred.
// But for function type inference, if the symbol isn't fully inferred with the information we have (the receiver and any explicit type arguments)
// then we won't return it.
internal static Symbol? GetReducedAndFilteredSymbol(this Symbol member, ImmutableArray<TypeWithAnnotations> typeArguments, TypeSymbol receiverType, CSharpCompilation compilation, bool checkFullyInferred)
Copy link
Member Author

Choose a reason for hiding this comment

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

📝 This logic is extracted from AddReducedAndFilteredSymbol, but the check for "fully inferred" was added.

{
if (member is MethodSymbol method)
{
// 1. construct with explicit type arguments if provided
MethodSymbol constructed;
Copy link
Member

@jjonescz jjonescz Mar 24, 2025 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

Shouldn't this be nullable? We seem to be comparing constructed to null in several places below.

Suggested change
MethodSymbol constructed;
MethodSymbol? constructed;
``` #Resolved

if (!typeArguments.IsDefaultOrEmpty && method.GetMemberArityIncludingExtension() == typeArguments.Length)
{
constructed = method.ConstructIncludingExtension(typeArguments);
Debug.Assert((object)constructed != null);

if (!checkConstraintsIncludingExtension(constructed, compilation, method.ContainingAssembly.CorLibrary.TypeConversions))
{
return null;
}
}
else
{
constructed = method;
}

// 2. infer type arguments based on the receiver type if needed, check applicability, reduce symbol (for classic extension methods), check whether fully inferred
if ((object)receiverType != null)
{
if (method.IsExtensionMethod)
{
constructed = constructed.ReduceExtensionMethod(receiverType, compilation, out bool wasFullyInferred);

if (checkFullyInferred && !wasFullyInferred)
{
return null;
}
}
else
{
Debug.Assert(method.GetIsNewExtensionMember());
constructed = (MethodSymbol)SourceNamedTypeSymbol.GetCompatibleSubstitutedMember(compilation, constructed, receiverType)!;
Copy link
Member

@jjonescz jjonescz Mar 24, 2025 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

Should we be also checking this result for null instead of suppressing nullability? Since we do that below for properties. #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Thanks. Added a test that hits that


if (checkFullyInferred && constructed.IsGenericMethod && typeArguments.IsDefaultOrEmpty)
{
return null;
}
}

if ((object)constructed == null)
Copy link
Member

@jjonescz jjonescz Mar 24, 2025 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

This looks unnecessary since we return constructed below anyway. #Resolved

{
return null;
}
}

return constructed;
}
else if (member is PropertySymbol property)
{
// infer type arguments based off the receiver type if needed, check applicability
Debug.Assert(receiverType is not null);
Debug.Assert(property.GetIsNewExtensionMember());
var constructedProperty = (PropertySymbol)SourceNamedTypeSymbol.GetCompatibleSubstitutedMember(compilation, property, receiverType)!;
Copy link
Member

@jjonescz jjonescz Mar 24, 2025 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

Why do we suppress nullability here but compare with null below? #Resolved


if (constructedProperty is null)
Copy link
Member

@jjonescz jjonescz Mar 24, 2025 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

Similar comment. #Resolved

{
return null;
}

return constructedProperty;
}

throw ExceptionUtilities.UnexpectedValue(member.Kind);

static bool checkConstraintsIncludingExtension(MethodSymbol symbol, CSharpCompilation compilation, TypeConversions conversions)
{
var constraintArgs = new ConstraintsHelper.CheckConstraintsArgs(compilation, conversions, includeNullability: false,
NoLocation.Singleton, diagnostics: BindingDiagnosticBag.Discarded, template: CompoundUseSiteInfo<AssemblySymbol>.Discarded);

bool success = true;

if (symbol.GetIsNewExtensionMember())
{
NamedTypeSymbol extensionDeclaration = symbol.ContainingType;
success = extensionDeclaration.CheckConstraints(constraintArgs);
}

if (success)
{
success = symbol.CheckConstraints(constraintArgs);
}

return success;
}
}
#nullable disable

/// <summary>
Expand Down
Loading