Keep Peek Definition off the main thread

docs/release-notes/.VisualStudio/18.vNext.md

@@ -5,6 +5,8 @@
 
 ### Fixed
 
+* Go To Definition no longer blocks the UI thread with a bare `Task.Wait`: the synchronous `IFSharpGoToDefinitionService` call now waits through the cancellable threaded-wait dialog, and the editor's `TaskCompletionSource` bridges run their continuations on the thread pool instead of inline on whichever thread finished the check, so repeated F12 on a large solution no longer starves semantic classification and other main-thread work. ([PR #20482](https://github.com/dotnet/fsharp/pull/20482))
+* Peek Definition on an F# symbol whose definition lives in metadata no longer deadlocks Visual Studio. Peek holds the main thread in `JoinableTaskFactory.Run` without pumping messages while it asks the language service for the definition, and generating the metadata document needs that same thread; Peek now stops at definitions that already have a document, and Go To Definition, which owns the wait it makes, still opens the generated one. ([PR #20503](https://github.com/dotnet/fsharp/pull/20503))
 * Improve Find All References performance by throttling parallel typechecks. ([PR #20128](https://github.com/dotnet/fsharp/pull/20128))
 * Fixed Rename incorrectly renaming `get` and `set` keywords for properties with explicit accessors. ([Issue #18270](https://github.com/dotnet/fsharp/issues/18270), [PR #19252](https://github.com/dotnet/fsharp/pull/19252))
 * Fixed Find All References crash when F# project contains non-F# files like `.cshtml`. ([Issue #16394](https://github.com/dotnet/fsharp/issues/16394), [PR #19252](https://github.com/dotnet/fsharp/pull/19252))

vsintegration/src/FSharp.Editor/Common/CancellableTasks.fs

@@ -777,7 +777,9 @@ module CancellableTasks =
                     }
 
             // try not to yield if on bg thread already
-            let tcs = new TaskCompletionSource<_>(TaskCreationOptions.None)
+            let tcs =
+                new TaskCompletionSource<_>(TaskCreationOptions.RunContinuationsAsynchronously)
+
             let barrier = VolatileBarrier()
 
             let reg =

vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs

@@ -135,7 +135,9 @@ module internal RoslynHelpers =
                     return! computation
                 }
 
-        let tcs = new TaskCompletionSource<_>(TaskCreationOptions.None)
+        let tcs =
+            new TaskCompletionSource<_>(TaskCreationOptions.RunContinuationsAsynchronously)
+
         let barrier = VolatileBarrier()
 
         let reg =

vsintegration/src/FSharp.Editor/Navigation/FindDefinitionService.fs

@@ -19,6 +19,6 @@ type internal FSharpFindDefinitionService [<ImportingConstructor>] (metadataAsSo
         member _.FindDefinitionsAsync(document: Document, position: int, cancellationToken: CancellationToken) =
             cancellableTask {
                 let navigation = FSharpNavigation(metadataAsSource, document, rangeStartup)
-                return! navigation.FindDefinitionsAsync(position)
+                return! navigation.FindDefinitionsWithoutMetadataAsync(position)
             }
             |> CancellableTask.start cancellationToken

vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs

@@ -793,33 +793,58 @@ type internal FSharpNavigation(metadataAsSource: FSharpMetadataAsSourceService,
             | _ -> return ImmutableArray.empty
         }
 
+    /// The same search, minus the definitions that only exist once a metadata document has been generated:
+    /// generating one takes the main thread, and Peek's broker holds it in `JoinableTaskFactory.Run` without
+    /// pumping messages until this returns, so asking for it there deadlocks Visual Studio.
+    member _.FindDefinitionsWithoutMetadataAsync(position) =
+        cancellableTask {
+            let gtd = GoToDefinition(metadataAsSource)
+            let! result = gtd.FindDefinitionAtPosition(initialDoc, position)
+
+            match result with
+            | ValueSome(FSharpGoToDefinitionResult.NavigableItem(navItem), _) -> return ImmutableArray.create navItem
+            | _ -> return ImmutableArray.empty
+        }
+
     member _.TryGoToDefinition(position, cancellationToken) =
-        // Once we migrate to Roslyn-exposed MAAS and sourcelink (https://github.com/dotnet/fsharp/issues/13951), this can be a "normal" task
-        // Wrap this in a try/with as if the user clicks "Cancel" on the thread dialog, we'll be cancelled.
-        // Task.Wait throws an exception if the task is cancelled, so be sure to catch it.
+        // Once we migrate to Roslyn-exposed MAAS and sourcelink (https://github.com/dotnet/fsharp/issues/13951), this can be a "normal" task.
+        // The IFSharpGoToDefinitionService contract is synchronous, so the main thread has to wait here: the threaded-wait dialog
+        // keeps it pumping and cancellable, where a bare Task.Wait froze it until the VS watchdog auto-cancelled.
         try
             use _ =
                 TelemetryReporter.ReportSingleEventWithDuration(TelemetryEvents.GoToDefinition, [||])
 
             let gtd = GoToDefinition(metadataAsSource)
-            let gtdTask = gtd.FindDefinitionAsync (initialDoc, position) cancellationToken
+            let navigated = ref false
 
-            gtdTask.Wait()
+            ThreadHelper.JoinableTaskFactory.Run(
+                SR.NavigatingTo(),
+                (fun _progress dialogCancellationToken ->
+                    let linked =
+                        CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, dialogCancellationToken)
 
-            if gtdTask.Status = TaskStatus.RanToCompletion && gtdTask.Result.IsSome then
-                match gtdTask.Result with
-                | ValueSome(FSharpGoToDefinitionResult.NavigableItem(navItem), _) ->
-                    gtd.NavigateToItem(navItem, cancellationToken) |> ignore
-                    true
-                | ValueSome(FSharpGoToDefinitionResult.ExternalAssembly(targetSymbolUse, metadataReferences), _) ->
-                    gtd.NavigateToExternalDeclaration(targetSymbolUse, metadataReferences, cancellationToken)
-                    |> ignore
+                    cancellableTask {
+                        use _ = linked
+
+                        match! gtd.FindDefinitionAsync(initialDoc, position) with
+                        | ValueSome(FSharpGoToDefinitionResult.NavigableItem(navItem), _) ->
+                            gtd.NavigateToItem(navItem, linked.Token) |> ignore
+                            navigated.Value <- true
+                        | ValueSome(FSharpGoToDefinitionResult.ExternalAssembly(targetSymbolUse, metadataReferences), _) ->
+                            gtd.NavigateToExternalDeclaration(targetSymbolUse, metadataReferences, linked.Token)
+                            |> ignore
+
+                            navigated.Value <- true
+                        | _ -> ()
+                    }
+                    |> CancellableTask.start linked.Token),
+                TimeSpan.FromSeconds 1
+            )
 
-                    true
-                | _ -> false
-            else
-                false
-        with exc ->
+            navigated.Value
+        with
+        | :? OperationCanceledException -> false
+        | exc ->
             TelemetryReporter.ReportFault(TelemetryEvents.GoToDefinition, FaultSeverity.General, exc)
             false