Skip to content

Commit

Permalink
Add warning when property has a backing field but a manually implemen…
Browse files Browse the repository at this point in the history
…ted accessor does not use it (#75325)
  • Loading branch information
RikkiGibson authored Oct 16, 2024
1 parent 5a5854a commit 69b7f75
Show file tree
Hide file tree
Showing 27 changed files with 957 additions and 92 deletions.
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

0 comments on commit 69b7f75

Please sign in to comment.