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

Add warning when property has a backing field but a manually implemented accessor does not use it #75325

Merged
merged 11 commits into from
Oct 16, 2024
Merged
6 changes: 6 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -8008,4 +8008,10 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="IDS_FeatureFirstClassSpan" xml:space="preserve">
<value>first-class Span types</value>
</data>
<data name="WRN_AccessorDoesNotUseBackingField" xml:space="preserve">
<value>The '{0}' accessor of property '{1}' should use 'field' because the other accessor is using it.</value>
</data>
<data name="WRN_AccessorDoesNotUseBackingField_Title" xml:space="preserve">
<value>Property accessor should use 'field' because the other accessor is using it.</value>
</data>
</root>
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2347,6 +2347,7 @@ internal enum ErrorCode

WRN_UninitializedNonNullableBackingField = 9264,
WRN_UnassignedInternalRefField = 9265,
WRN_AccessorDoesNotUseBackingField = 9266,

// Note: you will need to do the following after adding errors:
// 1) Update ErrorFacts.IsBuildOnlyDiagnostic (src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs)
Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,7 @@ internal static int GetWarningLevel(ErrorCode code)
case ErrorCode.WRN_PartialPropertySignatureDifference:
case ErrorCode.WRN_FieldIsAmbiguous:
case ErrorCode.WRN_UninitializedNonNullableBackingField:
case ErrorCode.WRN_AccessorDoesNotUseBackingField:
return 1;
default:
return 0;
Expand Down Expand Up @@ -2464,6 +2465,7 @@ or ErrorCode.ERR_CannotApplyOverloadResolutionPriorityToMember
or ErrorCode.ERR_PartialPropertyDuplicateInitializer
or ErrorCode.WRN_UninitializedNonNullableBackingField
or ErrorCode.WRN_UnassignedInternalRefField
or ErrorCode.WRN_AccessorDoesNotUseBackingField
=> false,
};
#pragma warning restore CS8524 // The switch expression does not handle some values of its input type (it is not exhaustive) involving an unnamed enum value.
Expand Down

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

Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,12 @@ private static SourcePropertySymbol Create(
out bool isExpressionBodied,
out bool hasGetAccessorImplementation,
out bool hasSetAccessorImplementation,
out bool usesFieldKeyword,
out bool getterUsesFieldKeyword,
out bool setterUsesFieldKeyword,
out var getSyntax,
out var setSyntax);

