Go to base: support metadata references and bug fixes by ivanbasov · Pull Request #39055 · dotnet/roslyn

ivanbasov · 2019-10-04T01:07:20Z

Support Go to base in case with bases is in metadata.
Removes interfaces implemented by hidden by new/Shadows members from the base hierarchy, if the interface is not implemented below the hide. For example:

interface I { void M(); }
class C : I { public virtual void M(); }
class D : C { public new void M(); }

here

I i = new D();
i.M();

calls C.M() not D.M(). Therefore, I.M() is not a base for D.M(). Fixed this.
3. Fixed a issue with overloads and interfaces.

ivanbasov · 2019-10-04T01:08:56Z

@olegtk could you please review dcb65ae ?
Is it the right way to navigate from table entries? #Resolved

CyrusNajmabadi · 2019-10-04T23:47:33Z

src/EditorFeatures/Core/GoToBase/AbstractGoToBaseService.cs

+
+            var found = false;
+
+            foreach (var baseSymbol in bases)


doc what this is doing. #Resolved

CyrusNajmabadi · 2019-10-04T23:48:10Z

src/EditorFeatures/Test2/GoToBase/CSharpGoToBaseTests.vb

        <Fact, Trait(Traits.Feature, Traits.Features.GoToBase)>
        Public Async Function TestWithSingleClass() As Task
-            Await TestAsync("class $$C { }")
+            Await TestAsync("class $$C { }", , metadataDefinitions:={"mscorlib:Object"})


not a fan of passing an omitted arg. #Resolved

CyrusNajmabadi · 2019-10-04T23:49:18Z

src/VisualStudio/Core/Def/Implementation/FindReferences/Entries/MetadataDefinitionItemEntry.cs

+                    var inlines = DefinitionBucket.DefinitionItem.DisplayParts
+                        .ToInlines(_presenter.ClassificationFormatMap, _presenter.TypeMap);
+
+                    var textBlock = inlines.ToTextBlock(_presenter.ClassificationFormatMap, wrap: false);


is this copying code elsewhere? can we share? #Resolved

CyrusNajmabadi · 2019-10-04T23:49:43Z

src/VisualStudio/Core/Def/Implementation/FindReferences/TableEntriesSnapshot.cs

+                {
+                    content = _entries[index];
+                    return true;
+                }


who calls into this with this value? #Resolved

CyrusNajmabadi · 2019-10-04T23:49:55Z

src/Workspaces/Core/Portable/FindSymbols/FindReferences/BaseTypeFinder.cs

-            return await ConvertToSymbolAndProjectIdsAsync(typesAndInterfaces.CastArray<ISymbol>(), project, cancellationToken).ConfigureAwait(false);
-        }
+        public static ImmutableArray<ISymbol> FindBaseTypesAndInterfaces(
+            INamedTypeSymbol type)


move to prevous line. #Resolved

ivanbasov · 2019-10-08T23:00:21Z

Thank you for the review, @CyrusNajmabadi ! Feedback addressed.

@jasonmalinowski and @JoeRobich , please take a look. #Resolved

JoeRobich

LGTM

jasonmalinowski

@ivanbasov I know I'm late to the party, but looking at the PR I'm wondering if there's actually a compiler bug around the "new"/hidden method case? Or are we misunderstanding something else here about the language behavior?

jasonmalinowski · 2019-10-15T23:11:00Z

src/EditorFeatures/Test2/GoToHelpers/GoToHelpers.vb

                Next
+
+                Dim actualDefintionsWithoutSpans = context.GetDefinitions().
+                Where(Function(d) d.SourceSpans.IsDefaultOrEmpty).


Indenting is off here, and usually we always do a _ at the end of a previous line here so the . stays on the next line. #Resolved

jasonmalinowski · 2019-10-15T23:28:50Z

src/EditorFeatures/Core/GoToBase/AbstractGoToBaseService.cs

+            var found = false;
+
+            // For each potential base, try to find its definition in sources.
+            // If found, add its' definitionItem to the context.


"it's" (and everywhere else) #Resolved

jasonmalinowski · 2019-10-15T23:31:27Z

src/EditorFeatures/Core/GoToBase/AbstractGoToBaseService.cs

