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

[API Proposal] Add persist able AssemblyBuilder implementation #83988

Closed
buyaa-n opened this issue Mar 27, 2023 · 14 comments
Closed

[API Proposal] Add persist able AssemblyBuilder implementation #83988

buyaa-n opened this issue Mar 27, 2023 · 14 comments
Labels
api-approved API was approved in API review, it can be implemented area-System.Reflection.Emit
Milestone

Comments

@buyaa-n
Copy link
Contributor

buyaa-n commented Mar 27, 2023

Contributes to Support equivalent of AssemblyBuilder.Save to save in-memory IL to an assembly

As per #62956 we plan to a add managed implementation of AssemblyBuilder that could save in-memory IL to an assembly file/stream.

Proposed design 1

Initial version would only support Save, no Run, therefore the factory method not accepting AssemblyBuilderAccess that defines the access mode for the assembly. It is the current prototype design.

namespace System.Reflection.Emit
{
+    public sealed class AssemblyFileBuilder : AssemblyBuilder
+    {
+        internal AssemblyFileBuilder(AssemblyName name, IEnumerable<CustomAttributeBuilder>? assemblyAttributes) { }

         // New static methods without AssemblyBuilderAccess, because initial version will only support Save
+        public static AssemblyFileBuilder DefineDynamicAssembly(AssemblyName name) { throw null; }
+        public static AssemblyFileBuilder DefineDynamicAssembly(AssemblyName name, Enumerable<CustomAttributeBuilder>? assemblyAttributes) { throw null; }
	
        // New instance methods
+        public void Save(Stream stream) { }
+        public void Save(string assemblyFileName) { } // same as in .NET Framework
+    }
}

Usage example

using System.Reflection.Emit;

public class MyType
{
    public void MyMethod(AssemblyName assemblyName, string fileName)
    {
	AssemblyFileBuilder builder = AssemblyFileBuilder.DefineDynamicAssembly(assemblyName);
	// ... Add module and other members 
	builder.Save(fileName);
    }
}

Alternative design 2:

The new persisted AssemblyBuilder type implementation will not exposed, instead we add Save into the abstract class AssemblyBuilder and protected abstract SaveCore method for implementation. Use the same factory method for creating AssemblyBuilder. This need for using reflection in order to create persisted AssemblyBuilder.

namespace System.Reflection.Emit
{
    public abstract class AssemblyBuilder : Assembly
    {
+        public void Save(Stream stream) { }
+        public void Save(string assemblyFileName) { }
+        protected abstract void SaveCore(Stream stream);
    }

    [Flags]
    public enum AssemblyBuilderAccess
    {
        Run = 1,
+	Save = 2,
        RunAndCollect = 8 | Run,
    }
}

Usage example

using System.Reflection.Emit;

public class MyType
{
    public void MyMethod(AssemblyName assemblyName, string fileName)
    {
	AssemblyBuilder builder = AssemblyBuilder.DefineDynamicAssembly(assemblyName, AssemblyBuilderAccess.Save);
	// ... Add module and other members 
	builder.Save(fileName);
    }
}

Alternative design 3:

Comment by @jkotas for above Alternative design 2:

  • Supporting both the runtime and persisted assemblies through the same API creates a choke-point for trimming and AOT. Save mode that somebody may want to use to build a compiler is compatible with trimming and AOT. The dynamic time mode is not compatible with AOT, and brings in additional runtime support. We should have two distinct APIs for the dynamic and persistent modes.
  • The core assembly should be passed in as Assembly to avoid ambiguity about what kind of name it is.

