From 1cc2cf60a3fe8b2e30b644e00ce96edb683e585d Mon Sep 17 00:00:00 2001 From: "Jeremy D. Miller" Date: Tue, 2 Jun 2026 10:59:22 -0500 Subject: [PATCH] Fix #4599: replace IsCompilableExpression heuristic with structural check EnumerableContains.Parse (and the HashSet sibling) crashed for receivers built as Property(Constant(), "") when the wrapper type overrides ToString() -- e.g. HotChocolate's ExpressionParameter driving [UseFiltering] `in`. ConstantExpression.ToString() only wraps in "value()" when the value uses the default Object.ToString(); with a custom override, it prints the custom string directly. The IsCompilableExpression heuristic (node.Expression.ToString().StartsWith("value(")) then returned false, the receiver was wrongly rejected by TryToParseConstant, and the parser fell into ParseWhereForContains -> ReduceToConstant on the *value-side* expression. That expression still referenced the outer Where lambda parameter, so FEC compiled a parameter-less lambda with a free parameter inside and crashed with "variable 'x' of type '' referenced from scope '', but it is not defined". - IsCompilableExpression: walk the member chain to its root and accept if the root is a ConstantExpression (or null, for static members). Structural; no string heuristic. Accepts the previously-rejected programmatic-receiver shape. - ReduceToConstant: add a free-parameter guard (visitor that ignores parameters bound by lambdas *within* the expression). If a free parameter is found, throw a clear BadLinqExpressionException naming it rather than letting FEC surface the confusing scope-binding message. Safety net for any other call site that might mistakenly feed a parameter-bearing subtree. Tests cover both Enumerable.Contains and HashSet.Contains receiver shapes against Postgres; the previously-failing repros now pass. Full LinqTests Contains/Where/GroupBy/Any/Modulo sweep (437 tests) still green. Co-Authored-By: Claude Opus 4.7 (1M context) --- ...umerable_contains_programmatic_receiver.cs | 129 ++++++++++++++++++ .../Linq/Parsing/LinqInternalExtensions.cs | 66 ++++++++- 2 files changed, 192 insertions(+), 3 deletions(-) create mode 100644 src/LinqTests/Bugs/Bug_4599_enumerable_contains_programmatic_receiver.cs diff --git a/src/LinqTests/Bugs/Bug_4599_enumerable_contains_programmatic_receiver.cs b/src/LinqTests/Bugs/Bug_4599_enumerable_contains_programmatic_receiver.cs new file mode 100644 index 0000000000..551695db9a --- /dev/null +++ b/src/LinqTests/Bugs/Bug_4599_enumerable_contains_programmatic_receiver.cs @@ -0,0 +1,129 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Linq.Expressions; +using System.Threading.Tasks; +using Marten; +using Marten.Testing.Harness; +using Shouldly; + +namespace LinqTests.Bugs; + +// Top-level types so the generated mt_doc_* table name stays under the 63-byte +// PostgreSQL identifier limit (the test class name itself is long). +// +// Note: the ToString() override is the trigger. ConstantExpression.ToString() wraps +// a value in "value()" only when the value's ToString() is the default +// Object.ToString(); when the type overrides ToString(), the custom string is used +// directly. IsCompilableExpression's "value(" string-prefix check then returns false +// and the receiver is wrongly rejected. HotChocolate's ExpressionParameter has a +// custom ToString() for diagnostics, which is why their `[UseFiltering]` `in` +// operator triggers this in practice. +public sealed class Bug4599ValueHolder +{ + public IEnumerable p { get; } + public Bug4599ValueHolder(IEnumerable values) => p = values; + public override string ToString() => $"Bug4599ValueHolder[{string.Join(',', p)}]"; +} + +public sealed class Bug4599HashSetHolder +{ + public HashSet p { get; } + public Bug4599HashSetHolder(HashSet values) => p = values; + public override string ToString() => $"Bug4599HashSetHolder[{string.Join(',', p)}]"; +} + +public class Bug4599Doc +{ + public Guid Id { get; set; } + public string Name { get; set; } = ""; +} + +// https://github.com/JasperFx/marten/issues/4599 +// +// EnumerableContains.Parse crashes with +// "variable '' of type '' referenced from scope '', but it is not defined" +// when the Contains() receiver is built programmatically as +// Property(Constant(), "") and the wrapper overrides ToString(). +// HotChocolate's [UseFiltering] `in` operator is one consumer that produces this +// shape via FilterExpressionBuilder.CreateAndConvertParameter. +// +// Each test uses its own DocumentStore with a unique schema so that parallel +// multi-target test runs (net8.0 / net9.0 / net10.0) don't interfere through a +// shared mt_doc_bug4599doc table. +public class Bug_4599_enumerable_contains_programmatic_receiver +{ + private static IDocumentStore BuildStore(string schema) + { + // Include the runtime major version so parallel multi-target test runs + // (net8.0 / net9.0 / net10.0) don't collide on the same schema in the + // shared marten_testing DB. + var suffix = Environment.Version.Major; + return DocumentStore.For(opts => + { + opts.Connection(ConnectionSource.ConnectionString); + opts.DatabaseSchemaName = $"{schema}{suffix}"; + }); + } + + private static async Task storeThreeDocs(IDocumentStore store) + { + await store.Advanced.Clean.CompletelyRemoveAllAsync(); + await using var session = store.LightweightSession(); + session.Store(new Bug4599Doc { Id = Guid.NewGuid(), Name = "a" }); + session.Store(new Bug4599Doc { Id = Guid.NewGuid(), Name = "b" }); + session.Store(new Bug4599Doc { Id = Guid.NewGuid(), Name = "c" }); + await session.SaveChangesAsync(); + } + + [Fact] + public async Task enumerable_contains_with_programmatic_receiver_should_query_correctly() + { + await using var store = BuildStore("b4599e"); + await storeThreeDocs(store); + + var holder = new Bug4599ValueHolder(new[] { "a", "b" }); + + // Build: doc => holder.p.Contains(doc.Name) + // -- the receiver is Property(Constant(holder), "p"), NOT a compiler-emitted closure. + var doc = Expression.Parameter(typeof(Bug4599Doc), "doc"); + var nameAccess = Expression.Property(doc, nameof(Bug4599Doc.Name)); + var receiver = Expression.Property(Expression.Constant(holder), nameof(Bug4599ValueHolder.p)); + var containsCall = Expression.Call( + typeof(Enumerable), + nameof(Enumerable.Contains), + [typeof(string)], + receiver, + nameAccess); + var predicate = Expression.Lambda>(containsCall, doc); + + await using var query = store.QuerySession(); + var results = await query.Query().Where(predicate).ToListAsync(); + + results.Select(x => x.Name).OrderBy(x => x) + .ShouldHaveTheSameElementsAs("a", "b"); + } + + [Fact] + public async Task hashset_contains_with_programmatic_receiver_should_query_correctly() + { + await using var store = BuildStore("b4599h"); + await storeThreeDocs(store); + + var holder = new Bug4599HashSetHolder(new HashSet { "a", "b" }); + + // Build: doc => holder.p.Contains(doc.Name) using HashSet.Contains (instance method) + var doc = Expression.Parameter(typeof(Bug4599Doc), "doc"); + var nameAccess = Expression.Property(doc, nameof(Bug4599Doc.Name)); + var receiver = Expression.Property(Expression.Constant(holder), nameof(Bug4599HashSetHolder.p)); + var containsMethod = typeof(HashSet).GetMethod(nameof(HashSet.Contains), [typeof(string)])!; + var containsCall = Expression.Call(receiver, containsMethod, nameAccess); + var predicate = Expression.Lambda>(containsCall, doc); + + await using var query = store.QuerySession(); + var results = await query.Query().Where(predicate).ToListAsync(); + + results.Select(x => x.Name).OrderBy(x => x) + .ShouldHaveTheSameElementsAs("a", "b"); + } +} diff --git a/src/Marten/Linq/Parsing/LinqInternalExtensions.cs b/src/Marten/Linq/Parsing/LinqInternalExtensions.cs index 5beaae83c7..03777e3455 100644 --- a/src/Marten/Linq/Parsing/LinqInternalExtensions.cs +++ b/src/Marten/Linq/Parsing/LinqInternalExtensions.cs @@ -205,11 +205,24 @@ public static ISqlFragment ReduceToValue(this IQueryableMemberCollection collect public static bool IsCompilableExpression(this MemberExpression node) { - // Field of the containing code + // A MemberExpression is reducible to a constant at parse time iff its inner + // subtree contains no free ParameterExpression -- which correctly covers both + // compiler-emitted closure captures (Property(Constant(), "p")) + // *and* shapes whose inner is a MethodCall over closure-captured values + // (e.g. Member(Call(ElementAt, [Member(Constant(), "list"), + // Constant(2)]), "Date")). + // + // The previous heuristic gated on `node.Expression.ToString().StartsWith("value(")`, + // which incidentally covered MethodCall shapes whose printed form started with + // the closure access, *but* missed programmatic receivers whose wrapper type + // overrides ToString() (the #4599 trigger). The "no free parameter" structural + // check is the precise predicate the heuristic was approximating and handles + // both cases correctly. if (node.Expression == null) return true; - return (node.Expression is ConstantExpression || node.Expression != null) && - node.Expression.ToString().StartsWith("value("); + var finder = new FreeParameterFinder(); + finder.Visit(node.Expression); + return finder.Found == null; } public static bool TryToParseConstant(this Expression? expression, [NotNullWhen(true)]out ConstantExpression? constant) @@ -337,6 +350,22 @@ public static ConstantExpression ReduceToConstant(this Expression expression) return constantExpression; } + // Guard: if the expression references a free ParameterExpression (one not + // bound by a lambda *inside* the expression), wrapping it in our parameter-less + // Lambda> below produces a body with unbound parameters, and + // FastExpressionCompiler crashes with the confusing + // "variable 'x' of type 'Doc' referenced from scope '', but it is not defined" + // message. That symptom (see #4599) is almost always a Linq-parser bug at the + // call site; surface a clear error instead of leaking the FEC message. + var freeParameterFinder = new FreeParameterFinder(); + freeParameterFinder.Visit(expression); + if (freeParameterFinder.Found != null) + { + var p = freeParameterFinder.Found; + throw new BadLinqExpressionException( + $"Marten cannot reduce the expression '{expression}' to a constant because it references the free parameter '{p.Name}' of type '{p.Type.Name}'. This usually indicates a Linq parser issue at the call site."); + } + var lambdaWithoutParameters = Expression.Lambda>(Expression.Convert(expression, typeof(object))); var compiledLambda = FastExpressionCompiler.ExpressionCompiler.CompileFast(lambdaWithoutParameters); @@ -352,4 +381,35 @@ public static ConstantExpression ReduceToConstant(this Expression expression) "Error while trying to find a value for the Linq expression " + expression, e); } } + + /// + /// Walks an expression looking for the first + /// that is *not* bound by a lambda inside the expression itself. Used by + /// as a safety net against feeding parameter-bearing + /// subtrees into the parameter-less lambda + FEC compile path (see #4599). + /// + private sealed class FreeParameterFinder: ExpressionVisitor + { + private readonly HashSet _bound = new(); + + public ParameterExpression? Found { get; private set; } + + protected override Expression VisitLambda(Expression node) + { + foreach (var p in node.Parameters) _bound.Add(p); + var result = base.VisitLambda(node); + foreach (var p in node.Parameters) _bound.Remove(p); + return result; + } + + protected override Expression VisitParameter(ParameterExpression node) + { + if (Found == null && !_bound.Contains(node)) + { + Found = node; + } + + return node; + } + } }