+                var sourceDefinition = await SymbolFinder.FindSourceDefinitionAsync(
+                   SymbolAndProjectId.Create(baseSymbol, projectId), solution, cancellationToken).ConfigureAwait(false);
+                if (sourceDefinition.Symbol != null &&
+                    sourceDefinition.Symbol.Locations.Any(l => l.IsInSource))


FindSourceDefinitionAsync should only be returning symbols from source in the first place... #Resolved

jasonmalinowski · 2019-10-15T23:34:47Z

src/EditorFeatures/Test2/GoToBase/VisuaBasicGoToBaseTests.vb

        Public Async Function TestWithVirtualMethodHiddenAndInterfaceImplementedOnDerivedType() As Task
-            ' We should not find a hidden method.
+            ' We should not find hidden methods.
+            ' We should not find methods of interfaces no implemented by the method symbol.


"not"? #Resolved

jasonmalinowski · 2019-10-15T23:37:00Z

src/EditorFeatures/Test2/GoToHelpers/GoToHelpers.vb

+                    metadataDefinitions = {}
+                End If
+
+                Assert.Equal(actualDefintionsWithoutSpans.Count, metadataDefinitions.Count)


Just do Assert.Equal -- there's an overload that compares to IEnumerables item by item, and if the count differs will also show you the actual contents which is a lot easier to debug than this. #Resolved

jasonmalinowski · 2019-10-15T23:42:42Z

