Roslyn: FindAllReferences and GoTo results should be simpler for multiple target frameworks

[srivatsn]

_From @NickCraver on January 17, 2017 16:52_

First, kudos on this dialog in 2017. Find All References is a huge improvement. I think we can do even better with some hopefully relatively simple changes.

This is a general issue, but here are repro specs for the specific screenshot used in this example:

Steps to reproduce

1. Grab the MiniProfiler repo here: https://github.com/MiniProfiler/dotnet/tree/261d3d1bdaf4047d59f0a1eba4b3b779f26a6e84
2. Open src\MiniProfiler.Shared\MiniProfiler.cs
3. Go to Line #75 for DurationMilliseconds
4. Right click > Find All References

The "Find All References" dialog looks like this:

This is quite cluttered, and that's fo a simple project with only 2 target frameworks. Imagine more. Every framework added is effectively a multiplier for volume in the results window (adjusting for #ifdefs of course).

But there are not n copies of this code, there are 1. It should be a single result, which would be far more usable. I propose instead it groups by the existing lines/columns of code there, with target framework being a comma-delimited list. In the screenshot above, picture one set of lines of code (no changes), but instead of the headers saying this:

MiniProfiler.Shared(net45) (3)
MiniProfiler.Shared(netstandard1.5) (3)

To a single header, like this:

MiniProfiler.Shared(net45, netstandard1.5) (3)

When you click the lines of code, they all go to the same place and do the same thing. We shouldn't show them many times, it's a developer experience that could be much better.

cc @CyrusNajmabadi

_Copied from original issue: dotnet/roslyn-project-system#1212_

[srivatsn]

_From @CyrusNajmabadi on January 17, 2017 19:24_

For cases where there truly is 100% overlap I totally agree we should do something like this. But sure what the best thing is when the overlap is not 1:1 though.

This is also tricky to pull off due to the streaming nature of the feature.

However, I totally agree that these types of projects lead to a poor experience, and that we should do something about that.

[srivatsn]

_From @NickCraver on January 17, 2017 21:6_

On the < 100% case: I'd say the groups are by the lines below. If a line if inside a #ifdef NET45 for example, another group showing only "(net45)" would show, with that 1 result inside.

FWIW, I'd readily trade the streaming nature of the feature for the more usable UI in the multi-targeting case.

[NickCraver]

Here's a screenshot of what this looks like on MiniProfiler - I'd argue it's just unusably cluttered at the moment - what are the chances this sees love before release?

Why do we need all of that "No references found" noise? It's irrelevant - I don't want everything that didn't match in any result list. This is inconsistent with any other "find" dialog in the product (or any product really). You can't see from the screenshot, but there are 4 total results. There's so much irrelevant stuff shown, you can't even see that far down. There are 14 "results" shown, which means we're 4 for 14.

This type of result list really isn't that abnormal. Here are some other examples:

I'd suggest grouping by Path at the top level as well. That's often far more relevant than the current options. Current Path is only allowed as a secondary grouping. A method signature is within a file, not the other way around - so it's conceptually backwards to how it'd be used or accessed for editing. If you look at this:

You'll see that "Definition then Path" isn't correct. You get a definition per project, so that sort is actually "Project then Definition then Path", it's just not showing the Project. I consider this to be a bug, since either the grouping or the label is wrong.

Generally speaking, I don't care what project it's used in. That's told to me by the path, or I'm just looking to find all the references, regardless of where they are. So when choosing an option that doesn't include "Project" in the grouping description, it shouldn't group that way.

I'll play with this some more, but please consider giving this love before RTM. It's really bad for many projects, multiple framework targets, and interfaces right now. I'm very quickly falling back to Ctrl+Shift+F really quickly after frustration at the moment, it's unfortunately just not in a usable state.

I'd be very happy to test any builds or ideas here.

[CyrusNajmabadi]

what are the chances this sees love before release?

Zero chance :) RTM is effectively locked down.

I am interested in improving things here in teh near future though. We also welcome PRs.

[CyrusNajmabadi]

Why do we need all of that "No references found" noise? It's irrelevant

I disagree that it's irrelevant. :) There may very well be references to those items. And knowing which items are/aren't referenced can be very relevant.

Now, i do think that we should prioritize things, and place the items that are more likely to have references at the top. We can also try to do something like collapse the entries that have no references.

[CyrusNajmabadi]

You'll see that "Definition then Path" isn't correct.

