[Sonic] Support multiple C# documents in code actions - #84203
Conversation
This looked suspicious, so rather than fix it, I added some tests to prove that we don't need it any more, and removed it. Our code action handling is vastly improved since this was written, I'm guessing.
This is off topic, but I couldn't resist
This makes most of the tests pass, but I thought I'd tackle the edge cases in separate commits to make things easier
…nd abstract class still work as expected
There was a problem hiding this comment.
Pull request overview
This PR updates Razor cohosting code action plumbing to correctly handle scenarios where a Razor position can map to both generated C# documents (impl + optional decl), and adjusts tests accordingly so behavior is deterministic and exercised by default.
Changes:
- Request delegated C# code actions for both generated documents when the Razor position is valid in both, then dedupe appropriately when returning results.
- Update code action resolution/remapping/formatting paths to use the correct generated C# document (based on hint name / decl-vs-impl) and simplify some callers to operate on
IDocumentSnapshot. - Enable previously skipped cohosting code action tests and add additional Fully Qualify coverage; stabilize test
ProjectIdto avoid nondeterministic generated document ordering.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Razor/src/Razor/test/Microsoft.CodeAnalysis.Razor.CohostingShared.UnitTests/CodeActions/ImplementInterfaceTests.cs | Unskips tests and updates expected output ordering for implement-interface scenarios. |
| src/Razor/src/Razor/test/Microsoft.CodeAnalysis.Razor.CohostingShared.UnitTests/CodeActions/ImplementAbstractClassTests.cs | Unskips implement-abstract-class test coverage. |
| src/Razor/src/Razor/test/Microsoft.CodeAnalysis.Razor.CohostingShared.UnitTests/CodeActions/GenerateTypeTests.cs | Unskips generate-type tests. |
| src/Razor/src/Razor/test/Microsoft.CodeAnalysis.Razor.CohostingShared.UnitTests/CodeActions/GeneratePropertyTests.cs | Unskips generate-property tests. |
| src/Razor/src/Razor/test/Microsoft.CodeAnalysis.Razor.CohostingShared.UnitTests/CodeActions/GenerateMethodTests.cs | Unskips generate-method tests and adjusts a formatting expectation. |
| src/Razor/src/Razor/test/Microsoft.CodeAnalysis.Razor.CohostingShared.UnitTests/CodeActions/GenerateFieldTests.cs | Unskips generate-field tests. |
| src/Razor/src/Razor/test/Microsoft.CodeAnalysis.Razor.CohostingShared.UnitTests/CodeActions/GenerateEventHandlerTests.cs | Unskips generate-event-handler tests. |
| src/Razor/src/Razor/test/Microsoft.CodeAnalysis.Razor.CohostingShared.UnitTests/CodeActions/GenerateDeconstructMethodTests.cs | Unskips deconstruct-method tests. |
| src/Razor/src/Razor/test/Microsoft.CodeAnalysis.Razor.CohostingShared.UnitTests/CodeActions/GenerateConversionTests.cs | Unskips conversion-generation tests. |
| src/Razor/src/Razor/test/Microsoft.CodeAnalysis.Razor.CohostingShared.UnitTests/CodeActions/GenerateConstructorTests.cs | Unskips generate-constructor tests. |
| src/Razor/src/Razor/test/Microsoft.CodeAnalysis.Razor.CohostingShared.UnitTests/CodeActions/CSharpCodeActionTests.cs | Unskips miscellaneous Roslyn C# code action tests. |
| src/Razor/src/Razor/test/Microsoft.CodeAnalysis.Razor.CohostingShared.UnitTests/CodeActions/AddUsingTests.cs | Adds additional Fully Qualify scenarios and unskips related tests. |
| src/Razor/src/Razor/test/Microsoft.AspNetCore.Razor.Test.Common.Cohosting/CohostTestBase.cs | Stabilizes ProjectId so generated document IDs/order are deterministic for tests. |
| src/Razor/src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/Formatting/RazorFormattingService.cs | Adjusts code-action formatting API to accept IDocumentSnapshot directly. |
| src/Razor/src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/CodeActions/RoslynCodeActionHelpers.cs | Ensures a specific generated C# document is selected when needed. |
| src/Razor/src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/CodeActions/RemoteCodeActionsService.cs | Produces both impl and decl delegated C# requests when applicable and passes both through. |
| src/Razor/src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/CodeActions/Razor/GenerateEventHandlerCodeActionResolver.cs | Routes code-action-style formatting/remapping through the updated formatting API. |
| src/Razor/src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/CodeActions/Razor/ComponentAccessibilityCodeActionProvider.cs | Switches diagnostic source to a document-selection API compatible with decl/impl split. |
| src/Razor/src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/CodeActions/ICodeActionsService.cs | Expands the service contract to accept html/csharp/decl-csharp code action arrays. |
| src/Razor/src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/CodeActions/CSharp/TypeAccessibilityCodeActionProvider.cs | Updates filtering/wrapping behavior and optimizes diagnostic allowlist lookup. |
| src/Razor/src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/CodeActions/CSharp/CSharpCodeActionResolver.cs | Remaps edits using the correct generated C# document chosen by hint name and formats appropriately. |
| src/Razor/src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/CodeActions/CodeActionsService.cs | Filters/de-dupes delegated actions from both generated C# documents and integrates new call shape. |
| src/Razor/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Remote/IRemoteCodeActionsService.cs | Updates the remote service contract for the split delegated action arrays. |
| src/Razor/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Remote/CodeActionRequestInfo.cs | Extends request info payload with an optional decl-document delegated request. |
| src/Razor/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Protocol/LanguageServerConstants.cs | Removes the no-longer-used UnformattedRemap action constant. |
| src/Razor/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/IRazorFormattingService.cs | Updates formatting service interface to match snapshot-based code-action formatting. |
| src/Razor/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/DocumentMapping/IDocumentMappingServiceExtensions.cs | Small adjustment in GetPositionInfo flow for decl/impl mapping output. |
| src/Razor/src/Razor/src/Microsoft.CodeAnalysis.Razor.CohostingShared/CodeActions/CohostCodeActionsEndpoint.cs | Fetches delegated code actions for both generated C# documents when provided by request info. |
…s platforms and environments
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/Razor/src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/CodeActions/CSharp/TypeAccessibilityCodeActionProvider.cs:37
Microsoft.CodeAnalysis.Remote.Razortargetsnetstandard2.0(seesrc/Razor/src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/Microsoft.CodeAnalysis.Remote.Razor.csproj), soFrozenSet<T>isn't available here. This will fail to compile for thenetstandard2.0TFMs. Consider using aHashSet<string>with an ordinal ignore-case comparer (orImmutableHashSet<string>) instead.
private static readonly FrozenSet<string> s_supportedDiagnostics = FrozenSet.Create(StringComparer.OrdinalIgnoreCase, [
// `The type or namespace name 'type/namespace' could not be found
// (are you missing a using directive or an assembly reference?)`
// https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-messages/cs0246
"CS0246",
| continue; | ||
| } | ||
| else if (IsExplicitExpressionNode(owner)) | ||
| if (TryGetOwner(context, out var owner) && |
There was a problem hiding this comment.
Yep, this was deliberate on my part. My thinking was that previously we needed the the owner to filter out a number of scenarios because what we supported was quite narrow. Now we only care about one thing, and unless we're in that state, we let the normal system handle things, so if we can't get an owner, then we can't be in the one state we care about not offering for, so let it through.
In reality I suspect owner will never be null.
| ConvertCodeActionsToSumType(filteredCodeActions, "B-Delegated", csharpDocumentUri, seenTitles); | ||
| } | ||
|
|
||
| void ConvertCodeActionsToSumType(ImmutableArray<RazorVSInternalCodeAction> codeActions, string groupName, Uri? csharpDocumentUri = null, HashSet<string>? seenTitles = null) |
There was a problem hiding this comment.
We only de-dupe C# code actions, so for Razor and Html it's null (and not passed in, for brevity)
Review commit-at-a-time is recommended. Most of code actions was straight forward, except things like Generate Members, Implement Interface etc. which are dependent on which generated document the compiler sees first. Rather than assuming anything, I just made it request code actions for both documents if the mapping is valid for both. Also had to hard code the
ProjectIdwe use in tests, otherwise the location of the generated code would move around, so expected output validation would fail.While in here, I also found some Fully Qualify scenarios that were explicitly avoided, but they worked just fine. Probably have for a couple of years too :)
Microsoft Reviewers: Open in CodeFlow