From e2f232a3a021122863f627a7f89194790f4b5253 Mon Sep 17 00:00:00 2001 From: Andrii Chebukin Date: Fri, 4 Sep 2026 19:24:39 +0200 Subject: [PATCH 1/5] Answer with a voption when F# project options are unavailable Every entry point in WorkspaceExtensions signalled "the project has no options yet" by raising an OperationCanceledException, which the callers then had to tell apart from a real cancellation. They cannot, so they treat both as "cancelled" and return an empty result. For a service Roslyn asks repeatedly while a solution loads, that is a wrong answer, not a missing one. Add `Try` siblings that return ValueNone instead, and keep the raising members as thin wrappers so the call sites that have not moved over still get the same exception with the same message. The four-tuple those members hand out becomes a named record, which is also what ProjectCache stores, so a cache hit hands back the instance it holds rather than rebuilding a tuple. Co-Authored-By: Claude Fable 5.1 --- .../LanguageService/WorkspaceExtensions.fs | 137 +++++++++++++----- 1 file changed, 104 insertions(+), 33 deletions(-) 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. From be6307e6f5200288496f2ddebcb5811d532ac17e Mon Sep 17 00:00:00 2001 From: Andrii Chebukin Date: Fri, 4 Sep 2026 19:24:40 +0200 Subject: [PATCH 2/5] Cache the whole-file classification of an open document Splitting the semantic classification cache in two (#15954) left the open-document branch reading the opened-documents cache and writing the unopened one, so the opened cache was never populated and every request for an open file re-ran the checker. What it wrote was also only the requested span, keyed by document and text version, so a second request for another span of the same version (scrolling, a split view, Roslyn asking around the viewport) would have hit that entry and sliced nothing out of it. Cache the classification of the whole file instead, and slice it per request, the way the unopened-documents branch already does. A check that cannot complete - the project is loading or reloading, or the check was superseded - must not answer with no classifications either: Roslyn replaces the tags of a span with whatever comes back, so an empty answer strips the colours the user is looking at, while a cancellation carrying Roslyn's own token leaves them alone. Keep the last classification per document and re-emit it for those requests, but only while the text it was computed from is still the current one, as its spans would otherwise land on the wrong characters. `ifCanceledThen` replaces `ifCanceledReturn ()` for the same reason: the checker relabels its internal cancellations with the caller's token, and those must produce the last known colours rather than none. Co-Authored-By: Claude Fable 5.1 --- docs/release-notes/.VisualStudio/18.vNext.md | 1 + .../Classification/ClassificationService.fs | 88 ++++++++-- .../FSharp.Editor/Common/CancellableTasks.fs | 11 ++ .../SemanticClassificationServiceTests.fs | 163 ++++++++++++++++++ 4 files changed, 244 insertions(+), 19 deletions(-) diff --git a/docs/release-notes/.VisualStudio/18.vNext.md b/docs/release-notes/.VisualStudio/18.vNext.md index ba03f663967..36224213908 100644 --- a/docs/release-notes/.VisualStudio/18.vNext.md +++ b/docs/release-notes/.VisualStudio/18.vNext.md @@ -15,6 +15,7 @@ * 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)) ### Changed diff --git a/vsintegration/src/FSharp.Editor/Classification/ClassificationService.fs b/vsintegration/src/FSharp.Editor/Classification/ClassificationService.fs index 738003d5e3a..c7dc7738587 100644 --- a/vsintegration/src/FSharp.Editor/Classification/ClassificationService.fs +++ b/vsintegration/src/FSharp.Editor/Classification/ClassificationService.fs @@ -8,6 +8,7 @@ open System.Collections.Generic open System.Collections.Immutable open System.Threading open System.Runtime.Caching +open System.Runtime.CompilerServices open Microsoft.CodeAnalysis open Microsoft.CodeAnalysis.Classification @@ -30,6 +31,12 @@ open Microsoft.VisualStudio.FSharp.Editor.Telemetry type SemanticClassificationData = SemanticClassificationView type SemanticClassificationLookup = IReadOnlyDictionary> +type internal LastGoodSemanticClassification = + { + Text: SourceText + Lookup: SemanticClassificationLookup + } + [)>] type internal FSharpClassificationService [] () = @@ -149,6 +156,41 @@ type internal FSharpClassificationService [] () = static let openedDocumentsSemanticClassificationCache = new DocumentCache("fsharp-opened-documents-semantic-classification-cache", 2.) + // Roslyn replaces a span's semantic tags with whatever this service returns, so answering "nothing" + // while the checker is unavailable (project loading or reloading, a superseded check) strips the + // colours the user already sees. Keep the last whole-file lookup per document, outliving the + // version-keyed cache above, and re-emit it for those requests. + static let lastGoodSemanticClassification = + ConditionalWeakTable() + + static let rememberLastGood (documentId: DocumentId) (text: SourceText) (lookup: SemanticClassificationLookup) = + let lastGood = { Text = text; Lookup = lookup } + // net472 has no ConditionalWeakTable.AddOrUpdate. + lock lastGoodSemanticClassification (fun () -> + lastGoodSemanticClassification.Remove documentId |> ignore + lastGoodSemanticClassification.Add(documentId, lastGood)) + + // 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 lastGoodSemanticClassification.TryGetValue documentId with + | true, lastGood when lastGood.Text.ContentEquals sourceText -> + addSemanticClassificationByLookup sourceText targetSpan lastGood.Lookup result + | _ -> () + + static let addLastGoodForCurrentText (document: Document) (targetSpan: TextSpan) (result: List) = + match document.TryGetText() with + | true, sourceText -> addLastGood document.Id sourceText targetSpan result + | _ -> () + + // Which of the two caches a document lands in is not observable from its classifications - a miss + // only costs a recheck - so tests reach the caches directly to tell the branches apart. + static member internal OpenedDocumentsSemanticClassificationCache = + openedDocumentsSemanticClassificationCache + + static member internal UnopenedDocumentsSemanticClassificationCache = + unopenedDocumentsSemanticClassificationCache + interface IFSharpClassificationService with // Do not perform classification if we don't have project options (#defines matter) member _.AddLexicalClassifications(_: SourceText, _: TextSpan, _: List, _: CancellationToken) = () @@ -252,11 +294,12 @@ 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 @@ -288,21 +331,28 @@ 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! document.TryGetFSharpParseAndCheckResultsAsync(nameof (IFSharpClassificationService)) with + | ValueNone -> addLastGood document.Id sourceText textSpan result + | ValueSome(struct (_, checkResults)) -> + // The cache is keyed by text version only, so it has to hold the whole file: + // the next request at this version is usually for a different span. + 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). Caching that would pin the version to no colours. + if classificationData.Length = 0 then + addLastGood document.Id sourceText textSpan result + else + let classificationDataLookup = itemToSemanticClassificationLookup classificationData + do! openedDocumentsSemanticClassificationCache.SetAsync(document, classificationDataLookup) + rememberLastGood document.Id sourceText classificationDataLookup + addSemanticClassificationByLookup sourceText textSpan classificationDataLookup 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/tests/FSharp.Editor.Tests/SemanticClassificationServiceTests.fs b/vsintegration/tests/FSharp.Editor.Tests/SemanticClassificationServiceTests.fs index 6c58f658ee9..cde75f71e4c 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/SemanticClassificationServiceTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/SemanticClassificationServiceTests.fs @@ -2,12 +2,16 @@ namespace FSharp.Editor.Tests +open System +open System.Threading 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 +35,53 @@ 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 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() + + // 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 +467,115 @@ 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( + isCached FSharpClassificationService.OpenedDocumentsSemanticClassificationCache document, + "Classifying an open document must populate the opened-documents cache." + ) + + Assert.False( + isCached FSharpClassificationService.UnopenedDocumentsSemanticClassificationCache document, + "An open document must not be cached as an unopened one." + ) + + // The cache is keyed by text version only, 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) + + // 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( + isCached FSharpClassificationService.OpenedDocumentsSemanticClassificationCache reopened, + "A miss must not be cached 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(isCached FSharpClassificationService.OpenedDocumentsSemanticClassificationCache 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 From 9f0f5f83eb61408c08ed259f23102cd363c8fb65 Mon Sep 17 00:00:00 2001 From: Andrii Chebukin Date: Thu, 10 Sep 2026 02:36:21 +0200 Subject: [PATCH 3/5] Key DocumentCache by a document's semantic version too, not just its text An edit elsewhere in the project can change what a document's names mean while its own text stands still, so a value cached under the text version alone went on answering for semantics that no longer hold. This is the same cache type behind the opened- and unopened-document classification caches this PR adds and the existing inlay-hints cache, so all three now invalidate on either kind of change. Co-Authored-By: Claude Sonnet 5 --- .../src/FSharp.Editor/Common/DocumentCache.fs | 19 +++-- .../FSharp.Editor.Tests/DocumentCacheTests.fs | 78 +++++++++++++++++++ .../FSharp.Editor.Tests.fsproj | 1 + 3 files changed, 93 insertions(+), 5 deletions(-) create mode 100644 vsintegration/tests/FSharp.Editor.Tests/DocumentCacheTests.fs 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/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 @@ + From becb275958642a67ae5ea47657c5a16b7dc38026 Mon Sep 17 00:00:00 2001 From: Andrii Chebukin Date: Fri, 4 Sep 2026 19:47:33 +0200 Subject: [PATCH 4/5] Share one whole-file classification between requests for a version The classification of an open document covers the whole file, so what it costs is proportional to the file, not to the viewport: on an 8000-line file a single pass takes about a second and a half. Two things made that pass run far more often than the text changed. The entry lived in a MemoryCache with a two-second sliding expiration, so the first scroll after any pause missed it and walked the file again for a text version that had already been classified. The entry now lives in the per-document store that the last-known-good fallback already used, keyed by text version and kept for the life of the document, and the separate opened-documents cache goes away. Requests that overlapped - Roslyn's taggers above and below the viewport, a split view - each walked the file on their own. Requests for the same version now join one in-flight computation; it starts with the first of them and is cancelled only when the last one leaves, so a superseded request never cancels the work its neighbours are still waiting for, and no request outlives its own token. Co-Authored-By: Claude Fable 5.1 --- docs/release-notes/.VisualStudio/18.vNext.md | 1 + .../Classification/ClassificationService.fs | 166 +++++++++++++----- .../SemanticClassificationServiceTests.fs | 87 ++++++++- 3 files changed, 203 insertions(+), 51 deletions(-) diff --git a/docs/release-notes/.VisualStudio/18.vNext.md b/docs/release-notes/.VisualStudio/18.vNext.md index 36224213908..03226c61ad3 100644 --- a/docs/release-notes/.VisualStudio/18.vNext.md +++ b/docs/release-notes/.VisualStudio/18.vNext.md @@ -16,6 +16,7 @@ * 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 c7dc7738587..8634507303f 100644 --- a/vsintegration/src/FSharp.Editor/Classification/ClassificationService.fs +++ b/vsintegration/src/FSharp.Editor/Classification/ClassificationService.fs @@ -4,9 +4,11 @@ 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 @@ -31,12 +33,52 @@ open Microsoft.VisualStudio.FSharp.Editor.Telemetry type SemanticClassificationData = SemanticClassificationView type SemanticClassificationLookup = IReadOnlyDictionary> -type internal LastGoodSemanticClassification = +/// The whole-file semantic classification of one version of an open document. +type internal OpenDocumentClassification = { + Version: VersionStamp Text: SourceText Lookup: SemanticClassificationLookup } +/// 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: VersionStamp, compute: CancellationToken -> Task) = + 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 [] () = @@ -153,29 +195,29 @@ 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() - // Roslyn replaces a span's semantic tags with whatever this service returns, so answering "nothing" - // while the checker is unavailable (project loading or reloading, a superseded check) strips the - // colours the user already sees. Keep the last whole-file lookup per document, outliving the - // version-keyed cache above, and re-emit it for those requests. - static let lastGoodSemanticClassification = - ConditionalWeakTable() + static let inFlightClassifications = + ConcurrentDictionary() - static let rememberLastGood (documentId: DocumentId) (text: SourceText) (lookup: SemanticClassificationLookup) = - let lastGood = { Text = text; Lookup = lookup } + static let remember (documentId: DocumentId) (classification: OpenDocumentClassification) = // net472 has no ConditionalWeakTable.AddOrUpdate. - lock lastGoodSemanticClassification (fun () -> - lastGoodSemanticClassification.Remove documentId |> ignore - lastGoodSemanticClassification.Add(documentId, lastGood)) + 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 lastGoodSemanticClassification.TryGetValue documentId with - | true, lastGood when lastGood.Text.ContentEquals sourceText -> - addSemanticClassificationByLookup sourceText targetSpan lastGood.Lookup result + 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) = @@ -183,10 +225,63 @@ type internal FSharpClassificationService [] () = | true, sourceText -> addLastGood document.Id sourceText targetSpan result | _ -> () - // Which of the two caches a document lands in is not observable from its classifications - a miss - // only costs a recheck - so tests reach the caches directly to tell the branches apart. - static member internal OpenedDocumentsSemanticClassificationCache = - openedDocumentsSemanticClassificationCache + static let classifyWholeFile (document: Document) (version: VersionStamp) (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: VersionStamp) (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 @@ -302,8 +397,10 @@ type internal FSharpClassificationService [] () = addSemanticClassificationByLookup sourceText textSpan classificationDataLookup result else - match! openedDocumentsSemanticClassificationCache.TryGetValueAsync document with - | ValueSome classificationDataLookup -> + let! version = document.GetTextVersionAsync(cancellationToken) + + 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() @@ -316,8 +413,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 = [| @@ -331,24 +428,9 @@ type internal FSharpClassificationService [] () = use _eventDuration = TelemetryReporter.ReportSingleEventWithDuration(TelemetryEvents.AddSemanticClassifications, eventProps) - match! document.TryGetFSharpParseAndCheckResultsAsync(nameof (IFSharpClassificationService)) with + match! classifyOpenDocument document version sourceText with + | ValueSome classification -> addSemanticClassificationByLookup sourceText textSpan classification.Lookup result | ValueNone -> addLastGood document.Id sourceText textSpan result - | ValueSome(struct (_, checkResults)) -> - // The cache is keyed by text version only, so it has to hold the whole file: - // the next request at this version is usually for a different span. - 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). Caching that would pin the version to no colours. - if classificationData.Length = 0 then - addLastGood document.Id sourceText textSpan result - else - let classificationDataLookup = itemToSemanticClassificationLookup classificationData - do! openedDocumentsSemanticClassificationCache.SetAsync(document, classificationDataLookup) - rememberLastGood document.Id sourceText classificationDataLookup - addSemanticClassificationByLookup sourceText textSpan classificationDataLookup result } // 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". diff --git a/vsintegration/tests/FSharp.Editor.Tests/SemanticClassificationServiceTests.fs b/vsintegration/tests/FSharp.Editor.Tests/SemanticClassificationServiceTests.fs index cde75f71e4c..183ad76d1be 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/SemanticClassificationServiceTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/SemanticClassificationServiceTests.fs @@ -3,7 +3,9 @@ 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 @@ -63,6 +65,14 @@ type SemanticClassificationServiceTests() = let isCached (cache: DocumentCache) (document: Document) = (cache.TryGetValueAsync document CancellationToken.None).GetAwaiter().GetResult().IsSome + let versionOf (document: Document) = + document.GetTextVersionAsync(CancellationToken.None).GetAwaiter().GetResult() + + let isRemembered (document: Document) = + match FSharpClassificationService.OpenDocumentClassifications.TryGetValue document.Id with + | true, classification -> classification.Version = versionOf document + | _ -> false + let lineSpan (text: SourceText) firstLine lastLine = TextSpan.FromBounds(text.Lines[firstLine].Start, text.Lines[lastLine].End) @@ -479,10 +489,7 @@ let result2 = s.(*2*)IsHyperbolicCaseWithLongName Assert.NotEmpty(classify document (TextSpan(0, text.Length))) - Assert.True( - isCached FSharpClassificationService.OpenedDocumentsSemanticClassificationCache document, - "Classifying an open document must populate the opened-documents cache." - ) + Assert.True(isRemembered document, "Classifying an open document must remember its classification.") Assert.False( isCached FSharpClassificationService.UnopenedDocumentsSemanticClassificationCache document, @@ -537,10 +544,7 @@ let result2 = s.(*2*)IsHyperbolicCaseWithLongName Assert.NotEmpty(classify reopened (TextSpan(0, reopenedText.Length))) - Assert.False( - isCached FSharpClassificationService.OpenedDocumentsSemanticClassificationCache reopened, - "A miss must not be cached as a result." - ) + 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. @@ -562,7 +566,7 @@ let result2 = s.(*2*)IsHyperbolicCaseWithLongName let text = sourceTextOf document Assert.Empty(classify document (TextSpan(0, text.Length))) - Assert.False(isCached FSharpClassificationService.OpenedDocumentsSemanticClassificationCache document) + 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. @@ -579,3 +583,68 @@ let result2 = s.(*2*)IsHyperbolicCaseWithLongName |> 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( + VersionStamp.Create(), + 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 = VersionStamp.Create() + 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( + VersionStamp.Create(), + 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 From 25a249c6a095b1aa07660e0adf6a63e581c9f3a3 Mon Sep 17 00:00:00 2001 From: Andrii Chebukin Date: Wed, 9 Sep 2026 23:28:32 +0200 Subject: [PATCH 5/5] Key a document's classification by its project's semantic version too An edit in another file of the project changes what this document's symbols mean while its own text version stays put, so a classification kept for the text version alone went on colouring the old semantics. Co-Authored-By: Claude Opus 5 --- .../Classification/ClassificationService.fs | 28 ++++++-- .../SemanticClassificationServiceTests.fs | 69 +++++++++++++++++-- 2 files changed, 87 insertions(+), 10 deletions(-) diff --git a/vsintegration/src/FSharp.Editor/Classification/ClassificationService.fs b/vsintegration/src/FSharp.Editor/Classification/ClassificationService.fs index 8634507303f..3eed5e4109f 100644 --- a/vsintegration/src/FSharp.Editor/Classification/ClassificationService.fs +++ b/vsintegration/src/FSharp.Editor/Classification/ClassificationService.fs @@ -33,18 +33,29 @@ 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: VersionStamp + 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: VersionStamp, compute: CancellationToken -> Task) = +type internal InFlightClassification(version: ClassificationVersion, compute: ClassifyWholeFile) = let cts = new CancellationTokenSource() let job = lazy (compute cts.Token) let mutable waiters = 0 @@ -225,7 +236,7 @@ type internal FSharpClassificationService [] () = | true, sourceText -> addLastGood document.Id sourceText targetSpan result | _ -> () - static let classifyWholeFile (document: Document) (version: VersionStamp) (sourceText: SourceText) = + static let classifyWholeFile (document: Document) (version: ClassificationVersion) (sourceText: SourceText) = cancellableTask { match! document.TryGetFSharpParseAndCheckResultsAsync(nameof (IFSharpClassificationService)) with | ValueNone -> return ValueNone @@ -252,7 +263,7 @@ type internal FSharpClassificationService [] () = // 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: VersionStamp) (sourceText: SourceText) = + static let classifyOpenDocument (document: Document) (version: ClassificationVersion) (sourceText: SourceText) = cancellableTask { let! cancellationToken = CancellableTask.getCancellationToken () @@ -397,7 +408,14 @@ type internal FSharpClassificationService [] () = addSemanticClassificationByLookup sourceText textSpan classificationDataLookup result else - let! version = document.GetTextVersionAsync(cancellationToken) + 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 -> diff --git a/vsintegration/tests/FSharp.Editor.Tests/SemanticClassificationServiceTests.fs b/vsintegration/tests/FSharp.Editor.Tests/SemanticClassificationServiceTests.fs index 183ad76d1be..a5a28ee06a5 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/SemanticClassificationServiceTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/SemanticClassificationServiceTests.fs @@ -66,19 +66,52 @@ type SemanticClassificationServiceTests() = (cache.TryGetValueAsync document CancellationToken.None).GetAwaiter().GetResult().IsSome let versionOf (document: Document) = - document.GetTextVersionAsync(CancellationToken.None).GetAwaiter().GetResult() + { + 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() @@ -496,7 +529,7 @@ let result2 = s.(*2*)IsHyperbolicCaseWithLongName "An open document must not be cached as an unopened one." ) - // The cache is keyed by text version only, so what it holds has to cover the whole file: + // 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``() = @@ -527,6 +560,32 @@ let result2 = s.(*2*)IsHyperbolicCaseWithLongName 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. [] @@ -592,7 +651,7 @@ let result2 = s.(*2*)IsHyperbolicCaseWithLongName let inFlight = InFlightClassification( - VersionStamp.Create(), + someVersion (), fun _ -> Interlocked.Increment computed |> ignore gate.Task @@ -604,7 +663,7 @@ let result2 = s.(*2*)IsHyperbolicCaseWithLongName let classification = { - Version = VersionStamp.Create() + Version = someVersion () Text = SourceText.From "" Lookup = Dictionary() } @@ -622,7 +681,7 @@ let result2 = s.(*2*)IsHyperbolicCaseWithLongName let inFlight = InFlightClassification( - VersionStamp.Create(), + someVersion (), fun ct -> sharedToken.Value <- ct gate.Task