Skip to content

Search a project's priority documents first in Navigate To - #20615

Open
xperiandri wants to merge 17 commits into
dotnet:mainfrom
xperiandri:fix/navigate-to-priority-documents
Open

Search a project's priority documents first in Navigate To#20615
xperiandri wants to merge 17 commits into
dotnet:mainfrom
xperiandri:fix/navigate-to-priority-documents

Conversation

@xperiandri

@xperiandri xperiandri commented Sep 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Roslyn passes SearchProjectAsync the documents the user has open as priorityDocuments, but the F# implementation ignored them and searched project.Documents in whatever order the project enumerates them, so a large project could spend its first throttle slots on files the user never opened. Priority documents are now searched first, in the same throttled pass as the rest, so a slow priority document holds nothing back.

Stacked on #20483: the diff shrinks to the last commit once #20483 merges.

🤖 Generated with Claude Code

@xperiandri
xperiandri requested a review from a team as a code owner September 22, 2026 11:20
@github-actions

github-actions Bot commented Sep 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

✅ Release-note check exempted

The NO_RELEASE_NOTES label exempts this pull request.

@github-actions github-actions Bot added the ⚠️ Affects-Design-Time Tooling check: PR touches type providers or dependency manager label Sep 22, 2026
@github-actions

This comment has been minimized.

Sign in to view
@xperiandri
xperiandri force-pushed the fix/navigate-to-priority-documents branch from 6ca4511 to 2419dc1 Compare September 22, 2026 11:34
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
T-Gro
T-Gro reviewed Sep 25, 2026
View reviewed changes

@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.

🤖 🕵️ AI review — verify independently.

Comment thread vsintegration/src/FSharp.Editor/Navigation/NavigateToSearchService.fs Outdated
Comment thread vsintegration/src/FSharp.Editor/Navigation/NavigateToSearchService.fs Outdated
@T-Gro T-Gro added the AI-reviewed PR reviewed by AI review council label Sep 25, 2026
@T-Gro
T-Gro self-requested a review September 25, 2026 08:54
Copilot AI lite review requested due to automatic review settings September 26, 2026 00:18
@xperiandri
xperiandri force-pushed the fix/navigate-to-priority-documents branch from 2419dc1 to f56537e Compare September 26, 2026 00:18
@xperiandri xperiandri mentioned this pull request Oct 1, 2026
Open
T-Gro
T-Gro reviewed Oct 1, 2026
View reviewed changes

@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.

🤖🕵️ AI review — verify independently.

Comment thread vsintegration/tests/FSharp.Editor.Tests/NavigateToSearchServiceTests.fs Outdated
@xperiandri
xperiandri force-pushed the fix/navigate-to-priority-documents branch from 89feb63 to 440bcf5 Compare October 2, 2026 16:03
T-Gro
T-Gro reviewed Oct 3, 2026
View reviewed changes

@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.

🤖 🕵️ Re-reviewed all recent changes as of 440bcf5; LGTM

@xperiandri
xperiandri force-pushed the fix/navigate-to-priority-documents branch from 440bcf5 to c3888df Compare October 8, 2026 01:11
T-Gro
T-Gro reviewed Oct 8, 2026
View reviewed changes

@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.

🤖 🕵️ Re-reviewed all recent changes as of c3888df; 1 finding

@xperiandri
xperiandri force-pushed the fix/navigate-to-priority-documents branch from c3888df to 9ab11c7 Compare October 10, 2026 22:28
xperiandri added a commit to xperiandri/fsharp that referenced this pull request Oct 11, 2026
@xperiandri @claude
Merge the parse-sharing fixes of dotnet#20483 into the priority docum…
dad2333 
…ents branch

Brings in the document-local reuse for standalone scripts and the release
note cleanup. The release notes hold one bullet for dotnet#20483, the parse
sharing, and one for dotnet#20615.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
T-Gro
T-Gro requested changes Oct 13, 2026
View reviewed changes

@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.

The diff is still way too big - apply the NoBloat instructions and CodeCompaction skill.

Especially the massive code comments would benefit not just with drastic reduction, but ideally with well named functions/arguments/bindings/types making the intention clear.

If a well named piece of code just tells what the comment says - delete the comment.

  • This diff adds 60 new code comment lines and 5375 new code comment chars.
  • NavigateToSearchService.fs adds 28 comment lines around roughly 100 code lines; more of that cache policy should be carried by named types, functions, and bindings.