With these two points incorporated, the design would be:

 class AssemblyBuilder
 {
     // Existing APIs
     [RequiresDynamicCode("Defining a dynamic assembly requires dynamic code.")]
     public static AssemblyBuilder DefineDynamicAssembly(AssemblyName name, AssemblyBuilderAccess access);
     [RequiresDynamicCode("Defining a dynamic assembly requires dynamic code.")]
     public static AssemblyBuilder DefineDynamicAssembly(AssemblyName name, AssemblyBuilderAccess access, System.Collections.Generic.IEnumerable<CustomAttributeBuilder>? assemblyAttributes);

+    // New API - note that it does not have RequiresDynamicCode annotation
+    public static AssemblyBuilder DefinePersistedAssembly(AssemblyName name, Assembly coreAssembly, IEnumerable<CustomAttributeBuilder>? assemblyAttributes = null);

+    public void Save(Stream stream) { }
+    public void Save(string assemblyFileName) { } // same as in .NET Framework
+    protected abstract void SaveCore(Stream stream);
 }

AssemblyBuilderAccess is not used in this design because it will only support "Save" initially and eventually we allow Run without requiring to specify Save/Run explicitly.

Usage example

using System.Reflection.Emit;

public class MyType
{
    public void MyMethod(AssemblyName assemblyName, string fileName)
    {
	AssemblyBuilder builder = AssemblyBuilder.DefinePersistedAssembly(assemblyName, AssemblyBuilderAccess.Save);
	// ... Add module and other members 
	builder.Save(fileName);
    }
}
@buyaa-n buyaa-n added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 27, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 27, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Mar 27, 2023
@ghost
Copy link

ghost commented Mar 27, 2023

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

Issue Details

Contributes to Support equivalent of AssemblyBuilder.Save to save in-memory IL to an assembly

As per #62956 we plan to a add managed implementation of AssemblyBuilder that could save in-memory IL to an assembly file/stream.

Initial version would only support Save, no Run, therefore the factory method not accepting AssemblyBuilderAccess that defines the access mode for the assembly.

namespace System.Reflection.Emit
{
+    public sealed class PersistableAssemblyBuilder : AssemblyBuilder
+    {
+        internal PersistableAssemblyBuilder (AssemblyName name, IEnumerable<CustomAttributeBuilder>? assemblyAttributes) { }

         // New static methods without AssemblyBuilderAccess, because initial version will only support Save
+        public static PersistableAssemblyBuilder DefineDynamicAssembly(AssemblyName name) { throw null; }
+        public static PersistableAssemblyBuilder DefineDynamicAssembly(AssemblyName name, Enumerable<CustomAttributeBuilder>? assemblyAttributes) { throw null; }
	
        // New instance methods
+        public void Save(Stream stream) { }
+        public void Save(string assemblyFileName) { } // same as in .NET Framework
+    }
}

Usage example

using System.Reflection.Emit;

public class MyType
{
    public void MyMethod(AssemblyName assemblyName, string fileName)
    {
	PersistableAssemblyBuilder builder = PersistableAssemblyBuilder.DefineDynamicAssembly(assemblyName);
	// ... Add module and other members 
	builder.Save(fileName);
    }
}

Alternative design

Support for AssemblyBuilderAccess.Run will be added to this type in the future. Therefore we might want to AssemblyBuilderAccess.Save option and use it for existing factory methods.

namespace System.Reflection.Emit
{
+    public sealed class PersistableAssemblyBuilder : AssemblyBuilder
+    {
+        internal PersistableAssemblyBuilder (AssemblyName name, IEnumerable<CustomAttributeBuilder>? assemblyAttributes) { }

+        public static new PersistableAssemblyBuilder DefineDynamicAssembly(AssemblyName name, AssemblyBuilderAccess access) { throw null; }
+        public static new PersistableAssemblyBuilder DefineDynamicAssembly(AssemblyName name, AssemblyBuilderAccess access, Enumerable<CustomAttributeBuilder>? assemblyAttributes) { throw null; }
	
        // New instance methods
+        public void Save(Stream stream) { }
+        public void Save(string assemblyFileName) { } // same as in .NET Framework
+    }

