Make more dependent-type-finder operations public through SymbolFinder. by CyrusNajmabadi · Pull Request #43622 · dotnet/roslyn

CyrusNajmabadi · 2020-04-23T22:06:52Z

~~Followup to #43593. That PR should go in first before this is reviewed.~~

This is a precursor to moving these operations OOP.

This shoudl be reviewed one commit at a time.

…ns helper

…ymbols

CyrusNajmabadi · 2020-04-29T18:43:13Z

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

            Solution solution,
            IImmutableSet<Project> projects,
-            bool transitive,
-            CancellationToken cancellationToken)


renamed these to 'Descend' or 'Add' to better help explain what they're doing. 'Find' was fairly generic.

This helped, thanks!

CyrusNajmabadi · 2020-04-29T19:46:57Z

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

            Solution solution,
            IImmutableSet<Project> projects,
-            bool transitive,
-            CancellationToken cancellationToken)


renamed this for clarity. 'Find' was a little vague. i think 'DescendInheritanceTree' gives more of an intuition about what is going on here.

jasonmalinowski · 2020-04-29T22:24:34Z

...Workspaces/Core/Portable/FindSymbols/FindReferences/DependentTypeFinder_ImplementingTypes.cs

+                // Only classes/struct implement interface types.  Derived interfaces can be found with
+                // FindDerivedInterfacesAsync.
+                return allTypes.WhereAsArray(t => t.TypeKind == TypeKind.Class || t.TypeKind == TypeKind.Struct);


Enums or delegates don't count? C# interactive says:

> enum E { A, B, C } > E.A is IComparable true

Although I could imagine there's some complicated conversion going on and it strictly not an implementation.

Very interesting case! It will come down to: does the compiler model this in the .Interfaces list of the enum. if so, then i should find these. if not, then i think it's fine not to.

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

jasonmalinowski · 2020-04-29T22:43:13Z

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

+                    if (!(reference is PortableExecutableReference peReference))
+                        continue;


Why? I see AddMatchingMetadataTypesInMetadataReferenceAsync is requiring a PortableExecutableReference for the index, but if there's other types of references do we still need to process those without the index?

Good question. The short answer is: no, there are no other types of refernces we need to process.

The long answer is:

There are effectively two types of references we ever care about "CompilationReference" and "PEReference". The latter is what we traditionally consider as a DLL, whereas the former is for a P2P reference. For the former, we're already processing the other projects higher up directly as we walk that project, so we don't want to walk again through the compilation reference.

jasonmalinowski · 2020-04-29T22:59:18Z

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

@@ -631,61 +521,37 @@ private static async Task FindImmediateMatchingMetadataTypesInMetadataReferenceA
                // our criteria.


So it seems a bit funky to me that it seems like we lost the information in the index about how it was related and now we're trying to recover that information again. Was there a specific reason for that?

great question! first, know that this is an index over metadata, and that index is built independent of that DLL's context in some containing project. THe way the index is built is to actually crack it open directly, read the tables and build information out of that. So the index doesn't store "this ISymbol derives from this other ISymbol", instead it stores "I remember that a type called 'FileStream' derived from a type called 'Stream'".

The goal of hte index was to be fast and lightweight and unencumbered from concerns like retargetting. Instead, it just stores basic data pulled out of the metadata tables in a lightweight fashion.

It's actually just like how the source-index works, just a little faster and more accurate because we can look at real md tokens.

jasonmalinowski · 2020-04-29T23:03:55Z

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

            Solution solution,
            IImmutableSet<Project> projects,
-            bool transitive,
-            CancellationToken cancellationToken)


This helped, thanks!

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

jasonmalinowski · 2020-04-29T23:26:31Z

src/Workspaces/Core/Portable/FindSymbols/SymbolFinder_Hierarchy.cs

-            return ImmutableArray<ISymbol>.Empty;
+            var types = await DependentTypeFinder.FindAndCacheDerivedClassesAsync(
+                type, solution, projects, transitive, cancellationToken).ConfigureAwait(false);
+            return types.WhereAsArray(t => IsAccessible(t));


Why is the accessibility check out here rather than inside the DependentTypeFinder code?

that's a good question.... let me noodle on it. This maintains the semantics from before, but i think we could potentially safely move this down.

CyrusNajmabadi

c

sharwell · 2020-04-30T13:28:59Z

...rkspaces/Core/Portable/FindSymbols/FindReferences/DependentTypeFinder.KeyEqualityComparer.cs

+        /// Special comparer we need for our cache keys.  Necessary because <see cref="IImmutableSet{T}"/> uses
+        /// reference equality and not value-equality semantics.


@CyrusNajmabadi ImmutableHashSet<T> uses EqualityComparer<T>.Default unless some other comparer has been specified.

Can you point me to where you see that being done for testing if two sets are .equals? Note that this is not about the elements of the set themselves but about checking if the two entire sets are. Equals when used as part of a tuple.

sharwell · 2020-04-30T13:30:44Z

