Offer F# declarations to the Copilot chat "#" mention picker - #20409
Offer F# declarations to the Copilot chat "#" mention picker#20409xperiandri wants to merge 15 commits into
Conversation
❗ Release notes requiredYou can open this PR in browser to add release notes: open in github.dev
|
|
github-actions
Bot
added
⚠️ Affects-Build-Infra
This comment has been minimized.
This comment has been minimized.
db075ee to
80d555b
Compare
T-Gro
left a comment
T-Gro
left a comment
There was a problem hiding this comment.
Really nice piece of work. The design is clean, the comments explain the why behind every non-obvious choice, and it fills a real gap — F# projects have no Roslyn Compilation, so Copilot's built-in provider never saw F# symbols.
What stood out as excellent
- Separation of concerns.
CopilotSymbolQueryholds all the lookup logic and takes aSolutiondirectly, so it's unit-tested with no VS workspace, whileFSharpCopilotContextProviderstays a thin brokered-service adapter. That split is exactly why the tests read so well. - No project-wide typecheck. Reusing the NavigateTo parse-tree cache (now cleanly extracted as
FSharpNavigableItemsCache, MEF-Sharedso both consumers share one instance) keeps the picker responsive per keystroke. - Throttling via
whenAllThrottled ProcessorCountmirrorsFindReferencesAsync, so a query doesn't launch a parse-per-document storm. - Exhaustive mappings.
symbolContextType/imageIdcover all 11NavigableItemKindcases — no partial-match surprises. - The cache extraction preserves the backtick/operator substring-match fallback verbatim; the
struct-tuple change on that per-keystroke path and thenull→matchconversion are tidy. - Release note added; tests cover search, dedup, snippet extent, doc-comment inclusion, kind mapping, and the snippet-location round-trip.
Suggestions (none blocking)
-
Exception safety at package load (
LanguageService.fs). The registration task handles anullproxy (Copilot absent), but only that. IfGetProxyAsync/RegisterContextProviderAsyncthrows — e.g. a Copilot contract-version mismatch, given you're compile-pinned to18.9.918but bind-redirect to whatever VS ships — the exception escapes theafterPackageLoadedTaskstask. Worth confirmingAddTask(false, …)isolates a faulting task, or wrapping the body intry/withso a Copilot hiccup can't perturb F# package load. -
Batch query is sequential, O(all docs) per query (
QueryMentionBatchAsync).for query in queries do let! … = queryMentions queryruns one full-solution scan per query, serially. Batches are usually tiny so it's fine in practice, but if Copilot ever sends several, they could be de-duplicated or run through the same throttle rather than back-to-back. -
One-line declarations drop their doc comment. In
definitionLines, a construct with no body scope (/// doc+let x = 1) falls through todeclarationLine, item.Range.EndLine, so its doc comment isn't captured — unlike the multi-line path, which deliberately reaches back over the doc comment. Minor; a follow-up could widen the one-line case to include an immediately-preceding///block. -
Nit: the test's hardcoded
"C:\\test.fs"is fine for Windows-only VS tests but couples toRoslynTestHelpers' internal path.
I reviewed statically and confirmed the in-tree helpers (whenAllThrottled, chooseV/tryHeadV/toImmutableArray, ValueOption.ofNullable) and the NavigableItemKind shape; I didn't run a VS-hosted build, so the Microsoft.VisualStudio.Copilot contract surface is taken on faith from the package reference.
Only item 1 feels worth a second look before merge. Thanks for this — it's going to be a delightful quality-of-life win for F# users in the Copilot picker. 🎉
80d555b to
28f13e7
Compare
github-project-automation
Bot
moved this from New
to In Progress
in F# Compiler and Tooling
T-Gro
left a comment
T-Gro
left a comment
There was a problem hiding this comment.
🤖🕵️ AI review — verify independently.
| |> Seq.collect id | ||
| |> Seq.sortBy (fun (struct (kind, item: NavigableItem, document: Document)) -> | ||
| document.IsFSharpSignatureFile, kind, item.Name.Length) | ||
| |> Seq.distinctBy (fun (struct (_, item, _)) -> CopilotSymbolMapping.fullyQualifiedName item) |
There was a problem hiding this comment.
🤖🕵️ Both declarations become M.a.b before broker escaping:
module M =
let ``a.b`` = 1
module a =
let b = 2Preserve F# name segments in the mention input.
There was a problem hiding this comment.
Fixed in 254d6ac. A declaration's own name now keeps its double backticks in the mention input whenever NavigateTo flags it NeedsBackticks, so the two here are M.``a.b`` and M.a.b. The last segment of the enclosing container is spelled the same way (M.``x.y``.z vs M.x.y.z). hasFullyQualifiedName reads that spelling without building the string, and the tests cover both pairs: each resolves to its own declaration and shows up as its own mention.
Two cases stay joined, because FCS hands them over that way: a dotted name in a container further out (of a container's path only NavigableContainer.Name, the last segment, comes apart from FullName), and a file's top-level module, which NavigateTo names by its whole dotted path. Closing those needs NavigateTo to expose the container's segments.
| <ItemGroup> | ||
| <!-- FSharp.Compiler.Service's PackageReference to System.Memory (for ReadOnlySpan/ReadOnlyMemory) is only | ||
| transitive here, so the SetTargetFramework override above does not carry it in on net472; pin it directly. --> | ||
| <PackageReference Include="System.Memory" /> |
There was a problem hiding this comment.
🤖🕵️ The copied #20443 snapshot fails restore with NU1510 and includes an unrelated public FCS API break. Revert those copied outlining commits; keep this PR on its original string[] path.
There was a problem hiding this comment.
Taken back out in 5c30ebf. The copies of #20443 ("Slice source text instead of copying it line by line for outlining", "Track comment lines by number instead of storing their text") are reverted, so the branch no longer touches src/, the FCS tests, the surface-area baseline or System.Memory: everything outside the Copilot files matches main. The Copilot snippets are back on the string[] lines Structure.getOutliningRanges takes there.
Copilot's built-in symbol provider reads symbols off the Roslyn compilation, which F# projects do not have, so F# declarations never appeared in the picker shown for "#". Proffer a brokered service from FSharp.Editor implementing Copilot's context-provider and mention-queryable contracts. Declarations come from the NavigateTo parse-tree cache, so the picker answers without waiting for a project check; that cache moves into a shared FSharpNavigableItemsCache used by both features. A picked mention resolves by fully qualified name against the current solution, so it survives a file moving, and carries the whole declaration - doc comment included - as its snippet. FSharpPackage now registers the provider moniker with Copilot after package load. The override is no longer DEBUG-only, so it calls its base implementation, which registers the editor factories. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e path Sequential per-document scanning made "search" and "declarationsOf" as slow as the slowest single file; run them across documents concurrently instead, throttled the same way FindReferencesAsync throttles its per-document typechecks, so a solution-wide scan does not launch a parse per document all at once. FSharpNavigableItemsCache's version-stamp entries move to struct tuples and its null workspace check to a match, matching this repo's allocation and null-narrowing conventions on a path every keystroke in the mention picker hits. CopilotSymbolMapping collapses its wrapping module into a single qualified top-level module declaration. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Package load runs its tasks back to back on a single loop, so an exception from the Copilot registration task escaped into F# package load. A Copilot contract version the installed build does not serve would have taken the whole package down; catch and log instead, leaving cancellation alone. A doc comment is only reported as an outlining scope once it spans several lines, so a one-line "///" in front of a declaration was invisible to the scope search and dropped from the snippet. Walk back over the preceding "///" lines directly. Batch mention queries scanned the solution once per query, serially. Distinct search texts now scan concurrently and repeated ones share a single scan. The snippet-location test asserted a hardcoded "C:\test.fs" rather than asking the solution where its document lives. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Resolving a picked mention walks every declaration in every document of the solution, and asked each one for its dotted path as a fresh string purely to compare it. Compare against the container and name in place instead, so the scan allocates nothing per declaration. The doc-comment probe trimmed each candidate line into a new string for the same reason. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Every consumer of Structure.getOutliningRanges built its sourceLines array by calling ToString() per line, allocating a fresh string for the entire file on every outlining pass - once per keystroke for the editor's block structure, and once per resolved Copilot mention for the snippet extent. getOutliningRanges now takes ReadOnlyMemory<char>[] and slices the already-materialized source text once (SourceText.GetLinesAsMemory()) instead. ReadOnlySpanCharExtensions in illib mirrors the existing Ordinal string helpers so span call sites read the same way string call sites do. A local recursive function closing over a ReadOnlySpan<char>-typed sibling cannot be compiled - the CLR disallows instantiating FSharpFunc<ReadOnlySpan<char>, _> as a closure field (FS0412) - so commentTypeOf moves to module scope, next to the CommentType it classifies. StructureTests.fs slices its own lines the same way at the call site, and FSharp.Compiler.Service.Tests needs a direct System.Memory PackageReference: FSharp.Compiler.Service's own reference to it is only transitive through the net472 ProjectReference's SetTargetFramework override, mirroring the FSharp.Core pin already in this project for the same reason. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
CommentList kept a copy of every comment line next to its line number, but the number alone identifies the line in the source array the function already holds, and only the first and last lines of a group are ever read back to compute the fold's columns. Store the numbers and index the source at the end, so grouping comments allocates no tuple per line. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
GetProxyAsync<ICopilotRegistrationService> is an exported brokered service, so calling it from a background package-load task constructs Copilot's MEF part graph on that thread. Its constructor does a blocking JoinableTask wait for the main thread; meanwhile the Git provider asks for the same proxy from the main thread while building its own services at solution open, and blocks inside MEF's PartLifecycleTracker waiting for the part the background thread owns. Neither side can proceed and Visual Studio hangs permanently. Move the registration out of the background package-load task and into LoadComponentsInBackgroundAfterSolutionFullyLoadedAsync (run after the solution is fully loaded, the way Roslyn's AbstractPackage defers this kind of work), and switch to the main thread before asking for the proxy so the two requesters serialise instead of deadlocking.
Diagnostic aid: on a large solution the "#" mention picker stays empty and nothing in the Debug pane says why. Log each step of RegisterCopilotContextProviderAsync so a hang or an early return (no brokered service container, a null proxy) is visible without a debugger attached.
The picker showed no F# declarations on large solutions. Every query walked every document of every F# project, parsing the ones nobody had opened, and `whenAllThrottled` queued a task per document on one semaphore, so a thousand documents meant a thousand tasks waiting to run while Copilot cancelled the query and took nothing. A query now visits the documents in three groups, stopping as soon as it holds as many declarations as it reports: the documents the user has open, the ones already in the parse cache, and only then the ones that would have to be parsed, which get a time budget of their own. The new cache lookup reads no text, so a closed document costs nothing. `forEachThrottled` pulls documents through a fixed set of workers instead of starting a task per document, and a batch of search texts visits each document once for all of them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A declaration in a file the user has open now carries a High priority into the picker, which merges answers from every provider and cannot infer that from their order. The obvious source for what the user is looking at, the shell's current document frame, is not reachable here: importing SVsServiceProvider by contract name pins the required type identity to System.IServiceProvider, which nothing exports, so the whole brokered service failed to compose and the picker held no F# symbols at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot asks for mentions while the user is still typing, before it has resolved what kind of mention that is, and the query then carries CopilotMentionType.Unknown. The provider only answered Context, so every early query - the ones that fill the list as the user types - went back empty, and no F# declaration reached the picker at all. Copilot's own symbol provider answers Unknown and Context alike. The priorities now match what that provider reports for C#: the file whose editor has focus is High, a file that is merely open is Low, anything else None. A tracker updated from GotAggregateFocus supplies the focused path, which the shell's own current-document frame cannot: reaching it needs SVsServiceProvider, which this composition does not export. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three gaps against Copilot's own symbol provider, which answers the picker
for C# (SymbolContextProvider in Microsoft.VisualStudio.Copilot.Core):
A bare "#" arrives as a query whose only input is empty. The provider read
that as no query at all and answered nothing, so the list the user first
sees never held an F# declaration. It now answers with every declaration
of the open files, and a text shorter than three characters is looked up
in the open files alone - what Copilot does, and what keeps the first
keystrokes from parsing the solution.
Copilot ranks a declaration Selection when its whole extent holds the
caret or overlaps the selection in the focused file. The focus tracker now
records the lines the caret or selection covers, and the search outlines
the focused file once to find the declarations around them. The caret in a
member's body selects the member, its type and the modules around it. The
extent is the full declaration, not the snippet: the snippet stops at 200
lines, which would miss a caret deep inside a large module.
The picker shows a mention's description beside it, and Copilot puts the
file name there; the provider put the container and the project. The
description is now the file name, and the tooltip follows Copilot's
"{0} in {1}\n{2}" - kind, file, and a member named by its container - from
a localizable resource.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
432ff25 to
540463c
Compare
github-actions
Bot
added
⚠️ Affects-Design-Time
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Reverts the copies of dotnet#20443 that came in with an earlier rebase: "Slice source text instead of copying it line by line for outlining" and "Track comment lines by number instead of storing their text". They changed the public FSharp.Compiler.Service surface (Structure.getOutliningRanges took ReadOnlyMemory<char>[]) and added a System.Memory reference that fails restore with NU1510 - neither belongs here, and dotnet#20443 carries that work on its own. The Copilot snippets go back to the string[] lines Structure.getOutliningRanges takes on main. Everything outside the Copilot files is now as on main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The mention input named a declaration by its container path and name joined with dots, so a value ``a.b`` in module M and the value b of M's nested module a were both "M.a.b": the picker showed one of them and resolving the mention gathered both. A name NavigateTo reports as needing backticks now keeps them, and so does the last segment of the container around it - the one segment of the path FCS hands over apart from the rest. hasFullyQualifiedName reads the same spelling without building the string. Still ambiguous: a dot in the name of a container further out, and in a file's top-level module, which NavigateTo names by its whole dotted path. Both reach the editor already joined. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
🔍 Tooling Safety Check — Affects-Agent-Config, Affects-Build-Infra, Affects-Design-Time, Affects-Restore
|
2 participants
Description
GitHub Copilot Chat's
#mention picker in Visual Studio lets you attach a symbol as context. Copilot's built-in provider reads symbols straight off the RoslynCompilation, which F# projects do not have, so F# types, modules, members and values never showed up there.This adds
FSharpCopilotContextProvider, a brokered service proffered fromFSharp.Editorthat implements Copilot'sICopilotContextProvider/ICopilotMentionQueryable/ICopilotMentionBatchQueryablecontracts directly, backed by the existing NavigateTo parse-tree cache (no project-wide typecheck needed, so the picker answers as fast as you type). A picked mention resolves by fully qualified name against the current solution and attaches the whole declaration — doc comment included — as its snippet.Solution-wide document scanning in
search/declarationsOfruns across documents concurrently, throttled the same wayFindReferencesAsyncthrottles its per-document typechecks, so a query does not launch a parse per document all at once.FSharpNavigableItemsCache's per-document cache entries move to struct tuples on this hot, per-keystroke path, and its null-workspace check moves to amatchper this repo's conventions.Checklist