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

Prohibiting Certain Attributes on Property and Event Accessors #375

Closed
RexJaeschke opened this issue Sep 13, 2021 · 4 comments
Closed

Prohibiting Certain Attributes on Property and Event Accessors #375

RexJaeschke opened this issue Sep 13, 2021 · 4 comments
Assignees
Labels
type: clarity While not technically incorrect, the Standard is potentially confusing
Milestone

Comments

@RexJaeschke
Copy link
Contributor

V8 removes a restriction on the Obsolete attribute by allowing it on property and event accessors. However, I don’t see that this was ever disallowed in early versions.

@gafter challenges this assertion in Roslyn Issue 32472 (dotnet/roslyn#32472 (comment)), by stating

The language spec says

The attribute Obsolete is used to mark types and members of types that should no longer be used.
A property accessor is neither a type nor a member of a type. So the current implementation is as required by the specification. Arguably, the attribute could be permitted on accessors but ignored on uses of the accessor. Either way, changing this would require a change to the specification.

I disagree; in an attribute context, accessors are treated like methods (which are members). I deduce that from the fact that for an attribute to be allowed on an accessor, the attribute type must have System.AttributeUsage(… System.AttributeTargets.Method …).

While researching this, I also found that the Conditional attribute is also disallowed on accessors, but, again, the spec does not say so. This type has [System.AttributeUsage(System.AttributeTargets.Class | System.AttributeTargets.Method, AllowMultiple=true)]. 22.5.3.2, “Conditional methods”, https://github.com/dotnet/csharpstandard/blob/draft-v6/standard/attributes.md#22532-conditional-methods, has no statement analogous to the one Neal cited for Obsolete.

I propose that we modify the spec where property and event accessors are defined, adding prohibitions on the use of these two attributes. And then add Notes in the Attribute chapter where these attributes are spec’d, pointing back to those prohibitions.

Then for V8, we'll remove the prohibition on Obsolete.

@RexJaeschke RexJaeschke added the meeting: proposal There is an informal proposal in the issue, worth discussing in a meeting label Sep 13, 2021
@RexJaeschke RexJaeschke self-assigned this Sep 13, 2021
@Nigel-Ecma
Copy link
Contributor

Further to Rex’s deduction we find in the second bulleted list in §22.3:

  • method — a constructor, finalizer, method, operator, property get and set accessors, indexer get and set accessors, and event add and remove accessors. A field-like event (i.e., one without accessors) can also have an attribute with this target.

To me it looks like the wording around ObsoleteAttribtute; §22.5.1, “used to mark a member as obsolete”; is just unfortunate and wasn’t intended to exclude accessors at all. In Ecma-355 §21.2.2 the attribute “Indicates that an element is not to be used” – “element” not “member”.

Given the loose language I don’t think is a Standard or compiler bug per se, I do wonder what the compiler does when importing a CLS compliant property accessor with this attribute (its a CLS requirement).

I think we just punt on this one, not worth changing the Standard now, but leave a note to make things clear come v8.

@jskeet jskeet added the type: clarity While not technically incorrect, the Standard is potentially confusing label Sep 22, 2021
@RexJaeschke RexJaeschke removed the meeting: proposal There is an informal proposal in the issue, worth discussing in a meeting label Oct 21, 2021
@RexJaeschke RexJaeschke added this to the C# 8.0 milestone Oct 21, 2021
@RexJaeschke
Copy link
Contributor Author

During the 2021-10-20 call, we agreed to leave the spec unchanged, even if it might be slightly broken, as the reportedly “missing” prohibition is lifted in V8.

@jskeet
Copy link
Contributor

jskeet commented Nov 12, 2021

Closing as per comment above.

@jskeet jskeet closed this as completed Nov 12, 2021
@KalleOlaviNiemitalo
Copy link
Contributor

KalleOlaviNiemitalo commented Aug 14, 2023

the reportedly “missing” prohibition is lifted in V8

It is lifted for ObsoleteAttribute but AFAIK not for ConditionalAttribute.
Should the specification be changed to explicitly disallow ConditionalAttribute on accessors?

I think ConditionalAttribute could be useful on the set accessor of a write-only property, or on add and remove accessors of an event; if the implementation does nothing anyway, then ConditionalAttribute could be used to elide the calls entirely. OTOH, in a statement like obj.Prop = Fun();, if the set accessor of Prop is conditional and the condition is false, then it would not be obvious whether Fun() is evaluated at all. Even worse, in obj.Prop += Fun();, it would not be obvious whether operator + is called.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: clarity While not technically incorrect, the Standard is potentially confusing
Projects
None yet
Development

No branches or pull requests

4 participants