Skip to content
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

[Breaking change]: C# overload resolution prefers span overloads which might not work in Expression lambdas #43952

Open
1 of 3 tasks
jjonescz opened this issue Dec 12, 2024 · 7 comments
Assignees
Labels
breaking-change Indicates a .NET Core breaking change 🏁 Release: .NET 10 Work items for the .NET 10 release 📌 seQUESTered Identifies that an issue has been imported into Quest.

Comments

@jjonescz
Copy link
Member

jjonescz commented Dec 12, 2024

Description

C# 14 introduces new built-in span conversions and type inference rules making overloads with span parameters applicable in more scenarios.

However, Expression lambdas cannot be interpreted when they involve ref structs like Span and ReadOnlySpan. For example:

using System;
using System.Linq;
using System.Linq.Expressions;

Expression<Func<int[], int, bool>> exp = (array, num) => array.Contains(num);
exp.Compile(preferInterpretation: true); // fails at runtime in C# 14

The array.Contains binds to Enumerable.Contains in C# 13 but binds to MemoryExtensions.Contains in C# 14. The latter involves ReadOnlySpan and hence crashes when Expression.Compile(preferInterpretation: true) is called.

Version

.NET 10 Preview 1

Previous behavior

In C# 13 and earlier, an extension method taking a ReadOnlySpan<T> or Span<T> receiver is not applicable to a value of type T[]. Therefore, only non-span extension methods like the ones from the System.Linq.Enumerable class are usually bound inside Expression lambdas.

New behavior

In C# 14 and later, methods with ReadOnlySpan<T> or Span<T> parameters can participate in type inference or be used as extension methods in more scenarios. This makes span-based methods like the ones from the System.MemoryExtensions class bind in more scenarios, including inside Expression lambdas where they will cause runtime exceptions when compiled with interpretation.

Type of breaking change

  • Binary incompatible: Existing binaries might encounter a breaking change in behavior, such as failure to load or execute, and if so, require recompilation.
  • Source incompatible: When recompiled using the new SDK or component or to target the new runtime, existing source code might require source changes to compile successfully.
  • Behavioral change: Existing binaries might behave differently at run time.

Reason for change

The C# language feature allows simplified API design and usage (e.g., one ReadOnlySpan extension method can apply to both spans and arrays).

Recommended action

If you need to continue using Expression interpretation, you should make sure the non-span overloads are bound, e.g., by casting arguments to the exact types the method signature takes or calling the extension methods as explicit static invocations:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Linq.Expressions;

M((array, num) => array.Contains(num)); // fails, binds to MemoryExtensions.Contains
M((array, num) => ((IEnumerable<int>)array).Contains(num)); // ok, binds to Enumerable.Contains
M((array, num) => array.AsEnumerable().Contains(num)); // ok, binds to Enumerable.Contains
M((array, num) => Enumerable.Contains(array, num)); // ok, binds to Enumerable.Contains

void M(Expression<Func<int[], int, bool>> e) => e.Compile(preferInterpretation: true);

Feature area

Other (please put exact area in description textbox)

Affected APIs

System.Linq.Expressions.Expression.Compile

Associated WorkItem - 360646

@BillWagner
Copy link
Member

Adding myself as an assignee here.

I need to publish https://github.com/dotnet/roslyn/blob/main/docs/compilers/CSharp/Compiler%20Breaking%20Changes%20-%20DotNet%2010.md, which includes this compiler change.

@jjonescz Are there associated library changes as well?

@gewarren gewarren assigned CamSoper and unassigned gewarren Dec 12, 2024
@jjonescz
Copy link
Member Author

jjonescz commented Dec 12, 2024

Are there associated library changes as well?

No, but I was told to file this breaking change issue in dotnet/runtime#109757 (comment)

Note also that the compiler breaking change doc does not describe the break in Expression trees, although the cause is the same and so those docs could be unified I imagine.

@jkotas
Copy link
Member

jkotas commented Dec 12, 2024

exp.Compile(preferInterpretation: true); // fails at runtime in C# 14

With Native AOT and potentially other targets that do not support compilation, this fails at runtime even with preferInterpretation: true omitted.

@davidroth
Copy link

davidroth commented Dec 19, 2024

This is basically a dead end when using classic "EF6" together with latest dotnet/c#. If you want to continue using the latest dotnet with latest lang versions with EF6, you are stuck at C#13. (Unless the EfCore Team implements a similar workaround for EF6 too).

@jjonescz
Copy link
Member Author

This is basically a dead end when using classic "EF6" together with latest dotnet/c#.

Not sure what you mean by "dead end". You should be always able to work around this break by casting the array to IEnumerable, or calling Enumerable.Contains as static method, etc.

@davidroth
Copy link

@jjonescz Yes the .AsEnumerable workaround works. However, when you have an app with thousands of queries you have to check all those queries add AsEnumerable in the correct places. Sure, I can search for .Contains( but I dont see a fully automated solution to just search+replace everything automatically, without manually checking each occurrence.

But yeah, there is a workaround.

@jjonescz
Copy link
Member Author

jjonescz commented Dec 23, 2024

I dont see a fully automated solution to just search+replace everything automatically, without manually checking each occurrence.

There might be an analyzer / code fixer written for this.

But we are also considering a language change to avoid the break. (The "first-class spans" feature that introduced this is still in preview after all.)

@CamSoper CamSoper added 🗺️ reQUEST Triggers an issue to be imported into Quest. and removed ⌚ Not Triaged Not triaged labels Jan 7, 2025
@dotnetrepoman dotnetrepoman bot added the 🗺️ mapQUEST Only used as a way to mark an issue as updated for quest. RepoMan should instantly remove it. label Jan 7, 2025
@dotnet-policy-service dotnet-policy-service bot removed the 🗺️ mapQUEST Only used as a way to mark an issue as updated for quest. RepoMan should instantly remove it. label Jan 7, 2025
@sequestor sequestor bot added 📌 seQUESTered Identifies that an issue has been imported into Quest. and removed 🗺️ reQUEST Triggers an issue to be imported into Quest. labels Jan 8, 2025
@dotnetrepoman dotnetrepoman bot added the 🗺️ mapQUEST Only used as a way to mark an issue as updated for quest. RepoMan should instantly remove it. label Jan 8, 2025
@dotnet-policy-service dotnet-policy-service bot removed the 🗺️ mapQUEST Only used as a way to mark an issue as updated for quest. RepoMan should instantly remove it. label Jan 8, 2025
@dotnetrepoman dotnetrepoman bot added the 🗺️ mapQUEST Only used as a way to mark an issue as updated for quest. RepoMan should instantly remove it. label Jan 9, 2025
@BillWagner BillWagner moved this from 🔖 Ready to 🏗 In progress in dotnet/docs January 2025 sprint project Jan 9, 2025
@dotnet-policy-service dotnet-policy-service bot removed the 🗺️ mapQUEST Only used as a way to mark an issue as updated for quest. RepoMan should instantly remove it. label Jan 9, 2025
BillWagner added a commit to BillWagner/docs that referenced this issue Jan 9, 2025
Contributes to dotnet#43952

Add the configuration to publish the roslyn breaking changes documents for changes since the .NET 9 GA.
BillWagner added a commit that referenced this issue Jan 9, 2025
* Fixes #44167

The `skiplocalsinit` flag is a compiler feature that is already documented in the compiler switches section. It's not a *language feature*, so remove the speclet from publication in the standard section.

* Fixes #44189

Contributes to #43952

Add the configuration to publish the roslyn breaking changes documents for changes since the .NET 9 GA.
@gewarren gewarren added the 🏁 Release: .NET 10 Work items for the .NET 10 release label Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Indicates a .NET Core breaking change 🏁 Release: .NET 10 Work items for the .NET 10 release 📌 seQUESTered Identifies that an issue has been imported into Quest.
Projects
Status: 🏗 In progress
Development

No branches or pull requests

6 participants