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
1 change: 1 addition & 0 deletions docs/release-notes/.VisualStudio/18.vNext.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 and reads every file once per target framework: the first instance searches every file, the others only the files they alone compile or that use conditional compilation, and a file's text is read only for a matched declaration. ([PR #20483](https://github.com/dotnet/fsharp/pull/20483))

### Changed

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ open Microsoft.VisualStudio.LanguageServices
open Microsoft.VisualStudio.Text.PatternMatching

open FSharp.Compiler.EditorServices
open FSharp.Compiler.Syntax
open CancellableTasks

[<Export(typeof<IFSharpNavigateToSearchService>); Shared>]
Expand All @@ -26,12 +27,22 @@ type internal FSharpNavigateToSearchService

let cache = ConcurrentDictionary<DocumentId, VersionStamp * NavigableItem array>()

/// Whether the file's parse depends on the defines, by file path: known once any instance has parsed it.
let conditionalDirectives =

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.

🤖🕵️ Newly conditional declarations are missing from the first solution search after an edit. Warm the cache on a shared file without directives, then replace its text in both target instances with:

module ModuleSecond
#if FOO
let addedFoo = 1
#endif

Searching addedFoo in the FOO instance first, then the plain owner, returns no result; repeating finds it. Roslyn prioritizes the active project, so this order occurs in normal searches. Validate the cached flag against the current text version before skipping.

ConcurrentDictionary<string, bool>(StringComparer.OrdinalIgnoreCase)

do
if workspace <> null then
workspace.WorkspaceChanged.Add
<| fun e ->
if e.NewSolution.Id <> e.OldSolution.Id then
cache.Clear()
conditionalDirectives.Clear()

let hasConditionalDirectives (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 {
Expand All @@ -44,9 +55,42 @@ type internal FSharpNavigateToSearchService
let! parseResults = document.GetFSharpParseResultsAsync(nameof (FSharpNavigateToSearchService))
let items = NavigateTo.GetNavigableItems parseResults.ParseTree
cache[document.Id] <- currentVersion, items

match document.FilePath with
| null -> ()
| path -> conditionalDirectives[path] <- hasConditionalDirectives parseResults.ParseTree

return items
}

/// A multi-targeted project is one Roslyn project per target framework over the same files. The
/// first instance in the solution searches every file; the others only the files they alone compile
/// and the files whose parse depends on the defines.
let searchedIn (project: Project) =
match project.FilePath with
| null -> fun (_: Document) -> true
| projectPath ->
let instances =
project.Solution.Projects
|> Seq.filter (fun p -> p.FilePath = projectPath)
|> Seq.map _.Id
|> List.ofSeq

fun (document: Document) ->
match document.FilePath with
| null -> true
| path ->
let documentIds = project.Solution.GetDocumentIdsWithFilePath path

let owner =
instances
|> List.find (fun id -> documentIds |> Seq.exists (fun documentId -> documentId.ProjectId = id))
Comment on lines +73 to +87

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I suppose it will be more performant, no?

Suggested change
let instances =
project.Solution.Projects
|> Seq.filter (fun p -> p.FilePath = projectPath)
|> Seq.map _.Id
|> List.ofSeq
fun (document: Document) ->
match document.FilePath with
| null -> true
| path ->
let documentIds = project.Solution.GetDocumentIdsWithFilePath path
let owner =
instances
|> List.find (fun id -> documentIds |> Seq.exists (fun documentId -> documentId.ProjectId = id))
let instances =
project.Solution.Projects
|> Seq.filter (fun p -> p.FilePath = projectPath)
|> Seq.map _.Id
|> Seq.toArray
fun (document: Document) ->
match document.FilePath with
| null -> true
| path ->
let documentIds = project.Solution.GetDocumentIdsWithFilePath path
let owner =
instances
|> Array.find (fun id -> documentIds |> Seq.exists (fun documentId -> documentId.ProjectId = id))


owner = project.Id

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.

🤖🕵️ Shared declarations disappear from Current Project searches on a non-owner target after the cache warms.

// Existing multi-target fixture; fresh service, second target active.
let p = solution.GetProject instances[1]
let run () =
    service.SearchProjectAsync(p, ImmutableArray.Empty, "plainUse",
        service.KindsProvided, CancellationToken.None).Result
run () // contains plainUse
run () // empty

Roslyn submits only the active project for this scope. Preserve project-local results; the solution-order owner is not searched.

|| (match conditionalDirectives.TryGetValue path with
| true, dependsOnDefines -> dependsOnDefines
| _ -> true)

let kindsProvided =
ImmutableHashSet.Create(
FSharpNavigateToItemKind.Module,
Expand Down Expand Up @@ -145,46 +189,44 @@ type internal FSharpNavigateToSearchService

let processDocument (tryMatch: NavigableItem -> PatternMatch voption) (kinds: IImmutableSet<string>) (document: Document) =
cancellableTask {
let! ct = CancellableTask.getCancellationToken ()

let! sourceText = document.GetTextAsync ct

let! items = getNavigableItems document

let processed =
let matches =
[|
for item in items do
let contains = kinds.Contains(navigateToItemKindToRoslynKind item.Kind)
let patternMatch = tryMatch item
if kinds.Contains(navigateToItemKindToRoslynKind item.Kind) then
match tryMatch item with
| ValueSome m -> yield struct (item, m)
| ValueNone -> ()
|]

match contains, patternMatch with
| true, ValueSome m ->
let sourceSpan = RoslynHelpers.TryFSharpRangeToTextSpan(sourceText, item.Range)
// The text, read from disk for a closed document, is only needed to place the matches.
if matches.Length = 0 then
return [||]
else
let! ct = CancellableTask.getCancellationToken ()
let! sourceText = document.GetTextAsync ct

match sourceSpan with
return
[|
for struct (item, m) in matches do
match RoslynHelpers.TryFSharpRangeToTextSpan(sourceText, item.Range) with
| ValueNone -> ()
| ValueSome sourceSpan ->
let glyph = navigateToItemKindToGlyph item.Kind
let kind = navigateToItemKindToRoslynKind item.Kind
let additionalInfo = formatInfo item.Container document

yield
FSharpNavigateToSearchResult(
additionalInfo,
kind,
formatInfo item.Container document,
navigateToItemKindToRoslynKind item.Kind,
patternMatchKindToNavigateToMatchKind m.Kind,
item.Name,
FSharpNavigableItem(
glyph,
navigateToItemKindToGlyph item.Kind,
ImmutableArray.Create(TaggedText(TextTags.Text, item.Name)),
document,
sourceSpan
)
)
| _ -> ()
|]

return processed
|]
}

interface IFSharpNavigateToSearchService with
Expand All @@ -194,22 +236,14 @@ 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.filter (searchedIn project)
|> 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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
<Compile Include="QuickInfoTests.fs" />
<Compile Include="TaskListServiceTests.fs" />
<Compile Include="NavigateToSearchServiceTests.fs" />
<Compile Include="MultiTargetNavigateToSearchTests.fs" />
<Compile Include="CodeFixes\CodeFixTestFramework.fs" />
<Compile Include="CodeFixes\AddInstanceMemberParameterTests.fs" />
<Compile Include="CodeFixes\ConvertToAnonymousRecordTests.fs" />
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
}
Comment thread
xperiandri marked this conversation as resolved.

[<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())
Comment on lines +384 to +386

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
syntheticProject.GetAllProjects()
|> List.distinctBy _.Name
|> List.map (fun project -> project, ProjectId.CreateNewId())
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
Expand Down
Loading
Loading