Change navigation helpers to operate in two steps. #59798

CyrusNajmabadi · 2022-02-27T02:05:21Z

WIth the existing form, a feature simply asks teh service to navigate directly to a final document location. This PR changes things to make this two-step (with extensions to make things simple for most clients). The first step does any expensive work and returns a final object that can be then be used to actually perform the navigation.

This will be used as part of the BackgroundWorkIndicator (BWI) work. In that change a feature (like goto def) will open the BWI, do the work to figure out where to navigate to, then tell the BWI to ignore navigation, then do the final navigation. Telling the BWI to ignore navigation is important as otherwise the navigation the feature does will trigger the BWI to think that an arbitrary navigation happened, which will cause it to cancel. If it cancels then the very navigation that we're trying to attempt will be stopped.

CyrusNajmabadi · 2022-02-27T02:07:32Z

src/Features/Core/Portable/Navigation/INavigableDocumentLocation.cs

+        /// method.  This method is async only to allow final clients to call this from a non-UI thread while allowing
+        /// the navigation to jump to the UI thread.
+        /// </summary>
+        Task<bool> NavigateToAsync(CancellationToken cancelletionToken);


@jasonmalinowski i started with this being sync... but it still felt oogy. i'm not sure how i feel about any particular feature needing to have to have way to move itself to the UI thread before calling this.

that said, i don't feel super strongly about this. i think there's merit in your point that thsi must be fast/sync/UI bound.

note: for this PR/set-of-refactorings, i'd like to have this async for the moment. then, once all the work is done, we should have a good idea if it can be safely made sync.

I'm OK with this, but let's keep an eye out for places the "this should be async only..." rule gets violated. And maybe we will still have cases where this needs to be async; I could imagine in VS even opening a file this may still have some async bits since we at least have to check if the file exists, or something.

CyrusNajmabadi · 2022-02-28T05:25:51Z

src/EditorFeatures/Core.Wpf/Interactive/InteractiveDocumentNavigationService.cs

@@ -37,19 +37,19 @@ public Task<bool> CanNavigateToLineAndOffsetAsync(Workspace workspace, DocumentI
        public Task<bool> CanNavigateToPositionAsync(Workspace workspace, DocumentId documentId, int position, int virtualSpace, CancellationToken cancellationToken)


View with whitespace off.

CyrusNajmabadi · 2022-02-28T05:28:19Z

src/VisualStudio/Core/Def/Implementation/Workspace/VisualStudioDocumentNavigationService.cs

+                if (!workspace.IsDocumentOpen(documentId))
+                    return false;
+
+                if (IsSecondaryBuffer(workspace, documentId))


it's a shame we can't do secondary buffer mapping until docs are opened.

CyrusNajmabadi · 2022-02-28T19:55:37Z

src/VisualStudio/Core/Def/Implementation/Workspace/SourceGeneratedFileManager.cs

-                windowFrame.Show();
-            }
+                await _threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken);
+                var openDocumentService = _serviceProvider.GetService<SVsUIShellOpenDocument, IVsUIShellOpenDocument>();


could potentially do this on the BG as well. but i think ti's highly unlikely that will help anything. this is MAS. we've already loaded docs, so this service is already goign to be avialable.

Agreed, seems unlikely for that to be expensive. 😄

CyrusNajmabadi · 2022-02-28T19:57:34Z

src/VisualStudio/Core/Def/Implementation/Workspace/VisualStudioDocumentNavigationService.cs

        {
-            // Have to be on the UI thread to open a document.
-            await _threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken);
-            var document = OpenDocument(workspace, documentId);


inlined this helper method.

jasonmalinowski

Seems reasonable; if I had any concern with the navigation being async in addition to the computation, it's more that there's an implicit risk there to think about that whenever that code does an await and yields that the state could have changed out from underneath it if the caller doesn't have a threaded wait dialog up or something else that may prevent user action.

Comments are mostly mop-ups, OK for that go in a follow-up PR since you have a few stacked behind this.

src/Features/Core/Portable/Navigation/INavigableDocumentLocation.cs

jasonmalinowski · 2022-03-01T00:26:11Z

src/Features/Core/Portable/Navigation/INavigableDocumentLocation.cs

+        /// method.  This method is async only to allow final clients to call this from a non-UI thread while allowing
+        /// the navigation to jump to the UI thread.
+        /// </summary>
+        Task<bool> NavigateToAsync(CancellationToken cancelletionToken);


I'm OK with this, but let's keep an eye out for places the "this should be async only..." rule gets violated. And maybe we will still have cases where this needs to be async; I could imagine in VS even opening a file this may still have some async bits since we at least have to check if the file exists, or something.

jasonmalinowski · 2022-03-01T00:27:37Z

src/EditorFeatures/Core.Wpf/Interactive/InteractiveDocumentNavigationService.cs

