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 7 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
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,16 @@ private enum Flags : byte
private readonly SourcePropertyAccessorSymbol? _getMethod;
private readonly SourcePropertyAccessorSymbol? _setMethod;
private object? _lazyBackingFieldSymbol = _lazyBackingFieldSymbolSentinel;

/// <summary>
/// An object to represent whether we reported that a semi auto property
/// override must override all accessors.
/// If null, we didn't yet report the diagnostic.
/// Otherwise, we reported it.
/// </summary>
private object? _semiAutoPropertyHasGivenMustOverrideDiagnostic;

private readonly BindingDiagnosticBag _diagnostics;
#nullable disable
private readonly TypeSymbol _explicitInterfaceType;
private ImmutableArray<PropertySymbol> _lazyExplicitInterfaceImplementations;
Expand Down Expand Up @@ -119,6 +129,7 @@ protected SourcePropertySymbolBase(
_refKind = refKind;
_modifiers = modifiers;
_explicitInterfaceType = explicitInterfaceType;
_diagnostics = diagnostics;
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved

if (isExplicitInterfaceImplementation)
{
Expand Down Expand Up @@ -414,6 +425,22 @@ private void EnsureSignature()
internal void MarkBackingFieldAsCalculated()
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
{
Interlocked.CompareExchange(ref _lazyBackingFieldSymbol, null, _lazyBackingFieldSymbolSentinel);
if (!IsOverride || _lazyBackingFieldSymbol is null)
{
return;
}

var fieldSymbol = (SynthesizedBackingFieldSymbol)_lazyBackingFieldSymbol;
if (fieldSymbol.IsCreatedForFieldKeyword &&
Interlocked.CompareExchange(ref _semiAutoPropertyHasGivenMustOverrideDiagnostic, new object(), null) == null)
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
{
// semi auto property should override all accessors.
if ((!HasSetAccessor && !this.IsReadOnly) ||
(!HasGetAccessor && !this.IsWriteOnly))
{
_diagnostics.Add(ErrorCode.ERR_AutoPropertyMustOverrideSet, Location);
}
}
}

/// <summary>
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,97 @@ 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 typeInfo = model.GetTypeInfo(comp.SyntaxTrees.Single().GetRoot().DescendantNodes().First(n => n is IdentifierNameSyntax identifier && identifier.Identifier.ContextualKind() == SyntaxKind.FieldKeyword));
333fred marked this conversation as resolved.
Show resolved Hide resolved
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),
// (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), // PROTOTYPE(semi-auto-props): Get rid of the extra diagnostic.
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
// (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