@CyrusNajmabadi @jinujoseph Can we port this one to 16.7 Preview 1? Otherwise it will be a dogfood blocker for me and I'll be stuck on 16.6 until 16.7 Preview 2 is released.

CyrusNajmabadi · 2020-04-30T18:03:15Z

Sure, I can do that if @jinujoseph is ok with it

jinujoseph · 2020-04-30T18:05:40Z

@CyrusNajmabadi @sharwell Do you anticipate any risk with this change?

CyrusNajmabadi · 2020-04-30T18:13:14Z

Yes. There's definitely some risk here. This changes a bunch of core algorithms that many other features layer on top of. We have lots of testing, but risk is always there

jinujoseph · 2020-04-30T18:42:46Z

@CyrusNajmabadi let's keep in master then.
@sharwell sorry you may have to update your extension in the meantime.

CyrusNajmabadi added 19 commits April 23, 2020 12:10

Extract find-callers code into its own partial type

c972b06

Extract symbolfinder helpers into their own partial type

2917008

Add FindImmediatelyDerivedClassesAsync to the public SymbolFinder api

16c1c78

Add strongly typed overload to help with common case

c194be7

Add the 'immediate' counterpart to the SymbolFinder.FindImplementatio…

17fe51c

…ns helper

Move code for handling derived classes into its own part.

01f11ad

Move code for finding implementations into its own part.

8dc35c6

Rename for clarity

024b38e

Fix

57ed185

Simplify implementation

01d3d3b

Simplify impl for clarity

4ac672c

Simplify and cache teh delegates we use for walking source/metadata s…

3bcd914

…ymbols

Add comment

93ada01

Add helpers to find derived interfaces

0aabcb0

Don't search so far when finding implementations of interface members

05421ad

Merge branch 'renameOOP7' into depTypeFinder

bc83df9

Remove unused types

3868510

Remove unused member

5b02094

Update tests.

8570865

CyrusNajmabadi marked this pull request as ready for review April 23, 2020 22:23

CyrusNajmabadi requested a review from a team as a code owner April 23, 2020 22:23

CyrusNajmabadi requested a review from a team April 23, 2020 22:23

CyrusNajmabadi changed the title ~~WIP: Make more dependent-type-finder operations public through SymbolFinder.~~ Make more dependent-type-finder operations public through SymbolFinder. Apr 23, 2020

CyrusNajmabadi added 7 commits April 23, 2020 15:27

Do not call directly into DependentTypeFinder. Use SymbolFinder instead.

03e1073

Inline method.

e37fa96

Fix caching for when project sets are passed along.

5ebb65c

Rename methods

0e6855d

Break out functionality.

b5ac83a

Merge remote-tracking branch 'upstream/master' into depTypeFinder

8ed3bf8

Have to checked transitively derived classes as well

440395c

CyrusNajmabadi added 11 commits April 29, 2020 10:03

Add tests

7b54e7f

Remove delegate type

6e4766a

Simplify

9b4f5dc

Add comment

107e78e

Make pattern clearer

ef0b2ac

Cleanup

ddcfb66

renames

3f30b1d

renames

4fb5474

Extract type

a7afaa6

Simplify

c9ec97d

Add docs

4889891

CyrusNajmabadi commented Apr 29, 2020

View reviewed changes

CyrusNajmabadi added 2 commits April 29, 2020 11:45

Remove allocation

621acc1

Rename

ff3f8e0

CyrusNajmabadi commented Apr 29, 2020

View reviewed changes

CyrusNajmabadi requested a review from jasonmalinowski April 29, 2020 22:15

jasonmalinowski approved these changes Apr 30, 2020

View reviewed changes

CyrusNajmabadi commented Apr 30, 2020

View reviewed changes

CyrusNajmabadi merged commit 83cd26a into dotnet:master Apr 30, 2020

ghost added this to the Next milestone Apr 30, 2020

CyrusNajmabadi deleted the depTypeFinder branch April 30, 2020 00:30

sharwell reviewed Apr 30, 2020

View reviewed changes

jinujoseph added the Area-IDE label Apr 30, 2020

JoeRobich modified the milestones: Next, 16.7.P2 May 18, 2020

		if (!(reference is PortableExecutableReference peReference))
		continue;

		@@ -631,61 +521,37 @@ private static async Task FindImmediateMatchingMetadataTypesInMetadataReferenceA
		// our criteria.

		/// Special comparer we need for our cache keys. Necessary because <see cref="IImmutableSet{T}"/> uses
		/// reference equality and not value-equality semantics.

Conversation

CyrusNajmabadi commented Apr 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell commented Apr 30, 2020

Uh oh!

CyrusNajmabadi commented Apr 30, 2020

Uh oh!

jinujoseph commented Apr 30, 2020

Uh oh!

CyrusNajmabadi commented Apr 30, 2020

Uh oh!

jinujoseph commented Apr 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CyrusNajmabadi commented Apr 23, 2020 •

edited

Loading