Handle custom modifiers on copy ctor by jcouv · Pull Request #45136 · dotnet/roslyn

jcouv · 2020-06-13T05:35:25Z

jcouv · 2020-06-13T05:36:50Z

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

-            // https://github.com/dotnet/roslyn/issues/45077
            return member is MethodSymbol { IsStatic: false, ParameterCount: 1, Arity: 0 } method &&
-                method.Parameters[0].Type.Equals(containingType, TypeCompareKind.CLRSignatureCompareOptions) &&
+                method.Parameters[0].Type.Equals(containingType, TypeCompareKind.AllIgnoreOptions) &&


📝 this seems more correct, although doesn't affect behavior. #Resolved

although doesn't affect behavior.

Of course, comparison makes a difference. For example, when there is a custom modifier within the type.

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

jcouv · 2020-06-13T05:38:19Z

src/Compilers/Test/Utilities/CSharp/CSharpTestBase.cs

 namespace System.Runtime.CompilerServices
 {
-    public sealed class IsExternalInit
+    public static class IsExternalInit


Thanks @RikkiGibson for adding the BCL API. I see that you've changed the type to static there, to match other similar compiler types. So I'm updating in our tests to match. #Resolved

AlekseyTs · 2020-06-16T17:33:56Z

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

-                    return member;
+                    // If one has fewer custom modifiers, that is better
+                    // (see OverloadResolution.BetterFunctionMember)
+                    var memberModCount = member.CustomModifierCount();


var memberModCount = member.CustomModifierCount(); [](start = 20, length = 50)

Consider doing this only when we detected possible ambiguity, otherwise it is a wasted work to deal with a very uncommon case. #Resolved

AlekseyTs · 2020-06-16T17:49:26Z

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

        }

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


CopyCtor_AmbiguitiesInMetadata [](start = 20, length = 30)

It is impossible to follow what this test is doing. I believe clarity of tests is more important than brevity and extreme code reuse. #Closed

AlekseyTs · 2020-06-16T17:54:06Z

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

+        public void CopyCtor_AmbiguitiesInMetadata()
+        {
+            // IL for a minimal `public record B { }` with injected copy constructors
+            var ilSource_template = @"


ilSource_template [](start = 16, length = 17)

This constant is used only in one place. Consider inlining, I think this will make the test code easier to follow #WontFix

I think it's helpful to see what we're going to compile (even if contains holes) towards the start of the test. Added comment

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

AlekseyTs · 2020-06-16T17:56:23Z

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

+}
+";
+
+            var source = @"


source [](start = 16, length = 6)

This variable is used only in one place. Consider inlining, I think this will make the test code easier to follow #WontFix

AlekseyTs

LGTM (iteration 1), modulo the lack of test for the change around TypeCompareKind

cston · 2020-06-16T21:54:14Z

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

        }

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


CopyCtor_NotInRecordType [](start = 20, length = 24)

What is this testing? It looks like the changes in the PR are specific to records. #Resolved

This is not related to the current PR, but I realized that I didn't test that scenario in previous PR. A constructor that looks like a copy constructor, but isn't in a record, isn't affected by change to skip initializer portion (MethodCompiler). #Resolved

cston · 2020-06-16T23:54:45Z

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

+                    }
+
                    var memberModCount = member.CustomModifierCount();
                    if (bestModifierCountSoFar >= 0 && memberModCount > bestModifierCountSoFar)


Isn't bestModifierCountSoFar >= 0 always true here?

cston · 2020-06-16T23:59:18Z

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

+                    }
+
+                    bestCandidate = member;
+                    bestModifierCountSoFar = memberModCount;


bestModifierCountSoFar = memberModCount [](start = 20, length = 39)

bestModiferCountSoFar is only a short cut if there are 3 copy constructors which is pretty unlikely. Consider simplifying and calling CustomModiferCount() each time.

if (bestCandidate.CustomModifierCount() > member.CustomModifierCount()) { bestCandidate = member; }

In a scenario with 3 overload, the first two having the same modifier count, we need to remember what that modifier count was when we consider the last overload. We need that state.

cston · 2020-06-17T02:53:26Z

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

+THROW
+");
+
+            // .ctor(modeopt1 B) vs. .ctor(modopt2 B)


modeopt [](start = 21, length = 7)

Typo

Handle custom modifiers on copy ctor

0b8ec81

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

jcouv self-assigned this Jun 13, 2020

jcouv commented Jun 13, 2020

View reviewed changes

jcouv marked this pull request as ready for review June 15, 2020 05:52

jcouv requested a review from a team as a code owner June 15, 2020 05:52

AlekseyTs reviewed Jun 16, 2020

View reviewed changes

AlekseyTs approved these changes Jun 16, 2020

View reviewed changes

cston reviewed Jun 16, 2020

View reviewed changes

Julien Couvreur added 2 commits June 16, 2020 15:00

Merge remote-tracking branch 'dotnet/master' into custom-modifiers

1edb7d8

Address feedback

d4d1d04

cston reviewed Jun 16, 2020

View reviewed changes

Simplify

6a70abe

cston reviewed Jun 17, 2020

View reviewed changes

cston approved these changes Jun 17, 2020

View reviewed changes

jcouv merged commit d702fa6 into dotnet:master Jun 17, 2020

ghost added this to the Next milestone Jun 17, 2020

jcouv deleted the custom-modifiers branch June 17, 2020 04:13

jcouv mentioned this pull request Jun 24, 2020

Consider copy modifiers when checking record base initializer #45077

Closed

dibarbet modified the milestones: Next, 16.7.P4 Jun 30, 2020

Conversation

jcouv commented Jun 13, 2020

Uh oh!

jcouv Jun 13, 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 16, 2020

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 13, 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 16, 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!

AlekseyTs Jun 16, 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 16, 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 16, 2020

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 16, 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!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

cston Jun 16, 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 16, 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!

cston Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

cston Jun 16, 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 Jun 17, 2020

Choose a reason for hiding this comment

Uh oh!

cston Jun 17, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jcouv Jun 13, 2020 •

edited

Loading

jcouv Jun 13, 2020 •

edited

Loading

AlekseyTs Jun 16, 2020 •

edited by jcouv

Loading

AlekseyTs Jun 16, 2020 •

edited

Loading

AlekseyTs Jun 16, 2020 •

edited by jcouv

Loading

AlekseyTs Jun 16, 2020 •

edited by jcouv

Loading

cston Jun 16, 2020 •

edited by jcouv

Loading

jcouv Jun 16, 2020 •

edited

Loading

cston Jun 16, 2020 •

edited

Loading