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] Make System.Reflection.Emit.ILGenerator constructor protected #86110

Closed
buyaa-n opened this issue May 11, 2023 · 2 comments · Fixed by #86594
Closed

[API Proposal] Make System.Reflection.Emit.ILGenerator constructor protected #86110

buyaa-n opened this issue May 11, 2023 · 2 comments · Fixed by #86594
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 May 11, 2023

For new persisting AssemblyBuilder.Save implementation we have abstracted out all other Reflection.Emit public APIs with #78542, similar treatment needed for ILGenerator in order to save IL using MetadataBuilder. Literally all public methods of this type are virtual, but the constructor is internal, so I could not implement it outside of CoreLib

namespace System.Reflection.Emit
{
-   public partial class ILGenerator
+   public abstract class ILGenerator // there is no abstract method, probably make it abstract to avoid creating an instance. 
    {
-       internal ILGenerator() { }
+       protected ILGenerator() { }
        public virtual int ILOffset { get { throw null; } }
        public virtual void BeginCatchBlock(System.Type? exceptionType) { }
        public virtual void BeginExceptFilterBlock() { }
        // ... All public APIs a virtual except one, we can use the `byte arg` overload for this
        [System.CLSCompliantAttribute(false)]
        public void Emit(System.Reflection.Emit.OpCode opcode, sbyte arg) { }
    }
}

Contributes to #62956

CC @AaronRobinsonMSFT @jkotas @steveharter

@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 11, 2023
@ghost
Copy link

ghost commented May 11, 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

For new persisting AssemblyBuilder.Save implementation we have abstracted out all other Reflection.Emit public APIs with #78542, similar treatment needed for ILGenerator in order to save IL using MetadataBuilder. Literally all public methods of this type are virtual, but the constructor is internal, so I could not implement it outside of CoreLib

namespace System.Reflection.Emit
{
-   public partial class ILGenerator
+   public abstract class ILGenerator // there is no abstract method, probably make abstract to avoid creating an instance 
    {
-       internal ILGenerator() { }
+       protected ILGenerator() { }
        public virtual int ILOffset { get { throw null; } }
        public virtual void BeginCatchBlock(System.Type? exceptionType) { }
        public virtual void BeginExceptFilterBlock() { }
        // ... All public APIs a virtual except one, we can use the `byte arg` overload for this
        [System.CLSCompliantAttribute(false)]
        public void Emit(System.Reflection.Emit.OpCode opcode, sbyte arg) { }
    }
}

CC @AaronRobinsonMSFT @jkotas @steveharter

Author: buyaa-n
Assignees: -
Labels:

area-System.Reflection.Emit

Milestone: -

@buyaa-n buyaa-n added this to the 8.0.0 milestone May 11, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label May 11, 2023
@buyaa-n buyaa-n added the api-ready-for-review API is ready for review, it is NOT ready for implementation label May 11, 2023
@buyaa-n buyaa-n changed the title Make System.Reflection.Emit.ILGenerator constructor protected [API Proposal] Make System.Reflection.Emit.ILGenerator constructor protected May 11, 2023
@bartonjs
Copy link
Member

Looks good as proposed.

The protected constructor and changing the type to abstract suggests that some of the virtual members should themselves flip to abstract (or that there's a protected abstract missing from the design). As a general reminder, abstract methods can't be added after the initial release where it becomes possible to actually extend the type, so for ILGenerator that means the current (.NET 8) release.

namespace System.Reflection.Emit
{
-   public partial class ILGenerator
+   public abstract partial class ILGenerator
    {
-       internal ILGenerator() { }
+       protected ILGenerator() { }
    }
}

@bartonjs bartonjs 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 May 11, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 22, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 23, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 22, 2023
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

Successfully merging a pull request may close this issue.

2 participants