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]: Support ref fields in System.Reflection.Metadata.Ecma335.BlobEncoder #68309

Closed
cston opened this issue Apr 21, 2022 · 10 comments · Fixed by #69378
Closed

[API Proposal]: Support ref fields in System.Reflection.Metadata.Ecma335.BlobEncoder #68309

cston opened this issue Apr 21, 2022 · 10 comments · Fixed by #69378
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Reflection.Metadata
Milestone

Comments

@cston
Copy link
Member

cston commented Apr 21, 2022

Background and motivation

Provide methods in System.Reflection.Metadata.Ecma335.BlobEncoder to more directly support emitting ref fields, consistent with the corresponding APIs provided for emitting ref parameters and ref locals.

API Proposal

Provide a FieldTypeEncoder type similar to ParameterTypeEncoder and LocalVariableTypeEncoder.

namespace System.Reflection.Metadata.Ecma335
{
    public readonly struct BlobEncoder
    {
        public FieldTypeEncoder FieldTypeEncoder();
    }

    public readonly struct FieldTypeEncoder
    {
        public BlobBuilder Builder { get; }

        public FieldTypeEncoder(BlobBuilder builder);

        public CustomModifiersEncoder CustomModifiers();

        public SignatureTypeEncoder Type(bool isByRef = false);

        public void TypedReference();
    }
}

API Usage

var blobBuilder = new BlobBuilder();
var encoder = new BlobEncoder(blobBuilder);

var fieldEncoder = encoder.FieldTypeEncoder();

// Ref custom modifiers: modopt(object)
var customModifiersEncoder = fieldEncoder.CustomModifiers();
customModifiersEncoder.AddModifier(MetadataTokens.TypeReferenceHandle(2), isOptional: true);

// Type: int&
var typeEncoder = fieldEncoder.Type(isByRef: true);
typeEncoder.PrimitiveType(PrimitiveTypeCode.Int32);

Alternative Designs

The alternative is the existing API which requires emitting SignatureTypeCode.ByReference and creating a CustomModifierEncoder explicitly. The existing API is sufficient, but inconsistent with the support for ref parameters and ref locals.

Using the existing API:

var blobBuilder = new BlobBuilder();
var encoder = new BlobEncoder(blobBuilder);

var typeEncoder = encoder.FieldSignature();

// Ref custom modifiers: modopt(object)
var customModifiersEncoder = new CustomModifiersEncoder(blobBuilder);
customModifiersEncoder.AddModifier(MetadataTokens.TypeReferenceHandle(2), isOptional: true);

// Type: int&
typeEncoder.Builder.WriteByte((byte)SignatureTypeCode.ByReference);
typeEncoder.PrimitiveType(PrimitiveTypeCode.Int32);

Risks

BlobEncoder already includes a FieldSignature method that returns an encoder for a field type, so having two distinct field encoder methods may be confusing.

    public readonly struct BlobEncoder
    {
        public SignatureTypeEncoder FieldSignature();
        public FieldTypeEncoder FieldTypeEncoder();
    }
@cston cston added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Apr 21, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Reflection.Metadata untriaged New issue has not been triaged by the area owner labels Apr 21, 2022
@ghost
Copy link

ghost commented Apr 21, 2022

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

Issue Details

Background and motivation

Provide methods in System.Reflection.Metadata.Ecma335.BlobEncoder to more directly support emitting ref fields, consistent with the corresponding APIs provided for emitting ref parameters and ref locals.

API Proposal

Provide a FieldTypeEncoder type similar to ParameterTypeEncoder and LocalVariableTypeEncoder.

namespace System.Reflection.Metadata.Ecma335
{
    public readonly struct BlobEncoder
    {
        public FieldTypeEncoder FieldTypeEncoder();
    }

    public readonly struct FieldTypeEncoder
    {
        public BlobBuilder Builder { get; }

        public FieldTypeEncoder(BlobBuilder builder);

        public CustomModifiersEncoder CustomModifiers();

        public SignatureTypeEncoder Type(bool isByRef = false);
    }
}

API Usage

var blobBuilder = new BlobBuilder();
var encoder = new BlobEncoder(blobBuilder);

var fieldEncoder = encoder.FieldTypeEncoder();

// Ref custom modifiers: modopt(object)
var customModifiersEncoder = fieldEncoder.CustomModifiers();
customModifiersEncoder.AddModifier(MetadataTokens.TypeReferenceHandle(2), isOptional: true);

