Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/release-notes/.VisualStudio/18.vNext.md
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖🕵️ [P2] Foreign mapped filenames now throw during document lookup on the editor's .NET Framework runtime. An imported DLL carrying this filename previously returned no matches; Path.IsPathRooted now throws before GetFullPathSafe can protect the lookup. Keep invalid/non-native filenames on a non-throwing path.

Imported declaration filename: /home/build/a|b/Library.fs
Base: 0 matching documents
HEAD: System.ArgumentException: Illegal characters in path.

| relative ->
[
for project in self.Projects do
for document in project.Documents do
if relative |> isTheFileAt document.FilePath then

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖🕵️ [P2] Repeated external-declaration lookups add seconds and gigabytes of allocations to Find All References. Project.FindFSharpReferencesAsync resolves the same declaration once per searched project, so a missing mapped source repeats this whole-solution scan. On desktop CLR, 201 lookups over 20,000 documents measured 2.72 s and 1.91 GB allocated, versus 0.37 ms and 32 KB before. Normalize the suffix once and reuse lookup results, including misses, across the search.

// ExternalLibrary.dll was built from Library.fs with --pathmap:C:\package=.\
// Solution: 200 projects, 100 documents each, all referencing that DLL.
// Library.fs is not in the solution. Find All References on value:
let result = ExternalLibrary.value
// Repeated declaration lookup input: .\\Library.fs

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
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<PortableExecutableReference>() do
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖🕵️ [P1] Find All References loses the consumer's reference when an unrelated document has the mapped suffix. The executed service returns 0 references here; base returns 1. The suffix match selects only Unrelated and bypasses the assembly-reference fallback. Check the defining assembly before restricting the scope.

// C:\package\Library.fs -> ExternalLibrary.dll, --pathmap:C:\package=.
module ExternalLibrary
let value = 42

// Consumer/App.fs; references ExternalLibrary.dll
module Consumer
let result = ExternalLibrary.value // Find All References on value

// Unrelated/Library.fs; separate solution project, no reference to the DLL
module Unrelated
let value = 99

|> Seq.map (fun x -> x.ProjectId)
|> Seq.distinct
|> Seq.map currentDocument.Project.Solution.GetProject
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
<Compile Include="CompletionProviderTests.fs" />
<Compile Include="FindReferencesTests.fs" />
<Compile Include="GoToDefinitionServiceTests.fs" />
<Compile Include="PathMapNavigationTests.fs" />
<Compile Include="HelpContextServiceTests.fs" />
<Compile Include="QuickInfoTests.fs" />
<Compile Include="TaskListServiceTests.fs" />
Expand Down
146 changes: 140 additions & 6 deletions vsintegration/tests/FSharp.Editor.Tests/Helpers/RoslynHelpers.fs
Original file line number Diff line number Diff line change
Expand Up @@ -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 () =

Expand Down Expand Up @@ -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>()
Expand Down Expand Up @@ -331,19 +366,118 @@ 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()
|> 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
Expand Down
Loading
Loading