Field-backed properties: definite assignment #75116

cston · 2024-09-13T22:01:40Z

Similar to changes started in earlier features/semi-auto-props branch in #58832, #60601.

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

RikkiGibson · 2024-09-19T20:46:40Z

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

+                                        propertyAccess.ReceiverOpt,
+                                        propertyAccess.InitialBindingReceiverIsSubjectToCloning,
+                                        propertyAccess.PropertySymbol,
+                                        useAsLvalue.Value ? AccessorKind.Set : AccessorKind.Get, // PROTOTYPE: What about compound assignment, increment?


Consider extracting and reusing logic from 'GetIndexerAccessorKind' in order to determine this. #Resolved

RikkiGibson

Per offline discussion I am wondering if a somewhat smaller change would be adequate to address the scenario. It looks like the existing behavior with auto properties in struct constructors is:

property get: auto-default all unassigned fields, even if getter is auto-implemented.
property set: don't auto-default anything, if the set accessor is auto-implemented or absent.

So, shouldn't a relatively small change to logic in definite assignment be adequate here (check if setter specifically is auto-implemented rather than whole property)? There would be no need to adjust handling of property reads, compound assignments or increment/decrement.

cston · 2024-09-20T15:23:19Z

... It looks like the existing behavior with auto properties in struct constructors is:

property get: auto-default all unassigned fields, even if getter is auto-implemented.

property set: don't auto-default anything, if the set accessor is auto-implemented or absent.

Calling get with an auto property today will cause that one backing field to be initialized before the call but does not affect other fields that have yet to be assigned.

For instance, with the following, y = P1 results in initialization of P1 but not F1. See sharplab.io.

struct S1
{
    public int F1;
    public int P1 { get; }
    public S1(int x, out int y)
    {
        y = P1;
        F1 = x;
    }
}

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

RikkiGibson

Sorry, I still need more time to finish review, but had a few more comments.

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

RikkiGibson · 2024-09-20T21:38:19Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

@@ -1073,6 +1074,7 @@ static IEnumerable<Symbol> getAllMembersToBeDefaulted(Symbol requiredMember)
                            }
                        }

+                        // PROTOTYPE: Is checking IsAutoProperty sufficient or should we check the appropriate accessors are missing or auto-implemented?


I will try and resolve this comment in my upcoming PR. I think we should just check for a BackingField and return it here.

Moved comment to test plan #57012.

RikkiGibson · 2024-09-20T21:43:30Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

