Skip to content

[GeneratedComInterface] Add unsafe to derived partial interface when base has unsafe methods#126109

Merged
jkoritzinsky merged 4 commits intomainfrom
copilot/fix-generated-com-interface-code
Mar 31, 2026
Merged

[GeneratedComInterface] Add unsafe to derived partial interface when base has unsafe methods#126109
jkoritzinsky merged 4 commits intomainfrom
copilot/fix-generated-com-interface-code

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 25, 2026

When a [GeneratedComInterface]-attributed derived interface inherits from a base interface containing unsafe methods (e.g., methods with pointer parameters), the generator emits shadowing/forwarding methods into a new partial interface declaration without the required unsafe modifier, causing CS0214.

Changes

  • ComInterfaceGenerator.cs: Extracted shadowing-method interface generation into a new GenerateShadowingMethodsInterface helper. The helper unconditionally adds unsafe to the generated partial interface's modifiers via AddToModifiers (no-op if already present), ensuring the emitted declaration is always valid regardless of whether the inherited methods use pointer types.

  • CodeSnippets.cs / Compiles.cs: Added DerivedComInterfaceTypeWithUnsafeBaseMethod test snippet covering the exact repro scenario.

Example

[GeneratedComInterface]
partial interface IBaseInterface
{
    unsafe void Read(void* pBuffer);  // base has unsafe method
}

[GeneratedComInterface]
partial interface IDerivedInterface : IBaseInterface  // no explicit 'unsafe' needed
{
    void Write();
}

Previously generated (broken):

public partial interface IDerivedInterface
{
    new void Read(void* pBuffer) => ((IBaseInterface)this).Read(pBuffer);  // CS0214
}

Now generates:

public unsafe partial interface IDerivedInterface
{
    new void Read(void* pBuffer) => ((IBaseInterface)this).Read(pBuffer);  // ✓
}
Original prompt

This section details on the original issue you should resolve

<issue_title>[GeneratedComInterface] Generates incorrect code if base interface has unsafe method</issue_title>
<issue_description>### Description

