Search each file of a multi-targeted F# project once in Go To All

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

@@ -20,6 +20,7 @@
 * 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))
 * Build a single-file project's `OtherOptions` reference flags with one array comprehension instead of two `Array.ofSeq` calls and an `Array.append`. ([PR #20499](https://github.com/dotnet/fsharp/pull/20499))
 * Fix syntax coloring being lost for a whole file when one symbol resolves into metadata that could not be read. ([Issue #20269](https://github.com/dotnet/fsharp/issues/20269), [PR #20274](https://github.com/dotnet/fsharp/pull/20274))
+* Go To All (Ctrl+T) on a multi-targeted F# project no longer parses every file once per target framework: a file whose parse holds no conditional directives does not depend on the defines, so every instance reuses the one parse, and a file's text is read only for a matched declaration. ([PR #20483](https://github.com/dotnet/fsharp/pull/20483))
 
 ### Changed
 

vsintegration/src/FSharp.Editor/LanguageService/FSharpProjectOptionsManager.fs

@@ -612,6 +612,15 @@ type internal FSharpProjectOptionsManager(checker: FSharpChecker, workspace: Wor
 
         CompilerEnvironment.GetConditionalDefinesForEditing parsingOptions, parsingOptions.LangVersionText
 
+    /// The defines and language version the document's project has produced, and nothing where it has not
+    /// produced its options yet: what the editing defaults answer there is a set no project asked for, so
+    /// anything kept under it belongs to no project either.
+    member _.TryGetCompilationDefinesAndLangVersionForEditingDocument(document: Document) =
+        match reactor.TryGetCachedOptionsByProjectId(document.Project.Id) with
+        | Some(_, parsingOptions, _) ->
+            ValueSome(struct (CompilerEnvironment.GetConditionalDefinesForEditing parsingOptions, parsingOptions.LangVersionText))
+        | _ -> ValueNone
+
     member _.TryGetOptionsByProject(project) =
         reactor.TryGetOptionsByProjectAsync(project)
 

vsintegration/src/FSharp.Editor/LanguageService/WorkspaceExtensions.fs

@@ -564,6 +564,13 @@ type Document with
         let workspaceService = this.Project.Solution.GetFSharpWorkspaceService()
         workspaceService.FSharpProjectOptionsManager.GetCompilationDefinesAndLangVersionForEditingDocument(this)
 
+    /// A non-async call that gets the defines and F# language version of the given F# document, and nothing
+    /// where its project has not produced its options yet: a guess names what the project never asked for,
+    /// which a parse must not be kept under.
+    member this.TryGetFSharpParsingOptionsData() =
+        let workspaceService = this.Project.Solution.GetFSharpWorkspaceService()
+        workspaceService.FSharpProjectOptionsManager.TryGetCompilationDefinesAndLangVersionForEditingDocument(this)
+
     /// A non-async call that quickly gets the defines of the given F# document.
     /// This tries to get the defines by looking at an internal cache; if it doesn't exist in the cache it will create an inaccurate but usable form of the defines.
     member this.GetFSharpQuickDefines() =

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

@@ -17,14 +17,49 @@ open Microsoft.VisualStudio.LanguageServices
 open Microsoft.VisualStudio.Text.PatternMatching
 
 open FSharp.Compiler.EditorServices
+open FSharp.Compiler.Syntax
 open CancellableTasks
 
+/// Where a parse of a file is kept: under what the parse depends on besides the text. The defines are those it
+/// was parsed with, or `AnyDefines` when its tree holds no conditional directives and so reads the same under
+/// any of them; the language version decides what the parser accepts and is never shared across versions. The
+/// remaining parsing options cannot differ for one path: the editor never applies line directives, and whether
+/// a file is interactive follows from its own extension.
+[<Struct>]
+type private NavigableItemsKey =
+    {
+        Defines: string
+        LangVersion: string
+        FilePath: string
+    }
+
+/// The navigable items of one parse of a file, and the text version it was taken from.
+[<Struct>]
+type private NavigableItemsEntry =
+    {
+        Version: VersionStamp
+        Items: NavigableItem array
+    }
+
 [<Export(typeof<IFSharpNavigateToSearchService>); Shared>]
 type internal FSharpNavigateToSearchService
     [<ImportingConstructor>]
     (patternMatcherFactory: IPatternMatcherFactory, [<Import(AllowDefault = true)>] workspace: VisualStudioWorkspace) =
 
-    let cache = ConcurrentDictionary<DocumentId, VersionStamp * NavigableItem array>()
+    /// A multi-targeted project is one Roslyn project per target framework over the same files, so the same
+    /// file is searched once per instance. What that costs is the parse, and a parse whose tree holds no
+    /// conditional directives does not depend on the defines: it is stored under `AnyDefines` and every
+    /// instance reuses it. One that does hold them is stored per define set, because those instances
+    /// genuinely parse the file differently.
+    ///
+    /// The duplicate results this produces are not for this service to remove. `NavigateToSearcher` pools its
+    /// seen set with `NavigateToSearchResultComparer`, which already collapses results by file path and span.
+    let cache = ConcurrentDictionary<NavigableItemsKey, NavigableItemsEntry>()
+
+    /// The key for a parse that does not depend on the defines. Not a define set any instance can have,
+    /// since defines are identifiers — an instance with none of its own must not read this entry as its own.
+    [<Literal>]
+    let AnyDefines = "?"
 
     do
         if workspace <> null then
@@ -33,18 +68,59 @@ type internal FSharpNavigateToSearchService
                 if e.NewSolution.Id <> e.OldSolution.Id then
                     cache.Clear()
 
+    let dependsOnDefines (parseTree: ParsedInput) =
+        match parseTree with
+        | ParsedInput.ImplFile file -> not file.Trivia.ConditionalDirectives.IsEmpty
+        | ParsedInput.SigFile file -> not file.Trivia.ConditionalDirectives.IsEmpty
+
     let getNavigableItems (document: Document) =
         cancellableTask {
             let! ct = CancellableTask.getCancellationToken ()
             let! currentVersion = document.GetTextVersionAsync(ct)
 
-            match cache.TryGetValue document.Id with
-            | true, (version, items) when version = currentVersion -> return items
-            | _ ->
+            match document.FilePath, document.TryGetFSharpParsingOptionsData() with
+            // A document with no path is keyed by nothing, and one whose project has not produced its options
+            // yet knows neither the defines nor the language version its own parse depends on - an entry
+            // another instance wrote under what it really parsed with must not answer for it.
+            | null, _
+            | _, ValueNone ->
                 let! parseResults = document.GetFSharpParseResultsAsync(nameof (FSharpNavigateToSearchService))
-                let items = NavigateTo.GetNavigableItems parseResults.ParseTree
-                cache[document.Id] <- currentVersion, items
-                return items
+                return NavigateTo.GetNavigableItems parseResults.ParseTree
+            | path, ValueSome(struct (documentDefines, langVersion)) ->
+                let defines = documentDefines |> String.concat ";"
+
+                let keyOf defines =
+                    {
+                        Defines = defines
+                        LangVersion = langVersion
+                        FilePath = path
+                    }
+
+                let cached defines =
+                    match cache.TryGetValue(keyOf defines) with
+                    | true, entry when entry.Version = currentVersion -> ValueSome entry.Items
+                    | _ -> ValueNone
+
+                match cached AnyDefines, cached defines with
+                | ValueSome items, _
+                | _, ValueSome items -> return items
+                | ValueNone, ValueNone ->
+                    let! parseResults = document.GetFSharpParseResultsAsync(nameof (FSharpNavigateToSearchService))
+                    let items = NavigateTo.GetNavigableItems parseResults.ParseTree
+
+                    let key =
+                        if dependsOnDefines parseResults.ParseTree then
+                            keyOf defines
+                        else
+                            keyOf AnyDefines
+
+                    cache[key] <-
+                        {
+                            Version = currentVersion
+                            Items = items
+                        }
+
+                    return items
         }
 
     let kindsProvided =
@@ -145,46 +221,44 @@ type internal FSharpNavigateToSearchService
 
     let processDocument (tryMatch: NavigableItem -> PatternMatch voption) (kinds: IImmutableSet<string>) (document: Document) =
         cancellableTask {
-            let! ct = CancellableTask.getCancellationToken ()
-
-            let! sourceText = document.GetTextAsync ct
-
             let! items = getNavigableItems document
 
-            let processed =
+            let matches =
                 [|
                     for item in items do
-                        let contains = kinds.Contains(navigateToItemKindToRoslynKind item.Kind)
-                        let patternMatch = tryMatch item
+                        if kinds.Contains(navigateToItemKindToRoslynKind item.Kind) then
+                            match tryMatch item with
+                            | ValueSome m -> yield struct (item, m)
+                            | ValueNone -> ()
+                |]
 
-                        match contains, patternMatch with
-                        | true, ValueSome m ->
-                            let sourceSpan = RoslynHelpers.TryFSharpRangeToTextSpan(sourceText, item.Range)
+            // The text, read from disk for a closed document, is only needed to place the matches.
+            if matches.Length = 0 then
+                return [||]
+            else
+                let! ct = CancellableTask.getCancellationToken ()
+                let! sourceText = document.GetTextAsync ct
 
-                            match sourceSpan with
+                return
+                    [|
+                        for struct (item, m) in matches do
+                            match RoslynHelpers.TryFSharpRangeToTextSpan(sourceText, item.Range) with
                             | ValueNone -> ()
                             | ValueSome sourceSpan ->
-                                let glyph = navigateToItemKindToGlyph item.Kind
-                                let kind = navigateToItemKindToRoslynKind item.Kind
-                                let additionalInfo = formatInfo item.Container document
-
                                 yield
                                     FSharpNavigateToSearchResult(
-                                        additionalInfo,
-                                        kind,
+                                        formatInfo item.Container document,
+                                        navigateToItemKindToRoslynKind item.Kind,
                                         patternMatchKindToNavigateToMatchKind m.Kind,
                                         item.Name,
                                         FSharpNavigableItem(
-                                            glyph,
+                                            navigateToItemKindToGlyph item.Kind,
                                             ImmutableArray.Create(TaggedText(TextTags.Text, item.Name)),
                                             document,
                                             sourceSpan
                                         )
                                     )
-                        | _ -> ()
-                |]
-
-            return processed
+                    |]
         }
 
     interface IFSharpNavigateToSearchService with
@@ -194,22 +268,13 @@ type internal FSharpNavigateToSearchService
             cancellableTask {
                 let tryMatch = createMatcherFor searchPattern
 
-                let tasks =
-                    [|
-                        for doc in project.Documents do
-                            yield processDocument tryMatch kinds doc
-                    |]
-
-                let! results = CancellableTask.whenAll tasks
-
-                let results' = ImmutableArray.CreateBuilder()
-
-                for navResults in results do
-                    for navResult in navResults do
-                        results'.Add navResult
-
-                return results'.ToImmutable()
+                let! results =
+                    project.Documents
+                    |> Seq.map (processDocument tryMatch kinds)
+                    // Throttle to avoid launching a parse per document in the project all at once.
+                    |> CancellableTask.whenAllThrottled (max 1 Environment.ProcessorCount)
 
+                return results |> Array.concat |> Array.toImmutableArray
             }
             |> CancellableTask.start cancellationToken
 

vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj

@@ -32,6 +32,7 @@
     <Compile Include="QuickInfoTests.fs" />
     <Compile Include="TaskListServiceTests.fs" />
     <Compile Include="NavigateToSearchServiceTests.fs" />
+    <Compile Include="MultiTargetNavigateToSearchTests.fs" />
     <Compile Include="CodeFixes\CodeFixTestFramework.fs" />
     <Compile Include="CodeFixes\AddInstanceMemberParameterTests.fs" />
     <Compile Include="CodeFixes\ConvertToAnonymousRecordTests.fs" />