// Type: int&
var typeEncoder = fieldEncoder.Type(isByRef: true);
typeEncoder.PrimitiveType(PrimitiveTypeCode.Int32);

Alternative Designs

The alternative is the existing API which requires emitting SignatureTypeCode.ByReference and creating a CustomModifierEncoder explicitly. The existing API is sufficient, but inconsistent with the support for ref parameters and ref locals.

Using the existing API:

var blobBuilder = new BlobBuilder();
var encoder = new BlobEncoder(blobBuilder);

// Ref custom modifiers: modopt(object)
var customModifiersEncoder = new CustomModifiersEncoder(blobBuilder);
customModifiersEncoder.AddModifier(MetadataTokens.TypeReferenceHandle(2), isOptional: true);

// Type: int&
var typeEncoder = encoder.FieldSignature();
typeEncoder.Builder.WriteByte((byte)SignatureTypeCode.ByReference);
typeEncoder.PrimitiveType(PrimitiveTypeCode.Int32);

Risks

No response

Author: cston
Assignees: -
Labels:

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

Milestone: -

@cston
Copy link
Member Author

cston commented Apr 21, 2022

cc @jaredpar, @tmat

@jaredpar
Copy link
Member

@AaronRobinsonMSFT

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Apr 21, 2022

@cston Will the following be needed with this new API. It is in the usage example but is also noted in the Alternative design in a manner that implies it wouldn't be needed if this new API was adopted.

// Ref custom modifiers: modopt(object)
var customModifiersEncoder = fieldEncoder.CustomModifiers();
customModifiersEncoder.AddModifier(MetadataTokens.TypeReferenceHandle(2), isOptional: true);

Edit Nevermind. A new instance is being created rather than using the existing one in the fieldEncoder instance.

@cston
Copy link
Member Author

cston commented Apr 21, 2022

A new instance is being created rather than using the existing one in the fieldEncoder instance.

Actually, in both examples an instance of CustomModifiersEncoder is created. The only difference is that the proposed FieldTypeEncoder type includes a CustomModifiers() method that makes it slightly more obvious how to obtain a CustomModifiersEncoder.

The overall difference between use of the existing API and the proposed API is small. It's perhaps just a question of whether the API for emitting fields should be aligned with the existing APIs for locals and parameters.

@AaronRobinsonMSFT AaronRobinsonMSFT added blocking Marks issues that we want to fast track in order to unblock other important work and removed untriaged New issue has not been triaged by the area owner labels Apr 27, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 7.0.0 milestone Apr 27, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT 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 Apr 27, 2022
@terrajobst
Copy link
Member

terrajobst commented May 5, 2022

Video

  • Let's call the method BlobEncoder just Field
namespace System.Reflection.Metadata.Ecma335;

public partial readonly struct BlobEncoder
{
    public FieldTypeEncoder Field();
}

public readonly struct FieldTypeEncoder
{
    public BlobBuilder Builder { get; }

    public FieldTypeEncoder(BlobBuilder builder);
    public CustomModifiersEncoder CustomModifiers();
    public SignatureTypeEncoder Type(bool isByRef = false);
    public void TypedReference();
}

@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 May 5, 2022
@teo-tsirpanis
Copy link
Contributor

I'm working on this.

@teo-tsirpanis teo-tsirpanis self-assigned this May 15, 2022
@AaronRobinsonMSFT
Copy link
Member

@teo-tsirpanis Please wait for @cston to confirm he doesn't already have an implementation. Work to support ref fields is close to complete and it is possible the work has been done just not posted yet.

@AaronRobinsonMSFT AaronRobinsonMSFT removed the blocking Marks issues that we want to fast track in order to unblock other important work label May 15, 2022
@cston
Copy link
Member Author

cston commented May 15, 2022

Please wait for @cston to confirm he doesn't already have an implementation.

I have not implemented this. The compiler is currently using the existing API.

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented May 15, 2022

Thanks @cston. @teo-tsirpanis Looking forward to the PR.

teo-tsirpanis added a commit to teo-tsirpanis/dotnet-runtime that referenced this issue May 15, 2022
@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels May 15, 2022
AaronRobinsonMSFT pushed a commit that referenced this issue May 17, 2022
…`. (#69378)

* Support ref fields in `System.Reflection.Metadata.Ecma335.BlobEncoder`.
Fixes #68309.
A test case was added.
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 17, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 16, 2022
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.Metadata
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants