Search each file of a multi-targeted F# project once in Go To All - #20483
Search each file of a multi-targeted F# project once in Go To All#20483xperiandri wants to merge 5 commits into
Conversation
…r tests Test helpers so far put every synthetic file into one Roslyn project. CreateMultiProjectSolution creates one project per synthetic project with project references, the way VS wires project-to-project references; CreateMultiTargetSolution creates one project per target instance sharing the project path and the document paths, the way VS loads a multi-targeted project. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ead its text only for matches Roslyn's NavigateTo searcher hands the F# service every target-framework instance of a project, one after another, and the service parsed every file of each instance, read the text of every file before matching anything, and started all of that for a project at once. On a solution with 135 project instances that meant one parse and one file read per file per framework, thousands of concurrent tasks, and results that arrived long after the user stopped typing. The first instance of a project file in the solution now searches every file; the other instances only search the files they alone compile and the files whose parse depends on the defines, known from whichever instance parsed the file first. A file's text is read only when one of its declarations matched, and a project's files are searched at most ProcessorCount at a time. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
❗ Release notes requiredYou can open this PR in browser to add release notes: open in github.dev
|
| syntheticProject.GetAllProjects() | ||
| |> List.distinctBy _.Name | ||
| |> List.map (fun project -> project, ProjectId.CreateNewId()) |
There was a problem hiding this comment.
| 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 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)) |
There was a problem hiding this comment.
I suppose it will be more performant, no?
| 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)) |
github-actions
Bot
added
the
AI-Tooling-Check-Scanned-Clean
| instances | ||
| |> List.find (fun id -> documentIds |> Seq.exists (fun documentId -> documentId.ProjectId = id)) | ||
|
|
||
| owner = project.Id |
There was a problem hiding this comment.
🤖🕵️ 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 () // emptyRoslyn submits only the active project for this scope. Preserve project-local results; the solution-order owner is not searched.
There was a problem hiding this comment.
Both are real, and they killed the design rather than a line of it. Pushed a rework.
The skip is gone. I had missed that NavigateToSearcher pools its seen set with NavigateToSearchResultComparer, which already collapses results by file path and span — so the duplicates the skip existed to prevent were never reaching the user anyway, and every instance can safely report what it compiles. That is your first case: the current-project scope submits one project, and the instance that was told to stay quiet is the only one asked.
What the instances should share is the parse, which is the cost. A parse whose tree holds no conditional directives does not depend on the defines, so it is stored under a key no define set can equal and every instance reuses it; one that does hold them is stored per define set. The entry carries the text version, which is your second case: an edit that puts a declaration behind #if is a new entry rather than a flag left over from the parse before it.
The key for the define-independent entry is "?" on purpose — defines are identifiers, so an instance that happens to define nothing cannot read that entry as its own.
The test asserting a declaration is reported once across the instances went with the skip; that is the searcher's job. In its place are your two cases plus one that keeps the define-keyed parse honest — a declaration behind #if FOO must not reach the instance without FOO.
One thing I could not do: run the new tests against the old implementation as a control. Reverting just that file makes fsc.exe die with 0x80131506 in my worktree, reproducibly and regardless of my change, and I did not chase it. So the evidence that these tests catch the two bugs is your repro, not a run of mine. Full FSharp.Editor.Tests on the rework: 7205 passed, 0 failed.
The release note said the old scheme out loud, so it is rewritten too.
| 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 = |
There was a problem hiding this comment.
🤖🕵️ 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
#endifSearching 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.
Skipping a file in every instance but the first is wrong twice over, as the review showed. Go To All scoped to the current project submits only that project, so the instance that was told to stay quiet is the only one asked, and a shared declaration disappears from it once the parse has taught the service the file holds no directives. And the flag that decision reads was keyed by path alone, so an edit that puts a declaration behind `#if` left the previous answer in place. Nothing needs skipping. `NavigateToSearcher` pools its seen set with `NavigateToSearchResultComparer`, which already collapses results by file path and span, so every instance can report what it compiles. What the instances should share is the parse, which is what costs. A parse whose tree holds no conditional directives does not depend on the defines, so it is kept under a key no define set can equal and every instance reuses it; one that does hold them is kept per define set, since those instances genuinely parse the file differently. The entry carries the text version, so an edit is a new entry rather than a stale flag. The test that asserted a declaration is reported once across the instances went with the skip: that is the searcher's job, not this service's. In its place are the two cases from the review — a lone instance reporting a shared declaration, and a declaration an edit puts behind a directive — and one that keeps the define-keyed parse honest by checking such a declaration does not reach the instance without the define. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
the
⚠️ Affects-Design-Time
|
🔍 Tooling Safety Check — Affects-Design-Time
|
2 participants
Description
Go To All (Ctrl+T / Code Search) on a multi-targeted F# solution showed F# results late — often only on the second search — and the window stalled while it searched. Reproduced on a solution with 135 project instances (26 project files, five target frameworks each for the app projects).
How the search runs: Roslyn's
NavigateToSearcherhands the F# service every project instance one after another (the ExternalAccess bridge loopsSearchProjectAsyncover the projects of a group), and only publishes a project's results when the whole project is done. The F# service then:DocumentIdand the defines differ;Change (
NavigateToSearchService.fs):ParsedInput … Trivia.ConditionalDirectives), remembered per file path from whichever instance parsed the file first; an instance that meets a file before any parse of it searches it as before. Results for one file therefore come from one instance, except under conditional compilation.ProcessorCountat a time (whenAllThrottled).Not changed: Roslyn searches only its own persisted index while the solution is not fully loaded (
SearchCachedDocumentsAsyncis skipped for services withoutIAdvancedNavigateToSearchService), so F# results still appear only once the solution has loaded; that needs an ExternalAccess extension.Tests:
MultiTargetNavigateToSearchTestsloads one project as two instances (one withoutFOOand without the fourth file, one with both) throughRoslynTestHelpers.CreateMultiTargetSolutionand searches both instances in solution order: a declaration in a file every instance compiles, one under#if FOO, and one in the instance-only file are each reported exactly once. The first commit (test helpers) is shared with #20462.No timings are claimed: per search the work goes from one parse and one file read per file per framework to one parse per file (plus the files with conditional directives) and a read per matched file.
Checklist
Test cases added
Performance benchmarks added in case of performance changes
Release notes entry updated:
🤖 Generated with Claude Code