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

Make ParameterBuilder abstract #83831

Closed
buyaa-n opened this issue Mar 23, 2023 · 4 comments · Fixed by #85446
Closed

Make ParameterBuilder abstract #83831

buyaa-n opened this issue Mar 23, 2023 · 4 comments · Fixed by #85446
Labels
api-approved API was approved in API review, it can be implemented area-System.Reflection
Milestone

Comments

@buyaa-n
Copy link
Contributor

buyaa-n commented Mar 23, 2023

With Abstract System.Reflection.Emit public types we have abstracted other System.Reflection.Emit.XyzBuilder types, ParameterBuilder type left out from this effort (probably because it lives in different ref file).

The type also needs to be abstracted same as others because:

  • Because of internal constructor could not create an instance of it outside of CoreLib. (The new AsemblyBuilder implementation will be added outside of CoreLib)
  • Even if we workaround the internal constructor we could not get CustomAttributes info set to the ParameterBuilder instance, same for the default value.

Suggested API change:

namespace System.Reflection.Emit
{
-   public class ParameterBuilder
+   public abstract class ParameterBuilder
    {
-       internal ParameterBuilder() { }
+       protected ParameterBuilder() { }
        public virtual int Attributes { get { throw null; } }
        public bool IsIn { get { throw null; } }
        public bool IsOptional { get { throw null; } }
        public bool IsOut { get { throw null; } }
        public virtual string? Name { get { throw null; } }
        public virtual int Position { get { throw null; } }
        public virtual void SetConstant(object? defaultValue) { }
        public void SetCustomAttribute(ConstructorInfo con, byte[] binaryAttribute) { }
+       protected abstract void SetCustomAttributeCore(ConstructorInfo con, byte[] binaryAttribute);
        public void SetCustomAttribute(CustomAttributeBuilder customBuilder) { }
    }
}
@buyaa-n buyaa-n added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 23, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Mar 23, 2023
@ghost
Copy link

ghost commented Mar 23, 2023

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

Issue Details

With Abstract System.Reflection.Emit public types we have abstracted other System.Reflection.Emit.XyzBuilder types, ParameterBuilder type left out from this effort (probably because it lives in different ref file).

The type also needs to be abstracted same as others because:

  • Because of internal constructor could not create an instance of it outside of CoreLib. (The new AsemblyBuilder implementation will be added outside of CoreLib)
  • Even if we workaround the internal constructor we could not get CustomAttributes info set to the ParameterBuilder instance, same for the default value.

Suggested API change:

namespace System.Reflection.Emit
{
-   public class ParameterBuilder
+   public abstract class ParameterBuilder
    {
-       internal ParameterBuilder() { }
+       protected ParameterBuilder() { }
        public virtual int Attributes { get { throw null; } }
        public bool IsIn { get { throw null; } }
        public bool IsOptional { get { throw null; } }
        public bool IsOut { get { throw null; } }
        public virtual string? Name { get { throw null; } }
        public virtual int Position { get { throw null; } }
        public virtual void SetConstant(object? defaultValue) { }
        public void SetCustomAttribute(ConstructorInfo con, byte[] binaryAttribute) { }
+       protected abstract void SetCustomAttributeCore(ConstructorInfo con, byte[] binaryAttribute);
        public void SetCustomAttribute(CustomAttributeBuilder customBuilder) { }
    }
}
Author: buyaa-n
Assignees: -
Labels:

api-suggestion, area-System.Reflection, untriaged

Milestone: -

@buyaa-n buyaa-n added this to the 8.0.0 milestone Mar 23, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Mar 23, 2023
@buyaa-n buyaa-n added api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Mar 23, 2023
@teo-tsirpanis
Copy link
Contributor

How about we seize the opportunity and make SetCustomAttributeCore accept a ReadOnlySpan<byte>? No need to also expose it as a public method, this can happen as part of #63419.

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Mar 24, 2023

Internal implementation currently works with byte[], of course we could refactor that, I think that is better handled by #63419 with all other types custom attributes handling (looks all of them handled similarly).

@terrajobst
Copy link
Member

terrajobst commented Mar 28, 2023

Video

  • The core method should be based on ReadOnlySpan<byte>
    • We should align the other ones we added this release
namespace System.Reflection.Emit;

-public class ParameterBuilder
+public abstract class ParameterBuilder
 {
-    internal ParameterBuilder();
+    protected ParameterBuilder();
     public virtual int Attributes { get; }
     public bool IsIn { get; }
     public bool IsOptional { get; }
     public bool IsOut { get; }
     public virtual string? Name { get; }
     public virtual int Position { get; }
     public virtual void SetConstant(object? defaultValue);
     public void SetCustomAttribute(ConstructorInfo con, byte[] binaryAttribute);
+    protected abstract void SetCustomAttributeCore(ConstructorInfo con, ReadOnlySpan<byte> binaryAttribute);
     public void SetCustomAttribute(CustomAttributeBuilder customBuilder);
 }

@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 29, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 27, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Apr 28, 2023
@ghost ghost locked as resolved and limited conversation to collaborators May 28, 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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants