Record copy ctor must invoke base copy ctor by jcouv · Pull Request #45022 · dotnet/roslyn

jcouv · 2020-06-10T03:04:48Z

Design notes:

don't run initializers in synthesized copy constructor
require that user-defined copy constructor delegate to base copy constructor
synthesized copy constructor delegates to base copy constructor, except for object
error if delegation fails during synthesis (member not found/accessible)
synthesized copy constructor should be protected

Corresponding spec change: dotnet/csharplang#3554

Fixes #44902 (delegate to base copy ctor)
Fixes #44897 (synthesized as protected)

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

src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordCopyCtor.cs

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

agocke · 2020-06-10T20:21:45Z

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


        #region Syntax

+        public bool IsRecord()


Why a method not a property?

I thought we were staying away from putting IsRecord on any symbol for now. #Resolved

The reason I used a method rather than a property is this method doesn't just return a simple value, or compute a value and return the cached result.
Yes, I'm also not trying to bite the bullet of adding a general IsRecord ;-)

I'll use the declaration check you suggested, but still trying not to add a general IsRecord...

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

agocke · 2020-06-10T20:22:01Z

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


+        public bool IsRecord()
+        {
+            foreach (var syntaxRef in this.SyntaxReferences)


I think you can just ask if declaration.Kind == DeclarationKind.Record #Resolved

agocke · 2020-06-10T20:27:16Z

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

+                    isExpanded = memberResolutionResult.Result.Kind == MemberResolutionKind.ApplicableInExpandedForm;
+                }
+
+                validateRecordCopyConstructor(containingType, constructor, baseType, resultMember, errorLocation, diagnostics);


I looks like we only need to run this in the non-synthesized case. #Resolved

Refactored and now synthesized cases don't come here at all.

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

agocke · 2020-06-10T20:28:40Z

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

+                    argsToParamsOpt = ImmutableArray.Create<int>(0);
+                    isExpanded = false;
+                    candidateConstructors = ImmutableArray<MethodSymbol>.Empty;
+                    analyzedArguments.Arguments.Add(new BoundParameter(constructor.GetNonNullSyntaxNode(), constructor.Parameters[0]));


How does this handle a record inheriting from object? #Resolved

We don't get here with object base type. That's handled by MethodCompiler.BindImplicitConstructorInitializer

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

AlekseyTs · 2020-06-11T01:39:12Z

I will take a look a bit later #Closed

AlekseyTs · 2020-06-11T01:56:25Z

src/Compilers/CSharp/Portable/CSharpResources.resx

+    <value>No accessible copy constructor found in base type '{0}'.</value>
+  </data>
+  <data name="ERR_CopyConstructorMustInvokeBaseCopyConstructor" xml:space="preserve">
+    <value>A copy constructor in a record type deriving from another type must invoke that base type's copy constructor '{0}'.</value>


type [](start = 69, length = 4)

record? #Closed

AlekseyTs · 2020-06-11T01:56:38Z

src/Compilers/CSharp/Portable/CSharpResources.resx

+    <value>No accessible copy constructor found in base type '{0}'.</value>
+  </data>
+  <data name="ERR_CopyConstructorMustInvokeBaseCopyConstructor" xml:space="preserve">
+    <value>A copy constructor in a record type deriving from another type must invoke that base type's copy constructor '{0}'.</value>


type [](start = 96, length = 4)

record? #Closed

AlekseyTs · 2020-06-11T02:06:30Z

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

+                MethodSymbol resultMember;
+                ImmutableArray<int> argsToParamsOpt;
+                bool isExpanded;
+                if (constructor is SynthesizedRecordCopyCtor)


if (constructor is SynthesizedRecordCopyCtor) [](start = 16, length = 45)

I think this code belongs in SynthesizedRecordCopyCtor.GenerateMethodBodyStatements. There is nothing to bind for that constructor, everything is synthesized, including the initializer. We also shouldn't call this function for SynthesizedRecordCopyCtor, which I think can easily be achieved by detecting it in MethodCompiler.BindImplicitConstructorInitializerIfAny. #Closed

Using MethodCompiler.GenerateBaseParameterlessConstructorInitializer as an example might help

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

I refactored the code out to a method similar to GenerateBaseParameterlessConstructorInitializer, but not into SynthesizedRecordCopyCtor.GenerateMethodBodyStatements.
I think that is more consistent with existing flow and overall was simpler. Please file an issue if you strongly think it should be refactored further.

In reply to: 438516676 [](ancestors = 438516676,438502981)

AlekseyTs · 2020-06-11T02:09:40Z

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

+
+                    if (found)
+                    {
+                        HashSet<DiagnosticInfo> ignoredUseSiteDiagnostics = null;


ignoredUseSiteDiagnostics [](start = 48, length = 25)

Why would we ignore use-site diagnostics here? #Closed

AlekseyTs · 2020-06-11T02:17:16Z

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

+                // Are we dealing with a user-defined copy constructor in a record type?
+                if (containingType is SourceNamedTypeSymbol sourceType &&
+                    sourceType.IsRecord() &&
+                    SynthesizedRecordCopyCtor.FindCopyConstructor(containingType).Equals(constructor, TypeCompareKind.ConsiderEverything))


SynthesizedRecordCopyCtor.FindCopyConstructor(containingType).Equals(constructor, TypeCompareKind.ConsiderEverything) [](start = 20, length = 117)

Why so complicated? Can't we simply check the signature of the constructor constructor at hand? #Closed

AlekseyTs · 2020-06-11T02:17:57Z

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

+                    sourceType.IsRecord() &&
+                    SynthesizedRecordCopyCtor.FindCopyConstructor(containingType).Equals(constructor, TypeCompareKind.ConsiderEverything))
+                {
+                    if (baseType.SpecialType == SpecialType.System_Object)


if (baseType.SpecialType == SpecialType.System_Object) [](start = 20, length = 54)

Why even call this function under this condition? #Closed

Simplifies the control flow of the calling method.

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

AlekseyTs · 2020-06-11T02:20:13Z

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

+                    var correctBaseCopyCtor = SynthesizedRecordCopyCtor.FindCopyConstructor(baseType);
+                    if (correctBaseCopyCtor is null)
+                    {
+                        // We'll have reported a problem with the base type of the record elsewhere


// We'll have reported a problem with the base type of the record elsewhere [](start = 24, length = 75)

The base can be declared in a different compilation. It looks like we might fail to report any error in this scenario (errors in a different compilation aren't good enough). #Closed

Thanks, indeed

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

AlekseyTs · 2020-06-11T02:21:23Z

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

+                    }
+
+                    // Unless the base type is 'object', the constructor should invoke the base type copy constructor
+                    var correctBaseCopyCtor = SynthesizedRecordCopyCtor.FindCopyConstructor(baseType);


var correctBaseCopyCtor = SynthesizedRecordCopyCtor.FindCopyConstructor(baseType); [](start = 19, length = 83)

Why so complicated? Can't we simply check the signature of the baseCtor at hand?
#Closed

AlekseyTs · 2020-06-11T02:25:40Z

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

            if (constructor.Initializer?.IsKind(SyntaxKind.ThisConstructorInitializer) != true &&
-                ContainingType.GetMembersUnordered().OfType<SynthesizedRecordConstructor>().Any())
+                ContainingType.GetMembersUnordered().OfType<SynthesizedRecordConstructor>().Any() &&
+                !SynthesizedRecordCopyCtor.FindCopyConstructor(ContainingType).Equals(this.ContainingMember(), TypeCompareKind.ConsiderEverything))


!SynthesizedRecordCopyCtor.FindCopyConstructor(ContainingType).Equals(this.ContainingMember(), TypeCompareKind.ConsiderEverything) [](start = 16, length = 130)

Why so complicated? Can't we simply check the signature of the containing method? #Closed

AlekseyTs · 2020-06-11T02:29:56Z

src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordCopyCtor.cs

            foreach (var field in ContainingType.GetFieldsToEmit())
            {
-                if (!field.IsStatic)
+                if (!field.IsStatic && field.ContainingType.Equals(ContainingType))


&& field.ContainingType.Equals(ContainingType) [](start = 35, length = 47)

Why is this check added, GetFieldsToEmit is not supposed to return fields that do not belong to the ContainingType. #Closed

AlekseyTs · 2020-06-11T02:33:35Z

src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordCopyCtor.cs

+                    method.Parameters[0].Type.Equals(containingType, TypeCompareKind.CLRSignatureCompareOptions) &&
+                    method.Parameters[0].RefKind == RefKind.None)
+                {
+                    return method;


return method; [](start = 20, length = 14)

What about ambiguity cases? Given that we are using this function to check if constructor is a copy constructor, we are likely to get false negatives from the checks. Please add tests for scenarios like that. #Closed

AlekseyTs · 2020-06-11T02:36:06Z

src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordCopyCtor.cs

+                    method.Parameters[0].Type.Equals(containingType, TypeCompareKind.CLRSignatureCompareOptions) &&
+                    method.Parameters[0].RefKind == RefKind.None)
+                {
+                    return method;


return method; [](start = 19, length = 15)

Also, it is quite possible to have two constructors in metadata different only by custom modifiers, I think we shouldn't just pick one of them arbitrarily and should report an error instead. Please add a test. #Closed

We might want to try to disambiguate based on the number of custom modifiers, as overload resolution does, I believe. However, there is still no guarantee that that would help in all cases.

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

RikkiGibson · 2020-06-11T19:12:29Z

src/Compilers/CSharp/Portable/CSharpResources.resx

  </data>
  <data name="ERR_CopyConstructorMustInvokeBaseCopyConstructor" xml:space="preserve">
-    <value>A copy constructor in a record type deriving from another type must invoke that base type's copy constructor '{0}'.</value>
+    <value>A copy constructor in a record type deriving from another record must invoke that base record's copy constructor '{0}'.</value>


This message still uses the term 'record type' #Resolved

This may be intentional 😄 #Resolved

cston · 2020-06-11T19:18:11Z

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

            { WasCompilerGenerated = true };
        }

+        internal static BoundCall GenerateBaseCopyConstructorInitializer(SynthesizedRecordCopyCtor constructor, DiagnosticBag diagnostics)


internal [](start = 8, length = 8)

private #Resolved

AlekseyTs · 2020-06-11T19:18:54Z

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

+                    // Unless the base type is 'object', the constructor should invoke a base type copy constructor
+                    HashSet<DiagnosticInfo> ignoredUseSiteDiagnostics = null;
+                    var foundBaseCopyCtor = SynthesizedRecordCopyCtor.FindCopyConstructor(baseType, containingType, ref ignoredUseSiteDiagnostics);
+                    if (foundBaseCopyCtor is null)


if (foundBaseCopyCtor is null) [](start = 20, length = 30)

I do not understand why are we performing this check at all. It really doesn't matter whether there is a copy constructor in the base or not if we are bound to something else. I think the diagnostics is actually going to be confusing. Rather than saying, you are using wrong thing, we say, something is missing in the base. Consider removing this logic. #Closed

I think we should always report ERR_CopyConstructorMustInvokeBaseCopyConstructor here, regardless of the situation.

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

You're suggesting removing the base copy constructor symbol from ERR_CopyConstructorMustInvokeBaseCopyConstructor, then?

In reply to: 439015232 [](ancestors = 439015232,439014453)

You're suggesting removing the base copy constructor symbol from ERR_CopyConstructorMustInvokeBaseCopyConstructor, then?

Yes, I think that would be fine.

In reply to: 439056805 [](ancestors = 439056805,439015232,439014453)

AlekseyTs · 2020-06-11T19:21:15Z

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

+                        // if there is no base type copy constructor, we'll report that instead
+                        diagnostics.Add(ErrorCode.ERR_NoCopyConstructorInBaseType, errorLocation, baseType);
+                    }
+                    else if (baseCtor is null || !SynthesizedRecordCopyCtor.HasCopyConstructorSignature(baseCtor))


