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

[Proposal]: method attribute target for records for targeting primary constructor #7047

Open
3 of 4 tasks
poizan42 opened this issue Mar 10, 2023 · 11 comments
Open
3 of 4 tasks
Assignees
Labels
Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification Proposal champion Proposal
Milestone

Comments

@poizan42
Copy link

poizan42 commented Mar 10, 2023

Allow "method" as an attribute target for records and record structs for targeting the primary constructor

  • Proposed
  • Prototype: ?
  • Implementation: Done, implemented in released version of C# 12
  • Specification: ?

Summary

It should be possible to specify that an attribute targets the primary constructor of a record/record struct, e.g.

[method: Attr]
public record Rec(
    [property: Foo] int X,
    [field: NonSerialized] int Y
);

Motivation

There is currently no way to add attributes to the auto generated primary constructor when declaring records. This is needed for example to add JsonConstructorAttribute to specify which constructor json serializers should use when deserializing a record type with additional constructors. (both System.Text.Json and Newtonsoft.Json has this). See also discussion at #3650

Detailed design

The "method" attribute target should be allowed on records and record structs and would result in the generated primary constructor (but not the generated copy constructor) having that attribute. Explicitly declared constructors are unaffected.

Attributes with the method target are only allowed on declarations specifying the primary constructor. They are ignored with a warning when used with declarations without a parameter list:

[method: FooAttr] // Good
public partial record Rec(
    [property: Foo] int X,
    [field: NonSerialized] int Y
);

[method: BarAttr] // warning CS0657: 'method' is not a valid attribute location for this declaration. Valid attribute locations for this declaration are 'type'. All attributes in this block will be ignored.
public partial record Rec
{
    public void Frobnicate()
    {
        ...
    }
}

[method: Attr] // Good
public record MyUnit1();

[method: Attr] // warning CS0657: 'method' is not a valid attribute location for this declaration. Valid attribute locations for this declaration are 'type'. All attributes in this block will be ignored.
public record MyUnit2;

Drawbacks

Still no way to target the copy constructor. Also "method" may be a bit confusing as opposed to adding a new "constructor" target.

Alternatives

  • Add a new "constructor" attribute target with the same functionality.
  • Make the attribute target both the primary and the copy constructor (this does not solve the problem with adding JsonConstructorAttribute though)

Unresolved questions

Design meetings

@poizan42
Copy link
Author

@RikkiGibson

@HaloFour
Copy link
Contributor

Also "method" may be a bit confusing as opposed to adding a new "constructor" target.

I'm with this, I think it'd be better to add a constructor target to the language than use method.

@poizan42
Copy link
Author

poizan42 commented Mar 10, 2023

@HaloFour Note that I proposed using method because of this comment by @333fred: #3650 (comment)

@FaustVX
Copy link

FaustVX commented Mar 10, 2023

Currently SetsRequiredMembers can only be on constructors but only accept method as attribute specifier
sharplab.io

@RikkiGibson
Copy link
Contributor

Thanks for writing up the proposal. A few questions:

What happens with partial types? We currently have a rule that only one partial declaration can specify a parameter list. Can any partial declaration specify method attributes?

I think it would be strange if only primary constructors had the ability to be split-out in this way, but not other kinds of constructors. Therefore I would lean toward only allowing the declaration with a parameter list to specify these. However, I don't feel strongly about it.

[method: Attr1]
partial record Rec(int x);

[method: Attr2] // ok?
partial record Rec;

What happens if explicit primary constructor body declarations are added to the language? Does that end up being another place to add attributes to the primary constructor? Is that fine?

[method: Attr1]
record Rec(int x)
{
    // strawman explicit primary constructor body syntax
    [Attr2] // ok?
    Rec
    {
    }

    // alternative strawman
    [Attr2] // ok?
    public primary Rec(int x)
    {
    }
}

