Skip to content

Honor MarshalAs.IidParameterIndex for out object in source-generated COM stubs#128214

Merged
AaronRobinsonMSFT merged 12 commits into
mainfrom
copilot/fix-com-interop-marshaling
May 19, 2026
Merged

Honor MarshalAs.IidParameterIndex for out object in source-generated COM stubs#128214
AaronRobinsonMSFT merged 12 commits into
mainfrom
copilot/fix-com-interop-marshaling

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 14, 2026

Source-generated COM accepted [MarshalAs(UnmanagedType.Interface)] out object but ignored IidParameterIndex, always returning IUnknown* to native callers. This change enables activation-style behavior by using the runtime IID for marshal-out and preserving unsupported-shape diagnostics.

  • Description
    • Parser + marshalling metadata
      • Added IidParameterIndex use-site propagation through marshalling info.
      • Added explicit supported-shape gating: only [MarshalAs(UnmanagedType.Interface)] out object.
      • Tracked IID parameter as an element dependency so codegen ordering remains valid.
    • Managed→unmanaged COM stub generation
      • Added a dedicated resolver for IID-indexed COM marshal-out.
      • Generated stub now:
        1. gets IUnknown* via ComInterfaceMarshaller<object>,
        2. calls Marshal.QueryInterface with the runtime IID parameter,
        3. writes the QI result to void**,
        4. releases the intermediate IUnknown*,
        5. throws COMException(hr) on QI failure (HRESULT propagation path).
      • Included failure cleanup for non-null QI out pointer on error.
    • Diagnostics
      • Preserved unsupported configuration diagnostics for non-qualifying shapes.
      • Updated message text to document the supported pattern.
    • Test updates
      • Added compile-pass coverage for the supported shape.
      • Added compile-fail coverage for non-supported shapes (ref/in usage path).
      • Added runtime coverage validating:
        • requested IID is honored,
        • unsupported IID returns E_NOINTERFACE,
        • supported IID returns the requested interface pointer.
[GeneratedComInterface]
[Guid("...")]
partial interface IActivationFactory
{
    void GetActivationFactory(
        in Guid iid,
        [MarshalAs(UnmanagedType.Interface, IidParameterIndex = 0)] out object factory);
}

Copilot AI self-assigned this May 14, 2026
Copilot AI review requested due to automatic review settings May 14, 2026 17:56
Copilot AI review requested due to automatic review settings May 14, 2026 17:56
Co-authored-by: AaronRobinsonMSFT <30635565+AaronRobinsonMSFT@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot May 14, 2026 18:13
Copilot AI changed the title [WIP] Fix COM interop to honor MarshalAsAttribute.IidParameterIndex Honor MarshalAs.IidParameterIndex for out object in source-generated COM stubs May 14, 2026
Copilot AI requested a review from AaronRobinsonMSFT May 14, 2026 18:14
Co-authored-by: AaronRobinsonMSFT <30635565+AaronRobinsonMSFT@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot May 14, 2026 18:37
Copilot AI requested a review from AaronRobinsonMSFT May 14, 2026 18:38
@AaronRobinsonMSFT AaronRobinsonMSFT marked this pull request as ready for review May 14, 2026 19:13
Copilot AI review requested due to automatic review settings May 14, 2026 19:13
@dotnet-policy-service
Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

Co-authored-by: AaronRobinsonMSFT <30635565+AaronRobinsonMSFT@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 15, 2026

@copilot The unresolved (outdated-by-line-drift, but still applicable) comment about isArrayType in MarshalAsParser.cs was not addressed by the subsequent commits. The current code at HEAD still s...

Fixed in 3d9dc1c.

  • MarshalAsParser now reports ConfigurationNotSupported when IidParameterIndex is used on array marshal shapes (LPArray / ByValArray) instead of silently ignoring it.
  • Added compile-fail coverage: MarshalAsIidParameterIndexOnArrayUnmanagedType_ReportsDiagnostic for [MarshalAs(UnmanagedType.LPArray, IidParameterIndex = 0)] out object[].
  • The test asserts the same IidParameterIndexConfigurationNameWithSupportedShape diagnostic message as the other unsupported-shape tests.

