Skip to content

Commit

Permalink
Field-backed properties: definite assignment (#75116)
Browse files Browse the repository at this point in the history
  • Loading branch information
cston authored Sep 23, 2024
1 parent 3bfa347 commit 5aceebc
Show file tree
Hide file tree
Showing 15 changed files with 2,542 additions and 424 deletions.
29 changes: 28 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,11 @@ private static AccessorKind GetIndexerAccessorKind(BoundIndexerAccess indexerAcc
return AccessorKind.Get;
}

return GetAccessorKind(valueKind);
}

private static AccessorKind GetAccessorKind(BindValueKind valueKind)
{
var coreValueKind = valueKind & ValueKindSignificantBitsMask;
return coreValueKind switch
{
Expand Down Expand Up @@ -523,6 +528,28 @@ private BoundExpression CheckValue(BoundExpression expr, BindValueKind valueKind
Debug.Assert(valueKind is (BindValueKind.Assignable or BindValueKind.RefOrOut or BindValueKind.RefAssignable) || diagnostics.DiagnosticBag is null || diagnostics.HasAnyResolvedErrors());
return expr;

case BoundKind.PropertyAccess:
if (!InAttributeArgument)
{
// If the property has a synthesized backing field, record the accessor kind of the property
// access for determining whether the property access can use the backing field directly.
var propertyAccess = (BoundPropertyAccess)expr;
if (HasSynthesizedBackingField(propertyAccess.PropertySymbol, out _))
{
expr = propertyAccess.Update(
propertyAccess.ReceiverOpt,
propertyAccess.InitialBindingReceiverIsSubjectToCloning,
propertyAccess.PropertySymbol,
autoPropertyAccessorKind: GetAccessorKind(valueKind),
propertyAccess.ResultKind,
propertyAccess.Type);
}
}
#if DEBUG
expr.WasPropertyBackingFieldAccessChecked = true;
#endif
break;

case BoundKind.IndexerAccess:
expr = BindIndexerDefaultArgumentsAndParamsCollection((BoundIndexerAccess)expr, valueKind, diagnostics);
break;
Expand Down Expand Up @@ -1694,7 +1721,7 @@ private bool CheckPropertyValueKind(SyntaxNode node, BoundExpression expr, BindV
if (setMethod is null)
{
var containing = this.ContainingMemberOrLambda;
if (!AccessingAutoPropertyFromConstructor(receiver, propertySymbol, containing, allowFieldKeyword: true)
if (!AccessingAutoPropertyFromConstructor(receiver, propertySymbol, containing, AccessorKind.Set)
&& !isAllowedDespiteReadonly(receiver))
{
Error(diagnostics, ErrorCode.ERR_AssgReadonlyProp, node, propertySymbol);
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Attributes.cs
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,7 @@ private BoundAssignmentOperator BindNamedAttributeArgument(AttributeArgumentSynt
var propertySymbol = namedArgumentNameSymbol as PropertySymbol;
if (propertySymbol is object)
{
lvalue = new BoundPropertyAccess(nameSyntax, receiverOpt: null, initialBindingReceiverIsSubjectToCloning: ThreeState.Unknown, propertySymbol, resultKind, namedArgumentType);
lvalue = new BoundPropertyAccess(nameSyntax, receiverOpt: null, initialBindingReceiverIsSubjectToCloning: ThreeState.Unknown, propertySymbol, autoPropertyAccessorKind: AccessorKind.Unknown, resultKind, namedArgumentType);
}
else
{
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8617,7 +8617,7 @@ private BoundExpression BindPropertyAccess(
WarnOnAccessOfOffDefault(node, receiver, diagnostics);
}

return new BoundPropertyAccess(node, receiver, initialBindingReceiverIsSubjectToCloning: ReceiverIsSubjectToCloning(receiver, propertySymbol), propertySymbol, lookupResult, propertySymbol.Type, hasErrors: (hasErrors || hasError));
return new BoundPropertyAccess(node, receiver, initialBindingReceiverIsSubjectToCloning: ReceiverIsSubjectToCloning(receiver, propertySymbol), propertySymbol, autoPropertyAccessorKind: AccessorKind.Unknown, lookupResult, propertySymbol.Type, hasErrors: (hasErrors || hasError));
}
#nullable disable

Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ private bool BindLengthAndIndexerForListPattern(SyntaxNode node, TypeSymbol inpu
hasErrors |= !TryGetSpecialTypeMember(Compilation, SpecialMember.System_Array__Length, node, diagnostics, out PropertySymbol lengthProperty);
if (lengthProperty is not null)
{
lengthAccess = new BoundPropertyAccess(node, receiverPlaceholder, initialBindingReceiverIsSubjectToCloning: ThreeState.False, lengthProperty, LookupResultKind.Viable, lengthProperty.Type) { WasCompilerGenerated = true };
lengthAccess = new BoundPropertyAccess(node, receiverPlaceholder, initialBindingReceiverIsSubjectToCloning: ThreeState.False, lengthProperty, autoPropertyAccessorKind: AccessorKind.Unknown, LookupResultKind.Viable, lengthProperty.Type) { WasCompilerGenerated = true };
}
else
{
Expand Down
38 changes: 28 additions & 10 deletions src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Microsoft.CodeAnalysis.CSharp.Syntax;
Expand Down Expand Up @@ -1749,27 +1750,43 @@ private DiagnosticInfo GetBadEventUsageDiagnosticInfo(EventSymbol eventSymbol)
new CSDiagnosticInfo(ErrorCode.ERR_BadEventUsageNoField, leastOverridden);
}

#nullable enable
internal static bool AccessingAutoPropertyFromConstructor(BoundPropertyAccess propertyAccess, Symbol fromMember)
{
return AccessingAutoPropertyFromConstructor(propertyAccess.ReceiverOpt, propertyAccess.PropertySymbol, fromMember);
return AccessingAutoPropertyFromConstructor(propertyAccess.ReceiverOpt, propertyAccess.PropertySymbol, fromMember, propertyAccess.AutoPropertyAccessorKind);
}

// PROTOTYPE: Review all callers for allowFieldKeyword.
private static bool AccessingAutoPropertyFromConstructor(BoundExpression receiver, PropertySymbol propertySymbol, Symbol fromMember, bool allowFieldKeyword = false)
private static bool AccessingAutoPropertyFromConstructor(BoundExpression? receiver, PropertySymbol propertySymbol, Symbol fromMember, AccessorKind accessorKind)
{
if (!HasSynthesizedBackingField(propertySymbol, out var sourceProperty))
{
return false;
}

var propertyIsStatic = propertySymbol.IsStatic;

return sourceProperty is { } &&
sourceProperty.CanUseBackingFieldDirectlyInConstructor(useAsLvalue: accessorKind != AccessorKind.Get) &&
TypeSymbol.Equals(sourceProperty.ContainingType, fromMember.ContainingType, TypeCompareKind.AllIgnoreOptions) &&
IsConstructorOrField(fromMember, isStatic: propertyIsStatic) &&
(propertyIsStatic || receiver?.Kind == BoundKind.ThisReference);
}

private static bool HasSynthesizedBackingField(PropertySymbol propertySymbol, [NotNullWhen(true)] out SourcePropertySymbolBase? sourcePropertyDefinition)
{
if (!propertySymbol.IsDefinition && propertySymbol.ContainingType.Equals(propertySymbol.ContainingType.OriginalDefinition, TypeCompareKind.IgnoreNullableModifiersForReferenceTypes))
{
propertySymbol = propertySymbol.OriginalDefinition;
}

var sourceProperty = propertySymbol as SourcePropertySymbolBase;
var propertyIsStatic = propertySymbol.IsStatic;
if (propertySymbol is SourcePropertySymbolBase { BackingField: { } } sourceProperty)
{
sourcePropertyDefinition = sourceProperty;
return true;
}

return (object)sourceProperty != null &&
(allowFieldKeyword ? sourceProperty.IsAutoPropertyOrUsesFieldKeyword : sourceProperty.IsAutoProperty) &&
TypeSymbol.Equals(sourceProperty.ContainingType, fromMember.ContainingType, TypeCompareKind.AllIgnoreOptions) &&
IsConstructorOrField(fromMember, isStatic: propertyIsStatic) &&
(propertyIsStatic || receiver.Kind == BoundKind.ThisReference);
sourcePropertyDefinition = null;
return false;
}

private static bool IsConstructorOrField(Symbol member, bool isStatic)
Expand All @@ -1779,6 +1796,7 @@ private static bool IsConstructorOrField(Symbol member, bool isStatic)
MethodKind.Constructor) ||
(member as FieldSymbol)?.IsStatic == isStatic;
}
#nullable disable

private TypeSymbol GetAccessThroughType(BoundExpression receiver)
{
Expand Down
21 changes: 21 additions & 0 deletions src/Compilers/CSharp/Portable/BoundTree/BoundNode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,11 @@ private enum BoundNodeAttributes : short

ParamsArrayOrCollection = 1 << 9,

/// <summary>
/// Set after checking if the property access should use the backing field directly.
/// </summary>
WasPropertyBackingFieldAccessChecked = 1 << 10,

AttributesPreservedInClone = HasErrors | CompilerGenerated | IsSuppressed | WasConverted | ParamsArrayOrCollection,
}

Expand Down Expand Up @@ -325,6 +330,22 @@ protected set
}
}
}

public bool WasPropertyBackingFieldAccessChecked
{
get
{
return (_attributes & BoundNodeAttributes.WasPropertyBackingFieldAccessChecked) != 0;
}
set
{
Debug.Assert((_attributes & BoundNodeAttributes.WasPropertyBackingFieldAccessChecked) == 0, "should not be set twice or reset");
if (value)
{
_attributes |= BoundNodeAttributes.WasPropertyBackingFieldAccessChecked;
}
}
}
#endif

public bool IsParamsArrayOrCollection
Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
Original file line number Diff line number Diff line change
Expand Up @@ -2194,6 +2194,8 @@
<!-- Whether receiver will need to be cloned during emit (only valid before lowering) -->
<Field Name="InitialBindingReceiverIsSubjectToCloning" Type="ThreeState"/>
<Field Name="PropertySymbol" Type="PropertySymbol"/>
<!-- The property accessor kind, set for properties with synthesized backing field only. -->
<Field Name="AutoPropertyAccessorKind" Type="AccessorKind" />
<Field Name="ResultKind" PropertyOverrides="true" Type="LookupResultKind"/>
</Node>

Expand Down
89 changes: 89 additions & 0 deletions src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1823,6 +1823,7 @@ syntaxNode is ConstructorDeclarationSyntax constructorSyntax &&

#if DEBUG
Debug.Assert(IsEmptyRewritePossible(methodBody));
Debug.Assert(WasPropertyBackingFieldAccessChecked.FindUncheckedAccess(methodBody) is null);
#endif

RefSafetyAnalysis.Analyze(compilation, method, methodBody, diagnostics);
Expand Down Expand Up @@ -2205,6 +2206,94 @@ public static bool FoundInUnboundLambda(BoundNode methodBody, IdentifierNameSynt
return base.Visit(node);
}
}

private sealed class WasPropertyBackingFieldAccessChecked : BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator
{
public static BoundPropertyAccess? FindUncheckedAccess(BoundNode node)
{
var walker = new WasPropertyBackingFieldAccessChecked();
walker.Visit(node);
return walker._found;
}

private BoundPropertyAccess? _found;
private bool _suppressChecking;

private WasPropertyBackingFieldAccessChecked()
{
}

public override BoundNode? Visit(BoundNode? node)
{
if (_found is { })
{
return null;
}

return base.Visit(node);
}

public override BoundNode? VisitPropertyAccess(BoundPropertyAccess node)
{
if (!_suppressChecking &&
!node.WasPropertyBackingFieldAccessChecked)
{
_found = node;
}

return base.VisitPropertyAccess(node);
}

public override BoundNode? VisitRangeVariable(BoundRangeVariable node)
{
using (new ChangeSuppression(this, suppressChecking: true))
{
return base.VisitRangeVariable(node);
}
}

public override BoundNode? VisitAssignmentOperator(BoundAssignmentOperator node)
{
using (new ChangeSuppression(this, suppressChecking: false))
{
return base.VisitAssignmentOperator(node);
}
}

public override BoundNode? VisitNameOfOperator(BoundNameOfOperator node)
{
using (new ChangeSuppression(this, suppressChecking: true))
{
return base.VisitNameOfOperator(node);
}
}

public override BoundNode? VisitBadExpression(BoundBadExpression node)
{
using (new ChangeSuppression(this, suppressChecking: true))
{
return base.VisitBadExpression(node);
}
}

private struct ChangeSuppression : IDisposable
{
private readonly WasPropertyBackingFieldAccessChecked _walker;
private readonly bool _previousValue;

internal ChangeSuppression(WasPropertyBackingFieldAccessChecked walker, bool suppressChecking)
{
_walker = walker;
_previousValue = walker._suppressChecking;
walker._suppressChecking = suppressChecking;
}

public void Dispose()
{
_walker._suppressChecking = _previousValue;
}
}
}
#endif
#nullable disable

Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9754,8 +9754,8 @@ private void VisitThisOrBaseReference(BoundExpression node)
{
// when binding initializers, we treat assignments to auto-properties or field-like events as direct assignments to the underlying field.
// in order to track member state based on these initializers, we need to see the assignment in terms of the associated member
case BoundFieldAccess { ExpressionSymbol: FieldSymbol { AssociatedSymbol: PropertySymbol autoProperty } } fieldAccess:
left = new BoundPropertyAccess(fieldAccess.Syntax, fieldAccess.ReceiverOpt, initialBindingReceiverIsSubjectToCloning: ThreeState.Unknown, autoProperty, LookupResultKind.Viable, autoProperty.Type, fieldAccess.HasErrors);
case BoundFieldAccess { ExpressionSymbol: FieldSymbol { AssociatedSymbol: PropertySymbol autoProperty }, Syntax: not FieldExpressionSyntax } fieldAccess:
left = new BoundPropertyAccess(fieldAccess.Syntax, fieldAccess.ReceiverOpt, initialBindingReceiverIsSubjectToCloning: ThreeState.Unknown, autoProperty, autoPropertyAccessorKind: AccessorKind.Unknown, LookupResultKind.Viable, autoProperty.Type, fieldAccess.HasErrors);
break;
case BoundFieldAccess { ExpressionSymbol: FieldSymbol { AssociatedSymbol: EventSymbol @event } } fieldAccess:
left = new BoundEventAccess(fieldAccess.Syntax, fieldAccess.ReceiverOpt, @event, isUsableAsField: true, LookupResultKind.Viable, @event.Type, fieldAccess.HasErrors);
Expand Down
Loading

0 comments on commit 5aceebc

Please sign in to comment.