Allow Obsolete Attribute on Accessors #32571

YairHalberstadt · 2019-01-17T20:59:25Z

See #32472, dotnet/csharplang#2152, https://github.com/dotnet/csharplang/blob/master/meetings/2019/LDM-2019-02-27.md

This PR makes the following changes:

Allow Obsolete and Deprecated attributes on property accessors in C# 8. They remain invalid on event accessors.
An obsolete or deprecated attribute on any accessor (property or event) will suppress warnings from the usage of obsolete/deprecated symbols in that accessor. For simplicity, this applies across all language versions.
A new error has been added 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 now reads:

                // (5,24): error CS8652: The feature 'obsolete on property accessor' is currently in Preview and *unsupported*. To use Preview features, use the 'preview' language version.

C# already had to be able to consume code which declared an accessor obsolete, as that is perfectly legal both in IL, and in VB. As such no changes have been necessary to allow consumption of obsolete accessors in C#.

In any cases where I made design decisions which I am unsure of, I will leave a review.

…bsolete works as expected.

…e accessor declared obsolete.

…warnings from an obsolete return type

…cessor

Forbid Obsolete/Deprecated on event accessors. Give suitable error messages for Obsolete/Deprected on event accessors, and on property accessors pre-C# 8 Add further tests

…dToMethod

…e on property accessor'. Fix missing using directive in unit test.

YairHalberstadt · 2019-03-14T09:05:34Z

src/Compilers/CSharp/Portable/Symbols/ObsoleteAttributeHelpers.cs

@@ -81,7 +76,15 @@ private static ThreeState GetObsoleteContextState(Symbol symbol, bool forceCompl
                    return state;
                }

-                symbol = symbol.ContainingSymbol;
+                // For property or event accessors, check the associated property or event next.
+                if (symbol.IsAccessor())


This suppresses warnings/errors from the usage of an obsolete symbol in an accessor if the accessor is marked obsolete itself.

We only really need to do this for properties, not for events, but it is simplest to treat them both equally, and it is arguable which way is most correct.

For a similar reason, we change this for all C# versions, not just for C# 8

YairHalberstadt · 2019-03-14T09:09:54Z

src/Compilers/CSharp/Portable/Errors/ErrorCode.cs

@@ -1598,7 +1598,7 @@ internal enum ErrorCode
        ERR_PossibleAsyncIteratorWithoutYieldOrAwait = 8420,
        ERR_StaticLocalFunctionCannotCaptureVariable = 8421,
        ERR_StaticLocalFunctionCannotCaptureThis = 8422,
-
+        ERR_AttributeNotOnEventAccessor = 8423,


I am not sure how new error codes are picked. I am also not sure whether this should go with C# 1 errors, or C# 8, as it's just a different name/message for what is essentially an old error. #Closed

This number (in the C# 8.0 range) is fine.

In reply to: 265474149 [](ancestors = 265474149)

YairHalberstadt · 2019-03-14T12:51:31Z

@gafter
This PR is ready for review. #Closed

jcouv · 2019-03-20T20:40:57Z

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

+                        // CS1667: Attribute '{0}' is not valid on event accessors. It is only valid on '{1}' declarations.
+                        AttributeUsageInfo attributeUsage = arguments.Attribute.AttributeClass.GetAttributeUsageInfo();
+                        arguments.Diagnostics.Add(ErrorCode.ERR_AttributeNotOnEventAccessor, arguments.AttributeSyntaxOpt.Name.Location, description.FullName, attributeUsage.GetValidTargetsErrorArgument());
+                    }


} [](start = 20, length = 1)

Feels like we should check the language version in an else branch here. There is no point in giving a language version diagnostic on events. Please add a test too. #Closed

jcouv · 2019-03-20T20:45:53Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_WellKnownAttributes.cs

@@ -5968,6 +5991,10 @@ public class TestClass
    [Obsolete(""Do not use Prop1"", false)]
    public int Prop1 { get; set; }

+    public int Prop2 { [Obsolete(""Do not use Prop2.Get"")] get; set; }


Could you test a scenario with Obsolete on both the property and the accessor? I think that will produce a single diagnostic (which is desirable). #Closed

What if one is an error, and one a warning?
Or both an error/warning, but with different messages?
It might be best if we treat it as two separate diagnostics.
We could just ban making both a property and an accessor obsolete, but that still wouldn't help when consuming the attribute from other languages.

From discussion with @cston two options seem ok:

report the worse one

report both

Either seems fine, whatever is easiest to implement. #Closed

It might be reasonable to give priority to the attribute on the accessor. #Closed

The current behaviour is to report both. Given that this is both an unlikely scenario, and the correct behaviour is debatable, I think it makes most sense to take the simplest option, and keep the current behaviour.

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_WellKnownAttributes.cs

