Implicit field initialization in struct constructors by RikkiGibson · Pull Request #59788 · dotnet/roslyn

RikkiGibson · 2022-02-26T01:00:45Z

Related to #60167

This design introduces a set of "disabled by default" diagnostics for definite assignment analysis of 'this' in struct constructors. Currently, in order to enable the diagnostics, the user would need to do something like the following:

# .editorconfig
dotnet_diagnostic.CS9018.severity = warning
dotnet_diagnostic.CS9019.severity = warning
dotnet_diagnostic.CS9020.severity = warning
dotnet_diagnostic.CS9021.severity = warning
dotnet_diagnostic.CS9022.severity = warning

I considered introducing some kind of shorthand which would allow specifying this whole set of diagnostics at once, but it feels like it might be worth waiting for user feedback before we add something like that.

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

RikkiGibson · 2022-03-03T20:20:43Z

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

+            var builder = ArrayBuilder<BoundStatement>.GetInstance(implicitlyInitializedFields.Length + 1);
+            foreach (var field in implicitlyInitializedFields)
+            {
+                builder.Add(new BoundExpressionStatement(


Do we need a use site diagnostic here? see CodeGenTupleTests.CustomValueTuple_StructWithConstructor in this branch.

It might make more sense to give a "Missing compiler required member" on the tuple declaration which is missing the well-known fields.

I'm leaning toward allowing the current behavior here. Declaring your own ValueTuple is a pretty rare thing to do and I wasn't able to find a decent way to introduce the use site diagnostics here without adding redundant use site diagnostics in other scenarios.

…assignment

src/Compilers/CSharp/Portable/Errors/DiagnosticBagExtensions.cs

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

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

RikkiGibson · 2022-03-21T20:02:38Z

@cston @jcouv for another review pass

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

src/Compilers/CSharp/Portable/Errors/ErrorCode.cs

src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs

docs/Language Feature Status.md

src/Compilers/CSharp/Test/Symbol/Symbols/Source/EventTests.cs

src/Compilers/Test/Utilities/CSharp/CSharpTestBase.cs

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

jcouv

Done with review pass (iteration 32). Overall, looks very nice

jcouv

LGTM Thanks (iteration 33)

jcouv · 2022-03-23T23:00:27Z

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

+                references: moduleReference,
+                options: TestOptions.DebugDll.WithWarningLevel(5),
+                parseOptions: TestOptions.Regular10,
+                verify: Verification.Skipped);


Consider modifying those tests to also verify execution (expectedOutput: should show reads of auto-defaulted values)

cston · 2022-03-24T20:39:28Z

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

+            {
+                var associatedSymbol = fieldSymbol.AssociatedSymbol;
+                var hasAssociatedProperty = associatedSymbol?.Kind == SymbolKind.Property;
+                var symbolName = associatedSymbol?.Kind == SymbolKind.Property ? associatedSymbol.Name : fieldSymbol.Name;


associatedSymbol?.Kind == SymbolKind.Property

Minor: hasAssociatedProperty

cston · 2022-03-24T21:00:28Z

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

+                while (true)
+                {
+                    var containingSlot = variableBySlot[fieldSlot].ContainingSlot;
+                    if (containingSlot == fieldSlot)


containingSlot == fieldSlot

Minor: This seems surprising to check that fieldSlot is the same as its containingSlot. Could we simply check if (fieldSlot == 0) instead, and before using variableBySlot[fieldSlot].ContainingSlot?

…t-definite-assignment

src/EditorFeatures/CSharpTest/ConvertTupleToStruct/ConvertTupleToStructTests.cs

RikkiGibson · 2022-03-26T04:19:03Z

This is should be ready to go now @jaredpar. There are some test plan items that can be addressed in the coming weeks.

…t-definite-assignment

WIP

b2ba13b

ghost added the Area-Compilers label Feb 26, 2022

Fix bugs. Update semantic tests.

e167c09

runfoapp bot mentioned this pull request Mar 1, 2022

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

Open

RikkiGibson added 2 commits March 1, 2022 17:36

Fix remaining semantic tests

741403b

Fix more tests

3fbe7e6

RikkiGibson mentioned this pull request Mar 2, 2022

Implicit struct initialization: handle fields at any depth? #59890

Open

RikkiGibson added 2 commits March 2, 2022 14:31

Address most prototype comments

c1b4562

PROTOTYPE -> TODO

e942cad

RikkiGibson commented Mar 2, 2022

View reviewed changes

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

don't give use site diagnostics for now

ffc6125

RikkiGibson commented Mar 3, 2022

View reviewed changes

Merge branch 'main' of github.com:dotnet/roslyn into struct-definite-…

5f23f47

…assignment

RikkiGibson marked this pull request as ready for review March 3, 2022 20:23

RikkiGibson requested review from a team as code owners March 3, 2022 20:23

cston reviewed Mar 7, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Errors/DiagnosticBagExtensions.cs Outdated Show resolved Hide resolved

cston reviewed Mar 7, 2022

View reviewed changes

src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs Outdated Show resolved Hide resolved

cston reviewed Mar 7, 2022

View reviewed changes

src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs Outdated Show resolved Hide resolved

RikkiGibson commented Mar 7, 2022

View reviewed changes

src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs Outdated Show resolved Hide resolved

cston reviewed Mar 7, 2022

View reviewed changes

src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs Outdated Show resolved Hide resolved

cston reviewed Mar 7, 2022

View reviewed changes

src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs Outdated Show resolved Hide resolved

RikkiGibson and others added 2 commits March 7, 2022 16:18

Address feedback

c0d76f2

Update src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs

24159b6

cston reviewed Mar 8, 2022

View reviewed changes

src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs Outdated Show resolved Hide resolved

cston reviewed Mar 8, 2022

View reviewed changes

src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs Outdated Show resolved Hide resolved

cston reviewed Mar 8, 2022

View reviewed changes

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

cston reviewed Mar 8, 2022

View reviewed changes

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

cston reviewed Mar 8, 2022

View reviewed changes

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

cston reviewed Mar 8, 2022

View reviewed changes

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

RikkiGibson added 2 commits March 18, 2022 12:27

Address prototype. Move error codes for now.

e16572c

Update ErrorFacts.Generated.cs

307fcab

jcouv reviewed Mar 21, 2022

View reviewed changes

src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs Outdated Show resolved Hide resolved

jcouv reviewed Mar 21, 2022

View reviewed changes

src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs Outdated Show resolved Hide resolved

jcouv reviewed Mar 21, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Errors/ErrorCode.cs Show resolved Hide resolved

jcouv reviewed Mar 21, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs Show resolved Hide resolved

jcouv reviewed Mar 21, 2022

View reviewed changes

docs/Language Feature Status.md Show resolved Hide resolved

jcouv reviewed Mar 21, 2022

View reviewed changes

src/Compilers/CSharp/Test/Symbol/Symbols/Source/EventTests.cs Show resolved Hide resolved

jcouv reviewed Mar 21, 2022

View reviewed changes

src/Compilers/Test/Utilities/CSharp/CSharpTestBase.cs Show resolved Hide resolved

jcouv reviewed Mar 21, 2022

View reviewed changes

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

jcouv reviewed Mar 21, 2022

View reviewed changes

Address feedback. Make diagnostic messages consistent.

4c13809

jcouv approved these changes Mar 22, 2022

View reviewed changes

jcouv reviewed Mar 23, 2022

View reviewed changes

cston reviewed Mar 24, 2022

View reviewed changes

cston approved these changes Mar 24, 2022

View reviewed changes

RikkiGibson added 2 commits March 25, 2022 21:11

Merge branch 'release/dev17.3' of github.com:dotnet/roslyn into struc…

0bb7707

…t-definite-assignment

Address feedback and pack error codes

6bcda83

RikkiGibson commented Mar 26, 2022

View reviewed changes

src/EditorFeatures/CSharpTest/ConvertTupleToStruct/ConvertTupleToStructTests.cs Show resolved Hide resolved

This was referenced Mar 28, 2022

csharpcodedefinitionwindow.codedefinitionwindowopensmetadataassource failing #60363

Closed

csharpcodedefinitionwindow.codedefinitionwindowopensmetadataassource failing #60364

Closed

Merge branch 'release/dev17.3' of github.com:dotnet/roslyn into struc…

99fad4e

…t-definite-assignment

jaredpar approved these changes Mar 28, 2022

View reviewed changes

davidwengier approved these changes Mar 28, 2022

View reviewed changes

RikkiGibson enabled auto-merge (squash) March 28, 2022 21:59

RikkiGibson merged commit 65d5234 into dotnet:release/dev17.3 Mar 28, 2022

RikkiGibson deleted the struct-definite-assignment branch March 28, 2022 23:08

Conversation

RikkiGibson commented Feb 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

RikkiGibson Mar 3, 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!

RikkiGibson Mar 21, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RikkiGibson commented Mar 21, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

cston Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

cston Mar 24, 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!

Uh oh!

RikkiGibson commented Mar 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

RikkiGibson commented Feb 26, 2022 •

edited

Loading

RikkiGibson Mar 3, 2022 •

edited

Loading

cston Mar 24, 2022 •

edited

Loading