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.

Sign up for GitHub

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 9 commits into from
May 9, 2022
Merged

[semi-auto-props]: Require overriding all accessors #61114

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
712aa77
[semi-auto-props]: Require overriding all accessors
Youssef1313 May 3, 2022
09e36a8
Report in binder
Youssef1313 May 5, 2022
10326e6
Update FieldKeywordBinder.cs
Youssef1313 May 5, 2022
5d61c97
Update SpeculativeFieldKeywordBinder.cs
Youssef1313 May 5, 2022
e507fcb
restore blank line
Youssef1313 May 5, 2022
3ed97f6
Test multiple occurrences of field
Youssef1313 May 5, 2022
786c0b8
non-working approach
Youssef1313 May 5, 2022
20bcc1f
Address review comments
Youssef1313 May 5, 2022
7bf8e2c
Address feedback
Youssef1313 May 6, 2022
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
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/Binder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,10 +194,10 @@ internal virtual ImmutableArray<LocalFunctionSymbol> GetDeclaredLocalFunctionsFo
return this.Next.GetDeclaredLocalFunctionsForScope(scopeDesignator);
}

internal virtual FieldSymbol? GetSymbolForPossibleFieldKeyword()
internal virtual FieldSymbol? GetSymbolForPossibleFieldKeyword(BindingDiagnosticBag diagnostics)
Copy link
Contributor

@AlekseyTs AlekseyTs May 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

BindingDiagnosticBag diagnostics

I extremely dislike the idea of bundling any diadnostic reporting with this API. I think there was already an attempt to do this in another PR. #Closed

{
RoslynDebug.Assert(Next is object);
return this.Next.GetSymbolForPossibleFieldKeyword();
return this.Next.GetSymbolForPossibleFieldKeyword(diagnostics);
}

