diff --git a/docs/release-notes/.VisualStudio/18.vNext.md b/docs/release-notes/.VisualStudio/18.vNext.md index ba03f663967..66cfd8a81a7 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)) +* 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 diff --git a/vsintegration/src/FSharp.Editor/Navigation/NavigateToSearchService.fs b/vsintegration/src/FSharp.Editor/Navigation/NavigateToSearchService.fs index 546b00e1b16..bc3ef82da74 100644 --- a/vsintegration/src/FSharp.Editor/Navigation/NavigateToSearchService.fs +++ b/vsintegration/src/FSharp.Editor/Navigation/NavigateToSearchService.fs @@ -17,6 +17,7 @@ open Microsoft.VisualStudio.LanguageServices open Microsoft.VisualStudio.Text.PatternMatching open FSharp.Compiler.EditorServices +open FSharp.Compiler.Syntax open CancellableTasks [); Shared>] @@ -24,7 +25,27 @@ type internal FSharpNavigateToSearchService [] (patternMatcherFactory: IPatternMatcherFactory, [] workspace: VisualStudioWorkspace) = - let cache = ConcurrentDictionary() + /// 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< + struct (string * string), + struct {| + Version: VersionStamp + Items: NavigableItem array + |} + >() + + /// 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. + [] + let AnyDefines = "?" do if workspace <> null then @@ -33,18 +54,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(struct (key, 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[struct (key, path)] <- + {| + Version = currentVersion + Items = items + |} + + return items } let kindsProvided = @@ -145,46 +196,44 @@ type internal FSharpNavigateToSearchService let processDocument (tryMatch: NavigableItem -> PatternMatch voption) (kinds: IImmutableSet) (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 +243,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 diff --git a/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj b/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj index ecce1205b8c..a71cae6e92b 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj +++ b/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj @@ -32,6 +32,7 @@ + diff --git a/vsintegration/tests/FSharp.Editor.Tests/Helpers/RoslynHelpers.fs b/vsintegration/tests/FSharp.Editor.Tests/Helpers/RoslynHelpers.fs index 25509f14ace..89a449eceb7 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/Helpers/RoslynHelpers.fs +++ b/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 + } + [] 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 + |> Seq.toList + static member SetProjectOptions projId (solution: Solution) (options: FSharpProjectOptions) = solution.Workspace.Services .GetService() @@ -331,12 +366,8 @@ 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 ] @@ -344,6 +375,109 @@ type RoslynTestHelpers private () = 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() + |> List.distinctBy _.Name + |> List.map (fun project -> project, ProjectId.CreateNewId()) + + 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 diff --git a/vsintegration/tests/FSharp.Editor.Tests/MultiTargetNavigateToSearchTests.fs b/vsintegration/tests/FSharp.Editor.Tests/MultiTargetNavigateToSearchTests.fs new file mode 100644 index 00000000000..65c1dc1c3eb --- /dev/null +++ b/vsintegration/tests/FSharp.Editor.Tests/MultiTargetNavigateToSearchTests.fs @@ -0,0 +1,102 @@ +// Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information. + +/// One project loaded as two target-framework instances: `plain` compiles without the fourth file +/// and without FOO, `foo` compiles everything with FOO defined. +module FSharp.Editor.Tests.MultiTargetNavigateToSearchTests + +open System.Collections.Immutable +open System.Threading +open Xunit +open Microsoft.CodeAnalysis +open Microsoft.CodeAnalysis.Text +open Microsoft.CodeAnalysis.ExternalAccess.FSharp.NavigateTo +open FSharp.Editor.Tests.Helpers +open FSharp.Test.ProjectGeneration + +let private project = + SyntheticProject.Create( + { sourceFile "First" [] with + ExtraSource = "let sharedFunc funcParam = funcParam * 2\n" + }, + { sourceFile "Second" [ "First" ] with + ExtraSource = "let plainUse x = ModuleFirst.sharedFunc x" + }, + { sourceFile "Third" [ "First" ] with + ExtraSource = "#if FOO\nlet fooUse x = ModuleFirst.sharedFunc x\n#endif" + }, + { sourceFile "Fourth" [ "First" ] with + ExtraSource = "let fooOnlyFileUse x = ModuleFirst.sharedFunc x" + } + ) + +let private solution, instances = + RoslynTestHelpers.CreateMultiTargetSolution( + project, + [ + { + Defines = [] + ExcludedFileIds = [ "Fourth" ] + } + { + Defines = [ "FOO" ] + ExcludedFileIds = [] + } + ] + ) + +let private service: IFSharpNavigateToSearchService = + MefHelpers.createExportProvider().GetExportedValue() + +let private searchIn (project: Project) pattern = + service.SearchProjectAsync(project, ImmutableArray.Empty, pattern, service.KindsProvided, CancellationToken.None).Result + |> Seq.map _.Name + |> Seq.filter ((=) pattern) + |> Seq.toList + +let private documentNamed (name: string) (project: Project) = + project.Documents |> Seq.find (fun document -> document.Name.Contains name) + +/// Roslyn submits only the active project when the search is scoped to the current one, so an instance has +/// to report what it compiles even when a sibling compiles the same file. The second search is the one that +/// used to come back empty: by then the file had been parsed, and the parse said it did not depend on the +/// defines. +[] +[] +[] +let ``an instance reports a shared declaration on its own, and again once the parse is cached`` (instance: int) = + let project = solution.GetProject instances[instance] + + Assert.Equal([ "plainUse" ], searchIn project "plainUse") + Assert.Equal([ "plainUse" ], searchIn project "plainUse") + +/// The parse of a file that does hold directives is kept per define set, so reusing it across the instances +/// must not leak the declaration into the instance that does not define FOO. +[] +let ``a declaration behind a directive is reported only by the instance that defines it`` () = + let plain = solution.GetProject instances[0] + let foo = solution.GetProject instances[1] + + Assert.Equal([ "fooUse" ], searchIn foo "fooUse") + Assert.Equal([], searchIn plain "fooUse") + +/// A file with no directives has its parse shared by every instance. When an edit gives it one, that shared +/// parse is stale: the entry is keyed by the text version, so the edit is a different entry rather than a +/// flag left over from before. +[] +let ``a declaration an edit puts behind a directive is reported by the instance that defines it`` () = + let edited = + (solution, instances) + ||> Seq.fold (fun (solution: Solution) instance -> + let document = solution.GetProject instance |> documentNamed "Second" + + solution.WithDocumentText( + document.Id, + SourceText.From "module ModuleSecond\n#if FOO\nlet addedFoo = 1\n#endif\n" + )) + + let foo = edited.GetProject instances[1] + let plain = edited.GetProject instances[0] + + // The instance without FOO goes first: that order is what left the stale answer behind. + Assert.Equal([], searchIn plain "addedFoo") + Assert.Equal([ "addedFoo" ], searchIn foo "addedFoo")