baseCtor is null [](start = 29, length = 16)

I do not think this method should ever be called with null baseCtor #Closed

See earlier comment and tests such as CopyCtor_UserDefinedButDoesNotDelegateToBaseCopyCtor that benefit from this diagnostic.

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

cston · 2020-06-11T19:21:21Z

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

+        {
+            NamedTypeSymbol containingType = constructor.ContainingType;
+            NamedTypeSymbol baseType = containingType.BaseTypeNoUseSiteDiagnostics;
+            Location diagnosticsLocation = constructor.Locations.IsEmpty ? NoLocation.Singleton : constructor.Locations[0];


constructor.Locations.IsEmpty ? NoLocation.Singleton : constructor.Locations[0] [](start = 43, length = 79)

constructor.Locations.FirstOrNone() #Resolved

AlekseyTs · 2020-06-11T19:23:18Z

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

+                                    allowProtectedConstructorsOfBaseType: true);
+                MethodSymbol resultMember = memberResolutionResult.Member;
+
+                validateRecordCopyConstructor(containingType, constructor, baseType, resultMember, errorLocation, diagnostics);


validateRecordCopyConstructor(containingType, constructor, baseType, resultMember, errorLocation, diagnostics); [](start = 16, length = 111)

I don't think we should be doing this if overload resolution failed, it already complained in that case. We should verify only success case. #Closed

CopyCtor_UserDefinedButDoesNotDelegateToBaseCopyCtor_NoPositionalMembers illustrates why it is useful to check when overload resolution fails.
Diagnostic #1 alone is not user-friendly (does not hint what is actually the problem or how to fix it). Diagnostic #2 serves that purpose.

public void CopyCtor_UserDefinedButDoesNotDelegateToBaseCopyCtor() { var source = @"public record B(object N1, object N2) { } public record C(object P1, object P2) : B(0, 1) { public C(C c) // 1, 2 { } } "; var comp = CreateCompilation(source); comp.VerifyDiagnostics( // (6,12): error CS1729: 'B' does not contain a constructor that takes 0 arguments // public C(C c) // 1, 2 Diagnostic(ErrorCode.ERR_BadCtorArgCount, "C").WithArguments("B", "0").WithLocation(6, 12), // (6,12): error CS8868: A copy constructor in a record deriving from another record must invoke that base record's copy constructor 'B.B(B)'. // public C(C c) // 1, 2 Diagnostic(ErrorCode.ERR_CopyConstructorMustInvokeBaseCopyConstructor, "C").WithArguments("B.B(B)").WithLocation(6, 12) ); }

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

In my opinion it doesn't serve the purpose and the first error is sufficient and to the point. But I can live with this.

In reply to: 439077473 [](ancestors = 439077473,439016789)

AlekseyTs · 2020-06-11T19:29:49Z

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

+  .maxstack  2
+  IL_0000:  ldarg.0
+  IL_0001:  ldarg.1
+  IL_0002:  stfld      ""int C.<I>k__BackingField""


This looks wrong. We are trying to store value of type C into a field of type int. Why this test didn't fail? Success scenarios should be executed to ensure complete verification. BTW, it looks like we are including initializers into the copy constructor, which contradicts what this PR description is saying: "don't run initializers in synthesized copy constructor" #Closed

This is not a synthesized copy constructor, thus the field initializer is included. We're putting 0 into the backing field, which has type int. None of this was affected by the current PR, which only potentially adds a diagnostic on such user-defined copy constructors.

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

This is not a synthesized copy constructor, thus the field initializer is included. We're putting 0 into the backing field, which has type int. None of this was affected by the current PR, which only potentially adds a diagnostic on such user-defined copy constructors.

This is a violation of the language design. A copy constructor is not supposed to run initializers

In reply to: 439058957 [](ancestors = 439058957,439020094)

Okay. The notes I sent following the meeting were incorrect. Sent correction

In reply to: 439066375 [](ancestors = 439066375,439058957,439020094)

Consider adding similar test without primary ctor, but with an explicit field initializer.

In reply to: 439072053 [](ancestors = 439072053,439066375,439058957,439020094)

Consider adding similar test without primary ctor, but with an explicit field initializer.

This was regarding CopyCtor_UserDefinedButDoesNotDelegateToBaseCopyCtor_DerivesFromObject test

In reply to: 439103373 [](ancestors = 439103373,439072053,439066375,439058957,439020094)

AlekseyTs · 2020-06-11T19:37:54Z

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

+";
+            var comp = CreateCompilation(source);
+            comp.VerifyDiagnostics();
+            var verifier = CompileAndVerify(comp, verify: ExecutionConditionUtil.IsCoreClr ? Verification.Skipped : Verification.Fails);


var verifier = CompileAndVerify(comp, verify: ExecutionConditionUtil.IsCoreClr ? Verification.Skipped : Verification.Fails); [](start = 12, length = 124)

I would expect an error here. According to the spec, a copy constructor should allways delegate to base. #Closed

AlekseyTs · 2020-06-11T19:40:47Z

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

+  .maxstack  2
+  IL_0000:  ldarg.0
+  IL_0001:  ldarg.1
+  IL_0002:  stfld      ""int C.<I>k__BackingField""


IL_0002: stfld ""int C.k__BackingField"" [](start = 2, length = 49)

Same issue with initializers #Closed

AlekseyTs · 2020-06-11T19:42:25Z

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

+                // (4,24): error CS7036: There is no argument given that corresponds to the required formal parameter 'N2' of 'B.B(object, object)'
+                //     protected C(C c) : base(c) { } // 1, 2
+                Diagnostic(ErrorCode.ERR_NoCorrespondingArgument, "base").WithArguments("N2", "B.B(object, object)").WithLocation(4, 24),
+                // (4,24): error CS8867: No accessible copy constructor found in base type 'B'.


// (4,24): error CS8867: No accessible copy constructor found in base type 'B'. [](start = 16, length = 79)

I believe this is a noise. #Closed

AlekseyTs · 2020-06-11T19:44:49Z

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

+}";
+            var comp2 = CreateCompilationWithIL(new[] { source2, IsExternalInitTypeDefinition }, ilSource: ilSource, parseOptions: TestOptions.RegularPreview);
+            comp2.VerifyDiagnostics(
+                // (4,12): error CS8867: No accessible copy constructor found in base type 'B'.


// (4,12): error CS8867: No accessible copy constructor found in base type 'B'. [](start = 16, length = 79)

The error is misleading, we wouldn't try to bind to anything, but parameter-less constructor here. The presence or lack of the copy constructor makes no difference. #Closed

AlekseyTs · 2020-06-11T19:45:58Z

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

+";
+            var comp = CreateCompilation(source);
+            comp.VerifyDiagnostics(
+                // (7,22): error CS8867: No accessible copy constructor found in base type 'B'.


// (7,22): error CS8867: No accessible copy constructor found in base type 'B'. [](start = 16, length = 79)

The error is misleading #Closed

AlekseyTs · 2020-06-11T19:47:14Z

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

+                // (11,22): error CS0122: 'B.B(B)' is inaccessible due to its protection level
+                //     private D(D d) : base(d) { } // 2, 3
+                Diagnostic(ErrorCode.ERR_BadAccess, "base").WithArguments("B.B(B)").WithLocation(11, 22),
+                // (11,22): error CS8867: No accessible copy constructor found in base type 'B'.


// (11,22): error CS8867: No accessible copy constructor found in base type 'B'. [](start = 16, length = 80)

This is a noise. Previous error clearly describes what is wrong. #Closed

AlekseyTs · 2020-06-11T19:47:54Z

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

+                // (11,22): error CS8867: No accessible copy constructor found in base type 'B'.
+                //     private D(D d) : base(d) { } // 2, 3
+                Diagnostic(ErrorCode.ERR_NoCopyConstructorInBaseType, "base").WithArguments("B").WithLocation(11, 22),
+                // (13,15): error CS8867: No accessible copy constructor found in base type 'B'.


// (13,15): error CS8867: No accessible copy constructor found in base type 'B'. [](start = 16, length = 80)

The error is misleading. #Closed

AlekseyTs · 2020-06-11T19:53:25Z

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

+}
+public record C(object P1) : B(0, 1)
+{
+    public C(C c) // 1, 2


public C(C c) // 1, 2 [](start = 4, length = 21)

It doesn't look like there are any tests covering and verifying IL for successfully declared copy constructor in a derived record with field initializers and primary constructor. I suspect initializers are going to be emitted for this scenario too. #Closed

Primary ctor and explicit field initializers should at least be tested separately because presence of a primary constructor might hide some issues around explicit initializers.

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

Adding CopyCtor_UserDefined_WithFieldInitializers and CopyCtor_Synthesized_WithFieldInitializers

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

AlekseyTs · 2020-06-11T19:59:56Z

Done with review pass (iteration 7) #Closed

AlekseyTs · 2020-06-11T20:01:32Z

                                            !(methodSymbol is SynthesizedRecordCopyCtor); // A record copy constructor is special, regular initializers are not supposed to be executed by it.

I suspect we want to add similar check here for a user-defined copy constructor #Closed

Refers to: src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs:980 in 82562e5. [](commit_id = 82562e5, deletion_comment = False)

jcouv · 2020-06-11T20:55:45Z

                                            !(methodSymbol is SynthesizedRecordCopyCtor); // A record copy constructor is special, regular initializers are not supposed to be executed by it.

~~User-defined constructors are emitted as normal, including field initializers. ~~
I stand corrected. Thanks

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

Refers to: src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs:980 in 82562e5. [](commit_id = 82562e5, deletion_comment = False)

AlekseyTs · 2020-06-11T22:34:54Z

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

+                {
+                    if (baseType.SpecialType == SpecialType.System_Object)
+                    {
+                        if (resultMember.ContainingType.SpecialType != SpecialType.System_Object)


resultMember [](start = 28, length = 12)

Can this be null? #Closed

Thought about it then zoned out. Thanks for catching

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

AlekseyTs

LGTM (iteration 10), assuming CI is passing.

AlekseyTs · 2020-06-12T00:38:42Z

@jcouv It looks like there legitimate test failures - System.Exception : Verification succeeded unexpectedly

jcouv added Area-Compilers Feature - Records Records labels Jun 10, 2020

jcouv added this to the 16.7 milestone Jun 10, 2020

jcouv self-assigned this Jun 10, 2020

jcouv force-pushed the copy-ctor branch from e563260 to 6465b06 Compare June 10, 2020 04:33

jcouv changed the base branch from master to release/dev16.7-preview3 June 10, 2020 14:48

Record copy ctor must invoke base copy ctor

f70f641

jcouv force-pushed the copy-ctor branch from 6465b06 to f70f641 Compare June 10, 2020 14:52

jcouv marked this pull request as ready for review June 10, 2020 21:22

jcouv requested a review from a team as a code owner June 10, 2020 21:22

jcouv commented Jun 10, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs Outdated Show resolved Hide resolved

Fix formatting

2f7fb8f

RikkiGibson approved these changes Jun 10, 2020

View reviewed changes

Julien Couvreur added 2 commits June 10, 2020 15:38

Add accessibility check

23d2583

Address feedback

fdd1244

RikkiGibson approved these changes Jun 10, 2020

View reviewed changes

agocke reviewed Jun 11, 2020

View reviewed changes

AlekseyTs reviewed Jun 11, 2020

View reviewed changes

RikkiGibson reviewed Jun 11, 2020

View reviewed changes

cston reviewed Jun 11, 2020

View reviewed changes

AlekseyTs reviewed Jun 11, 2020

View reviewed changes

cston reviewed Jun 11, 2020

View reviewed changes

AlekseyTs reviewed Jun 11, 2020

View reviewed changes

Julien Couvreur added 2 commits June 11, 2020 15:24

Address feedback

352b9fd

One more test

88e4665

AlekseyTs reviewed Jun 11, 2020

View reviewed changes

Fix null ref and add test

c45d934

AlekseyTs approved these changes Jun 11, 2020

View reviewed changes

Forgot to update the other test file

6e604bb

RikkiGibson self-assigned this Jun 12, 2020

Fix test

b9869b2

jcouv force-pushed the copy-ctor branch from 80afaa3 to b9869b2 Compare June 12, 2020 00:54

jcouv merged commit 94c70eb into dotnet:release/dev16.7-preview3 Jun 12, 2020

jcouv deleted the copy-ctor branch June 12, 2020 02:11

Conversation

jcouv commented Jun 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

agocke Jun 10, 2020 • 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!

jcouv Jun 11, 2020 • 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!

agocke Jun 10, 2020 • 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!

agocke Jun 10, 2020 • 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!

agocke Jun 10, 2020 • 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!

AlekseyTs commented Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Jun 11, 2020 • 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 Jun 11, 2020 • 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 Jun 11, 2020 • 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!

AlekseyTs Jun 11, 2020 • 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 Jun 11, 2020 • 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 Jun 11, 2020 • 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!

AlekseyTs Jun 11, 2020 • 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!

AlekseyTs Jun 11, 2020 • 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 Jun 11, 2020 • 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!

jcouv commented Jun 10, 2020 •

edited

Loading

agocke Jun 10, 2020 •

edited by jcouv

Loading

jcouv Jun 11, 2020 •

edited

Loading

agocke Jun 10, 2020 •

edited by jcouv

Loading

agocke Jun 10, 2020 •

edited by jcouv

Loading

agocke Jun 10, 2020 •

edited by jcouv

Loading

AlekseyTs commented Jun 11, 2020 •

edited

Loading

AlekseyTs Jun 11, 2020 •

edited

Loading

AlekseyTs Jun 11, 2020 •

edited

Loading

AlekseyTs Jun 11, 2020 •

edited

Loading

AlekseyTs Jun 11, 2020 •

edited

Loading

AlekseyTs Jun 11, 2020 •

edited

Loading

AlekseyTs Jun 11, 2020 •

edited

Loading

AlekseyTs Jun 11, 2020 •

edited

Loading

AlekseyTs Jun 11, 2020 •

edited

Loading

AlekseyTs Jun 11, 2020 •

edited

Loading

AlekseyTs Jun 11, 2020 •

edited

Loading

AlekseyTs Jun 11, 2020 •

edited

Loading

AlekseyTs Jun 11, 2020 •

edited

Loading

RikkiGibson Jun 11, 2020 •

edited by jcouv

Loading

RikkiGibson Jun 11, 2020 •

edited by jcouv

Loading

cston Jun 11, 2020 •

edited by jcouv

Loading

AlekseyTs Jun 11, 2020 •

edited

Loading

AlekseyTs Jun 11, 2020 •

edited

Loading

cston Jun 11, 2020 •

edited by jcouv

Loading

AlekseyTs Jun 11, 2020 •

edited

Loading