Can you share your project that demonstrates this issue. It's unclear to me why the same symbol would show up twice. Unless tihs is a linked file, and then it's actually two separate symbols (each which may have many references scattered around). If that's the case, then it really is showing 'definition' and then 'path'.

[CyrusNajmabadi]

Generally speaking, I don't care what project it's used in. That's told to me by the path, or I'm just looking to find all the references, regardless of where they are. So when choosing an option that doesn't include "Project" in the grouping description, it shouldn't group that way.

I'm sorry, i'm not seeing how things are grouped by project. Here's the first grouping:

i.e. grouping by symbol (if we really do think these are two different symbols).

Then we group in this manner:

i.e. we've broken things down by path.

[CyrusNajmabadi]

or I'm just looking to find all the references, regardless of where they are

In that case, i would turn off grouping entirely. Then you'll simply get the list of all references. Note: linked files will be over represented (since each representation of hte linked file may seem to have unique references). I'm definitely open to finding a way to simplify that presentation. However, it really will be weird. A linked file does belong to multiple projects (and can have different interpretations per project). For example, in Roslyn, we share code between our compiler and IDE layers this way, and each has a different interpretation (for example, different resource strings, different #defines, etc.). We'd essentially have to create a mapping layer that says "even those these are essentially two separate files, we'll try to avoid thinking of them that way when presenting results.".

[CyrusNajmabadi]

I'd be very happy to test any builds or ideas here.

I'd be very interested in mockups showing what you expect to see with the current settings you've picked.

Thanks!

[Pilchie]

Regarding duplicate results - I suspect that @nickcraver has a .NET Core project that is cross-targeting multiple frameworks.

[NickCraver]

I disagree that it's irrelevant. :) There may very well be references to those items. And knowing which items are/aren't referenced can be very relevant.

Total disagreement here for sure - IMO they're 100% irrelevant. I searched for all usages of SetViewedAsync() on SqlServerStorage (a specific implementation of the IAsyncStorage interface (repo here: MiniProfiler). I searched for where that was used. Not every other implementation, which is what the results window actually has. If I searched from the interface, sure - I totally agree they're relevant.

Now, i do think that we should prioritize things, and place the items that are more likely to have references at the top. We can also try to do something like collapse the entries that have no references.

Maybe if there was a section for unused references (with such a heading) at the bottom - which would also greatly collapse the vertical space by nature of reducing repetition. Still, they shouldn't be there in this particular case (last comment above).

Can you share your project that demonstrates this issue.

Yep - MiniProfiler link above. I think @Pilchie has already explained the issue though. But why does it present as 2 items? It's 1 class in 1 place with 1 symbol, why do we need to split the view? See my original comments on the issue about combining all cases of this anyway (e.g. (net4.6, netstandard1.5)).

If the thinking here is that to the user the same line of code is 2 different things (or really n different things) depending on the number of build configs, that's bad. As originally in the issue, seeing n results for what is one thing is just not a usable or scalable approach - this should change, and is also solved by collapsing.

In that case, i would turn off grouping entirely.

Sure! I'd happily try this...how do I do it? Intuitively, I'm looking for a "None" option under "Group by", but that doesn't exist - is there an option somewhere else?

For info: there's no file linking happening, no special shenanigans here - you can see in the repo above. I think this repro is pretty pain in terns of project setup, happy to be shown otherwise if there's something off.

I'll try and mockup something that works (will be a bit while I get these projects out...I'll keep testing cases) - I'm not as familiar with the VS project system, so a PR may not be practical, but I'll try to dig in a bit and see. And thanks for the back and forth here - much appreciated!

[CyrusNajmabadi]

Total disagreement here for sure - IMO they're 100% irrelevant.

We might want to then include an option to show/hide this stuff. For me, it's very relevant. That method may be an implementation of some interface method, and i want to know who is calling me through the interface, and who is calling me directly. For example, if i have:

class CFoo : IFoo { public void MFoo() { ... } } and I search for MFoo, then I will be unhappy with it saying "there are no references to MFoo" simply because it was always called through the interface method. After all, I might not even realize that this is an implementation of an interface method. In C# tehre is nothing on an implicit-implementation informing you that this has any relation to an interface method.

It gets even worse than that: one might have class CFoo { public void MFoo() { ... } } and 'MFoo' might still be the implementation of an interface method (even though none are declared on the type). If we say "this isn't referenced anywhere" people will want to delete the method, and that can totally cause build breaks.

[CyrusNajmabadi]

If I searched from the interface, sure - I totally agree they're relevant.

