Skip to content

Commit

Permalink
[semi-auto-props]: Refactor and rename 'AccessingAutoPropertyFromCons…
Browse files Browse the repository at this point in the history
…tructor' (#58832)
  • Loading branch information
Youssef1313 authored Jan 26, 2022
1 parent 8c6ed69 commit c0a7756
Show file tree
Hide file tree
Showing 7 changed files with 300 additions and 16 deletions.
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1092,7 +1092,7 @@ private bool CheckPropertyValueKind(SyntaxNode node, BoundExpression expr, BindV
if (setMethod is null)
{
var containing = this.ContainingMemberOrLambda;
if (!AccessingAutoPropertyFromConstructor(receiver, propertySymbol, containing)
if (!IsPropertyAssignedThroughBackingField(receiver, propertySymbol, containing)
&& !isAllowedDespiteReadonly(receiver))
{
Error(diagnostics, ErrorCode.ERR_AssgReadonlyProp, node, propertySymbol);
Expand Down
45 changes: 36 additions & 9 deletions src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1668,26 +1668,53 @@ private DiagnosticInfo GetBadEventUsageDiagnosticInfo(EventSymbol eventSymbol)
new CSDiagnosticInfo(ErrorCode.ERR_BadEventUsageNoField, leastOverridden);
}

internal static bool AccessingAutoPropertyFromConstructor(BoundPropertyAccess propertyAccess, Symbol fromMember)
// PROTOTYPE(semi-auto-props): Re-evaluate if this helper is needed. There is one callsite only.
internal static bool IsEquivalentToBackingFieldRead(BoundPropertyAccess propertyAccess, Symbol fromMember)
{
return AccessingAutoPropertyFromConstructor(propertyAccess.ReceiverOpt, propertyAccess.PropertySymbol, fromMember);
var propertySymbol = propertyAccess.PropertySymbol;
var receiver = propertyAccess.ReceiverOpt;
if (!propertySymbol.IsDefinition && propertySymbol.ContainingType.Equals(propertySymbol.ContainingType.OriginalDefinition, TypeCompareKind.IgnoreNullableModifiersForReferenceTypes))
{
propertySymbol = propertySymbol.OriginalDefinition;
}

return propertySymbol is SourcePropertySymbolBase sourceProperty &&
sourceProperty.GetMethod is SourcePropertyAccessorSymbol { IsEquivalentToBackingFieldAccess: true } &&
TypeSymbol.Equals(sourceProperty.ContainingType, fromMember.ContainingType, TypeCompareKind.ConsiderEverything2) &&
IsConstructorOrField(fromMember, isStatic: sourceProperty.IsStatic) &&
(sourceProperty.IsStatic || receiver.Kind == BoundKind.ThisReference);
}

internal static bool IsPropertyAssignedThroughBackingField(BoundPropertyAccess propertyAccess, Symbol fromMember)
{
return IsPropertyAssignedThroughBackingField(propertyAccess.ReceiverOpt, propertyAccess.PropertySymbol, fromMember);
}

private static bool AccessingAutoPropertyFromConstructor(BoundExpression receiver, PropertySymbol propertySymbol, Symbol fromMember)
/// <summary>
/// Determines whether a property is assigned through the backing field within a symbol <paramref name="fromMember"/>
/// </summary>
/// <remarks>
/// A property is assigned through backing field within a constructor or a field initializer.
/// </remarks>
private static bool IsPropertyAssignedThroughBackingField(BoundExpression receiver, PropertySymbol propertySymbol, Symbol fromMember)
{
if (!propertySymbol.IsDefinition && propertySymbol.ContainingType.Equals(propertySymbol.ContainingType.OriginalDefinition, TypeCompareKind.IgnoreNullableModifiersForReferenceTypes))
{
propertySymbol = propertySymbol.OriginalDefinition;
}

var sourceProperty = propertySymbol as SourcePropertySymbolBase;
var propertyIsStatic = propertySymbol.IsStatic;
// PROTOTYPE(semi-auto-props): TODO: Support assigning semi auto prop from constructors.

return (object)sourceProperty != null &&
sourceProperty.IsAutoPropertyWithGetAccessor &&
return propertySymbol is SourcePropertySymbolBase sourceProperty &&
// PROTOTYPE(semi-auto-props): Consider `public int P { get => field; set; }`
sourceProperty.GetMethod is SourcePropertyAccessorSymbol { IsEquivalentToBackingFieldAccess: true } &&
// To be assigned through backing field, either SetMethod is null, or it's equivalent to backing field write
// PROTOTYPE(semi-auto-props): TODO: Do we need to use `GetOwnOrInheritedSetMethod` instead of `SetMethod`?
// Legacy auto-properties are required to override all accessors. If this is not the case with semi auto props, we may need to use GetOwnOrInheritedSetMethod
sourceProperty.SetMethod is null or SourcePropertyAccessorSymbol { IsEquivalentToBackingFieldAccess: true } &&
TypeSymbol.Equals(sourceProperty.ContainingType, fromMember.ContainingType, TypeCompareKind.ConsiderEverything2) &&
IsConstructorOrField(fromMember, isStatic: propertyIsStatic) &&
(propertyIsStatic || receiver.Kind == BoundKind.ThisReference);
IsConstructorOrField(fromMember, isStatic: sourceProperty.IsStatic) &&
(sourceProperty.IsStatic || receiver.Kind == BoundKind.ThisReference);
}

private static bool IsConstructorOrField(Symbol member, bool isStatic)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -571,7 +571,7 @@ protected void VisitLvalue(BoundExpression node)
case BoundKind.PropertyAccess:
var access = (BoundPropertyAccess)node;

if (Binder.AccessingAutoPropertyFromConstructor(access, _symbol))
if (Binder.IsPropertyAssignedThroughBackingField(access, _symbol))
{
var backingField = (access.PropertySymbol as SourcePropertySymbolBase)?.BackingField;
if (backingField != null)
Expand Down Expand Up @@ -1915,7 +1915,7 @@ private bool RegularPropertyAccess(BoundExpression expr) // PROTOTYPE(semi-auto-
return false;
}

return !Binder.AccessingAutoPropertyFromConstructor((BoundPropertyAccess)expr, _symbol);
return !Binder.IsPropertyAssignedThroughBackingField((BoundPropertyAccess)expr, _symbol); // PROTOTYPE(semi-auto-props): Revise this method call is the behavior we want and add unit tests..
}

public override BoundNode VisitAssignmentOperator(BoundAssignmentOperator node)
Expand Down Expand Up @@ -2051,7 +2051,9 @@ public override BoundNode VisitPropertyAccess(BoundPropertyAccess node)
{
var property = node.PropertySymbol;

if (Binder.AccessingAutoPropertyFromConstructor(node, _symbol))
// PROTOTYPE(semi-auto-props): Add tests for semi auto property once assigning in constructors is supported.
// A test that gets to this code path can be similar to CodeGenConstructorInitTests.TestInitializerInCtor003
if (Binder.IsEquivalentToBackingFieldRead(node, _symbol))
{
var backingField = (property as SourcePropertySymbolBase)?.BackingField;
if (backingField != null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -964,7 +964,10 @@ protected override bool TryGetReceiverAndMember(BoundExpression expr, out BoundE
{
var propAccess = (BoundPropertyAccess)expr;

if (Binder.AccessingAutoPropertyFromConstructor(propAccess, this.CurrentSymbol))
// PROTOTYPE(semi-auto-props): The call to IsPropertyAssignedThroughBackingField isn't sensible.
// We get here for both property read and write.
// This applies to ALL IsPropertyAssignedThroughBackingField calls in this file.
if (Binder.IsPropertyAssignedThroughBackingField(propAccess, this.CurrentSymbol)) // PROTOTYPE(semi-auto-props): Revise this method call is the behavior we want and add unit tests..
{
var propSymbol = propAccess.PropertySymbol;
member = (propSymbol as SourcePropertySymbolBase)?.BackingField;
Expand Down Expand Up @@ -1189,7 +1192,7 @@ private bool IsAssigned(BoundExpression node, out int unassignedSlot)
case BoundKind.PropertyAccess:
{
var propertyAccess = (BoundPropertyAccess)node;
if (Binder.AccessingAutoPropertyFromConstructor(propertyAccess, this.CurrentSymbol))
if (Binder.IsPropertyAssignedThroughBackingField(propertyAccess, this.CurrentSymbol)) // PROTOTYPE(semi-auto-props): Revise this method call is the behavior we want and add unit tests..
{
var property = propertyAccess.PropertySymbol;
var backingField = (property as SourcePropertySymbolBase)?.BackingField;
Expand Down Expand Up @@ -2240,7 +2243,7 @@ public override BoundNode VisitFieldAccess(BoundFieldAccess node)
public override BoundNode VisitPropertyAccess(BoundPropertyAccess node)
{
var result = base.VisitPropertyAccess(node);
if (Binder.AccessingAutoPropertyFromConstructor(node, this.CurrentSymbol))
if (Binder.IsPropertyAssignedThroughBackingField(node, this.CurrentSymbol)) // PROTOTYPE(semi-auto-props): Revise this method call is the behavior we want and add unit tests..
{
var property = node.PropertySymbol;
var backingField = (property as SourcePropertySymbolBase)?.BackingField;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,11 @@ internal Accessibility LocalAccessibility
/// </summary>
internal bool BodyShouldBeSynthesized => _bodyShouldBeSynthesized;

/// <summary>
/// Indicates whether this get/set accessor is equivalent to a backing field read/write.
/// </summary>
internal bool IsEquivalentToBackingFieldAccess => _bodyShouldBeSynthesized;

/// <summary>
/// Indicates whether this accessor is readonly due to reasons scoped to itself and its containing property.
/// </summary>
Expand Down
138 changes: 138 additions & 0 deletions src/Compilers/CSharp/Test/Semantic/FlowAnalysis/RegionAnalysisTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,144 @@ static void Main(string[] args)
Assert.Null(GetSymbolNamesJoined(dataFlowAnalysisResults.CapturedOutside));
}

[Fact]
public void DataFlowsInAndNullable_Property_InConstructor()
{
var dataFlowAnalysisResults = CompileAndAnalyzeDataFlowStatements(@"
struct S
{
public int F;
public S(int f)
{
this.F = f;
int? i = 1;
S s = new S(1);
/*<bind>*/
Console.WriteLine(i.Value);
Console.WriteLine(s.P);
/*</bind>*/
}
public int P { get; set; }
}
");
Assert.Null(GetSymbolNamesJoined(dataFlowAnalysisResults.VariablesDeclared));
Assert.Null(GetSymbolNamesJoined(dataFlowAnalysisResults.AlwaysAssigned));
Assert.Equal("i, s", GetSymbolNamesJoined(dataFlowAnalysisResults.DataFlowsIn));
Assert.Null(GetSymbolNamesJoined(dataFlowAnalysisResults.DataFlowsOut));
Assert.Equal("f, i, s", GetSymbolNamesJoined(dataFlowAnalysisResults.DefinitelyAssignedOnEntry));
Assert.Equal("f, i, s", GetSymbolNamesJoined(dataFlowAnalysisResults.DefinitelyAssignedOnExit));
Assert.Equal("i, s", GetSymbolNamesJoined(dataFlowAnalysisResults.ReadInside));
Assert.Equal("this, f", GetSymbolNamesJoined(dataFlowAnalysisResults.ReadOutside));
Assert.Null(GetSymbolNamesJoined(dataFlowAnalysisResults.WrittenInside));
Assert.Equal("this, f, i, s", GetSymbolNamesJoined(dataFlowAnalysisResults.WrittenOutside));
Assert.Null(GetSymbolNamesJoined(dataFlowAnalysisResults.Captured));
Assert.Null(GetSymbolNamesJoined(dataFlowAnalysisResults.CapturedInside));
Assert.Null(GetSymbolNamesJoined(dataFlowAnalysisResults.CapturedOutside));
}

[Theory]
[InlineData("get;", true)]
[InlineData("get; set;", true)]
[InlineData("get => 0;", false)]
[InlineData("get => 0; set => _ = value;", false)]
public void DataFlowsInAndNullable_InstanceProperty(string accessors, bool isAutoProp)
{
var dataFlowAnalysisResults = CompileAndAnalyzeDataFlowStatements($@"
struct S
{{
public int P {{ {accessors} }}
public S(int p)
{{
this.P = p;
}}
static void Main(string[] args)
{{
S s = new S(1);
/*<bind>*/
Console.WriteLine(s.P);
/*</bind>*/
}}
}}
");
Assert.Null(GetSymbolNamesJoined(dataFlowAnalysisResults.VariablesDeclared));
Assert.Null(GetSymbolNamesJoined(dataFlowAnalysisResults.AlwaysAssigned));
Assert.Equal("s", GetSymbolNamesJoined(dataFlowAnalysisResults.DataFlowsIn));
Assert.Null(GetSymbolNamesJoined(dataFlowAnalysisResults.DataFlowsOut));
Assert.Equal("args, s", GetSymbolNamesJoined(dataFlowAnalysisResults.DefinitelyAssignedOnEntry));
Assert.Equal("args, s", GetSymbolNamesJoined(dataFlowAnalysisResults.DefinitelyAssignedOnExit));
Assert.Equal("s", GetSymbolNamesJoined(dataFlowAnalysisResults.ReadInside));
Assert.Null(GetSymbolNamesJoined(dataFlowAnalysisResults.ReadOutside));

if (isAutoProp)
{
Assert.Null(GetSymbolNamesJoined(dataFlowAnalysisResults.WrittenInside));
}
else
{
Assert.Equal("s", GetSymbolNamesJoined(dataFlowAnalysisResults.WrittenInside));
}

Assert.Equal("args, s", GetSymbolNamesJoined(dataFlowAnalysisResults.WrittenOutside));
Assert.Null(GetSymbolNamesJoined(dataFlowAnalysisResults.Captured));
Assert.Null(GetSymbolNamesJoined(dataFlowAnalysisResults.CapturedInside));
Assert.Null(GetSymbolNamesJoined(dataFlowAnalysisResults.CapturedOutside));
}

[Theory]
[InlineData("get;", true)]
[InlineData("get; set;", true)]
[InlineData("get => 0;", false)]
[InlineData("get => 0; set => _ = value;", false)]
public void DataFlowsInAndNullable_InstanceProperty_InConstructor(string accessors, bool isAutoProp)
{
var dataFlowAnalysisResults = CompileAndAnalyzeDataFlowStatements($@"
struct S
{{
public int P {{ {accessors} }}
public S(int p)
{{
this.P = p;
S s = new S(1);
/*<bind>*/
Console.WriteLine(s.P);
/*</bind>*/
}}
}}
");
Assert.Null(GetSymbolNamesJoined(dataFlowAnalysisResults.VariablesDeclared));
Assert.Null(GetSymbolNamesJoined(dataFlowAnalysisResults.AlwaysAssigned));
Assert.Equal("s", GetSymbolNamesJoined(dataFlowAnalysisResults.DataFlowsIn));
Assert.Null(GetSymbolNamesJoined(dataFlowAnalysisResults.DataFlowsOut));
Assert.Equal("this, p, s", GetSymbolNamesJoined(dataFlowAnalysisResults.DefinitelyAssignedOnEntry));
Assert.Equal("this, p, s", GetSymbolNamesJoined(dataFlowAnalysisResults.DefinitelyAssignedOnExit));
Assert.Equal("s", GetSymbolNamesJoined(dataFlowAnalysisResults.ReadInside));
Assert.Equal("this, p", GetSymbolNamesJoined(dataFlowAnalysisResults.ReadOutside));

if (isAutoProp)
{
Assert.Null(GetSymbolNamesJoined(dataFlowAnalysisResults.WrittenInside));
Assert.Equal("p, s", GetSymbolNamesJoined(dataFlowAnalysisResults.WrittenOutside));
}
else
{
Assert.Equal("s", GetSymbolNamesJoined(dataFlowAnalysisResults.WrittenInside));
Assert.Equal("this, p, s", GetSymbolNamesJoined(dataFlowAnalysisResults.WrittenOutside));
}

Assert.Null(GetSymbolNamesJoined(dataFlowAnalysisResults.Captured));
Assert.Null(GetSymbolNamesJoined(dataFlowAnalysisResults.CapturedInside));
Assert.Null(GetSymbolNamesJoined(dataFlowAnalysisResults.CapturedOutside));
}

[WorkItem(538238, "http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/538238")]
[Fact]
public void TestDataFlowsIn03()
Expand Down
Loading

0 comments on commit c0a7756

Please sign in to comment.