Fix Expression constant folding inside of MemberInitExpression

[MichalMarsalek]

Pull Request Template

Description

This PR fixes a bug described in #1664 where captured variables in LINQ expressions would not get expanded leading to a {"testName": "Test"}["testName"] expression (instead of the expected "Test") in the translated SQL, leading to an error reported by Cosmos.

However the problem appears to be more general as the issue seems to be that the recursion that partially evaluates expressions (so among other things resolves the captured variables and therefore effectively changes {"testName": "Test"}["testName"] to "Test") terminates whenever it encounters a node of type MemberInitExpression. This explains the difference in behaviour between anonymous types and a class as described in #1664, as anonymous types do not contain the MemberInitExpression node.

This is quite surprising since at least in the way I am used to using this library, vast majority of the expressions defining the document projections have the MemberInitExpression as the very root of the body of the Expression<Func<T1, T2>>, meaning that in this common case no constant folding would ever happen. Is that even possible? Or what am I missing here?

Type of change

• Bug fix (non-breaking change which fixes an issue)

Closing issues

To automatically close an issue: closes #1664

[NaluTripician]

Hi @MichalMarsalek, thank you for the contribution! This PR has been idle for ~285 days. Is this change still needed on your side? Flagging for SDK-team triage as part of an open-PR cleanup pass — we'd like to give you a clear yes/no rather than let it sit.

[azure-pipelines]

Azure Pipelines:
2 pipeline(s) require an authorized user to comment /azp run to run.

[MichalMarsalek]

Hi @NaluTripician. I just rebased my changes and yes, it seems that the problem they are addressing still exist. I think the change is still needed.

[NaluTripician]

@sdkReviewAgent

[NaluTripician]

Deep Review — Fix Expression constant folding inside MemberInitExpression

Verdict: Net-positive bug fix. No blockers (0 critical, 0 major). The fix correctly recurses into MemberInit bindings so closure-captured variables are folded.

The deliberate choice to not visit node.NewExpression is load-bearing — see the inline comment on SubtreeEvaluator.cs for the explanation. This is correct, not a defect.

Answering your question in the PR description

"vast majority of the expressions defining the document projections have the MemberInitExpression as the very root of the body … no constant folding would ever happen. Is that even possible?"

You aren't missing anything — the original 2018 implementation simply got this wrong, and the existing LinqTranslationBaselineTests.TestMemberInitializer baseline only exercises member access on the lambda parameter (doc.NumericField), not on captured locals. Anonymous-type projections use NewExpression and have always folded correctly, which is why this only surfaced in the relatively rare class-with-member-init case from #1664.

Observation — design layering (not in scope, worth a follow-up issue)

The Nominator nominates expressions bottom-up using the full default ExpressionVisitor, so it freely adds children of MemberInitExpression (including the inner NewExpression) to the candidate set. But SubtreeEvaluator cannot safely evaluate those candidates — folding a NewExpression inside a MemberInit would break the parent. The current workaround is "in SubtreeEvaluator, manually opt out of visiting paths that would lead us to fold something the parent can't accept." A more principled design would either (a) have the Nominator understand parent constraints, or (b) make EvaluateConstant return the original expression unchanged when the result would violate a structural constraint of the consumer. Worth filing a tracking issue; the customer-visible fix here is the right next step.

Findings overview

All inline comments are 🟡 Recommendations or 🟢 Suggestions — mostly test-coverage gaps. Please consider addressing #1, #2, #4, #5 before merge.

[NaluTripician]

Re-review after — ready to merge from review-feedback perspective

I pushed `` directly to fix-#1664 (maintainer_can_modify) to address the inline review feedback. All 8 review threads are now ✅ resolved.

What changed since the previous review pass

