[API Proposal]: Add protected factory method for creating Label in ILGenerator, abstract out LocalBuilder

[buyaa-n]

Background and motivation

For adding AssemblyBuilder.Save() equivalent in .NET Core we have abstracted most of the System.Refleciton.Emit types and adding new implementations for them. This abstraction did not cover Label and LocalBuilder types and its constructors are internal, so I could not create an instance from the new ILGenerator implementation.

API Proposal

• For LocalBuilder we need to unseal the LocalBuilder class and make it abstract, make the internal constructor protected, also add a public getter that exposes the method the local belongs to.

  namespace System.Reflection.Emit;
  
  -public sealed class LocalBuilder : System.Reflection.LocalVariableInfo
  +public abstract class LocalBuilder : System.Reflection.LocalVariableInfo
  {
  -   internal LocalBuilder(int localIndex, Type localType, MethodInfo method, bool isPinned) { }
  +   protected LocalBuilder() { }
  +   public abstract MethodInfo LocalMethod { get; }
  }

  Abstract LocalMethod property can be used for validation. Parent LocalVariableInfo has public virtual properties like LocalType, LocalIndex, IsPinned.

• For Label struct we need a protected factory method in the abstract ILGenerator type so that we can create an instance from derived types.

  namespace System.Reflection.Emit;
  
  public abstract class ILGenerator
  {
      protected ILGenerator() { }
      ...
      public abstract Label DefineLabel();
  +   protected static Label CreateLabel(int id) { throw null; }
  }
  
  public readonly partial struct Label : System.IEquatable<System.Reflection.Emit.Label>
  {
      ...
  +   public int Id { get; }
  }

API Usage

internal sealed class ILGeneratorImpl : ILGenerator
{
    public override Label DefineLabel()
    {
        LabelHandle metadataLabel = _il.DefineLabel();
        Label emitLabel = CreateLabel(metadataLabel.Id);
        _labelTable.Add(emitLabel, metadataLabel);
        return emitLabel;
    }
}

Related to #92975

CC @steveharter

[ghost]

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

For adding AssemblyBuilder.Save() equivalent in .NET Core we have abstracted most of the System.Refleciton.Emit types and adding new implementations for them. This abstraction did not cover Label, LocalBuilder types and their constructors are internal, so I could not create an instance from the new ILGenerator implementation. We need a protected factory methods that create these instances in the abstract ILGenerator type so that we can call theme derived type.

Related to #92975

CC @steveharter

API Proposal

namespace System.Reflection.Emit;

public abstract class ILGenerator
{
    protected ILGenerator() { }
    ....
+   protected Label CreateLabel(int id) { throw null; }
+   protected LocalBuilder CreateLocal(int localIndex, Type localType, MethodInfo methodBuilder, bool isPinned) { throw null; }
}

API Usage

internal sealed class ILGeneratorImpl : ILGenerator
{
    public override Label DefineLabel()
    {
        LabelHandle metadataLabel = _il.DefineLabel();
        Label emitLabel = CreateLabel(metadataLabel.Id);
        _labelTable.Add(emitLabel, metadataLabel);
        return emitLabel;
    }
}

Alternative Designs

No response

Risks

No response

Author:	buyaa-n
Assignees:	-
Labels:	api-suggestion, area-System.Reflection.Emit
Milestone:	-

[teo-tsirpanis]

How about we add a way to get the ID of a label, either as an Id property of Label, or (preferably) as a protected int GetLabelId(Label label); method on ILGenerator?

[buyaa-n]

How about we add a way to get the ID of a label, either as an Id property of Label, or (preferably) as a protected int GetLabelId(Label label); method on ILGenerator?

That can be useful for handling labels, I think it should be added to the Label struct.

[teo-tsirpanis]

How about we make CreateLabel static?

- protected Label CreateLabel(int id) { throw null; }
+ protected static Label CreateLabel(int id) { throw null; }

[buyaa-n]

How about we make CreateLabel static?

Updated, thanks!

[bartonjs]

Video

There was discussion around LocalBuilder.LocalMethod being an abstract property or a value passed in via the constructor. Since everything else in the hierarchy is abstract properties, it is locally consistent as proposed.

Since the method isn't a feature of the local, but instead describes the context where it exists, the property should be Method not LocalMethod

namespace System.Reflection.Emit;

-public sealed class LocalBuilder : System.Reflection.LocalVariableInfo
+public abstract class LocalBuilder : System.Reflection.LocalVariableInfo
{
-   internal LocalBuilder(int localIndex, Type localType, MethodInfo method, bool isPinned) { }
+   protected LocalBuilder() { }
+   public abstract MethodInfo Method { get; }
}

public abstract class ILGenerator
{
    protected ILGenerator() { }
    ...
    public abstract Label DefineLabel();
+   protected static Label CreateLabel(int id) { throw null; }
}

public readonly partial struct Label : System.IEquatable<System.Reflection.Emit.Label>
{
    ...
+   public int Id { get; }
}

[lambdageek]

This API seems wrong to me:

public abstract MethodInfo Method { get; }

I think this needs to be MethodBase, not MethodInfo. Constructors can have locals, too

[buyaa-n]

@bartonjs and @dotnet/fxdc there were push back on adding MethodInfo Method property for LocalBuilder, mono implementation uses ILGenerator instead of the Method the local belongs to. We had some discussion in the PR and experts suggesting to keep this implementation detail internal. Sorry for the inconvenience and I ask to remove the property from the approved API

-   public abstract MethodInfo Method { get; }

[buyaa-n]

Confirmed that removing the new Property is OK, no need any other action