Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TypeMap API (Dynamic implementation) #113954

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

AaronRobinsonMSFT
Copy link
Member

See #113362

Definition of TypeMaps APIs in BCL
CoreCLR implementation of TypeMap APIs

Contributes to #110691

Definition of TypeMaps APIs in BCL
CoreCLR implementation of TypeMap APIs
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

1 similar comment
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

Copy link
Contributor

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

@AaronRobinsonMSFT
Copy link
Member Author

@AaronRobinsonMSFT AaronRobinsonMSFT marked this pull request as ready for review March 27, 2025 19:02
Copy link
Contributor

@Copilot 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.

Pull Request Overview

This PR implements the dynamic TypeMap API in the BCL with CoreCLR support and introduces new test cases to validate both external and proxy type mappings. Key changes include adding comprehensive tests in TypeMapApp.cs and related libraries, providing the API definitions and implementation in System.Runtime.InteropServices, and introducing lazy dictionaries for efficient type map creation.

Reviewed Changes

Copilot reviewed 11 out of 20 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/tests/Interop/TypeMap/TypeMapApp.cs Adds tests for external and proxy type mapping functionality
src/tests/Interop/TypeMap/Lib3.cs, Lib4.cs, GroupTypes.cs Provides supporting types and assembly attributes for type mapping
src/libraries/System.Runtime.InteropServices/ref/System.Runtime.InteropServices.cs Updates API definitions for TypeMap and related attributes
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/TypeMapping.cs Implements the core TypeMapping API using lazy initialization
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/TypeMapLazyDictionary.cs Implements lazy dictionaries for external and proxy type mappings
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/TypeMapAttribute*.cs and TypeMapAssemblyTargetAttribute.cs Adds attribute definitions for type mapping configuration
Files not reviewed (9)
  • src/coreclr/vm/assemblynative.cpp: Language not supported
  • src/coreclr/vm/assemblynative.hpp: Language not supported
  • src/coreclr/vm/qcallentrypoints.cpp: Language not supported
  • src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems: Language not supported
  • src/tests/Interop/TypeMap/TypeMapApp.csproj: Language not supported
  • src/tests/Interop/TypeMap/TypeMapLib1.csproj: Language not supported
  • src/tests/Interop/TypeMap/TypeMapLib2.csproj: Language not supported
  • src/tests/Interop/TypeMap/TypeMapLib3.csproj: Language not supported
  • src/tests/Interop/TypeMap/TypeMapLib4.csproj: Language not supported

[RequiresUnreferencedCode("Lazy TypeMap isn't supported for Trimmer scenarios")]
private sealed class LazyProxyTypeDictionary : LazyTypeLoadDictionary<Type>
{
// We don't include the assembly name for the hash code since it is not
Copy link
Member

Choose a reason for hiding this comment

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

The FullName can include assembly names for generic components. Do we need to filter out the assembly names for the generic components here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we do. That is a going to be a complication, I think.

@jkotas
Copy link
Member

jkotas commented Mar 28, 2025

Did you have a chance to check the perf of the type map building at runtime? What's the typical time and memory cost of a single entry?

Remove assembly name check on add.
Add test cases.
Instantiated generic type is currently broken.
Copy link
Contributor

@Copilot 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.

Pull Request Overview

This PR introduces a dynamic implementation of the TypeMap API within the BCL, together with corresponding tests and updates to supporting assemblies and reference libraries.

  • Added extensive tests in TypeMapApp.cs to validate both external and proxy type mappings.
  • Introduced new assembly attributes in Lib3.cs and Lib4.cs to facilitate mapping resolution across multiple assemblies and detect circular references.
  • Updated TypeMap attribute definitions and related TypeMapping implementation in System.Private.CoreLib and reference assemblies, with modifications in System.Reflection.Metadata to support the new API.

Reviewed Changes

Copilot reviewed 15 out of 23 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/tests/Interop/TypeMap/TypeMapApp.cs New tests and assembly attribute definitions for dynamic TypeMap API validation.
src/tests/Interop/TypeMap/Lib3.cs, Lib4.cs, Lib2.cs, GroupTypes.cs New type definitions and mapping assemblies to support the new API functionalities including circular reference checks.
src/libraries/System.Runtime.InteropServices/ref/System.Runtime.InteropServices.cs Updated public API definitions to include new TypeMap attributes and TypeMapping helper methods.
src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName*.cs Adjusted implementation details for type name parsing to align with dynamic type mapping.
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/* New implementations for TypeMapping and associated attributes aligned with the dynamic API design.
Files not reviewed (8)
  • src/coreclr/vm/assemblynative.cpp: Language not supported
  • src/coreclr/vm/assemblynative.hpp: Language not supported
  • src/coreclr/vm/qcallentrypoints.cpp: Language not supported
  • src/libraries/System.Private.CoreLib/src/Resources/Strings.resx: Language not supported
  • src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems: Language not supported
  • src/tests/Interop/TypeMap/TypeMapApp.csproj: Language not supported
  • src/tests/Interop/TypeMap/TypeMapLib1.csproj: Language not supported
  • src/tests/Interop/TypeMap/TypeMapLib2.csproj: Language not supported
Comments suppressed due to low confidence (1)

src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs:592

  • Replacing an empty immutable array with null here might lead to null reference issues if later code expects a non-null collection. Consider using ImmutableArray.Empty instead.
genericTypeArguments: null,

[DoesNotReturn]
internal static void ThrowInvalidOperation_NotSimpleName(string fullName)
{
// This message is defined in both System.Private.CoreLib and System.Reflection.Metadata
Copy link
Member

@jkotas jkotas Mar 31, 2025

Choose a reason for hiding this comment

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

The pattern used elsewhere is to throw an exception with full message under SYSTEM_REFLECTION_METADATA ifdef. E.g. check SYSTEM_REFLECTION_METADATA ifdef earlier in this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not entirely sure I get the utility of the "simple name" check. I would imagine users can place typeof(int[]) as the type, which seems like it would match the notion of "array" mentioned in the check for IsSimple in the TypeName class.

Copy link
Member

@jkotas jkotas Mar 31, 2025

Choose a reason for hiding this comment

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

The method will replace the assembly name for the top level TypeName only. It does not produce the expected result for non-simple names - that's why it throws, and it is not clear what the expected result should be once you get to generics.

{
public void* Utf8TypeName { get; init; }
public int Utf8TypeNameLen { get; init; }
public void* Utf8AssemblyNameFallback { get; init; }
Copy link
Member

@jkotas jkotas Mar 31, 2025

Choose a reason for hiding this comment

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

Instead of storing assembly name, it may be better to just store RuntimeAssembly here and then call TypeNameResolver.GetTypeHelper to resolve the type. TypeNameResolver.GetTypeHelper is the method that is used to resolve type name in attributes and it will substitute the assembly name as needed. We won't need to worry about substituting the assembly name here or making sure that it is done exactly same way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants