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

[semi-auto-props]: Refactor and rename 'AccessingAutoPropertyFromConstructor' #58832

Merged
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) &&
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
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)
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
{
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) &&
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
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
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()
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
{
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