diff --git a/docs/release-notes/.VisualStudio/18.vNext.md b/docs/release-notes/.VisualStudio/18.vNext.md index ba03f663967..a153a483a20 100644 --- a/docs/release-notes/.VisualStudio/18.vNext.md +++ b/docs/release-notes/.VisualStudio/18.vNext.md @@ -5,6 +5,7 @@ ### Fixed +* Go To Definition, Find All References and Rename reach the source of a symbol whose assembly was built with `--pathmap` (as `DeterministicSourcePaths` sets it). Such an assembly names its files relative to a root it does not record, and that name was resolved against the process's current directory — which is not the root, and belongs to whatever last set it — so navigation landed on a file that does not exist and fell back to generated metadata. Such a name is now matched against the paths the solution already knows. ([PR #20519](https://github.com/dotnet/fsharp/pull/20519)) * Improve Find All References performance by throttling parallel typechecks. ([PR #20128](https://github.com/dotnet/fsharp/pull/20128)) * Fixed Rename incorrectly renaming `get` and `set` keywords for properties with explicit accessors. ([Issue #18270](https://github.com/dotnet/fsharp/issues/18270), [PR #19252](https://github.com/dotnet/fsharp/pull/19252)) * Fixed Find All References crash when F# project contains non-F# files like `.cshtml`. ([Issue #16394](https://github.com/dotnet/fsharp/issues/16394), [PR #19252](https://github.com/dotnet/fsharp/pull/19252)) @@ -15,6 +16,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 Definition into an F# project built with a path map (`DeterministicSourcePaths` or `PathMap`) opens its source instead of a generated signature: the IDE no longer applies `--pathmap` to the project options it checks with. ([PR #20470](https://github.com/dotnet/fsharp/pull/20470)) ### Changed diff --git a/vsintegration/src/FSharp.Editor/Common/CodeAnalysisExtensions.fs b/vsintegration/src/FSharp.Editor/Common/CodeAnalysisExtensions.fs index e0b29c8f9f1..bf8d36793d9 100644 --- a/vsintegration/src/FSharp.Editor/Common/CodeAnalysisExtensions.fs +++ b/vsintegration/src/FSharp.Editor/Common/CodeAnalysisExtensions.fs @@ -3,8 +3,32 @@ module internal Microsoft.VisualStudio.FSharp.Editor.CodeAnalysisExtensions open Microsoft.CodeAnalysis open FSharp.Compiler.Text +open System open System.IO +/// Whether the file name a compiler range carries is the file at this path. A build that maps its source +/// paths (`DeterministicSourcePaths`) leaves that name relative to a root the assembly never records, so a +/// relative one is matched by its tail rather than resolved against the process's current directory — +/// which is not that root, and belongs to whatever last set it. +let isTheFileAt (path: string) (fileName: string) = + // Paths, not identifiers: the file systems this runs on do not case them. + let comparison = StringComparison.OrdinalIgnoreCase + + match path, fileName with + | null, _ + | _, null -> false + | path, rooted when Path.IsPathRooted rooted -> String.Equals(Path.GetFullPathSafe rooted, path, comparison) + | path, relative -> + let separator = string Path.DirectorySeparatorChar + + let fromTheRoot = + relative.Split([| '/'; '\\' |], StringSplitOptions.RemoveEmptyEntries) + |> Array.filter (fun segment -> segment <> ".") + |> String.concat separator + + // Anchored on a separator so that a name matches whole directories, never the tail of one. + path.EndsWith($"{separator}{fromTheRoot}", comparison) + type Project with /// Returns the projectIds of all projects within the same solution that directly reference this project @@ -80,13 +104,26 @@ type Solution with member self.GetAllProjectsThisProjectDependsOn(projectId: ProjectId) = self.GetProjectIdsOfAllProjectReferences projectId |> Seq.map self.GetProject + /// The documents whose file is the one a compiler range names. A name a path map left relative + /// reaches no document through the workspace's index, which is keyed by the paths on disk, so it + /// is matched against those paths one by one instead. + member self.GetDocumentIdsWithFSharpFileName(fileName: string) = + match fileName with + | null -> [] + | rooted when Path.IsPathRooted rooted -> self.GetDocumentIdsWithFilePath(Path.GetFullPathSafe rooted) |> List.ofSeq + | relative -> + [ + for project in self.Projects do + for document in project.Documents do + if relative |> isTheFileAt document.FilePath then + document.Id + ] + /// Try to retrieve the corresponding DocumentId for the range's file in the solution /// and if a projectId is provided, only try to find the document within that project /// or a project referenced by that project member self.TryGetDocumentIdFromFSharpRange(range: range, ?projectId: ProjectId) = - let filePath = System.IO.Path.GetFullPathSafe range.FileName - let checkProjectId (docId: DocumentId) = if projectId.IsSome then docId.ProjectId = projectId.Value @@ -107,7 +144,7 @@ type Solution with matchingDoc tail | None -> Some docId - self.GetDocumentIdsWithFilePath filePath |> List.ofSeq |> matchingDoc + self.GetDocumentIdsWithFSharpFileName range.FileName |> matchingDoc /// Try to retrieve the corresponding Document for the range's file in the solution /// and if a projectId is provided, only try to find the document within that project diff --git a/vsintegration/src/FSharp.Editor/LanguageService/FSharpProjectOptionsManager.fs b/vsintegration/src/FSharp.Editor/LanguageService/FSharpProjectOptionsManager.fs index 7cd53631893..d040a8034fc 100644 --- a/vsintegration/src/FSharp.Editor/LanguageService/FSharpProjectOptionsManager.fs +++ b/vsintegration/src/FSharp.Editor/LanguageService/FSharpProjectOptionsManager.fs @@ -366,8 +366,10 @@ type private FSharpProjectOptionsReactor(checker: FSharpChecker) = [| // Clear any references from CompilationOptions. // We get the references from Project.ProjectReferences/Project.MetadataReferences. + // A path map belongs to the build output: applied here it rewrites the file name of + // every range imported from a referenced project, and navigation finds no document. for x in projectSite.CompilationOptions do - if not (x.Contains("-r:")) then + if not (x.Contains("-r:") || x.StartsWith("--pathmap:", StringComparison.Ordinal)) then x for x in project.MetadataReferences.OfType() do diff --git a/vsintegration/src/FSharp.Editor/LanguageService/Symbols.fs b/vsintegration/src/FSharp.Editor/LanguageService/Symbols.fs index 19e446f2d08..83e7854803a 100644 --- a/vsintegration/src/FSharp.Editor/LanguageService/Symbols.fs +++ b/vsintegration/src/FSharp.Editor/LanguageService/Symbols.fs @@ -64,7 +64,7 @@ type FSharpSymbolUse with Some(SymbolScope.Projects([ currentDocument.Project ], isSymbolLocalForProject)) else let projects = - currentDocument.Project.Solution.GetDocumentIdsWithFilePath(filePath) + currentDocument.Project.Solution.GetDocumentIdsWithFSharpFileName loc.FileName |> Seq.map (fun x -> x.ProjectId) |> Seq.distinct |> Seq.map currentDocument.Project.Solution.GetProject diff --git a/vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs b/vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs index 6bc86ae57a3..3fbd0c5727a 100644 --- a/vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs +++ b/vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs @@ -355,7 +355,7 @@ type internal GoToDefinition(metadataAsSource: FSharpMetadataAsSourceService) = let! ct = Async.CancellationToken |> liftAsync match targetSymbolUse.Symbol.DeclarationLocation with - | Some decl when decl.FileName = filePath -> return decl + | Some decl when decl.FileName |> isTheFileAt filePath -> return decl | _ -> let! _, checkFileResults = document.GetFSharpParseAndCheckResultsAsync("FindSymbolDeclarationInDocument") diff --git a/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj b/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj index ecce1205b8c..a62b25af1cf 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj +++ b/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj @@ -28,6 +28,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/PathMapNavigationTests.fs b/vsintegration/tests/FSharp.Editor.Tests/PathMapNavigationTests.fs new file mode 100644 index 00000000000..3e127a02140 --- /dev/null +++ b/vsintegration/tests/FSharp.Editor.Tests/PathMapNavigationTests.fs @@ -0,0 +1,108 @@ +// Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information. + +/// A library whose build maps its source paths, as DeterministicSourcePaths does: the symbols another +/// project imports from it must still name the files of the workspace. +module FSharp.Editor.Tests.PathMapNavigationTests + +open System +open System.IO +open System.Threading +open Xunit +open Microsoft.VisualStudio.FSharp.Editor +open Microsoft.VisualStudio.FSharp.Editor.CancellableTasks +open FSharp.Compiler.Text +open FSharp.Editor.Tests.Helpers +open FSharp.Test.ProjectGeneration + +/// As Directory.Build.props would set it: the same map on every project of the solution. +let private pathMap (project: SyntheticProject) = + [ $"--pathmap:{Path.GetDirectoryName project.ProjectDir}=.\\" ] + +let private library = + let library = SyntheticProject.Create("Library", sourceFile "Library" []) + + { library with + OtherOptions = pathMap library + } + +let private app = + let app = SyntheticProject.Create("App", sourceFile "App" [ "Library" ]) + + { app with + DependsOn = [ library ] + OtherOptions = pathMap app + } + +let private solution, _ = RoslynTestHelpers.CreateMultiProjectSolution app + +let private documentOf (project: SyntheticProject) fileId = + solution.GetDocumentIdsWithFilePath(project.GetFilePath fileId) + |> Seq.exactlyOne + |> solution.GetDocument + +[] +let ``the path map of a project is not applied in the IDE`` () = + let _, _, _, options = + (documentOf library "Library").GetFSharpCompilationOptionsAsync "test" + |> CancellableTask.runSynchronouslyWithoutCancellation + + Assert.DoesNotContain(options.OtherOptions, fun option -> option.StartsWith("--pathmap:", StringComparison.Ordinal)) + +[] +let ``goto definition into a project built with a path map reaches its source`` () = + let appDocument = documentOf app "App" + let text = appDocument.GetTextAsync(CancellationToken.None).Result.ToString() + + let position = + text.IndexOf("ModuleLibrary.f", StringComparison.Ordinal) + + "ModuleLibrary.f".Length + - 1 + + let result = + GoToDefinition(FSharpMetadataAsSourceService()).FindDefinitionAtPosition(appDocument, position) + |> CancellableTask.runSynchronouslyWithoutCancellation + + match result with + | ValueSome(FSharpGoToDefinitionResult.NavigableItem item, _) -> Assert.Equal(library.GetFilePath "Library", item.Document.FilePath) + | result -> failwith $"expected a navigable item, got %A{result}" + +/// The one rule the document lookup and the search for a declaration inside a document both go through. +/// A mapped name arrives with the separator its replacement doubled (`.\` + `\rest`), which is what the +/// compiler writes and what a build on a path map hands back. +[] +[] +[] +[] +[] +[] +let ``a relative name denotes the file whose path ends with it`` (path: string) (fileName: string) (expected: bool) = + Assert.Equal(expected, fileName |> isTheFileAt path) + +/// An assembly built with a path map records no root for the names it maps, so a name that arrives +/// relative cannot be resolved against the current directory: that belongs to the process, not to the +/// solution, and points wherever the last component to set it left it. +[] +let ``a range a path map left relative still names its document`` () = + let real = library.GetFilePath "Library" + let root = Path.GetDirectoryName library.ProjectDir + + let relative = + $".\\{real.Substring(root.Length).TrimStart(Path.DirectorySeparatorChar)}" + + let range = Range.mkRange relative (Position.mkPos 1 0) (Position.mkPos 1 0) + + match solution.TryGetDocumentIdFromFSharpRange range with + | Some documentId -> Assert.Equal(real, solution.GetDocument(documentId).FilePath) + | None -> failwith $"no document is named by {relative}" + +[] +let ``a relative name is matched by whole directories, not by the tail of one`` () = + let real = library.GetFilePath "Library" + let root = Path.GetDirectoryName library.ProjectDir + let insideASegment = real.Substring(root.Length + 2) + + let range = Range.mkRange insideASegment (Position.mkPos 1 0) (Position.mkPos 1 0) + + match solution.TryGetDocumentIdFromFSharpRange range with + | Some documentId -> failwith $"{insideASegment} must not name {solution.GetDocument(documentId).FilePath}" + | None -> ()