From 1f6905e52623429697a775ce469f237b98e0782e Mon Sep 17 00:00:00 2001 From: Dominique Schuppli Date: Wed, 28 Dec 2022 22:54:11 +0100 Subject: [PATCH 1/9] Add passing test for proxying empty generic `record` type --- .../Records/EmptyGenericRecord.cs | 20 +++++++++++++++++++ .../DynamicProxy.Tests/RecordsTestCase.cs | 6 ++++++ 2 files changed, 26 insertions(+) create mode 100644 src/Castle.Core.Tests/DynamicProxy.Tests/Records/EmptyGenericRecord.cs diff --git a/src/Castle.Core.Tests/DynamicProxy.Tests/Records/EmptyGenericRecord.cs b/src/Castle.Core.Tests/DynamicProxy.Tests/Records/EmptyGenericRecord.cs new file mode 100644 index 0000000000..7e16306f85 --- /dev/null +++ b/src/Castle.Core.Tests/DynamicProxy.Tests/Records/EmptyGenericRecord.cs @@ -0,0 +1,20 @@ +// Copyright 2004-2022 Castle Project - http://www.castleproject.org/ +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +namespace Castle.DynamicProxy.Tests.Records +{ + public record EmptyGenericRecord + { + } +} diff --git a/src/Castle.Core.Tests/DynamicProxy.Tests/RecordsTestCase.cs b/src/Castle.Core.Tests/DynamicProxy.Tests/RecordsTestCase.cs index f755dff4f7..c4ccccec0c 100644 --- a/src/Castle.Core.Tests/DynamicProxy.Tests/RecordsTestCase.cs +++ b/src/Castle.Core.Tests/DynamicProxy.Tests/RecordsTestCase.cs @@ -32,5 +32,11 @@ public void Can_proxy_derived_empty_record() { _ = generator.CreateClassProxy(new StandardInterceptor()); } + + [Test] + public void Can_proxy_empty_generic_record() + { + _ = generator.CreateClassProxy>(new StandardInterceptor()); + } } } From 2d944e40df820d0c89b107ad8368095b7e2b8dc8 Mon Sep 17 00:00:00 2001 From: Dominique Schuppli Date: Wed, 28 Dec 2022 22:55:34 +0100 Subject: [PATCH 2/9] Add failing test for proxying type derived from generic `record` type --- .../Records/DerivedEmptyGenericRecord.cs | 20 +++++++++++++++++++ .../DynamicProxy.Tests/RecordsTestCase.cs | 6 ++++++ 2 files changed, 26 insertions(+) create mode 100644 src/Castle.Core.Tests/DynamicProxy.Tests/Records/DerivedEmptyGenericRecord.cs diff --git a/src/Castle.Core.Tests/DynamicProxy.Tests/Records/DerivedEmptyGenericRecord.cs b/src/Castle.Core.Tests/DynamicProxy.Tests/Records/DerivedEmptyGenericRecord.cs new file mode 100644 index 0000000000..c3c685e8d2 --- /dev/null +++ b/src/Castle.Core.Tests/DynamicProxy.Tests/Records/DerivedEmptyGenericRecord.cs @@ -0,0 +1,20 @@ +// Copyright 2004-2022 Castle Project - http://www.castleproject.org/ +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +namespace Castle.DynamicProxy.Tests.Records +{ + public record DerivedEmptyGenericRecord : EmptyGenericRecord + { + } +} diff --git a/src/Castle.Core.Tests/DynamicProxy.Tests/RecordsTestCase.cs b/src/Castle.Core.Tests/DynamicProxy.Tests/RecordsTestCase.cs index c4ccccec0c..dff33aeaaa 100644 --- a/src/Castle.Core.Tests/DynamicProxy.Tests/RecordsTestCase.cs +++ b/src/Castle.Core.Tests/DynamicProxy.Tests/RecordsTestCase.cs @@ -38,5 +38,11 @@ public void Can_proxy_empty_generic_record() { _ = generator.CreateClassProxy>(new StandardInterceptor()); } + + [Test] + public void Can_proxy_derived_empty_generic_record() + { + _ = generator.CreateClassProxy(new StandardInterceptor()); + } } } From 06884f40ff85608d7fa2b477ef1f06afaa81ff0c Mon Sep 17 00:00:00 2001 From: Dominique Schuppli Date: Wed, 28 Dec 2022 22:59:15 +0100 Subject: [PATCH 3/9] Detail: adjust names of derived `record` types (The type name `DerivedEmptyGenericRecord` suggests that it is generic, which is not the case; `DerivedFromEmptyGenericRecord` is more accurate. For consistency, let's apply the same name change to `EmptyRecord` and its subtype.) --- ...yGenericRecord.cs => DerivedFromEmptyGenericRecord.cs} | 2 +- .../{DerivedEmptyRecord.cs => DerivedFromEmptyRecord.cs} | 2 +- .../DynamicProxy.Tests/RecordsTestCase.cs | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-) rename src/Castle.Core.Tests/DynamicProxy.Tests/Records/{DerivedEmptyGenericRecord.cs => DerivedFromEmptyGenericRecord.cs} (90%) rename src/Castle.Core.Tests/DynamicProxy.Tests/Records/{DerivedEmptyRecord.cs => DerivedFromEmptyRecord.cs} (92%) diff --git a/src/Castle.Core.Tests/DynamicProxy.Tests/Records/DerivedEmptyGenericRecord.cs b/src/Castle.Core.Tests/DynamicProxy.Tests/Records/DerivedFromEmptyGenericRecord.cs similarity index 90% rename from src/Castle.Core.Tests/DynamicProxy.Tests/Records/DerivedEmptyGenericRecord.cs rename to src/Castle.Core.Tests/DynamicProxy.Tests/Records/DerivedFromEmptyGenericRecord.cs index c3c685e8d2..78a687b4c6 100644 --- a/src/Castle.Core.Tests/DynamicProxy.Tests/Records/DerivedEmptyGenericRecord.cs +++ b/src/Castle.Core.Tests/DynamicProxy.Tests/Records/DerivedFromEmptyGenericRecord.cs @@ -14,7 +14,7 @@ namespace Castle.DynamicProxy.Tests.Records { - public record DerivedEmptyGenericRecord : EmptyGenericRecord + public record DerivedFromEmptyGenericRecord : EmptyGenericRecord { } } diff --git a/src/Castle.Core.Tests/DynamicProxy.Tests/Records/DerivedEmptyRecord.cs b/src/Castle.Core.Tests/DynamicProxy.Tests/Records/DerivedFromEmptyRecord.cs similarity index 92% rename from src/Castle.Core.Tests/DynamicProxy.Tests/Records/DerivedEmptyRecord.cs rename to src/Castle.Core.Tests/DynamicProxy.Tests/Records/DerivedFromEmptyRecord.cs index 518163f8be..dd2de54dc5 100644 --- a/src/Castle.Core.Tests/DynamicProxy.Tests/Records/DerivedEmptyRecord.cs +++ b/src/Castle.Core.Tests/DynamicProxy.Tests/Records/DerivedFromEmptyRecord.cs @@ -14,7 +14,7 @@ namespace Castle.DynamicProxy.Tests.Records { - public record DerivedEmptyRecord : EmptyRecord + public record DerivedFromEmptyRecord : EmptyRecord { } } diff --git a/src/Castle.Core.Tests/DynamicProxy.Tests/RecordsTestCase.cs b/src/Castle.Core.Tests/DynamicProxy.Tests/RecordsTestCase.cs index dff33aeaaa..77302a88e1 100644 --- a/src/Castle.Core.Tests/DynamicProxy.Tests/RecordsTestCase.cs +++ b/src/Castle.Core.Tests/DynamicProxy.Tests/RecordsTestCase.cs @@ -28,9 +28,9 @@ public void Can_proxy_empty_record() } [Test] - public void Can_proxy_derived_empty_record() + public void Can_proxy_record_derived_from_empty_record() { - _ = generator.CreateClassProxy(new StandardInterceptor()); + _ = generator.CreateClassProxy(new StandardInterceptor()); } [Test] @@ -40,9 +40,9 @@ public void Can_proxy_empty_generic_record() } [Test] - public void Can_proxy_derived_empty_generic_record() + public void Can_proxy_record_derived_from_empty_generic_record() { - _ = generator.CreateClassProxy(new StandardInterceptor()); + _ = generator.CreateClassProxy(new StandardInterceptor()); } } } From 6238c15cfde15bbf18130f8eccc60e12823e2d1e Mon Sep 17 00:00:00 2001 From: Dominique Schuppli Date: Wed, 28 Dec 2022 23:18:16 +0100 Subject: [PATCH 4/9] Also check for covariant return types if one of them is generic --- .../Generators/MethodSignatureComparer.cs | 27 +++++++++++-------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/src/Castle.Core/DynamicProxy/Generators/MethodSignatureComparer.cs b/src/Castle.Core/DynamicProxy/Generators/MethodSignatureComparer.cs index b4eb885fc8..4c8f2472e6 100644 --- a/src/Castle.Core/DynamicProxy/Generators/MethodSignatureComparer.cs +++ b/src/Castle.Core/DynamicProxy/Generators/MethodSignatureComparer.cs @@ -92,7 +92,7 @@ private bool EqualSignatureTypes(Type x, Type y, MethodInfo xm = null, MethodInf } else if (x.IsGenericType != y.IsGenericType) { - return false; + return IsCovariantReturnTypes(x, y, xm, ym); } if (x.IsGenericParameter) @@ -129,19 +129,24 @@ private bool EqualSignatureTypes(Type x, Type y, MethodInfo xm = null, MethodInf { if (!x.Equals(y)) { - // This enables covariant method returns for .NET 5 and newer. - // No need to check for runtime support, since such methods are marked with a custom attribute; - // see https://github.com/dotnet/runtime/blob/main/docs/design/features/covariant-return-methods.md. - if (preserveBaseOverridesAttribute != null) - { - return (xm != null && xm.IsDefined(preserveBaseOverridesAttribute, inherit: false) && y.IsAssignableFrom(x)) - || (ym != null && ym.IsDefined(preserveBaseOverridesAttribute, inherit: false) && x.IsAssignableFrom(y)); - } - - return false; + return IsCovariantReturnTypes(x, y, xm, ym); } } return true; + + static bool IsCovariantReturnTypes(Type x, Type y, MethodInfo xm, MethodInfo ym) + { + // This enables covariant method returns for .NET 5 and newer. + // No need to check for runtime support, since such methods are marked with a custom attribute; + // see https://github.com/dotnet/runtime/blob/main/docs/design/features/covariant-return-methods.md. + if (preserveBaseOverridesAttribute != null) + { + return (xm != null && xm.IsDefined(preserveBaseOverridesAttribute, inherit: false) && y.IsAssignableFrom(x)) + || (ym != null && ym.IsDefined(preserveBaseOverridesAttribute, inherit: false) && x.IsAssignableFrom(y)); + } + + return false; + } } public bool Equals(MethodInfo x, MethodInfo y) From ee083097af525b001a786cc1d14334cbb64e825c Mon Sep 17 00:00:00 2001 From: Dominique Schuppli Date: Wed, 28 Dec 2022 23:29:32 +0100 Subject: [PATCH 5/9] Detail: assert that `IsCovariantReturnTypes` only gets called for return types --- .../DynamicProxy/Generators/MethodSignatureComparer.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/Castle.Core/DynamicProxy/Generators/MethodSignatureComparer.cs b/src/Castle.Core/DynamicProxy/Generators/MethodSignatureComparer.cs index 4c8f2472e6..354a026473 100644 --- a/src/Castle.Core/DynamicProxy/Generators/MethodSignatureComparer.cs +++ b/src/Castle.Core/DynamicProxy/Generators/MethodSignatureComparer.cs @@ -16,6 +16,7 @@ namespace Castle.DynamicProxy.Generators { using System; using System.Collections.Generic; + using System.Diagnostics; using System.Reflection; internal class MethodSignatureComparer : IEqualityComparer @@ -136,6 +137,9 @@ private bool EqualSignatureTypes(Type x, Type y, MethodInfo xm = null, MethodInf static bool IsCovariantReturnTypes(Type x, Type y, MethodInfo xm, MethodInfo ym) { + Debug.Assert((xm == null && ym == null) + || (xm != null && ym != null && x == xm.ReturnType && y == ym.ReturnType)); + // This enables covariant method returns for .NET 5 and newer. // No need to check for runtime support, since such methods are marked with a custom attribute; // see https://github.com/dotnet/runtime/blob/main/docs/design/features/covariant-return-methods.md. From be223326ac78ce461707d09ccc11cc90f190baf9 Mon Sep 17 00:00:00 2001 From: Dominique Schuppli Date: Wed, 28 Dec 2022 23:43:13 +0100 Subject: [PATCH 6/9] Add failing tests for covariant return types involving a variant generic interface --- .../CovariantReturnsTestCase.cs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/Castle.Core.Tests/DynamicProxy.Tests/CovariantReturnsTestCase.cs b/src/Castle.Core.Tests/DynamicProxy.Tests/CovariantReturnsTestCase.cs index e128d09c73..689f7949e1 100644 --- a/src/Castle.Core.Tests/DynamicProxy.Tests/CovariantReturnsTestCase.cs +++ b/src/Castle.Core.Tests/DynamicProxy.Tests/CovariantReturnsTestCase.cs @@ -52,6 +52,7 @@ public void Reflection_returns_methods_from_a_derived_class_before_methods_from_ [TestCase(typeof(DerivedClassWithStringInsteadOfObject))] [TestCase(typeof(DerivedClassWithStringInsteadOfInterface))] [TestCase(typeof(DerivedClassWithStringInsteadOfGenericArg))] + [TestCase(typeof(BottleOfWater))] public void Can_proxy_type_having_method_with_covariant_return(Type classToProxy) { _ = generator.CreateClassProxy(classToProxy, new StandardInterceptor()); @@ -62,6 +63,7 @@ public void Can_proxy_type_having_method_with_covariant_return(Type classToProxy [TestCase(typeof(DerivedClassWithStringInsteadOfObject), typeof(string))] [TestCase(typeof(DerivedClassWithStringInsteadOfInterface), typeof(string))] [TestCase(typeof(DerivedClassWithStringInsteadOfGenericArg), typeof(string))] + [TestCase(typeof(BottleOfWater), typeof(Glass))] public void Proxied_method_has_correct_return_type(Type classToProxy, Type expectedMethodReturnType) { var proxy = generator.CreateClassProxy(classToProxy, new StandardInterceptor()); @@ -116,6 +118,22 @@ public class DerivedClassWithStringInsteadOfGenericArg : BaseClassWithGenericArg { public override string Method() => nameof(DerivedClassWithStringInsteadOfGenericArg); } + + public interface Glass { } + + public class Liquid { } + + public class Water : Liquid { } + + public class BottleOfLiquid + { + public virtual Glass Method() => default(Glass); + } + + public class BottleOfWater : BottleOfLiquid + { + public override Glass Method() => default(Glass); + } } } From a1b44d7750c3d33385a11b0f4d9e8fa43bde9298 Mon Sep 17 00:00:00 2001 From: Dominique Schuppli Date: Wed, 28 Dec 2022 23:47:25 +0100 Subject: [PATCH 7/9] Also check for covariant return types if both of them are generic --- .../DynamicProxy/Generators/MethodSignatureComparer.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/Castle.Core/DynamicProxy/Generators/MethodSignatureComparer.cs b/src/Castle.Core/DynamicProxy/Generators/MethodSignatureComparer.cs index 354a026473..624e55f830 100644 --- a/src/Castle.Core/DynamicProxy/Generators/MethodSignatureComparer.cs +++ b/src/Castle.Core/DynamicProxy/Generators/MethodSignatureComparer.cs @@ -121,9 +121,12 @@ private bool EqualSignatureTypes(Type x, Type y, MethodInfo xm = null, MethodInf return false; } - for (var i = 0; i < xArgs.Length; ++i) + if (IsCovariantReturnTypes(x, y, xm, ym) == false) { - if(!EqualSignatureTypes(xArgs[i], yArgs[i])) return false; + for (var i = 0; i < xArgs.Length; ++i) + { + if(!EqualSignatureTypes(xArgs[i], yArgs[i])) return false; + } } } else From eda2c80785207bdd9a85e71ad04af1ff0127e1f9 Mon Sep 17 00:00:00 2001 From: Dominique Schuppli Date: Thu, 29 Dec 2022 12:29:20 +0100 Subject: [PATCH 8/9] Move check for covariant return types Performing this check for anything other than return types is basically wasted time, so moving it to `EqualReturnTypes` means it will only run when actually needed. --- .../Generators/MethodSignatureComparer.cs | 50 +++++++++---------- 1 file changed, 23 insertions(+), 27 deletions(-) diff --git a/src/Castle.Core/DynamicProxy/Generators/MethodSignatureComparer.cs b/src/Castle.Core/DynamicProxy/Generators/MethodSignatureComparer.cs index 624e55f830..82cd760020 100644 --- a/src/Castle.Core/DynamicProxy/Generators/MethodSignatureComparer.cs +++ b/src/Castle.Core/DynamicProxy/Generators/MethodSignatureComparer.cs @@ -16,7 +16,6 @@ namespace Castle.DynamicProxy.Generators { using System; using System.Collections.Generic; - using System.Diagnostics; using System.Reflection; internal class MethodSignatureComparer : IEqualityComparer @@ -82,10 +81,27 @@ public bool EqualParameters(MethodInfo x, MethodInfo y) public bool EqualReturnTypes(MethodInfo x, MethodInfo y) { - return EqualSignatureTypes(x.ReturnType, y.ReturnType, x, y); + var xr = x.ReturnType; + var yr = y.ReturnType; + + if (EqualSignatureTypes(xr, yr)) + { + return true; + } + + // This enables covariant method returns for .NET 5 and newer. + // No need to check for runtime support, since such methods are marked with a custom attribute; + // see https://github.com/dotnet/runtime/blob/main/docs/design/features/covariant-return-methods.md. + if (preserveBaseOverridesAttribute != null) + { + return (x.IsDefined(preserveBaseOverridesAttribute, inherit: false) && yr.IsAssignableFrom(xr)) + || (y.IsDefined(preserveBaseOverridesAttribute, inherit: false) && xr.IsAssignableFrom(yr)); + } + + return false; } - private bool EqualSignatureTypes(Type x, Type y, MethodInfo xm = null, MethodInfo ym = null) + private bool EqualSignatureTypes(Type x, Type y) { if (x.IsGenericParameter != y.IsGenericParameter) { @@ -93,7 +109,7 @@ private bool EqualSignatureTypes(Type x, Type y, MethodInfo xm = null, MethodInf } else if (x.IsGenericType != y.IsGenericType) { - return IsCovariantReturnTypes(x, y, xm, ym); + return false; } if (x.IsGenericParameter) @@ -121,39 +137,19 @@ private bool EqualSignatureTypes(Type x, Type y, MethodInfo xm = null, MethodInf return false; } - if (IsCovariantReturnTypes(x, y, xm, ym) == false) + for (var i = 0; i < xArgs.Length; ++i) { - for (var i = 0; i < xArgs.Length; ++i) - { - if(!EqualSignatureTypes(xArgs[i], yArgs[i])) return false; - } + if(!EqualSignatureTypes(xArgs[i], yArgs[i])) return false; } } else { if (!x.Equals(y)) { - return IsCovariantReturnTypes(x, y, xm, ym); + return false; } } return true; - - static bool IsCovariantReturnTypes(Type x, Type y, MethodInfo xm, MethodInfo ym) - { - Debug.Assert((xm == null && ym == null) - || (xm != null && ym != null && x == xm.ReturnType && y == ym.ReturnType)); - - // This enables covariant method returns for .NET 5 and newer. - // No need to check for runtime support, since such methods are marked with a custom attribute; - // see https://github.com/dotnet/runtime/blob/main/docs/design/features/covariant-return-methods.md. - if (preserveBaseOverridesAttribute != null) - { - return (xm != null && xm.IsDefined(preserveBaseOverridesAttribute, inherit: false) && y.IsAssignableFrom(x)) - || (ym != null && ym.IsDefined(preserveBaseOverridesAttribute, inherit: false) && x.IsAssignableFrom(y)); - } - - return false; - } } public bool Equals(MethodInfo x, MethodInfo y) From 6b1613599af35cdb2423a4be03e0000aefb60014 Mon Sep 17 00:00:00 2001 From: Dominique Schuppli Date: Wed, 28 Dec 2022 23:49:04 +0100 Subject: [PATCH 9/9] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index aac32ea056..e7b2a4397b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## Unreleased Bugfixes: +- Proxies using records derived from a base generic record broken using .NET 6 compiler (@CesarD, #632) - Failure proxying type that has a non-inheritable custom attribute type applied where `null` argument is given for array parameter (@stakx, #637) - Nested custom attribute types do not get replicated (@stakx, #638)