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.

Sign up for GitHub

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

Allow Obsolete Attribute on Accessors #32571

Merged
merged 21 commits into from
Apr 2, 2019
Merged

Allow Obsolete Attribute on Accessors #32571

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
38179e3
Remove code dissallowing Obsolete on Properties, and Unit Test that O…
YairHalberstadt Jan 17, 2019
0af9dcf
Add further unit tests, and fix bug with suppression of obolete insid…
YairHalberstadt Jan 18, 2019
3eb45f4
Fix tests broken as a result of previous change
YairHalberstadt Jan 18, 2019
dc79ea5
Fix unit test CS1667ERR_AttributeNotOnAccessor
YairHalberstadt Jan 18, 2019
376c8d5
Add unit test to confirm that an obsolete accessor does not suppress …
YairHalberstadt Jan 21, 2019
272e44a
Fixed Missing ');' in AttributeTests_WellKnownAttributes
YairHalberstadt Jan 21, 2019
26d8a67
Update AttributeTests_WellKnownAttributes.cs
YairHalberstadt Jan 26, 2019
9c30cc3
Merge remote-tracking branch 'upstream/master' into AllowObsoleteOnAc…
YairHalberstadt Mar 11, 2019
da56be5
Allow Deprecated Attribute on property accessors.
YairHalberstadt Mar 14, 2019
451c738
Fix comment in SourceMemberMethodSymbol.VerifyObsoleteAttributeApplie…
YairHalberstadt Mar 14, 2019
b2b7adf
merged from master
YairHalberstadt Mar 14, 2019
4755eaa
Change feature message from 'obsolete property accessors' to 'obsolet…
YairHalberstadt Mar 14, 2019
c327ac2
Changes based on code review. Add more tests for obsolete on accessor…
YairHalberstadt Mar 22, 2019
dc32918
Report obsolete from setter on attribute property
YairHalberstadt Mar 22, 2019
904f95d
Fix typo
YairHalberstadt Mar 22, 2019
d1e4cfd
Add test for obsolete attribute on indexer accessors.
YairHalberstadt Mar 22, 2019
921240d
Replace Foo with Boo
YairHalberstadt Mar 23, 2019
4bc5bcb
Deal correctly with an obsolete inherited set method on an attribute
YairHalberstadt Mar 31, 2019
a57e934
merged from master
YairHalberstadt Mar 31, 2019
ad5175d
Reorder MessageId usages
YairHalberstadt Apr 1, 2019
9a563e3
Undo formatting change
YairHalberstadt Apr 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.Designer.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 7 additions & 1 deletion src/Compilers/CSharp/Portable/CSharpResources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5796,4 +5796,10 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_FeatureInPreview" xml:space="preserve">
<value>The feature '{0}' is currently in Preview and *unsupported*. To use Preview features, use the 'preview' language version.</value>
</data>
</root>
<data name="ERR_AttributeNotOnEventAccessor" xml:space="preserve">
<value>Attribute '{0}' is not valid on event accessors. It is only valid on '{1}' declarations.</value>
</data>
<data name="IDS_FeatureObsoleteOnPropertyAccessor" xml:space="preserve">
<value>obsolete property accessors</value>
</data>
</root>
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1598,7 +1598,7 @@ internal enum ErrorCode
ERR_PossibleAsyncIteratorWithoutYieldOrAwait = 8420,
ERR_StaticLocalFunctionCannotCaptureVariable = 8421,
ERR_StaticLocalFunctionCannotCaptureThis = 8422,

ERR_AttributeNotOnEventAccessor = 8423,
Copy link
Contributor Author

@YairHalberstadt YairHalberstadt Mar 14, 2019 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

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

#region diagnostics introduced for recursive patterns
// 8501, // available
ERR_WrongNumberOfSubpatterns = 8502,
Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/MessageID.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ internal enum MessageID
IDS_FeatureStaticLocalFunctions = MessageBase + 12755,
IDS_FeatureNameShadowingInNestedFunctions = MessageBase + 12756,
IDS_FeatureUnmanagedConstructedTypes = MessageBase + 12757,
IDS_FeatureObsoleteOnPropertyAccessor = MessageBase + 12758,
}

// Message IDs may refer to strings that need to be localized.
Expand Down Expand Up @@ -279,6 +280,7 @@ internal static LanguageVersion RequiredVersion(this MessageID feature)
case MessageID.IDS_FeatureStaticLocalFunctions:
case MessageID.IDS_FeatureNameShadowingInNestedFunctions:
case MessageID.IDS_FeatureUnmanagedConstructedTypes: // semantic check
case MessageID.IDS_FeatureObsoleteOnPropertyAccessor:
return LanguageVersion.CSharp8;

// C# 7.3 features.
Expand Down
17 changes: 10 additions & 7 deletions src/Compilers/CSharp/Portable/Symbols/ObsoleteAttributeHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,7 @@ private static ThreeState GetObsoleteContextState(Symbol symbol, bool forceCompl
{
while ((object)symbol != null)
{
// For property or event accessors, check the associated property or event instead.
if (symbol.IsAccessor())
{
symbol = ((MethodSymbol)symbol).AssociatedSymbol;
}
else if (symbol.Kind == SymbolKind.Field)
if (symbol.Kind == SymbolKind.Field)
{
// If this is the backing field of an event, look at the event instead.
var associatedSymbol = ((FieldSymbol)symbol).AssociatedSymbol;
Expand All @@ -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())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

{
symbol = ((MethodSymbol)symbol).AssociatedSymbol;
}
else
{
symbol = symbol.ContainingSymbol;
}
}

return ThreeState.False;
Expand Down
11 changes: 8 additions & 3 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1216,9 +1216,14 @@ private bool VerifyObsoleteAttributeAppliedToMethod(
{
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());
if (this is SourceEventAccessorSymbol)
{
// 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());
}
Copy link
Member

@jcouv jcouv Mar 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

} [](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


MessageID.IDS_FeatureObsoleteOnPropertyAccessor.CheckFeatureAvailability(arguments.Diagnostics, arguments.AttributeSyntaxOpt.Location);
}

return true;
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@
<target state="new">It is not legal to use nullable reference type '{0}?' in an as expression; use the underlying type '{0}' instead.</target>
<note />
</trans-unit>
<trans-unit id="ERR_AttributeNotOnEventAccessor">
<source>Attribute '{0}' is not valid on event accessors. It is only valid on '{1}' declarations.</source>
<target state="new">Attribute '{0}' is not valid on event accessors. It is only valid on '{1}' declarations.</target>
<note />
</trans-unit>
<trans-unit id="ERR_AwaitForEachMissingMember">
<source>Asynchronous foreach statement cannot operate on variables of type '{0}' because '{0}' does not contain a suitable public instance definition for '{1}'</source>
<target state="translated">Asynchronní příkaz foreach nejde použít pro proměnné typu {0}, protože {0} neobsahuje vhodnou veřejnou definici instance pro {1}.</target>
Expand Down Expand Up @@ -712,6 +717,11 @@
<target state="translated">omezení obecného typu objektu</target>
<note />
</trans-unit>
<trans-unit id="IDS_FeatureObsoleteOnPropertyAccessor">
<source>obsolete property accessors</source>
<target state="new">obsolete property accessors</target>
<note />
</trans-unit>
<trans-unit id="IDS_FeaturePragmaWarningEnableOrSafeOnly">
<source>warning action enable or safeonly</source>
<target state="new">warning action enable or safeonly</target>
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.de.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@
<target state="new">It is not legal to use nullable reference type '{0}?' in an as expression; use the underlying type '{0}' instead.</target>
<note />
</trans-unit>
<trans-unit id="ERR_AttributeNotOnEventAccessor">
<source>Attribute '{0}' is not valid on event accessors. It is only valid on '{1}' declarations.</source>
<target state="new">Attribute '{0}' is not valid on event accessors. It is only valid on '{1}' declarations.</target>
<note />
</trans-unit>
<trans-unit id="ERR_AwaitForEachMissingMember">
<source>Asynchronous foreach statement cannot operate on variables of type '{0}' because '{0}' does not contain a suitable public instance definition for '{1}'</source>
<target state="translated">Eine asynchrone foreach-Anweisung kann nicht für Variablen vom Typ "{0}" verwendet werden, weil "{0}" keine geeignete öffentliche Instanzdefinition für "{1}" enthält.</target>
Expand Down Expand Up @@ -712,6 +717,11 @@
<target state="translated">Einschränkung eines generischen Objekttyps</target>
<note />
</trans-unit>
<trans-unit id="IDS_FeatureObsoleteOnPropertyAccessor">
<source>obsolete property accessors</source>
<target state="new">obsolete property accessors</target>
<note />
</trans-unit>
<trans-unit id="IDS_FeaturePragmaWarningEnableOrSafeOnly">
<source>warning action enable or safeonly</source>
<target state="new">warning action enable or safeonly</target>
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.es.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@
<target state="new">It is not legal to use nullable reference type '{0}?' in an as expression; use the underlying type '{0}' instead.</target>
<note />
</trans-unit>
<trans-unit id="ERR_AttributeNotOnEventAccessor">
<source>Attribute '{0}' is not valid on event accessors. It is only valid on '{1}' declarations.</source>
<target state="new">Attribute '{0}' is not valid on event accessors. It is only valid on '{1}' declarations.</target>
<note />
</trans-unit>
<trans-unit id="ERR_AwaitForEachMissingMember">
<source>Asynchronous foreach statement cannot operate on variables of type '{0}' because '{0}' does not contain a suitable public instance definition for '{1}'</source>
<target state="translated">Una instrucción foreach asincrónica no puede funcionar en variables de tipo "{0}", porque "{0}" no contiene una definición de instancia pública adecuada para "{1}".</target>
Expand Down Expand Up @@ -712,6 +717,11 @@
<target state="translated">restricción de tipo genérico de objeto</target>
<note />
</trans-unit>
<trans-unit id="IDS_FeatureObsoleteOnPropertyAccessor">
<source>obsolete property accessors</source>
<target state="new">obsolete property accessors</target>
<note />
</trans-unit>
<trans-unit id="IDS_FeaturePragmaWarningEnableOrSafeOnly">
<source>warning action enable or safeonly</source>
<target state="new">warning action enable or safeonly</target>
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.fr.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@
<target state="new">It is not legal to use nullable reference type '{0}?' in an as expression; use the underlying type '{0}' instead.</target>
<note />
</trans-unit>
<trans-unit id="ERR_AttributeNotOnEventAccessor">
<source>Attribute '{0}' is not valid on event accessors. It is only valid on '{1}' declarations.</source>
<target state="new">Attribute '{0}' is not valid on event accessors. It is only valid on '{1}' declarations.</target>
<note />
</trans-unit>
<trans-unit id="ERR_AwaitForEachMissingMember">
<source>Asynchronous foreach statement cannot operate on variables of type '{0}' because '{0}' does not contain a suitable public instance definition for '{1}'</source>
<target state="translated">L'instruction foreach asynchrone ne peut pas fonctionner sur des variables de type '{0}', car '{0}' ne contient aucune définition d'instance publique appropriée pour '{1}'</target>
Expand Down Expand Up @@ -712,6 +717,11 @@
<target state="translated">contrainte de type générique d'objet</target>
<note />
</trans-unit>
<trans-unit id="IDS_FeatureObsoleteOnPropertyAccessor">
<source>obsolete property accessors</source>
<target state="new">obsolete property accessors</target>
<note />
</trans-unit>
<trans-unit id="IDS_FeaturePragmaWarningEnableOrSafeOnly">
<source>warning action enable or safeonly</source>
<target state="new">warning action enable or safeonly</target>
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.it.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@
<target state="new">It is not legal to use nullable reference type '{0}?' in an as expression; use the underlying type '{0}' instead.</target>
<note />
</trans-unit>
<trans-unit id="ERR_AttributeNotOnEventAccessor">
<source>Attribute '{0}' is not valid on event accessors. It is only valid on '{1}' declarations.</source>
<target state="new">Attribute '{0}' is not valid on event accessors. It is only valid on '{1}' declarations.</target>
<note />
</trans-unit>
<trans-unit id="ERR_AwaitForEachMissingMember">
<source>Asynchronous foreach statement cannot operate on variables of type '{0}' because '{0}' does not contain a suitable public instance definition for '{1}'</source>
<target state="translated">L'istruzione foreach asincrona non può funzionare con variabili di tipo '{0}' perché '{0}' non contiene una definizione di istanza pubblica idonea per '{1}'</target>
Expand Down Expand Up @@ -712,6 +717,11 @@
<target state="translated">vincolo di tipo generico object</target>
<note />
</trans-unit>
<trans-unit id="IDS_FeatureObsoleteOnPropertyAccessor">
<source>obsolete property accessors</source>
<target state="new">obsolete property accessors</target>
<note />
</trans-unit>
<trans-unit id="IDS_FeaturePragmaWarningEnableOrSafeOnly">
<source>warning action enable or safeonly</source>
<target state="new">warning action enable or safeonly</target>
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ja.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@
<target state="new">It is not legal to use nullable reference type '{0}?' in an as expression; use the underlying type '{0}' instead.</target>
<note />
</trans-unit>
<trans-unit id="ERR_AttributeNotOnEventAccessor">
<source>Attribute '{0}' is not valid on event accessors. It is only valid on '{1}' declarations.</source>
<target state="new">Attribute '{0}' is not valid on event accessors. It is only valid on '{1}' declarations.</target>
<note />
</trans-unit>
<trans-unit id="ERR_AwaitForEachMissingMember">
<source>Asynchronous foreach statement cannot operate on variables of type '{0}' because '{0}' does not contain a suitable public instance definition for '{1}'</source>
<target state="translated">'{0}' は '{1}' の適切なパブリック インスタンス定義を含んでいないため、型 '{0}' の変数に対して非同期 foreach ステートメントを使用することはできません</target>
Expand Down Expand Up @@ -712,6 +717,11 @@
<target state="translated">オブジェクト ジェネリック型の制約</target>
<note />
</trans-unit>
<trans-unit id="IDS_FeatureObsoleteOnPropertyAccessor">
<source>obsolete property accessors</source>
<target state="new">obsolete property accessors</target>
<note />
</trans-unit>
<trans-unit id="IDS_FeaturePragmaWarningEnableOrSafeOnly">
<source>warning action enable or safeonly</source>
<target state="new">warning action enable or safeonly</target>
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ko.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@
<target state="new">It is not legal to use nullable reference type '{0}?' in an as expression; use the underlying type '{0}' instead.</target>
<note />
</trans-unit>
<trans-unit id="ERR_AttributeNotOnEventAccessor">
<source>Attribute '{0}' is not valid on event accessors. It is only valid on '{1}' declarations.</source>
<target state="new">Attribute '{0}' is not valid on event accessors. It is only valid on '{1}' declarations.</target>
<note />
</trans-unit>
<trans-unit id="ERR_AwaitForEachMissingMember">
<source>Asynchronous foreach statement cannot operate on variables of type '{0}' because '{0}' does not contain a suitable public instance definition for '{1}'</source>
<target state="translated">'{0}' 형식 변수에서 비동기 foreach 문을 수행할 수 없습니다. '{0}'에는 '{1}'의 적합한 공용 인스턴스 정의가 없기 때문입니다.</target>
Expand Down Expand Up @@ -712,6 +717,11 @@
<target state="translated">개체 제네릭 형식 제약 조건</target>
<note />
</trans-unit>
<trans-unit id="IDS_FeatureObsoleteOnPropertyAccessor">
<source>obsolete property accessors</source>
<target state="new">obsolete property accessors</target>
<note />
</trans-unit>
<trans-unit id="IDS_FeaturePragmaWarningEnableOrSafeOnly">
<source>warning action enable or safeonly</source>
<target state="new">warning action enable or safeonly</target>
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.pl.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@
<target state="new">It is not legal to use nullable reference type '{0}?' in an as expression; use the underlying type '{0}' instead.</target>
<note />
</trans-unit>
<trans-unit id="ERR_AttributeNotOnEventAccessor">
<source>Attribute '{0}' is not valid on event accessors. It is only valid on '{1}' declarations.</source>
<target state="new">Attribute '{0}' is not valid on event accessors. It is only valid on '{1}' declarations.</target>
<note />
</trans-unit>
<trans-unit id="ERR_AwaitForEachMissingMember">
<source>Asynchronous foreach statement cannot operate on variables of type '{0}' because '{0}' does not contain a suitable public instance definition for '{1}'</source>
<target state="translated">Asynchroniczna instrukcja foreach nie może operować na zmiennych typu „{0}”, ponieważ typ „{0}” nie zawiera odpowiedniej publicznej definicji wystąpienia elementu „{1}”</target>
Expand Down Expand Up @@ -712,6 +717,11 @@
<target state="translated">ogólne ograniczenie typu obiektu</target>
<note />
</trans-unit>
<trans-unit id="IDS_FeatureObsoleteOnPropertyAccessor">
<source>obsolete property accessors</source>
<target state="new">obsolete property accessors</target>
<note />
</trans-unit>
<trans-unit id="IDS_FeaturePragmaWarningEnableOrSafeOnly">
<source>warning action enable or safeonly</source>
<target state="new">warning action enable or safeonly</target>
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.pt-BR.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@
<target state="new">It is not legal to use nullable reference type '{0}?' in an as expression; use the underlying type '{0}' instead.</target>
<note />
</trans-unit>
<trans-unit id="ERR_AttributeNotOnEventAccessor">
<source>Attribute '{0}' is not valid on event accessors. It is only valid on '{1}' declarations.</source>
<target state="new">Attribute '{0}' is not valid on event accessors. It is only valid on '{1}' declarations.</target>
<note />
</trans-unit>
<trans-unit id="ERR_AwaitForEachMissingMember">
<source>Asynchronous foreach statement cannot operate on variables of type '{0}' because '{0}' does not contain a suitable public instance definition for '{1}'</source>
<target state="translated">A instrução foreach assíncrona não pode operar em variáveis do tipo '{0}' porque '{0}' não contém uma definição da instância pública adequada para '{1}'</target>
Expand Down Expand Up @@ -712,6 +717,11 @@
<target state="translated">restrição de tipo genérico de objeto</target>
<note />
</trans-unit>
<trans-unit id="IDS_FeatureObsoleteOnPropertyAccessor">
<source>obsolete property accessors</source>
<target state="new">obsolete property accessors</target>
<note />
</trans-unit>
<trans-unit id="IDS_FeaturePragmaWarningEnableOrSafeOnly">
<source>warning action enable or safeonly</source>
<target state="new">warning action enable or safeonly</target>
Expand Down
Loading