Let F# hand Find All References an already classified line - #85281
Open
xperiandri wants to merge 2 commits into
Open
xperiandri wants to merge 2 commits into
xperiandri wants to merge 2 commits into
Conversation
C# and VB classify the line of each reference while they search and pass it in SourceReferenceItem. FSharpSourceReferenceItem always passed null, so the window asked the F# classification service for every line again, one reference at a time, after the search. A new constructor takes the classified line and the reference's place in it; the existing one is unchanged. The EA test project references the C# workspace layer, because AdhocWorkspace can only create a document for a language whose services are loaded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
dotnet-policy-service
Bot
added
the
Community
|
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
Add the new constructor entries to InternalAPI.Unshipped.txt.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This pull request lets F# Find All References forward precomputed line classification.
Changes:
- Adds a classification-aware
FSharpSourceReferenceItemconstructor. - Adds tests for classified and legacy paths.
- Adds the C# Workspaces test dependency.
- Requires API baseline entries for the new overload.
File summaries
| File | Summary |
|---|---|
src/VisualStudio/ExternalAccess/Test/Microsoft.VisualStudio.LanguageServices.ExternalAccess.UnitTests.csproj |
Adds the C# Workspaces dependency. |
src/VisualStudio/ExternalAccess/Test/FSharp/FSharpSourceReferenceItemTests.cs |
Tests classification propagation and compatibility. |
src/VisualStudio/ExternalAccess/Core/FSharp/FindUsages/FSharpSourceReferenceItem.cs |
Adds the classification-aware constructor; API baseline entries are still required. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
| /// first non-whitespace character to its end, with every character covered.</param> | ||
| /// <param name="highlightSpan">The reference within that line, relative to the start of the first classified | ||
| /// span.</param> | ||
| public FSharpSourceReferenceItem(FSharpDefinitionItem definition, FSharpDocumentSpan sourceSpan, ImmutableArray<ClassifiedSpan> classifiedSpans, TextSpan highlightSpan) |
Contributor
Author
There was a problem hiding this comment.
Added the new constructor to InternalAPI.Unshipped.txt for both namespaces in 7f2afa3cf3, beside the entry of the existing constructor.
…API baseline Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Find All References lists each reference with its line classified. C# and VB classify that line while they search:
DefinitionItemFactoryandFindLiteralsProgressAdaptercallClassifiedSpansAndHighlightSpanFactory.ClassifyAsyncand hand the result toSourceReferenceItem.FSharpSourceReferenceItemhas no way to do the same, so it always passesclassifiedSpans: null, andAbstractTableDataSourceFindUsagesContext.ExcerptAsyncasks the F# classification service for the syntactic and semantic classification of every line again, one reference at a time, after the search.For a file that is not open, the F# language service answers the semantic part from a whole-file classification it keeps for a few seconds, and computes that classification again once it has expired. The F# search already has the checked file in hand when it reports its references, so it can classify every reference line of a file in one pass.
This adds a constructor to
FSharpSourceReferenceItemthat takes the classified line and the reference's position in it, the two parts ofClassifiedSpansAndHighlightSpan. The existing constructor is unchanged, so a language service built against the previous version still loads and its references are still classified by the window.FSharpSourceReferenceItemis a class, so the new constructor is an overload that old binaries never call.The test project references
Microsoft.CodeAnalysis.CSharp.Workspaces, as in #85280, soAdhocWorkspacecan create the document a reference points into.The F# side is dotnet/fsharp#20533. It cannot merge until this has flowed.
Verified with the F# tests of
Microsoft.VisualStudio.LanguageServices.ExternalAccess.UnitTests(182, including the two new ones). The ExternalAccess projects build withRunAnalyzersDuringBuild=truewithout warnings.🤖 Generated with Claude Code