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]: Require overriding all accessors #61114

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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