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

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))
+* 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/Navigation/NavigateToSearchService.fs

@@ -17,14 +17,41 @@ 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 the defines it was parsed with, or under `AnyDefines` when its tree
+/// holds no conditional directives and so reads the same under any of them.
+[<Struct>]
+type private NavigableItemsKey = { Defines: 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 +60,48 @@ 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 with
+            | null ->
                 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 ->
+                let defines = document.GetFSharpQuickDefines() |> String.concat ";"
+
+                let cached key =
+                    match cache.TryGetValue({ Defines = key; FilePath = path }) 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
+                            defines
+                        else
+                            AnyDefines
+
+                    cache[{ Defines = key; FilePath = path }] <-
+                        {
+                            Version = currentVersion
+                            Items = items
+                        }
+
+                    return items
         }
 
     let kindsProvided =
@@ -145,46 +202,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 +249,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" />

vsintegration/tests/FSharp.Editor.Tests/Helpers/RoslynHelpers.fs

@@ -201,6 +201,14 @@ type TestHostServices() =
     override this.CreateWorkspaceServices(workspace) =
         new TestHostWorkspaceServices(this, workspace)
 
+/// One Roslyn project instance of a multi-targeted F# project: its extra defines and the
+/// synthetic files left out of it, as VS does per target framework.
+type TargetInstance =
+    {
+        Defines: string list
+        ExcludedFileIds: string list
+    }
+
 [<AbstractClass; Sealed>]
 type RoslynTestHelpers private () =
 
@@ -258,6 +266,33 @@ type RoslynTestHelpers private () =
             filePath = filePath
         )
 
+    static member private ProjectInfoFor
+        (id, name, filePath, outputFilePath, documents, projectReferences: ProjectReference list, metadataReferences: MetadataReference seq)
+        =
+        ProjectInfo.Create(
+            id,
+            VersionStamp.Create(DateTime.UtcNow),
+            name,
+            name,
+            LanguageNames.FSharp,
+            filePath = filePath,
+            outputFilePath = outputFilePath,
+            documents = documents,
+            projectReferences = projectReferences,
+            metadataReferences = metadataReferences
+        )
+
+    static member private MetadataReferencesOf(options: FSharpProjectOptions, excludedPaths: string seq) =
+        let excluded = HashSet(excludedPaths, StringComparer.OrdinalIgnoreCase)
+
+        options.OtherOptions
+        |> Seq.filter (fun x -> x.StartsWith("-r:", StringComparison.Ordinal))
+        |> Seq.map _.Substring(3)
+        |> Seq.filter (excluded.Contains >> not)
+        |> Seq.map MetadataReference.CreateFromFile
+        |> Seq.cast<MetadataReference>
+        |> Seq.toList
+
     static member SetProjectOptions projId (solution: Solution) (options: FSharpProjectOptions) =
         solution.Workspace.Services
             .GetService<IFSharpWorkspaceService>()
@@ -331,19 +366,119 @@ type RoslynTestHelpers private () =
 
         let options = syntheticProject.GetProjectOptions checker
 
-        let metadataReferences =
-            options.OtherOptions
-            |> Seq.filter (fun x -> x.StartsWith("-r:"))
-            |> Seq.map (fun x -> x.Substring(3) |> MetadataReference.CreateFromFile :> MetadataReference)
-
-        let projInfo = projInfo.WithMetadataReferences metadataReferences
+        let projInfo =
+            projInfo.WithMetadataReferences(RoslynTestHelpers.MetadataReferencesOf(options, []))
 
         let solution = RoslynTestHelpers.CreateSolution [ projInfo ]
 
         options |> RoslynTestHelpers.SetProjectOptions projId solution
 
         solution, checker
 
+    /// One Roslyn project per synthetic project, wired with project references the way VS wires
+    /// project-to-project references, so the options manager builds in-memory F# references.
+    static member CreateMultiProjectSolution(syntheticProject: SyntheticProject) =
+        let checker = syntheticProject.SaveAndCheck()
+
+        let projects =
+            syntheticProject.GetAllProjects()
+            |> Seq.distinctBy _.Name
+            |> Seq.map (fun project -> project, ProjectId.CreateNewId())
+            |> Seq.toList
+
+        let projectIds = dict [ for project, id in projects -> project.Name, id ]
+
+        let projectInfos =
+            [
+                for project, id in projects do
+                    let options = project.GetProjectOptions checker
+
+                    RoslynTestHelpers.ProjectInfoFor(
+                        id,
+                        project.Name,
+                        project.ProjectFileName,
+                        project.OutputFilename,
+                        [
+                            for path in project.SourceFilePaths -> RoslynTestHelpers.CreateDocumentInfo id path (File.ReadAllText path)
+                        ],
+                        [
+                            for dependency in project.DependsOn -> ProjectReference projectIds[dependency.Name]
+                        ],
+                        RoslynTestHelpers.MetadataReferencesOf(options, project.DependsOn |> List.map _.OutputFilename)
+                    )
+            ]
+
+        let solution = RoslynTestHelpers.CreateSolution projectInfos
+
+        for project, id in projects do
+            project.GetProjectOptions checker
+            |> RoslynTestHelpers.SetProjectOptions id solution
+
+        solution, checker
+
+    /// One Roslyn project per target instance, all sharing the .fsproj path and the document file
+    /// paths, like the per-target-framework projects VS creates for a multi-targeted project.
+    static member CreateMultiTargetSolution(syntheticProject: SyntheticProject, instances: TargetInstance list) =
+        assert (syntheticProject.DependsOn = [])
+
+        let checker = syntheticProject.SaveAndCheck()
+        let options = syntheticProject.GetProjectOptions checker
+        let metadataReferences = RoslynTestHelpers.MetadataReferencesOf(options, [])
+
+        let instances =
+            [
+                for instance in instances ->
+                    let excludedPaths =
+                        HashSet(
+                            [
+                                for fileId in instance.ExcludedFileIds do
+                                    syntheticProject.GetFilePath fileId
+
+                                    if (syntheticProject.Find fileId).HasSignatureFile then
+                                        syntheticProject.GetSignatureFilePath fileId
+                            ],
+                            StringComparer.OrdinalIgnoreCase
+                        )
+
+                    let sourceFiles =
+                        syntheticProject.SourceFilePaths |> List.filter (excludedPaths.Contains >> not)
+
+                    let id = ProjectId.CreateNewId()
+
+                    let projectInfo =
+                        RoslynTestHelpers.ProjectInfoFor(
+                            id,
+                            syntheticProject.Name,
+                            syntheticProject.ProjectFileName,
+                            syntheticProject.OutputFilename,
+                            [
+                                for path in sourceFiles -> RoslynTestHelpers.CreateDocumentInfo id path (File.ReadAllText path)
+                            ],
+                            [],
+                            metadataReferences
+                        )
+
+                    let instanceOptions =
+                        { options with
+                            SourceFiles = List.toArray sourceFiles
+                            OtherOptions =
+                                [|
+                                    yield! options.OtherOptions
+                                    for define in instance.Defines -> $"--define:{define}"
+                                |]
+                        }
+
+                    id, projectInfo, instanceOptions
+            ]
+
+        let solution =
+            RoslynTestHelpers.CreateSolution [ for _, projectInfo, _ in instances -> projectInfo ]
+
+        for id, _, instanceOptions in instances do
+            RoslynTestHelpers.SetProjectOptions id solution instanceOptions
+
+        solution, [ for id, _, _ in instances -> id ]
+
     static member GetFsDocument(code, ?customProjectOption: string, ?customEditorOptions) =
         let customProjectOptions =
             customProjectOption