@jtschuster
Copy link
Copy Markdown
Member

jtschuster commented May 15, 2026

I had CCA emit the compiler generated code and copied it here to review. I don't see any issues.

Details
// <auto-generated />
#pragma warning disable CS0612, CS0618, CS0649, CS1591

[global::System.Runtime.InteropServices.StructLayoutAttribute(global::System.Runtime.InteropServices.LayoutKind.Sequential)]
file unsafe struct InterfaceImplementationVtable
{
    public delegate* unmanaged[MemberFunction]<void*, global::System.Guid*, void**, int> QueryInterface_0;
    public delegate* unmanaged[MemberFunction]<void*, uint> AddRef_1;
    public delegate* unmanaged[MemberFunction]<void*, uint> Release_2;
    public delegate* unmanaged[MemberFunction]<global::System.Runtime.InteropServices.ComWrappers.ComInterfaceDispatch*, global::System.Guid*, void**, int> GetActivationFactory_3;
}

file unsafe sealed class InterfaceInformation : global::System.Runtime.InteropServices.Marshalling.IIUnknownInterfaceType
{
    public static global::System.Guid Iid { get; } = new([194, 86, 30, 120, 48, 165, 143, 74, 144, 254, 1, 36, 68, 38, 224, 204]);
    public static void** ManagedVirtualMethodTable => (void**)global::System.Runtime.CompilerServices.Unsafe.AsPointer(in InterfaceImplementation.Vtable);
}

