From d702fa672a155790023b8659bfec7ccd698203dc Mon Sep 17 00:00:00 2001 From: Julien Couvreur Date: Tue, 16 Jun 2020 21:12:55 -0700 Subject: [PATCH] Handle custom modifiers on copy ctor (#45136) --- .../Records/SynthesizedRecordCopyCtor.cs | 39 ++- .../Test/Semantic/Semantics/RecordTests.cs | 272 ++++++++++++++++++ .../Test/Utilities/CSharp/CSharpTestBase.cs | 2 +- 3 files changed, 305 insertions(+), 8 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordCopyCtor.cs b/src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordCopyCtor.cs index 9a5baaafe47..214c08a66b0 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordCopyCtor.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordCopyCtor.cs @@ -58,19 +58,46 @@ internal override void GenerateMethodBodyStatements(SyntheticBoundNodeFactory F, internal static MethodSymbol? FindCopyConstructor(NamedTypeSymbol containingType, NamedTypeSymbol within, ref HashSet useSiteDiagnostics) { - // We should handle ambiguities once we consider custom modifiers, as we do in overload resolution - // https://github.com/dotnet/roslyn/issues/45077 + MethodSymbol? bestCandidate = null; + int bestModifierCountSoFar = -1; // stays as -1 unless we hit an ambiguity foreach (var member in containingType.InstanceConstructors) { if (HasCopyConstructorSignature(member) && !member.HasUnsupportedMetadata && AccessCheck.IsSymbolAccessible(member, within, ref useSiteDiagnostics)) { - return member; + // If one has fewer custom modifiers, that is better + // (see OverloadResolution.BetterFunctionMember) + + if (bestCandidate is null && bestModifierCountSoFar < 0) + { + bestCandidate = member; + continue; + } + + if (bestModifierCountSoFar < 0) + { + bestModifierCountSoFar = bestCandidate.CustomModifierCount(); + } + + var memberModCount = member.CustomModifierCount(); + if (memberModCount > bestModifierCountSoFar) + { + continue; + } + + if (memberModCount == bestModifierCountSoFar) + { + bestCandidate = null; + continue; + } + + bestCandidate = member; + bestModifierCountSoFar = memberModCount; } } - return null; + return bestCandidate; } internal static bool IsCopyConstructor(Symbol member) @@ -86,10 +113,8 @@ internal static bool IsCopyConstructor(Symbol member) internal static bool HasCopyConstructorSignature(MethodSymbol member) { NamedTypeSymbol containingType = member.ContainingType; - // We should relax the comparison to AllIgnoreOptions, so that a copy constructor with a custom modifier is recognized - // 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) && method.Parameters[0].RefKind == RefKind.None; } } diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs index 842a083cee9..2e8baa329e8 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs @@ -4330,6 +4330,57 @@ .maxstack 2 }"); } + [Fact, WorkItem(44902, "https://github.com/dotnet/roslyn/issues/44902")] + public void CopyCtor_NotInRecordType() + { + var source = +@"public class C +{ + public object Property { get; set; } + public int field = 42; + + public C(C c) + { + } +} +public class D : C +{ + public int field2 = 43; + public D(D d) : base(d) + { + } +} +"; + var comp = CreateCompilation(source); + comp.VerifyDiagnostics(); + + var verifier = CompileAndVerify(comp); + verifier.VerifyIL("C..ctor(C)", @" +{ + // Code size 15 (0xf) + .maxstack 2 + IL_0000: ldarg.0 + IL_0001: ldc.i4.s 42 + IL_0003: stfld ""int C.field"" + IL_0008: ldarg.0 + IL_0009: call ""object..ctor()"" + IL_000e: ret +}"); + + verifier.VerifyIL("D..ctor(D)", @" +{ + // Code size 16 (0x10) + .maxstack 2 + IL_0000: ldarg.0 + IL_0001: ldc.i4.s 43 + IL_0003: stfld ""int D.field2"" + IL_0008: ldarg.0 + IL_0009: ldarg.1 + IL_000a: call ""C..ctor(C)"" + IL_000f: ret +}"); + } + [Fact, WorkItem(44902, "https://github.com/dotnet/roslyn/issues/44902")] public void CopyCtor_UserDefinedButDoesNotDelegateToBaseCopyCtor() { @@ -5111,6 +5162,227 @@ public void CopyCtor_InaccessibleInMetadata() ); } + [Fact, WorkItem(45077, "https://github.com/dotnet/roslyn/issues/45077")] + public void CopyCtor_AmbiguitiesInMetadata() + { + // IL for a minimal `public record B { }` with injected copy constructors + var ilSource_template = @" +.class public auto ansi beforefieldinit B extends [mscorlib]System.Object +{ + INJECT + + .method public hidebysig specialname newslot virtual instance class B '<>Clone' () cil managed + { + IL_0000: ldarg.0 + IL_0001: newobj instance void B::.ctor(class B) + IL_0006: ret + } + + .method public hidebysig specialname rtspecialname instance void .ctor () cil managed + { + IL_0000: ldarg.0 + IL_0001: call instance void [mscorlib]System.Object::.ctor() + IL_0006: ret + } +} +"; + + var source = @" +public record C : B +{ + public static void Main() + { + var c = new C(); + _ = c with { }; + } +}"; + + // We're going to inject various copy constructors into record B (at INJECT marker), and check which one is used + // by derived record C + // The RAN and THROW markers are shorthands for method bodies that print "RAN" and throw, respectively. + + // .ctor(B) vs. .ctor(modopt B) + verifyBoth(@" +.method public hidebysig specialname rtspecialname instance void .ctor ( class B '' ) cil managed +RAN +", +@" +.method public hidebysig specialname rtspecialname instance void .ctor ( class B modopt(int64) '' ) cil managed +THROW +"); + + // .ctor(modopt B) alone + verify(@" +.method public hidebysig specialname rtspecialname instance void .ctor ( class B modopt(int64) '' ) cil managed +RAN +"); + + // .ctor(B) vs. .ctor(modreq B) + verifyBoth(@" +.method public hidebysig specialname rtspecialname instance void .ctor ( class B '' ) cil managed +RAN +", +@" +.method public hidebysig specialname rtspecialname instance void .ctor ( class B modreq(int64) '' ) cil managed +THROW +"); + + // .ctor(modopt B) vs. .ctor(modreq B) + verifyBoth(@" +.method public hidebysig specialname rtspecialname instance void .ctor ( class B modopt(int64) '' ) cil managed +RAN +", +@" +.method public hidebysig specialname rtspecialname instance void .ctor ( class B modreq(int64) '' ) cil managed +THROW +"); + + // .ctor(B) vs. .ctor(modopt1 B) and .ctor(modopt2 B) + verifyBoth(@" +.method public hidebysig specialname rtspecialname instance void .ctor ( class B '' ) cil managed +RAN +", +@" +.method public hidebysig specialname rtspecialname instance void .ctor ( class B modopt(int64) '' ) cil managed +THROW + +.method public hidebysig specialname rtspecialname instance void .ctor ( class B modopt(int32) '' ) cil managed +THROW +"); + // .ctor(B) vs. .ctor(modopt1 B) and .ctor(modreq B) + verifyBoth(@" +.method public hidebysig specialname rtspecialname instance void .ctor ( class B '' ) cil managed +RAN +", +@" +.method public hidebysig specialname rtspecialname instance void .ctor ( class B modopt(int64) '' ) cil managed +THROW + +.method public hidebysig specialname rtspecialname instance void .ctor ( class B modreq(int32) '' ) cil managed +THROW +"); + + // .ctor(modeopt1 B) vs. .ctor(modopt2 B) + verifyBoth(@" +.method public hidebysig specialname rtspecialname instance void .ctor ( class B modopt(int64) '' ) cil managed +THROW +", +@" +.method public hidebysig specialname rtspecialname instance void .ctor ( class B modopt(int32) '' ) cil managed +THROW +", isError: true); + + // private .ctor(B) vs. .ctor(modopt1 B) and .ctor(modopt B) + verifyBoth(@" +.method private hidebysig specialname rtspecialname instance void .ctor ( class B '' ) cil managed +RAN +", +@" +.method public hidebysig specialname rtspecialname instance void .ctor ( class B modopt(int64) '' ) cil managed +THROW + +.method public hidebysig specialname rtspecialname instance void .ctor ( class B modopt(int32) '' ) cil managed +THROW +", isError: true); + + void verifyBoth(string inject1, string inject2, bool isError = false) + { + verify(inject1 + inject2, isError); + verify(inject2 + inject1, isError); + } + + void verify(string inject, bool isError = false) + { + var ranBody = @" +{ + IL_0000: ldstr ""RAN"" + IL_0005: call void [mscorlib]System.Console::WriteLine(string) + IL_000a: ret +} +"; + + var throwBody = @" +{ + IL_0000: ldnull + IL_0001: throw +} +"; + + var comp = CreateCompilationWithIL(new[] { source, IsExternalInitTypeDefinition }, + ilSource: ilSource_template.Replace("INJECT", inject).Replace("RAN", ranBody).Replace("THROW", throwBody), + parseOptions: TestOptions.RegularPreview, options: TestOptions.DebugExe); + + var expectedDiagnostics = isError ? new[] { + // (2,15): error CS8867: No accessible copy constructor found in base type 'B'. + // public record C : B + Diagnostic(ErrorCode.ERR_NoCopyConstructorInBaseType, "C").WithArguments("B").WithLocation(2, 15) + } : new DiagnosticDescription[] { }; + + comp.VerifyDiagnostics(expectedDiagnostics); + if (expectedDiagnostics is null) + { + CompileAndVerify(comp, expectedOutput: "RAN"); + } + } + } + + [Fact, WorkItem(45077, "https://github.com/dotnet/roslyn/issues/45077")] + public void CopyCtor_AmbiguitiesInMetadata_GenericType() + { + // IL for a minimal `public record B { }` with modopt in nested position of parameter type + var ilSource = @" +.class public auto ansi beforefieldinit B`1 extends [mscorlib]System.Object implements class [mscorlib]System.IEquatable`1> +{ + .method family hidebysig specialname rtspecialname instance void .ctor ( class B`1 '' ) cil managed + { + IL_0000: ldstr ""RAN"" + IL_0005: call void [mscorlib]System.Console::WriteLine(string) + IL_000a: ret + } + + .method public hidebysig specialname newslot virtual instance class B`1 '<>Clone' () cil managed + { + IL_0000: ldarg.0 + IL_0001: newobj instance void class B`1::.ctor(class B`1) + IL_0006: ret + } + + .method public hidebysig specialname rtspecialname instance void .ctor () cil managed + { + IL_0000: ldarg.0 + IL_0001: call instance void [mscorlib]System.Object::.ctor() + IL_0006: ret + } + + .method public newslot virtual instance bool Equals ( class B`1 '' ) cil managed + { + IL_0000: ldnull + IL_0001: throw + } +} +"; + + var source = @" +public record C : B { } + +public class Program +{ + public static void Main() + { + var c = new C(); + _ = c with { }; + } +}"; + + + var comp = CreateCompilationWithIL(new[] { source, IsExternalInitTypeDefinition }, + ilSource: ilSource, + parseOptions: TestOptions.RegularPreview, options: TestOptions.DebugExe); + + comp.VerifyDiagnostics(); + CompileAndVerify(comp, expectedOutput: "RAN"); + } + [Fact] public void Deconstruct_Simple() { diff --git a/src/Compilers/Test/Utilities/CSharp/CSharpTestBase.cs b/src/Compilers/Test/Utilities/CSharp/CSharpTestBase.cs index d7e219c75d6..fc6aec3e2f6 100644 --- a/src/Compilers/Test/Utilities/CSharp/CSharpTestBase.cs +++ b/src/Compilers/Test/Utilities/CSharp/CSharpTestBase.cs @@ -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 { } } -- GitLab