Proxies using Records with a base class broken since .NET 6 #26602
Labels
area-external
closed-fixed
The issue has been fixed and is/will be included in the release indicated by the issue milestone.
customer-reported
Servicing-approved
type-bug
Milestone
Comments
|
Any update on this ;-) |
|
Note for triage: this reproduces with with the .NET 6 SDK when targeting .NET 5 or .NET 6, regardless of the version of EF used. Could this be a compiler/SDK issue? I have not yet tried going back to the .NET 5 SDK. Full trace: |
|
I feel this comes from an internal change to how the clone method works and how Castle.Proxies does its stuff. Comparing .NET5 and 6 on sharplab.io (main for .NET6 and C# 9: Covariant(31 Jul 2020) for .NET5) the down-leveled code for records as it relates to records with inheritance looked like this:
C#9 / .NET5[System.Runtime.CompilerServices.NullableContext(1)]
[System.Runtime.CompilerServices.Nullable(0)]
public class Child : Base, IEquatable<Child>
{
// (...) Irrelevant stuff omitted
public override Base <Clone>$()
{
return new Child(this);
}
protected Child(Child original)
: base(original)
{
<MyInt>k__BackingField = original.<MyInt>k__BackingField;
}
public Child()
{
}
}C10 / .NET6[System.Runtime.CompilerServices.NullableContext(1)]
[System.Runtime.CompilerServices.Nullable(0)]
public class Child : Base, IEquatable<Child>
{
// (...) Irrelevant stuff omitted
[PreserveBaseOverrides]
public new virtual Child <Clone>$()
{
return new Child(this);
}
protected Child(Child original)
: base(original)
{
<MyInt>k__BackingField = original.<MyInt>k__BackingField;
}
public Child()
{
}
}The most glaring change here is going from simply overriding and return |
|
The C# compiler changed the emit strategy for records in .NET 6 to take advantage of covariant returns. The |
|
Filed castleproject/Core#601 on the Castle Proxies repo. |
|
"The C# compiler changed the emit strategy for records in .NET 6 to take advantage of covariant returns.... In .NET 6 though we finished off that feature and added it to records." (@jaredpar) @ajcvickers : If this is correct then surely the issue does lie with EF Core? i.e. EF Core must be changed to work correctly with the new version of Roslyn not vice versa? |
|
@richardpawson Agreed that Roslyn is not at issue. Instead, Castle Proxies needs to understand the differently generated code, hence the external issue here is castleproject/Core#601 |
|
@ajcvickers I'm concerned now. The Castle Proxies project doesn't seem very active: no code check-ins since March, and limited response to several raised issues. The EF Core team took the decision to make this Microsoft product dependent upon a third-party open source product. Can you provide some reassurance that you have sufficient influence or a private arrangement to get this bug fixed? It is impacting us substantially. |
|
@richardpawson We will follow up with them and provide help where we can. We are committed to an open-source ecosystem. The alternative to using Castle Proxies would likely have been to drop lazy-loading from EF Core, since usage numbers are very low, and lazy-loading is largely considered an anti-pattern these days. |
|
@ajcvickers Why has this been marked as closed? It might well be that the root of the issue lies in Castle (although they might argue -if they respond at all - that its is not a bug) but since the result is that EF Core does not work correctly in .NET 6, for a documented capability that worked correctly in .NET 5, surely this is a bug that you must take ownership of? Marking it as closed is denying that it is an issue. |
|
@richardpawson We are tracking the Castle issue. There is currently no action to take on the EF side. |
|
Reopening to consider a patch that avoids this problem for records with base types, inspired by #27073. |
ajcvickers
added a commit
that referenced
this issue
Jan 6, 2022
Fixes #26602 **Description** 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. Castle dynamic proxies throws when attempting to create a proxy for such types. This means EF Core lazy-loading and change-tracking proxies are broken for any record with a base type. An bug has been filed on Castle proxies (castleproject/Core#601), which is the correct place to ultimately fix this issue. However, since EF Core never needs to override the Clone method for its proxies, the fix here simply excludes the Clone method from the proxies we create. **Customer impact** Proxies for records with base types cannot be used. Reported by multiple customers. No known workaround. How found Multiple customer reports on 6.0. Regression Yes, From 5.0 Testing Added unit and functional tests for records with base types. Risk Very low; Clone methods are never used by EF proxies. Also added quirk to revert to previous behavior.
ajcvickers
added a commit
that referenced
this issue
Jan 10, 2022
ajcvickers
added
closed-fixed
|
In version 6.0.2 if base record is an abstract |
stakx
added a commit
to stakx/Castle.Core
that referenced
this issue
May 13, 2022
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.
stakx
added a commit
to stakx/Castle.Core
that referenced
this issue
May 14, 2022
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.
|
FYI - Castle.Core has a new (ish) version - 5.1.1. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Description
When using a record with a base class while having lazy loading proxies enabled any usage of the
DbContextwill immediately crash with the followingTypeLoadException:This exception did not happen before .NET 6 as we've used this kind of code all over our production code base with the expected results.
Repo
A Repo to quickly demonstrate the bug can be found here:
https://github.com/Jejuni/Net6RecordEfCoreProxies
Code
Entities look like this:
DbContext is configured to have
MyRecordbe an owned entity viaOwnsOne.Crashing code:
However, the exception is also thrown when
MyRecordis configured explicitly or per convention as aHasOne.The exception is not thrown when a class is used in place of a record
Exception:
Include provider and version information
EF Core version: 6.0.0
Database provider: Microsoft.EntityFrameworkCore.SqlServer / InMemory / Probably any other one as well
Target framework: .NET 6.0
Operating system: Windows 10
IDE: Visual Studio 2022 17.0.0
The text was updated successfully, but these errors were encountered: