Report error if 'record struct' constructor calls default parameterless constructor #58339

cston · 2021-12-15T00:27:57Z

Confirmed with C# LDT:
In a record struct with a primary constructor, explicit constructors require a this() initializer that invokes the primary constructor or another explicitly declared constructor.

Fixes #58328

cston · 2021-12-15T00:30:16Z

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

+
+        [Fact]
+        [WorkItem(58328, "https://github.com/dotnet/roslyn/issues/58328")]
+        public void ExplicitConstructors_05()


ExplicitConstructors_05

The this() constructor initializer calls in this test and the following resulted in invalid code previously.

Youssef1313 · 2021-12-15T14:25:49Z

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

@@ -1993,7 +1993,7 @@ internal enum ErrorCode
        WRN_CallerArgumentExpressionParamForUnconsumedLocation = 8966,
        ERR_NewlinesAreNotAllowedInsideANonVerbatimInterpolatedString = 8967,
        ERR_AttrTypeArgCannotBeTypeVar = 8968,
-        // WRN_AttrDependentTypeNotAllowed = 8969, // Backed out of of warning wave 6, may be reintroduced later
+        ERR_RecordStructConstructorCallsDefaultConstructor = 8969, // Added in 17.1 for an invalid C#10 scenario.


Is reusing the warning number okay here? #Resolved

Maybe, but reusing seems unnecessary. Updated.

…ss constructor

333fred

LGTM (commit 1)

AlekseyTs · 2021-12-20T16:01:47Z

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

@@ -3524,6 +3530,9 @@ private BoundNode BindConstructorBody(ConstructorDeclarationSyntax constructor,
                                                      null :
                                                      bodyBinder.BindExpressionBodyAsBlock(constructor.ExpressionBody,
                                                                                           constructor.Body == null ? diagnostics : BindingDiagnosticBag.Discarded));
+
+            bool hasAnyRecordConstructors() =>


hasAnyRecordConstructors

I think the name is confusing. The body is checking for a primary constructor, not just any constructor. #Closed

AlekseyTs · 2021-12-20T16:07:46Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -6911,6 +6911,9 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
  <data name="ERR_LambdaWithAttributesToExpressionTree" xml:space="preserve">
    <value>A lambda expression with attributes cannot be converted to an expression tree</value>
  </data>
+  <data name="ERR_RecordStructConstructorCallsDefaultConstructor" xml:space="preserve">
+    <value>A 'this' initializer for a 'record struct' constructor must call the primary constructor or an explicitly declared constructor.</value>


A 'this' initializer for a 'record struct' constructor must call the primary constructor or an explicitly declared constructor.

The wording looks too general. The requirement is only for records with primary constructors, I think the message should reflect that. See, for example, how we do that for UnexpectedOrMissingConstructorInitializerInRecord:

<data name="ERR_UnexpectedOrMissingConstructorInitializerInRecord" xml:space="preserve"> <value>A constructor declared in a record with parameter list must have 'this' constructor initializer.</value> </data> ``` #Closed

AlekseyTs · 2021-12-20T16:16:02Z

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

@@ -3514,6 +3514,12 @@ private BoundNode BindConstructorBody(ConstructorDeclarationSyntax constructor,
                && thisInitializer
                && ContainingType.IsDefaultValueTypeConstructor(initializer);

+            if (skipInitializer &&


skipInitializer

Should we also check if !constructorSymbol.IsStatic? #Closed

AlekseyTs · 2021-12-20T16:23:49Z

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

+}
+record struct S4(char A, char B)
+{
+    public S4(object o) : this() { }


public S4(object o) : this() { }

Consider also testing with static constructors. For example:

record struct S(int x) { static S() : this() {} static int F = 3; } ``` #Closed

AlekseyTs · 2021-12-20T16:38:12Z

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

@@ -3514,6 +3514,12 @@ private BoundNode BindConstructorBody(ConstructorDeclarationSyntax constructor,
                && thisInitializer
                && ContainingType.IsDefaultValueTypeConstructor(initializer);

+            if (skipInitializer &&


skipInitializer

The fact that skipInitializer depends on includesFieldInitializers looks suspicious. I think it is technically possible to have a primary constructor and no initializers. It would be good trying to construct a scenario like that. Consider instead using condition thisInitializer && ContainingType.IsDefaultValueTypeConstructor(initializer), I think that would exactly reflect the condition that design requires us to check.
#Closed

AlekseyTs · 2021-12-20T16:41:16Z

Done with review pass (commit 2)

cston · 2021-12-20T18:56:32Z

Responded to additional feedback in #58430.

…ss constructor (dotnet#58339)

…implicit parameterless constructor (#59055) * Report error if 'record struct' constructor calls default parameterless constructor (#58339) * Improve error reporting for 'this()' initializer from 'record struct' constructor * Require definite assignment of all fields if struct includes any field initializers (#57925)

dotnet-issue-labeler bot added the Area-Compilers label Dec 15, 2021

cston commented Dec 15, 2021

View reviewed changes

Youssef1313 reviewed Dec 15, 2021

View reviewed changes

cston force-pushed the 58328 branch from 5240c67 to d9b92c5 Compare December 15, 2021 16:22

Report error if 'record struct' constructor calls default parameterle…

f954682

…ss constructor

cston force-pushed the 58328 branch from d9b92c5 to f954682 Compare December 15, 2021 17:05

cston marked this pull request as ready for review December 15, 2021 17:48

cston requested a review from a team as a code owner December 15, 2021 17:48

333fred approved these changes Dec 15, 2021

View reviewed changes

chsienki approved these changes Dec 15, 2021

View reviewed changes

Update error message

5b40110

333fred approved these changes Dec 15, 2021

View reviewed changes

cston merged commit 9fa4adf into dotnet:main Dec 16, 2021

ghost added this to the Next milestone Dec 16, 2021

cston deleted the 58328 branch December 16, 2021 04:54

cston modified the milestones: Next, 17.1 Dec 16, 2021

runfoapp bot mentioned this pull request Dec 16, 2021

[Flaky Test] CSharp.CSharpCodeActions.AddUsingExactMatchBeforeRenameTracking #57982

Closed

AlekseyTs reviewed Dec 20, 2021

View reviewed changes

cston added a commit to cston/roslyn that referenced this pull request Dec 20, 2021

Updates to PR dotnet#58339

70aef9d

cston mentioned this pull request Dec 20, 2021

Improve error reporting for 'this()' initializer from 'record struct' constructor #58430

Merged

cston mentioned this pull request Jan 22, 2022

Type confusion and OOB memory access in positional record structs #59010

Closed

cston added a commit to cston/roslyn that referenced this pull request Jan 25, 2022

Report error if 'record struct' constructor calls default parameterle…

4b104f8

…ss constructor (dotnet#58339)

cston mentioned this pull request Jan 25, 2022

[17.0] Report errors for structs with missing field initializers and implicit parameterless constructor #59055

Merged

cston added a commit to cston/roslyn that referenced this pull request Jan 25, 2022

Report error if 'record struct' constructor calls default parameterle…

5fd73c5

…ss constructor (dotnet#58339)

cston mentioned this pull request Feb 3, 2022

Calling a record struct constructor throws System.InvalidProgramException #59239

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report error if 'record struct' constructor calls default parameterless constructor #58339

Report error if 'record struct' constructor calls default parameterless constructor #58339

cston commented Dec 15, 2021 •

edited

Loading

cston Dec 15, 2021

Youssef1313 Dec 15, 2021 •

edited by cston

Loading

cston Dec 15, 2021

333fred left a comment

AlekseyTs Dec 20, 2021 •

edited

Loading

AlekseyTs Dec 20, 2021 •

edited

Loading

AlekseyTs Dec 20, 2021 •

edited

Loading

AlekseyTs Dec 20, 2021 •

edited

Loading

AlekseyTs Dec 20, 2021 •

edited

Loading

AlekseyTs commented Dec 20, 2021

cston commented Dec 20, 2021

Report error if 'record struct' constructor calls default parameterless constructor #58339

Report error if 'record struct' constructor calls default parameterless constructor #58339

Conversation

cston commented Dec 15, 2021 • edited Loading

cston Dec 15, 2021

Choose a reason for hiding this comment

Youssef1313 Dec 15, 2021 • edited by cston Loading

Choose a reason for hiding this comment

cston Dec 15, 2021

Choose a reason for hiding this comment

333fred left a comment

Choose a reason for hiding this comment

AlekseyTs Dec 20, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 20, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 20, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 20, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 20, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Dec 20, 2021

cston commented Dec 20, 2021

cston commented Dec 15, 2021 •

edited

Loading

Youssef1313 Dec 15, 2021 •

edited by cston

Loading

AlekseyTs Dec 20, 2021 •

edited

Loading

AlekseyTs Dec 20, 2021 •

edited

Loading

AlekseyTs Dec 20, 2021 •

edited

Loading

AlekseyTs Dec 20, 2021 •

edited

Loading

AlekseyTs Dec 20, 2021 •

edited

Loading