Let Peek Definition ask another language for a metadata symbol's source - #85257
Open
xperiandri wants to merge 2 commits into
Open
Let Peek Definition ask another language for a metadata symbol's source#85257xperiandri wants to merge 2 commits into
xperiandri wants to merge 2 commits into
Conversation
|
Azure Pipelines: Successfully started running 2 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Address the two moderate findings in PeekableItemFactory.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR enables Peek Definition to show cross-language source, such as F# declarations, instead of metadata-as-source.
Changes:
- Adds cross-language file-location navigation.
- Updates Peek Definition fallback behavior.
- Adds versioned F# adapters, type forwarding, and API baselines.
File summaries
| File | Summary |
|---|---|
src/VisualStudio/ExternalAccess/FSharp/TypeForwards.cs |
Forwards the new F# interface. |
src/VisualStudio/ExternalAccess/FSharp/InternalAPI.Unshipped.txt |
Records F# API additions. |
src/VisualStudio/ExternalAccess/Core/InternalAPI.Unshipped.txt |
Records shared API additions. |
src/VisualStudio/ExternalAccess/Core/FSharp/Navigation/IFSharpCrossLanguageSymbolNavigationService.cs |
Adds the optional versioned F# contract. |
src/VisualStudio/ExternalAccess/Core/FSharp/Internal/Navigation/FSharpCrossLanguageSymbolNavigationService.cs |
Adapts the optional F# implementation. |
src/Features/Core/Portable/Navigation/ICrossLanguageSymbolNavigationService.cs |
Adds the file-location navigation contract. |
src/EditorFeatures/Core/Peek/PeekableItemFactory.cs |
Queries cross-language services before metadata fallback. Findings: moderate (3 votes), add regression coverage; moderate (1 vote), normalize to OriginalDefinition before generating the documentation ID. |
Review details
Suppressed comments (1)
src/EditorFeatures/Core/Peek/PeekableItemFactory.cs:115
- This lookup uses the constructed symbol's documentation ID, while Go To Definition first normalizes to
symbol.OriginalDefinition(VisualStudioSymbolNavigationService.cs:54). For a metadata member/type instantiated with concrete generic arguments,GetDocumentationCommentId()can include substituted types, so the F# service may not recognize the ID and Peek falls back to decompiled C# even though Go To Definition succeeds. Normalize tosymbol.OriginalDefinitionbefore generating the ID.
var docCommentId = symbol.GetDocumentationCommentId();
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
xperiandri
force-pushed
the
fsharp-peek-cross-language
branch
from
September 11, 2026 11:23
a0321a3 to
d056285
Compare
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Add adapter-level coverage for the v2 implementation and legacy-interface fallback.
Review details
Suppressed comments (1)
src/VisualStudio/ExternalAccess/Core/FSharp/Internal/Navigation/FSharpCrossLanguageSymbolNavigationService.cs:58
- The added Peek tests export a test
ICrossLanguageSymbolNavigationServicedirectly, so they bypass this adapter'sIFSharpCrossLanguageSymbolNavigationService2cast and forwarding. A regression in the external-access version detection would leave F# Peek broken while all current tests still pass; add an adapter-level test covering a v2 implementation and the old-interface-only fallback.
if (_underlyingService is not IFSharpCrossLanguageSymbolNavigationService2 fileLocationService)
return null;
return await fileLocationService.TryGetNavigableFileLocationAsync(
assemblyName, documentationCommentId, cancellationToken).ConfigureAwait(false);
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
xperiandri
force-pushed
the
fsharp-peek-cross-language
branch
from
September 11, 2026 17:17
d056285 to
2530cf6
Compare
Go To Definition on a symbol Roslyn sees as metadata asks the registered ICrossLanguageSymbolNavigationService exports first, so F# can navigate to its own source. Peek Definition never did: PeekableItemFactory only consulted GetExternalNavigationSymbolLocationAsync, the RQName/IVsSymbolicNavigationNotify path, and then generated metadata-as-source. Peeking an F# symbol from C# or Visual Basic therefore showed decompiled C# while F12 on the same symbol opened the F# file. Peek shows a file in place rather than navigating, so it cannot use the INavigableLocation the existing member returns. Add a sibling that returns the file and position, and have PeekableItemFactory ask it before falling back to metadata-as-source, in the order VisualStudioSymbolNavigationService already uses: source location, then navigable metadata symbol, then the other language. F# implements the member through IFSharpCrossLanguageSymbolNavigationService2, derived from the existing interface rather than added to it, so a FSharp.Editor built against a version without it still composes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Covers the file another language hands back, including for a member of a constructed generic type, and the paths that must not reach it: a symbol in source, a metadata symbol metadata-as-source cannot show, and a location the external navigation service already found. With no answer, Peek still shows metadata-as-source. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
xperiandri
force-pushed
the
fsharp-peek-cross-language
branch
from
September 11, 2026 20:43
2530cf6 to
7fcb389
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Peek Definition on a symbol Roslyn sees as metadata generates metadata-as-source for it, even when
another language owns the source. For F# that means peeking an F# symbol from C# or Visual Basic shows
decompiled C#, while Go To Definition on the same symbol opens the F# file.
The two commands take different routes. Go To Definition reaches
VisualStudioSymbolNavigationService.GetNavigableLocationAsync, which asks every exportedICrossLanguageSymbolNavigationServicebefore falling back to metadata-as-source ("See if there'sanother .Net language service that can handle navigating to this metadata symbol (for example, F#)").
Peek goes through
PeekableItemFactory.GetPeekableItemsAsync, which only consultsISymbolNavigationService.GetExternalNavigationSymbolLocationAsync— the RQName andIVsSymbolicNavigationNotifypath — and otherwise builds aDefinitionPeekableItemovermetadata-as-source.
ICrossLanguageSymbolNavigationServiceis never asked; that one call inVisualStudioSymbolNavigationServiceis its only consumer.Peek cannot use the existing member: it shows a file in place, so it needs a file and a position —
ExternalFilePeekableItemtakes aFileLinePositionSpan— whileTryGetNavigableLocationAsynchandsback an
INavigableLocationthat can only navigate.This adds that member and has Peek ask it.
ICrossLanguageSymbolNavigationService.TryGetNavigableFileLocationAsyncreturns(string filePath, LinePosition linePosition)?for the same assembly name and documentation commentid — the shape
GetExternalNavigationSymbolLocationAsyncalready returns, so its result drops intothe existing
ExternalFilePeekableItembranch.PeekableItemFactoryasks the cross-language services when the external navigation location comesback empty, in the order
VisualStudioSymbolNavigationServiceuses: a symbol with a source locationand one metadata-as-source cannot navigate to are left alone, and only a navigable metadata symbol is
offered to the other language before falling back.
IFSharpCrossLanguageSymbolNavigationService2derives fromIFSharpCrossLanguageSymbolNavigationServiceand carries the new member; the F# adapter implements it by casting the imported service, and returns
nullwhen the F# language service does not implement it. As withIFSharpAdvancedNavigateToSearchServicein Let F# take part in the Navigate To search that runs while the solution loads #85213, it is a separate optional interface rather than a member on the existing one: the
implementation lives outside this repository and Roslyn reaches Visual Studio ahead of it, so a
member added to the existing interface would fail type load during MEF composition for an F#
language service built against the previous version. The
2suffix followsFSharpNavigationOptions2in the same file.
ICrossLanguageSymbolNavigationServiceis internal and its only implementation is the F# adapter, soadding the member there changes nothing else. The type forward and the
InternalAPI.Unshipped.txtentries for both external access assemblies are added alongside.
The consumer is dotnet/fsharp#20520, which cannot merge until this has flowed. It is
written and its tests pass against a local build of this branch.
PeekTestscomposes a testICrossLanguageSymbolNavigationServicethat owns a referenced metadataassembly. Peek shows the file it hands back for a type, a method, and a member of a constructed generic
type, which the service is asked about by its definition's id (
M:Box`1.Set(`0)). The other tests pinwhat must not change: a symbol in source and a metadata symbol metadata-as-source cannot show are not
offered to the other language, a location
GetExternalNavigationSymbolLocationAsyncfound wins, andwith no answer Peek still shows metadata-as-source. Without the new branch in
PeekableItemFactory, thefour tests that rely on it fail.
This branch builds with
-p:RunAnalyzersDuringBuild=truewithout warnings. Both halves were deployedto the experimental hive against a mixed C#/F# solution: Peek Definition from C# on an F# symbol now
shows the F# declaration in place, and Go To Definition is unchanged.
🤖 Generated with Claude Code