If a base type has an unsafe method then GeneratedComInterface generates methods for the derived class that need unsafe but are missing the keyword:

    public partial interface IDerivedInterface
    {
        [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Interop.ComInterfaceGenerator", "9.0.12.41916")]
        [global::System.Runtime.CompilerServices.SkipLocalsInitAttribute]
        new void BaseMethod(void* pBuffer) => ((global::Repro.IBaseInterface)this).BaseMethod(pBuffer);
    }

Error: Pointers and fixed size buffers may only be used in an unsafe context

Practical scenario where this comes up: ISequentialStream defines Read/Write that take pointers and IStream derives from it.

I also tried net10.0 (10.0.100-rc.1.25451.107 ) and it still reproed.

Reproduction Steps

Put this code in a new console app (I also put the repro here: https://github.com/jevansaks/GeneratedComInterfaceRepro/blob/master/Program.cs)

    [Guid("00000000-0000-0000-0000-000000000011"), InterfaceType(ComInterfaceType.InterfaceIsIUnknown), GeneratedComInterface]
    public partial interface IBaseInterface
    {
        unsafe void BaseMethod(void* pBuffer);
    }

    [Guid("00000000-0000-0000-0000-000000000012"), InterfaceType(ComInterfaceType.InterfaceIsIUnknown), GeneratedComInterface]
    public partial interface IDerivedInterface : IBaseInterface
    {
        void DerivedMethod();
    }

Expected behavior

GeneratedComInterface needs to add unsafe

Actual behavior

No unsafe keyword.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response</issue_description>

<agent_instructions>We should apply the unsafe keyword to the partial interface definition we emit for the derived interface in this case</agent_instructions>

Comments on the Issue (you are @copilot in this section)


📱 Kick off Copilot coding agent tasks wherever you are with GitHub Mobile, available on iOS and Android.

@dotnet-policy-service
Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @vitek-karas, @BrzVlad, @kotlarmilos
See info in area-owners.md if you want to be subscribed.

…e has unsafe methods

Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/0c4933d7-3aef-4bef-8952-d134aa2a2767
Copy link
Copy Markdown
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot can we do this unconditionally instead of limiting it to cases where the methods need the unsafe keyword?

Copilot AI changed the title [WIP] Fix generated code for derived interface with unsafe method [GeneratedComInterface] Add unsafe to derived partial interface when base has unsafe methods Mar 25, 2026
Copilot AI requested a review from jkoritzinsky March 25, 2026 18:14
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 25, 2026

@copilot can we do this unconditionally instead of limiting it to cases where the methods need the unsafe keyword?

Done in 2ccd618. The GenerateShadowingMethodsInterface helper now unconditionally calls AddToModifiers(SyntaxKind.UnsafeKeyword) (which is a no-op if unsafe is already present).

@jkoritzinsky jkoritzinsky marked this pull request as ready for review March 25, 2026 20:04
Copilot AI review requested due to automatic review settings March 25, 2026 20:04
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a codegen correctness issue in the ComInterfaceGenerator where derived [GeneratedComInterface] interfaces could fail to compile when inheriting unsafe (pointer-typed) base methods, by ensuring the generated “shadowing methods” partial interface is emitted in an unsafe context.

Changes:

  • Refactored shadowing-method interface emission into GenerateShadowingMethodsInterface, and ensured the generated partial interface always includes the unsafe modifier.
  • Added a new compilation test snippet reproducing the derived-interface + unsafe-base-method scenario.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
src/libraries/System.Runtime.InteropServices/gen/ComInterfaceGenerator/ComInterfaceGenerator.cs Adds GenerateShadowingMethodsInterface and applies unsafe to the generated derived partial interface so pointer-typed shadowing methods compile.
src/libraries/System.Runtime.InteropServices/tests/ComInterfaceGenerator.Unit.Tests/CodeSnippets.cs Adds DerivedComInterfaceTypeWithUnsafeBaseMethod test snippet covering the repro.
src/libraries/System.Runtime.InteropServices/tests/ComInterfaceGenerator.Unit.Tests/Compiles.cs Wires the new snippet into the compilation test data set.

@tannergooding
Copy link
Copy Markdown
Member

Now generates:

public unsafe partial interface IDerivedInterface
{
    new void Read(void* pBuffer) => ((IBaseInterface)this).Read(pBuffer);  // ✓
}

This potentially needs to be changed to put it on the narrowest scope possible to ensure it remains compatible with the unsafe evolution feature.

The exact direction the feature lands is TBD, but the general recommendation in either scenario, at least for dotnet/runtime, sounds like its going to land at putting unsafe at the narrowest scope possible.

@tannergooding
Copy link
Copy Markdown
Member

CC. @agocke, @EgorBo

@jkoritzinsky
Copy link
Copy Markdown
Member

As we move away from using syntax here to determine what to write, I wanted to start with something simple. As we finalize the unsafe-evolution feature and our rules in dotnet/runtime, I'll adjust the generator as needed.

@jkoritzinsky jkoritzinsky enabled auto-merge (squash) March 31, 2026 03:46
@github-actions
Copy link
Copy Markdown
Contributor

🤖 Copilot Code Review — PR #126109

Note

This review was generated by Copilot and validated across multiple AI models (Claude Opus 4.6, Claude Sonnet 4.5, GPT-5.2). All models agreed on the findings below.

Holistic Assessment

Motivation: The PR fixes a real bug where the ComInterfaceGenerator produces uncompilable code when a derived [GeneratedComInterface] inherits unsafe methods (e.g., methods with pointer parameters) from a base interface. The generated shadow method partial declaration lacked unsafe, causing CS0227 compile errors.

Approach: The fix unconditionally adds unsafe to the generated shadowing methods interface declaration via AddToModifiers(SyntaxKind.UnsafeKeyword). This is the simplest correct approach and is consistent with how all other generated types in this generator handle unsafe (e.g., ImplementationInterfaceTemplate at line 546 has unsafe baked in unconditionally).

Summary: ✅ LGTM. The fix is correct, minimal, consistent with existing patterns, and includes an appropriate regression test. No blocking issues were identified by any reviewer.


Detailed Findings

✅ Correctness — Fix addresses root cause

The root cause is clear: WrapMemberInContainingSyntaxWithUnsafeModifier adds unsafe to containing types (outer nesting classes), not to the member (interface) itself. For top-level interfaces with no containing types, the ContainingSyntax collection is empty and the wrapper adds unsafe nowhere. The fix correctly adds unsafe to the interface's own modifiers.

AddToModifiers properly handles duplicates (returns unchanged if unsafe already exists) and preserves modifier ordering relative to partial/ref, so there is no risk of double-addition or syntax errors.

✅ Consistency — Matches established generator patterns

All other generated types that contain potentially unsafe code have unsafe added unconditionally:

  • ImplementationInterfaceTemplate (line 546): file unsafe partial
  • InterfaceImplementationVtableTemplate (line 582-583): file unsafe
  • InterfaceInformationTemplate: file unsafe

The shadowing methods interface was the only generated partial that used the user's modifiers without adding unsafe. This PR aligns it with the rest of the generator.

✅ Incremental generator caching — No impact

The change is in a syntax generation method (GenerateShadowingMethodsInterface), not in the pipeline's transform/equality logic. The input data (ComInterfaceAndMethodsContext) is unchanged. Incremental compilation behavior is unaffected.

✅ Refactoring — Clean extraction into named method

Extracting the inline lambda into GenerateShadowingMethodsInterface improves readability and is consistent with the other Generate* methods in the file.

✅ Test quality — Adequate regression test

The test creates the exact scenario that triggers the bug: a derived interface (IComInterfaceDerived) inheriting from a base with an unsafe method (unsafe void Method(void* pBuffer)). The test validates that the generated code compiles successfully via VerifySourceGeneratorAsync, which is the standard validation pattern for source generator tests in this project.

💡 Minor suggestion — Additional test scenarios (follow-up, non-blocking)

All three reviewing models noted that while the current test is sufficient, future coverage could be enhanced with:

  • A nested type scenario (derived interface inside a class) with unsafe shadowing methods
  • Explicit assertion that the generated syntax tree contains the unsafe modifier (not just that it compiles)

These are low-risk areas since AddToModifiers handles duplicates and the compilation test would catch regressions.

Generated by Code Review for issue #126109 ·

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[GeneratedComInterface] Generates incorrect code if base interface has unsafe method

5 participants