[global::System.Runtime.InteropServices.DynamicInterfaceCastableImplementationAttribute]
file unsafe interface InterfaceImplementation : global::ComInterfaceGenerator.Tests.IActivationFactory
{
    [global::System.Runtime.CompilerServices.FixedAddressValueTypeAttribute]
    public static readonly InterfaceImplementationVtable Vtable;

    static InterfaceImplementation()
    {
        global::System.Runtime.InteropServices.ComWrappers.GetIUnknownImpl(
            out *(nint*)&((InterfaceImplementationVtable*)global::System.Runtime.CompilerServices.Unsafe.AsPointer(ref Vtable))->QueryInterface_0,
            out *(nint*)&((InterfaceImplementationVtable*)global::System.Runtime.CompilerServices.Unsafe.AsPointer(ref Vtable))->AddRef_1,
            out *(nint*)&((InterfaceImplementationVtable*)global::System.Runtime.CompilerServices.Unsafe.AsPointer(ref Vtable))->Release_2);

        Vtable.GetActivationFactory_3 = &ABI_GetActivationFactory;
    }

    [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Interop.ComInterfaceGenerator", "42.42.42.42")]
    [global::System.Runtime.CompilerServices.SkipLocalsInitAttribute]
    void global::ComInterfaceGenerator.Tests.IActivationFactory.GetActivationFactory(in global::System.Guid iid, out object factory)
    {
        var(__this, __vtable) = ((global::System.Runtime.InteropServices.Marshalling.IUnmanagedVirtualMethodTableProvider)this).GetVirtualMethodTableInfoForKey(typeof(global::ComInterfaceGenerator.Tests.IActivationFactory));
        var __target = ((delegate* unmanaged[MemberFunction]<void*, global::System.Guid*, void**, int> )__vtable[3]);
        {
            bool __invokeSucceeded = default;
            factory = default;
            void* __factory_native = default;
            int __invokeRetVal = default;
            try
            {
                // Pin - Pin data in preparation for calling the P/Invoke.
                fixed (global::System.Guid* __iid_native = &iid)
                {
                    __invokeRetVal = __target(__this, __iid_native, &__factory_native);
                }
    
                // NotifyForSuccessfulInvoke - Keep alive any managed objects that need to stay alive across the call.
                global::System.Runtime.InteropServices.Marshal.ThrowExceptionForHR(__invokeRetVal, new([194, 86, 30, 120, 48, 165, 143, 74, 144, 254, 1, 36, 68, 38, 224, 204]), (global::System.IntPtr)__this);
                global::System.GC.KeepAlive(this);
                __invokeSucceeded = true;
                // Unmarshal - Convert native data to managed data.
                factory = global::System.Runtime.InteropServices.Marshalling.ComInterfaceMarshaller<object>.ConvertToManaged(__factory_native);
            }
            finally
            {
                if (__invokeSucceeded)
                {
                    // CleanupCalleeAllocated - Perform cleanup of callee allocated resources.
                    global::System.Runtime.InteropServices.Marshalling.ComInterfaceMarshaller<object>.Free(__factory_native);
                }
            }
        }
    }

    [global::System.Runtime.InteropServices.UnmanagedCallersOnlyAttribute(CallConvs = new[] { typeof(global::System.Runtime.CompilerServices.CallConvMemberFunction) })]
    internal static int ABI_GetActivationFactory(global::System.Runtime.InteropServices.ComWrappers.ComInterfaceDispatch* __this_native, global::System.Guid* __iid_native__param, void** __factory_native__param)
    {
        global::ComInterfaceGenerator.Tests.IActivationFactory @this = default;
        ref global::System.Guid __iid_native = ref *__iid_native__param;
        global::System.Guid iid = default;
        ref void* __factory_native = ref *__factory_native__param;
        object factory = default;
        int __retVal = default;
        try
        {
            // Unmarshal - Convert native data to managed data.
            iid = __iid_native;
            @this = global::System.Runtime.InteropServices.ComWrappers.ComInterfaceDispatch.GetInstance<global::ComInterfaceGenerator.Tests.IActivationFactory>(__this_native);
            @this.GetActivationFactory(in iid, out factory);
            // NotifyForSuccessfulInvoke - Keep alive any managed objects that need to stay alive across the call.
            __retVal = 0; // S_OK
            // Marshal - Convert managed data to native data.
            void* __factory_native__unknown = (void*)global::System.Runtime.InteropServices.Marshalling.ComInterfaceMarshaller<object>.ConvertToUnmanaged(factory);
            if (__factory_native__unknown != null)
            {
                global::System.IntPtr __factory_native__queriedInterface = 0;
                int __factory_native__queryInterfaceHResult = global::System.Runtime.InteropServices.Marshal.QueryInterface((global::System.IntPtr)__factory_native__unknown, in iid, out __factory_native__queriedInterface);
                global::System.Runtime.InteropServices.Marshal.Release((global::System.IntPtr)__factory_native__unknown);
                if (__factory_native__queryInterfaceHResult != 0)
                {
                    if (__factory_native__queriedInterface != 0)
                        global::System.Runtime.InteropServices.Marshal.Release(__factory_native__queriedInterface);
                    __factory_native = null;
                    return __factory_native__queryInterfaceHResult;
                }
    
                __factory_native = (void*)__factory_native__queriedInterface;
            }
            else
            {
                __factory_native = null;
            }
        }
        catch (global::System.Exception __exception)
        {
            __retVal = global::System.Runtime.InteropServices.Marshalling.ExceptionAsHResultMarshaller<int>.ConvertToUnmanaged(__exception);
        }
    
        return __retVal;
    }
}

namespace ComInterfaceGenerator.Tests
{
    [global::System.Runtime.InteropServices.Marshalling.IUnknownDerivedAttribute<InterfaceInformation, InterfaceImplementation>]
    unsafe partial interface IActivationFactory
    {
    }
}

Fixes the four remaining items raised on the PR:

1. Route QI failure through the standard exception-to-HRESULT path.
   Replaces the inlined 'return queryInterfaceHResult;' inside the per-
   parameter Marshal stage with Marshal.ThrowExceptionForHR(hr). This
   eliminates the bake-in 'int' return-type assumption and ensures the
   stub's normal cleanup stages run on failure, by reusing the existing
   ManagedHResultExceptionGeneratorResolver-driven catch path.

2. Localize the diagnostic hint text. Adds the
   IidParameterIndexUnsupportedConfigurationName resx entry (with
   translator comment and {0} placeholder for the API name) and switches
   MarshalAsParser to build the hint via SR.Format. The English text is
   unchanged so existing test expectations continue to hold.

3. Add compile-fail coverage for additional invalid IidParameterIndex
   shapes: out-of-bounds index, IID pointing at a non-Guid parameter,
   IID pointing at the marshalled out object itself, and IID pointing
   at an 'out Guid'. (A 'ref Guid' IID parameter is now accepted, so
   the corresponding compile-fail test is replaced with a compile-pass
   test in item 4.) Tests that were already asserting only the primary
   IidParameterIndex diagnostic for shapes that also produce a natural
   secondary diagnostic (Struct on out object, LPArray on out object[],
   IID-points-at-self) are updated to assert the full diagnostic set.

4. Add compile-pass coverage for multi-parameter shapes: two out
   object parameters sharing one IID, two out object parameters with
   distinct IIDs, an IID-driven out object interleaved with a non-IID
   out object and other parameters, by-value Guid IID, and ref Guid IID.

The IID-parameter validation also gains a RefKind constraint
(RefKind.None | In | Ref) so out Guid is rejected at parse time.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 15, 2026 23:21
@AaronRobinsonMSFT
Copy link
Copy Markdown
Member

@jtschuster Please take another look.

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.

Copilot's findings

  • Files reviewed: 26/26 changed files
  • Comments generated: 2

Copy link
Copy Markdown
Member

@jtschuster jtschuster left a comment

Choose a reason for hiding this comment

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

LGTM

Two follow-up fixes in IidParameterIndexMarshallerResolver:

- Release the intermediate IUnknown* via ComInterfaceMarshaller<object>.Free
  instead of a direct Marshal.Release call. Functionally identical today
  (Free just delegates to Marshal.Release after a null check) but keeps
  the cleanup consistent with the marshaller contract and forward-
  compatible with any future change to Free's logic.

- Change the QI-failure check from 'hr != 0' to 'hr < 0' to match the
  semantics of Marshal.ThrowExceptionForHR. With the previous condition,
  a positive non-zero HRESULT (e.g., S_FALSE) would enter the cleanup
  branch, release the QI'd interface, set the native out parameter to
  null, then return without throwing because ThrowExceptionForHR only
  throws on hr < 0 - leaving the subsequent fall-through assignment to
  rewrite the native pointer from a freshly-released variable. The
  scenario is theoretical for IUnknown::QueryInterface in practice
  (which is documented to return only S_OK or E_NOINTERFACE) but the
  fix removes the latent inconsistency.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@AaronRobinsonMSFT AaronRobinsonMSFT enabled auto-merge (squash) May 16, 2026 02:42
Copilot AI review requested due to automatic review settings May 17, 2026 22:50
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.

Copilot's findings

  • Files reviewed: 26/26 changed files
  • Comments generated: 1

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 19, 2026 02:28
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.

Copilot's findings

  • Files reviewed: 26/26 changed files
  • Comments generated: 1

@AaronRobinsonMSFT AaronRobinsonMSFT merged commit 5cc6b8b into main May 19, 2026
96 of 104 checks passed
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the copilot/fix-com-interop-marshaling branch May 19, 2026 05:20
@github-project-automation github-project-automation Bot moved this to Done in AppModel May 19, 2026
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.

Source-generated COM interop should honor MarshalAsAttribute.IidParameterIndex for out object parameters

4 participants