• SubtreeEvaluator.cs: added a load-bearing intent comment on VisitMemberInit and switched the bindings traversal to the protected static ExpressionVisitor.Visit<T> helper with a reference-equality short-circuit (no allocations on the common no-change path).
• ConstantEvaluatorTests.cs: removed 8 stale using directives, split the single combined test into 6 per-shape tests with descriptive names, replaced fragile Expression.ToString() equality with structural shape assertions, and added coverage for the gaps:
  • dictionary indexer keyed by a closure variable (the exact Error with Member Indexer when mapping value to a class model in select (if not const value) #1664 shape)
  • nested MemberInit
  • MemberMemberBinding (new Outer { Inner = { Prop = captured } })
  • MemberListBinding (new Outer { Items = { captured } })

Verification done locally

• dotnet build on src + Microsoft.Azure.Cosmos.Tests: clean (0 warnings, 0 errors)
• dotnet test --filter ~ConstantEvaluatorTests: 6 pass / 0 fail
• dotnet test --filter ~Linq: 26 pass / 0 fail
• Full Microsoft.Azure.Cosmos.Tests run: 2 637 pass, 11 skipped, 15 failed — all 15 failures are BinaryEncodingOverTheWireTests HttpRequestException: No connection (pre-existing infra dependency, unrelated to this PR)

Deferred — needs follow-up issue (not blocking this PR)

• End-to-end SQL baseline coverage in LinqTranslationBaselineTests.TestMemberInitializer requires regenerating the XML baseline against the Cosmos emulator. The unit test ClosuresUsedAsDictionaryIndexerInsideMemberInitAreFolded covers the Error with Member Indexer when mapping value to a class model in select (if not const value) #1664 shape at the constant-folding mechanism level (the layer this fix changes); the baseline coverage is a defense-in-depth addition that I'll track separately.

Remaining merge gates (out of scope for review feedback)

• /azp run needs to be issued by an authorized user to start the Azure Pipelines CI
• An approving review

Net: from a review-feedback perspective the PR is in good shape to merge once CI is green.

[NaluTripician]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[NaluTripician]

Confirmed the unit tests in this PR pass locally against the PR head (8fc8b9ce2):

dotnet test Microsoft.Azure.Cosmos.Tests --filter ConstantEvaluatorTests
  Passed ClosuresInsideMemberInitExpressionAreFolded
  Passed ClosuresInsideAnonymousObjectAreFolded
  Passed ClosuresUsedAsDictionaryIndexerInsideMemberInitAreFolded
  Passed ClosuresInsideNestedMemberInitAreFolded
  Passed ClosuresInsideMemberMemberBindingAreFolded
  Passed ClosuresInsideMemberListBindingAreFolded
Total: 6   Passed: 6   Failed: 0

dotnet test Microsoft.Azure.Cosmos.Tests --filter ~Linq
Total: 26  Passed: 26  Failed: 0

The two failing CI jobs (EmulatorTests Release - MultiMaster and ... - MultiRegion) are unrelated to this change — they require the $(COSMOSDB_MULTIMASTER) and $(COSMOSDB_MULTI_REGION) secret variables, which Azure DevOps does not mount on fork PR builds by default, so the env vars arrive empty and CosmosClient construction fails before any real test logic runs. The relevant CI job for this PR's changes, dotnet-v3-ci (Microsoft.Azure.Cosmos.Tests), passed.

[NaluTripician]

Approved, verified live tests pass locally until fork PR issue is fixed

[kushagraThapar]

PR looks good, request changes on the changelog entry.

[NaluTripician]

@kushagraThapar — added a changelog entry under a new Unreleased section in changelog.md (commit 960135d):

• 5298 LINQ: Fixes constant folding for closure-captured variables inside MemberInitExpression (resolves Error with Member Indexer when mapping value to a class model in select (if not const value) #1664). Previously, the recursion that partially evaluates expressions terminated whenever it encountered a MemberInitExpression node, so captured variables inside object initializers were not folded, producing invalid translated SQL.

Format matches the existing Fixed entries (PR# link → Category: Verb description). Ready for re-review.

[NaluTripician]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[kushagraThapar]

LGTM, thanks @MichalMarsalek !

[NaluTripician]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).