As mentioned above, these things are loosely coupled. Your code may only be referenced through an interface method that you don't even realize you're implementing. Stating 'there are no references' then gets extremely weird and makes users have to perform several steps to figure out how your code is actually being referenced in your project.

[CyrusNajmabadi]

Maybe if there was a section for unused references (with such a heading) at the bottom - which would also greatly collapse the vertical space by nature of reducing repetition.

I think that's somewhat reasonable to provide. We can try looking into that.

[CyrusNajmabadi]

But why does it present as 2 items? It's 1 class in 1 place with 1 symbol

It actually isn't. It's two unique symbols. You can see that just by looking at the output of your build. You'll have multiple dlls with different symbols in them. It's not hte same definition at all.

This is also easy to demonstrate through code like:

```c#

if COMPILER

namespace MS.CA.Compilers

else

namespace MS.CA.Workspaces

endif

{
internal static class NodeExtensions
{
}
}
```

There are two totally unique class symbols here, each belonging to a distinct namespace. The way to think about linked-files and shared projects is that everyone is getting a unique copy of that code, and everyone may interpret that code in entirely different ways. Indeed, it's very common for the different referencing projects to see very different interpretations of the symbols. They may be structs in one and classes in another. Or they may have different member. Or types may have different inheritance chains (and thus might be implementing totally different interfaces). Or their members may have different signatures with different referenced types.

Now, that said, I a totally get that this can lead to a confusing experience, and that it likely warrants some work in the UI to try to make this clearer/more intelligible. For example, if the fully qualified name of the symbol is the same, and all the definitions come from the same linked file, we might want to just merge the results and 'pretend' they're the same. This would not be accurate, but could be good enough, with enough value to warrant doing it.

[NickCraver]

As mentioned above, these things are loosely coupled. Your code may only be referenced through an interface method

Sure, that's fair - so why don't we show that? I would have no objections to showing the symbol I searched for, and another section that's <Interface>.Symbol and a list of those usages in another section - that seems totally reasonable and very helpful. But the current situation doesn't do that, it shows every possible implementation of the interface, and whether they are used.

Let's say I have classes ClassA and ClassB, both are based on Interface MyInterface. It has one method: string MyMethod(). If I did "Find All References" on ClassA.MyMethod, I'd expect to see 2 sections:

• ClassA.MyMethod():
  
  
  
  • List of direct usages
  
  
• MyInterface.MyMethod():
  
  
  
  • List of all interface usages
  
  

What happens today that I don't expect or want is all direct usages of ClassB.MyMethod() (and all other implementations, possibly dozens). Those are not relevant, and changes to ClassA.MyMethod() don't affect it.

[CyrusNajmabadi]

If the thinking here is that to the user the same line of code is 2 different things (or really n different things) depending on the number of build configs, that's bad

It's bad here. But it's not always bad. If someone wants to actually ask "i want to know how this specific interpretation of this symbol is referenced". I know I routinely ask that with code that we share between the Roslyn Compiler and IDE projects.

But, again, I'm not arguing against trying to improve the UI. I can see us opting to give the simplified view (that merges disparate definitions), with some sort of option to let people see all the distinct symbols if they want to.

[CyrusNajmabadi]

Sure! I'd happily try this...how do I do it?

Right click on the column headers:

I'll let the editor team know that they should likely provide a 'None' option under the "Group by" dropdown.

[NickCraver]

There are two totally unique class symbols here, each belonging to a distinct namespace.

I think that's mixing compiler output (yes, you're right), with what I'm editing (what most developers care about). I'm working with one chunk of code. I'm using an IDE to do it. I don't want 1 line of code to be treated like 12 because the tooling is generating 12 outputs from it (all usually the same). That should not be the user experience.

The tooling experience should be for the concerns of the developer and what they're conceptually dealing with, not mapping 1:1 to the output of a compiler 2 or 3 steps away. I'd certainly want the combined view to be a default. With a comma list (as I proposed early on) or something else to easily signify this. Perhaps it's a "Group by" (Target Framework) option?

[CyrusNajmabadi]

@NickCraver As i've mentioned several times, I understand what you are saying for you. But, for me, there are times when i definitely want this information, and I find it quite useful. Right now we've erred toward a model that provides all the information so that you have what you need if you want it. That does mean things are not great if you don't want it. But I don't want to assume that we should hide this information, especially when i know from first hand experience that it's sometimes desirable.

[NickCraver]