I'm tempted to see primary constructor bodies as a little bit like partial methods, in that its full meaning is determined by adding up information provided in two places. So it makes sense to me that if they were added to the language, then method attributes could be added to them, and this wouldn't be problematic in any way.

In other words, I feel there is no reason to block the proposed scenario of [method: Attr] on a type with a primary constructor, just because we haven't fully designed explicit primary constructors yet.

@poizan42
Copy link
Author

Thanks for writing up the proposal. A few questions:

What happens with partial types? We currently have a rule that only one partial declaration can specify a parameter list. Can any partial declaration specify method attributes?

I think it would be strange if only primary constructors had the ability to be split-out in this way, but not other kinds of constructors. Therefore I would lean toward only allowing the declaration with a parameter list to specify these. However, I don't feel strongly about it.

[method: Attr1]
partial record Rec(int x);

[method: Attr2] // ok?
partial record Rec;

Makes sense to only allow it on the declaration with a parameter list since it's the one "specifying" the constructor. From a more practical point of view, would this complicate the implementation in Roslyn (or simplify it?)?

What happens if explicit primary constructor body declarations are added to the language? Does that end up being another place to add attributes to the primary constructor? Is that fine?

[method: Attr1]
record Rec(int x)
{
    // strawman explicit primary constructor body syntax
    [Attr2] // ok?
    Rec
    {
    }

    // alternative strawman
    [Attr2] // ok?
    public primary Rec(int x)
    {
    }
}

I'm tempted to see primary constructor bodies as a little bit like partial methods, in that its full meaning is determined by adding up information provided in two places. So it makes sense to me that if they were added to the language, then method attributes could be added to them, and this wouldn't be problematic in any way.

In other words, I feel there is no reason to block the proposed scenario of [method: Attr] on a type with a primary constructor, just because we haven't fully designed explicit primary constructors yet.

Should a language proposal consider interaction with hypothetical future features? Seems pretty difficult to consider how it interacts with something we don't know what is going to look like or ever gets accepted and implemented.

@RikkiGibson
Copy link
Contributor

From a more practical point of view, would this complicate the implementation in Roslyn (or simplify it?)?

I would say the complexity difference is negligible. Roslyn is already pretty oriented around having n number of attribute lists coming from n number of places applying to a single symbol. Either permitting it or not permitting it here would both be pretty straightforward, I think.

Should a language proposal consider interaction with hypothetical future features?

One of the questions the LDM will tend to ask about language proposals is: will this inhibit our ability to do something else we may want in the future? If at all possible, you want to be able to structure your design, or provide some argument, to give confidence that the answer to that question is no.

@poizan42
Copy link
Author

@RikkiGibson I have addressed the first point in the proposal. I have also clarified that this proposal should apply to record structs as well.

About primary constructor bodies, the current proposal suggests the following syntax:

public class C(int i, string s) : B(s)
{
    {
        if (i < 0) throw new ArgumentOutOfRangeException(nameof(i));
    }
	int[] a = new int[i];
    public int S => s;
}

It does seem a bit odd to apply an attribute directly to a bare block. If that is the syntax that ends up being implemented then I don't think attributes should be applicable to the primary constructor body declaration. If on the other hand a method-like syntax is used then it seems fine to me to allow it and combine all the attribute declarations in the generated primary constructor.

@RikkiGibson
Copy link
Contributor

Thanks for checking the proposal.

I would lean a bit toward agreeing with you, though we do have a proposal somewhere to permit attributes on statements, including blocks. A method's body is not its signature, and the signature is generally what has the method attributes.

@poizan42
Copy link
Author

poizan42 commented Mar 8, 2024

@RikkiGibson Is everything that needs to be done here completed? Shall we close the issue?

@333fred
Copy link
Member

333fred commented Mar 8, 2024

Issues remain open until they are incorporated into the ecma specification. That's why it's labeled as such.

@dotnet dotnet locked as resolved and limited conversation to collaborators Nov 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification Proposal champion Proposal
Projects
None yet
Development

No branches or pull requests

5 participants