System.ArgumentException: Argument types do not match error in Asp.Net App-52ABP.School

[DotNetAppCompatFeiWang]

Describe the bug

An unhandled exception has occurred while executing the request.
System.ArgumentException: Argument types do not match

To Reproduce

1. Clone repo from https://github.com/yoyomooc/52ABP.School and build it.
2. On CMD, go to publish folder at C:\Repro\52ABP.School
3. run : dotnet LTM.School.dll
4. Open http://localhost:5000/Students/Details/1 with Edge

Further technical details

• ASP.NET Core version
  ASP.NET Core version 5.0.0-preview.1.20124.5

• Include the output of dotnet --info
  .NET Core SDK (reflecting any global.json):
  Version: 5.0.100-preview.1.20125.9
  Commit: 12e88ed2e5
  Runtime Environment:
  OS Name: Windows
  OS Version: 10.0.18363
  OS Platform: Windows
  RID: win10-x64
  Base Path: C:\Program Files\dotnet\sdk\5.0.100-preview.1.20125.9
  Host (useful for support):
  Version: 5.0.0-preview.1.20120.5
  Commit: 3c523a6
  .NET Core SDKs installed:
  5.0.100-preview.1.20125.9 [C:\Program Files\dotnet\sdk]
  .NET Core runtimes installed:
  Microsoft.AspNetCore.App 5.0.0-preview.1.20124.5 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 5.0.0-preview.1.20120.5 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 5.0.0-preview.1.20122.1 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  To install additional .NET Core runtimes or SDKs:
  https://aka.ms/dotnet-download
  To install additional .NET Core runtimes or SDKs:
  https://aka.ms/dotnet-download

• The IDE (VS / VS Code/ VS4Mac) you're running on, and it's version: CLI

Repro machine could be found at: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1075299

Add exception log:
52AbpException.txt

@dotnet-actwx-bot FYI

[rynowak]

Callstack:

C:\Repro\52ABP.School>dotnet LTM.School.dll
Hosting environment: Production
Content root path: C:\Repro\52ABP.School
Now listening on: http://localhost:5000
Now listening on: https://localhost:5001
Application started. Press Ctrl+C to shut down.
fail: Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware[1]
      An unhandled exception has occurred while executing the request.
System.ArgumentException: Argument types do not match
   at System.Linq.Expressions.Expression.Condition(Expression test, Expression ifTrue, Expression ifFalse)
   at Microsoft.EntityFrameworkCore.Query.Internal.IncludeCompiler.IncludeLoadTreeNode.BuildCollectionIncludeExpressions(INavigation navigation, Expression targetEntityExpression, Boolean trackingQuery, Expression relatedCollectionFuncExpression, MethodInfo includeCollectionMethodInfo, Expression cancellationTokenExpression, Int32 collectionIncludeId)
   at Microsoft.EntityFrameworkCore.Query.Internal.IncludeCompiler.IncludeLoadTreeNode.CompileCollectionInclude(QueryCompilationContext queryCompilationContext, Expression targetExpression, Expression entityParameter, Boolean trackingQuery, Boolean asyncQuery, Int32& collectionIncludeId)
   at Microsoft.EntityFrameworkCore.Query.Internal.IncludeCompiler.IncludeLoadTreeNodeBase.Compile(QueryCompilationContext queryCompilationContext, QueryModel queryModel, Boolean trackingQuery, Boolean asyncQuery, Int32& collectionIncludeId, QuerySourceReferenceExpression targetQuerySourceReferenceExpression)
   at Microsoft.EntityFrameworkCore.Query.Internal.IncludeCompiler.IncludeLoadTree.Compile(QueryCompilationContext queryCompilationContext, QueryModel queryModel, Boolean trackingQuery, Boolean asyncQuery, Int32& collectionIncludeId)
   at Microsoft.EntityFrameworkCore.Query.Internal.IncludeCompiler.CompileIncludes(QueryModel queryModel, Boolean trackingQuery, Boolean asyncQuery)
   at Microsoft.EntityFrameworkCore.Query.EntityQueryModelVisitor.OptimizeQueryModel(QueryModel queryModel, Boolean asyncQuery)
   at Microsoft.EntityFrameworkCore.Query.RelationalQueryModelVisitor.OptimizeQueryModel(QueryModel queryModel, Boolean asyncQuery)
   at Microsoft.EntityFrameworkCore.Query.EntityQueryModelVisitor.CreateAsyncQueryExecutor[TResult](QueryModel queryModel)
   at Microsoft.EntityFrameworkCore.Storage.Database.CompileAsyncQuery[TResult](QueryModel queryModel)
   at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.CompileAsyncQueryCore[TResult](Expression query, IQueryModelGenerator queryModelGenerator, IDatabase database)
   at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.<>c__DisplayClass22_0`1.<CompileAsyncQuery>b__0()
   at Microsoft.EntityFrameworkCore.Query.Internal.CompiledQueryCache.GetOrAddQueryCore[TFunc](Object cacheKey, Func`1 compiler)
   at Microsoft.EntityFrameworkCore.Query.Internal.CompiledQueryCache.GetOrAddAsyncQuery[TResult](Object cacheKey, Func`1 compiler)
   at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.CompileAsyncQuery[TResult](Expression query)
   at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.ExecuteAsync[TResult](Expression query, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.Query.Internal.EntityQueryProvider.ExecuteAsync[TResult](Expression expression, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.EntityFrameworkQueryableExtensions.ExecuteAsync[TSource,TResult](MethodInfo operatorMethodInfo, IQueryable`1 source, Expression expression, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.EntityFrameworkQueryableExtensions.ExecuteAsync[TSource,TResult](MethodInfo operatorMethodInfo, IQueryable`1 source, LambdaExpression expression, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.EntityFrameworkQueryableExtensions.SingleOrDefaultAsync[TSource](IQueryable`1 source, Expression`1 predicate, CancellationToken cancellationToken)
   at LTM.School.Controllers.StudentsController.Details(Nullable`1 id) in c:\Users\appcompat\52ABP.School\LTM.School\Controllers\StudentsController.cs:line 90
   at Microsoft.AspNetCore.Mvc.Internal.ActionMethodExecutor.TaskOfIActionResultExecutor.Execute(IActionResultTypeMapper mapper, ObjectMethodExecutor executor, Object controller, Object[] arguments)
   at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.InvokeActionMethodAsync()
   at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.InvokeNextActionFilterAsync()
   at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.Rethrow(ActionExecutedContext context)
   at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.InvokeInnerFilterAsync()
   at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeNextResourceFilter()
   at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.Rethrow(ResourceExecutedContext context)
   at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeFilterPipelineAsync()
   at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeAsync()
   at Microsoft.AspNetCore.Builder.RouterMiddleware.Invoke(HttpContext httpContext)
   at Microsoft.AspNetCore.StaticFiles.StaticFileMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware.Invoke(HttpContext context)

[ajcvickers]

@DotNetAppCompatFeiWang What exactly is this testing? It looks like you might be trying to run app with 2.1 EF Core on 5.0? If so, that pretty much won't work.

[PriyaPurkayastha]

@ajcvickers for compat testing we do run apps built with earlier .NET Core versions(2.x/3.x apps) against latest .NET Core release (.NET 5) runtime. The goal is to ensure that if there are breaking changes, they are investigated to confirm that these are intentional breaking changes that have been documented. For this example, what has changed in 5.0 and have those changes been documented? I have another thread with @DotNetAppCompatFeiWang where I have asked whether this app was running fine on .NET Core 3.x but are now failing on 5.0 and will wait for his response.

[ajcvickers]

@PriyaPurkayastha If you're going from 3.1 to 5.0, then that's reasonable. Going from 2.x to 3.x or higher is not something that will work.

[ajcvickers]

@PriyaPurkayastha However, note that the stack trace here is coming from a 2.x version of EF Core.

[DotNetAppCompatFeiWang]

@ajcvickers I have checked this app(target 2.1) works fine on .NET Core 3.1.103 build but not work on .NET 5.

[DotNetAppCompatFeiWang]

@ajcvickers The app is target on .NET Core 2.1, it has dependency to EF-core 2.14, when we run against .NET 3.1 Pass, but Fail on .NET 5.

[ajcvickers]

@DotNetAppCompatFeiWang Thanks for the additional information. So, since nothing has changed in the EF Core version that is being used, then this means something else has changed in .NET 5 that is causing EF to throw this exception. Top of the stack seems a reasonable place to start. @danmosemsft Who owns System.Linq.Expressions.Expression.Condition?

[danmoseley]

@ajcvickers per https://github.com/dotnet/runtime/blob/master/docs/area-owners.md it is @cston @333fred

cc @PriyaPurkayastha

[danmoseley]

Not much happened there it looks like. Seems we have a repro - does the debugger give a clue?

[ajcvickers]

Transferring to the runtime repo, since nothing has changed in EF (the app uses EF Core 2.1) but something updated in the runtime for 5.0 has now broken the app.

[danmoseley]

@cston thoughts?

[cston]

The types of the arguments to Expression.Condition() are not equivalent.

ifTrue.Type: System.Threading.Tasks.Task
ifFalse.Type: System.Threading.Tasks.Task`1[System.Threading.Tasks.VoidTaskResult]

I don't have access to BuildCollectionIncludeExpressions(). @ajcvickers, are those argument types expected?

[ajcvickers]

@cston All I know is nothing has changed since the exact same EF code is running in both places. We're not going to spend a lot of time investigating the EF code here since it's part of the query code that was totally re-written for 3.0. You can dig into the 2.1 source on GitHub if it helps.

[danmoseley]

As an aside, ideally perhaps the Argument types do not match message would include the .GetType().ToString() for both.

[cston]

BuildCollectionIncludeExpressions uses Expression.Constant(Task.CompletedTask) as an expression of type Task but Task.CompletedTask now returns a derived type of Task instead.

[stephentoub]

@benaadams, fyi, fallout from your change...
dotnet/coreclr#27437

But, we accepted the change as we do generally consider it to be acceptable to return more derived types. Unless it's widespread, I'd prefer to see EF fix the call site.

[benaadams]

dotnet/coreclr#27437 also means a sync completing async Task method and Task.CompletedTask match types whereas previously they wouldn't have

[stephentoub]

@ajcvickers, can we move this EF and fix it there?

[ajcvickers]

@stephentoub Does this meet the bar for a 2.1 patch? I don't think it does, but if you guys do then we can talk about the best way to approach it.

[stephentoub]

Does this meet the bar for a 2.1 patch? I don't think it does, but if you guys do then we can talk about the best way to approach it.

Sorry, I don't understand the question. What does 2.1 have to do with this?

[ajcvickers]

This EF code does not exist beyond 2.x. It was replaced in 3.0. Also this is testing using EF Core 2.1 on .NET 5. The first thing we would do if a customer reported this would be to very strongly advise that they don't do that.

[stephentoub]

Ah, thanks for the clarification. In that case, I would agree with you, and this can probably just be closed.

[marklio]

I think we got overly fixated on the specifics of this break. We use the appcompat lab to act as a proxy for our customers. The fact that the actual break is in EF code that has been superseded isn't the relevant piece of info here. Unless the broken code is known to be special in some way, we don't tend to distinguish between code ownership in the compat lab. We tend to think of failures as evidence that a change we made breaks real code that anyone could have written. We need to respond to this by evaluating the original change and documenting it through the breaking change process, or by mitigating it in code (which we could do with better diagnostics, analyzers, or whatever).

[jkotas]

@marklio This is bug in EF code. The EF code takes dependency on unspecified behavior. We do not document these types of changes via breaking change process for .NET Core.

[marklio]

We do not document these types of changes via breaking change process for .NET Core.

I see. Is that a statement about major versions, or all up? I assume if we had evidence of a customer pattern we would avoid breaking it at least in a servicing release, or document it if we had to change it for other reasons.

[jkotas]

For servicing releases, we maintain bug-for-bug compatibility. This change would not be acceptable in servicing.

For major releases, we maintain compatibility, but we do not maintain bug-for-bug compatibility. We do make exceptions when there is a strong evidence that change is causing breaks that are migration blockers, etc.

For NuGet packages, we try to make sure that everything works well when all NuGet packages are updated to latest. We had number of situations when old NuGet package + latest runtime combinations did not work well.

[PriyaPurkayastha]

@jkotas thanks for the clarification. What you have mentioned is inline with the compat principles that we have in place since the .NET Core 3.0 release. We understand why it is acceptable to make the change in the major release (at least until we hear of migration blockers). However, I am not clear on why the change shouldn't be documented as a breaking change (especially when we have evidence of this in at least one instance). Is it that no other apps can or should check the value of Task.CompletedTask similar to how EF has been doing? By documenting we will be proactively helping alert customers so that they can update their code, as needed. What is the concern with adding breaking change documentation for this change?

[benaadams]

It doesn't count as a breaking change? Property, Field, Parameter and Return Values

Allowed: Returning a value of a more derived type for a property, field, return or out value

[PriyaPurkayastha]

Thanks @benaadams. Change in behavior does count as a breaking change (https://docs.microsoft.com/en-us/dotnet/core/compatibility/categories) - this app was working fine with .NET Core 3.x and is throwing an exception when run against .NET 5. In this specific case it was a 2.1 app but even 3.x apps might get broken if they have the same usage pattern.
The link you have provided describes the kinds of breaking changes that are allowed in .NET Core and we are all agreeing that this change is allowed. What I would like to propose is that we document this using https://github.com/dotnet/docs/issues/new?template=dotnet-breaking-change.md to inform customers about this change.

[jkotas]

We have not been documenting the allowed changes. The allowed changes are allowed because they have very low risk of breaking real world apps. The chance that there is another app or library that will have the same bug as EF Core is low.

We have been changing the underlying return type for number of APIs between .NET Core releases. For example, Console.WriteLine(Encoding.UTF8.GetType() == typeof(UTF8Encoding)) will print true in .NET Core 2.x , but it will print false in .NET Core 3.x. I do not recall another situation where this caused a actual break in the app.

The breaking change lists that we publish are long and hard to comprehend already. If we make it a policy to document allowed breaking changes, the list will become many times longer. It will be hard to see the breaking changes that actually matter in the list. The lists do not include severity that would allow customers to filter to see what matters.

If we were to start documenting the allowed changes, we need to:

• Add severity to the documentation and allow filtering based on the severity
• Define the bar what it needs to be documented. For example, performance optimizations are allowed change that occasionally breaks real world apps by exposing race conditions in them. Are we going to include significant performance optimizations in the breaking change lists?

I think a reasonable thing to do here would be to document that EF Core 2.1 is not supported on .NET 5. It is the piece of information that our customers actually need to know. Where is the right place to document this?

[stephentoub]

I agree with everything Jan wrote. Just to highlight one piece in particular:

We have not been documenting the allowed changes.

Practically every commit in dotnet/runtime has the potential to cause an observable behavior change. Rename a private type? Someone using private reflection may no longer be able to find it. Add an entirely new feature? Someone could see an increased binary size (which they might be comparing against), or might be using reflection to enumerate even just public surface area and see something they weren't expecting. Add an additional member to an enum? That could cause problems if someone was expecting the exact number that was there before. Change the text of an exception message? Someone examining that text may no longer find something they were searching for. Make something faster? Race conditions in their code may now start to appear. Fix a null reference exception? Someone could have been depending on getting a null reference exception. Fix a crash? Someone could have had a separate process monitoring for that crash. And so on and so on.

We have to draw a line somewhere as to what we consider to be breaking such that it's worth either a) not making the change because the likelihood of it breaking lots of apps is way too high, or b) documenting the change because the benefit of it outweighs the downside. And that line is https://github.com/dotnet/runtime/blob/master/docs/coding-guidelines/breaking-change-rules.md.

The property being discussed here is basically a factory, and a key point of a factory is to return something that meets that criteria of the return type but that is often actually a derived type. For example, if tomorrow we change XmlReader XmlReader.Create(...) to return an internal MySpecial2Reader instead of an internal MySpecial1Reader, we do not consider that to be breaking, even though someone could be using reflection to look at the GetType().Name of the instance returned from Create. Same goes for Task.CompletedTask.

[marklio]

I think we're on the same page here for this issue, and I'm fine with the outcome on this. I think the thing I would ask is that we recognize that the breaking change rules give us a foundation on which to make autonomous decisions as developers. We sometimes break those rules to improve the platform, and I think we need to be open to considering the rules inadequate, at times, for making the platform great for customers. The compat lab's purpose is to show us where the rules we've followed to get where we are have been inadequate to keep apps running. We can absolutely decide that the good of the change outweighs the break, and that may be the right thing to do (and it might be right to chose not to document), but only because we've reviewed at a compat issue and decided it's the right thing to do.
I don't think Priya is suggesting a blanket policy where we document all changes. But, when we have evidence through data or other efforts like the compat lab that a change may have higher risk to customers, we should take the time to think through and make an informed decision rather than simply apply a set of rules.

[jkotas]

@marklio Do you agree that it makes sense to document that EF Core 2.1 is not supported on .NET 5 for this specific issue? Where would be the right place to document this? Does it make sense to include it in the breaking change list? (ie the title of the breaking change would read "EF Core 2.1 is not supported on .NET 5").

[ajcvickers]

@jkotas Do we know if it's more than just one query in one app at this point? Seems a bit extreme to drop all support.

/cc @Pilchie

[jkotas]

If you want to support EF Core 2.1 on .NET 5, you need to be willing to patch latent bugs in it.

Also, you have said above: The first thing we would do if a customer reported this would be to very strongly advise that they don't do that.

[marklio]

@marklio Do you agree that it makes sense to document that EF Core 2.1 is not supported on .NET 5 for this specific issue?

@jkotas Provided we get that statement agreed upon, that sounds completely reasonable. That would help with the specifics of this break. As you suspected, I was unable to find any other instances of this pattern in the wild.

@ajcvickers How straightforward is it to migrate between EF versions? Is it possible to hit the same DB with two different versions of EF without causing problems? (in other words, would being stuck on EF 2.1 prevent .NET 5 adoption? I think we would want to avoid that scenario)

[ajcvickers]

@jkotas There's a big difference between fixing a particular bug or not and saying something is not supported. There are lots of 2.1 bugs that we won't patch, but none of them resulted in us saying that it is not supported. Also, not recommended is not the same as not supported.

[ajcvickers]

How straightforward is it to migrate between EF versions?

Not straightforward, if you mean 2.x to 3.x+.

would being stuck on EF 2.1 prevent .NET 5 adoption

Unlikely, but possible. However, if that's the concern then we should fix this bug in a patch. However, it clearly wouldn't meet the bar for tactics, so unless we're greatly changing the patch bar I don't think this is relevant.

[marklio]

@ajcvickers What is our statement of support for EF 2.x in .NET 5? If we

• aren't recommending it
• know that it "pretty much won't work"
• and aren't fixing lots of bugs

I think there's value in making sure that's clear so current EF customers can know what a migration to .NET 5 will entail.

[ajcvickers]

@marklio For EF, most things should work. But for an ASP.NET Core app, it's not supported because shared framework. Also, ASP.NET Core pulls in newer versions of EF Core, so in practice you can't use the two together unless you're not using the bits of ASP.NET Core that pull in newer versions of EF Core.

EF Core 2.1 is LTS and therefore supported. It targets .NET Standard 2.0, so that support extends to any platform that supports .NET Standard 2.0.

Basically, we have packages, .NET standard versions, shared frameworks, and .NET itself in the picture here. We try to recommend people down happy paths--for example, if you're using ASP.NET Core, then use all the versions that align with that. Note however, that is not how our support policies are written, so in reality across .NET we support many combinations that are not recommended. Typically, if a customer falls into one of these cases, then we work with them to find the best way forward. Occasionally that means fixing an LTS bug, but usually it means working with the customer on updating their app or using targeted workarounds.

[PriyaPurkayastha]

@ajcvickers in terms of next steps in order to close this issue, do you want to document this as a known issue for apps that use EF Core 2.1 and try to run on .NET5? We can track this on the thread for .NET 5 Preview 1 known issues that Vivek sent out.

Additionally, we will remove this app from the test suite that we run for .NET 5 but continue to include this for .NET Core 2.x Servicing runs.

[ajcvickers]

@PriyaPurkayastha I don't believe this is necessary.

[PriyaPurkayastha]

All teams have agreed to the compatibility principles for .NET 5 that were shared during .NET Core tactics as part of the Fundamentals effort. Summarizing it here for those not in that meeting:
.NET Core 3.x/2.x apps should by and large, continue to work with .NET Core 3.x(later minor versions). Customers need to opt-in to major version upgrade (e.g. .NET 5) if desired runtime, as described by Minor Version Selection for Runtime binding, is not present but later major versions are available. Intentional breaking changes causing apps to behave differently, should be documented and guidance should be available describing steps that customers need to take to get their apps working again.
Earlier on this thread, EF Core breaking change documentation was mentioned as a possible resolution to this issue. This could either be EF Core breaking change documentation or EF Core known issue.
Given that we have had lengthy discussions and are not able to arrive at a consensus on this issue, I am adding QB for .NET 5 release @SteveCarrollMSFT to weigh in on this.

[ajcvickers]

@PriyaPurkayastha To be clear again, this is not an EF breaking change in any way. This is a bug in EF Core 2.1 that prevents it working for this particular query on .NET 5, which isn't recommended anyway. Also, we have no evidence that it is impacting customers in any way.

To me, this is a clear example of runaway process clouding sensible decisions. We don't need to do anything here. If the process says we do, then the process is wrong.