Handle custom modifiers on copy ctor #45136
Conversation
| 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) && |
There was a problem hiding this comment.
📝 this seems more correct, although doesn't affect behavior. #Resolved
There was a problem hiding this comment.
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)
| @@ -211,7 +211,7 @@ public sealed class NotNullIfNotNullAttribute : Attribute | |||
| protected const string IsExternalInitTypeDefinition = @" | |||
| namespace System.Runtime.CompilerServices | |||
| { | |||
| public sealed class IsExternalInit | |||
| public static class IsExternalInit | |||
There was a problem hiding this comment.
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
| return member; | ||
| // If one has fewer custom modifiers, that is better | ||
| // (see OverloadResolution.BetterFunctionMember) | ||
| var memberModCount = member.CustomModifierCount(); |
There was a problem hiding this comment.
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
| @@ -4662,6 +4713,166 @@ public void CopyCtor_InaccessibleInMetadata() | |||
| ); | |||
| } | |||
|
|
|||
| [Fact, WorkItem(45077, "https://github.com/dotnet/roslyn/issues/45077")] | |||
| public void CopyCtor_AmbiguitiesInMetadata() | |||
There was a problem hiding this comment.
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
| public void CopyCtor_AmbiguitiesInMetadata() | ||
| { | ||
| // IL for a minimal `public record B { }` with injected copy constructors | ||
| var ilSource_template = @" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
| } | ||
| "; | ||
|
|
||
| var source = @" |
There was a problem hiding this comment.
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
| @@ -3881,6 +3881,57 @@ .maxstack 2 | |||
| }"); | |||
| } | |||
|
|
|||
| [Fact, WorkItem(44902, "https://github.com/dotnet/roslyn/issues/44902")] | |||
| public void CopyCtor_NotInRecordType() | |||
There was a problem hiding this comment.
CopyCtor_NotInRecordType [](start = 20, length = 24)
What is this testing? It looks like the changes in the PR are specific to records. #Resolved
There was a problem hiding this comment.
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
| { | ||
| bestModifierCountSoFar = bestCandidate.CustomModifierCount(); | ||
| } | ||
|
|
||
| var memberModCount = member.CustomModifierCount(); | ||
| if (bestModifierCountSoFar >= 0 && memberModCount > bestModifierCountSoFar) |
There was a problem hiding this comment.
Isn't bestModifierCountSoFar >= 0 always true here?
| } | ||
|
|
||
| bestCandidate = member; | ||
| bestModifierCountSoFar = memberModCount; |
There was a problem hiding this comment.
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;
}There was a problem hiding this comment.
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.
| THROW | ||
| "); | ||
|
|
||
| // .ctor(modeopt1 B) vs. .ctor(modopt2 B) |
There was a problem hiding this comment.
modeopt [](start = 21, length = 7)
Typo
ghost
added this to the 
Fixes #44902