Field-backed properties: update uses of IsAutoProperty #74798
Conversation
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs
Outdated
Show resolved
Hide resolved
| comp.VerifyEmitDiagnostics( | ||
| // (3,28): error CS8080: Auto-implemented properties must override all accessors of the overridden property. | ||
| // public override object P1 { get => field; } | ||
| Diagnostic(ErrorCode.ERR_AutoPropertyMustOverrideSet, "P1").WithLocation(3, 28), |
There was a problem hiding this comment.
I always forget about this. Yes I feel fine with saying that if you use the field keyword then you must override all the accessors. It feels like if you forgot to override one and you are using that keyword then something is going wrong, a value is being dropped on the floor somewhere.
| @@ -1765,7 +1765,7 @@ private static bool AccessingAutoPropertyFromConstructor(BoundExpression receive | |||
| var propertyIsStatic = propertySymbol.IsStatic; | |||
|
|
|||
| return (object)sourceProperty != null && | |||
| sourceProperty.IsAutoProperty && | |||
| sourceProperty.IsAutoPropertyOrUsesFieldKeyword && | |||
There was a problem hiding this comment.
I'm not so sure that this is 100% correct. There's a usage in AbstractFlowPass that visits the backing field when this returns true; are we sure we want that behavior for all flow passes? #Resolved
There was a problem hiding this comment.
Good catch, thanks. For now, I've limited this change to the one caller that expects this method to include field cases.
| { | ||
| if (!IsStatic && SetMethod is { IsInitOnly: false }) | ||
| if (!IsStatic && ((_propertyFlags & Flags.HasAutoPropertySet) != 0) && SetMethod is { IsInitOnly: false }) |
There was a problem hiding this comment.
I'm not sure why we'd condition this on having set;, as opposed to any setter.
There was a problem hiding this comment.
Added an open question for language design: dotnet/csharplang#8389
src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs
Outdated
Show resolved
Hide resolved
| if ((overriddenProperty.GetMethod is { } && GetMethod is null) || | ||
| (overriddenProperty.SetMethod is { } && SetMethod is null)) |
There was a problem hiding this comment.
Correct me if I'm wrong, but this appears to be a fundamentally different check than was here earlier. What was reporting the error on a missing get override earlier?
There was a problem hiding this comment.
Previously, for a set-only auto-property that overrides a virtual property with a get, the compiler would report:
CS8051: Auto-implemented properties must have get accessors.
And the compiler would not report:
CS8080: Auto-implemented properties must override all accessors of the overridden property.
See sharplab.io.
The proposal text for initializers appears to have been based on LDM-2022-01-12 but there was a more recent decision in LDM-2022-03-02, as you found. I'll update the proposal and implementation. |
Co-authored-by: Fred Silberberg <fred@silberberg.xyz>
Relates to test plan #57012