Handle various cases for field-backed properties #74641
Conversation
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
cston
changed the title
| string source = $$""" | ||
| {{typeKind}} A | ||
| { | ||
| public static object P1 { get; set { _ = field; } } |
| { | ||
| static object P1 { get; set { _ = field; } } | ||
| static object P2 { get { return field; } set; } | ||
| object Q1 { get; set { _ = field; } } |
There was a problem hiding this comment.
Will review the tests in depth later, but it feels reasonable for the synthesized field's "location" to be the first usage of the field keyword, and to report the exact error you are saying there.
src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
| """; | ||
| var verifier = CompileAndVerify(source, verify: Verification.Skipped, targetFramework: TargetFramework.Net80); |
There was a problem hiding this comment.
Perhaps should also verify execution of the accessors. Not necessarily in this same test.
Should also verify that the manually-implemented accessors behave the way we expect. (and, for example, the impl is not being unexpectedly replaced with an auto-impl.) #Resolved
| usesFieldKeyword = body is { } && containsFieldKeyword(body); | ||
| Debug.Assert(accessorsHaveImplementation); // it's not clear how this even parsed as a property if it has no accessor list and no arrow expression. | ||
| Debug.Assert(hasGetAccessorImplementation); // it's not clear how this even parsed as a property if it has no accessor list and no arrow expression. |
There was a problem hiding this comment.
Since you're modifying this line anyway, consider making this comment an actual assert message.
| (hasGetAccessor && !hasSetAccessor) ? MessageID.IDS_FeatureReadonlyAutoImplementedProperties : MessageID.IDS_FeatureAutoImplementedProperties, | ||
| hasGetAccessor && hasSetAccessor ? | ||
| (hasAutoPropertyGet && hasAutoPropertySet ? MessageID.IDS_FeatureAutoImplementedProperties : MessageID.IDS_FeatureFieldKeyword) : | ||
| MessageID.IDS_FeatureReadonlyAutoImplementedProperties, |
There was a problem hiding this comment.
nit: It seems like the new factoring is going to diagnose a setter-only auto property (error case) as requiring the readonly auto-properties feature. #Resolved
| @@ -140,7 +142,9 @@ internal static SourcePropertySymbol Create(SourceMemberContainerTypeSymbol cont | |||
| { | |||
| Binder.CheckFeatureAvailability( | |||
| syntax, | |||
| (hasGetAccessor && !hasSetAccessor) ? MessageID.IDS_FeatureReadonlyAutoImplementedProperties : MessageID.IDS_FeatureAutoImplementedProperties, | |||
| hasGetAccessor && hasSetAccessor ? | |||
There was a problem hiding this comment.
nit: I think it would be easier to grasp what scenario we are in, in each branch, if the above IsAutoProperty were replaced with hasAutoPropertyGet || hasAutoPropertySet. #Resolved
| public int P3 { get; set { } } | ||
| public int P4 { get { return -4; } set; } | ||
| public int P5 { get; init { } } | ||
| public int P6 { get { return -6; } init; } |
There was a problem hiding this comment.
for the future, I would def consider a "half-auto" prop, where the manually implemented accessor never reads field, to possibly be a warning scenario.
For example, if you have get => notField; set; or get; set => notField = value, it practically means you want to ignore the value that was set. There are better ways to do that than introducing a backing field. It very likely means you are making a mistake.
Quite possibly this was already considered, apologies for reiterating these points if so.
There was a problem hiding this comment.
There is an open question in the proposal.
There was a problem hiding this comment.
To obtain these warnings, I added https://github.com/dotnet/csharplang/blob/main/proposals/field-keyword.md#field-usage-warnings since the proposal hadn't spoken to this aspect at all yet.
Changes:
[field:]attributes targeting synthesized backing field