src/Workspaces/Core/Portable/FindSymbols/FindReferences/BaseTypeFinder.cs

            {
                foreach (var member in type.GetMembers(symbol.Name))
                {
-                    var sourceMember = await SymbolFinder.FindSourceDefinitionAsync(


Now that this returns metadata symbols, do we expose this anywhere as a public API that this is a breaking change of? #Resolved

We didn't change FindSourceDefinitionAsync. Only in case, where we cannot get sources by it; we go to metadata.

In reply to: 335223864 [](ancestors = 335223864)

Methods changed belong to internal classes and should not be involved in public APIs

In reply to: 335726193 [](ancestors = 335726193,335223864)

jasonmalinowski · 2019-10-15T23:46:09Z

src/Workspaces/Core/Portable/FindSymbols/FindReferences/BaseTypeFinder.cs

-                        // We need to start from each base class for cases like N() Implements I.M() 
-                        // where N() can be hidden or overwritten in a nested class later on.
-                        interfaceImplementations.AddRange(member.ExplicitOrImplicitInterfaceImplementations());
+                        // We should add implementations only for overridden members but not for hidden ones.


I'm not understanding how this is working now: if you just have the simple case of:

interface I { void M(); } class A : I { public void M(); }

M() isn't an override of anything, but it implements A -- is the SymbolFinder.IsOverride still being fired? #Resolved

In the case, we will add the interface method above on line 22. Then, we will find that A is inherited from object. We will try to find "M" in the object but will not find it here and will not call SymbolFinder.IsOverride.

In reply to: 335224638 [](ancestors = 335224638)

jasonmalinowski · 2019-10-15T23:46:26Z

src/Workspaces/Core/Portable/FindSymbols/FindReferences/BaseTypeFinder.cs

+                        // will call the method from B. We should find the base for B.M in this case.
+                        // And if we change 'new' to 'override' in the original code and add 'virtual' where needed, 
+                        // we should find I.M as a base for B.M(). And the next line helps with this scenario.
+                        results.AddRange(member.ExplicitOrImplicitInterfaceImplementations());


I'm worried this is actually a bug in ExplicitOrImplicitInterfaceImplementations:

roslyn/src/Workspaces/Core/Portable/Shared/Extensions/ISymbolExtensions.cs

Line 121 in 1de29ff

memberInInterface => symbol.Equals(containingType.FindImplementationForInterfaceMember(memberInInterface)));

It looks like this is finding B, seeing it implements I, and then asks I what the explicit implementation is in B -- is it returning A.M or nothing at all? #Resolved

I think that Explicit in this context means VB implementations and C# ones like void I.M(). And implicit are all other.

Then, in the scenario
abstract class C : I { public abstract void |M| { } }
class D : C { public override void $$M() { } }
interface I { void |M|; }

In the line you mentioned
containingType.FindImplementationForInterfaceMember(memberInInterface))
is called for containingType = "D" and memberInInterface = "I.M()".

And it returns "C.M()" not "D.M()". Then it is rejected by symbol.Equals.

In reply to: 335224689 [](ancestors = 335224689)

jasonmalinowski · 2019-10-15T23:49:32Z

src/VisualStudio/Core/Def/Implementation/FindReferences/Entries/AbstractItemEntry.cs

+    {
+        private abstract class AbstractItemEntry : Entry
+        {
+            protected readonly StreamingFindUsagesPresenter _presenter;


Protected members shouldn't be prefixed with _ -- make this a property. #Resolved

jasonmalinowski · 2019-10-15T23:51:29Z

src/VisualStudio/Core/Def/Implementation/FindReferences/TableEntriesSnapshot.cs

    internal partial class StreamingFindUsagesPresenter
    {
+        // Name of the key used to retireve the whole entry object.
+        internal const string SelfKeyName = "self";


Is this a constant being shared with something else? #Resolved

It is just introduced and shared between FindUsagesTableControlEventProcessorProvider and StreamingFindUsagesPresenter

In reply to: 335225827 [](ancestors = 335225827)

ivanbasov added 9 commits September 30, 2019 17:56

extend test classes to support goto metadata locations

e989b79

tests for goto base to metadata locations

db422b2

tests for overloads

24369eb

correcting tests for new/shadow

e57c789

gotobase - fixing bugs

5432766

fix test harness for metadata references in gotobase

e17e82f

support bases for classes and structs in tests

b37b94b

refactoring

d88b0fa

UI support for displaying metadata references

dcb65ae

ivanbasov requested review from a team, JoeRobich, jasonmalinowski and olegtk October 4, 2019 01:07

ivanbasov added the Area-IDE label Oct 4, 2019

ivanbasov added this to the 16.4.P3 milestone Oct 4, 2019

olegtk approved these changes Oct 4, 2019

View reviewed changes

Merge branch 'master' into GoToBase

fdf4947

CyrusNajmabadi reviewed Oct 4, 2019

View reviewed changes

ivanbasov added 2 commits October 8, 2019 15:58

code review feedback

a966c1f

refactiring item entries

2c13e88

JoeRobich approved these changes Oct 10, 2019

View reviewed changes

ivanbasov merged commit 94afbe5 into dotnet:master Oct 10, 2019

ivanbasov deleted the GoToBase branch October 10, 2019 22:03

jasonmalinowski reviewed Oct 15, 2019

View reviewed changes

ivanbasov mentioned this pull request Oct 16, 2019

follow up for Go to base: support metadata references and bug fixes #39334

Merged

Conversation

ivanbasov commented Oct 4, 2019

Uh oh!

ivanbasov commented Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi Oct 4, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Oct 4, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Oct 4, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Oct 4, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Oct 4, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivanbasov commented Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoeRobich left a comment

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Oct 15, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Oct 15, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Oct 15, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Oct 15, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Oct 15, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Oct 15, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivanbasov Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

ivanbasov Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Oct 15, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivanbasov Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Oct 15, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivanbasov Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Oct 15, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

ivanbasov commented Oct 4, 2019 •

edited

Loading

CyrusNajmabadi Oct 4, 2019 •

edited by ivanbasov

Loading

CyrusNajmabadi Oct 4, 2019 •

edited by ivanbasov

Loading

CyrusNajmabadi Oct 4, 2019 •

edited by ivanbasov

Loading

CyrusNajmabadi Oct 4, 2019 •

edited by ivanbasov

Loading

CyrusNajmabadi Oct 4, 2019 •

edited by ivanbasov

Loading

ivanbasov commented Oct 8, 2019 •

edited

Loading

jasonmalinowski Oct 15, 2019 •

edited by ivanbasov

Loading

jasonmalinowski Oct 15, 2019 •

edited by ivanbasov

Loading

jasonmalinowski Oct 15, 2019 •

edited by ivanbasov

Loading

jasonmalinowski Oct 15, 2019 •

edited by ivanbasov

Loading

jasonmalinowski Oct 15, 2019 •

edited by ivanbasov

Loading

jasonmalinowski Oct 15, 2019 •

edited by ivanbasov

Loading

jasonmalinowski Oct 15, 2019 •

edited by ivanbasov

Loading

jasonmalinowski Oct 15, 2019 •

edited by ivanbasov

Loading

jasonmalinowski Oct 15, 2019 •

edited by ivanbasov

Loading

jasonmalinowski Oct 15, 2019 •

edited by ivanbasov

Loading