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

Go To Implementation show metadata results #59100

Merged
merged 20 commits into from
Feb 8, 2022

Conversation

davidwengier
Copy link
Member

@davidwengier davidwengier commented Jan 27, 2022

Part of #58147

Looks like this:
GoToImplementationFromMetadataWithFiltering

@davidwengier davidwengier marked this pull request as ready for review February 1, 2022 04:24
@davidwengier davidwengier requested a review from a team as a code owner February 1, 2022 04:24
@davidwengier
Copy link
Member Author

Still investigating the integration test failures, unfortunately I can't seem to run them at all locally.

@davidwengier
Copy link
Member Author

davidwengier commented Feb 2, 2022

@sharwell any pro tips for investigating the integration test failures? From the logs the exceptions seem to come from inside DTE, and whilst the failures are oddly related to what i'm working on (Find Refs vs Go To Impl) I don't believe any of the code I've changed could be at fault here.

Most frustratingly I can't seem to run any integration tests locally at the moment, but manually executing things the scenarios certainly work.

The error:

 Failed Roslyn.VisualStudio.NewIntegrationTests.CSharp.CSharpFindReferences.FindReferencesToLocals (VS2022) [2 s]
  Error Message:
   System.ArgumentException : index
  Stack Trace:
     at Microsoft.VisualStudio.Platform.WindowManagement.DTE.WindowsCollection.Item(Object index)
   at Microsoft.VisualStudio.Shell.ThreadHelper.Invoke[TResult](Func`1 method)
   at EnvDTE80.ToolWindows.GetToolWindow(String Name)
   at Roslyn.VisualStudio.IntegrationTests.InProcess.FindReferencesWindowInProcess.<GetFindReferencesWindowAsync>d__1.MoveNext() in /_/src/VisualStudio/IntegrationTest/New.IntegrationTests/InProcess/FindReferencesWindowInProcess.cs:line 62
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter`1.GetResult()
   at Roslyn.VisualStudio.IntegrationTests.InProcess.FindReferencesWindowInProcess.<GetContentsAsync>d__0.MoveNext() in /_/src/VisualStudio/IntegrationTest/New.IntegrationTests/InProcess/FindReferencesWindowInProcess.cs:line 28
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter`1.GetResult()
   at Roslyn.VisualStudio.NewIntegrationTests.CSharp.CSharpFindReferences.<FindReferencesToLocals>d__4.MoveNext() in /_/src/VisualStudio/IntegrationTest/New.IntegrationTests/CSharp/CSharpFindReferences.cs:line 105
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at Roslyn.VisualStudio.NewIntegrationTests.CSharp.CSharpFindReferences.<FindReferencesToLocals>d__4.MoveNext() in /_/src/VisualStudio/IntegrationTest/New.IntegrationTests/CSharp/CSharpFindReferences.cs:line 128
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.GetResult()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)

@CyrusNajmabadi
Copy link
Member

note: we likely want to move this to be entirely streaming so that we're not blockign the find-impls results on doign all the search and conversion here of all these types.

Copy link
Member

@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.

tentative approval, pending:

  1. confirmation that the stock-vs experience with go-to-impl still shows source results first.
  2. some clarity on the current shape.

@davidwengier
Copy link
Member Author

davidwengier commented Feb 3, 2022

  1. confirmation that the stock-vs experience with go-to-impl still shows source results first.

When I run devenv /ResetSettings /RootSuffix NewHiveForCyrus I see only source results, and have to switch to one of the other filters to get external sources to show. I cannot 100% rule out that isn't because of a saved setting somewhere, as the filter does remember (which also means, if this does regress the experience, its one click and it never will again). I signed out of VS and same result, so synced settings definitely aren't an issue.

From my understanding and experimentation however, nothing overrides user sorting/grouping/filtering in order to show source results first. It's certainly something I can raise with the editor team, but my preference would not be to block this PR on it, mainly because I'd rather get you, and everyone else, dogfooding it and see if people have usability issues, and we can always revert the PR.

The issue is that it's trivial to issue a Go To Implementation command on IDisposable.DIspose() and show how problematic this can be, but I feel like thats looking for a problem. I'd rather wait and see if users natural GoToImpl scenarios are adversely affected by this change.

@davidwengier
Copy link
Member Author

@sharwell you probably want to take a look at this, I had to make a bunch of integration test changes.

Turns out that the presence of additional filters in the Find References window changes the window caption. Personally I prefer the Guid approach anyway, window caption feels more vulnerable to flakiness.

@davidwengier davidwengier merged commit e37f01d into dotnet:main Feb 8, 2022
@davidwengier davidwengier deleted the GoToImplFromMetadata branch February 8, 2022 10:39
@ghost ghost added this to the Next milestone Feb 8, 2022
RikkiGibson added a commit that referenced this pull request Feb 8, 2022
RikkiGibson added a commit that referenced this pull request Feb 9, 2022
davidwengier added a commit to davidwengier/roslyn that referenced this pull request Feb 9, 2022
@RikkiGibson RikkiGibson modified the milestones: Next, 17.2.P2 Mar 1, 2022
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