[Interop] AddGeneratedComClassFixer: remove [ComVisible(true)] during conversion#126105
[Interop] AddGeneratedComClassFixer: remove [ComVisible(true)] during conversion#126105
Conversation
…rting to GeneratedComClass Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com> Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/9868b554-ef51-450a-8249-c7b0769c456a
|
@copilot please add another test where the class we will apply the fixer to has two partial definitions and the |
Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com> Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/4c11f33f-3060-459d-89a0-57e4a75b42cf
Added in b2e5714. The test |
There was a problem hiding this comment.
Pull request overview
Updates the ComInterfaceGenerator Roslyn code fix so converting a class to [GeneratedComClass] also removes redundant legacy [ComVisible(true)] attributes, reducing noise and aligning the produced code with the source-generated COM model.
Changes:
- Updated
AddGeneratedComClassFixerto find and remove[ComVisible(true)]during the conversion. - Made the fixer path async and added semantic checks using
SymbolEqualityComparer.Default. - Added unit tests validating removal/preservation behavior (including a same-file second partial declaration case).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/libraries/System.Runtime.InteropServices/gen/ComInterfaceGenerator/Analyzers/AddGeneratedComClassFixer.cs |
Removes [ComVisible(true)] while applying [GeneratedComClass] and partial. |
src/libraries/System.Runtime.InteropServices/tests/ComInterfaceGenerator.Unit.Tests/AddGeneratedComClassTests.cs |
Adds tests for [ComVisible(true)] removal, [ComVisible(false)] preservation, and partial declarations. |
| if (comVisibleAttr?.ApplicationSyntaxReference is { } syntaxRef) | ||
| { | ||
| var comVisibleAttrSyntax = await syntaxRef.GetSyntaxAsync(ct).ConfigureAwait(false); | ||
| editor.RemoveNode(comVisibleAttrSyntax); |
There was a problem hiding this comment.
comVisibleAttr.ApplicationSyntaxReference can point to an attribute on a different partial declaration in another document. In that case syntaxRef.GetSyntaxAsync(ct) returns a node from a different SyntaxTree, and DocumentEditor.RemoveNode will fail because the node is not part of the current document being edited. Please guard against cross-document syntax references (e.g., only remove when syntaxRef.SyntaxTree matches the current document) or rework the code action to edit the containing document/solution as needed.
| editor.RemoveNode(comVisibleAttrSyntax); | |
| if (comVisibleAttrSyntax.SyntaxTree == editor.OriginalRoot.SyntaxTree) | |
| { | |
| editor.RemoveNode(comVisibleAttrSyntax); | |
| } |
...tem.Runtime.InteropServices/gen/ComInterfaceGenerator/Analyzers/AddGeneratedComClassFixer.cs
Outdated
Show resolved
Hide resolved
....Runtime.InteropServices/tests/ComInterfaceGenerator.Unit.Tests/AddGeneratedComClassTests.cs
Show resolved
Hide resolved
…rue) removal Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/b1015873-0d8e-46e6-8159-57fa074fc344 Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Done in eeafcc6. The fix now:
|
The
AddGeneratedComClassFixercode fix was not removing[ComVisible(true)]when converting a class to use[GeneratedComClass], leaving a redundant legacy attribute alongside the new one. This includes cases where[ComVisible(true)]appears on a different partial declaration in a separate file.Description
ConvertToSourceGeneratedInteropFixer.cs: Changed the fix delegate type fromFunc<DocumentEditor, CancellationToken, Task>toFunc<SolutionEditor, DocumentId, CancellationToken, Task>throughout (base class,RegisterCodeFixesAsync,ApplyActionAndEnableUnsafe, andCustomFixAllProvider). This enables fixers to edit multiple documents in a single code action, which is necessary for cross-document attribute removal.AddGeneratedComClassFixer.cs: RewroteAddGeneratedComClassAsyncto useSolutionEditor. Now obtains theINamedTypeSymbolbefore applying edits, removes all[ComVisible(true)]attributes (not just the first) across any partial declarations, and usessolutionEditor.OriginalSolution.GetDocumentId(syntaxRef.SyntaxTree)to find which document each attribute lives in — safely handling cross-document, same-document, and metadata (null) cases.ConvertComImportToGeneratedComInterfaceFixer.csandConvertToLibraryImportFixer.cs: UpdatedCreateFixForSelectedOptionsto use the new solution-level signature; each implementation obtains itsDocumentEditorfrom theSolutionEditorinternally.AddGeneratedComClassTests.cs: Added four tests:TypeWithComVisibleTrue_RemovesComVisibleAttribute—[ComVisible(true)]is removed (same declaration)TypeWithComVisibleFalse_PreservesComVisibleAttribute—[ComVisible(false)]is preserved (different semantics)TypeWithComVisibleTrueOnSecondPartialDeclaration_RemovesComVisibleAttribute—[ComVisible(true)]on a second partial declaration in the same file is removedTypeWithComVisibleTrueInSeparateFile_RemovesComVisibleAttribute—[ComVisible(true)]on a partial declaration in a separate file is removed correctlyBefore fix, applying the codefix to:
would produce
[ComVisible(true)][GeneratedComClass] partial class C : I. After, it correctly produces[GeneratedComClass] partial class C : I— even when[ComVisible(true)]is on a different partial declaration in another file.Customer Impact
Users applying the "convert to GeneratedComClass" codefix are left with a stale
[ComVisible(true)]attribute that has no effect in the new model and causes unnecessary noise/confusion. This applies whether the attribute is on the same declaration or any other partial declaration of the class, including those in separate files.Regression
No — this is a missing feature in the codefix, not a regression.
Testing
8 unit tests in
ComInterfaceGenerator.Unit.Tests(including the 4 new ones), all passing. Full suite of 852 tests passes. LibraryImportGenerator unit tests (95 tests) also pass, confirming no regressions in the refactored base class.Risk
Low. Change is isolated to the Roslyn analyzer/fixer; only affects the code transformation applied by the IDE codefix. Runtime behavior is unchanged. The base class refactoring to
SolutionEditoris a pure internal restructuring — all existing fixer behavior is preserved.Original prompt
💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.