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 doc-idea Indicates issues that are suggestions for new topics [org][type][category] Pri1 High priority, do before Pri2 and Pri3 ⌚ Not Triaged Not triaged

Comments

@jjonescz
Copy link
Member

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
@jjonescz jjonescz added breaking-change Indicates a .NET Core breaking change doc-idea Indicates issues that are suggestions for new topics [org][type][category] Pri1 High priority, do before Pri2 and Pri3 labels Dec 12, 2024
@dotnetrepoman dotnetrepoman bot added ⌚ Not Triaged Not triaged labels Dec 12, 2024
@BillWagner BillWagner self-assigned this Dec 12, 2024
@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.)

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 doc-idea Indicates issues that are suggestions for new topics [org][type][category] Pri1 High priority, do before Pri2 and Pri3 ⌚ Not Triaged Not triaged
Projects
None yet
Development

No branches or pull requests

6 participants