Strip partial modifier from generated COM interface method stubs#126719
Strip partial modifier from generated COM interface method stubs#126719
Conversation
…d VtableIndexStubGenerator The ComInterfaceGenerator and VtableIndexStubGenerator were copying the partial modifier from user-declared interface methods to the generated stub code, producing invalid C#. This change strips the partial keyword from method modifiers when creating the method syntax template, similar to how the new keyword and accessibility modifiers are already stripped. Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/de757761-5c54-412d-9875-7403edd00714 Co-authored-by: jtschuster <36744439+jtschuster@users.noreply.github.com>
src/libraries/System.Runtime.InteropServices/tests/ComInterfaceGenerator.Unit.Tests/Compiles.cs
Show resolved
Hide resolved
src/libraries/System.Runtime.InteropServices/tests/ComInterfaceGenerator.Unit.Tests/Compiles.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Jackson Schuster <36744439+jtschuster@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the COM interface source generators to avoid emitting invalid C# when user-declared COM interface methods include the partial modifier, by stripping partial from copied method modifiers and adding a regression compilation test.
Changes:
- Strip
SyntaxKind.PartialKeywordfrom copied method modifiers inComInterfaceGeneratorandVtableIndexStubGenerator. - Add a regression test ensuring partial method modifiers on
[GeneratedComInterface]methods don’t break generated code compilation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/libraries/System.Runtime.InteropServices/tests/ComInterfaceGenerator.Unit.Tests/Compiles.cs | Adds a new compilation regression test for partial method modifiers on COM interface methods. |
| src/libraries/System.Runtime.InteropServices/gen/ComInterfaceGenerator/VtableIndexStubGenerator.cs | Filters out partial from method modifiers when constructing the syntax template for generated stubs. |
| src/libraries/System.Runtime.InteropServices/gen/ComInterfaceGenerator/ComInterfaceGenerator.cs | Filters out partial (alongside existing new filtering) from method modifiers used in generated stubs. |
| internal virtual partial interface IComInterface | ||
| { | ||
| void Method(); | ||
| public partial void PartialMethod(); | ||
| } | ||
| internal virtual partial interface IComInterface |
There was a problem hiding this comment.
virtual is not a valid modifier on an interface declaration, so this test source will fail to compile before the generator runs. Remove virtual from both IComInterface declarations (keep partial).
| internal virtual partial interface IComInterface | |
| { | |
| void Method(); | |
| public partial void PartialMethod(); | |
| } | |
| internal virtual partial interface IComInterface | |
| internal partial interface IComInterface | |
| { | |
| void Method(); | |
| public partial void PartialMethod(); | |
| } | |
| internal partial interface IComInterface |
src/libraries/System.Runtime.InteropServices/tests/ComInterfaceGenerator.Unit.Tests/Compiles.cs
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
src/libraries/System.Runtime.InteropServices/tests/ComInterfaceGenerator.Unit.Tests/Compiles.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Jackson Schuster <36744439+jtschuster@users.noreply.github.com>
| var containingSyntaxContext = new ContainingSyntaxContext(syntax); | ||
|
|
||
| var methodSyntaxTemplate = new ContainingSyntax(syntax.Modifiers.StripAccessibilityModifiers(), SyntaxKind.MethodDeclaration, syntax.Identifier, syntax.TypeParameterList); | ||
| var methodSyntaxTemplate = new ContainingSyntax(new SyntaxTokenList(syntax.Modifiers.Where(static m => !m.IsKind(SyntaxKind.PartialKeyword))).StripAccessibilityModifiers(), SyntaxKind.MethodDeclaration, syntax.Identifier, syntax.TypeParameterList); |
There was a problem hiding this comment.
The PR description mentions that both ComInterfaceGenerator and VtableIndexStubGenerator already strip the new modifier, but in this code path VtableIndexStubGenerator only strips accessibility (and now partial). Either update the PR description, or (if intended) extend this modifier filter to also remove SyntaxKind.NewKeyword for consistency with ComInterfaceGenerator.
🤖 Copilot Code Review — PR #126719Note This review was generated by Copilot (Claude Opus 4.6). A GPT-5.4 sub-agent was launched in parallel but did not return within the timeout window; this review reflects a single-model analysis. Holistic AssessmentMotivation: Justified. When a user declares a Approach: Correct and consistent with established patterns. The Summary: ✅ LGTM. The fix is correct, minimal, and well-tested for the primary code path. One minor test coverage observation below (non-blocking). Detailed Findings✅ Correctness — Partial modifier stripping is correctThe generated methods are explicit interface implementations (via ✅ Test — The test validates the fix for ComInterfaceGeneratorThe new 💡 Test coverage — VtableIndexStubGenerator path not directly testedThe fix modifies both 💡 Pre-existing inconsistency —
|
Description
ComInterfaceGeneratorandVtableIndexStubGeneratorcopy method modifiers from user-declared interface methods into generated stub code. Thenewkeyword and accessibility modifiers were already stripped, butpartialwas not — producing invalid C# when the user declares partial methods on a[GeneratedComInterface]interface.Changes:
ComInterfaceGenerator.cs: FilterSyntaxKind.PartialKeywordalongside existingNewKeywordfilter inCalculateStubInformationVtableIndexStubGenerator.cs: AddPartialKeywordfilter to modifier stripping inCalculateStubInformationCompiles.cs: Add regression test confirming partial method modifiers don't produce invalid generated codeNote:
LibraryImportGeneratorandDownlevelLibraryImportGeneratorare intentionally unchanged — they generate partial method implementations wherepartialmust be preserved.