diff --git a/docs/release-notes/.VisualStudio/18.vNext.md b/docs/release-notes/.VisualStudio/18.vNext.md index ba03f663967..03226c61ad3 100644 --- a/docs/release-notes/.VisualStudio/18.vNext.md +++ b/docs/release-notes/.VisualStudio/18.vNext.md @@ -15,6 +15,8 @@ * Fix doubled F# diagnostics in tooltips. ([Issue #16360](https://github.com/dotnet/fsharp/issues/16360)) * Fix `NotSupportedException` in the memory-mapped-file optimization when copying `ReadOnlyMemory` into `MemoryMappedFileViewStream`. ([Issue #20263](https://github.com/dotnet/fsharp/issues/20263)) * Reduce allocations in the VS project options reactor: the command-line options and project options caches and the mailbox reply payloads now hold struct tuples, and `IProjectSite.CompilationBinOutputPath` returns `string voption` picked with a new `Array.tryPickV`. ([PR #20413](https://github.com/dotnet/fsharp/pull/20413)) +* Fix semantic colours of open documents disappearing: their classification was stored in the unopened-documents cache and only covered the requested span, so scrolling re-ran the checker or got nothing back, and a check that could not complete answered with no classifications, which clears the colours already shown. ([Issue #20445](https://github.com/dotnet/fsharp/issues/20445), [PR #20450](https://github.com/dotnet/fsharp/pull/20450)) +* Requests for one version of an open document share a single whole-file semantic classification, kept for the life of the document instead of two seconds, so scrolling a large file after a pause no longer re-walks every symbol in it. ([PR #20452](https://github.com/dotnet/fsharp/pull/20452)) ### Changed diff --git a/vsintegration/src/FSharp.Editor/Classification/ClassificationService.fs b/vsintegration/src/FSharp.Editor/Classification/ClassificationService.fs index 738003d5e3a..3eed5e4109f 100644 --- a/vsintegration/src/FSharp.Editor/Classification/ClassificationService.fs +++ b/vsintegration/src/FSharp.Editor/Classification/ClassificationService.fs @@ -4,10 +4,13 @@ namespace Microsoft.VisualStudio.FSharp.Editor open System open System.Composition +open System.Collections.Concurrent open System.Collections.Generic open System.Collections.Immutable open System.Threading +open System.Threading.Tasks open System.Runtime.Caching +open System.Runtime.CompilerServices open Microsoft.CodeAnalysis open Microsoft.CodeAnalysis.Classification @@ -30,6 +33,63 @@ open Microsoft.VisualStudio.FSharp.Editor.Telemetry type SemanticClassificationData = SemanticClassificationView type SemanticClassificationLookup = IReadOnlyDictionary> +/// What a classification answers for: the text it colours and the semantics it was checked against. +/// An edit elsewhere in the project reclassifies this document without touching its text. +[] +type internal ClassificationVersion = + { + TextVersion: VersionStamp + SemanticVersion: VersionStamp + } + +/// The whole-file semantic classification of one version of an open document. +type internal OpenDocumentClassification = + { + Version: ClassificationVersion + Text: SourceText + Lookup: SemanticClassificationLookup + } + +type internal ClassifyWholeFile = CancellationToken -> Task + +/// One classification of a document version, shared by every request that arrives while it runs. +/// The computation starts with the first waiter and is cancelled only when the last one leaves. +[] +type internal InFlightClassification(version: ClassificationVersion, compute: ClassifyWholeFile) = + let cts = new CancellationTokenSource() + let job = lazy (compute cts.Token) + let mutable waiters = 0 + + member _.Version = version + + member _.IsCancelled = cts.IsCancellationRequested + + member _.IsCompleted = job.IsValueCreated && job.Value.IsCompleted + + member _.Join(cancellationToken: CancellationToken) : Task = + Interlocked.Increment &waiters |> ignore + let job = job.Value + let left = ref 0 + + let leave () = + if + Interlocked.Exchange(left, 1) = 0 + && Interlocked.Decrement &waiters = 0 + && not job.IsCompleted + then + cts.Cancel() + + task { + use _ = cancellationToken.Register(fun () -> leave ()) + + try + let! _ = Task.WhenAny(job, Task.Delay(Timeout.Infinite, cancellationToken)) + cancellationToken.ThrowIfCancellationRequested() + return! job + finally + leave () + } + [)>] type internal FSharpClassificationService [] () = @@ -146,8 +206,96 @@ type internal FSharpClassificationService [] () = static let unopenedDocumentsSemanticClassificationCache = new DocumentCache("fsharp-unopened-documents-semantic-classification-cache", 5.) - static let openedDocumentsSemanticClassificationCache = - new DocumentCache("fsharp-opened-documents-semantic-classification-cache", 2.) + // The classification of an open document's latest checked version. Roslyn asks for it span by span + // for as long as that version is on screen, and replaces a span's semantic tags with whatever comes + // back - so it is kept for the life of the document rather than expiring, and when the checker + // cannot answer (project loading or reloading, a superseded check) it is re-emitted rather than + // answering "nothing", which would strip the colours the user already sees. + static let openDocumentClassifications = + ConditionalWeakTable() + + static let inFlightClassifications = + ConcurrentDictionary() + + static let remember (documentId: DocumentId) (classification: OpenDocumentClassification) = + // net472 has no ConditionalWeakTable.AddOrUpdate. + lock openDocumentClassifications (fun () -> + openDocumentClassifications.Remove documentId |> ignore + openDocumentClassifications.Add(documentId, classification)) + + // Only for the text it was computed from: the lookup names positions, so against edited text it + // would colour the wrong characters. + static let addLastGood (documentId: DocumentId) (sourceText: SourceText) (targetSpan: TextSpan) (result: List) = + match openDocumentClassifications.TryGetValue documentId with + | true, classification when classification.Text.ContentEquals sourceText -> + addSemanticClassificationByLookup sourceText targetSpan classification.Lookup result + | _ -> () + + static let addLastGoodForCurrentText (document: Document) (targetSpan: TextSpan) (result: List) = + match document.TryGetText() with + | true, sourceText -> addLastGood document.Id sourceText targetSpan result + | _ -> () + + static let classifyWholeFile (document: Document) (version: ClassificationVersion) (sourceText: SourceText) = + cancellableTask { + match! document.TryGetFSharpParseAndCheckResultsAsync(nameof (IFSharpClassificationService)) with + | ValueNone -> return ValueNone + | ValueSome(struct (_, checkResults)) -> + let classificationData = + checkResults.GetSemanticClassification(None, RelatedSymbolUseKind.All) + + // Every checked file resolves at least its enclosing module, so nothing here means + // the classification itself failed (SemanticClassification.fs recovers with an empty + // array). Remembering that would pin the version to no colours. + if classificationData.Length = 0 then + return ValueNone + else + let classification = + { + Version = version + Text = sourceText + Lookup = itemToSemanticClassificationLookup classificationData + } + + remember document.Id classification + return ValueSome classification + } + + // Requests for the same version that overlap - split views, the taggers above and below the + // viewport - share one classification instead of each walking the whole file. + static let classifyOpenDocument (document: Document) (version: ClassificationVersion) (sourceText: SourceText) = + cancellableTask { + let! cancellationToken = CancellableTask.getCancellationToken () + + let start () = + InFlightClassification(version, classifyWholeFile document version sourceText) + + let inFlight = + inFlightClassifications.AddOrUpdate( + document.Id, + (fun _ -> start ()), + fun _ running -> + if running.Version = version && not running.IsCancelled then + running + else + start () + ) + + try + return! inFlight.Join cancellationToken + finally + // A waiter that leaves early keeps the entry for those still waiting. + if inFlight.IsCompleted || inFlight.IsCancelled then + (inFlightClassifications :> ICollection>).Remove(KeyValuePair(document.Id, inFlight)) + |> ignore + } + + // Which store a document lands in is not observable from its classifications - a miss only costs + // a recheck - so tests reach them directly to tell the branches apart. + static member internal OpenDocumentClassifications = openDocumentClassifications + + static member internal UnopenedDocumentsSemanticClassificationCache = + unopenedDocumentsSemanticClassificationCache interface IFSharpClassificationService with // Do not perform classification if we don't have project options (#defines matter) @@ -252,15 +400,25 @@ type internal FSharpClassificationService [] () = use _eventDuration = TelemetryReporter.ReportSingleEventWithDuration(TelemetryEvents.AddSemanticClassifications, eventProps) - let! classificationData = document.GetFSharpSemanticClassificationAsync(nameof (FSharpClassificationService)) - - let classificationDataLookup = toSemanticClassificationLookup classificationData - do! unopenedDocumentsSemanticClassificationCache.SetAsync(document, classificationDataLookup) - addSemanticClassificationByLookup sourceText textSpan classificationDataLookup result + match! document.TryGetFSharpSemanticClassificationAsync(nameof (FSharpClassificationService)) with + | ValueNone -> () + | ValueSome classificationData -> + let classificationDataLookup = toSemanticClassificationLookup classificationData + do! unopenedDocumentsSemanticClassificationCache.SetAsync(document, classificationDataLookup) + addSemanticClassificationByLookup sourceText textSpan classificationDataLookup result else - match! openedDocumentsSemanticClassificationCache.TryGetValueAsync document with - | ValueSome classificationDataLookup -> + let! textVersion = document.GetTextVersionAsync(cancellationToken) + let! semanticVersion = document.Project.GetDependentSemanticVersionAsync(cancellationToken) + + let version = + { + TextVersion = textVersion + SemanticVersion = semanticVersion + } + + match openDocumentClassifications.TryGetValue document.Id with + | true, classification when classification.Version = version -> let eventProps: (string * obj) array = [| "context.document.project.id", document.Project.Id.Id.ToString() @@ -273,8 +431,8 @@ type internal FSharpClassificationService [] () = use _eventDuration = TelemetryReporter.ReportSingleEventWithDuration(TelemetryEvents.AddSemanticClassifications, eventProps) - addSemanticClassificationByLookup sourceText textSpan classificationDataLookup result - | ValueNone -> + addSemanticClassificationByLookup sourceText textSpan classification.Lookup result + | _ -> let eventProps: (string * obj) array = [| @@ -288,21 +446,13 @@ type internal FSharpClassificationService [] () = use _eventDuration = TelemetryReporter.ReportSingleEventWithDuration(TelemetryEvents.AddSemanticClassifications, eventProps) - let! _, checkResults = document.GetFSharpParseAndCheckResultsAsync(nameof (IFSharpClassificationService)) - - let targetRange = - RoslynHelpers.TextSpanToFSharpRange(document.FilePath, textSpan, sourceText) - - let classificationData = - checkResults.GetSemanticClassification(Some targetRange, RelatedSymbolUseKind.All) - - if classificationData.Length > 0 then - let classificationDataLookup = itemToSemanticClassificationLookup classificationData - do! unopenedDocumentsSemanticClassificationCache.SetAsync(document, classificationDataLookup) - - addSemanticClassification sourceText textSpan classificationData result + match! classifyOpenDocument document version sourceText with + | ValueSome classification -> addSemanticClassificationByLookup sourceText textSpan classification.Lookup result + | ValueNone -> addLastGood document.Id sourceText textSpan result } - |> CancellableTask.ifCanceledReturn () + // A cancellation that is not Roslyn's own (a superseded or aborted check surfaces as one) + // must not turn into an empty answer, which Roslyn would paint as "no colours". + |> CancellableTask.ifCanceledThen (fun () -> addLastGoodForCurrentText document textSpan result) |> CancellableTask.startAsTask cancellationToken // Do not perform classification if we don't have project options (#defines matter) diff --git a/vsintegration/src/FSharp.Editor/Common/CancellableTasks.fs b/vsintegration/src/FSharp.Editor/Common/CancellableTasks.fs index 7520395a084..29e54491a86 100644 --- a/vsintegration/src/FSharp.Editor/Common/CancellableTasks.fs +++ b/vsintegration/src/FSharp.Editor/Common/CancellableTasks.fs @@ -1160,6 +1160,17 @@ module CancellableTasks = return value } + /// If this CancellableTask gets canceled for another reason than the token being canceled, run the fallback instead. + let inline ifCanceledThen ([] fallback: unit -> unit) (ctask: CancellableTask) = + cancellableTask { + let! ct = getCancellationToken () + + try + return! ctask + with :? OperationCanceledException when ct.IsCancellationRequested = false -> + return fallback () + } + /// [] module MergeSourcesExtensions = diff --git a/vsintegration/src/FSharp.Editor/Common/DocumentCache.fs b/vsintegration/src/FSharp.Editor/Common/DocumentCache.fs index 9233d95e8f4..5c578857e32 100644 --- a/vsintegration/src/FSharp.Editor/Common/DocumentCache.fs +++ b/vsintegration/src/FSharp.Editor/Common/DocumentCache.fs @@ -18,17 +18,26 @@ type DocumentCache<'Value when 'Value: not struct>(name: string, ?cacheItemPolic let policy = defaultArg cacheItemPolicy (CacheItemPolicy(SlidingExpiration = (TimeSpan.FromSeconds defaultSlidingExpiration))) + // A document's own text is not the whole story: an edit elsewhere in the project can change what + // its names mean while its text stands still, so the key has to cover both versions. + static let currentVersion (doc: Document) (ct: CancellationToken) = + task { + let! textVersion = doc.GetTextVersionAsync ct + let! semanticVersion = doc.Project.GetDependentSemanticVersionAsync ct + return textVersion, semanticVersion + } + static let tryGetCachedValueAsync (doc: Document, cache: MemoryCache, ct: CancellationToken) = if ct.IsCancellationRequested then Task.FromCanceled<'Value voption>(ct) else task { - let! currentVersion = doc.GetTextVersionAsync ct + let! version = currentVersion doc ct match cache.Get(doc.Id.ToString()) with | null -> return ValueNone - | :? (VersionStamp * 'Value) as value -> - if fst value = currentVersion then + | :? ((VersionStamp * VersionStamp) * 'Value) as value -> + if fst value = version then return ValueSome(snd value) else return ValueNone @@ -40,8 +49,8 @@ type DocumentCache<'Value when 'Value: not struct>(name: string, ?cacheItemPolic Task.FromCanceled(ct) else task { - let! currentVersion = doc.GetTextVersionAsync ct - do cache.Set(doc.Id.ToString(), (currentVersion, value), policy) + let! version = currentVersion doc ct + do cache.Set(doc.Id.ToString(), (version, value), policy) } new(name: string, slidingExpirationSeconds: float) = diff --git a/vsintegration/src/FSharp.Editor/LanguageService/WorkspaceExtensions.fs b/vsintegration/src/FSharp.Editor/LanguageService/WorkspaceExtensions.fs index 2406f3a6e32..ffcf870ec34 100644 --- a/vsintegration/src/FSharp.Editor/LanguageService/WorkspaceExtensions.fs +++ b/vsintegration/src/FSharp.Editor/LanguageService/WorkspaceExtensions.fs @@ -24,14 +24,22 @@ open System.Text.Json.Nodes #nowarn "57" // Experimental stuff +/// Everything the checker needs for one Roslyn project, resolved once and cached per project. +type internal FSharpCompilationOptions = + { + Checker: FSharpChecker + OptionsManager: FSharpProjectOptionsManager + ParsingOptions: FSharpParsingOptions + ProjectOptions: FSharpProjectOptions + } + [] module internal ProjectCache = /// This is a cache to maintain FSharpParsingOptions and FSharpProjectOptions per Roslyn Project. /// The Roslyn Project is held weakly meaning when it is cleaned up by the GC, the FSharParsingOptions and FSharpProjectOptions will be cleaned up by the GC. /// At some point, this will be the main caching mechanism for FCS projects instead of FCS itself. - let Projects = - ConditionalWeakTable() + let Projects = ConditionalWeakTable() module internal SolutionConfigCache = @@ -170,12 +178,12 @@ module private CheckerExtensions = let exist xs = xs |> Seq.isEmpty |> not - let getFSharpOptionsForProject (this: Project) = + let tryGetFSharpOptionsForProject (this: Project) : CancellableTask = if not this.IsFSharp then - raise (OperationCanceledException("Project is not a FSharp project.")) + CancellableTask.singleton ValueNone else match ProjectCache.Projects.TryGetValue(this) with - | true, result -> CancellableTask.singleton result + | true, result -> CancellableTask.singleton (ValueSome result) | _ -> cancellableTask { @@ -185,14 +193,32 @@ module private CheckerExtensions = let projectOptionsManager = service.FSharpProjectOptionsManager match! projectOptionsManager.TryGetOptionsByProject(this, ct) with - | ValueNone -> return raise (OperationCanceledException("FSharp project options not found.")) + | ValueNone -> return ValueNone | ValueSome(parsingOptions, projectOptions) -> let result = - (service.Checker, projectOptionsManager, parsingOptions, projectOptions) + { + Checker = service.Checker + OptionsManager = projectOptionsManager + ParsingOptions = parsingOptions + ProjectOptions = projectOptions + } - return ProjectCache.Projects.GetValue(this, ConditionalWeakTable<_, _>.CreateValueCallback(fun _ -> result)) + return + ValueSome(ProjectCache.Projects.GetValue(this, ConditionalWeakTable<_, _>.CreateValueCallback(fun _ -> result))) } + // The raising members predate the `Try` ones; they keep their tuple shape until every caller has + // moved over, so a missing result stays an OperationCanceledException with the same message there. + let getFSharpOptionsForProject (this: Project) = + if not this.IsFSharp then + raise (OperationCanceledException("Project is not a FSharp project.")) + else + cancellableTask { + match! tryGetFSharpOptionsForProject this with + | ValueSome options -> return options.Checker, options.OptionsManager, options.ParsingOptions, options.ProjectOptions + | ValueNone -> return raise (OperationCanceledException("FSharp project options not found.")) + } + let documentToSnapshot (document: Document) = cancellableTask { let! version = document.GetTextVersionAsync() @@ -512,13 +538,14 @@ module private CheckerExtensions = type Document with - /// Get the FSharpParsingOptions and FSharpProjectOptions from the F# project that is associated with the given F# document. - member this.GetFSharpCompilationOptionsAsync(userOpName) = + /// Get the compilation options of the F# project that is associated with the given F# document, + /// or ValueNone while the project has none yet (still loading, reloading, a miscellaneous file). + member this.TryGetFSharpCompilationOptionsAsync(userOpName) : CancellableTask = if not this.Project.IsFSharp then - raise (OperationCanceledException("Document is not a FSharp document.")) + CancellableTask.singleton ValueNone else match ProjectCache.Projects.TryGetValue(this.Project) with - | true, result -> CancellableTask.singleton result + | true, result -> CancellableTask.singleton (ValueSome result) | _ -> cancellableTask { let service = this.Project.Solution.GetFSharpWorkspaceService() @@ -526,14 +553,36 @@ type Document with let! ct = CancellableTask.getCancellationToken () match! projectOptionsManager.TryGetOptionsForDocumentOrProject(this, ct, userOpName) with - | ValueNone -> return raise (OperationCanceledException("FSharp project options not found.")) + | ValueNone -> return ValueNone | ValueSome(parsingOptions, projectOptions) -> let result = - (service.Checker, projectOptionsManager, parsingOptions, projectOptions) + { + Checker = service.Checker + OptionsManager = projectOptionsManager + ParsingOptions = parsingOptions + ProjectOptions = projectOptions + } - return ProjectCache.Projects.GetValue(this.Project, ConditionalWeakTable<_, _>.CreateValueCallback(fun _ -> result)) + return + ValueSome( + ProjectCache.Projects.GetValue( + this.Project, + ConditionalWeakTable<_, _>.CreateValueCallback(fun _ -> result) + ) + ) } + /// Get the FSharpParsingOptions and FSharpProjectOptions from the F# project that is associated with the given F# document. + member this.GetFSharpCompilationOptionsAsync(userOpName) = + if not this.Project.IsFSharp then + raise (OperationCanceledException("Document is not a FSharp document.")) + else + cancellableTask { + match! this.TryGetFSharpCompilationOptionsAsync(userOpName) with + | ValueSome options -> return options.Checker, options.OptionsManager, options.ParsingOptions, options.ProjectOptions + | ValueNone -> return raise (OperationCanceledException("FSharp project options not found.")) + } + /// Get the compilation defines and language version from F# project that is associated with the given F# document. member this.GetFsharpParsingOptionsAsync(userOpName) = async { @@ -581,33 +630,55 @@ type Document with return! checker.ParseDocument(this, parsingOptions, userOpName) } + /// Parses and checks the given F# document; ValueNone while its project has no compilation options + /// or the check was aborted. + member this.TryGetFSharpParseAndCheckResultsAsync + (userOpName) + : CancellableTask = + cancellableTask { + match! this.TryGetFSharpCompilationOptionsAsync(userOpName) with + | ValueNone -> return ValueNone + | ValueSome options -> + match! options.Checker.ParseAndCheckDocument(this, options.ProjectOptions, userOpName, allowStaleResults = false) with + | Some(parseResults, checkResults) -> return ValueSome(struct (parseResults, checkResults)) + | None -> return ValueNone + } + /// Parses and checks the given F# document. member this.GetFSharpParseAndCheckResultsAsync(userOpName) = cancellableTask { - let! checker, _, _, projectOptions = this.GetFSharpCompilationOptionsAsync(userOpName) + match! this.TryGetFSharpParseAndCheckResultsAsync(userOpName) with + | ValueSome(struct (parseResults, checkResults)) -> return parseResults, checkResults + | ValueNone -> return raise (OperationCanceledException("Unable to get FSharp parse and check results.")) + } - match! checker.ParseAndCheckDocument(this, projectOptions, userOpName, allowStaleResults = false) with - | Some results -> return results - | _ -> return raise (OperationCanceledException("Unable to get FSharp parse and check results.")) + /// Get the semantic classifications of the given F# document; ValueNone while its project has no + /// compilation options or the background check produced none. + member this.TryGetFSharpSemanticClassificationAsync + (userOpName) + : CancellableTask = + cancellableTask { + match! this.TryGetFSharpCompilationOptionsAsync(userOpName) with + | ValueNone -> return ValueNone + | ValueSome options -> + let! result = + if this.Project.UseTransparentCompiler then + async { + let! projectSnapshot = getProjectSnapshotForDocument (this, options.ProjectOptions) + return! options.Checker.GetBackgroundSemanticClassificationForFile(this.FilePath, projectSnapshot) + } + else + options.Checker.GetBackgroundSemanticClassificationForFile(this.FilePath, options.ProjectOptions) + + return ValueOption.ofOption result } /// Get the semantic classifications of the given F# document. member this.GetFSharpSemanticClassificationAsync(userOpName) = cancellableTask { - let! checker, _, _, projectOptions = this.GetFSharpCompilationOptionsAsync(userOpName) - - let! result = - if this.Project.UseTransparentCompiler then - async { - let! projectSnapshot = getProjectSnapshotForDocument (this, projectOptions) - return! checker.GetBackgroundSemanticClassificationForFile(this.FilePath, projectSnapshot) - } - else - checker.GetBackgroundSemanticClassificationForFile(this.FilePath, projectOptions) - - return - result - |> Option.defaultWith (fun _ -> raise (OperationCanceledException("Unable to get FSharp semantic classification."))) + match! this.TryGetFSharpSemanticClassificationAsync(userOpName) with + | ValueSome classification -> return classification + | ValueNone -> return raise (OperationCanceledException("Unable to get FSharp semantic classification.")) } /// Find F# references in the given F# document. diff --git a/vsintegration/tests/FSharp.Editor.Tests/DocumentCacheTests.fs b/vsintegration/tests/FSharp.Editor.Tests/DocumentCacheTests.fs new file mode 100644 index 00000000000..74350cfb9fe --- /dev/null +++ b/vsintegration/tests/FSharp.Editor.Tests/DocumentCacheTests.fs @@ -0,0 +1,78 @@ +// Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information. + +namespace FSharp.Editor.Tests + +open System.Threading +open Xunit +open Microsoft.CodeAnalysis +open Microsoft.CodeAnalysis.Text +open Microsoft.VisualStudio.FSharp.Editor +open FSharp.Editor.Tests.Helpers + +type DocumentCacheTests() = + // Two files of one project: `Other` compiles ahead of `Target`, so a change to `Other` bumps the + // project's dependent semantic version without touching `Target`'s own text. + let openTwoFileProject () = + let projectId = ProjectId.CreateNewId() + let otherPath = "C:\Other.fs" + let targetPath = "C:\Target.fs" + + let otherInfo = + RoslynTestHelpers.CreateDocumentInfo projectId otherPath "let value = 1" + + let targetInfo = + RoslynTestHelpers.CreateDocumentInfo projectId targetPath "let read () = 1" + + let projectInfo = + RoslynTestHelpers.CreateProjectInfo projectId "C:\test.fsproj" [ otherInfo; targetInfo ] + + let solution = RoslynTestHelpers.CreateSolution [ projectInfo ] + + { RoslynTestHelpers.DefaultProjectOptions with + SourceFiles = [| otherPath; targetPath |] + } + |> RoslynTestHelpers.SetProjectOptions projectId solution + + otherInfo.Id, targetInfo.Id, solution.Workspace + + let tryGetValue (cache: DocumentCache) document = + (cache.TryGetValueAsync document CancellationToken.None).GetAwaiter().GetResult() + + let setValue (cache: DocumentCache) document value = + (cache.SetAsync (document, value) CancellationToken.None).GetAwaiter().GetResult() + + // A document's own text is not the whole story a cached value depends on: an edit elsewhere in + // the project can change what its names mean while its text stands still. + [] + member _.``A cached value is invalidated when the project's dependent semantic version changes``() = + use cache = new DocumentCache("DocumentCacheTests") + let otherId, targetId, workspace = openTwoFileProject () + + let targetDocument () = + workspace.CurrentSolution.GetDocument targetId + + setValue cache (targetDocument ()) "cached" + + Assert.True((tryGetValue cache (targetDocument ())).IsSome, "The value must be readable before anything changes.") + + Assert.True( + workspace.TryApplyChanges(workspace.CurrentSolution.WithDocumentText(otherId, SourceText.From "let value = 2")), + "The workspace has to accept the change to the other file." + ) + + Assert.True( + (tryGetValue cache (targetDocument ())).IsNone, + "A change to another file of the project must invalidate the cached value." + ) + + [] + member _.``A cached value survives when nothing about the project has changed``() = + use cache = new DocumentCache("DocumentCacheTests") + let _, targetId, workspace = openTwoFileProject () + + let targetDocument () = + workspace.CurrentSolution.GetDocument targetId + + setValue cache (targetDocument ()) "cached" + + Assert.Equal(ValueSome "cached", tryGetValue cache (targetDocument ())) diff --git a/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj b/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj index ecce1205b8c..a743d8c8240 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj +++ b/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj @@ -82,6 +82,7 @@ + diff --git a/vsintegration/tests/FSharp.Editor.Tests/SemanticClassificationServiceTests.fs b/vsintegration/tests/FSharp.Editor.Tests/SemanticClassificationServiceTests.fs index 6c58f658ee9..a5a28ee06a5 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/SemanticClassificationServiceTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/SemanticClassificationServiceTests.fs @@ -2,12 +2,18 @@ namespace FSharp.Editor.Tests +open System +open System.Collections.Generic +open System.Threading +open System.Threading.Tasks open Xunit +open Microsoft.CodeAnalysis open Microsoft.VisualStudio.FSharp.Editor open FSharp.Compiler.EditorServices open FSharp.Compiler.Text open Microsoft.CodeAnalysis.Text open Microsoft.CodeAnalysis.Classification +open Microsoft.CodeAnalysis.ExternalAccess.FSharp.Classification open FSharp.Compiler.CodeAnalysis open FSharp.Editor.Tests.Helpers open FSharp.Test @@ -31,6 +37,94 @@ type SemanticClassificationServiceTests() = |> Option.toList |> List.collect Array.toList + let openDocument (source: string) = + let solution = RoslynTestHelpers.CreateSolution source + let workspace = solution.Workspace + let documentId = (RoslynTestHelpers.GetSingleDocument solution).Id + workspace.OpenDocument documentId + let document = workspace.CurrentSolution.GetDocument documentId + Assert.True(workspace.IsDocumentOpen documentId, "The document under test has to be open.") + document + + let sourceTextOf (document: Document) = + document.GetTextAsync(CancellationToken.None).GetAwaiter().GetResult() + + let classifyWith (ct: CancellationToken) (document: Document) (span: TextSpan) = + let result = ResizeArray() + + (FSharpClassificationService() :> IFSharpClassificationService) + .AddSemanticClassificationsAsync(document, span, result, ct) + .GetAwaiter() + .GetResult() + + List.ofSeq result + + let classify document span = + classifyWith CancellationToken.None document span + + let isCached (cache: DocumentCache) (document: Document) = + (cache.TryGetValueAsync document CancellationToken.None).GetAwaiter().GetResult().IsSome + + let versionOf (document: Document) = + { + TextVersion = document.GetTextVersionAsync(CancellationToken.None).GetAwaiter().GetResult() + SemanticVersion = document.Project.GetDependentSemanticVersionAsync(CancellationToken.None).GetAwaiter().GetResult() + } + + let isRemembered (document: Document) = + match FSharpClassificationService.OpenDocumentClassifications.TryGetValue document.Id with + | true, classification -> classification.Version = versionOf document + | _ -> false + + let someVersion () = + { + TextVersion = VersionStamp.Create() + SemanticVersion = VersionStamp.Create() + } + + let lineSpan (text: SourceText) firstLine lastLine = + TextSpan.FromBounds(text.Lines[firstLine].Start, text.Lines[lastLine].End) + + let clearProjectOptions (document: Document) = + document.Project.Solution.Workspace.Services.GetService().FSharpProjectOptionsManager.ClearAllCaches() + + // Two files of one project, the second using what the first declares, with the second one open. + let openDependentDocument (declarations: string) (usage: string) = + let projectId = ProjectId.CreateNewId() + let declarationsPath = "C:\\declarations.fs" + let usagePath = "C:\\usage.fs" + + let declarationsInfo = + RoslynTestHelpers.CreateDocumentInfo projectId declarationsPath declarations + + let usageInfo = RoslynTestHelpers.CreateDocumentInfo projectId usagePath usage + + let projectInfo = + RoslynTestHelpers.CreateProjectInfo projectId "C:\\test.fsproj" [ declarationsInfo; usageInfo ] + + let solution = RoslynTestHelpers.CreateSolution [ projectInfo ] + + { RoslynTestHelpers.DefaultProjectOptions with + SourceFiles = [| declarationsPath; usagePath |] + } + |> RoslynTestHelpers.SetProjectOptions projectId solution + + solution.Workspace.OpenDocument usageInfo.Id + declarationsInfo.Id, usageInfo.Id, solution.Workspace + + // A project whose options were never supplied, i.e. one Visual Studio is still loading. + let openDocumentWithoutProjectOptions (source: string) = + let projectId = ProjectId.CreateNewId() + let documentInfo = RoslynTestHelpers.CreateDocumentInfo projectId "test.fs" source + + let projectInfo = + RoslynTestHelpers.CreateProjectInfo projectId "test.fsproj" [ documentInfo ] + + let solution = RoslynTestHelpers.CreateSolution [ projectInfo ] + let documentId = (RoslynTestHelpers.GetSingleDocument solution).Id + solution.Workspace.OpenDocument documentId + solution.Workspace.CurrentSolution.GetDocument documentId + let verifyClassificationAtEndOfMarker (fileContents: string, marker: string, classificationType: string) = let text = SourceText.From(fileContents) let ranges = getRanges fileContents @@ -416,3 +510,200 @@ let result2 = s.(*2*)IsHyperbolicCaseWithLongName && Range.rangeContainsPos item.Range longCasePos) Assert.True(longCaseUnionItems.Length > 0, "Expected a UnionCase classification covering 'IsHyperbolicCaseWithLongName'") + + // Which cache a document lands in is invisible in its classifications - a miss only costs a + // recheck - so these reach the caches directly. Splitting one cache in two (#15954) left the + // open-document branch reading the opened cache and writing the unopened one, so the opened + // cache was never populated and every request for an open file re-ran the checker. + [] + member _.``Semantic classification of an open document is cached for opened documents``() = + let document = openDocument "let x = 1" + let text = sourceTextOf document + + Assert.NotEmpty(classify document (TextSpan(0, text.Length))) + + Assert.True(isRemembered document, "Classifying an open document must remember its classification.") + + Assert.False( + isCached FSharpClassificationService.UnopenedDocumentsSemanticClassificationCache document, + "An open document must not be cached as an unopened one." + ) + + // The cache is keyed by version, not by span, so what it holds has to cover the whole file: + // Roslyn asks for the visible span, then for other spans of the same version as the user scrolls. + [] + member _.``Semantic classification computed for one span of an open document serves another span at the same version``() = + let source = + [ + "type R = { Doop: int }" + "let r = { Doop = 12 }" + "" + "let mutable first = 12" + "let g () = first" + ] + |> String.concat "\n" + + let document = openDocument source + let text = sourceTextOf document + let spanA = lineSpan text 0 1 + let spanB = lineSpan text 3 4 + Assert.False(spanA.IntersectsWith spanB) + + let first = classify document spanA + Assert.NotEmpty first + + let second = classify document spanB + Assert.NotEmpty second + Assert.All(second, fun span -> Assert.True(spanB.Contains span.TextSpan)) + + // A freshly opened copy has a new DocumentId and therefore cold caches: a direct computation for B. + Assert.Equal(classify (openDocument source) spanB, second) + Assert.Equal(first, classify document spanA) + + // What a document's symbols mean is decided by the whole project, so an edit in another file + // reclassifies this one while its own text version stays put. + [] + member _.``Semantic classification of an open document follows a change to another file of its project``() = + let declarationsId, usageId, workspace = + openDependentDocument "module Declarations\nlet counter = 1" "open Declarations\nlet read () = counter" + + let classifyUsage () = + let document = workspace.CurrentSolution.GetDocument usageId + + classify document (TextSpan(0, (sourceTextOf document).Length)) + |> List.map _.ClassificationType + + let before = classifyUsage () + Assert.NotEmpty before + Assert.DoesNotContain(FSharpClassificationTypes.MutableVar, before) + + let mutableCounter = SourceText.From "module Declarations\nlet mutable counter = 1" + + Assert.True( + workspace.TryApplyChanges(workspace.CurrentSolution.WithDocumentText(declarationsId, mutableCounter)), + "The workspace has to accept the change to the other file." + ) + + Assert.Contains(FSharpClassificationTypes.MutableVar, classifyUsage ()) + + // Roslyn replaces a span's tags with whatever comes back, so "no result" must re-emit the last + // good one rather than strip the colours the user already sees. + [] + member _.``Semantic classification serves the last good lookup when project options become unavailable``() = + let source = "let x = 1\nlet y = 2" + let document = openDocument source + let text = sourceTextOf document + Assert.NotEmpty(classify document (TextSpan(0, text.Length))) + + clearProjectOptions document + // A new text version misses the versioned cache; the text itself is unchanged, so the last + // good lookup still describes it. + let reopened = document.WithText(SourceText.From source) + let reopenedText = sourceTextOf reopened + + Assert.NotEmpty(classify reopened (TextSpan(0, reopenedText.Length))) + + Assert.False(isRemembered reopened, "A miss must not be remembered as a result.") + + // The lookup names positions in the text it was computed from, so against edited text it would + // colour the wrong characters. + [] + member _.``Semantic classification does not serve the last good lookup for edited text``() = + let document = openDocument "let x = 1\nlet y = 2" + let text = sourceTextOf document + Assert.NotEmpty(classify document (TextSpan(0, text.Length))) + + clearProjectOptions document + let edited = document.WithText(SourceText.From "// a comment\nlet x = 1\nlet y = 2") + let editedText = sourceTextOf edited + + Assert.Empty(classify edited (TextSpan(0, editedText.Length))) + + [] + member _.``Semantic classification without project options and without an earlier result returns nothing and caches nothing``() = + let document = openDocumentWithoutProjectOptions "let x = 1" + let text = sourceTextOf document + + Assert.Empty(classify document (TextSpan(0, text.Length))) + Assert.False(isRemembered document) + Assert.False(isCached FSharpClassificationService.UnopenedDocumentsSemanticClassificationCache document) + + // Roslyn keeps the previous tags only for a cancellation carrying its own token; nothing may catch it. + [] + member _.``Semantic classification propagates cancellation as a canceled task``() = + let document = openDocument "let x = 1" + let text = sourceTextOf document + + let task = + (FSharpClassificationService() :> IFSharpClassificationService) + .AddSemanticClassificationsAsync(document, TextSpan(0, text.Length), ResizeArray(), CancellationToken(true)) + + Assert.ThrowsAny(fun () -> task.GetAwaiter().GetResult()) + |> ignore + + Assert.True task.IsCanceled + + // Roslyn's viewport taggers ask for the same version at once; the file must be walked once for all of them. + [] + member _.``Overlapping requests for one version share a single classification``() = + let gate = TaskCompletionSource() + let computed = ref 0 + + let inFlight = + InFlightClassification( + someVersion (), + fun _ -> + Interlocked.Increment computed |> ignore + gate.Task + ) + + let first = inFlight.Join CancellationToken.None + let second = inFlight.Join CancellationToken.None + Assert.False(first.IsCompleted || second.IsCompleted) + + let classification = + { + Version = someVersion () + Text = SourceText.From "" + Lookup = Dictionary() + } + + gate.SetResult(ValueSome classification) + + Assert.Same(classification.Lookup, first.Result.Value.Lookup) + Assert.Same(classification.Lookup, second.Result.Value.Lookup) + Assert.Equal(1, computed.Value) + + [] + member _.``A shared classification is cancelled only when its last waiter leaves``() = + let gate = TaskCompletionSource() + let sharedToken = ref CancellationToken.None + + let inFlight = + InFlightClassification( + someVersion (), + fun ct -> + sharedToken.Value <- ct + gate.Task + ) + + use first = new CancellationTokenSource() + use second = new CancellationTokenSource() + let firstJoin = inFlight.Join first.Token + let secondJoin = inFlight.Join second.Token + + first.Cancel() + + Assert.ThrowsAny(fun () -> firstJoin.GetAwaiter().GetResult() |> ignore) + |> ignore + + Assert.False(sharedToken.Value.IsCancellationRequested, "One waiter leaving must not cancel the others.") + Assert.False(secondJoin.IsCompleted) + + second.Cancel() + + Assert.ThrowsAny(fun () -> secondJoin.GetAwaiter().GetResult() |> ignore) + |> ignore + + Assert.True(sharedToken.Value.IsCancellationRequested, "The last waiter leaving must cancel the work.") + Assert.True inFlight.IsCancelled