Repository navigation
Make ThenBy require an ordered source - #96
Merged
Merged
Conversation
The interceptor emitter hard-coded `this IExpressiveQueryable<T> source` and the `IExpressiveQueryable<T>` return in all four output templates, so a stub declared on a derived interface could not be intercepted: Roslyn requires an interceptor's signature to match the intercepted method's declared types. Derive both from the stub symbol instead. `method` is the reduced extension symbol, whose Parameters[0] is the first non-receiver parameter, so the declared receiver comes from ReducedFrom. No behaviour change: every existing stub declares `this IExpressiveQueryable<T>` and returns either `IExpressiveQueryable<T>` or a scalar, so all four templates emit byte-identical output. Verified by the snapshot suite — 474 tests, zero baselines moved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ThenBy/ThenByDescending were declared on IExpressiveQueryable<T>, so the compiler allowed them anywhere in a chain — including where no ordering existed. Every such shape failed at runtime instead: .OrderBy().Include().ThenBy() InvalidCastException .OrderBy().AsNoTracking().ThenBy() ArgumentException .Where().ThenBy() ArgumentException Model orderedness in the type system with a zero-member marker interface that exists only to steer overload resolution, mirroring EF Core's IIncludableQueryable. All three shapes are now compile errors, on every provider, with no runtime machinery. - IOrderedExpressiveQueryable<T> : IExpressiveQueryable<T>, IOrderedQueryable<T> - OrderBy/OrderByDescending return it; ThenBy/ThenByDescending require it - AsExpressive gains an IOrderedQueryable<T> overload, so the generated ordering interceptors re-wrap with no generator-side factory selection, and source.OrderBy(...).AsExpressive().ThenBy(...) keeps working - ExpressiveMongoQueryable<T> implements the marker so ordering does not re-wrap it and drop IAsyncCursorSource<T> - EXP0029's sibling-stub search now matches derived receivers, otherwise it stops deferring to EXP0026 and both fire on the same call This makes the surface match System.Linq.Queryable and EF Core rather than diverging from them, so it needs no user-facing documentation; only the provider-author contract (a custom IExpressiveQueryable<T> wrapper should implement the marker) is new, and that is documented in custom-providers. Deliberately no ordered x includable cross-product interface: Include and the chain-continuity operators must precede the ordering, exactly as plain EF Core requires. An ordered overload of AsNoTracking would compile and then throw, since its expression node is typed IQueryable<T>. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The change is cohesive, aligns with established LINQ ordering semantics, and is backed by targeted unit/integration/compiler/snapshot coverage across runtime, generator, analyzers, and provider integrations.
Review effort: Lite
Findings: None
What changed in this PR
This PR tightens the ordering contract of Expressive LINQ chains by introducing an ordered marker interface (IOrderedExpressiveQueryable<T>) so ThenBy/ThenByDescending become compile-time constrained to ordered sources, eliminating the previously observed runtime failures and aligning behavior with System.Linq/EF Core ordering semantics.
Changes:
- Added
IOrderedExpressiveQueryable<T>and updated ordering stubs soOrderBy*returns an ordered source andThenBy*requires one. - Updated
AsExpressivewith anIOrderedQueryable<T>overload and adjusted wrappers/providers to preserve orderedness and provider-specific interfaces across ordering. - Updated the polyfill interceptor generator + analyzers accordingly, and added coverage across unit, integration, and generator snapshot tests (plus docs).
| File | Description |
|---|---|
| tests/ExpressiveSharp.Tests/Extensions/ExpressiveQueryableExtensionsTests.cs | Adds unit tests validating ordered AsExpressive wrapping/preservation behavior. |
| tests/ExpressiveSharp.MongoDB.IntegrationTests/Tests/PolyfillInterceptorTests.cs | Adds regression test ensuring ordered chains keep Mongo cursor source behavior working with ToListAsync. |
| tests/ExpressiveSharp.IntegrationTests/Tests/OrderingInterceptorTests.cs | New integration tests validating OrderBy/ThenBy composition semantics over expressive queryables. |
| tests/ExpressiveSharp.Generator.Tests/PolyfillInterceptorGenerator/SingleLambdaQueryableTests.ThenByDescending_GeneratesInterceptor.verified.txt | Updates snapshot to reflect ordered receiver/return types in generated interceptors. |
| tests/ExpressiveSharp.Generator.Tests/PolyfillInterceptorGenerator/SingleLambdaQueryableTests.OrderByDescending_GeneratesInterceptor.verified.txt | Updates snapshot to reflect ordered return type for OrderByDescending. |
| tests/ExpressiveSharp.Generator.Tests/PolyfillInterceptorGenerator/OrderByTests.ThenBy_CastsToIOrderedQueryable.verified.txt | Updates snapshot to reflect ordered receiver/return types for ThenBy. |
| tests/ExpressiveSharp.Generator.Tests/PolyfillInterceptorGenerator/OrderByTests.OrderBy_GeneratesInterceptor.verified.txt | Updates snapshot to reflect ordered return type for OrderBy. |
| tests/ExpressiveSharp.Generator.Tests/PolyfillInterceptorGenerator/ComparerOverloadTests.OrderBy_WithComparer_GeneratesInterceptor.verified.txt | Updates snapshot to reflect ordered return type for comparer overload. |
| tests/ExpressiveSharp.Generator.Tests/PolyfillInterceptorGenerator/AnonymousElementTests.OrderByDescending_AfterAnonymousSelect_GeneratesInterceptor.verified.txt | Updates snapshot to cast/wrap as ordered expressive queryable when ordering an anonymous element. |
| tests/ExpressiveSharp.Generator.Tests/PolyfillInterceptorGenerator/OrderedChainBindingTests.cs | New compilation-binding tests enforcing that ThenBy is only available on ordered expressive chains. |
| tests/ExpressiveSharp.Generator.Tests/CodeFixers/ExpressiveQueryableDropoutAnalyzerTests.cs | Adds analyzer regression test for ThenBy on ordered expressive types to avoid duplicate diagnostics. |
| tests/ExpressiveSharp.EntityFrameworkCore.IntegrationTests/Infrastructure/IncludeTestBase.cs | Adds EF Core integration tests proving Include + OrderBy + ThenBy composition executes correctly. |
| src/ExpressiveSharp/IOrderedExpressiveQueryable.cs | Introduces ordered marker interface mirroring IOrderedQueryable<T> semantics for expressive chains. |
| src/ExpressiveSharp/Extensions/ExpressiveQueryableLinqExtensions.cs | Updates ordering stub signatures to return/require IOrderedExpressiveQueryable<T>. |
| src/ExpressiveSharp/Extensions/ExpressiveQueryableExtensions.cs | Adds ordered AsExpressive(IOrderedQueryable<T>) overload preserving static orderedness. |
| src/ExpressiveSharp/ExpressiveQueryableWrapper.cs | Updates wrapper to serve as fallback for both AsExpressive overload shapes. |
| src/ExpressiveSharp.MongoDB/Infrastructure/ExpressiveMongoQueryable.cs | Ensures Mongo wrapper implements ordered expressive marker to avoid losing cursor-source interfaces after ordering. |
| src/ExpressiveSharp.Generator/PolyfillInterceptorGenerator.cs | Emits interceptor signatures matching each stub’s declared receiver/return types (supports ordered marker receivers/returns). |
| src/ExpressiveSharp.CodeFixers/ExpressiveQueryableDropoutAnalyzer.cs | Broadens sibling overload discovery to stubs declared on derived markers (e.g., ordered expressive). |
| docs/guide/integrations/custom-providers.md | Documents requirement for provider wrappers to implement ordered expressive marker to avoid re-wrapping losses. |
| CLAUDE.md | Updates architecture notes to reflect ordered marker and declared-type-based interceptor emission. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three shapes compiled and then failed at runtime: .OrderBy().Include().ThenBy() (InvalidCastException), .OrderBy().AsNoTracking().ThenBy() and .Where().ThenBy() (both ArgumentException).
This introduces
IOrderedExpressiveQueryable<T>mimicking EFCore's very ownIOrderedQueryable