Disallow record with ambiguous constructors #49975

jcouv · 2020-12-15T05:12:30Z

Fixes #49628
Spec change dotnet/csharplang#4234

Youssef1313 · 2020-12-15T13:00:53Z

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

+        public void AmbigCtor()
+        {
+            var src = @"
+record R(R x);


From what I understand, this change is specific to positional records only.

Shouldn't the same error be issued for something like the following:

record R(string S) { public R(R x) {} }; ``` #Closed

Shouldn't the same error be issued for something like the following:

Is there a problem with this scenario?

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

Is there a problem with this scenario?

@AlekseyTs It's currently allowed in master.

The PR doesn't seem to disallow it.
So my question was: should this scenario (explicit declaration of a copy constructor) be disallowed? #Closed

So my question was: should this scenario (explicit declaration of a copy constructor) be disallowed?

Does this violate specification? What would be the reason to do this?

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

Does this violate specification?

On its current state, I think no.

What would be the reason to do this?

It's still ambiguous (if I'm not missing something). Also, if it will replace the synthesized copy constructor, probably with initializers won't work as expected in this case (because the <Clone>$ method depends on the copy constructor). So at minimum I'd expect either a compiler warning or an IDE suggestion (or a CA code quality rule) to warn for this. #Closed

It's still ambiguous.

With what?

(if I'm not missing something)

This PR is about detecting an ambiguity between a synthesized copy constructor and a primary constructor. This ambiguity for explicitly declared copy constructor is already detected, I think.

So at minimum I'd expect either a compiler warning or an IDE suggestion (or a CA code quality rule) to warn for this.

Explicit declaration of a copy constructor is intentionally supported/allowed (https://github.com/dotnet/csharplang/blob/master/proposals/csharp-9.0/records.md#copy-and-clone-members).

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

AlekseyTs · 2020-12-15T14:18:01Z

Fixes #49338

It looks like this PR is not related to the referenced issue. #Closed

AlekseyTs · 2020-12-15T14:19:17Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -6591,4 +6591,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
  <data name="WRN_UnreadRecordParameter_Title" xml:space="preserve">
    <value>Parameter is unread. Did you forget to use it to initialize the property with that name?</value>
  </data>
+  <data name="ERR_RecordAmbigCtor" xml:space="preserve">
+    <value>A record cannot be declared with a single positional parameter with the type of that record.</value>


with the type of that record [](start = 74, length = 28)

"of the same type as the record"? #Closed

I think it would be better to report more specific error. Some thing like: "A copy constructor cannot be synthesized for a record with the only positional parameter of the same type as the record."

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

"Synthesized copy constructor conflicts with a primary constructor." is even better, I think.

In reply to: 543408078 [](ancestors = 543408078,543383571)

AlekseyTs · 2020-12-15T14:24:23Z

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

+                if (ctor.ParameterCount == 1 && ctor.Parameters[0].Type.Equals(this))
+                {
+                    diagnostics.Add(ErrorCode.ERR_RecordAmbigCtor, ctor.Locations[0]);
+                    return null;


return null; [](start = 20, length = 12)

This doesn't feel right. I think we should suppress creation of synthesized copy constructor instead. The primary constructor is explicitly declared, we cannot just ignore the declaration even if it is erroneous. #Closed

If we suppress the primary constructor, then we'll also get a warning for unused parameter. I think it doesn't really matter which one we suppress. In my mind the copy constructor is always there, and in this case prevents the primary constructor from being declared. I don't see a problem.

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

If we suppress the primary constructor, then we'll also get a warning for unused parameter

I am not following. It looks like you are suppressing it.

In reply to: 543463726 [](ancestors = 543463726,543389753)

Whatever unexpected warning we get, should be suppressed, of course. But that should not come at the "expense" of symbols.

In my mind the copy constructor is always there, and in this case prevents the primary constructor from being declared. I don't see a problem.

I do. The primary constructor is clearly declared. So, there should be a corresponding symbol for it. I am also not suggesting to suppress copy constructor. All we need to do is to detect an ambiguity and report it.

In reply to: 543465253 [](ancestors = 543465253,543463726,543389753)

I am also not suggesting to suppress copy constructor.

To clarify. I did suggest this as an alternative at the beginning because that constructor is synthesized and I would be comfortable with not synthesizing something "illegal" as opposed to not creating a symbol for something explicitly declared in code. However, I would be fine with leaving that symbol along as well. We could get in a situation of both symbols present when copy constructor is declared explicitly, therefore, the pipeline should be able to operate reliably with both symbols present.

In reply to: 543470457 [](ancestors = 543470457,543465253,543463726,543389753)

AlekseyTs · 2020-12-15T14:27:20Z

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

-                members.Add(ctor);
-                return ctor;
+
+                if (ctor.ParameterCount == 1 && ctor.Parameters[0].Type.Equals(this))


Equals(this) [](start = 72, length = 12)

All aspects should be ignored here, I think. #Closed

We're already ignoring differences that can appear in source code (nullability). But I think you're right in terms of future-proofing. Thanks

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

AlekseyTs · 2020-12-15T14:34:49Z

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

+
+                if (ctor.ParameterCount == 1 && ctor.Parameters[0].Type.Equals(this))
+                {
+                    diagnostics.Add(ErrorCode.ERR_RecordAmbigCtor, ctor.Locations[0]);


diagnostics.Add(ErrorCode.ERR_RecordAmbigCtor, ctor.Locations[0]); [](start = 20, length = 66)

I am not sure if we should perform the check and report this error for a case when a copy constructor is declared explicitly, we already report an error in that scenario. #Closed

AlekseyTs · 2020-12-15T14:49:52Z

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

+record R(R x);
+
+#nullable enable
+record R2(R2? x) { }


What happens if this parameter is referenced in a field initializer. What information is going to be returned for that reference by SemanticModel? #Closed

AlekseyTs · 2020-12-15T14:53:31Z

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

@@ -229,6 +229,34 @@ class E
            comp.VerifyDiagnostics();
        }

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


AmbigCtor [](start = 20, length = 9)

We should also test a case when an ambiguous (against primary constructor) copy constructor is declared explicitly. #Closed

Note, prior to this change we already reported an expected error for a scenario like that:

record R(R y) { public R(R x) {} }

error CS0111: Type 'R' already defines a member called '.ctor' with the same parameter types

There is no need to change error reporting for a scenario like that.

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

AlekseyTs · 2020-12-15T14:54:44Z

Done with review pass (commit 1) #Closed

AlekseyTs · 2020-12-15T15:53:53Z

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

-                members.Add(ctor);
-                return ctor;
+
+                if (ctor.ParameterCount == 1 && ctor.Parameters[0].Type.Equals(this))


if (ctor.ParameterCount == 1 && ctor.Parameters[0].Type.Equals(this)) [](start = 16, length = 69)

Do we understand why a regular process of detecting ambiguous members doesn't detect the ambiguity? #Closed

Youssef1313 · 2020-12-15T19:57:01Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -6592,6 +6592,6 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
    <value>Parameter is unread. Did you forget to use it to initialize the property with that name?</value>
  </data>
  <data name="ERR_RecordAmbigCtor" xml:space="preserve">
-    <value>A record cannot be declared with a single positional parameter with the type of that record.</value>
+    <value>Synthesized copy constructor conflicts with a primary constructor</value>


Should end with a period? #Resolved

We're not very consistent on that, but sure

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

AlekseyTs · 2020-12-15T21:36:57Z

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

@@ -397,7 +397,7 @@ internal static bool IncludeFieldInitializersInBody(this MethodSymbol methodSymb
            return methodSymbol.IsConstructor()
                && !methodSymbol.HasThisConstructorInitializer()
                && !(methodSymbol is SynthesizedRecordCopyCtor) // A record copy constructor is special, regular initializers are not supposed to be executed by it.
-                && !Binder.IsUserDefinedRecordCopyConstructor(methodSymbol);
+                && (!Binder.IsUserDefinedRecordCopyConstructor(methodSymbol) || methodSymbol is SynthesizedRecordConstructor);


IsUserDefinedRecordCopyConstructor [](start = 28, length = 34)

I think IsUserDefinedRecordCopyConstructor should be adjusted to not confuse primary constructor with a copy constructor. #Closed

AlekseyTs · 2020-12-15T21:53:48Z

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

            if (!(paramList is null))
            {
                Debug.Assert(builder.RecordDeclarationWithParameters is object);
                Debug.Assert(builder.InstanceInitializersForRecordDeclarationWithParameters is object);

                var ctor = addCtor(builder.RecordDeclarationWithParameters);

-                if (ctor.ParameterCount != 0)
+                if (ctor is not null && ctor.ParameterCount != 0)


ctor is not null [](start = 20, length = 16)

This condition is always true. #Closed

AlekseyTs · 2020-12-15T21:55:09Z

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

                {
                    var existingOrAddedMembers = addProperties(ctor.Parameters);
                    addDeconstruct(ctor, existingOrAddedMembers);
                }
+                if (ctor.ParameterCount == 1 && ctor.Parameters[0].Type.Equals(this, TypeCompareKind.AllIgnoreOptions))


ctor.ParameterCount == 1 && ctor.Parameters[0].Type.Equals(this, TypeCompareKind.AllIgnoreOptions) [](start = 20, length = 98)

Consider simply assigning this value. #Closed

AlekseyTs · 2020-12-15T22:01:29Z

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

+                );
+
+            var r = comp.GlobalNamespace.GetTypeMember("R");
+            Assert.Equal(new[] { "R..ctor(R x)", "R..ctor(R x)", "R..ctor()" }, r.GetMembers(".ctor").ToTestDisplayStrings());


"R..ctor()" [](start = 65, length = 11)

A parameter-less constructor shouldn't be synthesized for this scenario. We should review all consumers of HasCopyConstructorSignature and make sure they properly deal with the ambiguity. #Closed

AlekseyTs · 2020-12-15T22:02:14Z

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

+                );
+
+            var r = comp.GlobalNamespace.GetTypeMember("R");
+            Assert.Equal(new[] { "R..ctor(R x)", "R..ctor(R original)", "R..ctor()" }, r.GetMembers(".ctor").ToTestDisplayStrings());


"R..ctor()" [](start = 72, length = 11)

A parameter-less constructor shouldn't be synthesized. #Closed

AlekseyTs · 2020-12-15T22:07:36Z

Done with review pass (commit 3) #Closed

jcouv · 2020-12-16T02:12:23Z

Thanks. Bad copy/paste. Fixed the link to #49628

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

AlekseyTs · 2020-12-16T15:25:02Z

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

+                // (8,33): error CS0121: The call is ambiguous between the following methods or properties: 'R.R(R)' and 'R.R(R)'
+                //     public Derived(Derived y) : base(y) => throw null; // 3, 4, 5
+                Diagnostic(ErrorCode.ERR_AmbigCall, "base").WithArguments("R.R(R)", "R.R(R)").WithLocation(8, 33),
+                // (8,33): error CS8868: A copy constructor in a record must call a copy constructor of the base, or a parameterless object constructor if the record inherits from object.


// (8,33): error CS8868: A copy constructor in a record must call a copy constructor of the base, or a parameterless object constructor if the record inherits from object. [](start = 16, length = 171)

Should we suppress this error, it feels like it is cascading?

AlekseyTs

LGTM (commit 4)

RikkiGibson · 2020-12-16T22:37:38Z

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

@@ -3540,7 +3549,8 @@ private void AddSynthesizedConstructorsIfNecessary(ArrayBuilder<Symbol> members,
                    {
                        case MethodKind.Constructor:
                            // Ignore the record copy constructor
-                            if (!IsRecord || !SynthesizedRecordCopyCtor.HasCopyConstructorSignature(method))
+                            if (!IsRecord ||
+                                !(SynthesizedRecordCopyCtor.HasCopyConstructorSignature(method) && method is not SynthesizedRecordConstructor))


Somehow, my brain just can't work out the exact purpose or meaning of this expression. When I apply De Morgan's to it (which I think makes it a little easier to understand) I get:

!IsRecord || !SynthesizedRecordCopyCtor.HasCopyConstructorSignature(method) || method is SynthesizedRecordConstructor

So we use a constructor as evidence to skip the implicit parameterless constructor if:

it's not contained in a record

it doesn't have the copy constructor signature, or

it is a synthesized record constructor.

I wasn't sure why (3) is necessary. If the record primary constructor gets skipped because it matches the copy constructor signature, isn't that fine, since it's an error scenario? #Resolved

If the record primary constructor gets skipped because it matches the copy constructor signature, isn't that fine, since it's an error scenario?

No, it is not fine. We know it is not a copy constructor. We know it is a constructor explicitly declared in code, therefore, its presence should suppress declaration of a parameter-less constructor.

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

RikkiGibson · 2020-12-16T22:39:19Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -6591,4 +6591,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
  <data name="WRN_UnreadRecordParameter_Title" xml:space="preserve">
    <value>Parameter is unread. Did you forget to use it to initialize the property with that name?</value>
  </data>
+  <data name="ERR_RecordAmbigCtor" xml:space="preserve">
+    <value>Synthesized copy constructor conflicts with a primary constructor.</value>


nit: consider wording the message as something like "Primary constructor is not allowed to use the copy constructor signature 'Rec(Rec)'", for example. 'Rec(Rec)' could be given by passing the copy constructor symbol as an argument to the diagnostic.

The primary constructor is the bit that needs to change, so should be the first to be mentioned, and showing the SymbolDisplay I think provides a bit of a hint as to what about the primary constructor needs to change. #Resolved

Makes sense to mention the primary constructor first. Adjusted

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

RikkiGibson

LGTM with some nits/suggestions

Disallow record with ambiguous constructors

17c6ad9

jcouv added Area-Compilers Feature - Records Records labels Dec 15, 2020

jcouv self-assigned this Dec 15, 2020

jcouv requested a review from a team as a code owner December 15, 2020 05:12

Youssef1313 reviewed Dec 15, 2020

View reviewed changes

AlekseyTs reviewed Dec 15, 2020

View reviewed changes

jcouv marked this pull request as draft December 15, 2020 15:45

AlekseyTs reviewed Dec 15, 2020

View reviewed changes

Address feedback

6fb8ea6

Youssef1313 reviewed Dec 15, 2020

View reviewed changes

period

5a81171

AlekseyTs reviewed Dec 15, 2020

View reviewed changes

Address feedback

467db66

jcouv marked this pull request as ready for review December 16, 2020 03:07

AlekseyTs reviewed Dec 16, 2020

View reviewed changes

AlekseyTs approved these changes Dec 16, 2020

View reviewed changes

RikkiGibson self-assigned this Dec 16, 2020

RikkiGibson reviewed Dec 16, 2020

View reviewed changes

RikkiGibson approved these changes Dec 16, 2020

View reviewed changes

jcouv added 3 commits December 17, 2020 14:32

tweak message

19e0d88

Merge remote-tracking branch 'dotnet/master' into ambig-ctor

58a0fa9

resolve conflict

73f0606

jcouv merged commit 68279b0 into dotnet:master Dec 18, 2020

ghost added this to the Next milestone Dec 18, 2020

jcouv deleted the ambig-ctor branch December 18, 2020 06:30

dibarbet modified the milestones: Next, 16.9.P3 Dec 19, 2020

Disallow record with ambiguous constructors #49975

Disallow record with ambiguous constructors #49975

Conversation

jcouv commented Dec 15, 2020 • edited Loading

Youssef1313 Dec 15, 2020 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Youssef1313 Dec 15, 2020 • edited by jcouv Loading

Choose a reason for hiding this comment

AlekseyTs Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Youssef1313 Dec 15, 2020 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Dec 15, 2020 • edited Loading

AlekseyTs Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Dec 15, 2020 • edited Loading

AlekseyTs Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Youssef1313 Dec 15, 2020 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Dec 15, 2020 • edited Loading

jcouv commented Dec 16, 2020

AlekseyTs Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

RikkiGibson Dec 16, 2020 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Dec 16, 2020 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson left a comment

Choose a reason for hiding this comment

jcouv commented Dec 15, 2020 •

edited

Loading

Youssef1313 Dec 15, 2020 •

edited by jcouv

Loading

Youssef1313 Dec 15, 2020 •

edited by jcouv

Loading

AlekseyTs Dec 15, 2020 •

edited

Loading

Youssef1313 Dec 15, 2020 •

edited by jcouv

Loading

AlekseyTs commented Dec 15, 2020 •

edited

Loading

AlekseyTs Dec 15, 2020 •

edited

Loading

AlekseyTs Dec 15, 2020 •

edited

Loading

AlekseyTs Dec 15, 2020 •

edited

Loading

AlekseyTs Dec 15, 2020 •

edited

Loading

AlekseyTs Dec 15, 2020 •

edited

Loading

AlekseyTs Dec 15, 2020 •

edited

Loading

AlekseyTs Dec 15, 2020 •

edited

Loading

AlekseyTs commented Dec 15, 2020 •

edited

Loading

AlekseyTs Dec 15, 2020 •

edited

Loading

Youssef1313 Dec 15, 2020 •

edited by jcouv

Loading

AlekseyTs Dec 15, 2020 •

edited

Loading

AlekseyTs Dec 15, 2020 •

edited

Loading

AlekseyTs Dec 15, 2020 •

edited

Loading

AlekseyTs Dec 15, 2020 •

edited

Loading

AlekseyTs Dec 15, 2020 •

edited

Loading

AlekseyTs commented Dec 15, 2020 •

edited

Loading

AlekseyTs Dec 16, 2020 •

edited

Loading

RikkiGibson Dec 16, 2020 •

edited by jcouv

Loading

RikkiGibson Dec 16, 2020 •

edited by jcouv

Loading