Skip to content

Commit

Permalink
[semi-auto-props]: Require overriding all accessors (#61114)
Browse files Browse the repository at this point in the history
  • Loading branch information
Youssef1313 authored May 9, 2022
1 parent 1125db2 commit f214736
Show file tree
Hide file tree
Showing 4 changed files with 110 additions and 12 deletions.
2 changes: 0 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1709,8 +1709,6 @@ private static bool IsPropertyAssignedThroughBackingField(BoundExpression receiv
// 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: sourceProperty.IsStatic) &&
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -586,7 +586,7 @@ private void CompileNamedType(NamedTypeSymbol containingType)
if ((getMethod is null || PassesFilter(_filterOpt, getMethod)) &&
(setMethod is null || PassesFilter(_filterOpt, setMethod)))
{
property.MarkBackingFieldAsCalculated();
property.MarkBackingFieldAsCalculated(_diagnostics);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -411,9 +411,24 @@ private void EnsureSignature()
/// <remarks>
/// This should be called only if we're sure the backing field can't become non-null value if it's not already.
/// </remarks>
internal void MarkBackingFieldAsCalculated()
internal void MarkBackingFieldAsCalculated(BindingDiagnosticBag? diagnostics)
{
Interlocked.CompareExchange(ref _lazyBackingFieldSymbol, null, _lazyBackingFieldSymbolSentinel);
if (diagnostics is null || !IsOverride || _lazyBackingFieldSymbol is null)
{
return;
}

var fieldSymbol = (SynthesizedBackingFieldSymbol)_lazyBackingFieldSymbol;
if (fieldSymbol.IsCreatedForFieldKeyword)
{
// semi auto property should override all accessors.
if ((!HasSetAccessor && !this.IsReadOnly) ||
(!HasGetAccessor && !this.IsWriteOnly))
{
diagnostics.Add(ErrorCode.ERR_AutoPropertyMustOverrideSet, Location);
}
}
}

/// <summary>
Expand Down Expand Up @@ -449,7 +464,7 @@ private void EnsureBackingFieldIsSynthesized()
}

// If binding both the getter and setter didn't get a backing field, set it to null so that we don't re-calculate.
MarkBackingFieldAsCalculated();
MarkBackingFieldAsCalculated(diagnostics: null);

void noteAccessorBinding()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,6 @@ namespace Microsoft.CodeAnalysis.CSharp.Semantic.UnitTests.Semantics
// both expression body and block body. We should confirm that SemanticModel doesn't bind an expression body in presence of a block body.

// PROTOTYPE(semi-auto-props): Add ENC tests.

// PROTOTYPE(semi-auto-props): Add a test where a virtual `get; set;` auto property is overridden by a sealed `get => 0;`.
// A synthesized sealed accessor is expected to be produced.

// PROTOTYPE(semi-auto-props): Add a test where a virtual property is `public virtual int P6 { get => 0; set { } }`, and
// it's overridden by `public override int P6 { get => field; }`
// The assignment of the property in constructor should use the base setter.
public class PropertyFieldKeywordTests : CompilingTestBase
{
/// <summary>
Expand Down Expand Up @@ -87,6 +80,98 @@ private void VerifyTypeIL(CSharpCompilation compilation, string typeName, string
CompileAndVerify(compilation).VerifyTypeIL(typeName, expected);
}

[Theory, CombinatorialData]
public void TestVirtualPropertyOverride(bool callGetFieldsToEmit, bool callSemanticModel)
{
var comp = CreateCompilation(@"
public class Base
{
public virtual int P1 { get; set; }
public virtual int P2 { get => 0; set { } }
}
public class Derived1 : Base
{
public override int P1 { get { _ = field; return field; } }
public override int P2 { get => field; }
}
public class Derived2 : Base
{
public override int P1 { set => _ = field; }
public override int P2 { set => _ = field; }
}
public class Derived3 : Base
{
// PROTOTYPE(semi-auto-props):
// This should produce ERR_AutoPropertyMustOverrideSet ""Auto-implemented properties must override all accessors of the overridden property.""
// instead of ERR_AutoPropertyMustHaveGetAccessor, unless https://github.com/dotnet/csharplang/issues/6089 is accepted.
public override int P1 { set; }
}
public class Derived4 : Base
{
public override int P1 { get => field; set => field = value; }
}
");
var accessorBindingData = new SourcePropertySymbolBase.AccessorBindingData();
comp.TestOnlyCompilationData = accessorBindingData;
if (callGetFieldsToEmit)
{
var baseFields = comp.GetTypeByMetadataName("Base").GetFieldsToEmit().ToArray();
Assert.Equal(1, baseFields.Length);
Assert.Equal("System.Int32 Base.<P1>k__BackingField", baseFields[0].ToTestDisplayString());

var derived1Fields = comp.GetTypeByMetadataName("Derived1").GetFieldsToEmit().ToArray();
Assert.Equal(2, derived1Fields.Length);
Assert.Equal("System.Int32 Derived1.<P1>k__BackingField", derived1Fields[0].ToTestDisplayString());
Assert.Equal("System.Int32 Derived1.<P2>k__BackingField", derived1Fields[1].ToTestDisplayString());

var derived2Fields = comp.GetTypeByMetadataName("Derived2").GetFieldsToEmit().ToArray();
Assert.Equal(2, derived2Fields.Length);
Assert.Equal("System.Int32 Derived2.<P1>k__BackingField", derived2Fields[0].ToTestDisplayString());
Assert.Equal("System.Int32 Derived2.<P2>k__BackingField", derived2Fields[1].ToTestDisplayString());

var derived3Fields = comp.GetTypeByMetadataName("Derived3").GetFieldsToEmit().ToArray();
Assert.Equal(0, derived3Fields.Length);

var derived4Fields = comp.GetTypeByMetadataName("Derived4").GetFieldsToEmit().ToArray();
Assert.Equal(1, derived4Fields.Length);
Assert.Equal("System.Int32 Derived4.<P1>k__BackingField", derived4Fields[0].ToTestDisplayString());
}

if (callSemanticModel)
{
var model = comp.GetSemanticModel(comp.SyntaxTrees.Single());
var nodes = comp.SyntaxTrees.Single().GetRoot().DescendantNodes().Where(n => n is IdentifierNameSyntax identifier && identifier.Identifier.ContextualKind() == SyntaxKind.FieldKeyword);
foreach (var node in nodes)
{
var typeInfo = model.GetTypeInfo(node);
Assert.Equal(SpecialType.System_Int32, typeInfo.Type.SpecialType);
}
}

comp.VerifyDiagnostics(
// (10,25): error CS8080: Auto-implemented properties must override all accessors of the overridden property.
// public override int P1 { get { _ = field; return field; } }
Diagnostic(ErrorCode.ERR_AutoPropertyMustOverrideSet, "P1").WithLocation(10, 25),
// (11,25): error CS8080: Auto-implemented properties must override all accessors of the overridden property.
// public override int P2 { get => field; }
Diagnostic(ErrorCode.ERR_AutoPropertyMustOverrideSet, "P2").WithLocation(11, 25),
// (16,25): error CS8080: Auto-implemented properties must override all accessors of the overridden property.
// public override int P1 { set => _ = field; }
Diagnostic(ErrorCode.ERR_AutoPropertyMustOverrideSet, "P1").WithLocation(16, 25),
// (17,25): error CS8080: Auto-implemented properties must override all accessors of the overridden property.
// public override int P2 { set => _ = field; }
Diagnostic(ErrorCode.ERR_AutoPropertyMustOverrideSet, "P2").WithLocation(17, 25),
// (25,30): error CS8051: Auto-implemented properties must have get accessors.
// public override int P1 { set; }
Diagnostic(ErrorCode.ERR_AutoPropertyMustHaveGetAccessor, "set").WithArguments("Derived3.P1.set").WithLocation(25, 30)
);
Assert.Equal(callGetFieldsToEmit ? 5 : 0, accessorBindingData.NumberOfPerformedAccessorBinding);
}

[Fact]
public void TestInInterface()
{
Expand Down

0 comments on commit f214736

Please sign in to comment.