Skip to content

Comments

Extensions: FindReferences should link extension members and implementations#81685

Merged
jcouv merged 7 commits intodotnet:mainfrom
jcouv:extension-far
Dec 16, 2025
Merged

Extensions: FindReferences should link extension members and implementations#81685
jcouv merged 7 commits intodotnet:mainfrom
jcouv:extension-far

Conversation

@jcouv
Copy link
Member

@jcouv jcouv commented Dec 14, 2025

Addresses part of #81507

{
// If the given symbol is an extension member, cascade to its implementation method
if (symbol.AssociatedExtensionImplementation is { } associatedExtensionImplementation)
result.Add(associatedExtensionImplementation);
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Dec 14, 2025

Choose a reason for hiding this comment

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

can likely inline this method, and just replace it with result.AddIfNotNull(symbol.AssociatedExtensionImplementation) #Resolved

private static void CascadeFromExtensionImplementation(IMethodSymbol symbol, ArrayBuilder<ISymbol> result)
{
// If the given symbol is an implementation method of an extension member, cascade to the extension member itself
var containingType = symbol.ContainingType;
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Dec 14, 2025

Choose a reason for hiding this comment

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

since we're going from teh classic Foo(this whatever) method to the modern extension, can't you just check if the .MethodKind is Extension here instead of the MightContainExtensionMethods bit? #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.

The implementation method is not necessarily an extension method. It's only an extension method if the extension member is a non-static extension method.
If it's a static extension method or a property accessor, the implementation method is not an extension method.
So you can't do 42.get_P() for instance, you can only do E.get_P(42).

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we at least check and only do this if the member is implicitly declared by the compiler? It feels weird to do this search for all members, including normal user ones.

if (containingType is null || !containingType.MightContainExtensionMethods || !symbol.IsStatic)
return;

var implementationDefinition = symbol.OriginalDefinition;
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Dec 14, 2025

Choose a reason for hiding this comment

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

i don't think you need to do .OriginalDefinition within the REfFinder code. Everything is OriginalDefs here. #Resolved

Comment on lines 68 to 72
if (associated is null)
continue;

if (!object.ReferenceEquals(associated.OriginalDefinition, implementationDefinition))
continue;
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Dec 14, 2025

Choose a reason for hiding this comment

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

Suggested change
if (associated is null)
continue;
if (!object.ReferenceEquals(associated.OriginalDefinition, implementationDefinition))
continue;
if (!Equals(associated, implementationDefinition))
continue;

(we don't use RefEquals or == for symbols in IDE layer).
#Resolved

result.Add(method);
}
}
}
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Dec 14, 2025

Choose a reason for hiding this comment

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

note: having this reverse mapping be available from the copielr would be nice (fi the methods are linked at that level). having to do the search like this feels not great. of course, if it's not available, this is fine. #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.

Such mapping isn't available (the compiler doesn't keep a map to easily expose a public API for this)

Copy link
Member Author

@jcouv jcouv Dec 14, 2025

Choose a reason for hiding this comment

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

Filed tracking issue #81686 to collect usage scenarios where such an API would be useful

{
// If the given symbol is an extension accessor, cascade to its implementation method
if (symbol.AssociatedExtensionImplementation is { } associatedExtensionImplementation)
result.Add(associatedExtensionImplementation);
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Dec 14, 2025

Choose a reason for hiding this comment

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

like above. inline this and replace with:

result.AddIfNotNull(symbol.AssociatedExtensionImplementation);
``` #Resolved

Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

Signing off with feedback. Would love an actual compiler api to do some of the contorted stuff.

@jcouv jcouv marked this pull request as ready for review December 14, 2025 16:48
@jcouv jcouv requested a review from a team as a code owner December 14, 2025 16:48
@jcouv
Copy link
Member Author

jcouv commented Dec 15, 2025

@CyrusNajmabadi I'd missed operator scenarios. Added

// If the given symbol is an implementation method of an extension member, cascade to the extension member itself
var containingType = symbol.ContainingType;
if (containingType is null || !containingType.MightContainExtensionMethods || !symbol.IsStatic)
if (containingType is null || !containingType.MightContainExtensionMethods || !symbol.IsStatic || !symbol.IsImplicitlyDeclared)
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Dec 16, 2025

Choose a reason for hiding this comment

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

if (symbol is not { IsStatic: true, IsImplicitlyDeclared: true ContainingType.MightContainExtensionMethods: true })
    return;
``` #Resolved

Comment on lines 66 to 68
if (associated is null)
continue;

Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Dec 16, 2025

Choose a reason for hiding this comment

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

Suggested change
if (associated is null)
continue;
``` #Resolved

Comment on lines 30 to 31
if (!options.AssociatePropertyReferencesWithSpecificAccessor && symbol.AssociatedSymbol != null)
result.Add(symbol.AssociatedSymbol);
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Dec 16, 2025

Choose a reason for hiding this comment

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

if (!options.AssociatePropertyReferencesWithSpecificAccessor)
    result.AddIfNotNull(symbol.AssociatedSymbol)
``` #Resolved

result.Add(symbol.AssociatedSymbol);

// If the given symbol is an extension accessor, cascade to its implementation method
result.AddIfNotNull(symbol.AssociatedExtensionImplementation);
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Dec 16, 2025

Choose a reason for hiding this comment

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

what cascades abck from teh impl method to the extension accessor? #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.

That's handled by OrdinaryMethodReferenceFinder since the implementation method is an ordinary method. Then the iterative process of cascading should make the cascading transitive.


foreach (var member in nestedType.GetMembers())
{
if (member is IMethodSymbol method)
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Dec 16, 2025

Choose a reason for hiding this comment

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

is this ok that we filtered down to just methods? couldn't an impl method for a property map back to an property?

or will it map back to the accessor? which then maps back to the property? #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.

The latter. From my understanding cascading is an iterative process (see DetermineInitialSearchSymbolsAsync)

Copy link
Contributor

Choose a reason for hiding this comment

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

ok great. thanks :)

Comment on lines 86 to 92
if (symbol.AssociatedExtensionImplementation is { } associatedExtensionMethod)
{
// If given an extension operator `E.extension(...).operator+`, we cascade to the its implementation method `E.op_Addition(...)`
return new([associatedExtensionMethod]);
}

return new([]);
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Dec 16, 2025

Choose a reason for hiding this comment

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

Suggested change
if (symbol.AssociatedExtensionImplementation is { } associatedExtensionMethod)
{
// If given an extension operator `E.extension(...).operator+`, we cascade to the its implementation method `E.op_Addition(...)`
return new([associatedExtensionMethod]);
}
return new([]);
// If given an extension operator `E.extension(...).operator+`, we cascade to the its implementation method `E.op_Addition(...)`
return symbol.AssociatedExtensionImplementation is { } associatedExtensionMethod)
? new([associatedExtensionMethod])
: new([]);
``` #Resolved

@CyrusNajmabadi
Copy link
Contributor

minor cleanup feedback. overall looks great. thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants