Skip to content

Fix build with latest roslyn #35149

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 19, 2024
Merged

Fix build with latest roslyn #35149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions test/EFCore.Specification.Tests/CustomConvertersTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1184,8 +1184,8 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
b.Property(e => e.ByteArray5)
.HasConversion(
new ValueConverter<byte[], byte[]>(
v => v.Reverse().Concat(new byte[] { 4, 20 }).ToArray(),
v => v.Reverse().Skip(2).ToArray()),
v => Enumerable.Reverse(v).Concat(new byte[] { 4, 20 }).ToArray(),
v => Enumerable.Reverse(v).Skip(2).ToArray()),
bytesComparer)
.HasMaxLength(7);

Expand Down
6 changes: 3 additions & 3 deletions test/EFCore.Specification.Tests/Query/PrimitiveCollectionsQueryTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)));
Copy link
Member

@roji roji Nov 20, 2024 •
edited
Loading

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/cc @am11 @ViktorHofer

Copy link
Member Author

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.

Copy link
Member

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...

Copy link
Member

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.

Copy link
Member

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!

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)));
}

[ConditionalTheory]
Expand Down Expand Up @@ -574,7 +574,7 @@ public virtual Task Column_collection_of_nullable_ints_Contains_null(bool async)
public virtual Task Column_collection_of_strings_contains_null(bool async)
=> AssertQuery(
async,
ss => ss.Set<PrimitiveCollectionsEntity>().Where(c => c.Strings.Contains(null)),
ss => ss.Set<PrimitiveCollectionsEntity>().Where(c => c.Strings.Any(s => s == null)),
assertEmpty: true);

[ConditionalTheory]
Expand Down