Skip to content

Search each file of a multi-targeted F# project once in Find All References and Rename - #20464

Draft
xperiandri wants to merge 10 commits into
dotnet:mainfrom
xperiandri:perf/find-references-multitarget
Draft

xperiandri wants to merge 10 commits into
dotnet:mainfrom
xperiandri:perf/find-references-multitarget

Conversation

@xperiandri

@xperiandri xperiandri commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Find All References and Rename on a symbol of a multi-targeted F# project did every piece of work once per target framework instance — measured at 86 instances of 26 project files (1246 documents) for a scope of two projects, with Roslyn's window hiding the duplicate results and the user only seeing the time.

Each project file's instances now share one full search plus the files that can differ between them — compiled only in one, or conditionally on a define the two disagree on; the rest of the search is skipped, and a use is reported once per range no matter how many instances or project files compile the file that has it. Rename needs no separate change: it already merges the edits of linked documents.

With the option that enables this off, every instance is still searched in full, but the concurrency bound now applies across the solution rather than per project. With it on, an instance can in principle resolve an overload differently without any #if — an extension member shadowed by a newer BCL intrinsic; turning the option off restores the full search.

No timings are claimed for the change itself: the instance count above is what the search scope becomes (86 → 26, plus the conditional and instance-only files of the other 60). The first commit (test helpers) is shared with #20462 and #20463.

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

❗ Release notes required

You can open this PR in browser to add release notes: open in github.dev


✅ Found changes and release notes in following paths:

Change path Release notes path Description
`vsintegration/src` docs/release-notes/.VisualStudio/18.vNext.md

xperiandri added a commit to xperiandri/fsharp that referenced this pull request Sep 6, 2026
@github-actions github-actions Bot added the AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files label Sep 6, 2026
CancellableTask.singleton false
else
document.GetFSharpParseResultsAsync userOpName
|> CancellableTask.map (fun parseResults -> dependsOnDefines differingDefines parseResults.ParseTree)

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] Rename leaves the other target uncompilable when an earlier file changes a shared consumer’s inferred type. Compile these files in order in two instances, only one defining FOO, then rename A.Record.value from the instance without FOO: BASE returns three ranges; HEAD omits Second.fs, and renaming to renamed leaves FS0039 in the FOO instance. A consumer without directives still needs searching when its dependencies differ.

// First.fs
module First
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
// Second.fs
module Second
let output = First.input.value

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.

You are right, and I think your example rules out the narrower fix I would have reached for.

My first instinct was to keep the skip and tighten the predicate — compare the defines the two instances actually differ in, rather than asking whether the file contains any #if at all. Second.fs kills that: it has no directives, and nothing about the file itself says its meaning depends on FOO. The difference arrives through First.fs, so any predicate that looks at the candidate file alone is unsound. Making it sound means asking whether the file's transitive dependencies resolve identically in both instances, which is the type check I am trying to avoid running.

So the honest options are:

  1. Skip an instance's file only when that instance is identical to the owner in both defines and file set. Correct, and it keeps the win for the case where two instances really are the same (the same defines under two target frameworks, which does happen), but on a genuinely multi-targeted project it degenerates to searching everything — the PR stops delivering what its title claims.
  2. Drop the skip and find the saving somewhere that does not change results. For Find All References and Rename I do not see one: the cost here is the per-instance type check, and with different defines those check results genuinely differ, so there is nothing to share.

Unless you see a third option, I would rather withdraw this one than ship an unsound filter, and keep the sibling PR for Go To All, where the results come from parsing rather than checking — there the parse of a file with no directives really is instance-independent, so the same file can be parsed once and its items reused, and Roslyn's own NavigateToSearchResultComparer already collapses the duplicate results by file and span.

Two things I should own about the original: the (file, range) result dedup you would expect on the F# side was never there, which is why the filter was carrying that weight, and the #if predicate was far too loose a proxy for "same results" even before your dependency case.

How would you like to proceed?

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.

Good catch, thanks — fixed in a670ed8. The per-file predicate was wrong: it only asked whether this file's own #if tests a differing define, but a file with no directives at all can still resolve differently when an earlier file's conditional compilation changes what a shared name means, exactly like your repro.

Replaced it with identicallyCompiledPrefix, which walks SourceFiles in compile order and stops at the first file that can diverge (either its own conditional compilation depends on a differing define, or the two instances' source-file lists themselves differ from that point on); only files before that point are skipped, everything from there on is searched in full.

Added your repro almost verbatim as a test (a file without directives is searched when an earlier file changes what its names mean): Chooser.fs picks A.Record/B.Record under #if FOO, Consumer.fs reads .value with no directives of its own; searching A.Record.value from the instance without FOO now still finds the use in Consumer.fs. Confirmed it fails on the old per-file rule and passes with the fix.

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.

Walking back my "fixed in a670ed8" from a few minutes ago — I hadn't seen the fuller analysis above it on this thread when I posted that. identicallyCompiledPrefix does correctly handle the repro you gave, but it doesn't close the gap that reply already identified: it only tracks divergence through #if/file-set, not through referenced assemblies, so it's not actually sound in general.

I went and measured option 1 (skip only when the two instances are identical in both defines and file set) before trying to implement it, since it seemed worth checking whether it degenerates the way the analysis above predicted. It does, completely: built the same trivial two-file project as net472;net9.0, net8.0;net9.0, and compared what fsc.exe actually receives.

  • Defines: net472 gets 17 (NETFRAMEWORK, NET47_OR_GREATER, …), net9.0 gets 15 (NETCOREAPP, NET9_0, NET9_0_OR_GREATER, …) — only DEBUG/TRACE in common. Even adjacent net8.0/net9.0 differ (each has its own exact-version symbol and a different _OR_GREATER boundary).
  • References: net472 resolves 12, net9.0 resolves 165 — nowhere close by path (different TFM folder) or by count.

So for any two distinct real TargetFrameworks, both signals always say "different," every time — the SDK auto-injects framework-identifying defines specifically so code can branch on them. Option 1 isn't a narrower, safer version of the skip; it's a no-op dressed up as one, and it would ship silently doing nothing on exactly the projects the PR's own numbers were measured against.

I don't see a fourth option beyond the three you laid out, and I'm not going to invent a per-file soundness argument without running the check it's meant to avoid. Moved this to draft for now. Is there an angle I'm missing, or does this settle it — keep #20483 and close this one?

@T-Gro T-Gro added the AI-reviewed PR reviewed by AI review council label Sep 9, 2026
@T-Gro
T-Gro self-requested a review September 9, 2026 09:15
xperiandri added a commit to xperiandri/fsharp that referenced this pull request Sep 9, 2026
@xperiandri
xperiandri force-pushed the perf/find-references-multitarget branch from 66bec25 to a670ed8 Compare September 9, 2026 23:04
@github-actions github-actions Bot added the ⚠️ Affects-Design-Time Tooling check: PR touches type providers or dependency manager label Sep 9, 2026
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

🔍 Tooling Safety Check — Affects-Design-Time
Affects-Design-Time: Find References and Rename changes execute in IDE workflows.

Generated by PR Tooling Safety Check · gpt56 2.6M ·

@xperiandri
xperiandri marked this pull request as draft September 10, 2026 10:02
xperiandri added a commit to xperiandri/fsharp that referenced this pull request Sep 11, 2026
@xperiandri
xperiandri force-pushed the perf/find-references-multitarget branch from a670ed8 to b1e5b1b Compare September 11, 2026 16:18

@T-Gro T-Gro left a comment

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.

🤖🕵️ If this fixes an issue or implements an RFC/suggestion, link it (Fixes #... when applicable). Otherwise, give a short management-level summary in simplified technical English: what user scenario improves and what this achieves.

Please apply this PR-description guidance. Remove the implementation inventory already visible in Files, but keep necessary scope, compatibility, and dependency caveats.

@github-project-automation github-project-automation Bot moved this from New to In Progress in F# Compiler and Tooling Sep 14, 2026
xperiandri and others added 10 commits September 14, 2026 17:31
…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>
The IFSharpFindUsagesContext stub of FindReferencesTests moves to
RoslynTestHelpers.CreateFindUsagesContext so other test files can collect the
definitions and references a search reports.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… the solution

Find All References and Rename searched every target-framework instance of a
multi-targeted F# project in full, and built the snapshots of all projects in
scope before the first search started. Instances of one project file are now
grouped: the instance of the current document (or one in its dependency
closure) is searched in full, the others only for files compiled solely there
and for files with conditional compilation directives, whose sources can
differ between instances. Searches start as soon as a project's snapshot is
ready, snapshots are built only for the transparent compiler, and one
SemaphoreSlim bounds the concurrent file checks across the whole search
instead of per project.

With EnableFastFindReferencesAndRename off every instance is still searched in
full; the concurrency bound then replaces the previous per-project sequential
loop.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
One project loaded as two instances (one without FOO and without the fourth
file, one with both): every file is reported once, and Rename gets one
document per file, owned by an instance that compiles it.

The fixture lives in its own file: the modules of one file share a static
initializer, so a second fixture module makes the first module's values
observable before they are assigned when xunit runs the classes in parallel.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…editor

Three things the search did on every request: it walked the solution for Find
Implementations, which reports no uses at all and threw the result away; it
started a task per reference found, each fetching the document's text again;
and it swallowed the cancellation Roslyn raises when the user closes the window
or starts another search, so the search ran on.

The callback now takes the uses of a document at once, so its text is read
once and the reports go out in order, and cancellation propagates. The
documents of a project go through a fixed set of workers instead of a task per
document parked on the throttle, and the throttle itself is one budget for the
whole editor, one core smaller than the machine, so the thread drawing the
results keeps one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Searching each file of a multi-targeted project once left two holes, and a
solution that uses conditional compilation widely fell through both: rename and
Find All References reported the same use once per target framework.

A secondary instance re-searched a file whenever it held any conditional
directive, so an inactive `#if DEBUG` was enough to search it again under every
target framework. Only the defines the two instances disagree on can make a
shared file parse differently, so the directives are now read for the idents
they test and the file is re-searched only when one of those defines is in that
difference. Instances whose defines match skip their shared files outright,
which also spares the checker the project builds those searches would force.

The uses themselves were never deduplicated on the F# side, so a file two
projects compile - the instances of one project file, or two project files
sharing a source file - reported its every use twice over. A range carries its
file, so the first project to report one keeps it.

The test project gained a use beside the `#if FOO` block and a file guarded by
a define both instances share: its only conditional use used to sit inside the
disabled branch, which is why the duplicates went unnoticed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Grouping the target-framework instances of a project file sorted each group to
read its head, though what follows the head is never ordered: every secondary
instance is searched the same way. One pass for the best-ranked instance says
that outright, and `Array.minBy` keeps the first of equal rank, which is what
the stable sort put at the head.

Sorting the whole solution once and letting the grouping keep that order reads
better still, but it measures worse - 86 instances over 26 project files, on
net472: 14.5 us and 16.1 KB for the pass per group against 17.2 us and 17.4 KB
for the single sort, and 15.0 us and 16.7 KB for the list-and-sort this
replaces. Neither number matters next to the project checks the grouping
schedules; the sort is dropped because it is not paid for.

`start` becomes `startSearching`, with the reason it is not awaited written
down, and the callback that collects the uses becomes a function rather than a
lambda bound to a name.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A file with no conditional directives of its own can still resolve
differently between two instances when an earlier file's conditional
compilation changes what a shared name means. Skipping such a file because
it itself tests no differing define left it unsearched, and Rename could
leave the other instance uncompilable.

Track the leading run of files both instances compile identically instead
of judging each file alone; the search starts at the first file that
diverges and covers everything after it.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@xperiandri
xperiandri force-pushed the perf/find-references-multitarget branch from b1e5b1b to dbae72c Compare September 14, 2026 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

⚠️ Affects-Design-Time Tooling check: PR touches type providers or dependency manager AI-reviewed PR reviewed by AI review council AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

2 participants