/// <summary>
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1566,7 +1566,7 @@ private BoundExpression BindIdentifier(
else if (node.Identifier.ContextualKind() == SyntaxKind.FieldKeyword &&
ContainingMember().CanHaveFieldKeywordBackingField())
{
if (GetSymbolForPossibleFieldKeyword() is { } backingField)
if (GetSymbolForPossibleFieldKeyword(diagnostics) is { } backingField)
{
expression = BindNonMethod(node, backingField, diagnostics, LookupResultKind.Viable, indexed: false, isError: false);
if (IsInsideNameof)
Expand Down
2 changes: 0 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs
View file
Open in desktop
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/Binder/BuckStopsHereBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ internal override BoundStatement BindLockStatementParts(BindingDiagnosticBag dia
throw ExceptionUtilities.Unreachable;
}

internal override FieldSymbol? GetSymbolForPossibleFieldKeyword()
internal override FieldSymbol? GetSymbolForPossibleFieldKeyword(BindingDiagnosticBag diagnostics)
{
// There's supposed to either be a FieldKeywordBinder or a SpeculativeFieldKEywordBinder in the chain.
throw ExceptionUtilities.Unreachable;
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/FieldKeywordBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ internal FieldKeywordBinder(SourcePropertyAccessorSymbol accessor, Binder next)
_accessor = accessor;
}

internal override FieldSymbol? GetSymbolForPossibleFieldKeyword()
internal override FieldSymbol? GetSymbolForPossibleFieldKeyword(BindingDiagnosticBag diagnostics)
{
return _accessor.Property.GetOrCreateBackingFieldForFieldKeyword();
return _accessor.Property.GetOrCreateBackingFieldForFieldKeyword(diagnostics);
}
}
}
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/SpeculativeFieldKeywordBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Licensed to the .NET Foundation under one or more agreements.
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

Expand All @@ -19,7 +19,7 @@ internal SpeculativeFieldKeywordBinder(SourcePropertyAccessorSymbol accessor, Bi
_accessor = accessor;
}

internal override FieldSymbol? GetSymbolForPossibleFieldKeyword()
internal override FieldSymbol? GetSymbolForPossibleFieldKeyword(BindingDiagnosticBag diagnostics)
{
// field in the speculative model does not bind to a backing field if the original location was not a semi-auto property
return _accessor.Property.FieldKeywordBackingField;
Expand Down
19 changes: 14 additions & 5 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ protected SourcePropertySymbolBase(
Debug.Assert(!IsIndexer);
// PROTOTYPE(semi-auto-props): Make sure that TestSemiAutoPropertyWithInitializer (when enabled back) is affected by this.
// That is, if we removed "hasInitializer", the test should fail, or any other test should get affected.
GetOrCreateBackingField(isCreatedForFieldKeyword: hasInitializer && !isAutoProperty, isEarlyConstructed: true);
GetOrCreateBackingField(isCreatedForFieldKeyword: hasInitializer && !isAutoProperty, isEarlyConstructed: true, diagnostics);
}

if (hasGetAccessor)
Expand All @@ -198,7 +198,7 @@ protected SourcePropertySymbolBase(
}
}

private SynthesizedBackingFieldSymbol? GetOrCreateBackingField(bool isCreatedForFieldKeyword, bool isEarlyConstructed)
private SynthesizedBackingFieldSymbol? GetOrCreateBackingField(bool isCreatedForFieldKeyword, bool isEarlyConstructed, BindingDiagnosticBag diagnostics)
{
Debug.Assert(!IsIndexer);
if (_lazyBackingFieldSymbol == _lazyBackingFieldSymbolSentinel)
Expand All @@ -210,15 +210,24 @@ protected SourcePropertySymbolBase(
hasInitializer: (_propertyFlags & Flags.HasInitializer) != 0,
isCreatedForFieldKeyword: isCreatedForFieldKeyword,
isEarlyConstructed: isEarlyConstructed);
Interlocked.CompareExchange(ref _lazyBackingFieldSymbol, backingField, _lazyBackingFieldSymbolSentinel);
if (Interlocked.CompareExchange(ref _lazyBackingFieldSymbol, backingField, _lazyBackingFieldSymbolSentinel) == _lazyBackingFieldSymbolSentinel &&
isCreatedForFieldKeyword)
{
// semi auto property should override all accessors.
if ((!HasSetAccessor && !this.IsReadOnly) ||
Copy link
Contributor

@AlekseyTs AlekseyTs May 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

((!HasSetAccessor && !this.IsReadOnly) ||

Please add an explicit check for IsOverrides #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

@AlekseyTs Yup I was going to do that. I added the IsOverride assert in another PR just to be sure my assumption is correct :)

(!HasGetAccessor && !this.IsWriteOnly))
{
diagnostics.Add(ErrorCode.ERR_AutoPropertyMustOverrideSet, Location, this);
Copy link
Member Author

@Youssef1313 Youssef1313 May 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Some questions here:

  • The actual message doesn't have anything to do with "setters", so the naming of the ErrorCode is misleading. I can rename that, should I go for it (it's already misleading prior to my change)?
  • The actual message is Auto-implemented properties must override all accessors of the overridden property., is it already accurate? I don't know what will be the expecting "branding" of semi-auto properties? Should we mention them as "Auto-implemented properties" in error messages, or as "Semi-auto implemented properties", or something else?
  • I'm adding the diagnostic just after we create the backing field to avoid accessing FieldKeywordBackingField early which will trigger extra binding. Is that a good/correct way?

Copy link
Contributor

@AlekseyTs AlekseyTs May 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The actual message doesn't have anything to do with "setters", so the naming of the ErrorCode is misleading. I can rename that, should I go for it (it's already misleading prior to my change)?

We prefer pure refactoring changes (a rename is a pure refactoring) kept separate from behavior changes.

The actual message is "Auto-implemented properties must override all accessors of the overridden property.", is it already accurate? I don't know what will be the expecting "branding" of semi-auto properties? Should we mention them as "Auto-implemented properties" in error messages, or as "Semi-auto implemented properties", or something else?

Consider adding a dedicated (new) error. Something like: "Properties using 'field' keyword must override all accessors of the overridden property."

I'm adding the diagnostic just after we create the backing field to avoid accessing FieldKeywordBackingField early which will trigger extra binding. Is that a good/correct way?

I think other comments already provided feedback on the implementation strategy.

333fred marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

@AlekseyTs AlekseyTs May 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

diagnostics.Add(ErrorCode.ERR_AutoPropertyMustOverrideSet, Location, this);

Reporting any diagnostics here conditionally, based on whether the symbol is created now or by another call, is really a bad idea. The caller of this API is method binding, it is not responsible to propagate this diagnostics to a "user". The diagnostics can be discarded. In fact, EnsureBackingFieldIsSynthesized below does exactly that. Whether an error is reported to a user shouldn't depend on the order of operations performed by compiler internally. I think it should be pretty easy to come up with a test scenario, for which this error is simply getting "dropped on the floor". Please create a test like that before making changes to the current implementation. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed. Calling GetFieldsToEmit before anything does a force binding and loses the diagnostics.

}
}
}

