Allow Obsolete attribute on getters and setters

[YairHalberstadt]

Currently in C# the Obsolete attribute is not allowed on property getters and setters.

using System;

class C
{
    int P { get; [Obsolete] set; }
}

Has the error

error CS1667: Attribute 'System.ObsoleteAttribute' is not valid on property or event accessors. It is only valid on 'class, struct, enum, constructor, method, property, indexer, field, event, interface, delegate' declarations.

In VB it is allowed, and seems to work as expected.

It would also be useful, for example when I want to start making a property readonly, and so mark usages of the setter obsolete in the meantime. Also a property setter might have to be declared for eg. XmlSerialization, but I don't want anyone else using it, so I would like to mark it obsolete.

Since the obsolete attribute seems to be unique in this regard among attributes, I'm guessing this is either a compiler or a language feature.

I'm not sure which repository is the correct place for this issue, so I'm asking it here, and if necessary I will move it to csharplang.

[YairHalberstadt]

The ecma specification seems silent on this:

9 24.4.3 The Obsolete attribute
The attribute Obsolete is used to mark types and members of types that should no longer be used.

     using System;
	[AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct |
					AttributeTargets.Enum | AttributeTargets.Interface |
					AttributeTargets.Delegate | AttributeTargets.Method |
					AttributeTargets.Constructor | AttributeTargets.Property |
					AttributeTargets.Field | AttributeTargets.Event)]
	public class ObsoleteAttribute : Attribute
	{
		public ObsoleteAttribute() { ... }

		public ObsoleteAttribute(string message) { ... }

		public ObsoleteAttribute(string message, bool error) { ... }

		public string Message { get { ... } }

		public bool IsError { get { ... } }
	}

If a program uses a type or member that is decorated with the Obsolete attribute, then the compiler shall issue a warning or error in order to alert the developer, so the offending code can be fixed.

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 a compile-time error if the error parameter is specified and has the value true.

I guess its opinion on this depends on how you define Member

[YairHalberstadt]

the C# 6 draft specification is identical: https://github.com/dotnet/csharplang/blob/master/spec/attributes.md#the-obsolete-attribute

[jhinder]

The behaviour of C# and VB w.r.t. these "special" attributes (CLSCompliant, Conditional and Obsolete, according to the docs) differs greatly.

• VB allows Conditional and Obsolete, and emits a warning for CLSCompliant (BC40043).
• C# emits an error for all three (CS1667).

I think VB's behaviour is somewhat better than C#'s.

• Conditional: not sure about that one. Sure, you can #if away an accessor (or its body) anyway, so why forbid it? But then again the two methods produce very different results (not emitting IL vs not emitting calls), and there are lots of ways for Conditional on property accessors to confuse consumers, or even break code on re-compilation in subtle but painful ways -- you'd have to look at the IL to notice that you're suddenly missing a setter call. In addition, Conditional should be forbidden on getters (similar to all other methods with non-void return types).
  I personally am against allowing Conditional on accessors.
• Obsolete: yeah, why not.
• Using CLSCompliant in invalid locations is a warning in C# anyway (CS3000 to CS3027 are warnings) -- why is it an error on accessors?

[jinujoseph]

@jaredpar AttributeTargets doesn't contain the value for accessors.
Should we be looking at a different value for cases like this?

[svick]

@jinujoseph As far as I can tell, attributes on property accessors are allowed if the attribute has AttributeTargets.Method, which ObsoleteAttribute does have.

Though I think this is not really about AttributeTargets: for some reason, Roslyn contains code specifically to ensure ObsoleteAttribute is not applied to accessors:

roslyn/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs

Line 1167 in 9e50109

else if (VerifyObsoleteAttributeAppliedToMethod(ref arguments, AttributeDescription.ObsoleteAttribute))

roslyn/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs

Lines 1211 to 1228 in 9e50109

	private bool VerifyObsoleteAttributeAppliedToMethod(
	ref DecodeWellKnownAttributeArguments<AttributeSyntax, CSharpAttributeData, AttributeLocation> arguments,
	AttributeDescription description)
	{
	if (arguments.Attribute.IsTargetAttribute(this, description))
	{
	if (this.IsAccessor())
	{
	// CS1667: Attribute '{0}' is not valid on property or event accessors. It is only valid on '{1}' declarations.
	AttributeUsageInfo attributeUsage = arguments.Attribute.AttributeClass.GetAttributeUsageInfo();
	arguments.Diagnostics.Add(ErrorCode.ERR_AttributeNotOnAccessor, arguments.AttributeSyntaxOpt.Name.Location, description.FullName, attributeUsage.GetValidTargetsErrorArgument());
	}
	return true;
	}
	return false;
	}

[sharwell]

@svick Thanks for the details there. Hopefully the compiler team can shed some light. 😄

[jaredpar]

@gafter

[YairHalberstadt]

I have removed the code disallowing the Obsolete Attribute, and confirmed that everything works as expected, without requiring any further changes:

https://github.com/YairHalberstadt/roslyn/tree/AllowObsoleteOnPropertyAccessors

If this is considered not to require approval from the LDC, I will open a PR.

[CyrusNajmabadi]

based on @svick's analysis, i'm leaning toward this simply being a bug. As far as i can tell, the rest of the compiler consider accessors to just be methods as far as other attributes go. So it seems reasonable to be consistent there with this specific attribute. Barring something in the language spec itself disallowing this (and i don't see anything https://github.com/dotnet/csharplang/blob/master/spec/attributes.md#the-obsolete-attribute that would make me think that exists), then it seems like this makes sense given how we've interpreted AttributeTargets.Method everywhere else.

[gafter]

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.

Having said that, I doubt such a change would be very controversial.

[YairHalberstadt]

In that case I will close the associated pull request. Will you be able to move this issue to csharplang?
Thanks

[YairHalberstadt]

I see you have already

[gafter]

Hang on the to pull request. Lets check with the language design group and if approved we may be able to use that implementation.

[YairHalberstadt]

Should I close the PR if this is championed?

[gafter]

We'll discuss this on Jan 23. If the LDM likes it we'll review the PR.

[YairHalberstadt]

Thanks

[jnm2]

I'd expect nameof and XML crefs not to give warnings if only one accessor was obsolete.

Should it fail to compile if you obsolete both accessors, since you should move the obsoletion to the whole property? Valid configurations would be:

• No obsoletion
• Obsoletion of entire property
• Obsoletion of exactly one accessor

[YairHalberstadt]

Note the exact same question arises if the getter is obsoleted and there is no setter, or vice versa.

If obsoletion of both accessors was allowed (which I think is pointless, but kind of like from a symmetry standpoint), I would suggest nameof ought to still work on the property, as the property has not been declared obsolete. But this is a relatively trivial detail TBH.

Also, I'm guessing this discussion ought to me be to the championed issue.

[jnm2]

I would suggest nameof ought to still work on the property, as the property has not been declared obsolete.

This feels like a pitfall to me. Maybe you need a different message between the two, but what's the meaning of obsoleting all accessors versus obsoleting the property versus both? For a getter-only property as well, why should obsoleting just the accessor be legal or desirable?

[YairHalberstadt]

What does VB do?

[YairHalberstadt]

https://github.com/dotnet/csharplang/blob/master/meetings/2019/LDM-2019-02-27.md

It looks like the LDC has supported this proposal.

[gafter]

The LDM asked to ensure that this works for ObsoleteAttribute as well as DeprecatedAttribute (and that there be tests for that)

[YairHalberstadt]

I'm in the middle of cleaning up and finishing off the original pull request.

I intend to allow Obsolete and Deprecated on property accessors, but not event accessors.

I also will change current behaviour so that an obsolete or deprecated attribute on any accessor (property or event) will suppress warnings from the usage of obsolete/deprecated symbols in that accessor.

A new error will be needed to indicate that deprecated and obsolete attributes cannot be used on event accessors but can be used on property accessors.

The error for using obsolete/deprecated on an attribute pre C# 8 will be changed to indicate that this will be allowed in C# 8.

No work needs to be done to consume these attributes correctly, as they were already legal in VB.

[gafter]

@YairHalberstadt That all sounds perfect.

[gafter]

Fixed in #32571