    [Flags]
    public enum AssemblyBuilderAccess
    {
        Run = 1,
+	Save = 2,
        RunAndCollect = 8 | Run,
    }
}

Usage example

using System.Reflection.Emit;

public class MyType
{
    public void MyMethod(AssemblyName assemblyName, string fileName)
    {
	PersistableAssemblyBuilder builder = PersistableAssemblyBuilder.DefineDynamicAssembly(assemblyName, AssemblyBuilderAccess.Save);
	// ... Add module and other members 
	builder.Save(fileName);
    }
}
Author: buyaa-n
Assignees: -
Labels:

api-suggestion, area-System.Reflection, untriaged, needs-area-label

Milestone: -

@vcsjones vcsjones added area-System.Reflection.Emit and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Mar 27, 2023
@ghost
Copy link

ghost commented Mar 27, 2023

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

Issue Details

Contributes to Support equivalent of AssemblyBuilder.Save to save in-memory IL to an assembly

As per #62956 we plan to a add managed implementation of AssemblyBuilder that could save in-memory IL to an assembly file/stream.

Initial version would only support Save, no Run, therefore the factory method not accepting AssemblyBuilderAccess that defines the access mode for the assembly.

namespace System.Reflection.Emit
{
+    public sealed class PersistableAssemblyBuilder : AssemblyBuilder
+    {
+        internal PersistableAssemblyBuilder (AssemblyName name, IEnumerable<CustomAttributeBuilder>? assemblyAttributes) { }

         // New static methods without AssemblyBuilderAccess, because initial version will only support Save
+        public static PersistableAssemblyBuilder DefineDynamicAssembly(AssemblyName name) { throw null; }
+        public static PersistableAssemblyBuilder DefineDynamicAssembly(AssemblyName name, Enumerable<CustomAttributeBuilder>? assemblyAttributes) { throw null; }
	
        // New instance methods
+        public void Save(Stream stream) { }
+        public void Save(string assemblyFileName) { } // same as in .NET Framework
+    }
}

Usage example

using System.Reflection.Emit;

public class MyType
{
    public void MyMethod(AssemblyName assemblyName, string fileName)
    {
	PersistableAssemblyBuilder builder = PersistableAssemblyBuilder.DefineDynamicAssembly(assemblyName);
	// ... Add module and other members 
	builder.Save(fileName);
    }
}

Alternative design

Support for AssemblyBuilderAccess.Run will be added to this type in the future. Therefore we might want to AssemblyBuilderAccess.Save option and use it for existing factory methods.

namespace System.Reflection.Emit
{
+    public sealed class PersistableAssemblyBuilder : AssemblyBuilder
+    {
+        internal PersistableAssemblyBuilder (AssemblyName name, IEnumerable<CustomAttributeBuilder>? assemblyAttributes) { }

+        public static new PersistableAssemblyBuilder DefineDynamicAssembly(AssemblyName name, AssemblyBuilderAccess access) { throw null; }
+        public static new PersistableAssemblyBuilder DefineDynamicAssembly(AssemblyName name, AssemblyBuilderAccess access, Enumerable<CustomAttributeBuilder>? assemblyAttributes) { throw null; }
	
        // New instance methods
+        public void Save(Stream stream) { }
+        public void Save(string assemblyFileName) { } // same as in .NET Framework
+    }

    [Flags]
    public enum AssemblyBuilderAccess
    {
        Run = 1,
+	Save = 2,
        RunAndCollect = 8 | Run,
    }
}

Usage example

using System.Reflection.Emit;

public class MyType
{
    public void MyMethod(AssemblyName assemblyName, string fileName)
    {
	PersistableAssemblyBuilder builder = PersistableAssemblyBuilder.DefineDynamicAssembly(assemblyName, AssemblyBuilderAccess.Save);
	// ... Add module and other members 
	builder.Save(fileName);
    }
}
Author: buyaa-n
Assignees: -
Labels:

api-suggestion, area-System.Reflection, area-System.Reflection.Emit, untriaged

Milestone: -

@buyaa-n buyaa-n added this to the 8.0.0 milestone Mar 27, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Mar 27, 2023
@jkotas
Copy link
Member

jkotas commented Mar 27, 2023

Alternative design

Another alternative is add Save methods and abstract SaveCore method on the AssemblyBuilder.

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Mar 27, 2023

Another alternative is add Save methods and abstract SaveCore method on the AssemblyBuilder.

Are you suggesting adding AssemblyBuilderAccess.Save option and creating appropriate instance of AssemblyBuilder implementation with AssemblyBuilder.DefineDynamicAssembly(assemblyName, AssemblyBuilderAccess.Save);:

namespace System.Reflection.Emit
{
    public abstract class AssemblyBuilder : Assembly
    {
+        public void Save(Stream stream) { }
+        public void Save(string assemblyFileName) { } // same as in .NET Framework
+        protected abstract void SaveCore(Stream stream);
    }

    [Flags]
    public enum AssemblyBuilderAccess
    {
        Run = 1,
+	Save = 2,
        RunAndCollect = 8 | Run,
    }
}

Usage example

using System.Reflection.Emit;

public class MyType
{
    public void MyMethod(AssemblyName assemblyName, string fileName)
    {
	AssemblyBuilder builder = AssemblyBuilder.DefineDynamicAssembly(assemblyName, AssemblyBuilderAccess.Save);
	// ... Add module and other members 
	builder.Save(fileName);
    }
}

The problem with that design is it would need reflection in order to create persist able AssemblyBuilder instance as it will not be added in CoreLib. What you think about this @jkotas?

@buyaa-n buyaa-n added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Mar 27, 2023
@jkotas
Copy link
Member

jkotas commented Mar 27, 2023

In the fulness of time, we want to use the managed assembly builder for both the persistent and non-persistent modes. We will need reflection for the non-persisted mode since the API for that is in CoreLib already. I do not see a problem with using reflection for the persistent mode as well.

For the persistent mode, we may want to have argument that specifies the core assembly - to address https://github.com/dotnet/runtime/pull/83554/files#diff-c12b00ae2e7f16430a305235a8e8ea053806a0d3fc4b15ea4e18f5c0cb904cfaR40 TODO, similar to how MetadataLoadContext takes coreAssemblyName argument.

@buyaa-n buyaa-n added the blocking Marks issues that we want to fast track in order to unblock other important work label Mar 27, 2023
@buyaa-n
Copy link
Contributor Author

buyaa-n commented Mar 28, 2023

In the fulness of time, we want to use the managed assembly builder for both the persistent and non-persistent modes. We will need reflection for the non-persisted mode since the API for that is in CoreLib already.

Yes, reflection needed for persistent mode only

I do not see a problem with using reflection for the persistent mode as well.

OK, adding this approach as an alternative design

For the persistent mode, we may want to have argument that specifies the core assembly - to address https://github.com/dotnet/runtime/pull/83554/files#diff-c12b00ae2e7f16430a305235a8e8ea053806a0d3fc4b15ea4e18f5c0cb904cfaR40 TODO, similar to how MetadataLoadContext takes coreAssemblyName argument.

Sounds good, if that is the case, we can add overloads to DefineDynamicAssembly for that:

public abstract class AssemblyBuilder : Assembly
{
+     public static AssemblyBuilder DefineDynamicAssembly(AssemblyName name, AssemblyBuilderAccess access, string? coreAssemblyName = null) { throw null; }
+     public static AssemblyBuilder DefineDynamicAssembly(AssemblyName name, AssemblyBuilderAccess access, Enumerable<CustomAttributeBuilder>? assemblyAttributes, string? coreAssemblyName = null) { throw null; }
}

@jkotas
Copy link
Member

jkotas commented Mar 28, 2023

  • Suporting both the runtime and persisted assemblies through the same API creates a choke-point for trimming and AOT. Save mode that somebody may want to use to build a compiler is compatible with trimming and AOT. The dynamic time mode is not compatible with AOT, and brings in additional runtime support. We should have two distinct APIs for the dynamic and persistent modes.
  • The core assembly should be passed in as Assembly to avoid ambiguity about what kind of name it is.

With these two points incorporated, the design would be:

 class AssemblyBuilder
 {
     // Existing APIs
     [RequiresDynamicCode("Defining a dynamic assembly requires dynamic code.")]
     public static AssemblyBuilder DefineDynamicAssembly(AssemblyName name, AssemblyBuilderAccess access);
     [RequiresDynamicCode("Defining a dynamic assembly requires dynamic code.")]
     public static AssemblyBuilder DefineDynamicAssembly(AssemblyName name, AssemblyBuilderAccess access, System.Collections.Generic.IEnumerable<CustomAttributeBuilder>? assemblyAttributes);

+    // New API - note that it does not have RequiresDynamicCode annotation
+    public static AssemblyBuilder DefinePersistedAssembly(AssemblyName name, Assembly coreAssembly, IEnumerable<CustomAttributeBuilder>? assemblyAttributes = null);

+    public void Save(Stream stream) { }
+    public void Save(string assemblyFileName) { } // same as in .NET Framework
+    protected abstract void SaveCore(Stream stream);
 }

@stephentoub
Copy link
Member

With these two points incorporated, the design would be

That looks nice and simple / streamlined.

@steveharter
Copy link
Member

With both factory APIs (DefineDynamicAssembly and DefinePersistedAssembly) returning the same base class AssemblyBuilder, plus the need to use reflection to instantiate the derived\concrete builder class, we need to verify trimming still works.

@jkotas
Copy link
Member

jkotas commented Mar 28, 2023

I do not see any trimming problems with that design. We just need to follow the patterns recognized by the linker.

@steveharter
Copy link
Member

I do not see any trimming problems with that design. We just need to follow the patterns recognized by the linker.

Per offline discussion, as long as a string literal is used with Type.GetType(""), the linker will work as expected:

Type.GetType("System.Reflection.Emit.PersistableAssemblyBuilder, System.Reflection.Emit")

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Mar 28, 2023

Thank you for your feedbacks! Updated the description accordingly.

@terrajobst
Copy link
Member

terrajobst commented Mar 28, 2023

Video

  • Looks good as proposed
namespace System.Reflection.Emit;

public partial class AssemblyBuilder
{
    // Existing APIs
    // [RequiresDynamicCode("Defining a dynamic assembly requires dynamic code.")]
    // public static AssemblyBuilder DefineDynamicAssembly(AssemblyName name, AssemblyBuilderAccess access);
    // [RequiresDynamicCode("Defining a dynamic assembly requires dynamic code.")]
    // public static AssemblyBuilder DefineDynamicAssembly(AssemblyName name, AssemblyBuilderAccess access, System.Collections.Generic.IEnumerable<CustomAttributeBuilder>? assemblyAttributes);

    // New API - note that it does not have RequiresDynamicCode annotation
    public static AssemblyBuilder DefinePersistedAssembly(AssemblyName name, Assembly coreAssembly, IEnumerable<CustomAttributeBuilder>? assemblyAttributes = null);

    public void Save(Stream stream);
    public void Save(string assemblyFileName);
    protected abstract void SaveCore(Stream stream);
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Mar 28, 2023
@buyaa-n buyaa-n removed the blocking Marks issues that we want to fast track in order to unblock other important work label Mar 28, 2023
@buyaa-n
Copy link
Contributor Author

buyaa-n commented Feb 6, 2024

Fixed with #97177

@buyaa-n buyaa-n closed this as completed Feb 6, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Mar 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Reflection.Emit
Projects
None yet
Development

No branches or pull requests

6 participants