Convert Marshal test Variant usage to ComVariant

[Copilot]

Note

This pull request description was generated with AI/Copilot assistance.

Summary

• Replaced System.Runtime.InteropServices.Tests.Common.Variant usage in GetNativeVariantForObjectTests with ComVariant.
• Updated variant assertions to use ComVariant.VarType and GetRawDataRef<T>().
• Removed the now-obsolete test helper file Marshal/Common/Variant.cs and its project include.

Validation

• Baseline build: ./build.sh clr+libs -rc release
• dotnet build in src/libraries/System.Runtime.InteropServices
• dotnet build /t:test for System.Runtime.InteropServices.Tests.csproj
• parallel_validation run (Code Review + CodeQL)

Notes

• Multi-model code review was run; one model flagged incomplete migration, but this was a false positive because the other Variant usages are separate test-local structs, and the build/tests pass.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

This PR migrates GetNativeVariantForObjectTests away from the test-local System.Runtime.InteropServices.Tests.Common.Variant helper to the runtime-supported ComVariant, and removes the obsolete helper from the test project.

Changes:

• Updated GetNativeVariantForObjectTests (and Windows-specific companion) to allocate/read native VARIANTs using ComVariant, including updated assertions via VarType and GetRawDataRef<T>().
• Added a local Record layout helper struct for validating the VT_RECORD payload.
• Deleted Marshal/Common/Variant.cs and removed it from the test project file.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
.../Marshal/GetNativeVariantForObjectTests.cs	Switches VARIANT interop in tests to ComVariant and updates validation logic.
.../Marshal/GetNativeVariantForObjectTests.Windows.cs	Switches Windows-only invalid-array test to allocate using ComVariant.
.../Marshal/Common/Variant.cs	Removes obsolete test helper struct.
System.Runtime.InteropServices.Tests.csproj	Drops compile include for the removed helper file.

[Copilot]

In the VT_RECORD case, the VARIANT typically owns both the record data and an IRecordInfo COM pointer. This test reads the pointers but doesn’t clear the VARIANT afterwards, so the record allocation/COM reference can leak. Consider disposing/clearing the VARIANT stored at pNative (e.g., ((ComVariant*)pNative)->Dispose() on Windows) once assertions are complete; ensure this is skipped on the NanoServer/exception path where the VARIANT may not have been initialized.

[Copilot]

pNative holds a native VARIANT produced by Marshal.GetNativeVariantForObject. Some inputs in this test data produce VARIANTs that own native resources (e.g., VT_BSTR, VT_ARRAY/SAFEARRAY, COM interface pointers), but the buffer is later freed without clearing the VARIANT, which can leak those resources across the test run. Since this test now uses ComVariant, consider clearing the VARIANT stored at pNative (e.g., via an unsafe cast to ComVariant* and calling Dispose()) before freeing the backing buffer; make sure to guard the dispose so it only runs after GetNativeVariantForObject succeeds.

[github-actions]

🤖 Copilot Code Review — PR #126968

Note

This review was generated by Copilot.

Holistic Assessment

Motivation: The PR replaces a custom test-only Variant struct with the production ComVariant type from System.Runtime.InteropServices.Marshalling. This is justified — the custom struct was a simplified reimplementation of a type that already exists in the framework, and eliminating it reduces test infrastructure maintenance.

Approach: Straightforward mechanical replacement. The ComVariant.VarType property replaces manual (VarEnum)result.vt casts, and GetRawDataRef<T>() replaces direct field access to the union data area. A small local Record struct is added to support typed access to VT_RECORD data. The deleted Common/Variant.cs was only consumed by the modified test files (other test files define their own local Variant structs).

Summary: ✅ LGTM. The memory layout mapping between the old Variant and new ComVariant is correct, no test coverage is lost, and the deleted file has no other consumers. Multi-model review (Claude Opus 4.6, GPT-5.3-Codex, Claude Haiku 4.5) found no blocking issues.

---

Detailed Findings

✅ Memory Layout Correctness — Verified

The old Variant placed bstrVal at offset 8 (after four ushort header fields) and pRecInfo at offset 8 + IntPtr.Size. ComVariant.GetRawDataRef<T>() returns ref Unsafe.As<UnionTypes, T>(ref _typeUnion._unionTypes), which is the same offset 8 position. The Record struct used with GetRawDataRef<Record>() has _record at offset 0 (global offset 8) and _recordInfo at offset IntPtr.Size (global offset 8 + IntPtr.Size), matching the old layout exactly. Marshal.SizeOf<ComVariant>() equals the native VARIANT size (24 bytes on 64-bit, 16 bytes on 32-bit), same as the old struct.

✅ Allocation Size — Correct

All Marshal.AllocHGlobal(Marshal.SizeOf(v)) calls (where v was a Variant instance) are replaced with Marshal.AllocHGlobal(Marshal.SizeOf<ComVariant>()). The sizes are identical because both structs match the native VARIANT layout.

✅ Deleted File Has No Other Consumers — Verified

Common/Variant.cs was in the System.Runtime.InteropServices.Tests.Common namespace. Grep confirms no other files reference Tests.Common.Variant — the remaining using Tests.Common directives in other test files reference types from CommonTypes.cs and COMWrappersImpl.cs.

✅ No New Public API Surface

All changes are test-only. No ref/ assembly changes.

💡 Minor: Record Struct Duplication

GetNativeVariantForObjectTests adds a private struct Record { nint _record; nint _recordInfo; } that is layout-identical to the public struct Record already defined in GetObjectForNativeVariantTests. This is fine for test code — both are small, self-contained, and scoped to their respective test classes — but could optionally be consolidated into a shared location if desired.

Generated by Code Review for issue #126968 · ◷