@@ -37,19 +37,19 @@ public Task<bool> CanNavigateToLineAndOffsetAsync(Workspace workspace, DocumentI
        public Task<bool> CanNavigateToPositionAsync(Workspace workspace, DocumentId documentId, int position, int virtualSpace, CancellationToken cancellationToken)
            => SpecializedTasks.False;

-        public async Task<bool> TryNavigateToSpanAsync(Workspace workspace, DocumentId documentId, TextSpan textSpan, NavigationOptions options, bool allowInvalidSpan, CancellationToken cancellationToken)
+        public async Task<INavigableLocation?> GetLocationForSpanAsync(Workspace workspace, DocumentId documentId, TextSpan textSpan, NavigationOptions options, bool allowInvalidSpan, CancellationToken cancellationToken)


Suggested change

public async Task<INavigableLocation?> GetLocationForSpanAsync(Workspace workspace, DocumentId documentId, TextSpan textSpan, NavigationOptions options, bool allowInvalidSpan, CancellationToken cancellationToken)

public async Task<INavigableLocation?> GetNavigableLocationForSpanAsync(Workspace workspace, DocumentId documentId, TextSpan textSpan, NavigationOptions options, bool allowInvalidSpan, CancellationToken cancellationToken)

I actually liked the new name, since it's the DocNavigationService and it returns a NavigableLocation, i found it just fine to say GetLocation. but if you feewl strongly, i can change :)

src/EditorFeatures/Core.Wpf/Interactive/InteractiveDocumentNavigationService.cs

jasonmalinowski · 2022-03-01T00:30:05Z

src/EditorFeatures/Core.Wpf/Interactive/InteractiveDocumentNavigationService.cs

@@ -58,35 +58,40 @@ public async Task<bool> TryNavigateToSpanAsync(Workspace workspace, DocumentId d
            var textSnapshot = document?.GetTextSynchronously(cancellationToken).FindCorrespondingEditorTextSnapshot();
            if (textSnapshot == null)
            {
-                return false;
+                return null;
            }

            var snapshotSpan = new SnapshotSpan(textSnapshot, textSpan.Start, textSpan.Length);
            var virtualSnapshotSpan = new VirtualSnapshotSpan(snapshotSpan);

            if (!textView.TryGetSurfaceBufferSpan(virtualSnapshotSpan, out var surfaceBufferSpan))


Does this bit need to move into the deferred work? Since I think if there was async stuff streaming to the interactive window, the buffers could be updated and this would be stale?

in this case, we're technically on the UI thread the entire time. It's teh first line of GetLocationForSpanAsync. So it's less clear how much value any split has here. The split i was at least going for was having the first half be the part that detemines "is this safe to nav to?" and the second half is the actual "ok, now actually go there".

jasonmalinowski · 2022-03-01T00:37:01Z

...dio/Core/Def/Implementation/LanguageService/AbstractLanguageService`2.VsLanguageDebugInfo.cs

-                var text = document.GetTextSynchronously(cancellationToken);
+                var text = await document.GetTextAsync(cancellationToken).ConfigureAwait(false);


Is the assumption here this almost always completes synchronously? Because otherwise the ConfigureAwait(false) worries me a bit because if it doesn't complete, then we're going to be waiting on the thread pool while blocking the UI when we could have just used the UI thread originally.

yes... that's a good point. bleag... this is a rough area.

jasonmalinowski · 2022-03-01T00:38:05Z

src/VisualStudio/Core/Def/Implementation/Workspace/SourceGeneratedFileManager.cs

-                windowFrame.Show();
-            }
+                await _threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken);
+                var openDocumentService = _serviceProvider.GetService<SVsUIShellOpenDocument, IVsUIShellOpenDocument>();


Agreed, seems unlikely for that to be expensive. 😄

jasonmalinowski · 2022-03-01T00:40:26Z

src/VisualStudio/Core/Def/Implementation/Workspace/SourceGeneratedFileManager.cs

-            }
+                // We should have the file now, so navigate to the right span
+                return _openFiles.TryGetValue(temporaryFilePath, out var openFile) &&
+                    await openFile.NavigateToSpanAsync(sourceSpan, cancellationToken).ConfigureAwait(false);


This seems like a reasonable argument for your navigate method being async: at some point I'd imagine that OpenDocumentViaProject could be come async, or at least we asynchronously populate the text buffer. At that point, we'd still need this navigation to happen after that.

jasonmalinowski · 2022-03-01T00:43:05Z

src/VisualStudio/Core/Def/Implementation/Workspace/VisualStudioDocumentNavigationService.cs

        {
+            Contract.ThrowIfNull(textBuffer);


Is this file and callers not annotated?

callers are not annotated. so i'm being hyper anal.

jasonmalinowski · 2022-03-01T00:46:18Z

src/VisualStudio/Core/Def/Implementation/Workspace/VisualStudioDocumentNavigationService.cs

+                // Now that we've opened the document reacquire the corresponding Document in the current solution.
+                var document = workspace.CurrentSolution.GetDocument(documentId);
+                if (document == null)
+                    return false;


Hmm, this is something else that would have also been busted by #59211 since we're assuming that's happening. Nothing impactful for this PR; I wanted to tag it so when we look to resurrect #59211 we can remember to look at this.

Yes. I tlikely would be. We can talk more offline, however i think what we likely want is an async OpenFileAsync that can be non-UI-thread affinitized, and which returns back the actual opened document that later code can use.

Indeed, that would be useful here.

CyrusNajmabadi · 2022-03-01T02:04:37Z

Will take this for 17.3 intead. @RikkiGibson where should 17.3 work go?

CyrusNajmabadi added 5 commits February 25, 2022 16:57

In progress

779c8b0

Simplify

e18cef8

WIP

95ef3e9

Merge remote-tracking branch 'upstream/main' into asyncNavigation2

988cdf6

Make async

f3ad0ba

CyrusNajmabadi requested a review from jasonmalinowski February 27, 2022 02:05

CyrusNajmabadi requested review from a team as code owners February 27, 2022 02:05

dotnet-issue-labeler bot added the Area-IDE label Feb 27, 2022

CyrusNajmabadi commented Feb 27, 2022

View reviewed changes

CyrusNajmabadi added 3 commits February 26, 2022 18:09

Make async

ec02282

Test helpers

9db7519

lint

ac8820a

CyrusNajmabadi requested a review from a team as a code owner February 27, 2022 03:26

CyrusNajmabadi mentioned this pull request Feb 27, 2022

Update go-to-def handler to support entirely async go-to-def operation. #59801

Merged

CyrusNajmabadi added 9 commits February 26, 2022 19:57

Rename

31dd3ee

Rename

67e91b0

Rename

d419a10

Lint

3596605

Have to be on main thread

33b6fbc

In progress

5a7c62c

more async

f963940

Add comment

757220b

Fix provisional teab

fc5df02

CyrusNajmabadi commented Feb 28, 2022

View reviewed changes

CyrusNajmabadi added 3 commits February 27, 2022 22:39

Merge remote-tracking branch 'upstream/main' into asyncNavigation2

b7a3faf

Don't try to open docs already open

a3a29fe

Simplify

23491d4

CyrusNajmabadi commented Feb 28, 2022

View reviewed changes

jasonmalinowski approved these changes Mar 1, 2022

View reviewed changes

CyrusNajmabadi enabled auto-merge March 1, 2022 01:12

CyrusNajmabadi disabled auto-merge March 1, 2022 02:04

CyrusNajmabadi enabled auto-merge March 1, 2022 05:16

CyrusNajmabadi disabled auto-merge March 1, 2022 05:27

CyrusNajmabadi changed the base branch from main to release/dev17.3 March 1, 2022 18:21

CyrusNajmabadi enabled auto-merge March 1, 2022 18:21

CyrusNajmabadi merged commit cd34435 into dotnet:release/dev17.3 Mar 1, 2022

CyrusNajmabadi deleted the asyncNavigation2 branch March 1, 2022 20:06

runfoapp bot mentioned this pull request Mar 1, 2022

roslyn.visualstudio.integrationtests.visualbasic.basiclinecommit.commitonsave #59855

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change navigation helpers to operate in two steps. #59798

Change navigation helpers to operate in two steps. #59798

CyrusNajmabadi commented Feb 27, 2022

CyrusNajmabadi Feb 27, 2022

CyrusNajmabadi Feb 27, 2022

jasonmalinowski Mar 1, 2022

CyrusNajmabadi Feb 28, 2022

CyrusNajmabadi Feb 28, 2022

CyrusNajmabadi Feb 28, 2022

jasonmalinowski Mar 1, 2022

CyrusNajmabadi Feb 28, 2022

jasonmalinowski left a comment

jasonmalinowski Mar 1, 2022

jasonmalinowski Mar 1, 2022

CyrusNajmabadi Mar 1, 2022

jasonmalinowski Mar 1, 2022

CyrusNajmabadi Mar 1, 2022

jasonmalinowski Mar 1, 2022

CyrusNajmabadi Mar 1, 2022

jasonmalinowski Mar 1, 2022

jasonmalinowski Mar 1, 2022

jasonmalinowski Mar 1, 2022

CyrusNajmabadi Mar 1, 2022

jasonmalinowski Mar 1, 2022

CyrusNajmabadi Mar 1, 2022

jasonmalinowski Mar 1, 2022

CyrusNajmabadi commented Mar 1, 2022

		@@ -37,19 +37,19 @@ public Task<bool> CanNavigateToLineAndOffsetAsync(Workspace workspace, DocumentI
		public Task<bool> CanNavigateToPositionAsync(Workspace workspace, DocumentId documentId, int position, int virtualSpace, CancellationToken cancellationToken)

	public async Task<INavigableLocation?> GetLocationForSpanAsync(Workspace workspace, DocumentId documentId, TextSpan textSpan, NavigationOptions options, bool allowInvalidSpan, CancellationToken cancellationToken)
	public async Task<INavigableLocation?> GetNavigableLocationForSpanAsync(Workspace workspace, DocumentId documentId, TextSpan textSpan, NavigationOptions options, bool allowInvalidSpan, CancellationToken cancellationToken)

		var text = document.GetTextSynchronously(cancellationToken);
		var text = await document.GetTextAsync(cancellationToken).ConfigureAwait(false);

Change navigation helpers to operate in two steps. #59798

Change navigation helpers to operate in two steps. #59798

Conversation

CyrusNajmabadi commented Feb 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmalinowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Mar 1, 2022