diff --git a/docs/release-notes/.VisualStudio/18.vNext.md b/docs/release-notes/.VisualStudio/18.vNext.md index ba03f663967..195eacb11e0 100644 --- a/docs/release-notes/.VisualStudio/18.vNext.md +++ b/docs/release-notes/.VisualStudio/18.vNext.md @@ -15,6 +15,9 @@ * 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)) +* Find All References and Rename search each file of a multi-targeted F# project once instead of once per target framework, revisiting a file in another instance only when it is compiled there alone or under conditional compilation; searches start as each project's snapshot is ready and type checks are bounded across the solution. ([PR #20464](https://github.com/dotnet/fsharp/pull/20464)) +* Find Implementations no longer searches the whole solution for uses it then discards; it reports the declarations alone. +* Find All References reports the uses of a document in one go instead of starting a task per reference, stops when the request is cancelled, and searches the documents of a project through a fixed set of workers under one solution-wide budget that leaves a core to the editor. ### Changed diff --git a/vsintegration/src/FSharp.Editor/Common/CancellableTasks.fs b/vsintegration/src/FSharp.Editor/Common/CancellableTasks.fs index 7520395a084..d699bdba0bf 100644 --- a/vsintegration/src/FSharp.Editor/Common/CancellableTasks.fs +++ b/vsintegration/src/FSharp.Editor/Common/CancellableTasks.fs @@ -1130,6 +1130,31 @@ module CancellableTasks = return! allTask } + /// Runs the work over the items with at most maxDegreeOfParallelism of them in flight. A worker + /// takes the next item when it frees up, so cancellation cancels the workers rather than a + /// pending task per item. + let forEachThrottled maxDegreeOfParallelism (work: 'T -> CancellableTask) (items: 'T seq) = + cancellableTask { + let! ct = getCancellationToken () + let items = Seq.toArray items + let mutable next = -1 + + let worker () = + backgroundTask { + let mutable index = Interlocked.Increment &next + + while index < items.Length do + ct.ThrowIfCancellationRequested() + do! work items[index] ct + index <- Interlocked.Increment &next + } + + let workers = + Array.init (min (max 1 maxDegreeOfParallelism) items.Length) (fun _ -> worker ()) + + do! (Task.WhenAll workers :> Task) + } + let inline whenAllTasks (tasks: CancellableTask seq) = cancellableTask { let! ct = getCancellationToken () diff --git a/vsintegration/src/FSharp.Editor/LanguageService/SymbolHelpers.fs b/vsintegration/src/FSharp.Editor/LanguageService/SymbolHelpers.fs index 36319820f80..b5810298bb2 100644 --- a/vsintegration/src/FSharp.Editor/LanguageService/SymbolHelpers.fs +++ b/vsintegration/src/FSharp.Editor/LanguageService/SymbolHelpers.fs @@ -7,6 +7,7 @@ open System.Collections.Concurrent open System.Collections.Generic open System.Collections.Immutable open System.IO +open System.Threading open System.Threading.Tasks open Microsoft.CodeAnalysis @@ -70,34 +71,107 @@ module internal SymbolHelpers = return symbolUses } - let getSymbolUsesInProjects (symbol: FSharpSymbol, projects: Project list, onFound: Document -> range -> CancellableTask) = - match projects with + /// Ranks the target-framework instances of a project file: the one the current document lives in, + /// then those in its dependency closure, whose references resolve the symbol the same way. + let private rankInstances (currentProject: Project) = + let graph = currentProject.Solution.GetProjectDependencyGraph() + + let related = + HashSet + [ + yield! graph.GetProjectsThatThisProjectTransitivelyDependsOn currentProject.Id + yield! graph.GetProjectsThatTransitivelyDependOnThisProject currentProject.Id + ] + + fun (project: Project) -> + if project.Id = currentProject.Id then 0 + elif related.Contains project.Id then 1 + else 2 + + /// One search per project file: the best-ranked target-framework instance is searched in full, + /// the others only where their sources can differ from it. + let private groupInstances (currentProject: Project) (projects: Project seq) = + let rank = rankInstances currentProject + + seq { + for _, instances in projects |> Seq.groupBy _.FilePath do + let instances = Seq.toArray instances + // Ordering the rest buys nothing, and ranking every instance of the solution at once + // measures slower than one pass per group. + let primary = Array.minBy rank instances + struct (primary, instances |> Seq.filter (fun instance -> instance.Id <> primary.Id)) + } + + /// One core is left to the thread that has to stay responsive, and every search in the editor + /// shares what remains. + let searchThrottle = new SemaphoreSlim(max 1 (Environment.ProcessorCount - 1)) + + let getSymbolUsesInProjects + (symbol: FSharpSymbol, currentProject: Project, projects: Project list, onFound: Document -> range seq -> CancellableTask) + = + match projects |> List.filter _.IsFSharp with | [] -> CancellableTask.singleton () - | firstProject :: _ -> + | firstProject :: _ as projects -> let isFastFindReferencesEnabled = firstProject.IsFastFindReferencesEnabled // TODO: this needs to use already boxed boolean instead of boxing it every time. let props = [| nameof isFastFindReferencesEnabled, isFastFindReferencesEnabled :> obj |] + let groups = + if isFastFindReferencesEnabled then + groupInstances currentProject projects + else + seq { for project in projects -> struct (project, Seq.empty) } + cancellableTask { // TODO: this needs to be a single event with a duration TelemetryReporter.ReportSingleEvent(TelemetryEvents.GetSymbolUsesInProjectsStarted, props) + let! ct = CancellableTask.getCancellationToken () + + // A file that several projects compile - the target-framework instances of one project + // file, or two project files sharing a source file - is searched in each of them and + // reports the same range every time. The range carries its file, so the first project + // to report a use keeps it and the rest are dropped. + let reported = ConcurrentDictionary() + + let onFound document ranges = + let fresh = + ranges |> Seq.filter (fun range -> reported.TryAdd(range, ())) |> Seq.toArray + + if fresh.Length = 0 then + CancellableTask.singleton () + else + onFound document fresh + // Mutated by the checker while a snapshot is built, so snapshots are built one at a time. let snapshotAccumulator = Dictionary() + let searches = ResizeArray() - let! projects = - projects - |> Seq.map (fun project -> - project.GetFSharpProjectSnapshot(snapshotAccumulator) - |> CancellableTask.map (fun s -> project, s)) - |> CancellableTask.sequential + let snapshotFor (project: Project) = + if project.UseTransparentCompiler then + project.GetFSharpProjectSnapshot snapshotAccumulator + |> CancellableTask.map ValueSome + else + CancellableTask.singleton ValueNone - do! - projects - |> Seq.map (fun (project, snapshot) -> - project.FindFSharpReferencesAsync(symbol, snapshot, onFound, "getSymbolUsesInProjects")) - |> CancellableTask.whenAll + // Started, not awaited: the next project's snapshot is built while this one searches. + let startSearching (project: Project) snapshot searchedInstance = + searches.Add( + project.FindFSharpReferencesAsync + (symbol, snapshot, searchedInstance, searchThrottle, onFound, "getSymbolUsesInProjects") + ct + ) + + for struct (primary, secondaries) in groups do + let! snapshot = snapshotFor primary + startSearching primary snapshot ValueNone + + for secondary in secondaries do + let! snapshot = snapshotFor secondary + startSearching secondary snapshot (ValueSome primary) + + do! Task.WhenAll searches TelemetryReporter.ReportSingleEvent(TelemetryEvents.GetSymbolUsesInProjectsFinished, props) } @@ -106,7 +180,7 @@ module internal SymbolHelpers = (symbolUse: FSharpSymbolUse) (currentDocument: Document) (checkFileResults: FSharpCheckFileResults) - (onFound: Document -> range -> CancellableTask) + (onFound: Document -> range seq -> CancellableTask) = cancellableTask { match symbolUse.GetSymbolScope currentDocument with @@ -115,10 +189,7 @@ module internal SymbolHelpers = let symbolUses = checkFileResults.GetUsesOfSymbolInFile(symbolUse.Symbol, relatedSymbolKinds = RelatedSymbolUseKind.All) - do! - symbolUses - |> Seq.map (fun symbolUse -> onFound currentDocument symbolUse.Range) - |> CancellableTask.whenAll + do! onFound currentDocument (symbolUses |> Seq.map _.Range) | Some SymbolScope.SignatureAndImplementation -> let otherFile = getOtherFile currentDocument.FilePath @@ -132,52 +203,49 @@ module internal SymbolHelpers = } | ValueNone -> CancellableTask.singleton [] - let symbolUses = - (checkFileResults, currentDocument) :: otherFileCheckResults - |> Seq.collect (fun (checkFileResults, doc) -> + for checkFileResults, doc in (checkFileResults, currentDocument) :: otherFileCheckResults do + let symbolUses = checkFileResults.GetUsesOfSymbolInFile(symbolUse.Symbol, relatedSymbolKinds = RelatedSymbolUseKind.All) - |> Seq.map (fun symbolUse -> (doc, symbolUse.Range))) - do! symbolUses |> Seq.map ((<||) onFound) |> CancellableTask.whenAll + do! onFound doc (symbolUses |> Seq.map _.Range) - | scope -> + | Some(SymbolScope.Projects(scopeProjects, isLocalForProject)) -> let projectsToCheck = - match scope with - | Some(SymbolScope.CurrentDocument) - | Some(SymbolScope.SignatureAndImplementation) -> - // For current document or signature/implementation, just search current project - [ currentDocument.Project ] - | Some(SymbolScope.Projects(scopeProjects, false)) -> + if isLocalForProject then + scopeProjects + else [ for scopeProject in scopeProjects do yield scopeProject yield! scopeProject.GetDependentProjects() ] |> List.distinct - | Some(SymbolScope.Projects(scopeProjects, true)) -> scopeProjects - // The symbol is declared in .NET framework, an external assembly or in a C# project within the solution. - // Optimization: Only search projects that reference the specific assembly - | None -> - match symbolUse.Symbol.Assembly.FileName with - | Some assemblyPath -> - let referencingProjects = - ProjectFiltering.getProjectsReferencingAssembly assemblyPath currentDocument.Project.Solution - - if List.isEmpty referencingProjects then - Seq.toList currentDocument.Project.Solution.Projects - else - referencingProjects - | None -> Seq.toList currentDocument.Project.Solution.Projects - - do! getSymbolUsesInProjects (symbolUse.Symbol, projectsToCheck, onFound) + + do! getSymbolUsesInProjects (symbolUse.Symbol, currentDocument.Project, projectsToCheck, onFound) + + // The symbol is declared in .NET framework, an external assembly or in a C# project within the solution. + // Optimization: Only search projects that reference the specific assembly + | None -> + let projectsToCheck = + match symbolUse.Symbol.Assembly.FileName with + | Some assemblyPath -> + match ProjectFiltering.getProjectsReferencingAssembly assemblyPath currentDocument.Project.Solution with + | [] -> Seq.toList currentDocument.Project.Solution.Projects + | referencingProjects -> referencingProjects + | None -> Seq.toList currentDocument.Project.Solution.Projects + + do! getSymbolUsesInProjects (symbolUse.Symbol, currentDocument.Project, projectsToCheck, onFound) } let getSymbolUses (symbolUse: FSharpSymbolUse) (currentDocument: Document) (checkFileResults: FSharpCheckFileResults) = cancellableTask { let symbolUses = ConcurrentBag() - let onFound = - fun document range -> cancellableTask { symbolUses.Add(document, range) } + let onFound document (ranges: range seq) = + cancellableTask { + for range in ranges do + symbolUses.Add(document, range) + } do! findSymbolUses symbolUse currentDocument checkFileResults onFound diff --git a/vsintegration/src/FSharp.Editor/LanguageService/WorkspaceExtensions.fs b/vsintegration/src/FSharp.Editor/LanguageService/WorkspaceExtensions.fs index 2406f3a6e32..b24473a39ee 100644 --- a/vsintegration/src/FSharp.Editor/LanguageService/WorkspaceExtensions.fs +++ b/vsintegration/src/FSharp.Editor/LanguageService/WorkspaceExtensions.fs @@ -2,7 +2,9 @@ module internal Microsoft.VisualStudio.FSharp.Editor.WorkspaceExtensions open System +open System.Collections.Generic open System.Runtime.CompilerServices +open System.Threading open Microsoft.CodeAnalysis open Microsoft.VisualStudio.FSharp.Editor @@ -11,6 +13,8 @@ open FSharp.Compiler open FSharp.Compiler.CodeAnalysis open FSharp.Compiler.CodeAnalysis.ProjectSnapshot open FSharp.Compiler.Symbols +open FSharp.Compiler.Syntax +open FSharp.Compiler.SyntaxTrivia open FSharp.Compiler.BuildGraph open CancellableTasks @@ -610,16 +614,22 @@ type Document with |> Option.defaultWith (fun _ -> raise (OperationCanceledException("Unable to get FSharp semantic classification."))) } - /// Find F# references in the given F# document. - member inline this.FindFSharpReferencesAsync(symbol, projectSnapshot: FSharpProjectSnapshot, [] onFound, userOpName) = + /// Find F# references in the given F# document, through the project snapshot when the transparent + /// compiler is in use. + member inline this.FindFSharpReferencesAsync + ( + symbol, + projectSnapshot: FSharpProjectSnapshot voption, + [] onFound: Text.range seq -> CancellableTask, + userOpName + ) = cancellableTask { let! checker, _, _, projectOptions = this.GetFSharpCompilationOptionsAsync(userOpName) let! symbolUses = - - if this.Project.UseTransparentCompiler then - checker.FindBackgroundReferencesInFile(this.FilePath, projectSnapshot, symbol) - else + match projectSnapshot with + | ValueSome projectSnapshot -> checker.FindBackgroundReferencesInFile(this.FilePath, projectSnapshot, symbol) + | ValueNone -> checker.FindBackgroundReferencesInFile( this.FilePath, projectOptions, @@ -628,11 +638,7 @@ type Document with fastCheck = this.Project.IsFastFindReferencesEnabled ) - do! - symbolUses - |> Seq.map onFound - |> CancellableTask.whenAll - |> CancellableTask.ignore + do! onFound symbolUses } /// Try to find a F# lexer/token symbol of the given F# document and position. @@ -657,24 +663,106 @@ type Document with ) } -type Project with +let rec private definesTestedBy expr = + seq { + match expr with + | IfDirectiveExpression.And(left, right) + | IfDirectiveExpression.Or(left, right) -> + yield! definesTestedBy left + yield! definesTestedBy right + | IfDirectiveExpression.Not expr -> yield! definesTestedBy expr + | IfDirectiveExpression.Ident name -> yield name + } + +/// Whether the file can parse differently under the given defines: it tests one of them in a +/// conditional directive. A file whose `#if` only tests defines the two instances share compiles to +/// the same tree in both, however many directives it has. +let private dependsOnDefines (defines: string Set) (parseTree: ParsedInput) = + let directives = + match parseTree with + | ParsedInput.ImplFile file -> file.Trivia.ConditionalDirectives + | ParsedInput.SigFile file -> file.Trivia.ConditionalDirectives + + directives + |> List.exists (function + | ConditionalDirectiveTrivia.If(expr, _) + | ConditionalDirectiveTrivia.Elif(expr, _) -> definesTestedBy expr |> Seq.exists defines.Contains + | ConditionalDirectiveTrivia.Else _ + | ConditionalDirectiveTrivia.EndIf _ -> false) + +/// The files two instances of one project compile identically, counting from the first. F# reads a +/// name's meaning from the files ahead of it, so a file that follows one parsing differently can +/// resolve differently however plain it looks itself - the shared run ends at the first such file. +let private identicallyCompiledPrefix + differingDefines + (documentsByPath: Dictionary) + (sourceFiles: string array) + (searchedSourceFiles: string array) + userOpName + = + cancellableTask { + let mutable index = 0 + let mutable diverged = false + + while not diverged && index < sourceFiles.Length do + let path = sourceFiles[index] + + let! compilesTheSame = + if + index >= searchedSourceFiles.Length + || not (String.Equals(path, searchedSourceFiles[index], StringComparison.OrdinalIgnoreCase)) + then + CancellableTask.singleton false + elif Set.isEmpty differingDefines then + CancellableTask.singleton true + else + match documentsByPath.TryGetValue path with + | true, document -> + document.GetFSharpParseResultsAsync userOpName + |> CancellableTask.map (fun parseResults -> not (dependsOnDefines differingDefines parseResults.ParseTree)) + | _ -> CancellableTask.singleton false + + if compilesTheSame then + index <- index + 1 + else + diverged <- true - /// Find F# references in the given project. - member this.FindFSharpReferencesAsync(symbol: FSharpSymbol, projectSnapshot, onFound, userOpName) = - cancellableTask { + return HashSet(Seq.truncate index sourceFiles, StringComparer.OrdinalIgnoreCase) + } - let declarationLocation = - symbol.SignatureLocation - |> Option.map Some - |> Option.defaultValue symbol.DeclarationLocation +/// How many documents of one project a search keeps in flight. The throttle it shares with the other +/// projects decides how many of those actually run. +[] +let private WorkersPerProject = 4 + +type Project with + /// Find F# references in the given project. When `searchedInstance` is another target-framework + /// instance of the same project file that has already been searched, the leading files both + /// compile identically are left to it; the search starts at the first file that can differ and + /// covers everything after it. + member this.FindFSharpReferencesAsync + ( + symbol: FSharpSymbol, + projectSnapshot: FSharpProjectSnapshot voption, + searchedInstance: Project voption, + throttle: SemaphoreSlim, + onFound, + userOpName + ) = + cancellableTask { let declarationDocument = - declarationLocation |> Option.bind this.Solution.TryGetDocumentFromFSharpRange + symbol.SignatureLocation + |> Option.orElse symbol.DeclarationLocation + |> Option.bind (fun range -> + this.Solution.GetDocumentIdsWithFilePath(Path.GetFullPathSafe range.FileName) + |> Seq.tryFind (fun id -> id.ProjectId = this.Id) + |> Option.map this.GetDocument) - // Can we skip documents, which are above current, since they can't contain symbols from current one. + // Documents before the declaration in compile order cannot refer to it. let! canSkipDocuments = match declarationDocument with - | Some document when this.IsFastFindReferencesEnabled && document.Project = this -> + | Some document when this.IsFastFindReferencesEnabled -> cancellableTask { let! _, _, _, options = document.GetFSharpCompilationOptionsAsync(userOpName) @@ -685,7 +773,6 @@ type Project with null return - options.SourceFiles |> Seq.takeWhile ((<>) document.FilePath) |> Seq.filter ((<>) signatureFile) @@ -693,21 +780,51 @@ type Project with } | _ -> CancellableTask.singleton Set.empty - let documents = + // Only the defines one instance has and the other lacks can make a shared file parse + // differently; the ones they agree on cannot, however many directives test them. + let optionsOf (project: Project) = + getFSharpOptionsForProject project + |> CancellableTask.map (fun (_, _, parsingOptions: FSharpParsingOptions, options: FSharpProjectOptions) -> + Set parsingOptions.ConditionalDefines, options.SourceFiles) + + let! alreadyCovered = + match searchedInstance with + | ValueNone -> CancellableTask.singleton (HashSet StringComparer.OrdinalIgnoreCase) + | ValueSome instance -> + cancellableTask { + let! defines, sourceFiles = optionsOf this + let! searchedDefines, searchedSourceFiles = optionsOf instance + + let differingDefines = (defines - searchedDefines) + (searchedDefines - defines) + + let documentsByPath = Dictionary(StringComparer.OrdinalIgnoreCase) + + for document in this.Documents do + documentsByPath[document.FilePath] <- document + + return! identicallyCompiledPrefix differingDefines documentsByPath sourceFiles searchedSourceFiles userOpName + } + + let search (document: Document) = + cancellableTask { + let! ct = CancellableTask.getCancellationToken () + do! throttle.WaitAsync ct + + try + do! document.FindFSharpReferencesAsync(symbol, projectSnapshot, onFound document, userOpName) + finally + throttle.Release() |> ignore + } + + do! this.Documents - |> Seq.filter (fun document -> isFSharpSourceFile document.FilePath) - |> Seq.filter (fun document -> not (canSkipDocuments.Contains document.FilePath)) - - if this.IsFastFindReferencesEnabled then - do! - documents - |> Seq.map (fun doc -> - doc.FindFSharpReferencesAsync(symbol, projectSnapshot, (fun range -> onFound doc range), userOpName)) - // Throttle to avoid launching a typecheck per document in the project all at once. - |> CancellableTask.whenAllThrottled (max 1 Environment.ProcessorCount) - else - for doc in documents do - do! doc.FindFSharpReferencesAsync(symbol, projectSnapshot, (onFound doc), userOpName) + |> Seq.filter (fun document -> + isFSharpSourceFile document.FilePath + && not (canSkipDocuments.Contains document.FilePath) + && not (alreadyCovered.Contains document.FilePath)) + // Workers take the next document when they free up. Starting one task per document + // instead would leave every document of the solution parked on the throttle at once. + |> CancellableTask.forEachThrottled WorkersPerProject search } member this.GetFSharpCompilationOptionsAsync() = this |> getFSharpOptionsForProject diff --git a/vsintegration/src/FSharp.Editor/Navigation/FindUsagesService.fs b/vsintegration/src/FSharp.Editor/Navigation/FindUsagesService.fs index c313d4f27ee..cc80c1338c1 100644 --- a/vsintegration/src/FSharp.Editor/Navigation/FindUsagesService.fs +++ b/vsintegration/src/FSharp.Editor/Navigation/FindUsagesService.fs @@ -2,6 +2,7 @@ namespace Microsoft.VisualStudio.FSharp.Editor +open System open System.Collections.Immutable open System.Composition open System.Threading.Tasks @@ -17,8 +18,9 @@ open CancellableTasks module FSharpFindUsagesService = + /// Reports the uses found in one document: its text is read once for all of them, and Roslyn takes + /// them one at a time anyway. let onSymbolFound - allReferences declarationRange externalDefinitionItem definitionItems @@ -26,36 +28,38 @@ module FSharpFindUsagesService = symbolName (onReferenceFoundAsync: FSharpSourceReferenceItem -> Task) (doc: Document) - (symbolUse: range) + (symbolUses: range seq) = cancellableTask { let! cancellationToken = CancellableTask.getCancellationToken () let! sourceText = doc.GetTextAsync(cancellationToken) - match declarationRange, RoslynHelpers.TryFSharpRangeToTextSpan(sourceText, symbolUse) with - | Some declRange, _ when Range.equals declRange symbolUse -> () - | _, ValueNone -> () - | _, ValueSome _ when not allReferences -> () - | _, ValueSome textSpan -> - match textSpan with - | Tokenizer.FixedSpan sourceText symbolName fixedSpan -> - let definitionItem = - if isExternal then - externalDefinitionItem - else - definitionItems - |> Array.tryFind (snd >> (=) doc.Project.FilePath) - |> Option.map (fun (definitionItem, _) -> definitionItem) - |> Option.defaultValue externalDefinitionItem - - let referenceItem = - FSharpSourceReferenceItem(definitionItem, FSharpDocumentSpan(doc, fixedSpan)) - // REVIEW: OnReferenceFoundAsync is throwing inside Roslyn, putting a try/with so find-all refs doesn't fail. - try - do! onReferenceFoundAsync referenceItem - with _ -> - () - | _ -> () + let definitionItem = + if isExternal then + externalDefinitionItem + else + definitionItems + |> Array.tryFind (snd >> (=) doc.Project.FilePath) + |> Option.map (fun (definitionItem, _) -> definitionItem) + |> Option.defaultValue externalDefinitionItem + + for symbolUse in symbolUses do + cancellationToken.ThrowIfCancellationRequested() + + match declarationRange, RoslynHelpers.TryFSharpRangeToTextSpan(sourceText, symbolUse) with + | Some declRange, _ when Range.equals declRange symbolUse -> () + | _, ValueNone -> () + | _, ValueSome textSpan -> + match textSpan with + | Tokenizer.FixedSpan sourceText symbolName fixedSpan -> + let referenceItem = + FSharpSourceReferenceItem(definitionItem, FSharpDocumentSpan(doc, fixedSpan)) + // REVIEW: OnReferenceFoundAsync is throwing inside Roslyn, putting a try/with so find-all refs doesn't fail. + try + do! onReferenceFoundAsync referenceItem + with error when not (error :? OperationCanceledException) -> + () + | _ -> () } // File can be included in more than one project, hence single `range` may results with multiple `Document`s. @@ -149,17 +153,19 @@ module FSharpFindUsagesService = if isExternal then do! context.OnDefinitionFoundAsync(externalDefinitionItem) - let onFound = - onSymbolFound - allReferences - declarationRange - externalDefinitionItem - definitionItems - isExternal - symbol.Ident.idText - context.OnReferenceFoundAsync - - do! SymbolHelpers.findSymbolUses symbolUse document checkFileResults onFound + // Find Implementations wants the definitions alone: reporting a use is what + // `allReferences` gates, so searching for them would throw the whole search away. + if allReferences then + let onFound = + onSymbolFound + declarationRange + externalDefinitionItem + definitionItems + isExternal + symbol.Ident.idText + context.OnReferenceFoundAsync + + do! SymbolHelpers.findSymbolUses symbolUse document checkFileResults onFound } open FSharpFindUsagesService diff --git a/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj b/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj index ecce1205b8c..6a7f3366dd0 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj +++ b/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj @@ -27,6 +27,7 @@ + diff --git a/vsintegration/tests/FSharp.Editor.Tests/FindReferencesTests.fs b/vsintegration/tests/FSharp.Editor.Tests/FindReferencesTests.fs index 5519fdd337b..4d6755736d8 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/FindReferencesTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/FindReferencesTests.fs @@ -1,12 +1,8 @@ module FSharp.Editor.Tests.FindReferencesTests -open System.Threading.Tasks -open System.Threading open System.IO -open System.Collections.Concurrent open Microsoft.CodeAnalysis.ExternalAccess.FSharp.Editor.FindUsages -open Microsoft.CodeAnalysis.ExternalAccess.FSharp.FindUsages open Microsoft.VisualStudio.FSharp.Editor open Xunit @@ -40,27 +36,7 @@ module FindReferences = let findUsagesService = FSharpFindUsagesService() :> IFSharpFindUsagesService let getContext () = - let foundDefinitions = ConcurrentBag() - let foundReferences = ConcurrentBag() - - let context = - { new IFSharpFindUsagesContext with - - member _.OnDefinitionFoundAsync(definition: FSharpDefinitionItem) = - foundDefinitions.Add definition - Task.CompletedTask - - member _.OnReferenceFoundAsync(reference: FSharpSourceReferenceItem) = - foundReferences.Add reference - Task.CompletedTask - - member _.ReportMessageAsync _ = Task.CompletedTask - member _.ReportProgressAsync(_, _) = Task.CompletedTask - member _.SetSearchTitleAsync _ = Task.CompletedTask - member _.CancellationToken = CancellationToken.None - } - - context, foundDefinitions, foundReferences + RoslynTestHelpers.CreateFindUsagesContext() [] let ``Find references to a document-local symbol`` () = diff --git a/vsintegration/tests/FSharp.Editor.Tests/Helpers/RoslynHelpers.fs b/vsintegration/tests/FSharp.Editor.Tests/Helpers/RoslynHelpers.fs index 25509f14ace..663c81a2298 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/Helpers/RoslynHelpers.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/Helpers/RoslynHelpers.fs @@ -6,9 +6,14 @@ open System open System.IO open System.Reflection open System.Linq +open System.Collections.Concurrent open System.Collections.Generic open System.Collections.Immutable +open System.Threading +open System.Threading.Tasks open Microsoft.CodeAnalysis +open Microsoft.CodeAnalysis.ExternalAccess.FSharp.Editor.FindUsages +open Microsoft.CodeAnalysis.ExternalAccess.FSharp.FindUsages open Microsoft.VisualStudio.Composition open Microsoft.CodeAnalysis.Host open Microsoft.CodeAnalysis.Text @@ -201,6 +206,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 +271,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() @@ -270,6 +310,28 @@ type RoslynTestHelpers private () = static member SetEditorOptions (solution: Solution) options = solution.Workspace.Services.GetService().With(options) + static member CreateFindUsagesContext() = + let foundDefinitions = ConcurrentBag() + let foundReferences = ConcurrentBag() + + let context = + { new IFSharpFindUsagesContext with + member _.OnDefinitionFoundAsync definition = + foundDefinitions.Add definition + Task.CompletedTask + + member _.OnReferenceFoundAsync reference = + foundReferences.Add reference + Task.CompletedTask + + member _.ReportMessageAsync _ = Task.CompletedTask + member _.ReportProgressAsync(_, _) = Task.CompletedTask + member _.SetSearchTitleAsync _ = Task.CompletedTask + member _.CancellationToken = CancellationToken.None + } + + context, foundDefinitions, foundReferences + static member CreateSolution(source, ?options: FSharpProjectOptions, ?extraFSharpProjectOtherOptions: string array, ?editorOptions) = let projId = ProjectId.CreateNewId() @@ -331,12 +393,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 +402,110 @@ 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() + |> 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 diff --git a/vsintegration/tests/FSharp.Editor.Tests/MultiTargetFindReferencesTests.fs b/vsintegration/tests/FSharp.Editor.Tests/MultiTargetFindReferencesTests.fs new file mode 100644 index 00000000000..02bfee08159 --- /dev/null +++ b/vsintegration/tests/FSharp.Editor.Tests/MultiTargetFindReferencesTests.fs @@ -0,0 +1,212 @@ +// Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information. + +/// Projects loaded as two target-framework instances each. In the first, `plain` compiles without +/// the fourth file and without FOO, `foo` compiles everything with FOO defined, and both define +/// COMMON; the second pair differs only on FOO. +module FSharp.Editor.Tests.MultiTargetFindReferencesTests + +open System +open System.IO +open System.Threading +open Xunit +open Microsoft.CodeAnalysis +open Microsoft.CodeAnalysis.ExternalAccess.FSharp.Editor.FindUsages +open Microsoft.VisualStudio.FSharp.Editor +open Microsoft.VisualStudio.FSharp.Editor.CancellableTasks +open FSharp.Compiler.Text +open FSharp.Editor.Tests.Helpers +open FSharp.Test.ProjectGeneration + +let private project = + SyntheticProject.Create( + { sourceFile "First" [] with + SignatureFile = AutoGenerated + 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\nlet useBesideFoo x = ModuleFirst.sharedFunc x" + }, + { sourceFile "Fourth" [ "First" ] with + ExtraSource = "let fooOnlyFileUse x = ModuleFirst.sharedFunc x" + }, + { sourceFile "Fifth" [ "First" ] with + ExtraSource = "#if COMMON\nlet commonUse x = ModuleFirst.sharedFunc x\n#endif" + } + ) + +let private solution, plainId, fooId = + let solution, instances = + RoslynTestHelpers.CreateMultiTargetSolution( + project, + [ + { + Defines = [ "COMMON" ] + ExcludedFileIds = [ "Fourth" ] + } + { + Defines = [ "COMMON"; "FOO" ] + ExcludedFileIds = [] + } + ] + ) + + match instances with + | [ plainId; fooId ] -> solution, plainId, fooId + | _ -> failwith "two instances expected" + +let private firstPath = project.GetFilePath "First" + +let private declarationPosition = + (File.ReadAllText firstPath).IndexOf("sharedFunc", StringComparison.Ordinal) + +let private instanceOf index = if index = 0 then plainId else fooId + +let private documentOf (solution: Solution) (projectId: ProjectId) path = + solution.GetDocumentIdsWithFilePath path + |> Seq.find (fun id -> id.ProjectId = projectId) + |> solution.GetDocument + +let private documentIn projectId path = documentOf solution projectId path + +[] +[] +[] +let ``every file is searched once, files under conditional compilation and instance-only files included`` (instance: int) = + let context, foundDefinitions, foundReferences = + RoslynTestHelpers.CreateFindUsagesContext() + + (FSharpFindUsagesService() :> IFSharpFindUsagesService) + .FindReferencesAsync(documentIn (instanceOf instance) firstPath, declarationPosition, context) + .Wait() + + Assert.Equal(1, foundDefinitions.Count) + // The signature, Second, both uses in Third, Fourth compiled only into foo, and Fifth under the + // define both instances share: one hit each, not one per instance. + Assert.Equal(6, foundReferences.Count) + +/// What Rename works from: the symbol's uses grouped by Roslyn document. +let private usesByDocument position (document: Document) = + let sourceText = document.GetTextAsync(CancellationToken.None).Result + let textLine = sourceText.Lines.GetLineFromPosition position + + let fcsLine = Line.fromZ (sourceText.Lines.GetLinePosition position).Line + + let lexerSymbol = + Tokenizer.getSymbolAtPosition ( + document.Id, + sourceText, + position, + document.FilePath, + [], + SymbolLookupKind.Greedy, + false, + false, + None, + CancellationToken.None + ) + |> Option.defaultWith (fun () -> failwith "symbol not found") + + let _, checkFileResults = + document.GetFSharpParseAndCheckResultsAsync "MultiTargetFindReferences" + |> CancellableTask.runSynchronouslyWithoutCancellation + + let symbolUse = + checkFileResults.GetSymbolUseAtLocation(fcsLine, lexerSymbol.Ident.idRange.EndColumn, textLine.ToString(), lexerSymbol.FullIsland) + |> Option.defaultWith (fun () -> failwith "symbol use not found") + + SymbolHelpers.getSymbolUsesInSolution (symbolUse, checkFileResults, document) + |> CancellableTask.runSynchronouslyWithoutCancellation + +[] +[] +[] +let ``rename gets every use once, from an instance that compiles its file`` (instance: int) = + let uses = + usesByDocument declarationPosition (documentIn (instanceOf instance) firstPath) + + let located = + [ + for KeyValue(documentId, ranges) in uses do + for range in ranges -> solution.GetDocument(documentId).FilePath, range + ] + + // A file several instances compile is searched in each of them; the same use must reach Rename once. + Assert.Equal<(string * range) list>(List.distinct located, located) + + let files = located |> List.map fst |> List.distinct + + for fileId in [ "Second"; "Third"; "Fourth"; "Fifth" ] do + Assert.Contains(project.GetFilePath fileId, files) + + for documentId in uses.Keys do + let compiledHere = + solution.GetProject(documentId.ProjectId).Documents |> Seq.map _.FilePath + + Assert.Contains(solution.GetDocument(documentId).FilePath, compiledHere) + +/// `Consumer` carries no directive of its own, but the record whose field it reads is inferred from +/// `Chooser`, which the two instances compile differently: with FOO the field is `A.Record.value`, +/// without it `B.Record.value`. +let private inferredProject = + SyntheticProject.Create( + "MultiTargetInferredType", + { sourceFile "Chooser" [] with + ExtraSource = + [ + "module A =" + " type Record = { value: int }" + "module B =" + " type Record = { value: int }" + "#if FOO" + "let input: A.Record = { value = 1 }" + "#else" + "let input: B.Record = { value = 1 }" + "#endif" + ] + |> String.concat "\n" + }, + { sourceFile "Consumer" [ "Chooser" ] with + ExtraSource = "let output = ModuleChooser.input.value" + } + ) + +let private inferredSolution, inferredPlainId = + let solution, instances = + RoslynTestHelpers.CreateMultiTargetSolution( + inferredProject, + [ + { Defines = []; ExcludedFileIds = [] } + { + Defines = [ "FOO" ] + ExcludedFileIds = [] + } + ] + ) + + match instances with + | [ plainId; _ ] -> solution, plainId + | _ -> failwith "two instances expected" + +let private chooserPath = inferredProject.GetFilePath "Chooser" + +/// `A.Record.value`, the field only the FOO instance reads outside its own declaration. +let private inferredDeclarationPosition = + (File.ReadAllText chooserPath).IndexOf("value", StringComparison.Ordinal) + +// Searching from the instance without FOO, `Consumer` holds no use of this field - it reads +// `B.Record.value` there. Its use in the FOO instance is still a use, and Rename that misses it +// leaves that build calling a field that no longer exists. +[] +let ``a file without directives is searched when an earlier file changes what its names mean`` () = + let uses = + usesByDocument inferredDeclarationPosition (documentOf inferredSolution inferredPlainId chooserPath) + + let files = + [ + for KeyValue(documentId, _) in uses -> inferredSolution.GetDocument(documentId).FilePath + ] + + Assert.Contains(inferredProject.GetFilePath "Consumer", files)