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

C# 7.x: Allow field-targeted attributes on auto-property declarations #262

Merged
merged 4 commits into from
Jan 4, 2023

Conversation

RexJaeschke
Copy link
Contributor

Note that v6 PR #11 modifies much of 15.7.4, and in this new PR, we’re adding new text at the end of that section, which should not overlap with the v6 mods, which had not yet been merged when this new PR was created.

@RexJaeschke RexJaeschke added this to the C# 7.x milestone Apr 17, 2021
@RexJaeschke RexJaeschke requested a review from BillWagner April 17, 2021 15:14
@RexJaeschke RexJaeschke self-assigned this Apr 17, 2021
@RexJaeschke RexJaeschke removed their assignment May 12, 2021
@RexJaeschke RexJaeschke added the Review: pending Proposal is available for review label Sep 16, 2021
@BillWagner BillWagner marked this pull request as draft February 3, 2022 15:14
@BillWagner
Copy link
Member

converting to draft: C# 7.x feature

@BillWagner BillWagner force-pushed the Rex-v7-attr-on-autoprops branch from 3964d86 to 2c551e4 Compare October 2, 2022 22:14
@BillWagner BillWagner marked this pull request as ready for review October 5, 2022 22:19
@jskeet jskeet self-requested a review January 3, 2023 08:30
@jskeet
Copy link
Contributor

jskeet commented Jan 3, 2023

I'll fix up the example - possibly replacing the attribute with a different one, to avoid any interpretation that this feature is linked to serialization.

@jskeet jskeet added Review: complete at least one person has reviewed this meeting: discuss This issue should be discussed at the next TC49-TG2 meeting and removed Review: pending Proposal is available for review labels Jan 3, 2023
@jskeet
Copy link
Contributor

jskeet commented Jan 3, 2023

Okay, having changed the attribute in question, I'm personally happy with this - if others are happy, we can merge it tomorrow.

@jskeet jskeet force-pushed the Rex-v7-attr-on-autoprops branch from 0e70b82 to a14d97c Compare January 3, 2023 17:22
@KalleOlaviNiemitalo
Copy link
Contributor

This needs changes in §21.3 Attribute specification as well.

Change to allow field on auto properties:

- `field` — a field. A field-like event (i.e., one without accessors) can also have an attribute with this target.

Change to list field and property as valid targets on auto property, and specify that property is the default:

Certain contexts permit the specification of an attribute on more than one target. A program can explicitly specify the target by including an *attribute_target_specifier*. Without an *attribute_target_specifier* a default is applied, but an *attribute_target_specifier* can be used to affirm or override the default. The contexts are resolved as follows:

@jskeet
Copy link
Contributor

jskeet commented Jan 3, 2023

This needs changes in §21.3 Attribute specification as well.

Done in another commit. I've currently kept the singular form in the first part of the description, for consistency with the rest, but I'm tempted to change it to "Field-like events and automatically implemented properties can ..."

@jskeet jskeet force-pushed the Rex-v7-attr-on-autoprops branch from da63466 to ab777da Compare January 3, 2023 17:41
@KalleOlaviNiemitalo
Copy link
Contributor

Shall the compiler issue a warning here?

using System;
class C {
    C() { P = 1; }

    [field: Obsolete]
    int P { get; }
}

When a property is specified as an automatically implemented property, a hidden backing field is automatically available for the property, and the accessors are implemented to read from and write to that backing field. The hidden backing field is inaccessible, it can be read and written only through the automatically implemented property accessors, even within the containing type. If the auto-property has no set accessor, the backing field is considered `readonly` ([§14.5.3](classes.md#1453-readonly-fields)). Just like a `readonly` field, a read-only auto-property may also be assigned to in the body of a constructor of the enclosing class. Such an assignment assigns directly to the read-only backing field of the property.
If a program uses a type or member that is decorated with the `Obsolete` attribute, the compiler shall issue a warning or an error. Specifically, the compiler shall issue a warning if no error parameter is provided, or if the error parameter is provided and has the value `false`. The compiler shall issue an error if the error parameter is specified and has the value `true`.

jskeet added a commit to jskeet/csharpstandard that referenced this pull request Jan 4, 2023
@KalleOlaviNiemitalo
Copy link
Contributor

Could perhaps add to §14.7.4

Such an assignment assigns directly to the read-only backing field of the property. If the backing field is decorated with the Obsolete attribute (§21.5.4), that shall not cause the compiler to issue a warning or an error about the assignment.

or to §21.5.4

If a program uses a type or member that is decorated with the Obsolete attribute, the compiler shall issue a warning or an error, unless the member is the backing field of an automatically implemented property (§14.7.4).

@jskeet
Copy link
Contributor

jskeet commented Jan 4, 2023

I'm not sure that's necessary, as I'd personally argue that the property (which is the member being used) effectively isn't decorated with the Obsolete attribute. However, the technical group can discuss that in our meeting in a few hours.

@jskeet
Copy link
Contributor

jskeet commented Jan 4, 2023

Whoops - I understand your point now, about that specific case referring to the field.
The same is true for field-like events.

Given the latter part, the group has decided that we'll merge this PR, and raise a new issue about it to fix for both. (The reverse case is interesting too, where the property is obsolete but the field isn't.) Intuition from Mads: the code effectively refers to the property, and the fact that it refers to a field is "sort of" an implementation detail - so it may be that we tweak 14.7.4 a little. But we'll deal with it in the new issue (which I'll link to this one).

Copy link
Member

@gafter gafter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐸

@jskeet jskeet merged commit 2db5e32 into draft-v7 Jan 4, 2023
@jskeet jskeet deleted the Rex-v7-attr-on-autoprops branch January 4, 2023 20:33
jskeet added a commit that referenced this pull request Jan 4, 2023
@jskeet
Copy link
Contributor

jskeet commented Jan 5, 2023

@KalleOlaviNiemitalo: On a separate note, thank you for everything you've been spotting in this PR and others. We really appreciate it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meeting: discuss This issue should be discussed at the next TC49-TG2 meeting Review: complete at least one person has reviewed this
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants