-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix build with latest roslyn #35149
Fix build with latest roslyn #35149
Conversation
@@ -427,10 +427,10 @@ public virtual async Task Parameter_collection_of_strings_Contains_nullable_stri | |||
|
|||
await AssertQuery( | |||
async, | |||
ss => ss.Set<PrimitiveCollectionsEntity>().Where(c => strings.Contains(c.NullableString))); | |||
ss => ss.Set<PrimitiveCollectionsEntity>().Where(c => strings.Any(s => s == c.NullableString))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change probably needs to be reverted - the specific point of this test is to test LINQ translation of Contains, not Any (so the two are not really equivalent in this context)... I understand there's a (hopefully temporary) problem, but we should disable the test or similar rather than change to Any...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @am11 @ViktorHofer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good point. Here is a better fix for CS8620 from C# 14 compiler: #35166. It has became a bit more pedantic in nullable overload resolution for collection types (which I believe is by design). dotnet/aspnetcore has already moved to .NET 10 in global.json, if you want, I can bump the version here as well so we can see these errors in CI. LMK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting #35166 @am11, looks good.
We generally bump dotnet SDK version on with previews, to avoid too much churn and also make it easier for external contributors to work with our repo (i.e. without having to install daily builds of the SDK etc.).
@AndriySvyryd any thoughts here? We can just wait for preview.1 and deal with this then...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roji just for context, efcore builds as part of the VMR and there we already use a nightly .NET 10 SDK. Therefore we had to submit a few PRs like this to enable the repository to build with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thanks @ViktorHofer, that does make things clearer!
Unblocks dotnet/sdk#44843
Similar to dotnet/runtime#109467.
cc @ViktorHofer