return (SynthesizedBackingFieldSymbol?)_lazyBackingFieldSymbol;
}

internal SynthesizedBackingFieldSymbol? GetOrCreateBackingFieldForFieldKeyword()
internal SynthesizedBackingFieldSymbol? GetOrCreateBackingFieldForFieldKeyword(BindingDiagnosticBag diagnostics)
{
return GetOrCreateBackingField(isCreatedForFieldKeyword: true, isEarlyConstructed: false);
return GetOrCreateBackingField(isCreatedForFieldKeyword: true, isEarlyConstructed: false, diagnostics);
}

private void EnsureSignatureGuarded(BindingDiagnosticBag diagnostics)
Expand Down
64 changes: 57 additions & 7 deletions src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs
View file
Open in desktop
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,63 @@ private void VerifyTypeIL(CSharpCompilation compilation, string typeName, string
CompileAndVerify(compilation).VerifyTypeIL(typeName, expected);
}

[Fact]
public void TestVirtualPropertyOverride()
Copy link
Contributor

@AlekseyTs AlekseyTs May 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

TestVirtualPropertyOverride

I do not see a single scenario with multiple field keywords used by the same property. #Closed

Copy link
Contributor

@AlekseyTs AlekseyTs May 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

TestVirtualPropertyOverride

Please add a test where SemanticModel is the first one to bind the accessor. Are we going to get any diagnostics after that? #Closed

{
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; }
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;
comp.VerifyDiagnostics(
// (10,25): error CS8080: Auto-implemented properties must override all accessors of the overridden property.
// public override int P1 { get => field; } // PROTOTYPE(semi-auto-props): This should error
Copy link
Contributor

@AlekseyTs AlekseyTs May 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

// PROTOTYPE(semi-auto-props): This should error

For this and other similar comments, please, clarify what exact error do you expect to report in the future. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

This is an outdated PROTOTYPE comment. I had it written when I wrote the test but before fixing implementation. Removing.

Diagnostic(ErrorCode.ERR_AutoPropertyMustOverrideSet, "P1").WithArguments("Derived1.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; } // PROTOTYPE(semi-auto-props): This should error
Diagnostic(ErrorCode.ERR_AutoPropertyMustOverrideSet, "P2").WithArguments("Derived1.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; } // PROTOTYPE(semi-auto-props): This should error
Diagnostic(ErrorCode.ERR_AutoPropertyMustOverrideSet, "P1").WithArguments("Derived2.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; } // PROTOTYPE(semi-auto-props): This should error
Diagnostic(ErrorCode.ERR_AutoPropertyMustOverrideSet, "P2").WithArguments("Derived2.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(0, accessorBindingData.NumberOfPerformedAccessorBinding);
}

[Fact]
public void TestInInterface()
{
Expand Down