Allow public parameterless struct constructors declared in source by cston · Pull Request #51679 · dotnet/roslyn

cston · 2021-03-04T22:52:59Z

Support public parameterless struct constructors and declared in source, and support struct field initializers.

Proposal: parameterless-struct-constructors
Test plan: #51698

AlekseyTs · 2021-03-04T23:54:59Z

Allow parameterless struct constructors from source or metadata

I thought we already handled parameter-less struct constructors defined in metadata and made use of them. Could you please elaborate what behavior difference, if any, with regard to these constructors in this PR?

cston · 2021-03-05T00:13:19Z

I thought we already handled parameter-less struct constructors defined in metadata and made use of them.

You're right, the compiler was using public parameterless constructors from metadata.

But private parameterless constructors were ignored, so for a struct with a private parameterless constructor, the compiler would emit initobj when instantiating the struct rather than reporting an error, and type substitution of a type parameter with new() constraint was allowed.

There's a question of whether the compiler should recognize cases where a struct with inaccessible parameterless constructor is being instantiated directly, or substituted for a type parameter with new() constraint, and report an error or a warning for those cases? Or perhaps the compiler should warn, when a struct with non-public parameterless constructor is defined in source, that the constructor may be ignored.

In reply to: 791037969 [](ancestors = 791037969)

Youssef1313 · 2021-03-05T06:44:22Z

src/Compilers/CSharp/Portable/Symbols/ConstraintsHelper.cs

-        private static bool HasPublicParameterlessConstructor(NamedTypeSymbol type)
+        private static bool? HasPublicParameterlessConstructor(NamedTypeSymbol type)
        {
-            Debug.Assert(type.TypeKind == TypeKind.Class);


Instead of deleting the assertion, should it be updated to type.TypeKind is TypeKind.Class or TypeKind.Struct? #Resolved

…rameterless .ctors from metadata

jcouv · 2021-03-31T21:01:32Z

There are some legitimate test failures.

In reply to: 811462702

cston · 2021-05-11T20:19:10Z

src/Compilers/CSharp/Portable/Emitter/Model/AttributeDataAdapter.cs


        Cci.IMethodReference Cci.ICustomAttribute.Constructor(EmitContext context, bool reportDiagnostics)
        {
-            if (this.AttributeConstructor.IsDefaultValueTypeConstructor())


this.AttributeConstructor.IsDefaultValueTypeConstructor()

The change ensures we report ERR_NotAnAttributeClass even in the case of an explicit parameterless constructor. #Closed

Should we allow such attributes when the default struct constructor exists for real (ie. is emitted)?

…ruct-ctor

cston · 2021-05-21T17:26:25Z

@dotnet/roslyn-compiler, please review.

333fred · 2021-06-07T20:30:37Z

src/Compilers/CSharp/Portable/Symbols/NamedTypeSymbol.cs

+        /// <summary>
+        /// Returns true if the type is defined in source and contains field initializers.
+        /// </summary>
+        internal virtual bool HasFieldInitializers() => false;


Feels like this should follow our general pattern of being abstract here, rather than virtual. #Closed

For example, I don't see any changes to WrappedNameTypeSymbol, which I would expect to see.

The method is only valid when called on a definition. Added an assert.

Re-opened this for the question about abstract versus virtual.

Never mind. I'm slow on the uptake.

333fred · 2021-06-07T20:34:48Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_WellKnownAttributes.cs

+{
+    public static void M(in int i)
+    {
+        System.Console.WriteLine(i);


I'm not really certain what this is testing: is the constructor called at runtime or not? Consider putting some side-effecting thing in to inform. #Closed

We're testing the struct type can be used as an "attribute type" if the type has a public constructor. (The attribute type is not instantiated at runtime.) Added a comment to the test.

333fred · 2021-06-07T20:35:39Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleTest.cs

-"System.ITupleInternal.Size" },
-                         m1Tuple.MemberNames.ToArray());
+                "Item1",
+                "Item2",


Consider reverting the changes in this file. #Closed

333fred · 2021-06-07T20:39:49Z

src/Compilers/CSharp/Test/Emit/Emit/NoPiaEmbedTypes.cs

                    Assert.Same(f1, test9.GetMembers()[0]);
                    Assert.Same(f2, test9.GetMembers()[1]);
-                    Assert.True(((MethodSymbol)test9.GetMembers()[2]).IsDefaultValueTypeConstructor());
+                    Assert.True(((MethodSymbol)test9.GetMembers()[2]).IsDefaultValueTypeConstructor(requireZeroInit: false));


Why are these asserting false? Shouldn't existing code be true? #Closed

333fred · 2021-06-07T20:47:27Z

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

+            CompileAndVerify(sourceB, references: new[] { refA }, expectedOutput:
+$@"True
+True
+{ExecutionConditionUtil.IsCoreClr}"); // Desktop framework ignores constructor in Activator.CreateInstance<T>() where T : struct.


Desktop framework ignores constructor in Activator.CreateInstance() where T : struct.

Per LDM today, this isn't quite correct. #Closed

333fred · 2021-06-07T20:50:36Z

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

+{{
+    {accessibility} S() {{ }}
+}}";
+            var comp = CreateCompilation(sourceA, parseOptions: TestOptions.RegularPreview);


I thought this was supposed to be a compiler error? #Closed

This is an error but the error was not being verified since the focus of the test was using the resulting type. I've added VerifyDiagnostics() so the expected behavior is clear.

333fred · 2021-06-07T20:56:51Z

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

+}
+struct S1
+{
+    internal bool Initialized = false;


Initialized

Consider using a different type here. I can't tell whether new S1().Initialized ran the field initializer or not. #Closed

This is a general comment for all the ThisInitializer tests, I can't reason about the behavior of the existing code.

@cston this appears unaddressed.

333fred · 2021-06-07T21:04:10Z

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

+{
+    static void Main()
+    {
+        Console.WriteLine(new A<object>.S1().X);


S1

Consider verifying HasFieldInitializers() on these types. I'd expect that with the current implementation, we'd see false on the SubstitutedNestedNamedTypeSymbol invovled since there is no override of the default virtual base. #Closed

I'd expect that with the current implementation, we'd see false on the SubstitutedNestedNamedTypeSymbol ...

HasFieldInitializers() should only be called on a definition. Added an assert to that method to make it clear.

333fred · 2021-06-07T21:07:03Z

Done review pass (commit 17)

In reply to: 856257577

cston · 2021-07-08T00:43:21Z

@jcouv, @333fred, all feedback has been addressed, thanks.

…ruct-ctor-merge

333fred · 2021-07-08T20:49:00Z

Done review pass (commit 21).

333fred

LGTM (commit 22)

333fred

LGTM (commit 23)

jcouv · 2021-07-12T16:30:10Z

src/Compilers/CSharp/Portable/Symbols/ConstraintsHelper.cs

            switch (typeArgument.TypeKind)
            {
                case TypeKind.Struct:
+                    return HasPublicParameterlessConstructor((NamedTypeSymbol)typeArgument, synthesizedIfMissing: true);


Just curious, do you know why the class case has a type argument check, but the struct case doesn't need one? #Resolved

The typeArgument is the struct type and cannot be abstract.

jcouv · 2021-07-12T16:59:00Z

src/Compilers/CSharp/Portable/Lowering/ClosureConversion/ExpressionLambdaRewriter.cs

            if ((object)node.Constructor == null ||
                (node.Arguments.Length == 0 && !node.Type.IsStructType()) ||
-                node.Constructor.IsDefaultValueTypeConstructor())
+                node.Constructor.IsDefaultValueTypeConstructor(requireZeroInit: false))


Looks like expression trees scenarios are handled already.
Could you clarify the follow-up work item tracked in test plan? "Expression tree: synthesized parameterless constructor is not called in Expression e = () => new S();" #Closed

This call should have used requireZeroInit: true. Fixed and checked off work item in test plan.

jcouv

LGTM Thanks (iteration 25)

Invoke parameterless struct constructor if available

fdbeaeb

cston requested a review from a team as a code owner March 4, 2021 22:52

ghost added the Area-Compilers label Mar 4, 2021

Fix tests

1aa3850

cston force-pushed the struct-ctor branch from 8b079c0 to 1aa3850 Compare March 5, 2021 05:23

Youssef1313 mentioned this pull request Mar 5, 2021

Update cs0568.md dotnet/docs#23127

Merged

Youssef1313 reviewed Mar 5, 2021

View reviewed changes

cston added 2 commits March 5, 2021 08:16

PR feedback

2a95565

Add link to test plan

26df202

jcouv self-assigned this Mar 5, 2021

cston marked this pull request as draft March 10, 2021 00:52

cston force-pushed the struct-ctor branch 2 times, most recently from 2b1524b to f8e3948 Compare March 11, 2021 17:06

Initial changes for field initializers

b387800

cston force-pushed the struct-ctor branch from f8e3948 to b387800 Compare March 11, 2021 19:56

cston marked this pull request as ready for review March 11, 2021 19:57

cston added 2 commits March 11, 2021 16:08

Add test

2d4fe9e

Report error for non-public parameterless .ctor; ignore non-public pa…

d36a701

…rameterless .ctors from metadata

cston added 2 commits April 1, 2021 10:44

Fix tests

c93e78d

Synthesize .ctor if any field initializers and no explicit .ctors

421ff22

cston force-pushed the struct-ctor branch from 170f984 to 421ff22 Compare May 11, 2021 18:40

cston changed the title ~~Allow parameterless struct constructors from source or metadata~~ Allow public parameterless struct constructors declared in source May 11, 2021

cston commented May 11, 2021

View reviewed changes

cston added 3 commits May 11, 2021 16:17

Update tests

ccee188

Misc.

521e4c0

Merge remote-tracking branch 'upstream/features/struct-ctors' into st…

7c36e4a

…ruct-ctor

cston requested review from a team and 333fred June 1, 2021 16:07

333fred reviewed Jun 7, 2021

View reviewed changes

cston added 2 commits July 7, 2021 16:32

PR feedback

477f245

Misc.

f51586c

cston added 2 commits July 7, 2021 22:53

Merge remote-tracking branch 'upstream/features/struct-ctors' into st…

c032e53

…ruct-ctor-merge

Updates following merge

fe3cd11

cston force-pushed the struct-ctor branch from 39e57e2 to fe3cd11 Compare July 8, 2021 06:31

Fix comment

16a71ff

333fred approved these changes Jul 8, 2021

View reviewed changes

PR feedback

390f08e

333fred approved these changes Jul 8, 2021

View reviewed changes

jcouv self-requested a review July 10, 2021 04:54

jcouv reviewed Jul 12, 2021

View reviewed changes

cston added 2 commits July 12, 2021 12:01

PR feedback

c7ffae8

Update ExpressionTree case

5bdf7ce

jcouv approved these changes Jul 12, 2021

View reviewed changes

cston merged commit bbfeab5 into dotnet:features/struct-ctors Jul 12, 2021

cston deleted the struct-ctor branch July 12, 2021 21:22

Conversation

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

Uh oh!

Uh oh!

AlekseyTs commented Mar 4, 2021

Uh oh!

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

Uh oh!

Uh oh!

Youssef1313 Mar 5, 2021 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv commented Mar 31, 2021 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston May 11, 2021 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston commented May 21, 2021

Uh oh!

333fred Jun 7, 2021 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Jun 7, 2021 • 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 Jul 7, 2021 • 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!

333fred Jun 7, 2021 • 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!

333fred Jun 7, 2021 • 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!

333fred Jun 7, 2021 • 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!

333fred Jun 7, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

333fred Jun 7, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Jul 8, 2021 • 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!

333fred Jun 7, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

cston commented Mar 4, 2021 •

edited

Loading

cston commented Mar 5, 2021 •

edited

Loading

Youssef1313 Mar 5, 2021 •

edited by cston

Loading

jcouv commented Mar 31, 2021 •

edited by cston

Loading

cston May 11, 2021 •

edited by jcouv

Loading

333fred Jun 7, 2021 •

edited by jcouv

Loading

333fred Jun 7, 2021 •

edited

Loading

cston Jul 7, 2021 •

edited

Loading

333fred Jun 7, 2021 •

edited

Loading

333fred Jun 7, 2021 •

edited

Loading

333fred Jun 7, 2021 •

edited

Loading

333fred Jun 7, 2021 •

edited

Loading

333fred Jun 7, 2021 •

edited

Loading

cston Jul 8, 2021 •

edited

Loading

333fred Jun 7, 2021 •

edited

Loading

333fred commented Jun 7, 2021 •

edited

Loading

jcouv Jul 12, 2021 •

edited by cston

Loading

jcouv Jul 12, 2021 •

edited

Loading