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

Youssef1313 · 2022-05-03T21:59:33Z

Test plan: #57012

LDM notes: https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-05-02.md#partial-overrides-of-virtual-properties

@AlekseyTs @333fred

Youssef1313 · 2022-05-03T22:02:52Z

src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs

+                    if ((!HasSetAccessor && !this.IsReadOnly) ||
+                        (!HasGetAccessor && !this.IsWriteOnly))
+                    {
+                        diagnostics.Add(ErrorCode.ERR_AutoPropertyMustOverrideSet, Location, this);


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?

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.

src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs

333fred

Done review pass (commit 1). Did not look at tests.

AlekseyTs · 2022-05-04T13:51:25Z

src/Compilers/CSharp/Portable/Binder/Binder.cs

@@ -194,10 +194,10 @@ internal virtual ImmutableArray<LocalFunctionSymbol> GetDeclaredLocalFunctionsFo
            return this.Next.GetDeclaredLocalFunctionsForScope(scopeDesignator);
        }

-        internal virtual FieldSymbol? GetSymbolForPossibleFieldKeyword()
+        internal virtual FieldSymbol? GetSymbolForPossibleFieldKeyword(BindingDiagnosticBag diagnostics)


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

AlekseyTs · 2022-05-04T13:56:09Z

src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs

@@ -87,6 +80,63 @@ private void VerifyTypeIL(CSharpCompilation compilation, string typeName, string
            CompileAndVerify(compilation).VerifyTypeIL(typeName, expected);
        }

+        [Fact]
+        public void TestVirtualPropertyOverride()


TestVirtualPropertyOverride

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

AlekseyTs · 2022-05-04T13:57:47Z

src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs

@@ -87,6 +80,63 @@ private void VerifyTypeIL(CSharpCompilation compilation, string typeName, string
            CompileAndVerify(compilation).VerifyTypeIL(typeName, expected);
        }

+        [Fact]
+        public void TestVirtualPropertyOverride()


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

AlekseyTs · 2022-05-04T14:11:47Z

src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs

+                    if ((!HasSetAccessor && !this.IsReadOnly) ||
+                        (!HasGetAccessor && !this.IsWriteOnly))
+                    {
+                        diagnostics.Add(ErrorCode.ERR_AutoPropertyMustOverrideSet, Location, this);


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

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

AlekseyTs · 2022-05-04T14:13:26Z

                binder?.BindMethodBody(getMethod.SyntaxNode, BindingDiagnosticBag.Discarded);

Dropping diagnostics reported by GetOrCreateBackingField?

In reply to: 1117364955

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs:448 in 712aa77. [](commit_id = 712aa77, deletion_comment = False)

AlekseyTs · 2022-05-04T14:22:35Z

src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs

+                    isCreatedForFieldKeyword)
+                {
+                    // semi auto property should override all accessors.
+                    if ((!HasSetAccessor && !this.IsReadOnly) ||


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

Please add an explicit check for IsOverrides #Closed

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

AlekseyTs · 2022-05-04T14:57:03Z

src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs

+            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


// 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

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

AlekseyTs · 2022-05-04T14:59:29Z

Done with review pass (commit 1).

src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs

src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs

AlekseyTs · 2022-05-05T20:27:35Z

Done with review pass (commit 7)

333fred

Implementation looks good. One test comment.

src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs

333fred

LGTM (commit 9)

AlekseyTs

LGTM (commit 9)

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

712aa77

Youssef1313 requested a review from a team as a code owner May 3, 2022 21:59

ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label May 3, 2022

dotnet-issue-labeler bot added Area-Compilers and removed Community The pull request was submitted by a contributor who is not a Microsoft employee. labels May 3, 2022

Youssef1313 commented May 3, 2022

View reviewed changes

333fred reviewed May 3, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs Outdated Show resolved Hide resolved

333fred reviewed May 3, 2022

View reviewed changes

AlekseyTs reviewed May 4, 2022

View reviewed changes

Youssef1313 added 5 commits May 5, 2022 13:42

Report in binder

09e36a8

Update FieldKeywordBinder.cs

10326e6

Update SpeculativeFieldKeywordBinder.cs

5d61c97

restore blank line

e507fcb

Test multiple occurrences of field

3ed97f6

Youssef1313 commented May 5, 2022

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs Outdated Show resolved Hide resolved

non-working approach

786c0b8

AlekseyTs reviewed May 5, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed May 5, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed May 5, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs Outdated Show resolved Hide resolved

Address review comments

20bcc1f

333fred reviewed May 5, 2022

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/PropertyFieldKeywordTests.cs Outdated Show resolved Hide resolved

Address feedback

7bf8e2c

Youssef1313 requested review from AlekseyTs and 333fred May 6, 2022 06:00

333fred approved these changes May 6, 2022

View reviewed changes

AlekseyTs approved these changes May 9, 2022

View reviewed changes

AlekseyTs merged commit f214736 into dotnet:features/semi-auto-props May 9, 2022

Youssef1313 deleted the semi-override-all branch May 12, 2022 04:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Youssef1313 commented May 3, 2022 •

edited

Loading

Youssef1313 May 3, 2022 •

edited

Loading

AlekseyTs May 4, 2022 •

edited

Loading

333fred left a comment

AlekseyTs May 4, 2022 •

edited

Loading

AlekseyTs May 4, 2022 •

edited

Loading

AlekseyTs May 4, 2022 •

edited

Loading

AlekseyTs May 4, 2022 •

edited

Loading

Youssef1313 May 5, 2022

AlekseyTs commented May 4, 2022 •

edited

Loading

AlekseyTs May 4, 2022 •

edited

Loading

Youssef1313 May 4, 2022

AlekseyTs May 4, 2022 •

edited

Loading

Youssef1313 May 5, 2022

AlekseyTs commented May 4, 2022

AlekseyTs commented May 5, 2022

333fred left a comment

333fred left a comment

AlekseyTs left a comment

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

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

Conversation

Youssef1313 commented May 3, 2022 • edited Loading

Youssef1313 May 3, 2022 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 4, 2022 • edited Loading

Choose a reason for hiding this comment

333fred left a comment

Choose a reason for hiding this comment

AlekseyTs May 4, 2022 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 4, 2022 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 4, 2022 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 4, 2022 • edited Loading

Choose a reason for hiding this comment

Youssef1313 May 5, 2022

Choose a reason for hiding this comment

AlekseyTs commented May 4, 2022 • edited Loading

AlekseyTs May 4, 2022 • edited Loading

Choose a reason for hiding this comment

Youssef1313 May 4, 2022

Choose a reason for hiding this comment

AlekseyTs May 4, 2022 • edited Loading

Choose a reason for hiding this comment

Youssef1313 May 5, 2022

Choose a reason for hiding this comment

AlekseyTs commented May 4, 2022

AlekseyTs commented May 5, 2022

333fred left a comment

Choose a reason for hiding this comment

333fred left a comment

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

Youssef1313 commented May 3, 2022 •

edited

Loading

Youssef1313 May 3, 2022 •

edited

Loading

AlekseyTs May 4, 2022 •

edited

Loading

AlekseyTs May 4, 2022 •

edited

Loading

AlekseyTs May 4, 2022 •

edited

Loading

AlekseyTs May 4, 2022 •

edited

Loading

AlekseyTs May 4, 2022 •

edited

Loading

AlekseyTs commented May 4, 2022 •

edited

Loading

AlekseyTs May 4, 2022 •

edited

Loading

AlekseyTs May 4, 2022 •

edited

Loading