@@ -1964,6 +1966,8 @@ protected override bool TryGetReceiverAndMember(BoundExpression expr, out BoundE
                        var propAccess = (BoundPropertyAccess)expr;
                        var propSymbol = propAccess.PropertySymbol;
                        member = propSymbol;
+                        // PROTOTYPE: Why don't we call Binder.AccessingAutoPropertyFromConstructor


I will also try to resolve this comment. I think I know why--nullable prefers to rewrite field initializers to be assigning to the property, for example, and then when a field is visited for constructor checks, we work back to the property and use the property's slot. Perhaps definite assignment works the opposite way, deciding when a property access is "morally" an access of the backing field, and treating it as the latter instead.

Moved comment to test plan #57012.

RikkiGibson · 2024-09-23T00:30:19Z

src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs

+            {
+                if (_found is { })
+                {
+                    return node;


Is this return value used? #Resolved

No, the return value is not used, and in fact most of the visit methods return null. I'll change this to return null;.

RikkiGibson

LGTM, nothing blocking.

RikkiGibson · 2024-09-23T18:23:32Z

src/Compilers/CSharp/Test/Emit3/FieldKeywordTests.cs

+                {
+                    public int F1;
+                    public int P1 { get; }
+                    public C1(int i) { P1 += i; }


It seems like this and the other scenarios in this test would be more illustrative if F1 were explicitly assigned after the P1 += i; statement. That would show whether the property access is causing auto-default of the whole instance.

Same for the ++, --, ??= tests below. #Resolved

Thanks, that makes sense.

RikkiGibson · 2024-09-23T18:58:01Z

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

@@ -2194,7 +2194,7 @@
    <!-- Whether receiver will need to be cloned during emit (only valid before lowering) -->
    <Field Name="InitialBindingReceiverIsSubjectToCloning" Type="ThreeState"/>
    <Field Name="PropertySymbol" Type="PropertySymbol"/>
-    <!-- The property accessor referenced if the property access should use the backing field directly instead. -->
+    <!-- The property accessor kind, set for properties with synthesized backing field only. -->
    <Field Name="UseBackingField" Type="AccessorKind" />


I'm not 100% happy with this field name, but I don't have any suggestions for a better one. It feels like this name is saying "this property access should be replaced with a use of the backing field". #Resolved

Renamed to BoundPropertyAccess.AutoPropertyAccessorKind.

RikkiGibson · 2024-09-23T20:14:05Z

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

@@ -2195,7 +2195,7 @@
    <Field Name="InitialBindingReceiverIsSubjectToCloning" Type="ThreeState"/>
    <Field Name="PropertySymbol" Type="PropertySymbol"/>
    <!-- The property accessor kind, set for properties with synthesized backing field only. -->


is it also the case that this will only have a non-Unknown value when auto accessors are being used? #Resolved

The value will be set to something other than AccessorKind.Unknown for classic auto properties and for properties that use field. Properties that use field may include an auto-implemented accessor.

Field-backed properties: definite assignment

cc60b13

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Sep 13, 2024

cston added 3 commits September 13, 2024 15:11

Remove parameter

192059b

More definite assignment; initial nullability support

e005992

More definite assignment

719e3f0

cston force-pushed the field-more branch 2 times, most recently from a1ecdc9 to 45ab7f9 Compare September 18, 2024 05:37

build-analysis bot mentioned this pull request Sep 18, 2024

Roslyn analyzer throws error AD0001 NullReferenceException dotnet/dnceng#3305

Open

3 tasks

Differentiate l-value and r-value property use in ctor

eb352eb

cston force-pushed the field-more branch from 46ed4f4 to eb352eb Compare September 18, 2024 16:12

Cleanup; more tests

f036e7c

cston force-pushed the field-more branch from 8425e3f to f036e7c Compare September 18, 2024 20:56

Add BoundPropertyAccess.UseBackingField

ae768e6

cston force-pushed the field-more branch from e555744 to ae768e6 Compare September 19, 2024 18:05

cston added 2 commits September 19, 2024 11:38

Fix test

3c87d9b

Cleanup

823dd09

cston marked this pull request as ready for review September 19, 2024 20:11

cston requested a review from a team as a code owner September 19, 2024 20:11

cston requested review from 333fred and RikkiGibson September 19, 2024 20:12

RikkiGibson self-assigned this Sep 19, 2024

333fred reviewed Sep 19, 2024

View reviewed changes

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

RikkiGibson reviewed Sep 19, 2024

View reviewed changes

333fred approved these changes Sep 19, 2024

View reviewed changes

RikkiGibson reviewed Sep 19, 2024

View reviewed changes

Use GetAccessorKind(); additional test cases

f813aef

RikkiGibson reviewed Sep 20, 2024

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs Show resolved Hide resolved

RikkiGibson reviewed Sep 20, 2024

View reviewed changes

cston added 3 commits September 21, 2024 07:50

Address feedback

84cc176

Fix tests

00556f1

Add visitor to verify backing field access

03a36a4

RikkiGibson reviewed Sep 23, 2024

View reviewed changes

cston added 3 commits September 22, 2024 22:58

Return null

c7c492d

Cache accessor kind, ignoring accessor implementation

ab72902

Update comment

0fc26c9

RikkiGibson approved these changes Sep 23, 2024

View reviewed changes

cston added 3 commits September 23, 2024 12:29

Rename to BoundPropertyAccess.AutoPropertyAccessorKind

d008fa5

Update tests to include field assignment

9a6f703

Remove PROTOTYPE comments

6129f43

cston mentioned this pull request Sep 23, 2024

Test plan for "field keyword in properties" #57012

Open

RikkiGibson reviewed Sep 23, 2024

View reviewed changes

RikkiGibson approved these changes Sep 23, 2024

View reviewed changes

333fred approved these changes Sep 23, 2024

View reviewed changes

cston merged commit 5aceebc into dotnet:features/field-keyword Sep 23, 2024
24 checks passed

cston deleted the field-more branch September 23, 2024 22:22

RikkiGibson mentioned this pull request Sep 25, 2024

NullableWalker.Scan().getFieldSymbolToBeInitialized(): Is checking IsAutoProperty sufficient or should we check the appropriate accessors are missing or auto-implemented #75245

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Field-backed properties: definite assignment #75116

Field-backed properties: definite assignment #75116

cston commented Sep 13, 2024 •

edited

Loading

RikkiGibson Sep 19, 2024 •

edited by cston

Loading

RikkiGibson left a comment •

edited

Loading

cston commented Sep 20, 2024

RikkiGibson left a comment

RikkiGibson Sep 20, 2024

cston Sep 23, 2024

RikkiGibson Sep 20, 2024 •

edited by cston

Loading

cston Sep 23, 2024

RikkiGibson Sep 23, 2024 •

edited by cston

Loading

cston Sep 23, 2024

RikkiGibson left a comment

RikkiGibson Sep 23, 2024 •

edited by cston

Loading

cston Sep 23, 2024

RikkiGibson Sep 23, 2024 •

edited by cston

Loading

cston Sep 23, 2024

RikkiGibson Sep 23, 2024 •

edited by cston

Loading

cston Sep 23, 2024

Field-backed properties: definite assignment #75116

Field-backed properties: definite assignment #75116

Conversation

cston commented Sep 13, 2024 • edited Loading

RikkiGibson Sep 19, 2024 • edited by cston Loading

Choose a reason for hiding this comment

RikkiGibson left a comment • edited Loading

Choose a reason for hiding this comment

cston commented Sep 20, 2024

RikkiGibson left a comment

Choose a reason for hiding this comment

RikkiGibson Sep 20, 2024

Choose a reason for hiding this comment

cston Sep 23, 2024

Choose a reason for hiding this comment

RikkiGibson Sep 20, 2024 • edited by cston Loading

Choose a reason for hiding this comment

cston Sep 23, 2024

Choose a reason for hiding this comment

RikkiGibson Sep 23, 2024 • edited by cston Loading

Choose a reason for hiding this comment

cston Sep 23, 2024

Choose a reason for hiding this comment

RikkiGibson left a comment

Choose a reason for hiding this comment

RikkiGibson Sep 23, 2024 • edited by cston Loading

Choose a reason for hiding this comment

cston Sep 23, 2024

Choose a reason for hiding this comment

RikkiGibson Sep 23, 2024 • edited by cston Loading

Choose a reason for hiding this comment

cston Sep 23, 2024

Choose a reason for hiding this comment

RikkiGibson Sep 23, 2024 • edited by cston Loading

Choose a reason for hiding this comment

cston Sep 23, 2024

Choose a reason for hiding this comment

cston commented Sep 13, 2024 •

edited

Loading

RikkiGibson Sep 19, 2024 •

edited by cston

Loading

RikkiGibson left a comment •

edited

Loading

RikkiGibson Sep 20, 2024 •

edited by cston

Loading

RikkiGibson Sep 23, 2024 •

edited by cston

Loading

RikkiGibson Sep 23, 2024 •

edited by cston

Loading

RikkiGibson Sep 23, 2024 •

edited by cston

Loading

RikkiGibson Sep 23, 2024 •

edited by cston

Loading