Debug.Assert(!usesFieldKeyword ||
Debug.Assert(!(getterUsesFieldKeyword || setterUsesFieldKeyword) ||
((CSharpParseOptions)syntax.SyntaxTree.Options).IsFeatureEnabled(MessageID.IDS_FeatureFieldKeyword));

bool accessorsHaveImplementation = hasGetAccessorImplementation || hasSetAccessorImplementation;
Expand Down Expand Up @@ -90,7 +91,8 @@ private static SourcePropertySymbol Create(
hasAutoPropertySet: hasAutoPropertySet,
isExpressionBodied: isExpressionBodied,
accessorsHaveImplementation: accessorsHaveImplementation,
usesFieldKeyword: usesFieldKeyword,
getterUsesFieldKeyword: getterUsesFieldKeyword,
setterUsesFieldKeyword: setterUsesFieldKeyword,
memberName,
location,
diagnostics);
Expand All @@ -110,7 +112,8 @@ private SourcePropertySymbol(
bool hasAutoPropertySet,
bool isExpressionBodied,
bool accessorsHaveImplementation,
bool usesFieldKeyword,
bool getterUsesFieldKeyword,
bool setterUsesFieldKeyword,
string memberName,
Location location,
BindingDiagnosticBag diagnostics)
Expand All @@ -129,7 +132,8 @@ private SourcePropertySymbol(
hasAutoPropertySet: hasAutoPropertySet,
isExpressionBodied: isExpressionBodied,
accessorsHaveImplementation: accessorsHaveImplementation,
usesFieldKeyword: usesFieldKeyword,
getterUsesFieldKeyword: getterUsesFieldKeyword,
setterUsesFieldKeyword: setterUsesFieldKeyword,
syntax.Type.SkipScoped(out _).GetRefKindInLocalOrReturn(diagnostics),
memberName,
syntax.AttributeLists,
Expand Down Expand Up @@ -214,7 +218,8 @@ private static void GetAccessorDeclarations(
out bool isExpressionBodied,
out bool hasGetAccessorImplementation,
out bool hasSetAccessorImplementation,
out bool usesFieldKeyword,
out bool getterUsesFieldKeyword,
out bool setterUsesFieldKeyword,
out AccessorDeclarationSyntax? getSyntax,
out AccessorDeclarationSyntax? setSyntax)
{
Expand All @@ -225,7 +230,8 @@ private static void GetAccessorDeclarations(

if (!isExpressionBodied)
{
usesFieldKeyword = false;
getterUsesFieldKeyword = false;
setterUsesFieldKeyword = false;
hasGetAccessorImplementation = false;
hasSetAccessorImplementation = false;
foreach (var accessor in syntax.AccessorList!.Accessors)
Expand All @@ -237,6 +243,7 @@ private static void GetAccessorDeclarations(
{
getSyntax = accessor;
hasGetAccessorImplementation = hasImplementation(accessor);
getterUsesFieldKeyword = containsFieldExpressionInAccessor(accessor);
}
else
{
Expand All @@ -249,6 +256,7 @@ private static void GetAccessorDeclarations(
{
setSyntax = accessor;
hasSetAccessorImplementation = hasImplementation(accessor);
setterUsesFieldKeyword = containsFieldExpressionInAccessor(accessor);
}
else
{
Expand All @@ -266,16 +274,15 @@ private static void GetAccessorDeclarations(
default:
throw ExceptionUtilities.UnexpectedValue(accessor.Kind());
}

usesFieldKeyword = usesFieldKeyword || containsFieldExpressionInAccessor(accessor);
}
}
else
{
var body = GetArrowExpression(syntax);
hasGetAccessorImplementation = body is object;
hasSetAccessorImplementation = false;
usesFieldKeyword = body is { } && containsFieldExpressionInGreenNode(body.Green);
getterUsesFieldKeyword = body is { } && containsFieldExpressionInGreenNode(body.Green);
setterUsesFieldKeyword = false;
Debug.Assert(hasGetAccessorImplementation); // it's not clear how this even parsed as a property if it has no accessor list and no arrow expression.
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,13 @@ private enum Flags : ushort
IsExpressionBodied = 1 << 0,
HasAutoPropertyGet = 1 << 1,
HasAutoPropertySet = 1 << 2,
UsesFieldKeyword = 1 << 3,
IsExplicitInterfaceImplementation = 1 << 4,
HasInitializer = 1 << 5,
AccessorsHaveImplementation = 1 << 6,
HasExplicitAccessModifier = 1 << 7,
RequiresBackingField = 1 << 8,
GetterUsesFieldKeyword = 1 << 3,
SetterUsesFieldKeyword = 1 << 4,
IsExplicitInterfaceImplementation = 1 << 5,
HasInitializer = 1 << 6,
AccessorsHaveImplementation = 1 << 7,
HasExplicitAccessModifier = 1 << 8,
RequiresBackingField = 1 << 9,
}

// TODO (tomat): consider splitting into multiple subclasses/rare data.
Expand Down Expand Up @@ -90,7 +91,8 @@ protected SourcePropertySymbolBase(
bool hasAutoPropertySet,
bool isExpressionBodied,
bool accessorsHaveImplementation,
bool usesFieldKeyword,
bool getterUsesFieldKeyword,
bool setterUsesFieldKeyword,
RefKind refKind,
string memberName,
SyntaxList<AttributeListSyntax> indexerNameAttributeLists,
Expand Down Expand Up @@ -133,9 +135,14 @@ protected SourcePropertySymbolBase(
_propertyFlags |= Flags.HasAutoPropertySet;
}

if (usesFieldKeyword)
if (getterUsesFieldKeyword)
{
_propertyFlags |= Flags.UsesFieldKeyword;
_propertyFlags |= Flags.GetterUsesFieldKeyword;
}

if (setterUsesFieldKeyword)
{
_propertyFlags |= Flags.SetterUsesFieldKeyword;
}

if (hasInitializer)
Expand Down Expand Up @@ -171,7 +178,7 @@ protected SourcePropertySymbolBase(
_name = _lazySourceName = memberName;
}

if (usesFieldKeyword || hasAutoPropertyGet || hasAutoPropertySet || hasInitializer)
if (getterUsesFieldKeyword || setterUsesFieldKeyword || hasAutoPropertyGet || hasAutoPropertySet || hasInitializer)
{
Debug.Assert(!IsIndexer);
_propertyFlags |= Flags.RequiresBackingField;
Expand Down Expand Up @@ -305,6 +312,48 @@ protected void CheckInitializerIfNeeded(BindingDiagnosticBag diagnostics)
}
}

#nullable enable
private static void CheckFieldKeywordUsage(SourcePropertySymbolBase property, BindingDiagnosticBag diagnostics)
{
Debug.Assert(property.PartialImplementationPart is null);
if (!property.DeclaringCompilation.IsFeatureEnabled(MessageID.IDS_FeatureFieldKeyword))
{
return;
}

SourcePropertyAccessorSymbol? accessorToBlame = null;
var propertyFlags = property._propertyFlags;
var getterUsesFieldKeyword = (propertyFlags & Flags.GetterUsesFieldKeyword) != 0;
var setterUsesFieldKeyword = (propertyFlags & Flags.SetterUsesFieldKeyword) != 0;
if (property._setMethod is { IsAutoPropertyAccessor: false } setMethod
&& !setterUsesFieldKeyword
&& !property.IsSetOnEitherPart(Flags.HasInitializer)
&& (property.HasAutoPropertyGet || getterUsesFieldKeyword))
{
accessorToBlame = setMethod;
}
else if (property._getMethod is { IsAutoPropertyAccessor: false } getMethod
&& !getterUsesFieldKeyword
&& (property.HasAutoPropertySet || setterUsesFieldKeyword))
{
accessorToBlame = getMethod;
}

if (accessorToBlame is not null)
{
var accessorName = accessorToBlame switch
{
{ MethodKind: MethodKind.PropertyGet, IsInitOnly: false } => SyntaxFacts.GetText(SyntaxKind.GetKeyword),
{ MethodKind: MethodKind.PropertySet, IsInitOnly: false } => SyntaxFacts.GetText(SyntaxKind.SetKeyword),
{ MethodKind: MethodKind.PropertySet, IsInitOnly: true } => SyntaxFacts.GetText(SyntaxKind.InitKeyword),
_ => throw ExceptionUtilities.UnexpectedValue(accessorToBlame)
};

diagnostics.Add(ErrorCode.WRN_AccessorDoesNotUseBackingField, accessorToBlame.GetFirstLocation(), accessorName, property);
}
}
#nullable disable

public sealed override RefKind RefKind
{
get
Expand Down Expand Up @@ -648,10 +697,10 @@ public bool HasSkipLocalsInitAttribute
}

internal bool IsAutoPropertyOrUsesFieldKeyword
=> IsSetOnEitherPart(Flags.HasAutoPropertyGet | Flags.HasAutoPropertySet | Flags.UsesFieldKeyword);
=> IsSetOnEitherPart(Flags.HasAutoPropertyGet | Flags.HasAutoPropertySet | Flags.GetterUsesFieldKeyword | Flags.SetterUsesFieldKeyword);

internal bool UsesFieldKeyword
=> IsSetOnEitherPart(Flags.UsesFieldKeyword);
=> IsSetOnEitherPart(Flags.GetterUsesFieldKeyword | Flags.SetterUsesFieldKeyword);

protected bool HasExplicitAccessModifier
=> (_propertyFlags & Flags.HasExplicitAccessModifier) != 0;
Expand Down Expand Up @@ -811,6 +860,7 @@ internal override void AfterAddingTypeMembersChecks(ConversionsBase conversions,
this.CheckModifiers(isExplicitInterfaceImplementation, Location, IsIndexer, diagnostics);

CheckInitializerIfNeeded(diagnostics);
CheckFieldKeywordUsage((SourcePropertySymbolBase?)PartialImplementationPart ?? this, diagnostics);

if (RefKind != RefKind.None && IsRequired)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ public SynthesizedRecordEqualityContractProperty(SourceMemberContainerTypeSymbol
hasAutoPropertySet: false,
isExpressionBodied: false,
accessorsHaveImplementation: true,
usesFieldKeyword: false,
getterUsesFieldKeyword: false,
setterUsesFieldKeyword: false,
RefKind.None,
PropertyName,
indexerNameAttributeLists: new SyntaxList<AttributeListSyntax>(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ public SynthesizedRecordPropertySymbol(
hasAutoPropertySet: true,
isExpressionBodied: false,
accessorsHaveImplementation: true,
usesFieldKeyword: false,
getterUsesFieldKeyword: false,
setterUsesFieldKeyword: false,
RefKind.None,
backingParameter.Name,
indexerNameAttributeLists: new SyntaxList<AttributeListSyntax>(),
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf

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

10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.de.xlf

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

10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.es.xlf

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

10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.fr.xlf

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

10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.it.xlf

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

Loading
Loading