jcouv · 2019-03-20T20:52:43Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_WellKnownAttributes.cs

@@ -6447,38 +6553,50 @@ public int Prop
        get { return 1; }
        set { }
    }
+    [Obsolete(""Property"", true)]


[Obsolete(""Property"", true)] [](start = 4, length = 30)

is it intentional to have Obsolete on both the property and the setter here? The Obsolete on the setter does not seem to come into play #Closed

It looks like there's an (already existing) bug here in that we don't report diagnostics from an obsolete setter when we use the property in an attribute constructor.

using System; [AttributeUsage(AttributeTargets.All, AllowMultiple = true)] public class Att : Attribute { public int Prop { get; [obsolete] set; } } [Att(Prop = 1)] // no warning reported here public class Test { }

Although fixing this is a breaking change (as it is possible that people were already consuming such attributes from Visual Basic) I do not think it is an issue to fix as it is not binary breaking, and adding an Obsolete attribute is explicitly trying to break peoples source.

jcouv · 2019-03-20T20:56:05Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_WellKnownAttributes.cs

    }
 }

 class Class6
 {
-    int P1
+    public int P1
    {
        [Deprecated(""P1.get is deprecated."", DeprecationType.Remove, 1)]


[Deprecated(""P1.get is deprecated."", DeprecationType.Remove, 1)] [](start = 8, length = 66)

Consider adding a test for Deprecated on the setter as well. #Closed

jcouv

Done with review pass (iteration 12).
Sorry for the delay. We've been focused on fixing nullable issues.

AlekseyTs · 2019-03-21T20:53:29Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_WellKnownAttributes.cs

+}
+class E2 : E1
+{
+    public override int Goo { [Obsolete] get; set;}


[Obsolete] [](start = 30, length = 10)

Please make sure we have a separate unit-test for consumption of an overridden property, i.e. the Obsolete attribute is on different levels of inheritance hierarchy, including cases when not all accessors are overridden in a derived class #Closed

We completely ignore any obsolete attributes on an overridden member, and only look at the attributes on the base member. This is true for accessors, properties, methods, etc. #Resolved

AlekseyTs

Done with review pass (iteration 12)

Didn't mean to sign-off

AlekseyTs · 2019-03-21T21:05:01Z

Done with review pass (iteration 12) #Closed

…s, and Don't report 2 diagnostics for obsolete on an event in C# 7.

jcouv · 2019-03-22T17:42:43Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_WellKnownAttributes.cs

+
+class Base
+{
+    public virtual int Foo { [Obsolete] get; set;}


Foo [](start = 23, length = 3)

Foo is disallowed in the codebase, as it triggers some word check tools we run. #Closed

I hope Boo is better than Foo :-)

jcouv

LGTM Thanks (iteration 16) modulo "Foo"

YairHalberstadt · 2019-03-28T18:33:25Z

@jcouv
Is there anything else that needs to be done here?
Thanks

jcouv · 2019-03-28T18:47:01Z

@AlekseyTs for second review. Thanks

AlekseyTs · 2019-03-29T20:55:07Z

src/Compilers/CSharp/Portable/Binder/Binder_Attributes.cs

+            if (namedArgumentNameSymbol.Kind == SymbolKind.Property)
+            {
+                var propertySymbol = (PropertySymbol)namedArgumentNameSymbol;
+                if (propertySymbol.SetMethod != null)


propertySymbol [](start = 20, length = 14)

If the property is overridden, which property do we get here the one from the base, or the one from the derived type? Derived type doesn't have to override all accessors. If we are dealing with an overriding property that overrides only getter, we will fail the null check and will not check the setter for obsoleteness. Please add a test for a scenario like that. #Closed

Thanks for spotting that. Fixed.

AlekseyTs · 2019-03-29T21:03:12Z

Done with review pass (iteration 17) #Closed

AlekseyTs

LGTM (iteration 21)

jcouv · 2019-04-02T20:53:18Z

Merged for dev16.1 preview2. Thanks @YairHalberstadt!

gafter · 2019-04-02T22:53:49Z

Thank you so much for your contribution, @YairHalberstadt!

@deprecated

Fixes: #1033 Context: dotnet/roslyn#32571 When we are converting Java get-method and set-method pairs to C# properties, we can hit an interesting scenario where a getter may be `@Deprecated` and the setter is not, or vice versa: class Example { public boolean hasOptionsMenu () { ... } @deprecated public void setHasOptionsMenu (boolean hasMenu) { ... } } C# has traditionally not allowed `[Obsolete]` to be placed on just a `get` or a `set` block; it could only be on the entire property: partial class Example { [Obsolete] public bool HasOptionsMenu { get; set; } } This can lead to confusion because using the getter will report an obsolete warning when it is not obsolete. Thus, for properties, we only add `[Obsolete]` in 2 cases: 1. The `get` is obsolete and there is no `set` 2. Both the `get` and `set` are obsolete We have this comment in our code: // Unlike [Register], [Obsolete] cannot be put on property accessors, so we can apply them only under limited condition... However, the C# compiler team has determined that preventing `[Obsolete]` on property accessors was a bug, and has fixed it in C#8 via dotnet/roslyn#32571. Update `generator` to support scenarios in which only the Java getter or setter is marked as `@Deprecated`, by placing `[Obsolete]` on the appropriate `get` or `set` block: partial class Example { public bool HasOptionsMenu { get => … [Obsolete] set =>… } }

Remove code dissallowing Obsolete on Properties, and Unit Test that O…

38179e3

…bsolete works as expected.

YairHalberstadt requested a review from a team as a code owner January 17, 2019 20:59

YairHalberstadt and others added 5 commits January 18, 2019 09:01

Add further unit tests, and fix bug with suppression of obolete insid…

0af9dcf

…e accessor declared obsolete.

Fix tests broken as a result of previous change

3eb45f4

Fix unit test CS1667ERR_AttributeNotOnAccessor

dc79ea5

Add unit test to confirm that an obsolete accessor does not suppress …

376c8d5

…warnings from an obsolete return type

Fixed Missing ');' in AttributeTests_WellKnownAttributes

272e44a

jinujoseph added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Jan 23, 2019

Update AttributeTests_WellKnownAttributes.cs

26d8a67

YairHalberstadt mentioned this pull request Mar 11, 2019

Champion: "Allow Obsolete attribute on getters and setters" (16.3, Core 3) dotnet/csharplang#2152

Open

5 tasks

Merge remote-tracking branch 'upstream/master' into AllowObsoleteOnAc…

9c30cc3

…cessor

YairHalberstadt changed the title ~~Allow Obsolete Attribute on Accessors~~ WIP: Allow Obsolete Attribute on Accessors Mar 13, 2019

YairHalberstadt added 4 commits March 14, 2019 08:35

Allow Deprecated Attribute on property accessors.

da56be5

Forbid Obsolete/Deprecated on event accessors. Give suitable error messages for Obsolete/Deprected on event accessors, and on property accessors pre-C# 8 Add further tests

Fix comment in SourceMemberMethodSymbol.VerifyObsoleteAttributeApplie…

451c738

…dToMethod

merged from master

b2b7adf

Change feature message from 'obsolete property accessors' to 'obsolet…

4755eaa

…e on property accessor'. Fix missing using directive in unit test.

YairHalberstadt commented Mar 14, 2019

View reviewed changes

YairHalberstadt changed the title ~~WIP: Allow Obsolete Attribute on Accessors~~ Allow Obsolete Attribute on Accessors Mar 14, 2019

jcouv assigned gafter and jcouv Mar 20, 2019

jcouv reviewed Mar 20, 2019

View reviewed changes

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_WellKnownAttributes.cs Show resolved Hide resolved

jcouv reviewed Mar 20, 2019

View reviewed changes

jcouv approved these changes Mar 20, 2019

View reviewed changes

AlekseyTs reviewed Mar 21, 2019

View reviewed changes

AlekseyTs previously approved these changes Mar 21, 2019

View reviewed changes

YairHalberstadt added 4 commits March 22, 2019 08:40

Changes based on code review. Add more tests for obsolete on accessor…

c327ac2

…s, and Don't report 2 diagnostics for obsolete on an event in C# 7.

Report obsolete from setter on attribute property

dc32918

Fix typo

904f95d

Add test for obsolete attribute on indexer accessors.

d1e4cfd

jcouv self-requested a review March 22, 2019 17:40

jcouv reviewed Mar 22, 2019

View reviewed changes

jcouv approved these changes Mar 22, 2019

View reviewed changes

Replace Foo with Boo

921240d

AlekseyTs reviewed Mar 29, 2019

View reviewed changes

YairHalberstadt and others added 4 commits March 31, 2019 11:06

Deal correctly with an obsolete inherited set method on an attribute

4bc5bcb

merged from master

a57e934

Reorder MessageId usages

ad5175d

Undo formatting change

9a563e3

AlekseyTs approved these changes Apr 2, 2019

View reviewed changes

jcouv added the Auto-Merge If Tests Pass label Apr 2, 2019

jcouv merged commit ef24d3f into dotnet:master Apr 2, 2019

gafter mentioned this pull request Jul 16, 2019

Allow Obsolete attribute on getters and setters #32472

Closed

jzabroski mentioned this pull request Oct 2, 2019

Extracting inteface from MigrationScopeHandler and exposing it via DI #1047 fluentmigrator/fluentmigrator#1069

Merged

jpobst mentioned this pull request Aug 30, 2022

Handle properties with getter or setter only [Obsolete] attribute dotnet/java-interop#1033

Closed

jpobst mentioned this pull request Dec 6, 2022

[generator] Better support deprecated property getter/setters. dotnet/java-interop#1062

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Obsolete Attribute on Accessors #32571

Allow Obsolete Attribute on Accessors #32571

YairHalberstadt commented Jan 17, 2019 •

edited

Loading

YairHalberstadt Mar 14, 2019

YairHalberstadt Mar 14, 2019 •

edited by jcouv

Loading

jcouv Mar 20, 2019

YairHalberstadt commented Mar 14, 2019 •

edited by jcouv

Loading

jcouv Mar 20, 2019 •

edited

Loading

jcouv Mar 20, 2019 •

edited

Loading

YairHalberstadt Mar 21, 2019

jcouv Mar 21, 2019 •

edited

Loading

AlekseyTs Mar 21, 2019 •

edited

Loading

YairHalberstadt Mar 21, 2019

jcouv Mar 20, 2019 •

edited

Loading

YairHalberstadt Mar 22, 2019

jcouv Mar 20, 2019 •

edited

Loading

jcouv left a comment

AlekseyTs Mar 21, 2019 •

edited

Loading

YairHalberstadt Mar 22, 2019 •

edited by jcouv

Loading

AlekseyTs left a comment

AlekseyTs commented Mar 21, 2019 •

edited by jcouv

Loading

jcouv Mar 22, 2019 •

edited

Loading

YairHalberstadt Mar 24, 2019

jcouv left a comment

YairHalberstadt commented Mar 28, 2019

jcouv commented Mar 28, 2019

AlekseyTs Mar 29, 2019 •

edited

Loading

YairHalberstadt Mar 31, 2019

AlekseyTs commented Mar 29, 2019 •

edited

Loading

AlekseyTs left a comment

jcouv commented Apr 2, 2019

gafter commented Apr 2, 2019

Allow Obsolete Attribute on Accessors #32571

Allow Obsolete Attribute on Accessors #32571

Conversation

YairHalberstadt commented Jan 17, 2019 • edited Loading

YairHalberstadt Mar 14, 2019

Choose a reason for hiding this comment

YairHalberstadt Mar 14, 2019 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Mar 20, 2019

Choose a reason for hiding this comment

YairHalberstadt commented Mar 14, 2019 • edited by jcouv Loading

jcouv Mar 20, 2019 • edited Loading

Choose a reason for hiding this comment

jcouv Mar 20, 2019 • edited Loading

Choose a reason for hiding this comment

YairHalberstadt Mar 21, 2019

Choose a reason for hiding this comment

jcouv Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

YairHalberstadt Mar 21, 2019

Choose a reason for hiding this comment

jcouv Mar 20, 2019 • edited Loading

Choose a reason for hiding this comment

YairHalberstadt Mar 22, 2019

Choose a reason for hiding this comment

jcouv Mar 20, 2019 • edited Loading

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

AlekseyTs Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

YairHalberstadt Mar 22, 2019 • edited by jcouv Loading

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

AlekseyTs commented Mar 21, 2019 • edited by jcouv Loading

jcouv Mar 22, 2019 • edited Loading

Choose a reason for hiding this comment

YairHalberstadt Mar 24, 2019

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

YairHalberstadt commented Mar 28, 2019

jcouv commented Mar 28, 2019

AlekseyTs Mar 29, 2019 • edited Loading

Choose a reason for hiding this comment

YairHalberstadt Mar 31, 2019

Choose a reason for hiding this comment

AlekseyTs commented Mar 29, 2019 • edited Loading

AlekseyTs left a comment

Choose a reason for hiding this comment

jcouv commented Apr 2, 2019

gafter commented Apr 2, 2019

YairHalberstadt commented Jan 17, 2019 •

edited

Loading

YairHalberstadt Mar 14, 2019 •

edited by jcouv

Loading

YairHalberstadt commented Mar 14, 2019 •

edited by jcouv

Loading

jcouv Mar 20, 2019 •

edited

Loading

jcouv Mar 20, 2019 •

edited

Loading

jcouv Mar 21, 2019 •

edited

Loading

AlekseyTs Mar 21, 2019 •

edited

Loading

jcouv Mar 20, 2019 •

edited

Loading

jcouv Mar 20, 2019 •

edited

Loading

AlekseyTs Mar 21, 2019 •

edited

Loading

YairHalberstadt Mar 22, 2019 •

edited by jcouv

Loading

AlekseyTs commented Mar 21, 2019 •

edited by jcouv

Loading

jcouv Mar 22, 2019 •

edited

Loading

AlekseyTs Mar 29, 2019 •

edited

Loading

AlekseyTs commented Mar 29, 2019 •

edited

Loading