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

Make System.Reflection.Emit public types abstract #78544

Merged
merged 45 commits into from
Jan 27, 2023

Conversation

buyaa-n
Copy link
Contributor

@buyaa-n buyaa-n commented Nov 18, 2022

We plan to abstract out Reflection.Emit APIs for adding AssemblyBuilder.Save() in .NET Core. So that we can have two implementations, one that having old code to support all downlevel runtime, and a new implementation that replaces the current Reflection.Emit implementation with AssemblyBuilder.Save() support.

This PR is based on @jkotas prototype, but only covers the effort of abstracting all public APIs in System.Reflection.Emit.

Fixes #78542

@dotnet-issue-labeler
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, to 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.

@ghost ghost assigned buyaa-n Nov 18, 2022
@ghost
Copy link

ghost commented Nov 18, 2022

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

Issue Details

We plan to abstract out Reflection.Emit APIs for adding AssemblyBuilder.Save() in .NET Core. So that we can have two implementations, one that having old code to support all downlevel runtime, and a new implementation that replaces the current Reflection.Emit implementation with AssemblyBuilder.Save() support.

This PR is based on @jkotas prototype, but only covers the effort of abstracting all public APIs in System.Reflection.Emit.

Creating draft PR as the API proposal is not yet approved

Fixes #78542

Author: buyaa-n
Assignees: -
Labels:

area-System.Reflection, new-api-needs-documentation

Milestone: -

@ghost
Copy link

ghost commented Nov 18, 2022

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

Issue Details

We plan to abstract out Reflection.Emit APIs for adding AssemblyBuilder.Save() in .NET Core. So that we can have two implementations, one that having old code to support all downlevel runtime, and a new implementation that replaces the current Reflection.Emit implementation with AssemblyBuilder.Save() support.

This PR is based on @jkotas prototype, but only covers the effort of abstracting all public APIs in System.Reflection.Emit.

Creating draft PR as the API proposal is not yet approved

Fixes #78542

Author: buyaa-n
Assignees: buyaa-n
Labels:

area-System.Reflection.Emit, new-api-needs-documentation

Milestone: -

@ViktorHofer
Copy link
Member

@buyaa-n you forgot to also update the mono suppression file.

That said, with #78654 we can and should move the shared suppressions into the src/libraries/System.Private.CoreLib/src/CompatibilitySuppressions.xml file. That removes the duplication between the different CoreLib' suppression files.

src/coreclr/vm/ecalllist.h Outdated Show resolved Hide resolved
@vargaz
Copy link
Contributor

vargaz commented Jan 6, 2023

The mono changes look ok.

src/coreclr/vm/ecalllist.h Outdated Show resolved Hide resolved
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

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

Other than the filename changes, this LGTM.

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Jan 25, 2023

Update: Updated API proposal for using protected abstract APIs approved.

For the new ModuleBuilder APIs instead of GetMetdataToken() sugested:

GetTypeMetadataToken(Type type)
GetFieldMetadataToken(FieldInfo field)
GetMethodMetadataToken(MethodInfo method)
GetMethodMetadataToken(ConstructorInfo constructor)
GetSignatureMetadataToken(SignatureHelper signature)
GetStringMetadataToken(string stringConstant)

All suggested changes are applied to the PR

@steveharter
Copy link
Member

This can be removed in the main description:

Creating draft PR as #78542 is not yet approved

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Jan 27, 2023

The failures unrelated and reported: #75244 #81294, merging

@buyaa-n buyaa-n merged commit 93b487a into dotnet:main Jan 27, 2023
@buyaa-n buyaa-n deleted the reflection-emit branch January 27, 2023 20:36
@ghost ghost locked as resolved and limited conversation to collaborators Feb 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API Proposal]: Abstract System.Reflection.Emit public types
7 participants