@github-project-automation github-project-automation Bot moved this from New to In Progress in F# Compiler and Tooling Oct 13, 2026
@T-Gro T-Gro added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label Oct 14, 2026
xperiandri and others added 17 commits October 15, 2026 01:26
@xperiandri @claude
Build multi-project and multi-target Roslyn solutions in FSharp.Edito…
bf970ad 
…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>
@xperiandri @claude
Search each file of a multi-targeted project once in NavigateTo and r…
2e06071 
…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>
@xperiandri @claude
Test NavigateTo across the target-framework instances of a project
55e6038 
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@xperiandri @claude
Add the release note for the NavigateTo multi-target fix
cc25922 
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@xperiandri @claude
Share the parse instead of skipping the file
b4ef42f 
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>
@xperiandri @claude
Name the navigable items cache's key and entry
73fd328 
The cache was a dictionary from a struct tuple of two strings to an anonymous
struct record, spelled out in the type arguments at the one place it is
declared, and read back as `struct (key, path)` wherever it is used. Nothing at
those sites said which string was the defines and which the path.

Give both a name at the top of the file. They stay struct records, so the key
keeps the same equality and the entries allocate no more than before.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@xperiandri @claude
Format the multi-target NavigateTo tests
8f83b17 
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@xperiandri @claude
Build the synthetic projects' id list through Seq and materialize once
9856ebf 
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@xperiandri @claude
Key a kept parse by everything it depends on
5cd23d7 
The defines a file was parsed with are not the whole of what its tree depends on: the
language version decides what the parser accepts, and the defines were taken from the
editing defaults when the project had not produced its options yet - a set another
project really has, so its parse answered for a project that never asked for it, with
the branch of a `#if` that project chose.

The key now carries the language version, and both it and the defines are read only
where the project has produced them: a project without options parses for itself
instead of reading an entry that is not about it. The rest of the parsing options
cannot differ for one path - the editor never applies line directives, and whether a
file is interactive follows from its extension.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@xperiandri @claude
Take the shared test fixture as dotnet#20462 lands it
f381a3d 
The multi-project and multi-target helpers now match the ones dotnet#20462 adds,
struct results included, so this branch rebases onto it without reconciling
two copies. A struct tuple cannot be bound by a `let private` pattern, so the
module-level fixture bindings lose the modifier.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@xperiandri @claude
Keep the shared fixture's results private
e643efe 
A struct tuple cannot be bound with `let private` (FS0010: the grammar takes an
access modifier only before a name), so the binding sits in a private AutoOpen
module: the tests here see both names, nothing outside does.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@xperiandri @claude
Keep a document's own parse where its project has no options
462822d 
A standalone script has its options kept per document, so its project never
produces the defines and language version a parse is keyed by, and every
search of the unchanged script asked for the parse again. Such a document's
navigable items are now kept under its DocumentId with the text version.
Nothing is shared across projects, and a project whose first parse makes it
produce its options is still keyed by them from the next search on.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@xperiandri @claude
Drop the release note of the approach the parse sharing replaced
a118991 
The rebase onto main brought the bullet back beside the one that describes
what the PR does now.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@xperiandri @claude
Key Navigate To's parse cache by the options the parse uses
b868c62 
The cache key is built from the parsing options the document's parse is made
with: the ones the options reactor already holds, else the awaited ones. That
removes the per-document cache, the cold-project branch, the sentinel define
set and the forwarding members for the options peek, and lets the key say in
its type whether a parse depends on the defines.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@xperiandri @claude
Compact the Navigate To tests
f3c95e4 
One search helper for the three that repeated it, the unused synthetic files
and excluded-file data gone, the edit test run in both search orders, and the
comments that restated the test names replaced by those names.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@xperiandri @claude
Give the instances of a file one text version in the multi-target tests
502ab34 
Documents that AdhocWorkspace.AddSolution loads each carry their own version,
so no instance could read another's parse and the tests passed on main. They
now share a version per file, as the projects of a loaded solution do, and a
test counts the parse requests across the instances.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@xperiandri @claude
Search a project's priority documents first in Navigate To
c144a65 
Roslyn passes the documents the user has open as priority documents. The
project's documents are ordered by that, priority ones first, and go through
the one throttled pass, so a slow priority document holds nothing back.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@xperiandri
xperiandri force-pushed the fix/navigate-to-priority-documents branch from da23e29 to c144a65 Compare October 15, 2026 14:13
@xperiandri

xperiandri commented Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

Compacted along with #20483: +461/−48 → +296/−30, added comment lines 60 → 1.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@T-Gro T-Gro T-Gro requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

⚠️ Affects-Compiler-Output Tooling check: PR touches IL emission or codegen ⚠️ Affects-Design-Time Tooling check: PR touches type providers or dependency manager AI-reviewed PR reviewed by AI review council NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes

Projects

F# Compiler and Tooling
Status: In Progress

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@xperiandri @T-Gro