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
42 changes: 33 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,50 @@ private DiagnosticInfo GetBadEventUsageDiagnosticInfo(EventSymbol eventSymbol)
new CSDiagnosticInfo(ErrorCode.ERR_BadEventUsageNoField, leastOverridden);
}

internal static bool AccessingAutoPropertyFromConstructor(BoundPropertyAccess propertyAccess, Symbol fromMember)
internal static bool IsReadingThroughBackingFieldInConstructor(BoundPropertyAccess propertyAccess, Symbol fromMember)
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
{
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 assignment.
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
/// </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 &&
sourceProperty.GetMethod is not null &&
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
// 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`? Add tests.
Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to find a scenario that can be affected by this, but couldn't. @AlekseyTs Can you think of any? if not we can delete this prototype comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried to find a scenario that can be affected by this, but couldn't. Can you think of any? if not we can delete this prototype comment.

Legacy auto-properties are required to override all accessors. Therefore, we don't need to worry about an overriding case for them. If that invariant is "broken" for semi-auto properties, then I think we will need to strengthen the check, detect the presence of an inherited setter and return false for that case. Capturing this information in a PROTOTYPE comment would be useful.

sourceProperty.SetMethod is not SourcePropertyAccessorSymbol { IsEquivalentToBackingFieldAccess: false } &&
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
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);
}
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved

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.IsReadingThroughBackingFieldInConstructor(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 property read.
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
// 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 @@ -429,7 +429,6 @@ .maxstack 2
");
}


[Fact]
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
public void TestInitializerInCtor004()
{
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