CsWin32 follow-up: ComReference family + typelib registration interop - #13975
Conversation
Continues the CsWin32 struct-based COM interop migration (after dotnet#13872), converting the remaining hand-rolled COM in src/Tasks to the Windows.Win32 PInvoke / ComScope<T> / AgileComPointer<T> surface defined in src/Framework. ComReference resolution (net472-only, #if FEATURE_APPDOMAIN): - ComReferenceInfo.cs: typeLibPointer is now AgileComPointer<ITypeLib> and attr is TLIBATTR; the type library loads through PInvoke.LoadTypeLibEx into a ComScope<ITypeLib> using the PInvoke.LOAD_TLB_AS_32BIT/_64BIT constants. - ComReference.cs: GetTypeLibAttrForTypeLib / GetTypeAttrForTypeInfo / GetVarDescForVarIndex / GetFuncDescForDescIndex / GetTypeLibNameForITypeLib (QI to ITypeLib2 + VARIANT) / Ado27Installed take struct pointers; GetPathOfTypeLib inlines PInvoke.QueryPathOfRegTypeLib(out BSTR). - ComDependencyWalker.cs: struct pointers throughout; IID comparisons via IUnknown/IDispatch.IID_Guid; GetCustData via VARIANT + VariantClear; drops the MarshalReleaseComObject delegate and the IFixedTypeInfo usage. - TlbReference / AxReference / PiaReference / AxTlbBaseReference / IComReferenceResolver / ResolveComReference: gated; the TYPELIBATTR alias is repointed to Windows.Win32.System.Com.TLIBATTR. - IFixedTypeInfo.cs is retained as public API but is no longer used internally. Type-library registration (net472-only): - RegisterAssembly.cs / UnregisterAssembly.cs register and unregister the type library through PInvoke.RegisterTypeLib / UnRegisterTypeLib / LoadTypeLibEx / LoadRegTypeLib with the pointer held in a `using ComScope<ITypeLib>`. GenerateResource zone check: - Deletes the hand-rolled Interop.cs (IInternetSecurityManager and friends). IsDangerous now CoCreates the security manager via PInvoke.CoCreateInstance, caches it in an AgileComPointer<IInternetSecurityManager> behind a double-checked lock, and calls MapUrlToZone on a ComScope. Inlining / dead-code removal: - Move.cs: MoveFileEx inlined as a #if FEATURE_WINDOWSINTEROP helper over PInvoke.MoveFileEx + MOVE_FILE_FLAGS. - Exec.cs: AllDrivesMapped inlined over PInvoke.GetLogicalDrives. - NativeMethods.cs: removes the now-dead RegisterTypeLib / UnregisterTypeLib / LoadTypeLibEx / LoadRegTypeLib / QueryPathOfRegTypeLib / REGKIND / MoveFileEx / MoveFileFlags / AllDrivesMapped wrappers and the duplicate IUnknown/IDispatch/ITypeInfo IID constants. Framework plumbing: - NativeMethods.txt: adds ITypeLib2, IDispatch, IDispatchEx, IEnumVARIANT, IInternetSecurityManager, LoadTypeLibEx, LoadRegTypeLib, RegisterTypeLib, UnRegisterTypeLib, QueryPathOfRegTypeLib, LOAD_TLB_AS_32BIT, LOAD_TLB_AS_64BIT. - IComIIDPolyfills.cs: ITypeLib/ITypeLib2/ITypeInfo/ITypeComp IComIID partials. - GeneratedInteropClsCompliance.cs: CLSCompliant(false) partials for the newly pulled interfaces (ITypeLib2, IDispatchEx, IEnumVARIANT, IServiceProvider) to keep warn-as-error happy on net10. Tests: - The COM-reference tests are gated #if FEATURE_APPDOMAIN and drive the struct-based code through a CCW bridge: managed mocks implement the BCL System.Runtime.InteropServices.ComTypes.* interfaces and are handed to the SUT as interface pointers via Marshal.GetComInterfaceForObject. MockTypeLib throws COMException (not Assert) on out-of-range input; MockTypeInfo drops IFixedTypeInfo from its base list with a <remarks> explaining why it must not be reintroduced (shared IID 00020401, different ABI). Skill docs: - cswin32-com / cswin32-interop SKILL.md document the CCW-mocking boundary rules, standalone PInvoke constants, and the CS3016 transitive-CLSCompliant gotcha, and are compressed for token efficiency. Verified: Microsoft.Build.Framework and Microsoft.Build.Tasks build clean on net472 and net10.0; the COM-reference, Move, Exec, and RegisterAssembly unit tests pass on net472.
🔍 Skill Validator Results
Summary
Full validator output```text Found 2 skill(s) [cswin32-com] 📊 cswin32-com: 4,238 BPE tokens [chars/4: 4,095] (standard ~), 14 sections, 7 code blocks [cswin32-com] ⚠ Skill is 4,238 BPE tokens (chars/4 estimate: 4,095) — approaching "comprehensive" range where gains diminish. [cswin32-interop] 📊 cswin32-interop: 3,568 BPE tokens [chars/4: 3,337] (standard ~), 14 sections, 3 code blocks [cswin32-interop] ⚠ Skill is 3,568 BPE tokens (chars/4 estimate: 3,337) — approaching "comprehensive" range where gains diminish. ✅ All checks passed (2 skill(s)) ``` |
There was a problem hiding this comment.
Pull request overview
This PR continues the MSBuild src/Tasks COM interop migration to the CsWin32 struct-based pattern (Windows.Win32.PInvoke, ComScope<T>, AgileComPointer<T>), removing remaining [ComImport]/[DllImport] COM declarations from COM reference resolution, type library registration, and the GenerateResource zone check path (net472 / Windows-only as appropriate).
Changes:
- Migrates COM reference resolution and dependency walking to struct-based COM pointers (incl.
TLIBATTR,VARIANT,BSTR) and removes legacy COM wrapper plumbing fromNativeMethods.cs. - Migrates type library registration/unregistration to CsWin32 (
RegisterTypeLib/UnRegisterTypeLib/LoadTypeLibEx/LoadRegTypeLib) withComScope<T>lifetimes. - Replaces the hand-rolled urlmon COM interop for MOTW/zone checks in
GenerateResourcewithCoCreateInstance+ cachedAgileComPointer<IInternetSecurityManager>, updates unit tests to drive struct-based COM via CCW bridging, and refreshes cswin32 skills docs.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Tasks/UnregisterAssembly.cs | Switches typelib unregistration to CsWin32 LoadTypeLibEx + UnRegisterTypeLib with ComScope<T>. |
| src/Tasks/RegisterAssembly.cs | Switches typelib registration to CsWin32 LoadTypeLibEx + RegisterTypeLib with ComScope<T>. |
| src/Tasks/ComReferenceInfo.cs | Stores typelibs as AgileComPointer<ITypeLib> and uses CsWin32 TLIBATTR/LoadTypeLibEx flags. |
| src/Tasks/ComReference.cs | Rewrites typelib/typeinfo helpers to pointer-based COM, VARIANT/VariantClear, and BSTR out params. |
| src/Tasks/ComDependencyWalker.cs | Converts dependency walker to struct-based COM pointers and removes Marshal.ReleaseComObject injection. |
| src/Tasks/ResolveComReference.cs | Updates dependency scanning and task-item metadata parsing for CsWin32 TLIBATTR shapes. |
| src/Tasks/TlbReference.cs | Bridges agile ITypeLib* back to an RCW for TypeLibConverter (BCL API constraint). |
| src/Tasks/PiaReference.cs | Updates PIA lookup callsites for TLIBATTR field types. |
| src/Tasks/AxReference.cs | Updates wrapper filename version handling for TLIBATTR field types. |
| src/Tasks/AxTlbBaseReference.cs | Gates COM reference code behind FEATURE_APPDOMAIN. |
| src/Tasks/IComReferenceResolver.cs | Gates COM reference resolver contract behind FEATURE_APPDOMAIN and swaps to CsWin32 TLIBATTR. |
| src/Tasks/GenerateResource.cs | Replaces urlmon COM interop with CoCreateInstance + cached AgileComPointer<IInternetSecurityManager>. |
| src/Tasks/Exec.cs | Inlines AllDrivesMapped using CsWin32 GetLogicalDrives. |
| src/Tasks/Move.cs | Inlines MoveFileEx helper over CsWin32 PInvoke.MoveFileEx with managed fallback. |
| src/Tasks/NativeMethods.cs | Removes dead COM/interop wrappers and clarifies StdOle identifier as LIBID, not IID. |
| src/Tasks/Microsoft.Build.Tasks.csproj | Removes Interop.cs from compilation. |
| src/Tasks/Interop.cs | Deleted legacy [ComImport] urlmon COM definitions. |
| src/Tasks.UnitTests/ResolveComReference_Tests.cs | Updates test expectations for CsWin32 TLIBATTR field types. |
| src/Tasks.UnitTests/MockTypeLib.cs | Adjusts mock behavior for CCW boundary (throws COMException on invalid input). |
| src/Tasks.UnitTests/MockTypeInfo.cs | Removes IFixedTypeInfo from mock to avoid IID/ABI ambiguity across CCW. |
| src/Tasks.UnitTests/ComReferenceWalker_Tests.cs | Switches tests to pass CCW pointers into struct-based walker (Marshal.GetComInterfaceForObject). |
| src/Tasks.UnitTests/ComReference_Tests.cs | Gates COM reference tests behind FEATURE_APPDOMAIN. |
| src/Framework/NativeMethods.txt | Adds COM interfaces/APIs/constants required for migrated paths. |
| src/Framework/Polyfills/IComIIDPolyfills.cs | Adds IComIID polyfills for additional generated COM structs. |
| src/Framework/Windows/Win32/GeneratedInteropClsCompliance.cs | Adds [CLSCompliant(false)] partials for new transitive COM interfaces. |
| .github/skills/cswin32-interop/SKILL.md | Documents CsWin32 constant-vs-enum flag behavior for LoadTypeLibEx flags. |
| .github/skills/cswin32-com/SKILL.md | Documents CCW-based testing boundary rules for struct-based COM and CLS compliance gotchas. |
- GenerateResource.cs: mark the lazily-CoCreated InternetSecurityManager field volatile so the double-checked lock in IsDangerous has safe publication; a second thread can no longer observe the AgileComPointer before the GIT cookie written in its constructor is visible. - TlbReference.cs: release the temporary RCW from Marshal.GetObjectForIUnknown in a finally so it does not hold a COM reference on the type library until GC in long-lived MSBuild processes (server / node reuse).
The TYPELIBATTR alias and Windows.Win32.System.Com import in ResolveComReference.cs were under #if !NET, but the netstandard2.0 leg compiled by source build (DotNetBuildSourceOnly=true) also matches #if !NET while lacking FEATURE_WINDOWSINTEROP, so the CsWin32-generated TLIBATTR type does not exist there (CS0234). Normal builds define FEATURE_WINDOWSINTEROP and so compiled cleanly, hiding the break. Move the two CsWin32 usings into an #if FEATURE_APPDOMAIN block (matching the sibling ComReference files). The real implementation that uses them is net472-only, so the gate is correct; the netstandard2.0 / source-build stub no longer references the generated type.
…ds.cs Deletes src/Tasks/ManifestUtil/NativeMethods.cs and moves its P/Invokes to the CsWin32 PInvoke surface (or inline at the call site for the one CLR-internal API). - EmbeddedManifestReader.cs: LoadLibraryEx / EnumResourceNames / FindResource / LoadResource / LockResource / SizeofResource / FreeLibrary via PInvoke. The EnumResourceNames callback is an unmanaged stdcall function pointer bridged from a managed delegate (works on .NET Framework and .NET Core). The whole native path is #if FEATURE_WINDOWSINTEROP with a no-op source-build fallback. - ComImporter.cs: rewritten to use the struct-based ITypeLib/ITypeInfo directly (LoadTypeLibEx + ComScope<ITypeLib>) instead of marshalling back to an RCW. Early-returns on !IsWindows and on load failure; conditions inverted to flatten nesting. GetDocumentation passes null for the out-params each call site ignores. - SecurityUtil.cs: clr.dll loading (SetDllDirectory / LoadLibraryEx / FreeLibrary) via PInvoke, nested inside the existing RUNTIME_TYPE_NETCORE gate. - MetadataReader.cs: GetAssemblyIdentityFromFile is a CLR-internal mscorwks export (not in the Win32 metadata), so it is declared inline at its sole call site. - NativeMethods.txt: add EnumResourceNames, SetDllDirectory, SfcIsFileProtected. Windows-only call sites are guarded by NativeMethodsShared.IsWindows (a windows6.1 platform guard) to satisfy CA1416 without rippling [SupportedOSPlatform] onto cross-platform callers. Validated: Microsoft.Build.Tasks builds clean on net472, net10.0, and source-build netstandard2.0; ManifestUtil/signing unit tests pass on net472 and net10.0.
…EROP The source-build leg compiles Microsoft.Build.Tasks for net10.0 (in addition to netstandard2.0) with code-style analyzers as errors. With FEATURE_WINDOWSINTEROP stripped, the previous commit left dangling members that tripped warn-as-error: EmbeddedManifestReader.cs(20): CS0649 _manifest never assigned EmbeddedManifestReader.cs(6): IDE0005 unnecessary using ComImporter.cs(6): IDE0005 unnecessary using ComImporter.cs(176): IDE0051 GetRegisteredClassInfo unused Gate all the now-Windows-only members and usings consistently: - EmbeddedManifestReader: move _manifest, the const/delegate, the ctor, the callback and Read into the FEATURE_WINDOWSINTEROP block; the source-build path is a single public static Stream Read(string) => null;. - ComImporter: gate the instance fields, the static known-key arrays, the registry/documentation helpers and the ClassInfo nested type; only the public ctor and the Success/ComClasses/TypeLib properties stay ungated. Validated with -c Release -p:EnforceCodeStyleInBuild=true -warnaserror on all four configs: net472, net10.0, source-build net10.0, and source-build netstandard2.0. ManifestUtil unit tests pass on net472.
|
https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/746732 experimental insertion passed |
|
/review |
|
❌ Expert Code Review (command) failed. Please review the logs for details. |
JanProvaznik
left a comment
There was a problem hiding this comment.
I read through it, seems good.
then I gave it to worflow reviewer locally
Here's the review of PR #13975 — "CsWin32 follow-up: ComReference family + typelib registration interop" (@JeremyKuhne, 32 files).
Verification (done during review)
- ✅ Builds clean (0 warn / 0 err): net472, net10.0, netstandard2.0, and source-only (DotNetBuildSourceOnly=true, WINDOWSINTEROP off)
- ✅ COM tests pass: ComReferenceWalker_Tests 13/13, ResolveComReference_Tests 21/21
Verdict: ✅ Approve with nits — no blocking issues
The hardest parts of this kind of migration are all handled correctly: COM ref-count balance (ComScope<T> disposal, AgileComPointer(takeOwnership:false) paired with GIT register/revoke = net-zero), BSTR/VARIANT freeing (using/VariantClear in finally), HRESULT→same-exception equivalence, the EnumResourceNames callback kept alive via GC.KeepAlive, and the IInternetSecurityManager double-checked lock (volatile + safe publication). The "no behavior change" claim holds; the FEATURE_APPDOMAIN ⟹ FEATURE_WINDOWSINTEROP gating is sound.
Findings (all Low / Nit — none block merge)
- L1 ComImporter.cs — non-Windows early-return leaves Success == true with null ComClasses/TypeLib. Set Success = false (unreachable in practice, but inconsistent). Most worth fixing.
- L2 UnregisterAssembly.cs — ReleaseTLibAttr moved out of a finally; lost null-attr guard + debug assert. Restore the finally. Most worth fixing.
- L3 ComDependencyWalker.cs:219 — GetCustData COM error now swallowed (.Succeeded) instead of aborting the typelib; only differs on genuine errors.
- L4 ComReferenceWalker_Tests — fault-injection assertion relaxed (Count <= 1 + type check); necessary for CCW boundary but slightly less precise.
- L5 GenerateResource.IsDangerous — zone check additionally gated on FEATURE_WINDOWSINTEROP; fail-safe in the hypothetical net4x-source-only case, fragile coupling.
- L6 ResolveComReference.cs — short→ushort/int→uint parse widening (actually a correctness improvement; downstream casts still truncate).
- Nits: MockTypeLib.cs:247 stray brace formatting; over-broad fixed block in RegisterAssembly.cs; IFixedTypeInfo correctly retained for public-API compat but now dead.
Context
Continues the CsWin32 struct-based COM interop migration started in #13872, converting the remaining hand-rolled COM in
src/Tasksto theWindows.Win32PInvoke/ComScope<T>/AgileComPointer<T>surface defined insrc/Framework. This removes the last[ComImport]/[DllImport]COM declarations in the COM-reference resolution, type-library registration, and resource-zone paths.Changes Made
ComReference resolution (net472-only,
#if FEATURE_APPDOMAIN)ComReferenceInfo.cs:typeLibPointeris nowAgileComPointer<ITypeLib>andattrisTLIBATTR; the type library loads throughPInvoke.LoadTypeLibExinto aComScope<ITypeLib>using thePInvoke.LOAD_TLB_AS_32BIT/_64BITconstants.ComReference.cs:GetTypeLibAttrForTypeLib/GetTypeAttrForTypeInfo/GetVarDescForVarIndex/GetFuncDescForDescIndex/GetTypeLibNameForITypeLib(QI toITypeLib2+VARIANT) /Ado27Installedtake struct pointers;GetPathOfTypeLibinlinesPInvoke.QueryPathOfRegTypeLib(out BSTR).ComDependencyWalker.cs: struct pointers throughout; IID comparisons viaIUnknown/IDispatch.IID_Guid;GetCustDataviaVARIANT+VariantClear; drops theMarshalReleaseComObjectdelegate and theIFixedTypeInfousage.TlbReference.cs/AxReference.cs/PiaReference.cs/AxTlbBaseReference.cs/IComReferenceResolver.cs/ResolveComReference.csgated; theTYPELIBATTRalias is repointed toWindows.Win32.System.Com.TLIBATTR.IFixedTypeInfo.csis retained as public API but is no longer used internally.Type-library registration (net472-only)
RegisterAssembly.cs/UnregisterAssembly.csregister and unregister the type library throughPInvoke.RegisterTypeLib/UnRegisterTypeLib/LoadTypeLibEx/LoadRegTypeLib, with the pointer held in ausing ComScope<ITypeLib>.GenerateResource zone check
Interop.cs(IInternetSecurityManagerand friends).IsDangerousnow CoCreates the security manager viaPInvoke.CoCreateInstance, caches it in anAgileComPointer<IInternetSecurityManager>behind a double-checked lock, and callsMapUrlToZoneon aComScope.Inlining / dead-code removal
Move.cs:MoveFileExinlined as a#if FEATURE_WINDOWSINTEROPhelper overPInvoke.MoveFileEx+MOVE_FILE_FLAGS.Exec.cs:AllDrivesMappedinlined overPInvoke.GetLogicalDrives.NativeMethods.cs: removes the now-deadRegisterTypeLib/UnregisterTypeLib/LoadTypeLibEx/LoadRegTypeLib/QueryPathOfRegTypeLib/REGKIND/MoveFileEx/MoveFileFlags/AllDrivesMappedwrappers and the duplicateIUnknown/IDispatch/ITypeInfoIID constants.Framework plumbing
NativeMethods.txt: addsITypeLib2,IDispatch,IDispatchEx,IEnumVARIANT,IInternetSecurityManager,LoadTypeLibEx,LoadRegTypeLib,RegisterTypeLib,UnRegisterTypeLib,QueryPathOfRegTypeLib,LOAD_TLB_AS_32BIT,LOAD_TLB_AS_64BIT.IComIIDPolyfills.cs:ITypeLib/ITypeLib2/ITypeInfo/ITypeCompIComIIDpartials.GeneratedInteropClsCompliance.cs:CLSCompliant(false)partials for the newly pulled interfaces (ITypeLib2,IDispatchEx,IEnumVARIANT,IServiceProvider) to keep warn-as-error happy on net10.Tests
#if FEATURE_APPDOMAINand drive the struct-based code through a CCW bridge: managed mocks implement the BCLSystem.Runtime.InteropServices.ComTypes.*interfaces and are handed to the SUT as interface pointers viaMarshal.GetComInterfaceForObject.MockTypeLibthrowsCOMException(notAssert) on out-of-range input;MockTypeInfodropsIFixedTypeInfofrom its base list with a<remarks>explaining why it must not be reintroduced (shared IID00020401, different ABI).Skill docs
cswin32-com/cswin32-interopSKILL.mddocument the CCW-mocking boundary rules, standalonePInvokeconstants, and the CS3016 transitive-CLSCompliantgotcha, and are compressed for token efficiency.ClickOnce ManifestUtil interop (follow-up commit)
src/Tasks/ManifestUtil/NativeMethods.csand moves its P/Invokes to the CsWin32PInvokesurface (addingEnumResourceNames,SetDllDirectory,SfcIsFileProtectedtoNativeMethods.txt).EmbeddedManifestReader.cs—LoadLibraryEx/EnumResourceNames/FindResource/LoadResource/LockResource/SizeofResource/FreeLibraryviaPInvoke. TheEnumResourceNamescallback is an unmanaged stdcall function pointer bridged from a managed delegate (works on .NET Framework and .NET Core). The native path is#if FEATURE_WINDOWSINTEROPwith a no-op source-build fallback.ComImporter.cs— rewritten to use the struct-basedITypeLib/ITypeInfodirectly (LoadTypeLibEx+ComScope<ITypeLib>) instead of marshalling back to an RCW; early-returns on!IsWindowsand on load failure, with inverted conditions to flatten nesting.SecurityUtil.cs—clr.dllloading (SetDllDirectory/LoadLibraryEx/FreeLibrary) viaPInvoke, nested inside the existingRUNTIME_TYPE_NETCOREgate.MetadataReader.cs—GetAssemblyIdentityFromFileis a CLR-internalmscorwksexport (not in the Win32 metadata), so it is declared inline at its sole call site rather than CsWin32-generated.NativeMethodsShared.IsWindows(awindows6.1platform guard) to satisfy CA1416 without rippling[SupportedOSPlatform]onto cross-platform callers.Testing
Microsoft.Build.FrameworkandMicrosoft.Build.Tasksbuild clean (0 errors, 0 warnings) on net472 and net10.0.-p:TargetFramework=netstandard2.0 -p:DotNetBuildSourceOnly=true).Move,Exec, andRegisterAssemblyunit tests pass on net472;MetadataReader/SecurityUtil/AddToWin32Manifesttests pass on net472 and net10.0.Notes
#if FEATURE_APPDOMAIN) and the Windows interop is gated#if FEATURE_WINDOWSINTEROP, so source-build (DotNetBuildSourceOnly=true) and non-Windows builds are unaffected by construction.IFixedTypeInfois kept purely as a public-API compatibility surface even though it is now unused internally.