Zero fields before running field initializers in struct constructor with default `: this()` initializer by cston · Pull Request #60471 · dotnet/roslyn

cston · 2022-03-30T00:08:20Z

When a struct instance constructor has a this() constructor initializer that represents the default parameterless constructor, the declared constructor implicitly clears all instance fields and performs the initializations specified by the variable_initializers of the instance fields declared in its struct. Immediately upon entry to the constructor, all value type fields are set to their default value and all reference type fields are set to null. Immediately after that, a sequence of assignments corresponding to the variable_initializers are executed.

Fixes #58790

RikkiGibson

LGTM, however we did discuss offline whether it's possible to make the change simpler by keeping the existing constructor initializer code path the same and adjusting the path that adds field initializers to the body to do a special check for the default parameterless constructor. Would like to see whether any changes are made as a result of that exploration before signing off.

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

RikkiGibson · 2022-03-30T21:56:01Z

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

+            comp.VerifyDiagnostics(
+                // (10,12): error CS0171: Field 'S1.y' must be fully assigned before control is returned to the caller
+                //     public S1() { }
+                Diagnostic(ErrorCode.ERR_UnassignedThis, "S1").WithArguments("S1.y").WithLocation(10, 12),


Let's make sure we keep an eye out after this is merged to update the test baselines when this flows forward to 17.3/main-vs-deps.

RikkiGibson · 2022-03-30T21:59:49Z

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

+                Diagnostic(ErrorCode.ERR_FieldInitRefNonstatic, "y1").WithArguments("S1.y1").WithLocation(3, 14),
+                // (3,14): error CS0170: Use of possibly unassigned field 'y1'
+                //     int x1 = y1 + 1;
+                Diagnostic(ErrorCode.ERR_UseDefViolationField, "y1").WithArguments("y1").WithLocation(3, 14),


Many of the new [Theory] tests will have divergent diagnostics when this flows to main-vs-deps. The tests can still be written this way, just wanted to call it out.

It looks like there are 5 or so tests that will be affected. But it might simpler if I leave these combined for now, then split them when merging into main.

AlekseyTs · 2022-03-30T22:28:36Z

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

                                                  bodyBinder.GetDeclaredLocalsForScope(constructor),
-                                                  skipInitializer ? new BoundNoOpStatement(constructor, NoOpStatementFlavor.Default)
-                                                      : initializer == null ? null : bodyBinder.BindConstructorInitializer(initializer, diagnostics),
+                                                  initializer == null ? null : bodyBinder.BindConstructorInitializer(initializer, diagnostics),


initializer == null ? null : bodyBinder.BindConstructorInitializer(initializer, diagnostics),

Is this change observable in IOperation tree? Consider adding a test. #Closed

cston · 2022-03-30T22:41:32Z

make the change simpler by keeping the existing constructor initializer code path the same and adjusting the path that adds field initializers to the body

The current approach combines the constructor initializer and body (into a BoundConstructorMethodBody) in Binder.BindConstructorBody and then inserts the field initializers between the two sets of statements a couple of callers higher in MethodCompiler.CompileMethod. But in between, MethodCompiler.BindMethodBody also generates the methodBodyForSemanticModel from the BoundConstructorMethodBody so it feels like that method body should include the constructor initializer as it would for any other constructor initializer case.

In reply to: 925601353

AlekseyTs · 2022-03-30T22:44:42Z

I also like the clean-up in the Binder and prefer if we go with that.

In reply to: 1083707550

AlekseyTs · 2022-03-30T23:08:47Z

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

+{
+    int x = 1;
+    int y;
+    public S(int y) : this()


public S(int y) : this()

Is this a scenario that used to be an error? #Closed

Yes, previously:

(6,12): error CS0171: Field 'S.y' must be fully assigned before control is returned to the caller public S(int y) : this() (15,12): error CS0171: Field 'R.y' must be fully assigned before control is returned to the caller public R(int y) : this()

AlekseyTs

LGTM (commit 6)

…ros fields before field initializers

cston · 2022-03-31T17:33:47Z

The failure in Test_Windows_Desktop_Spanish_Release_64 appears to be a known infrastructure issue.

cston marked this pull request as ready for review March 30, 2022 06:03

cston requested a review from a team as a code owner March 30, 2022 06:03

RikkiGibson self-assigned this Mar 30, 2022

runfoapp bot mentioned this pull request Mar 30, 2022

[Flaky integration tests] IDE version '17.2.32314.265' unexpectedly dismissed the light bulb. #60461

Closed

cston requested review from AlekseyTs and RikkiGibson March 30, 2022 18:06

RikkiGibson reviewed Mar 30, 2022

View reviewed changes

RikkiGibson approved these changes Mar 30, 2022

View reviewed changes

AlekseyTs reviewed Mar 30, 2022

View reviewed changes

AlekseyTs approved these changes Mar 30, 2022

View reviewed changes

runfoapp bot mentioned this pull request Mar 31, 2022

Timeout on Microsoft.VisualStudio.LanguageServices.CSharp.UnitTests.dll tests #55638

Open

cston added 7 commits March 31, 2022 08:15

struct constructor with default ': this()' constructor initializer ze…

37d0b4a

…ros fields before field initializers

Remove unused parameters

488c7cd

Add tests

764619c

More tests

25068bc

Cleanup

e37dce1

PR feedback

08cf4bf

Formatting

fe43cb3

cston force-pushed the 58790 branch from 9ce64ec to fe43cb3 Compare March 31, 2022 15:16

RikkiGibson merged commit afe5eef into dotnet:release/dev17.2 Mar 31, 2022

cston deleted the 58790 branch March 31, 2022 18:29

cston mentioned this pull request Mar 31, 2022

Struct ': this()' initializers should not have definite assignment errors #58790

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Zero fields before running field initializers in struct constructor with default `: this()` initializer#60471

Zero fields before running field initializers in struct constructor with default `: this()` initializer#60471
RikkiGibson merged 7 commits intodotnet:release/dev17.2from
cston:58790

cston commented Mar 30, 2022 •

edited

Loading

Uh oh!

RikkiGibson left a comment

Uh oh!

Uh oh!

RikkiGibson Mar 30, 2022

Uh oh!

RikkiGibson Mar 30, 2022

Uh oh!

cston Mar 30, 2022

Uh oh!

AlekseyTs Mar 30, 2022 •

edited

Loading

Uh oh!

cston commented Mar 30, 2022

Uh oh!

AlekseyTs commented Mar 30, 2022

Uh oh!

AlekseyTs Mar 30, 2022 •

edited

Loading

Uh oh!

cston Mar 30, 2022 •

edited

Loading

Uh oh!

AlekseyTs left a comment

Uh oh!

cston commented Mar 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

cston commented Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RikkiGibson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RikkiGibson Mar 30, 2022

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Mar 30, 2022

Choose a reason for hiding this comment

Uh oh!

cston Mar 30, 2022

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston commented Mar 30, 2022

Uh oh!

AlekseyTs commented Mar 30, 2022

Uh oh!

AlekseyTs Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

cston commented Mar 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cston commented Mar 30, 2022 •

edited

Loading

AlekseyTs Mar 30, 2022 •

edited

Loading

AlekseyTs Mar 30, 2022 •

edited

Loading

cston Mar 30, 2022 •

edited

Loading