WIP : FSharp.Core 'query' related fixes #19243
Open
+1,331
−210
Conversation
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
Contributor
|
T-Gro
added
the
NO_RELEASE_NOTES
Fixes #11131 and #15648 When anonymous records or regular F# records had fields specified in a non-alphabetical order, the LINQ expression converter would generate a lambda-invoke pattern like (v => body).Invoke(e) instead of a clean constructor call. This pattern is not understood by LINQ providers like Entity Framework Core, causing translation failures. The root cause was in the Let binding handling in LeafExpressionConverter. When F# generates a quotation for non-alphabetically ordered record fields, it creates Let bindings to reorder the values. The old code converted these Let bindings into lambda+invoke patterns. The fix changes the Let case to inline the bound expression directly instead of creating a lambda. This is safe for query expressions because the expressions are side-effect free. Changes: - Linq.fs: Modified Let case to inline bindings instead of using Invoke - Added 4 new tests for field ordering scenarios: - Anonymous records with non-alphabetical field order - Nested anonymous records - F# records with non-declaration field order - Verification that both orderings produce equivalent results
The ArrayLookupQ pattern in Linq.fs expected 3 type parameters but GetArray only has 1, causing the pattern to never match. This resulted in array access like x.u.[0] being translated as GetArray(x.u, 0) method calls instead of proper array index expressions. Fixed by changing GenericArgs [|_; _; _|] to GenericArgs [|_|]. Now array access in LINQ expressions generates x.u[0] instead of GetArray(x.u, 0), enabling LINQ providers like Azure Cosmos DB to translate array indexing correctly.
Add Equals and GetHashCode implementations to AnonymousObject types in MutableTuple.fs to enable proper structural equality comparison for tuple join keys and groupBy keys. Root cause: AnonymousObject types (used to represent tuples in LINQ query translation) did not implement Equals/GetHashCode, causing join operations to use reference equality instead of structural equality. This resulted in inline tuple joins like join b on ((a.Id1, a.Id2) = (b.Id1, b.Id2)) returning no matches even when data should match. The fix implements Equals using EqualityComparer<T>.Default for proper generic equality comparison, and GetHashCode using a consistent hash combining algorithm. Changes: - src/FSharp.Core/MutableTuple.fs: Add Equals/GetHashCode to all AnonymousObject generic types (1-8 type parameters) - tests/FSharp.Core.UnitTests: Add new QueryTests.fs with 10 tests verifying join and groupBy with tuple keys - Update surface area baselines for new public methods - Update release notes
- Use Queryable.Select instead of Enumerable.Select + AsQueryable() when the source is IQueryable, preserving query composition - This enables async operations like ToListAsync() in Entity Framework Core - Add 7 new tests verifying tuple select composability
#3445) - Add handlers for Sequential, VarSet, FieldSet, PropertySet patterns in Linq.fs - Fix EvaluateQuotation to handle unit return type using Action delegates - Fix query conditionals without else branch by extracting element type for MakeEmpty - Document #3845 headOrDefault NRE as known limitation (requires compiler warning) - Add 9 new tests covering all scenarios
Issue #422 is more complex than initially scoped. The FS1182 warning fires for query expression variables used in projection lambdas (like where/sortBy) but not in the final select. Root cause: Query translation creates two separate Val objects for the same pattern - varSpace Vals for tracking, and Lambda Vals during typechecking. Marking varSpace Vals as referenced doesn't help the Lambda Vals. Proper fix requires deeper compiler changes (Val sharing, query-specific warning suppression, or modified typechecking approach). Workaround: Users can prefix query variables with underscore. The warning is off by default (only with --warnon:1182), so impact is limited. This completes the Sprint 5 DoD. Sprint 6 (Issue #422) is documented as a known limitation requiring future investigation.
- Mark synthetic lambda parameters in query translation as compiler-generated to suppress false 'unused variable' warnings (FS1182) - Add helper functions markSimplePatAsCompilerGenerated and markSimplePatsAsCompilerGenerated in CheckComputationExpressions.fs - Update mkSimplePatForVarSpace to use mkSynCompGenSimplePatVar - Mark join/groupJoin/zip patterns as compiler-generated - Add 5 new tests verifying query variable usage patterns - Update Project12 test baseline for compgen symbol attributes This fixes the issue where variables bound with 'for' in query expressions would trigger FS1182 warnings when used in where/let/select clauses but not directly in the for pattern's scope.
Add tests for: - T1.1: VarSet - mutable variable assignment: `<@ let mutable x = 1; x <- 2; x @>` - T1.2: FieldSet - mutable field assignment - T1.3: PropertySet - settable property assignment - T1.4: indexed PropertySet - array index assignment Fix: Handle mutable let bindings properly in ConvExprToLinqInContext by creating a ParameterExpression (which is writeable) for mutable variables instead of inlining the expression. This allows VarSet (x <- value) to work correctly. All 7 EvaluateQuotation tests pass, including 4 new mutation tests.
Add two tests verifying EvaluateQuotation handles deeply nested let bindings: - Simple passthrough: let a = x in let b = a in let c = b in c - With computation: let a = 1 in let b = a+1 in let c = b+1 in let d = c+1 in d These tests verify the let-binding inlining logic doesn't break with deep nesting.
- Mark T1.1-T1.11 (mutation, field order, groupBy tests) as done - Mark I2.6-I2.7 (field order fix) as done - Mark I2.1-I2.5 (#3845) as 'Known limitation - requires compiler warning' - Mark Q3.1-Q3.3 as 'Deferred - acceptable tech debt' - Mark Q3.5 (nested let test) as done - Mark C4.4-C4.9 (compat tests) as done where applicable - Mark D5.1 (release notes), V5.5-V5.7 (formatting, baselines, issue refs) as done - Update Issue-to-Task Mapping table with correct status - Add status notes explaining rationale for deferred/N/A items
- Ran ILVerify on FSharp.Core and FSharp.Compiler.Service - All 8 configurations pass with no baseline updates needed - C4.1: ILVerify passes for all targets - C4.2: Regression tests cover binary compatibility - C4.3: API documented in surface area baselines
- D5.2: Add inline comment at line 626 in Linq.fs explaining #16918 fix (GetArray has 1 type param, pattern expects GenericArgs [|_|]) - Q3.6: Add test with 15 nested let bindings verifying O(n) performance - Update TASKLIST.md: Mark Q3.4, D5.2, Q3.6 complete; D5.3, V5.4 as N/A Build: ✅ 6050 tests passed, 0 failed Formatting: ✅ dotnet fantomas . --check passes
Query expression tests for issue #422 belong in the Language/ComputationExpressionTests.fs file, not in CompilerOptions/fsc/warnon. The tests verify that synthetic lambda parameters in query translation are properly marked as compiler-generated to prevent false FS1182 warnings.
Documents the following breaking changes for release notes: - AnonymousObject structural equality (HIGH) - Expression tree structure for let-bindings (MEDIUM) - Expression tree structure for array indexing (MEDIUM) - IQueryable type preservation (MEDIUM) - FCS Symbol API IsCompilerGenerated change (LOW)
Adds concrete C# code examples showing how to update custom ExpressionVisitor implementations for: - Let-binding detection (Lambda.Invoke → inlined) - Array access handling (GetArray → ArrayIndex) - Compatibility pattern supporting both old and new FSharp.Core
… identity The inlining approach was incorrect because it evaluated expressions multiple times when the variable was used more than once. This broke reference semantics for expressions like 'let x = obj() in PhysicalEquality x x'. Using Expression.Block with proper variable scoping: 1. Evaluates the expression exactly once 2. Preserves reference identity 3. Avoids Lambda.Invoke which LINQ providers can't translate
Previous baseline update incorrectly included ReadOnlySpan and TaskBuilderBase.Using APIs that are not part of this PR. Reset baselines from main and add only the AnonymousObject Equals/GetHashCode methods that this PR actually introduces.
Add null check before calling FSharpType.IsUnion on BaseType. Interface types have null BaseType, which caused ArgumentNullException.
1. Extract getUnionCaseCoercionType helper - used by both PropertyGet and PropertySet - Fixes null BaseType bug in PropertyGet (was pre-existing) - Removes duplicated coercion logic 2. Extract compileAndInvoke helper in EvaluateQuotation - Removes duplicated compile/invoke/exception handling code - Unit and non-unit paths now share common infrastructure
The IfThenElse handler now fails fast if t.Type is not generic, rather than silently using the wrong type. This is consistent with line 1630 which also assumes the type is generic. In practice, t.Type is always IQueryable<T> or IEnumerable<T> (always generic) because F# queries require generic sources.
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.
Summary
Fixes 10 query expression bugs affecting LINQ provider compatibility (EF Core, Cosmos DB, etc.).
Fixed Issues
Expression Tree Generation (
src/FSharp.Core/Linq.fs)Lambda.InvokepatternQueryTests.fs— field order testsArrayIndexinstead ofGetArrayArrayLookupQpattern match (1 type param, not 3)FSharpQuotations.fs— 3 array indexing testsLeafExpressionConverter.EvaluateQuotationfails to handle some cases #19099 —EvaluateQuotationfails for Sequential/VarSet/unitSequential,VarSet,FieldSet,PropertySetQueryTests.fs— 7 EvaluateQuotation testsQuery Translation (
src/FSharp.Core/Query.fs)Queryable.Selectinstead ofEnumerable.Select+AsQueryableQueryTests.fs— IQueryable preservation testsMakeEmptyinIfThenElseQueryTests.fs— conditional query testsAnonymousObject Equality (
src/FSharp.Core/MutableTuple.fs)Equals/GetHashCodetoAnonymousObject<T>typesQueryTests.fs— tuple join/groupBy testsCompiler (
src/Compiler/.../CheckComputationExpressions.fs)ComputationExpressionTests.fs— 6 FS1182 testsNot Fixed
headOrDefaultwith struct/tuple returns null'T : defaultconstraint (fslang-suggestions#1423)select (Some x)+headOrDefaultwith pattern matchingBreaking Changes
See
BREAKING_CHANGES.md. Summary:AnonymousObject<T>now uses structural equalityIQueryabletype