Fallback navigate to by sharwell · Pull Request #28294 · dotnet/roslyn

sharwell · 2018-07-05T12:54:51Z

Customer scenario

In the latest 15.8 Preview 4 internal builds, Navigate To does not work for TypeScript or F#.

Bugs this fixes

https://devdiv.visualstudio.com/DevDiv/_workitems/edit/644180

Workarounds, if any

None.

Risk

Medium risk. The safest option for release would be reverting the original change, but we would lose the substantial (clearly user visible) performance benefit it provided for Go To File.

Performance impact

This change enumerates the projects in a Solution to determine the supported Navigate To kinds each time the feature is used. However, this is not expected to result in substantial overhead compared to the underlying navigation operation.

Is this a regression from a previous update?

Yes

Root cause analysis

TypeScript and F# support Navigate To by implementing INavigateToSearchService (only available via IVTs and thus not under the protection of our public API surface change review process), but there is no test coverage for this feature in either language in either the dotnet/roslyn PR builds or the DDRITs used during the RI process.

How was the bug found?

Dogfooding

Test documentation updated?

No

…nges

CyrusNajmabadi · 2018-07-05T19:31:56Z

@sharwell What is this for?

sharwell · 2018-07-06T15:47:21Z

@CyrusNajmabadi ask mode template updated to explain the change

sharwell · 2018-07-06T16:33:26Z

@jasonmalinowski @minestarks @brettfo I have manually verified the repro state and the fix for both TypeScript and F#, so I am now confident this will work.

jasonmalinowski

Calling this an "approve with requested changes" -- mostly just a request for some comments and better text for the Obsolete, since the message isn't actually advice we want to give anyone.

jasonmalinowski · 2018-07-06T22:20:05Z

src/EditorFeatures/Core.Wpf/NavigateTo/NavigateToItemProvider.cs

+                    }
+
+#pragma warning disable CS0618 // Type or member is obsolete
+                    var legacyNavigateToSearchService = project.LanguageServices.GetService<INavigateToSearchService>();


Any reason this method isn't just using your other helper that already does the "return an interface"? I recognize it'll allocate a shim which will then cause this loop to bail, but it's a single allocation per search, which seems pretty cheap compared to the duplicate logic here.

(bonus points for a "get all services" helper that just does the loop and is consumed in both properties)

jasonmalinowski · 2018-07-06T22:20:44Z

src/EditorFeatures/Core.Wpf/NavigateTo/NavigateToItemProvider.cs

