Emit full 'IMap[View]Methods' types for dictionaries in 'cswinrtgen' #2165
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
Replaced hardcoded HSTRING types in delegate comments with <ELEMENT_TYPE>, <KEY_TYPE>, and <VALUE_TYPE> placeholders to generalize the function pointer signatures for broader applicability. This improves clarity and maintainability when generating code for different types.
Changed a comment to indicate that arguments are loaded inside an outer 'try/finally' block instead of 'try/catch', improving code clarity.
Inserted the Conv_U opcode in both IAsyncInfoMethods and IVectorViewMethods to ensure correct type conversion during interop method generation. This change improves type safety and correctness in the generated interop code.
Introduces a local variable 'elementAbiType' to store the ABI type of the element and uses it consistently when defining locals and method signatures for the 'GetAt' method. This improves code clarity and ensures the correct ABI type is used throughout the method generation process.
Moved the construction of the HasKey method for IMapView<K,V> from InteropTypeDefinitionBuilder to a new InteropMethodDefinitionFactory.IMapViewMethods class. This centralizes method creation logic and enables more maintainable and reusable code for interop method definitions.
Refactored Lookup method creation in InteropTypeDefinitionBuilder to use InteropMethodDefinitionFactory.IMapViewMethods.Lookup. Added full implementation of the Lookup method in InteropMethodDefinitionFactory, including method body, local variables, and parameter/return value rewriting logic for IMapView<K, V> interop.
Replaced manual construction of HasKey and Lookup methods with calls to InteropMethodDefinitionFactory.IMapViewMethods. This improves maintainability and centralizes method creation logic.
Moved the Insert method definition for IDictionary2 from InteropTypeDefinitionBuilder to a new InteropMethodDefinitionFactory.IMapMethods helper. This centralizes Insert method creation logic and improves maintainability by encapsulating method body generation and parameter marshaling.
Refactored the Remove method definition for IMap<TKey, TValue> interop types to use a new factory method. The new implementation generates a complete method body with proper marshaling, exception handling, and resource cleanup, improving maintainability and correctness.
Replaces valueType with valueAbiType in the call to WellKnownTypeSignatureFactory.IReadOnlyDictionary2LookupImpl to ensure the correct ABI type is used for the value parameter.
There was a problem hiding this comment.
Pull request overview
This PR implements full code generation for IMapViewMethods and IMapMethods types for dictionaries in the WinRT interop generator. The changes enable proper vtable handling and method emission for IDictionary<K,V> and IReadOnlyDictionary<K,V> interfaces.
Key Changes:
- Added complete implementations for
IMapViewMethods.HasKeyandIMapViewMethods.Lookupmethods - Added complete implementations for
IMapMethods.InsertandIMapMethods.Removemethods - Fixed vtable type selection bugs for
IDictionary<K,V>interfaces - Improved ABI type handling in various interop method factories
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| InteropGenerator.Emit.cs | Added missing emitState parameter when defining IReadOnlyDictionary types |
| WellKnownTypeDefinitionFactory.cs | Updated comments to use generic type placeholders instead of HSTRING |
| InteropMethodDefinitionFactory.IVectorViewMethods.cs | Fixed ABI type handling by extracting elementAbiType once and using consistently |
| InteropMethodDefinitionFactory.IMapViewMethods.cs | New file implementing HasKey and Lookup methods for IMapView interfaces |
| InteropMethodDefinitionFactory.IMapMethods.cs | New file implementing Insert and Remove methods for IMap interfaces |
| InteropMethodDefinitionFactory.IAsyncInfoMethods.cs | Added missing Conv_U instruction before vtable method call |
| InteropTypeDefinitionBuilder.IReadOnlyDictionary2.cs | Replaced placeholder implementations with actual method factory calls |
| InteropTypeDefinitionBuilder.IDictionary2.cs | Fixed vtable type references and replaced placeholder implementations |
| InteropTypeDefinitionBuilder.Delegate.cs | Corrected comment from 'try/catch' to 'try/finally' |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| /// <summary> | ||
| /// Creates a <see cref="MethodDefinition"/> for the <c>Remove</c> method for some <c>IMaplt;K, V></c> interface. |
There was a problem hiding this comment.
Corrected 'IMaplt;K' to 'IMap<K' in XML documentation.
Suggested change
| /// Creates a <see cref="MethodDefinition"/> for the <c>Remove</c> method for some <c>IMaplt;K, V></c> interface. | |
| /// Creates a <see cref="MethodDefinition"/> for the <c>Remove</c> method for some <c>IMap<K, V></c> interface. |
|
|
||
| // Track rewriting the return value for this method | ||
| emitState.TrackNativeParameterMethodRewrite( | ||
| paraneterType: keyType, |
There was a problem hiding this comment.
Corrected spelling of 'paraneterType' to 'parameterType'.
Suggested change
| paraneterType: keyType, | |
| parameterType: keyType, |
|
|
||
| // Track rewriting the 'key' parameter for this method | ||
| emitState.TrackNativeParameterMethodRewrite( | ||
| paraneterType: keyType, |
There was a problem hiding this comment.
Corrected spelling of 'paraneterType' to 'parameterType'.
Suggested change
| paraneterType: keyType, | |
| parameterType: keyType, |
|
|
||
| // Track rewriting the two parameters for this method | ||
| emitState.TrackNativeParameterMethodRewrite( | ||
| paraneterType: keyType, |
There was a problem hiding this comment.
Corrected spelling of 'paraneterType' to 'parameterType'.
| parameterIndex: 1); | ||
|
|
||
| emitState.TrackNativeParameterMethodRewrite( | ||
| paraneterType: valueType, |
There was a problem hiding this comment.
Corrected spelling of 'paraneterType' to 'parameterType'.
|
|
||
| // Track rewriting the return value for this method | ||
| emitState.TrackNativeParameterMethodRewrite( | ||
| paraneterType: keyType, |
There was a problem hiding this comment.
Corrected spelling of 'paraneterType' to 'parameterType'.
Suggested change
| paraneterType: keyType, | |
| parameterType: keyType, |
|
|
||
| // Add and implement the 'Lookup' method | ||
| mapViewMethodsType.AddMethodImplementation( | ||
| declaration: interopReferences.IMapViewMethodsImpl2HasKey(keyType, valueType).Import(module), |
There was a problem hiding this comment.
The 'Lookup' method is incorrectly implemented against the 'HasKey' interface declaration. This should use 'IMapViewMethodsImpl2Lookup' instead of 'IMapViewMethodsImpl2HasKey'.
Suggested change
| declaration: interopReferences.IMapViewMethodsImpl2HasKey(keyType, valueType).Import(module), | |
| declaration: interopReferences.IMapViewMethodsImpl2Lookup(keyType, valueType).Import(module), |
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.
Title. Also fixes some small bugs around the vtables for
IDictionary<,>types.Examples of codegen:
Note
Draft because there's some commits to move and this branch also needs to be rebased.