@CyrusNajmabadi I'm not discounting you sometimes want it, it's only a question of usability and defaults if we can provide both experiences. How often would you saw you want a split view vs. a combined view (from the target framework grouping perspective)?

I disagree that's we're hiding the information (that's my confusion), if we clearly show a comma-delimited list. And what are we taking away? Every click today goes to the same place on all child items that would be combined...is there a scenario where that's not the case? If the grouping is by symbol, line number, etc. to show under the same target framework-combined group, aren't we just saving vertical space and duplication? Often to the tune of 3+ lines per result.

I'm trying to understand the objection to the "(net4.6, netstandard1.5)" grouping by default...because I'm not understanding how that hides information. What am I missing there?

[CyrusNajmabadi]

What happens today that I don't expect or want is all direct usages of ClassB.MyMethod() (and all other implementations, possibly dozens). Those not relevant, and changes to ClassA.MyMethod() don't affect it.

That's fair. We can definitely explore only showing the direct inheritance hierarchy (i.e. only up/down from the initial symbol you search for), and not continuing to cascade updown. Here's the problem though, say you have this code:

c# class C : IFoo { public virtual void F() { } } class D : C { public override void F() { } } class E : D, IBar { } interface IFoo { void F(); } interface IBar { void F(); }

If I search for references to 'C.F' I'd have to still cascade through all interfaces. i.e. someone might be calling C.F through a reference to IBar. This is why, in general, we cascade.

Another reason we do this 'cascading' is that people often use FindAllReferences to find all the things to investigate in an inheritance hierarchy. For example, there might be some virtual method that someone wants to change. i.e. maybe they want to add/remove a parameter from it. They want to see the entire inheritance hierarchy for that method to make that determination. Hiding information means that something might break, or that they might go down a path thinking a change was safe, only to discover that they didn't see some reference where things were slightly different and that they shouldn't go forward.

[CyrusNajmabadi]

I disagree that's we're hiding the information (that's my confusion), if we clearly show a comma-delimited list.

I don't know what you mean. As mentioned originally, can you show some mockups of what you're talking about? I feel like you have some ideas in your head, but i don't know what they are :) If you show me waht you mean, that will help out a lot!

[CyrusNajmabadi]

I'm trying to understand the objection to the "(net4.6, netstandard1.5)" grouping by default...because I'm not understanding how that hides information. What am I missing there?

I haven't objected. I just don't know what you're asking for. Hence why i keep asking for an actual mockup so i can be on hte same page as you :) I simply don't precisely know what you're asking for, and it would be helpful if that were presented to get to the heart of things.

Thanks!

[NickCraver]

Here's the problem though, say you have this code

Okay totally valid example. I think the problem isn't necessarily the cascade or the showing "no references", it's doing both that results in a lot of noise. I'm not sure what the best default is there. I'll try some photoshop now...this won't be pretty!

[CyrusNajmabadi]

@NickCraver I'm totally in agreement :) Just to be clear, i'm totally behind improvements here. I'm just trying to explain many of the things we need to take into account and be careful of. Once we get your ideas, we can start coming up with a total list of things that we can potentially work on. To start with, i'd like to potentially just collapse items that don't have any references. You'll already see the '0' next to the symbol name, so you won't have to expand it.

[NickCraver]

All of this is with MiniProfiler (direct link).

Here's current:

Notice it's identical, but repeated per framework...it'd get a lot worse if I had more target frameworks (which we may actually add in this case).

I'd collapse that while still indicating the target frameworks by making that list comma delimited:

Hopefully that clears up the idea a little for discussion - if not let me know and I'll bang my head against Photoshop some more :)

[CyrusNajmabadi]

Ok. So i can see how that would work. However, we'd have to figure out what to do in the case where the set of references was not identical.

@dpoeschl Given a Roslyn project that represents a shared-project, how do you get the set of all projects that it can represent. i.e. here:

We have two MiniProfiler.Shared projects (one per platform). Is there anything in our Workspace APIs to relate these?

[Pilchie]

See also #19376 which shows how the same issue impacts Go To as well.

[CyrusNajmabadi]

Good examples of how this could look.

Right now we have:

It would be nice to have:

[jnm2]

Thanks for moving this. Don't know why I typed netstandard instead of netstandard20 or something, but too much effort to fix it now. 😄

[SolderedMushroom]

@CyrusNajmabadi Super late, but can you explain this comment?

It gets even worse than that: one might have class CFoo { public void MFoo() { ... } } and 'MFoo' might still be the implementation of an interface method (even though none are declared on the type). If we say "this isn't referenced anywhere" people will want to delete the method, and that can totally cause build breaks.

How could one be referencing CFoo's implementation of MFoo through an interface, if CFoo doesn't implement any interfaces (and you don't write ISomething.MFoo for the signature of MFoo)?

[CyrusNajmabadi]

@SolderedMushroom C# allows for implicit bridged interface methods . So you can write the following:

c# interface IFoo { void MFoo(); } class C { public void MFoo(); } // elsewhere (including another assembly) class DFoo : C, IFoo { }

This is totally legal, and C.MFoo is now effectively the interface implementation of IFoo.MFoo because of D.

[CyrusNajmabadi]

Note: i want to seperate out the concepts of 'completeness of results' versus 'presentation of results'. I think results need to be complete. If we say something isn't referenced at all, and yet, there's a real runtime codepath that can hit it, then people are not going to be happy.

But, at the same time, we totally get that the overall presentation gets extremely clunky and overwrought in very normal situations. That sucks. No question about it. I want us to invest here in making the UI great for the times when users don't care about the 'completeness' question. It's just, unfortunately, not as important as other stuff we're doing for 15.3. I hope we can do much better here in 15.6.

Thanks!

[Pilchie]

Also reported in https://developercommunity.visualstudio.com/content/problem/56296/c-find-all-references-finds-same-thing-twice.html

[Pilchie]

Just a note that per https://developercommunity.visualstudio.com/content/problem/73354/result-is-shown-multiple-times-in-solution-explore.html - this also affects search in solution explorer.

[NickCraver]

Reminder on this issue as it gets worse every time we add a target. Here's what I see today, it's so bad that my default has moved to Ctrl+Shift+F instead. The usability is really bad for anyone multi-targeting.

How many people need to report this is an issue to impact the scheduling of working on it? With netstandard2.x being a thing, more and more people will hit this, even if they're only in the netstandard world we're pointing them to.

[steffenloesch]

We hit this issue with "Go To ..." as well, and it's a reason why some of our developers prefer ReSharper search.

[jnm2]

Yes. I'm still trying to figure out what the improvement would be, but I find VS's results more visually confusing. The presentation is more 'clever' but takes me longer to look through.

[CyrusNajmabadi]

So, i totally think things can be improved here. Part of the problem, simply, is that there are two distinct systems in play.

1. The LS, and hte data is produces. Right now, this is highly accurate, and truly represents the perspective of the language as to what are defs and what are refs.
2. The UI. (paging @olegtk. can you pull in Daofo? I don't know his github address). This has built in knowledge of special columns (like file/project/etc.)

The issue here (as i see it), is that the current UI formalization is great for many uses, but really not great for this case here. IMO, we should do something similar to the 'error list' where the same error reported across multiple linked files in multiple projects get collapsed.

to make this work though, we'd need to fix up the UI layer to have the understanding that the project/file columns could represent a set of files/projects, with appropriate displays to make it clear what was going on. If the UI could incorporate this concept, then the LS could fill it appropriate with its data model.

The thing that gets tough is how this plays along with things like "group by project" or "group by file". How are these supposed to work? Imagine the following. You have Symbol 'Foo' which is found at the same linked file location in projects A, B, C and in another linked location in B, C, D

How do you present this when you group by project? Should there be a project group for each individual project? If so, this has exploded out again into 4 results. Or should there by two distinct project-set-groups? i.e. one grouping for 'projects a, b, c' and one for 'projects b, c, d'? Would there also be individual nodes for 'project A' (or B, C, D) if there was a single non-linked result found in it?

With the growth of shared projects, i really do think that some work is needed here. But it needs to be well thought out, with all the myriad cases considered.

[CyrusNajmabadi]

I created https://github.com/dotnet/roslyn/issues/25128 to track work that can be done to improve GoTo. I think we should have this issue be the one for 'Find references'. Trying to discuss both in the same issue will be very confusing and difficult as these are very different systems.

[olegtk]

@lineralpha (Daofa)

[Havie]

I was having a similar issue after reinstalling VS2019 and it was showing identical duplicates, with the exact same path to file, I changed it from Entire Solution to Current Project and the duplicates went away . Very weird

[sharwell]

I'd like to see this work the same way the Error List currently works: items appearing in multiple projects via linked files are automatically collapsed to a single line showing multiple values in the Project column (this includes cases with multiple projects due to multitargeting).