Skip to content
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

Update the Find-References feature to only find inheritance results that could lead to a specific member being used at runtime. #51637

Merged
13 commits merged into from
Mar 5, 2021

Conversation

CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented Mar 3, 2021

Previously, 'Find References', which has both an programmatic API (FAR-API), and an end user facing feature (FAR-feature) would walk unilaterally through the inheritance hierarchy for a member with a particular. So, if you had:

class C { public virtual void Foo() {} }
class D1 : C { public override void Foo() { } }
class D2 : C { public override void Foo() { } }

Then doing a Find-References on D1.Foo, would find references to all three Foo methods shown there. This was non ideal if you consider this sort of case:

D2 d = new D2();
d.Foo();

Here, this call to Foo can never, and will never call into D1.Foo(). Any call through it will never reach D1's Foo method. Contrast this with:

C c = ...;
c.Foo();

Here, c.Foo() could call into D1.Foo if c is an instance of D1 at runtime.

The reason FAR-feature worked this way, is that it calls FAR-API to get the results. This FAR-API was designed for needs of consumers like Rename. In Rename, if you rename any of these three methods, then all have to be renamed as there is a coupling here between the members in terms of satisfying interface implementations. That coupling goes beyond what FAR-feature needs, but it caused the suboptimal behavior seen above.

This change introduces a new concept to FAR-API where the client can specify if they want this behavior or not. Existing clients (like rename, or 3rd parties) will get the same results as before. However, clients (like FAR-feature, highlight-refs, and CodeLens) can opt into new behavior where we only find references that could actually call into that member at runtime.

This also has added benefits for performance as things like codelens (and FAR-feature, and highlight-refs) wont' cascade to many symbols that aren't needed when computing these results.

CancellationToken cancellationToken)
{
// Static methods can't cascade.
if (!symbol.IsStatic)
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 is the meat of hte change. might be easier to read with whitespace changes off.

Base automatically changed from master to main March 3, 2021 23:53
symbol, document, cancellationToken).ConfigureAwait(false));
var cascaded = await changeSignatureService.DetermineCascadedSymbolsFromDelegateInvokeAsync(
symbol, document, cancellationToken).ConfigureAwait(false);
result.AddRange(cascaded.SelectAsArray(s => (s, cascadeDirection)));
Copy link
Member

Choose a reason for hiding this comment

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

💡 The SelectAsArray isn't helping here. It would be better to either use Select, or an extension that allows AddRange with a transformation argument.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Auto-approval

@ghost ghost merged commit 628d74c into dotnet:main Mar 5, 2021
@CyrusNajmabadi CyrusNajmabadi deleted the farDirection branch March 5, 2021 22:54
@allisonchou allisonchou added this to the Next milestone Mar 12, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants