Update implementation of required record members. by AlekseyTs · Pull Request #46053 · dotnet/roslyn

AlekseyTs · 2020-07-16T20:39:18Z

Closes #45296.
Closes #44903.
Fixes #45012.

Closes dotnet#45296. Closes dotnet#44903. Fixes dotnet#45012.

AlekseyTs · 2020-07-17T03:00:14Z

@cston, @jcouv, @RikkiGibson, @dotnet/roslyn-compiler Please review. #Closed

AlekseyTs · 2020-07-17T17:00:16Z

@cston, @jcouv, @RikkiGibson, @dotnet/roslyn-compiler Please review. #Closed

jcouv · 2020-07-17T18:00:31Z

Looking #Resolved

jcouv · 2020-07-17T21:11:07Z

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

+        }

-        internal override bool SynthesizesLoweredBoundBody => true;
+        protected override int GetParameterCountFromSyntax() => _ctor.ParameterCount;


GetParameterCountFromSyntax [](start = 31, length = 27)

This is strange. I don't think it makes sense to ask how many parameters the Deconstruct method has in syntax, given that it has no syntax.
I think we should override ParameterCount and throw in this method (unreachable) #WontFix

Why "won't fix"?

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

Why "won't fix"?

I find nothing strange in this name. It was not introduced in this PR. This is just an abstraction to give out the number of parameters without creating parameters themselves. Completely makes sense to me for any method. I can see that you might want this method to have a different name, but changing it is not in scope for this PR. Overriding ParameterCount would be the wrong thing to do, because we want it to do exactly what it does today, even for this method.

In reply to: 456689715 [](ancestors = 456689715,456676237)

jcouv · 2020-07-17T21:11:14Z

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


-        internal override ImmutableArray<string> GetAppliedConditionalSymbols()
-            => ImmutableArray<string>.Empty;
+            static bool modifiersAreValid(DeclarationModifiers modifiers)


This should probably be marked as conditional. Consider making it a conditional method or filing a follow-up issue (when attributes on local functions become available for use) #Resolved

This should probably be marked as conditional. Consider making it a conditional method or filing a follow-up issue (when attributes on local functions become available for use)

Added conditional compilation

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

jcouv · 2020-07-17T21:11:24Z

    => System.Console.Write(""RAN"");

Let's use throw null instead, since doesn't run #WontFix

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs:8739 in 11384c2. [](commit_id = 11384c2, deletion_comment = False)

jcouv · 2020-07-17T21:11:38Z

        var ilSource = @"

as far as I could tell, this is the same IL as Clone_05. Consider factoring tests or leaving comments to make differences visible. Same for Clone_07+Clone_08, and Clone_09+Clone_10 #WontFix

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs:1662 in 11384c2. [](commit_id = 11384c2, deletion_comment = False)

jcouv · 2020-07-17T21:11:49Z

            "C C." + WellKnownMemberNames.CloneMethodName + "()",

What caused this change in ordering? #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs:5666 in 11384c2. [](commit_id = 11384c2, deletion_comment = False)

jcouv · 2020-07-17T21:11:55Z

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

            if (this.IsGenericType && !localBase.IsErrorType() && this.DeclaringCompilation.IsAttributeType(localBase))
            {
-                var baseLocation = FindBaseRefSyntax(localBase);
+                baseLocation ??= FindBaseRefSyntax(localBase);


??= [](start = 29, length = 3)

nit: = seemed fined. Why change? #Closed

nit: = seemed fined. Why change?

To make it safe to add something above the if later

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

jcouv · 2020-07-17T21:12:07Z

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

-                if (!memberSignatures.ContainsKey(ctor))
-                {
-                    members.Add(ctor);
+                    if (deconstruct.ReturnType.SpecialType != SpecialType.System_Void && !deconstruct.ReturnType.IsErrorType())


&& !deconstruct.ReturnType.IsErrorType() [](start = 85, length = 41)

nit: if Deconstruct returns an error type, then clearly it's not returning void. Feels like this check could be removed. #Closed

Feels like this check could be removed.

I do not see any harm in keeping it, find it better to follow the pattern and not relying on any special treatment/analysis around special types.

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

jcouv · 2020-07-17T21:12:14Z

    public void Clone_17()

nit: meaningful test names or comments would greatly help figuring out what tests are about (what is the intent) #WontFix

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs:2484 in 11384c2. [](commit_id = 11384c2, deletion_comment = False)

jcouv · 2020-07-17T21:12:18Z

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

        }

-        internal static MethodSymbol? FindValidCloneMethod(NamedTypeSymbol containingType)
+        internal static MethodSymbol? FindValidCloneMethod(TypeSymbol containingType, ref HashSet<DiagnosticInfo>? useSiteDiagnostics)


useSiteDiagnostics [](start = 115, length = 18)

nit: consider commenting or renaming (baseUseSiteDiagnostics) to clarify they are for the base type, not for the clone method. Could also be renamed in calling code #Closed

consider commenting or renaming (baseUseSiteDiagnostics) to clarify they are for the base type, not for the clone method. Could also be renamed in calling code

I do not believe we want play games like that with names. It is intentionally doesn't matter where the errors are coming from, the point is that they should be reported at the use-site and that is exactly what the name is reflecting. We consistently use this name throughout the code base and intentionally do not try to specify the source/reason for the errors.

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

jcouv · 2020-07-17T21:12:35Z

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

+                    candidate.ReturnType,
+                    TypeCompareKind.AllIgnoreOptions,
+                    ref useSiteDiagnostics))
+            {


not blocking this PR: should we check that the return value isn't nullable? (updated the note in test plan with clone method) Also added a note for nullability in user-provided Deconstruct method. #Closed

should we check that the return value isn't nullable? (updated the note in test plan with clone method) Also added a note for nullability in user-provided Deconstruct method.

I do not believe nullable annotations should matter for the purpose of this method , they do not affect semantics.

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

jcouv

Done with review pass (iteration 3)

AlekseyTs · 2020-07-17T21:30:10Z

            "C C." + WellKnownMemberNames.CloneMethodName + "()",

What caused this change in ordering?

The order got assigned explicitly based on when this member is actually synthesized

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

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs:5666 in 11384c2. [](commit_id = 11384c2, deletion_comment = False)

AlekseyTs · 2020-07-17T21:33:17Z

        var ilSource = @"

as far as I could tell, this is the same IL as Clone_05

Clone_05 tests inheritance, this test tests with expression.

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

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs:1662 in 11384c2. [](commit_id = 11384c2, deletion_comment = False)

jcouv · 2020-07-17T21:56:25Z

            "C C." + WellKnownMemberNames.CloneMethodName + "()",

Sorry, I didn't follow. Looking at the logic in SourceMemberContainerSymbol, I don't see how we changed the order of members getting added.

In reply to: 660345586 [](ancestors = 660345586,660340192)

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs:5666 in 11384c2. [](commit_id = 11384c2, deletion_comment = False)

jcouv

LGTM Thanks (iteration 3). I re-opened two questions, although I don't want to block the PR for that.

AlekseyTs · 2020-07-17T22:00:47Z

            "C C." + WellKnownMemberNames.CloneMethodName + "()",

Sorry, I didn't follow. Looking at the logic in SourceMemberContainerSymbol, I don't see how we changed the order of members getting added.

Note memberOffset parameter:
members.Add(new SynthesizedRecordClone(this, memberOffset: members.Count, diagnostics));

In reply to: 660353978 [](ancestors = 660353978,660345586,660340192)

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs:5666 in 11384c2. [](commit_id = 11384c2, deletion_comment = False)

AlekseyTs · 2020-07-17T22:03:32Z

@cston, @RikkiGibson, @dotnet/roslyn-compiler Please review, need a second sign-off. #Closed

cston · 2020-07-18T18:01:33Z

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

                return;
            }

+            Location baseLocation = null;


baseLocation [](start = 21, length = 12)

It looks like all code paths assign baseLocation, and always to the same value. Why not assign here? #Closed

cston · 2020-07-18T18:14:25Z

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

+        {
+            HashSet<DiagnosticInfo>? ignoredUseSiteDiagnostics = null; // This is reported when we bind bases
+            NamedTypeSymbol baseType = ContainingType.BaseTypeNoUseSiteDiagnostics;
+            return (ReturnType: !baseType.IsObjectType() && FindValidCloneMethod(baseType, ref ignoredUseSiteDiagnostics) is { } baseClone ?


!baseType.IsObjectType() && FindValidCloneMethod(baseType, ref ignoredUseSiteDiagnostics) is { } baseClone [](start = 32, length = 106)

Consider extracting a helper method for use here and in MakeDeclarationModifiers(). It looks like the helper method would also encapsulate ignoredUseSiteDiagnostics and baseType locals. #Closed

AlekseyTs · 2020-07-19T05:34:16Z

@cston I think I responded to your feedback.

cston · 2020-07-20T19:00:41Z

        var verifier = CompileAndVerify(comp, expectedOutput: "(, )").VerifyDiagnostics();

Already called above. #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs:5517 in 8859145. [](commit_id = 8859145, deletion_comment = False)

cston · 2020-07-20T19:02:47Z

(1, 2)").VerifyDiagnostics();

Moving VerifyDiagnostics() seems unnecessary, here and other tests below. #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs:6163 in 8859145. [](commit_id = 8859145, deletion_comment = False)

AlekseyTs · 2020-07-20T19:14:30Z

(1, 2)").VerifyDiagnostics();

Moving VerifyDiagnostics() seems unnecessary, here and other tests below.

It is more efficient because it checks against diagnostics that verifier collects anyway, and it also covers any diagnostics reported by emit phase, which regular VerifyDiagnostics doesn't cover.

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

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs:6163 in 8859145. [](commit_id = 8859145, deletion_comment = False)

AlekseyTs · 2020-07-20T19:15:30Z

        var verifier = CompileAndVerify(comp, expectedOutput: "(, )").VerifyDiagnostics();

Already called above.

They are not equivalent, but you are correct the previous call can be removed.

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

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs:5517 in 8859145. [](commit_id = 8859145, deletion_comment = False)

Update implementation of required record members.

11384c2

Closes dotnet#45296. Closes dotnet#44903. Fixes dotnet#45012.

AlekseyTs added Area-Compilers Feature - Records Records labels Jul 16, 2020

AlekseyTs requested review from a team, RikkiGibson, cston and jcouv July 16, 2020 20:39

AlekseyTs added 2 commits July 17, 2020 11:08

Merge branch 'master' into Records_19

ee9cc5f

Fix bad merge

c2b47d8

jcouv reviewed Jul 17, 2020

View reviewed changes

jcouv approved these changes Jul 17, 2020

View reviewed changes

Conditional local function

b3239fb

cston reviewed Jul 18, 2020

View reviewed changes

PR feedback

1245ed7

Merge branch 'master' into Records_19

8859145

cston approved these changes Jul 20, 2020

View reviewed changes

AlekseyTs merged commit 1e511fa into dotnet:master Jul 20, 2020

ghost added this to the Next milestone Jul 20, 2020

JoeRobich modified the milestones: Next, 16.8.P1 Jul 20, 2020

Conversation

AlekseyTs commented Jul 16, 2020

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

jcouv commented Jul 17, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Jul 17, 2020 • edited by AlekseyTs 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 Jul 17, 2020

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jul 17, 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 Jul 17, 2020 • edited by AlekseyTs 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 Jul 17, 2020

Choose a reason for hiding this comment

Uh oh!

jcouv commented Jul 17, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Jul 17, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Jul 17, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Jul 17, 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 Jul 17, 2020

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 17, 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 Jul 17, 2020

Choose a reason for hiding this comment

Uh oh!

jcouv commented Jul 17, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Jul 17, 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 Jul 17, 2020

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 17, 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 Jul 17, 2020

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jul 17, 2020

Uh oh!

AlekseyTs commented Jul 17, 2020

Uh oh!

jcouv commented Jul 17, 2020

Uh oh!

jcouv left a comment

AlekseyTs commented Jul 17, 2020 •

edited

Loading

AlekseyTs commented Jul 17, 2020 •

edited

Loading

jcouv commented Jul 17, 2020 •

edited by AlekseyTs

Loading

jcouv Jul 17, 2020 •

edited by AlekseyTs

Loading

AlekseyTs Jul 17, 2020 •

edited

Loading

jcouv Jul 17, 2020 •

edited by AlekseyTs

Loading

jcouv commented Jul 17, 2020 •

edited by AlekseyTs

Loading

jcouv commented Jul 17, 2020 •

edited by AlekseyTs

Loading

jcouv commented Jul 17, 2020 •

edited by AlekseyTs

Loading

jcouv Jul 17, 2020 •

edited

Loading

jcouv Jul 17, 2020 •

edited

Loading

jcouv commented Jul 17, 2020 •

edited by AlekseyTs

Loading

jcouv Jul 17, 2020 •

edited

Loading

jcouv Jul 17, 2020 •

edited

Loading

AlekseyTs commented Jul 17, 2020 •

edited

Loading

cston Jul 18, 2020 •

edited

Loading

cston Jul 18, 2020 •

edited

Loading

cston commented Jul 20, 2020 •

edited by AlekseyTs

Loading

cston commented Jul 20, 2020 •

edited by AlekseyTs

Loading