Skip to content

Conversation

roji
Copy link
Member

@roji roji commented Jul 28, 2025

In the latest versions of the SDK I've noticed CS9236 being suggested on some test methods; this PR adds explicit typing information to the offending lambdas and makes CS9236 an error.

This knocks off ~3 seconds (~10%) from the compilation time of EFCore.Specification.Tests 🎉 :

Test commandline (in test/EFCore.Specification.Tests):

hyperfine --warmup 1 'touch EFCore.Specification.Tests.csproj && dotnet build-server shutdown && dotnet build'

BEFORE CHANGE:
Benchmark 1: touch EFCore.Specification.Tests.csproj && dotnet build-server shutdown && dotnet build
  Time (mean ± σ):     31.553 s ±  7.155 s    [User: 2.457 s, System: 0.612 s]
  Range (min … max):   26.723 s … 51.103 s    10 runs

AFTER CHANGE: 
Benchmark 1: touch EFCore.Specification.Tests.csproj && dotnet build-server shutdown && dotnet build
  Time (mean ± σ):     28.481 s ±  1.640 s    [User: 2.093 s, System: 0.591 s]
  Range (min … max):   26.138 s … 30.825 s    10 runs

/cc @artl93

@@ -1833,6 +1833,7 @@ public virtual Task Where_query_composition3(bool async)
where c1.City == ss.Set<Customer>().OrderBy(c => c.CustomerID).First(c => c.IsLondon).City
select c1));

#pragma warning disable CS9236 // Compiling requires binding the lambda expression at least 200 times
Copy link
Member Author

Choose a reason for hiding this comment

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

/cc @jaredpar @CyrusNajmabadi

Here I got a CS9236, but oddly adding the typing information didn't make it go away (SDK 10.0.100-preview.6.25358.103). A naive attempt at a minimal repro didn't work (see code below). Let me know if there are known false positives like this or how I can help.

Failed attempted repro
await using var context = new CustomerContext();
await context.Database.EnsureDeletedAsync();
await context.Database.EnsureCreatedAsync();

await AssertTranslationFailed(
    () => AssertQuery(
        async: true,
        ss => from c1 in ss.Set<Customer>().OrderBy(c => c.CustomerID).Take(2)
                where c1.City
                    == (from c2 in ss.Set<Customer>().OrderBy(c => c.CustomerID)
                        // from c3 in ss.Set<Customer>().OrderBy(bool (Customer c) => c.IsLondon).ThenBy(string (Customer c) => c.CustomerID)
                        from c3 in ss.Set<Customer>().OrderBy(c => c.IsLondon).ThenBy(string (Customer c) => c.CustomerID)
                        select new { c3 }).First().c3.City
                select c1));

static Task AssertTranslationFailed(Func<Task> query)
    => throw new NotImplementedException();

Task AssertQuery<TResult>(
        bool async,
        Func<CustomerContext, IQueryable<TResult>> query,
        Func<TResult, object>? elementSorter = null,
        Action<TResult, TResult>? elementAsserter = null,
        bool assertOrder = false,
        bool assertEmpty = false,
        QueryTrackingBehavior? queryTrackingBehavior = null,
        [CallerMemberName] string testMethodName = "")
        => throw new NotImplementedException();

public class CustomerContext : DbContext;

public class Customer
{
    public required string CustomerID { get; set; }
    public required string City { get; set; }
    public bool IsLondon { get; set; }
}

Copy link
Member

Choose a reason for hiding this comment

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

it's possible the outer from/where and the selects are themselves the ones having issues. you may have to write this without linq entirely.

@roji roji enabled auto-merge (squash) July 28, 2025 16:08
@roji roji self-assigned this Jul 28, 2025
@roji roji merged commit 0b27eb0 into dotnet:main Jul 28, 2025
7 checks passed
@roji roji deleted the ExplicitLambdas branch July 28, 2025 17:26
@roji
Copy link
Member Author

roji commented Jul 29, 2025

Follow-up for a case where CS9236 is being reported even when lambda parameters/return type are explicitly specified: dotnet/roslyn#79650. The minimal repro there is based on one of the two EF tests where the diagnostic was suppressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants