Add support for covariant returns #619
Conversation
The .NET runtime design document Covariant Return Methods mentions |
stakx
force-pushed
the
bugfix/covariant-returns
branch
from
16bf56b
to
62e90b8
Compare
This is ensured by the fact that Reflection reports methods from a derived class before methods from its base class. Because of that, DynamicProxy sees override methods before overridden methods, and will treat only the latter as duplicates. It's important to note that our implementation relies on a specific behavior of .NET Reflection. I've documented that assumption as a unit test. |
stakx
force-pushed
the
bugfix/covariant-returns
branch
2 times, most recently
from
fb6a724
to
b8703ba
Compare
|
This is now ready for reviewing. As I mentioned in #601, I'm open to suggestions for further tests to be added to the test suite. |
On .NET 6+ this fails with a `TypeLoadException`: > Return type in method `DerivedEmptyRecord.<Clone>$()` [...] is not > compatible with base type method `EmptyRecord.<Clone>$()` From dotnet/efcore#26602 (comment): > The C# compiler changed the emit strategy for records in .NET 6 to > take advantage of covariant returns. The Clone method now always has a > return type that matches the containing type. Covariant returns were > added to the runtime and language in .NET 5 but records didn't take > advantage of them (just ran out of time). In .NET 6 though we finished > off that feature and added it to records. So we'll need to add support for covariant returns to DynamicProxy.
The previous commit's test fails due to DynamicProxy treating the base and derived record classes' `<Clone>$` methods as two distinct methods (because they differ in their exact return type), instead of as a single overridden method. We can change this by adjusting `MethodSignature- Comparer` such that it also accepts assignment-compatible return types.
Covariant returns were introduced with .NET 5. For all earlier runtimes, the last commit may have relaxed method signature comparison too much. We can resolve this by checking for a specific custom attribute that .NET compilers are expected to put on override methods using covariant returns. Reference: https://github.com/dotnet/runtime/blob/main/docs/design/features/covariant-return-methods.md
So far, we simply assumed that `x` := override method, and `y` := over- ridden method, but there is actually no guarantee for that.
stakx
force-pushed
the
bugfix/covariant-returns
branch
from
b8703ba
to
01c2c6a
Compare
|
@jonorossi ping |
This should fix #601.
Right now, there are only two very basic unit tests.Some more tests may be needed to test various aspects of support for covariant returns.For example:interplay between covariant returns and generics (if that's a feasible scenario at all)ensuring DynamicProxy always picks the "most derived" method for proxyingSome more things to think about (I'll expand this list as further concerns show up):
Do we need to detect runtime support for covariant returns, or are the required changes compatible with older platforms that don't support them?