Refactor marshaller types/methods resolution in 'cswinrtgen' #2164
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Consolidates logic for resolving marshaller types for custom-mapped Windows Runtime structs and classes, removing special cases for TimeSpan and DateTimeOffset. Updates checks to use IsCustomMappedWindowsRuntimeNonGenericStructOrClassType for consistency and maintainability.
Introduces a new TryGetNullableUnderlyingType method to extract the underlying type from a constructed Nullable<T> type. Also updates XML documentation to reference types without the System. prefix for clarity.
Moved marshaller type resolution logic from InteropMethodRewriteFactory to a new InteropMarshallerTypeResolver class. Updated all usages to use the new resolver, improving code organization and maintainability. Also fixed a bug in WindowsRuntimeExtensions to correctly check for value types in generic instance signatures.
Marked ExceptionMarshaller as unsafe and added BoxToUnmanaged and UnboxToManaged methods to support boxing and unboxing of System.Exception objects for interop scenarios.
Introduces InteropMarshallerType as a ref struct to encapsulate marshaller type and method resolution. Updates all usages to leverage strongly-typed marshaller method accessors, improving code clarity and reducing repeated logic. Refactors InteropMarshallerTypeResolver to return InteropMarshallerType instead of ITypeDefOrRef.
Removed special-case handling for generic instance types (such as KeyValuePair and constructed interfaces/delegates) in parameter and return value marshalling. Generic types are now handled by the standard marshaller resolution logic, simplifying the code and reducing duplication.
Introduces the CreateStind extension method to generate the appropriate indirect store (stind) instruction based on the provided type signature. This enhances the CilInstructionExtensions utility for handling various value types and custom types when emitting IL.
Replaces the switch statement for selecting the correct indirect store instruction for blittable types with a call to CilInstruction.CreateStind. This simplifies the code and centralizes the logic for determining the appropriate store instruction.
There was a problem hiding this comment.
Pull request overview
This PR refactors the marshalling type and method resolution logic in the cswinrtgen generator by consolidating it into dedicated resolver types (InteropMarshallerTypeResolver and InteropMarshallerType). It also adds missing BoxToUnmanaged and UnboxToManaged methods to the ExceptionMarshaller class and moves stind instruction generation to a reusable extension method.
Key changes:
- Centralizes marshaller type/method resolution into new resolver classes
- Adds box/unbox methods for
Exceptiontype - Extracts
stindinstruction logic toCilInstructionExtensions
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/WinRT.Runtime2/ABI/System/Exception.cs |
Adds BoxToUnmanaged and UnboxToManaged methods to ExceptionMarshaller |
src/WinRT.Interop.Generator/Resolvers/InteropMarshallerTypeResolver.cs |
New resolver for determining marshaller types for Windows Runtime types |
src/WinRT.Interop.Generator/Resolvers/InteropMarshallerType.cs |
New type providing methods to resolve marshaller method references |
src/WinRT.Interop.Generator/Factories/InteropMethodRewriteFactory.cs |
Removes duplicated marshaller type resolution methods (now in resolver) |
src/WinRT.Interop.Generator/Factories/InteropMethodRewriteFactory.ReturnValue.cs |
Refactored to use new InteropMarshallerTypeResolver |
src/WinRT.Interop.Generator/Factories/InteropMethodRewriteFactory.RetVal.cs |
Refactored to use new InteropMarshallerTypeResolver and CreateStind extension |
src/WinRT.Interop.Generator/Factories/InteropMethodRewriteFactory.NativeParameter.cs |
Refactored to use new InteropMarshallerTypeResolver |
src/WinRT.Interop.Generator/Factories/InteropMethodRewriteFactory.ManagedParameter.cs |
Refactored to use new InteropMarshallerTypeResolver |
src/WinRT.Interop.Generator/Extensions/WindowsRuntimeExtensions.cs |
Adds TryGetNullableUnderlyingType helper method for extracting Nullable<T> underlying types |
src/WinRT.Interop.Generator/Extensions/CilInstructionExtensions.cs |
Adds CreateStind extension method for generating indirect store instructions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// <returns>The resulting <see cref="IMethodDefOrRef"/> value.</returns> | ||
| public IMethodDefOrRef BoxToUnmanaged() | ||
| { | ||
| // When boxing, the parameter is either 'Nullable<T>' for value types, or just the same type |
There was a problem hiding this comment.
The BoxToUnmanaged method determines the parameter type based on _type.IsValueType, but according to line 77 in Exception.cs, BoxToUnmanaged for Exception (a reference type) takes global::System.Exception? as a parameter. However, the logic on line 87-89 would use just _type for reference types. Consider adding documentation explaining why reference types don't get wrapped in Nullable<T> for boxing, as this distinction may not be immediately clear to maintainers.
Suggested change
| // When boxing, the parameter is either 'Nullable<T>' for value types, or just the same type | |
| // When boxing, the parameter is either 'Nullable<T>' for value types, or just the same type for reference types. | |
| // | |
| // Value types: | |
| // For value types we explicitly construct Nullable<T> so that the boxed representation can encode null. | |
| // | |
| // Reference types: | |
| // Reference types are already nullable at the CLR level, so 'T' and 'T?' share the same underlying | |
| // metadata type. This means a C# signature such as 'Exception?' is represented using the same | |
| // TypeSignature as 'Exception'. As a result, for reference types we intentionally pass '_type' directly | |
| // instead of wrapping it in Nullable<T>. |
| /// <returns>Whether <paramref name="underlyingType"/> was successfully retrieved.</returns> | ||
| public bool TryGetNullableUnderlyingType(InteropReferences interopReferences, [NotNullWhen(true)] out TypeSignature? underlyingType) | ||
| { | ||
| // First check that we have some constructed generic value type. |
There was a problem hiding this comment.
The pattern match checks IsValueType: true when extracting the underlying type from Nullable<T>. While Nullable<T> is indeed a value type, this constraint might be overly restrictive or unclear in intent. Consider adding a comment explaining why this check is necessary, or if it's primarily a sanity check to ensure we're dealing with value type generics.
Suggested change
| // First check that we have some constructed generic value type. | |
| // First check that we have some constructed generic value type. | |
| // The 'IsValueType: true' constraint is a sanity check to ensure we're dealing with a | |
| // value-type generic (such as Nullable<T>) and not an arbitrary reference-type generic. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR refactors and centralizes the logic to resolve marshalling types and methods in 'cswinrtgen'. It also moves some
stindinstruction calculation logic to a separate extension, and adds the missing box/unbox methods forException.