dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.91k stars 4.63k forks source link

[API Proposal]: Add `AssemblyBuilder.ManifestModuleBuilder` and obsolete `AssemblyBuilder.DefineDynamicModule`. #67053

Closed teo-tsirpanis closed 1 year ago

teo-tsirpanis commented 2 years ago

Background and motivation

With multi-module assemblies not supported in modern .NET, the AssemblyBuilder.DefineDynamicModule has lost its reason of existence, and is a source of confusion over the module name it accepts as a parameter and whether it actually matters, while in fact is does not influence the dynamic assembly at all.

To further prove that the API is meaningless, try running the following:

var ab = AssemblyBuilder.DefineDynamicAssembly(new("Test"), AssemblyBuilderAccess.RunAndCollect);
Console.WriteLine(ab.DefineDynamicModule("foo").Name);

It will print <In Memory Module>.

To streamline the Reflection.Emit API and remove the concept of the one-to-many relation between assemblies and modules from the codebases and the things new developers have to learn, I propose to obsolete AssemblyBuilder.DefineDynamicModule (edit: and GetDynamicModule) and add a new property that returns the one and only ModuleBuilder of an AssemblyBuilder, without needing to explicitly give it a name.

API Proposal

namespace System.Reflection.Emit;

public sealed class AssemblyBuilder
{
    public ModuleBuilder ManifestModuleBuilder { get; }
    // existing methods
    [Obsolete("Multi-module assemblies are not supported. Use AssemblyBuilder.ManifestModuleBuilder instead.", DiagnosticId = "SYSLIB_NEXT", UrlFormat = "https://aka.ms/dotnet-warnings/{0}")]
    public ModuleBuilder DefineDynamicModule(string name);
    [Obsolete("Multi-module assemblies are not supported. Use AssemblyBuilder.ManifestModuleBuilder instead.", DiagnosticId = "SYSLIB_NEXT", UrlFormat = "https://aka.ms/dotnet-warnings/{0}")]
    public ModuleBuilder? GetDynamicModule(string name);
}

This property will return the dynamic assembly's module. It can be accessed multiple times. DefineDynamicModule will still check that the name parameter is valid and that it has been called only once, but it will always return ManifestModuleBuilder, without having any other effect.

If this API gets approved I can implement it.

API Usage

var ab = AssemblyBuilder.DefineDynamicAssembly(new("Test"), AssemblyBuilderAccess.RunAndCollect);
var mb = ab.ManifestModuleBuilder;

// ...

Alternative Designs

Risks

ghost commented 2 years ago

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

Issue Details
### Background and motivation With multi-module assemblies not supported in modern .NET, the `AssemblyBuilder.DefineDynamicModule` has lost its reason of existence, and is a source of confusion over the module name it accepts as a parameter and whether it actually matters, while in fact [is does not influence the dynamic assembly at all](https://github.com/dotnet/runtime/blob/a02b49a0486e92ace50803b977adf8c533ab118f/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/AssemblyBuilder.cs#L210-L237). To further prove that the API is meaningless, try running the following: ``` csharp var ab = AssemblyBuilder.DefineDynamicAssembly(new("Test"), AssemblyBuilderAccess.RunAndCollect); Console.WriteLine(ab.DefineDynamicModule("foo").Name); ``` It will print ``. To streamline the Reflection.Emit API and remove the concept of the one-to-many relation between assemblies and modules from the codebases and the things new developers have to learn, I propose to obsolete `AssemblyBuilder.DefineDynamicModule` and add a new property that returns the one and only `ModuleBuilder` of an `AssemblyBuilder`, without needing to explicitly give it a name. ### API Proposal ```C# namespace System.Reflection.Emit; public sealed class AssemblyBuilder { public ModuleBuilder ManifestModuleBuilder { get; } [Obsolete("Multi-module assemblies are not supported. Use AssemblyBuilder.ManifestModuleBuilder instead.", DiagnosticId = "SYSLIB_NEXT", UrlFormat = "https://aka.ms/dotnet-warnings/{0}")] // existing method public ModuleBuilder DefineDynamicModule(string name); } ``` This property will return the dynamic assembly's module. It can be accessed multiple times. `DefineDynamicModule` will still check that the `name` parameter is valid and that it has been called only once, but it will always return `ManifestModuleBuilder`, without having any other effect. If this API gets approved I can implement it. ### API Usage ```C# var ab = AssemblyBuilder.DefineDynamicAssembly(new("Test"), AssemblyBuilderAccess.RunAndCollect); var mb = ab.ManifestModuleBuilder; // ... ``` ### Alternative Designs * We could give it a different name like `ModuleBuilder` (akin to `FileStream.SafeFileHandle` property that has the same name as its type), or `MainModuleBuilder`. The name I proposed matches the existing `ManifestModule` property and there is little chance for the property to be overlooked; the obsoletion message will guide developers to it. * We could cement the one-to-one relationship between modules and assemblies by adding `DynamicModule`'s `Define***` methods to `DynamicAsembly`. That was my initial thought, but still an assembly and a module are distinct things (attributes can be applied to either of them for example), and it would make existing code slightly harder to migrate, or even present challenges for a library that multi-targets. My proposal is much easier to migrate to. ### Risks * Perhaps the `Reflection.Emit` APIs are mature enough to not warrant an essentially cosmetic change. * The module's name might still be useful to customize, especially when `AssemblyBuilder.Save` gets implemented.
Author: teo-tsirpanis
Assignees: -
Labels: `api-suggestion`, `area-System.Reflection.Emit`
Milestone: -
buyaa-n commented 2 years ago

public ModuleBuilder? GetDynamicModule(string name) could also obsoleted as its could only return the _manifestModuleBuilder when ManifestModuleName is passed https://github.com/dotnet/runtime/blob/25f2fb5bcdffc9108059a2da7f3ad25b4b48b064/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/AssemblyBuilder.cs#L285-L296

teo-tsirpanis commented 2 years ago

Thanks @buyaa-n, I didn't notice GetModuleBuilder, I edited the proposal. In fact that API is broken; it will only return the manifest module when the name parameter is equal to "RefEmit_InMemoryManifestModule", which no one could guess.

joperezr commented 2 years ago

Perhaps we will want to separate the API proposal from the obsoletion, mainly because the decision for marking the other API as obsolete might not make sense if we ever want to support AssemblyBuilder.Save. cc @jkotas

jkotas commented 2 years ago

The current .NET Core behavior looks like a bug that got introduced with stripping support for multimodule assemblies. If you run this:

var ab = AssemblyBuilder.DefineDynamicAssembly(new AssemblyName("Test"), AssemblyBuilderAccess.Run);
var mb = ab.DefineDynamicModule("foo");
Console.WriteLine(mb);

It is going to print "foo" on .NET Framework and RefEmit_InMemoryManifestModule on .NET Core.

Would it make more sense to fix DefineDynamicModule to respect the provided module name just like .NET Framework did instead of hardcoding an arbitrary one?

jkotas commented 2 years ago

To streamline the Reflection.Emit API and remove the concept of the one-to-many relation between assemblies and modules from the codebases and the things new developers have to learn

If we want to start obsoleting methods related to multi-module support, we should start with System.Reflection that is much more prevalent than System.Reflection.Emit.

buyaa-n commented 1 year ago

Closing as per the @jkotas's comment above