+                    var navigateToSearchService = project.LanguageServices.GetService<INavigateToSearchService_RemoveInterfaceAboveAndRenameThisAfterInternalsVisibleToUsersUpdate>();
+                    if (navigateToSearchService != null)
+                    {
+                        result = result.Union(navigateToSearchService.KindsProvided);


Use a builder?

This is supposed to be a NOP on each loop. It's not clear whether ImmutableHashSet<T> implements the necessary edge cases as fast paths.

When result is empty, the right hand side is returned

When the right hand side is empty, result is returned unaltered

In all other cases where the solution does not contain both C#/VB and F#/TypeScript projects, result and the right hand side reference the same object

Even if the edge cases aren't dedicated fast paths, the fact that most iterations do not alter result means a builder would not provide noticeable benefits.

jasonmalinowski · 2018-07-06T22:21:02Z

src/EditorFeatures/Core.Wpf/NavigateTo/NavigateToItemProvider.cs

+                    if (navigateToSearchService != null)
+                    {
+                        result = result.Union(navigateToSearchService.KindsProvided);
+                        continue;


continue doesn't need to be here.

jasonmalinowski · 2018-07-06T22:25:16Z

src/Features/Core/Portable/NavigateTo/INavigateToSearchService.cs


 namespace Microsoft.CodeAnalysis.NavigateTo
 {
+    [Obsolete("Use " + nameof(INavigateToSearchService_RemoveInterfaceAboveAndRenameThisAfterInternalsVisibleToUsersUpdate) + " instead.")]


So I'm not sure that's really the right message here: we sure don't want people moving to this new interface either...at least not with that name!

This order allows the new interface to be implemented independently (the hard/slow part), then we can coordinate with all the repositories on an interface rename (the easy part).

I guess I assumed that our path forward would be to directly work forward having people consume the existing name with new surface area, rather than moving to the new name and then moving back.

In any case "above" is a pretty meaningless name then for an external consumer to consume.

jasonmalinowski · 2018-07-06T22:30:40Z

src/Features/Core/Portable/NavigateTo/INavigateToSearchService.cs

+        Task<ImmutableArray<INavigateToSearchResult>> SearchDocumentAsync(Document document, string searchPattern, CancellationToken cancellationToken);
+    }
+
+    internal interface INavigateToSearchService_RemoveInterfaceAboveAndRenameThisAfterInternalsVisibleToUsersUpdate : ILanguageService


Please link to the tracking bug tracking the cleanup, and a quick discussion of how this type came into being. The PR description nor commit description really make this clear unless you're willing to chase through the original PR and figure out the context there.

* Use TryGetNavigateToSearchService instead of duplicating logic * Simplify code flow * Link to dotnet#28343 tracking interface rename

jinujoseph · 2018-07-09T16:46:49Z

Approved to merge for 15.8.Preview5

jinujoseph · 2018-07-09T17:04:04Z

@dotnet-bot test

jinujoseph · 2018-07-09T17:04:38Z

@dotnet-bot retest this please

mmitche · 2018-07-09T17:18:10Z

@dotnet-bot test this please

dpoeschl

Something strange is happening with the tests. Temporarily blocking while we sort it out.

jinujoseph · 2018-07-10T23:43:59Z

Merged for 15.8.Preview5

sharwell added 2 commits July 5, 2018 07:31

Implement INavigateToItemProvider2 without requiring breaking API cha…

2373284

…nges

Use immutable sets in Navigate To abstraction

f524d6e

sharwell requested a review from a team as a code owner July 5, 2018 12:54

jinujoseph added Area-IDE Needs Shiproom Approval labels Jul 6, 2018

jinujoseph added this to the 15.8 milestone Jul 6, 2018

jinujoseph assigned jasonmalinowski Jul 6, 2018

jasonmalinowski approved these changes Jul 6, 2018

View reviewed changes

sharwell mentioned this pull request Jul 6, 2018

Remove/replace INavigateToSearchService #28343

Closed

sharwell added 2 commits July 6, 2018 18:03

Move nested helper TryGetNavigateToSearchService to the containing class

a365647

Code cleanup to address review feedback

7beddef

* Use TryGetNavigateToSearchService instead of duplicating logic * Simplify code flow * Link to dotnet#28343 tracking interface rename

jasonmalinowski approved these changes Jul 6, 2018

View reviewed changes

jinujoseph removed the Needs Shiproom Approval label Jul 9, 2018

jinujoseph added the Approved to merge label Jul 9, 2018

jasonmalinowski changed the base branch from dev15.8-preview4 to master July 9, 2018 17:35

jasonmalinowski changed the base branch from master to dev15.8-preview4 July 9, 2018 17:35

jinujoseph changed the base branch from dev15.8-preview4 to dev15.8.x July 10, 2018 17:25

jinujoseph added Needs Shiproom Approval and removed Approved to merge labels Jul 10, 2018

jasonmalinowski changed the base branch from dev15.8.x to master July 10, 2018 21:38

jasonmalinowski changed the base branch from master to dev15.8.x July 10, 2018 21:39

sharwell changed the base branch from dev15.8.x to dev15.8-preview4 July 10, 2018 22:36

sharwell changed the base branch from dev15.8-preview4 to dev15.8.x July 10, 2018 22:36

dpoeschl suggested changes Jul 10, 2018

View reviewed changes

dpoeschl approved these changes Jul 10, 2018

View reviewed changes

dpoeschl merged commit be711c3 into dotnet:dev15.8.x Jul 10, 2018

jinujoseph removed the Needs Shiproom Approval label Jul 10, 2018

sharwell deleted the fallback-navigate-to branch July 23, 2018 22:35

Conversation

sharwell commented Jul 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

Uh oh!

CyrusNajmabadi commented Jul 5, 2018

Uh oh!

sharwell commented Jul 6, 2018

Uh oh!

sharwell commented Jul 6, 2018

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Jul 6, 2018

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Jul 6, 2018

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Jul 6, 2018

Choose a reason for hiding this comment

Uh oh!

sharwell Jul 6, 2018 • edited 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 Jul 6, 2018

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Jul 6, 2018

Choose a reason for hiding this comment

Uh oh!

sharwell Jul 6, 2018

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Jul 6, 2018 • edited 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 Jul 6, 2018

Choose a reason for hiding this comment

Uh oh!

jinujoseph commented Jul 9, 2018

Uh oh!

jinujoseph commented Jul 9, 2018

Uh oh!

jinujoseph commented Jul 9, 2018

Uh oh!

mmitche commented Jul 9, 2018

Uh oh!

dpoeschl left a comment

Choose a reason for hiding this comment

Uh oh!

jinujoseph commented Jul 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sharwell commented Jul 5, 2018 •

edited

Loading

sharwell Jul 6, 2018 •

edited

Loading

jasonmalinowski Jul 6, 2018 •

edited

Loading