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

Handle various cases for field-backed properties #74641

Merged
merged 19 commits into from
Aug 12, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -1765,7 +1765,7 @@ private static bool AccessingAutoPropertyFromConstructor(BoundExpression receive
var propertyIsStatic = propertySymbol.IsStatic;

return (object)sourceProperty != null &&
sourceProperty.IsAutoPropertyWithGetAccessor &&
sourceProperty.IsAutoProperty &&
TypeSymbol.Equals(sourceProperty.ContainingType, fromMember.ContainingType, TypeCompareKind.AllIgnoreOptions) &&
IsConstructorOrField(fromMember, isStatic: propertyIsStatic) &&
(propertyIsStatic || receiver.Kind == BoundKind.ThisReference);
Expand Down
3 changes: 1 addition & 2 deletions src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1879,8 +1879,7 @@ syntaxNode is ConstructorDeclarationSyntax constructorSyntax &&
}
else
{
var property = sourceMethod.AssociatedSymbol as SourcePropertySymbolBase;
if (property is not null && property.IsAutoPropertyWithGetAccessor)
if (sourceMethod is SourcePropertyAccessorSymbol { IsAutoPropertyAccessor: true })
{
return MethodBodySynthesizer.ConstructAutoPropertyAccessorBody(sourceMethod);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1075,7 +1075,7 @@ static IEnumerable<Symbol> getAllMembersToBeDefaulted(Symbol requiredMember)
}

static Symbol getFieldSymbolToBeInitialized(Symbol requiredMember)
=> requiredMember is SourcePropertySymbol { IsAutoPropertyWithGetAccessor: true } prop ? prop.BackingField : requiredMember;
=> requiredMember is SourcePropertySymbol { IsAutoProperty: true } prop ? prop.BackingField : requiredMember;
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ private BoundExpression MakePropertyAssignment(
if (setMethod is null)
{
var autoProp = (SourcePropertySymbolBase)property.OriginalDefinition;
Debug.Assert(autoProp.IsAutoPropertyWithGetAccessor,
Debug.Assert(autoProp.IsAutoProperty,
"only autoproperties can be assignable without having setters");
Debug.Assert(property.Equals(autoProp, TypeCompareKind.IgnoreNullableModifiersForReferenceTypes));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -485,6 +485,8 @@ internal sealed override bool IsDeclaredReadOnly
}
}

internal bool IsAutoPropertyAccessor => _isAutoPropertyAccessor;

internal sealed override bool IsInitOnly => !IsStatic && _usesInit;

private static DeclarationModifiers MakeModifiers(NamedTypeSymbol containingType, SyntaxTokenList modifiers, bool isExplicitInterfaceImplementation,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,16 @@ private static SourcePropertySymbol Create(
GetAccessorDeclarations(
syntax,
diagnostics,
out bool hasAccessorList,
out bool accessorsHaveImplementation,
out bool isExpressionBodied,
out bool hasGetAccessorImplementation,
out bool hasSetAccessorImplementation,
out bool usesFieldKeyword,
out bool isInitOnly,
out var getSyntax,
out var setSyntax);

bool accessorsHaveImplementation = hasGetAccessorImplementation || hasSetAccessorImplementation; // PROTOTYPE: Remove if not needed.

var explicitInterfaceSpecifier = GetExplicitInterfaceSpecifier(syntax);
SyntaxTokenList modifiersTokenList = GetModifierTokensSyntax(syntax);
bool isExplicitInterfaceImplementation = explicitInterfaceSpecifier is object;
Expand All @@ -60,8 +63,8 @@ private static SourcePropertySymbol Create(
diagnostics,
out _);

bool isAutoProperty = (modifiers & DeclarationModifiers.Partial) == 0 && !accessorsHaveImplementation;
bool isExpressionBodied = !hasAccessorList && GetArrowExpression(syntax) != null;
bool hasAutoPropertyGet = (modifiers & DeclarationModifiers.Partial) == 0 && getSyntax != null && !hasGetAccessorImplementation;
bool hasAutoPropertySet = (modifiers & DeclarationModifiers.Partial) == 0 && setSyntax != null && !hasSetAccessorImplementation;

binder = binder.SetOrClearUnsafeRegionIfNecessary(modifiersTokenList);
TypeSymbol? explicitInterfaceType;
Expand All @@ -78,7 +81,8 @@ private static SourcePropertySymbol Create(
aliasQualifierOpt,
modifiers,
hasExplicitAccessMod: hasExplicitAccessMod,
isAutoProperty: isAutoProperty,
hasAutoPropertyGet: hasAutoPropertyGet,
hasAutoPropertySet: hasAutoPropertySet,
isExpressionBodied: isExpressionBodied,
isInitOnly: isInitOnly,
accessorsHaveImplementation: accessorsHaveImplementation,
Expand All @@ -98,7 +102,8 @@ private SourcePropertySymbol(
string? aliasQualifierOpt,
DeclarationModifiers modifiers,
bool hasExplicitAccessMod,
bool isAutoProperty,
bool hasAutoPropertyGet,
bool hasAutoPropertySet,
bool isExpressionBodied,
bool isInitOnly,
bool accessorsHaveImplementation,
Expand All @@ -109,15 +114,16 @@ private SourcePropertySymbol(
: base(
containingType,
syntax,
hasGetAccessor,
hasSetAccessor,
hasGetAccessor: hasGetAccessor,
hasSetAccessor: hasSetAccessor,
isExplicitInterfaceImplementation,
explicitInterfaceType,
aliasQualifierOpt,
modifiers,
hasInitializer: HasInitializer(syntax),
hasExplicitAccessMod: hasExplicitAccessMod,
isAutoProperty: isAutoProperty,
hasAutoPropertyGet: hasAutoPropertyGet,
hasAutoPropertySet: hasAutoPropertySet,
isExpressionBodied: isExpressionBodied,
isInitOnly: isInitOnly,
accessorsHaveImplementation: accessorsHaveImplementation,
Expand Down Expand Up @@ -202,23 +208,25 @@ public override OneOrMany<SyntaxList<AttributeListSyntax>> GetAttributeDeclarati
private static void GetAccessorDeclarations(
CSharpSyntaxNode syntaxNode,
BindingDiagnosticBag diagnostics,
out bool hasAccessorList,
out bool accessorsHaveImplementation,
out bool isExpressionBodied,
out bool hasGetAccessorImplementation,
out bool hasSetAccessorImplementation,
out bool usesFieldKeyword,
out bool isInitOnly,
out CSharpSyntaxNode? getSyntax,
out CSharpSyntaxNode? setSyntax)
{
var syntax = (BasePropertyDeclarationSyntax)syntaxNode;
hasAccessorList = syntax.AccessorList != null;
isExpressionBodied = syntax.AccessorList is null;
getSyntax = null;
setSyntax = null;
isInitOnly = false;

if (hasAccessorList)
if (!isExpressionBodied)
{
usesFieldKeyword = false;
accessorsHaveImplementation = false;
hasGetAccessorImplementation = false;
hasSetAccessorImplementation = false;
foreach (var accessor in syntax.AccessorList!.Accessors)
{
switch (accessor.Kind())
Expand All @@ -227,6 +235,7 @@ private static void GetAccessorDeclarations(
if (getSyntax == null)
{
getSyntax = accessor;
hasGetAccessorImplementation = hasImplementation(accessor);
}
else
{
Expand All @@ -238,6 +247,7 @@ private static void GetAccessorDeclarations(
if (setSyntax == null)
{
setSyntax = accessor;
hasSetAccessorImplementation = hasImplementation(accessor);
if (accessor.Keyword.IsKind(SyntaxKind.InitKeyword))
{
isInitOnly = true;
Expand All @@ -260,21 +270,22 @@ private static void GetAccessorDeclarations(
throw ExceptionUtilities.UnexpectedValue(accessor.Kind());
}

var body = (SyntaxNode?)accessor.Body ?? accessor.ExpressionBody;
if (body != null)
{
accessorsHaveImplementation = true;
}

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

Choose a reason for hiding this comment

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

Since you're modifying this line anyway, consider making this comment an actual assert message.

}

static bool hasImplementation(AccessorDeclarationSyntax accessor)
{
var body = (SyntaxNode?)accessor.Body ?? accessor.ExpressionBody;
return body != null;
}

static bool containsFieldKeyword(SyntaxNode syntax)
Expand Down Expand Up @@ -324,7 +335,7 @@ private static (DeclarationModifiers modifiers, bool hasExplicitAccessMod) MakeM
SyntaxTokenList modifiers,
bool isExplicitInterfaceImplementation,
bool isIndexer,
bool accessorsHaveImplementation,
bool accessorsHaveImplementation, // PROTOTYPE: How is this used?
Location location,
BindingDiagnosticBag diagnostics,
out bool modifierErrors)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,13 @@ internal abstract class SourcePropertySymbolBase : PropertySymbol, IAttributeTar
private enum Flags : byte
{
IsExpressionBodied = 1 << 0,
IsAutoProperty = 1 << 1,
IsExplicitInterfaceImplementation = 1 << 2,
HasInitializer = 1 << 3,
AccessorsHaveImplementation = 1 << 4,
HasExplicitAccessModifier = 1 << 5,
HasAutoPropertyGet = 1 << 1,
HasAutoPropertySet = 1 << 2,
UsesFieldKeyword = 1 << 3,
IsExplicitInterfaceImplementation = 1 << 4,
HasInitializer = 1 << 5,
AccessorsHaveImplementation = 1 << 6,
HasExplicitAccessModifier = 1 << 7,
}

// TODO (tomat): consider splitting into multiple subclasses/rare data.
Expand Down Expand Up @@ -80,7 +82,8 @@ protected SourcePropertySymbolBase(
DeclarationModifiers modifiers,
bool hasInitializer,
bool hasExplicitAccessMod,
bool isAutoProperty,
bool hasAutoPropertyGet,
bool hasAutoPropertySet,
bool isExpressionBodied,
bool isInitOnly,
bool accessorsHaveImplementation,
Expand All @@ -91,7 +94,7 @@ protected SourcePropertySymbolBase(
Location location,
BindingDiagnosticBag diagnostics)
{
Debug.Assert(!isExpressionBodied || !isAutoProperty);
Debug.Assert(!isExpressionBodied || !(hasAutoPropertyGet || hasAutoPropertySet));
Debug.Assert(!isExpressionBodied || !hasInitializer);
Debug.Assert(!isExpressionBodied || accessorsHaveImplementation);
Debug.Assert((modifiers & DeclarationModifiers.Required) == 0 || this is SourcePropertySymbol);
Expand All @@ -113,16 +116,33 @@ protected SourcePropertySymbolBase(
}

bool isIndexer = IsIndexer;
isAutoProperty = isAutoProperty && !(containingType.IsInterface && !IsStatic) && !IsAbstract && !IsExtern && !isIndexer;
if (hasAutoPropertyGet || hasAutoPropertySet)
{
if ((containingType.IsInterface && !IsStatic) || IsAbstract || IsExtern || isIndexer)
{
hasAutoPropertyGet = false;
hasAutoPropertySet = false;
}
}

if (hasExplicitAccessMod)
{
_propertyFlags |= Flags.HasExplicitAccessModifier;
}

if (isAutoProperty)
if (hasAutoPropertyGet)
{
_propertyFlags |= Flags.HasAutoPropertyGet;
}

if (hasAutoPropertySet)
{
_propertyFlags |= Flags.HasAutoPropertySet;
}

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

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

if (usesFieldKeyword || (isAutoProperty && hasGetAccessor) || hasInitializer)
if (usesFieldKeyword || hasAutoPropertyGet || hasAutoPropertySet || hasInitializer)
{
Debug.Assert(!IsIndexer);
string fieldName = GeneratedNames.MakeBackingFieldName(_name);
Expand All @@ -175,12 +195,12 @@ protected SourcePropertySymbolBase(

if (hasGetAccessor)
{
_getMethod = CreateGetAccessorSymbol(isAutoPropertyAccessor: isAutoProperty, diagnostics);
_getMethod = CreateGetAccessorSymbol(hasAutoPropertyGet, diagnostics);
}

if (hasSetAccessor)
{
_setMethod = CreateSetAccessorSymbol(isAutoPropertyAccessor: isAutoProperty, diagnostics);
_setMethod = CreateSetAccessorSymbol(hasAutoPropertySet, diagnostics);
}
}

Expand Down Expand Up @@ -640,14 +660,17 @@ public bool HasSkipLocalsInitAttribute
}
}

internal bool IsAutoPropertyWithGetAccessor
=> IsAutoProperty && _getMethod is object;
internal bool IsAutoPropertyOrUsesFieldKeyword
=> IsAutoProperty || UsesFieldKeyword;

protected bool UsesFieldKeyword
=> (_propertyFlags & Flags.UsesFieldKeyword) != 0;

protected bool HasExplicitAccessModifier
=> (_propertyFlags & Flags.HasExplicitAccessModifier) != 0;

protected bool IsAutoProperty
=> (_propertyFlags & Flags.IsAutoProperty) != 0;
internal bool IsAutoProperty
=> (_propertyFlags & (Flags.HasAutoPropertyGet | Flags.HasAutoPropertySet)) != 0;

protected bool AccessorsHaveImplementation
=> (_propertyFlags & Flags.AccessorsHaveImplementation) != 0;
Expand Down Expand Up @@ -702,10 +725,8 @@ internal override void AfterAddingTypeMembersChecks(ConversionsBase conversions,
diagnostics.Add(ErrorCode.ERR_RefReturningPropertiesCannotBeRequired, Location);
}

if (IsAutoPropertyWithGetAccessor)
if (IsAutoProperty)
{
Debug.Assert(GetMethod is object);

if (!IsStatic && SetMethod is { IsInitOnly: false })
{
if (ContainingType.IsReadOnly)
Expand Down Expand Up @@ -1084,7 +1105,7 @@ private SynthesizedSealedPropertyAccessor MakeSynthesizedSealedAccessor()
AttributeLocation IAttributeTargetSymbol.DefaultAttributeLocation => AttributeLocation.Property;

AttributeLocation IAttributeTargetSymbol.AllowedAttributeLocations
=> IsAutoPropertyWithGetAccessor
=> IsAutoPropertyOrUsesFieldKeyword
? AttributeLocation.Property | AttributeLocation.Field
: AttributeLocation.Property;

Expand Down Expand Up @@ -1649,7 +1670,7 @@ protected virtual void ValidatePropertyType(BindingDiagnosticBag diagnostics)
{
diagnostics.Add(ErrorCode.ERR_FieldCantBeRefAny, TypeLocation, type);
}
else if (this.IsAutoPropertyWithGetAccessor && type.IsRefLikeOrAllowsRefLikeType() && (this.IsStatic || !this.ContainingType.IsRefLikeType))
else if (this.IsAutoPropertyOrUsesFieldKeyword && type.IsRefLikeOrAllowsRefLikeType() && (this.IsStatic || !this.ContainingType.IsRefLikeType))
{
diagnostics.Add(ErrorCode.ERR_FieldAutoPropCantBeByRefLike, TypeLocation, type);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ public SynthesizedRecordEqualityContractProperty(SourceMemberContainerTypeSymbol
},
hasInitializer: false,
hasExplicitAccessMod: false,
isAutoProperty: false,
hasAutoPropertyGet: false,
hasAutoPropertySet: false,
isExpressionBodied: false,
isInitOnly: false,
accessorsHaveImplementation: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ public SynthesizedRecordPropertySymbol(
modifiers: DeclarationModifiers.Public | (isOverride ? DeclarationModifiers.Override : DeclarationModifiers.None),
hasInitializer: true, // Synthesized record properties always have a synthesized initializer
hasExplicitAccessMod: false,
isAutoProperty: true,
hasAutoPropertyGet: true,
hasAutoPropertySet: true,
isExpressionBodied: false,
isInitOnly: ShouldUseInit(containingType),
accessorsHaveImplementation: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ internal override void PostDecodeWellKnownAttributes(ImmutableArray<CSharpAttrib
{
base.PostDecodeWellKnownAttributes(boundAttributes, allAttributeSyntaxNodes, diagnostics, symbolPart, decodedData);

if (!allAttributeSyntaxNodes.IsEmpty && _property.IsAutoPropertyWithGetAccessor)
if (!allAttributeSyntaxNodes.IsEmpty && _property.IsAutoPropertyOrUsesFieldKeyword)
{
CheckForFieldTargetedAttribute(diagnostics);
}
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Test/Emit/Emit/EmitMetadataTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1294,7 +1294,7 @@ private static void VerifyAutoProperty(PropertySymbol property, bool isFromSourc
{
if (property is SourcePropertySymbol sourceProperty)
{
Assert.True(sourceProperty.IsAutoPropertyWithGetAccessor);
Assert.True(sourceProperty.IsAutoProperty);
}
}
else
Expand Down
Loading