From 9a3ccd26ffbb937f0ed0aec6f1cc7e18732e4f31 Mon Sep 17 00:00:00 2001 From: majocha Date: Fri, 14 Apr 2023 07:59:23 +0200 Subject: [PATCH 1/4] use joinable task factory --- .../Navigation/GoToDefinition.fs | 77 +++++++++---------- .../src/FSharp.Editor/QuickInfo/Views.fs | 2 +- 2 files changed, 38 insertions(+), 41 deletions(-) diff --git a/vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs b/vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs index e2e4d716900..6f638a5b275 100644 --- a/vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs +++ b/vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs @@ -568,46 +568,43 @@ type internal FSharpNavigation(metadataAsSource: FSharpMetadataAsSourceService, && solution.TryGetDocumentIdFromFSharpRange(range, initialDoc.Project.Id) |> Option.isSome - member _.RelativePath(range: range) = - let relativePathEscaped = - match solution.FilePath with - | null -> range.FileName - | sfp -> - let targetUri = Uri(range.FileName) - Uri(sfp).MakeRelativeUri(targetUri).ToString() - - relativePathEscaped |> Uri.UnescapeDataString - - member _.NavigateTo(range: range, cancellationToken: CancellationToken) = - asyncMaybe { - let targetPath = range.FileName - let! targetDoc = solution.TryGetDocumentFromFSharpRange(range, initialDoc.Project.Id) - let! targetSource = targetDoc.GetTextAsync(cancellationToken) - let! targetTextSpan = RoslynHelpers.TryFSharpRangeToTextSpan(targetSource, range) - let gtd = GoToDefinition(metadataAsSource) - - // To ensure proper navigation decsions, we need to check the type of document the navigation call - // is originating from and the target we're provided by default: - // - signature files (.fsi) should navigate to other signature files - // - implementation files (.fs) should navigate to other implementation files - let (|Signature|Implementation|) filepath = - if isSignatureFile filepath then - Signature - else - Implementation - - match initialDoc.FilePath, targetPath with - | Signature, Signature - | Implementation, Implementation -> return gtd.TryNavigateToTextSpan(targetDoc, targetTextSpan, cancellationToken) - - // Adjust the target from signature to implementation. - | Implementation, Signature -> return! gtd.NavigateToSymbolDefinitionAsync(targetDoc, targetSource, range, cancellationToken) + member _.NavigateTo(range: range) = + try + ThreadHelper.JoinableTaskFactory.Run( + SR.NavigatingTo(), + (fun progress cancellationToken -> + Async.StartImmediateAsTask( + asyncMaybe { + let! targetDoc = solution.TryGetDocumentFromFSharpRange(range, initialDoc.Project.Id) + let! targetSource = targetDoc.GetTextAsync(cancellationToken) + let! targetTextSpan = RoslynHelpers.TryFSharpRangeToTextSpan(targetSource, range) + let gtd = GoToDefinition(metadataAsSource) - // Adjust the target from implmentation to signature. - | Signature, Implementation -> return! gtd.NavigateToSymbolDeclarationAsync(targetDoc, targetSource, range, cancellationToken) - } - |> Async.Ignore - |> Async.StartImmediate + if isSignatureFile initialDoc.FilePath then + return gtd.TryNavigateToTextSpan(targetDoc, targetTextSpan, cancellationToken) + else + progress.Report( + ThreadedWaitDialogProgressData( + "Navigating to implementation.", + statusBarText = SR.NavigatingTo(), + isCancelable = true + ) + ) + + let! result = + gtd.NavigateToSymbolDefinitionAsync(targetDoc, targetSource, range, cancellationToken) + |> liftAsync + + if result.IsNone then + // fallback to fast navigation + return gtd.TryNavigateToTextSpan(targetDoc, targetTextSpan, cancellationToken) + } + |> Async.Ignore, + cancellationToken + )) + ) + with :? OperationCanceledException -> + () member _.FindDefinitions(position, cancellationToken) = let gtd = GoToDefinition(metadataAsSource) @@ -701,7 +698,7 @@ type FSharpNavigableLocation(metadataAsSource: FSharpMetadataAsSourceService, sy | Signature -> return! gtd.NavigateToSymbolDefinitionAsync(targetDoc, targetSource, symbolRange, cancellationToken) | Implementation -> return! gtd.NavigateToSymbolDeclarationAsync(targetDoc, targetSource, symbolRange, cancellationToken) } - |> Async.map (fun a -> a.IsSome) + |> Async.map Option.isSome |> RoslynHelpers.StartAsyncAsTask cancellationToken [)>] diff --git a/vsintegration/src/FSharp.Editor/QuickInfo/Views.fs b/vsintegration/src/FSharp.Editor/QuickInfo/Views.fs index 415b38798b1..cbdaebcb416 100644 --- a/vsintegration/src/FSharp.Editor/QuickInfo/Views.fs +++ b/vsintegration/src/FSharp.Editor/QuickInfo/Views.fs @@ -83,7 +83,7 @@ module internal QuickInfoViewProvider = | LineBreak :: rest -> loop rest [] (encloseRuns runs :: stack) | :? NavigableTaggedText as item :: rest when navigation.IsTargetValid item.Range -> let classificationTag = layoutTagToClassificationTag item.Tag - let action = fun () -> navigation.NavigateTo(item.Range, CancellationToken.None) + let action = fun () -> navigation.NavigateTo(item.Range) let run = ClassifiedTextRun(classificationTag, item.Text, action, getTooltip item.Range.FileName) From 06e5916fca9b6a744646ab2fe44cde06d92060ce Mon Sep 17 00:00:00 2001 From: majocha Date: Fri, 14 Apr 2023 09:25:03 +0200 Subject: [PATCH 2/4] simpler --- .../src/FSharp.Editor/Navigation/GoToDefinition.fs | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs b/vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs index 6f638a5b275..f2d513a223e 100644 --- a/vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs +++ b/vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs @@ -572,7 +572,7 @@ type internal FSharpNavigation(metadataAsSource: FSharpMetadataAsSourceService, try ThreadHelper.JoinableTaskFactory.Run( SR.NavigatingTo(), - (fun progress cancellationToken -> + (fun _progress cancellationToken -> Async.StartImmediateAsTask( asyncMaybe { let! targetDoc = solution.TryGetDocumentFromFSharpRange(range, initialDoc.Project.Id) @@ -583,14 +583,6 @@ type internal FSharpNavigation(metadataAsSource: FSharpMetadataAsSourceService, if isSignatureFile initialDoc.FilePath then return gtd.TryNavigateToTextSpan(targetDoc, targetTextSpan, cancellationToken) else - progress.Report( - ThreadedWaitDialogProgressData( - "Navigating to implementation.", - statusBarText = SR.NavigatingTo(), - isCancelable = true - ) - ) - let! result = gtd.NavigateToSymbolDefinitionAsync(targetDoc, targetSource, range, cancellationToken) |> liftAsync @@ -601,7 +593,8 @@ type internal FSharpNavigation(metadataAsSource: FSharpMetadataAsSourceService, } |> Async.Ignore, cancellationToken - )) + )), + TimeSpan.FromSeconds 1 ) with :? OperationCanceledException -> () From d2ad0089eea3bc93c54753357e0d7406ab0cbde4 Mon Sep 17 00:00:00 2001 From: majocha Date: Mon, 17 Apr 2023 17:40:39 +0200 Subject: [PATCH 3/4] clarify comments --- .../src/FSharp.Editor/Navigation/GoToDefinition.fs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs b/vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs index f2d513a223e..a16ec3ebeef 100644 --- a/vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs +++ b/vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs @@ -580,15 +580,21 @@ type internal FSharpNavigation(metadataAsSource: FSharpMetadataAsSourceService, let! targetTextSpan = RoslynHelpers.TryFSharpRangeToTextSpan(targetSource, range) let gtd = GoToDefinition(metadataAsSource) + // Whenever possible: + // - signature files (.fsi) should navigate to other signature files + // - implementation files (.fs) should navigate to other implementation files if isSignatureFile initialDoc.FilePath then + // Target range will point to .fsi file if only there is one so we can just use Roslyn navigation service. return gtd.TryNavigateToTextSpan(targetDoc, targetTextSpan, cancellationToken) else + // Navigation request was made in a .fs file, so we try to find the implmentation of the symbol at target range. + // This is the part that may take some time, because of type checks involved. let! result = gtd.NavigateToSymbolDefinitionAsync(targetDoc, targetSource, range, cancellationToken) |> liftAsync if result.IsNone then - // fallback to fast navigation + // In case the above fails, we just navigate to target range. return gtd.TryNavigateToTextSpan(targetDoc, targetTextSpan, cancellationToken) } |> Async.Ignore, From 510fe216110a3be4d3e9789e0c0e06dfdc2bddb7 Mon Sep 17 00:00:00 2001 From: majocha Date: Mon, 17 Apr 2023 18:31:12 +0200 Subject: [PATCH 4/4] add comment --- vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs b/vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs index a16ec3ebeef..8eea7460ba9 100644 --- a/vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs +++ b/vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs @@ -600,6 +600,10 @@ type internal FSharpNavigation(metadataAsSource: FSharpMetadataAsSourceService, |> Async.Ignore, cancellationToken )), + // Default wait time before VS shows the dialog allowing to cancel the long running task is 2 seconds. + // This seems a bit too long to leave the user without any feedback, so we shorten it to 1 second. + // Note: it seems anything less than 1 second will get rounded down to zero, resulting in flashing dialog + // on each navigation, so 1 second is as low as we cen get from JoinableTaskFactory. TimeSpan.FromSeconds 1 ) with :? OperationCanceledException ->