Skip to content

Extensions: update semantic model APIs #77619

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 13 commits into from
Mar 20, 2025
Merged
Show file tree
Hide file tree
Changes from 7 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 @@ -102,7 +102,7 @@ private static void ComputeDeclarations(
case SyntaxKind.StructDeclaration:
case SyntaxKind.RecordDeclaration:
case SyntaxKind.RecordStructDeclaration:
// PROTOTYPE likely needs work for semantic model
// PROTOTYPE likely needs work for analyzers
{
if (associatedSymbol is IMethodSymbol ctor)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1465,7 +1465,7 @@ static ImmutableArray<MethodSymbol> filterOutBadGenericMethods(

if (!typeParameters.IsEmpty)
{
if (resolution.IsExtensionMethodGroup)
if (resolution.IsExtensionMethodGroup) // PROTOTYPE we need to handle new extension methods
{
// We need to validate an ability to infer type arguments as well as check conversion to 'this' parameter.
// Overload resolution doesn't check the conversion when 'this' type refers to a type parameter
Expand Down
40 changes: 28 additions & 12 deletions src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8001,7 +8001,11 @@ private BoundExpression MakeMemberAccessValue(BoundExpression expr, BindingDiagn
if (resolution.IsNonMethodExtensionMember(out Symbol? extensionMember))
{
Debug.Assert(typeArgumentsOpt.IsDefault);
diagnostics.AddRange(resolution.Diagnostics); // PROTOTYPE test dependencies/diagnostics
if (!receiver.HasErrors)
{
diagnostics.AddRange(resolution.Diagnostics); // PROTOTYPE test dependencies/diagnostics
}

resolution.Free();

return GetExtensionMemberAccess(syntax, receiver, extensionMember, diagnostics);
Expand All @@ -8028,6 +8032,7 @@ private BoundExpression GetExtensionMemberAccess(SyntaxNode syntax, BoundExpress
return BindPropertyAccess(syntax, receiver, propertySymbol, diagnostics, LookupResultKind.Viable, hasErrors: false);

case ExtendedErrorTypeSymbol errorTypeSymbol:
// PROTOTYPE we should likely reduce (ie. do type inference and substitute) the candidates (like ToBadExpression)
return new BoundBadExpression(syntax, LookupResultKind.Viable, errorTypeSymbol.CandidateSymbols!, [receiver], CreateErrorType());

default:
Expand Down Expand Up @@ -8589,15 +8594,12 @@ static bool tryResolveExtensionInScope(
result = default;

// 1. gather candidates
scope.Binder.LookupExtensionMembersInSingleBinder(
lookupResult, left.Type, memberName, arity,
basesBeingResolved: null, lookupOptions, originalBinder: binder, ref useSiteInfo);

CompoundUseSiteInfo<AssemblySymbol> classicExtensionUseSiteInfo = binder.GetNewCompoundUseSiteInfo(diagnostics);
binder.LookupExtensionMethods(classicExtensionLookupResult, scope, memberName, arity, ref classicExtensionUseSiteInfo);
diagnostics.Add(expression, classicExtensionUseSiteInfo);
scope.Binder.LookupAllExtensionMembersInSingleBinder(
lookupResult, memberName, arity, lookupOptions,
originalBinder: binder, useSiteInfo: ref useSiteInfo, classicExtensionUseSiteInfo: ref classicExtensionUseSiteInfo);

lookupResult.MergeEqual(classicExtensionLookupResult);
diagnostics.Add(expression, classicExtensionUseSiteInfo);

if (!lookupResult.IsMultiViable)
{
Expand Down Expand Up @@ -8694,12 +8696,26 @@ static MethodGroupResolution resolveMethods(
// we can still prune the inapplicable extension methods using the receiver type
for (int i = methodGroup.Methods.Count - 1; i >= 0; i--)
{
if (methodGroup.Methods[i].IsExtensionMethod
&& (object)methodGroup.Methods[i].ReduceExtensionMethod(left.Type, binder.Compilation) == null)
MethodSymbol method = methodGroup.Methods[i];
TypeSymbol? receiverType = left.Type;
Debug.Assert(receiverType is not null);

bool inapplicable = false;
if (method.IsExtensionMethod
&& (object)method.ReduceExtensionMethod(receiverType, binder.Compilation) == null)
{
inapplicable = true;
}
else if (method.GetIsNewExtensionMember()
&& SourceNamedTypeSymbol.GetCompatibleSubstitutedMember(binder.Compilation, method, receiverType) == null)
{
inapplicable = true;
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 18, 2025

Choose a reason for hiding this comment

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

inapplicable = true;

Are we testing this code path? #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.

Yes. The interesting test is PropertyAccess_NotAmbiguousWithInapplicableMethod

}

if (inapplicable)
{
methodGroup.Methods.RemoveAt(i);
}
// PROTOTYPE we'll want to do the same for new extension methods
}

if (methodGroup.Methods.Count != 0)
Expand Down Expand Up @@ -11012,7 +11028,7 @@ static bool isCandidateUnique(ref MethodSymbol? method, MethodSymbol candidate)
foreach (var scope in new ExtensionScopes(this))
{
methodGroup.Clear();
PopulateExtensionMethodsFromSingleBinder(scope, methodGroup, node.Syntax, receiver, node.Name, typeArguments, BindingDiagnosticBag.Discarded);
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)
{
Expand Down
80 changes: 58 additions & 22 deletions src/Compilers/CSharp/Portable/Binder/Binder_Lookup.cs
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,19 @@ internal void LookupSymbolsSimpleName(
}
}

internal void LookupExtensionMethods(LookupResult result, string name, int arity, LookupOptions options, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
#nullable enable
internal void LookupAllExtensions(LookupResult result, string? name, LookupOptions options)
{
var discardedUseSiteInfo = CompoundUseSiteInfo<AssemblySymbol>.Discarded;

foreach (var scope in new ExtensionScopes(this))
{
this.LookupExtensionMethodsInSingleBinder(scope, result, name, arity, options, ref useSiteInfo);
scope.Binder.LookupAllExtensionMembersInSingleBinder(
result, name, arity: 0, options: options,
originalBinder: this, useSiteInfo: ref discardedUseSiteInfo, classicExtensionUseSiteInfo: ref discardedUseSiteInfo);
}
}
#nullable disable

/// <summary>
/// Look for any symbols in scope with the given name and arity.
Expand Down Expand Up @@ -177,34 +183,63 @@ protected void LookupMembersInternal(LookupResult result, NamespaceOrTypeSymbol
}

#nullable enable
private void LookupExtensionMembersInSingleBinder(LookupResult result, TypeSymbol receiverType,
string name, int arity, ConsList<TypeSymbol>? basesBeingResolved, LookupOptions options,
Binder originalBinder, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
private void LookupAllExtensionMembersInSingleBinder(LookupResult result, string? name,
int arity, LookupOptions options, Binder originalBinder, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo, ref CompoundUseSiteInfo<AssemblySymbol> classicExtensionUseSiteInfo)
{
Debug.Assert(name is not null);
var singleLookupResults = ArrayBuilder<SingleLookupResult>.GetInstance();
EnumerateAllExtensionMembersInSingleBinder(singleLookupResults, name, arity, options, originalBinder, ref useSiteInfo, ref classicExtensionUseSiteInfo);
foreach (var singleLookupResult in singleLookupResults)
{
result.MergeEqual(singleLookupResult);
}

var extensions = ArrayBuilder<NamedTypeSymbol>.GetInstance();
singleLookupResults.Free();
}

Debug.Assert(!receiverType.IsDynamic());
if (!receiverType.IsErrorType())
internal void EnumerateAllExtensionMembersInSingleBinder(ArrayBuilder<SingleLookupResult> result,
string? name, int arity, LookupOptions options, Binder originalBinder, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo, ref CompoundUseSiteInfo<AssemblySymbol> classicExtensionUseSiteInfo)
{
// 1. Collect new extension members
PooledHashSet<MethodSymbol>? implementationsToShadow = null;
var extensionDeclarations = ArrayBuilder<NamedTypeSymbol>.GetInstance();
this.GetExtensionDeclarations(extensionDeclarations, originalBinder);

foreach (NamedTypeSymbol extensionDeclaration in extensionDeclarations)
{
this.GetExtensionDeclarations(extensions, originalBinder);
var candidates = name is null ? extensionDeclaration.GetMembers() : extensionDeclaration.GetMembers(name);

foreach (var candidate in candidates)
{
SingleLookupResult resultOfThisMember = originalBinder.CheckViability(candidate, arity, options, null, diagnose: true, useSiteInfo: ref useSiteInfo);
result.Add(resultOfThisMember);

if (candidate is MethodSymbol { IsStatic: false } shadows &&
shadows.OriginalDefinition.TryGetCorrespondingExtensionImplementationMethod() is { } toShadow)
{
implementationsToShadow ??= PooledHashSet<MethodSymbol>.GetInstance();
implementationsToShadow.Add(toShadow);
}
}
}

var tempResult = LookupResult.GetInstance();
foreach (NamedTypeSymbol extension in extensions)
{
// No need for "diagnose" since we discard the lookup results (and associated diagnostic info)
// unless the results are good.
LookupMembersInClass(tempResult, extension, name, arity, basesBeingResolved,
options, originalBinder, diagnose: false, ref useSiteInfo);
extensionDeclarations.Free();

// 2. Collect classic extension methods
var extensionMethods = ArrayBuilder<MethodSymbol>.GetInstance();
this.GetCandidateExtensionMethods(extensionMethods, name, arity, options, originalBinder: originalBinder);

result.MergeEqual(tempResult);
tempResult.Clear();
foreach (var method in extensionMethods)
{
// Prefer instance extension declarations vs. their implementations
if (implementationsToShadow is null || !implementationsToShadow.Remove(method.OriginalDefinition))
{
SingleLookupResult resultOfThisMember = originalBinder.CheckViability(method, arity, options, null, diagnose: true, useSiteInfo: ref classicExtensionUseSiteInfo);
result.Add(resultOfThisMember);
}
}

tempResult.Free();
extensions.Free();
extensionMethods.Free();
implementationsToShadow?.Free();
}
#nullable disable

Expand Down Expand Up @@ -484,6 +519,7 @@ private static void LookupMembersInNamespace(LookupResult result, NamespaceSymbo
}
}

// PROTOTYPE we should be able to remove this method once all the callers are updated to account for new extension members
/// <summary>
/// Lookup extension methods by name and arity in the given binder and
/// check viability in this binder. The lookup is performed on a single
Expand Down Expand Up @@ -1839,7 +1875,7 @@ private static bool WrongArity(Symbol symbol, int arity, bool diagnose, LookupOp
if (arity != 0 || (options & LookupOptions.AllMethodsOnArityZero) == 0)
{
MethodSymbol method = (MethodSymbol)symbol;
if (method.GetMemberTotalArity() != arity)
if (method.GetMemberArityIncludingExtension() != arity)
{
if (method.Arity == 0)
{
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Symbols.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1537,7 +1537,7 @@ private void CheckWhatCandidatesWeHave(
foreach (var scope in new ExtensionScopes(this))
{
lookupResult ??= LookupResult.GetInstance();
LookupExtensionMethods(lookupResult, scope, plainName, arity, ref useSiteInfo);
LookupExtensionMethods(lookupResult, scope, plainName, arity, ref useSiteInfo); // PROTOTYPE account for new extension members

if (lookupResult.IsMultiViable)
{
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/LookupOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,9 @@ internal enum LookupOptions
UseBaseReferenceAccessibility = 1 << 9,

/// <summary>
/// Include extension methods.
/// Include extension members.
/// </summary>
IncludeExtensionMethods = 1 << 10,
IncludeExtensionMembers = 1 << 10,

/// <summary>
/// Consider only attribute types.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,41 +47,15 @@ internal void PopulateWithExtensionMethods(
this.PopulateHelper(receiverOpt, resultKind, error);
this.IsExtensionMethodGroup = true;

PooledHashSet<MethodSymbol> implementationsToShadow = null;
Copy link
Member Author

@jcouv jcouv Mar 16, 2025

Choose a reason for hiding this comment

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

📝 this deduplication between skeleton and implementation method is now handled upstream, in EnumerateAllExtensionMembersInSingleBinder #Resolved


if (members.Any(static m => m is MethodSymbol { IsExtensionMethod: true }) &&
members.Any(static m => m is MethodSymbol { IsExtensionMethod: false, IsStatic: false }))
{
implementationsToShadow = PooledHashSet<MethodSymbol>.GetInstance();

foreach (var member in members)
{
if (member is MethodSymbol { IsExtensionMethod: false, IsStatic: false } shadows &&
shadows.OriginalDefinition.TryGetCorrespondingExtensionImplementationMethod() is { } toShadow)
{
implementationsToShadow.Add(toShadow);
}
}
}

foreach (var member in members)
{
if (member is MethodSymbol method)
{
Debug.Assert(method.IsExtensionMethod || method.GetIsNewExtensionMember());

// Prefer instance extension declarations vs. their implementations
if (method.IsExtensionMethod && implementationsToShadow?.Remove(method.OriginalDefinition) == true)
{
continue;
}

this.Methods.Add(method);
}
}

implementationsToShadow?.Free();

if (!typeArguments.IsDefault)
{
this.TypeArguments.AddRange(typeArguments);
Expand Down
Loading
Loading