[chained relational comparison] Constant folding#83243
Draft
CyrusNajmabadi wants to merge 70 commits intodotnet:features/chained-relational-comparisonfrom
Draft
[chained relational comparison] Constant folding#83243CyrusNajmabadi wants to merge 70 commits intodotnet:features/chained-relational-comparisonfrom
CyrusNajmabadi wants to merge 70 commits intodotnet:features/chained-relational-comparisonfrom
Conversation
added 30 commits
April 20, 2026 10:06
Binder plumbing for C# preview feature "chained relational comparison" (spec: csharplang/proposals/chained-relational-comparison.md; §11.11.13): given an operation `A op B` where op is one of `<`, `<=`, `>`, `>=` and A is itself a bool-typed BoundBinaryOperator whose top-level operator is also in that set, the binder now falls back to a chained interpretation when ordinary overload resolution produces a binding-time error. This commit lays the infrastructure + binder hook + flow-analysis dispatch; lowering, CodeGen backstops, expression-tree refusal, CFG builder, semantic model fixup, and tests are TODO. - Add BoundBinaryOperator.UncommonData.IsChainedRelational + the ChainedRelationalLeftOperand slot for the pre-converted Y value. - Add BoundBinaryOperator.IsChainedRelational and IsShortCircuiting property shortcuts. - Add IDS_FeatureChainedRelationalComparison MessageID (preview). - Add ERR_NoChainedRelationalComparison (CS9379) and ERR_ExpressionTreeContainsChainedRelationalComparison (CS9380), resource strings, and IsBuildOnlyDiagnostic entries. - Add BinaryOperatorKind.IsChainableRelational() helper (<, <=, >, >=). - Hook chain fallback in BindSimpleBinaryOperator between overload-resolution failure and ReportBinaryOperatorError, using the standard try-then-commit BindingDiagnosticBag pattern. On rule 2(b) failure emit the specific diagnostic rather than CS0019. - Suppress outer LeftConversion for chained (it applies to Y, not to the bool-typed inner BoundBinaryOperator); keep RightConversion as usual. Store the converted Y in ChainedRelationalLeftOperand for the lowerer. - Placeholder FoldChainedRelationalOperator returning null; real chain constant-folding is TODO. - AbstractFlowPass.VisitBinaryOperator dispatch and the left-spine stack walker switch from IsLogical() to IsShortCircuiting; the inner logical loop treats IsChainedRelational nodes as isAnd=true / isBool=false (so the Unsplit/Split cycle synthesises the conditional state of the Y-op-B comparison). - VisitBinaryOperatorChildren stack-walker stops at IsShortCircuiting nodes too, so chained nodes don't get flattened into straight-line non-short-circuit traversal. - NullableWalker override of VisitBinaryLogicalOperatorChildren has the same isAnd/isBool forcing for chained.
Continues WIP work on chained relational comparison (spec §11.11.13).
Smoke tests with a dev csc now produce correct results for:
- 0 <= 5 < 10, 0 <= 5 < 2, 10 <= 5 < 100 (happy paths + short-circuit)
- min <= ComputeVal() <= max (single evaluation of middle)
- a < b < c < d (N-ary) (chain length 3+)
- a < b > c (mixed direction)
- Goo() < Bar() < Baz(arg) (Baz + arg NOT evaluated
when Goo() < Bar() is false)
- LocalRewriter.VisitBinaryOperator: detect IsChainedRelational and route
to the new RewriteChainedRelationalOperator; also teach the left-spine
stack walker not to flatten chained-relational nodes into the normal
non-short-circuit evaluation path.
- LocalRewriter_ChainedRelationalOperator.cs: new file. Collects the
chained-outer nodes down the left spine, identifies the innermost
classical relational link (the base link e0 op1 e1), allocates one
temp per shared middle operand, and builds a short-circuit &&-chain
with inline (temp = Y, temp) assignments for each capture point. The
N-th (outermost) link reads the final e_N directly; interior links
capture the next Y into the next temp.
- Notably passes oldNode: null to MakeBinaryOperator so that bind-time
constant values on the classical base (e.g. `0 <= 5` folded to true)
are not copied onto the rewritten base link, which would cause later
passes to drop the temp assignment and yield an uninitialised value.
- CodeGen/EmitOperators.cs: add Debug.Assert(!expression.IsChainedRelational)
backstop in EmitBinaryOperatorExpression so any path that forgets to
lower is caught early.
- DiagnosticsPass_ExpressionTrees.cs: emit
ERR_ExpressionTreeContainsChainedRelationalComparison on any chained
node seen inside an expression lambda conversion.
- ExpressionLambdaRewriter.cs: defensive Debug.Assert documenting that
DiagnosticsPass_ExpressionTrees rejects chained nodes before the
rewriter runs.
Still TODO: constant folding for chains, AbstractFlowPass fine-tuning
for the Unsplit/Split cycle (isBool=false for chained), CFG builder
IsConditional, CSharpSemanticModel.GetSymbolsAndResultKind surfacing,
spill-sequence backstop, and the full formal test files.
The stored `isChainedRelational` bool and `ChainedRelationalLeftOperand` reference carried the same information: the bool is true exactly when the reference is non-null (an invariant we already asserted). Drop the redundant bool from UncommonData's state and have BoundBinaryOperator.IsChainedRelational derive from whether the converted-Y reference is present. No behavioural change; smoke tests still pass.
Explain why `y` is extracted from leftBinaryOperator.Right (it is the shared middle operand of the chain), and why the attempt runs through a scratch BindingDiagnosticBag and a scratch OperatorResolutionForReporting (to keep failed chain attempts from leaking diagnostics or reporting state into the caller).
Corresponds to the "Asymmetric conversions of a shared middle operand"
open question in proposals/chained-relational-comparison.md. Each test
exercises a case where a shift_expression Y is used as the right
operand of one link and the left operand of the next, and documents
the current Roslyn implementation's behaviour (Option A, permissive).
- int<int<long - widening required only on the outer link
- short<int<int - widening required only on the inner link (safe)
- short<int<long - both links disagree on Y's required conversion
- int.MinValue sanity - rule out silent truncation at the extremes
- exhaustive grid - cross-check every (short, int, long) combination
against the equivalent hand-written &&-chain
- short-circuit - confirms the outer operand + its arguments are
not evaluated when the inner link is false
KNOWN ISSUE exposed by these tests: the current lowering stores the
outer-link's pre-converted Y in the temp, which emits unverifiable IL
for asymmetric cases ("Unexpected type on the stack. Found = Long,
Expected = Int32"). The IL runs correctly on RyuJIT thanks to
sign-extension but is rejected by formal IL verification, so the
affected tests use Verification.FailsILVerify.
When LDM decides between Option A and Option B, the tests can flip
appropriately (Option A -> fix the lowering to keep the temp at Y's
inner type and switch to Verification.Passes; Option B -> replace
ExpectedOutput with a VerifyDiagnostics expectation on
ERR_NoChainedRelationalComparison).
Explain what `resultOperatorKind &= ~BinaryOperatorKind.TypeMask` achieves on the ERR_NoChainedRelationalComparison branch: the kind is packed as OpMask | TypeMask | flags, and we keep the operator (OpMask, e.g. LessThan) so downstream passes still see *which* operator was written, while clearing the operand-type bits (TypeMask) so no consumer concludes that we successfully bound to a specific signature. Mirrors the same clearing in the neighbouring default-error branch.
Replace the iterative spine collection + reverse + two-pass loop with a single recursive BuildChainLink helper: each chained node allocates one temp for the Y it contributes, forms (temp = Y, temp), recurses into the inner link passing that inline-assign as the inner's right-operand replacement, and combines the lowered inner with its own link via short-circuit &&. The non-chained base case emits loweredLeft op innerRightReplacement directly. Source-level chains beyond a handful of operands are extremely rare (unlike `a + b + c + ... + z`), so there is no real need for an explicit stack here. Same lowered shape as before for every canonical case; smoke tests and all six asymmetric-conversion tests continue to pass.
Replace ImmutableArray<T>.Empty and ImmutableArray.Create<T>(x) with the collection-expression form [] and [x] respectively; ArrayBuilder producers stay on ToImmutableAndFree. Drop the now-unused System.Collections.Immutable using, and fix a stray unresolved cref in the XML doc.
…n recursion - Replace the nullable `innerRightReplacement` + `??` fallback with a single non-null `thisRight` parameter meaning "the right-operand expression this link should use". The top-level caller supplies `VisitExpression(node.Right)`; every recursive call supplies the parent's inline-assign. No sentinel, no ?? at the chained-branch. - Drop the Debug.Assert on non-null replacement in the base case (the parameter is now statically non-null). - Document why the function must drive its own recursion into `node.Left` rather than delegating to the generic VisitExpression machinery: injecting each level's inline-assign into the right spot of the inner link is the whole point of this pass, and the injection site is several BoundBinaryOperator nodes deep for long chains, so each level has to control how its own left subtree is lowered in order to thread the substitution through.
Only one call site, so the helper was extra indirection for no benefit. The comment moves to the inline site.
Replace the two `(object)x.Type != null && x.Type.SpecialType == ...`
compound checks I introduced in this branch with the equivalent property
pattern `x.Type is { SpecialType: ... }`. Same semantics, less noise,
and no lingering `(object)` cast.
Only touches code this branch added. Pre-existing null checks elsewhere
in the files are left alone.
The two booleans were not independent: any code path that sets
`hasErrors = true` leaves `foundOperator = false`, and any path that
sets `foundOperator = true` leaves `hasErrors = false`. The cascade
`if (!foundOperator) { ... chain attempt ...; if (!foundOperator && !hasErrors) { ReportBinaryOperatorError } }`
was therefore really a three-way state machine (classical-success /
chain-success / error) with two flavours of error.
Restructure to make that explicit: the chain-shape check is an `if`
whose `else` is the "no chain shape applies; report ordinary CS0019"
path. No more `&& !hasErrors` guard, and a dedicated `hasErrors`
variable is no longer needed to communicate between the two former
top-level `if` blocks (we still keep it, because later code still
threads it through to the returned BoundBinaryOperator).
While here, consolidate the four `operatorResolutionForReporting.Free()`
calls (one per branch) into a single Free at the end of the outer
block: it's safe for every path because ReportBinaryOperatorError (the
only consumer) is called before the outer block ends.
Also merge the big TypeMask-clear comment that used to live on the
chain-failure branch up to just above the outer `if`, since the same
reasoning applies to both error branches now.
All six ChainedRelationalComparisonTests still pass and the dev-csc
smoke test produces identical results.
The binder already threads the chain-fold hook through the chained path (constant value captured on UncommonData, participates in hasErrors the same way non-chained FoldBinaryOperator does). The body returns null for now, which matches the "not a constant" contract, so a chain of all-constant operands is simply not a constant expression yet. Runtime behaviour is unaffected. Restate the comment as "Deferred: ... tracked as the follow-up" instead of an informal TODO so a future reader immediately sees this is intentional for the binding-foundation PR and knows where to look for the spec rule to implement.
Investigation of the LeftTruthOperatorMethod slot on BoundBinaryOperator confirmed it's provably null for every chained-relational node: that property is only populated when OperatorKind.IsDynamic() && OperatorKind.IsLogical(), and a chained-relational node's OperatorKind is always one of <, <=, >, >=. §11.11.13 rule 2(b) further guarantees every link returns bool, and the chain is combined via _factory.LogicalAnd (i.e. LogicalBoolAnd), so `operator true` / `operator false` never come into play. - Simplify the lowering: pass BinaryOperatorMethod directly to MakeBinaryOperator rather than `LeftTruthOperatorMethod ?? BinaryOperatorMethod`; the `??` was vacuous and the truth-operator path is unreachable here. Added a comment explaining why so a future reader doesn't wonder. New tests in CSharp15/ChainedRelationalComparisonTests.cs pinning the invariants: - OperatorTrueFalse_DefinedOnOperandType_DoesNotAffectChain: a struct that has bool-returning `<` plus `operator true`/`operator false` participates in a chain like any other type. The truth operators are never called; the chain's `&&` combination is over bools. - NonBoolReturningOperator_PreservesClassicalBinding: a struct whose `operator <(T, T)` returns T (not bool) keeps its classical left-associative meaning. Chain fallback does not fire (classical binding succeeds at every link), so the user sees CS0029 when trying to use the T result as a bool - the existing back-compat story is preserved. - ChainFallback_OuterLinkHasNoApplicableOperator_ReportsCS9379: a bool-returning `<` on S plus a mismatched outer RHS (Point) triggers the chain fallback, which finds no applicable `S < Point` operator, so the specific ERR_NoChainedRelationalComparison fires instead of the generic CS0019. Even with `operator true`/`false` defined on S, the truth operators play no role.
…cking
Add broader coverage to ChainedRelationalComparisonTests.cs; the file
previously focused narrowly on asymmetric conversions and operator
true/false. New regions:
- Basic binding and runtime behaviour:
* Int BasicHappyPath
* Nullable (lifted ops return bool)
* MixedDirection (a < b > c)
* FourAndFiveOperands (n-ary chains)
* MiddleOperandEvaluatedOnce
* IL_CanonicalBoundsCheckShape (pins `0 <= i < a.Length` emit)
* UserDefinedBoolReturningOperator
- Back-compat and error cases:
* ParensBlockChainShape -> ordinary CS0019
* EqualityOperators do not chain
* ChainInsideExpressionTree -> CS9380
* Dynamic binds dynamically; no chain fallback (runtime throws
RuntimeBinderException on `bool < int`, pinning the spec's
"§11.11.13 does not apply" rule)
- Language-version gating:
* ChainRequiresPreviewLanguageVersion -> CS8652 (ERR_FeatureInPreview)
Spec bug fixed in Binder_Operators.cs: the chain-shape check now also
requires `node.Left is BinaryExpressionSyntax`, which enforces spec
§11.11.13's "parentheses around the left-hand operand prevent the
chain interpretation" rule. Before this change, `(a < b) < c` would
incorrectly bind as a chain because BoundParenthesizedExpression is
collapsed out of the bound tree. The new syntactic check restores the
spec's requirement that A be an operation of the form `X op' Y`.
21 tests total; all pass, dev-csc smoke test still identical.
Move IsChainableRelational from BinaryOperatorKind (in OperatorKindExtensions) to SyntaxKind (in SyntaxKindExtensions), and rename it to IsChainableRelationalExpression to match the SyntaxKind naming convention. Update both call sites in the binder to check the SyntaxKind of the outer BinaryExpressionSyntax and the inner BinaryExpressionSyntax respectively. Rationale: the chain rule only applies to relational comparisons the user wrote in source. Checking the BoundBinaryOperator's semantic OperatorKind would also match compiler-synthesized bound nodes that happen to carry a relational kind, which is not the user-visible situation §11.11.13 governs. Gating on SyntaxKind directly makes the rule robust against any such synthesis. Side benefit: the parens-blocking behaviour becomes implicit rather than an extra check. A ParenthesizedExpressionSyntax is not one of the chainable SyntaxKinds, so `(a < b) < c` is already rejected by the `node.Left is BinaryExpressionSyntax inner && inner.Kind().IsChainableRelationalExpression()` pattern without a separate parens check. Same for checked(...), conditional expressions, etc. All 21 tests pass, including ParensBlockChainShape_ReportsOrdinaryCS0019 (which was the test that originally surfaced the parens-blocking bug).
…tors helper The only callers are the two chain-fallback checks inside BindSimpleBinaryOperator. Moving it out of SyntaxKindExtensions keeps the shared extension type uncluttered and keeps the helper scoped to the single file that actually cares about it. Same semantics (syntactic check on the user-written SyntaxKind), just local to Binder_Operators.cs now as a private static helper rather than an internal extension. All 21 tests still pass.
Previous single-evaluation coverage only exercised chains whose
endpoints were locals or literals (e.g. `0 <= M() <= 10` or
`a < B() < c`). Add two tests that close the gap:
- Chain_AllOperandsAreMethodCalls_EachEvaluatedOnceInLeftToRightOrder:
`A() < B() < C()`. Each call logs its name; the test asserts the log
is exactly [A, B, C] when all links succeed, [C, B] when the inner
link fails (outer skipped entirely), and [A, C, B] when the outer
link fails after the inner succeeds. B() appears at most once - the
naive `A() < B() && B() < C()` rewrite, which the chain must not
produce, would show B twice.
- Chain_FourMethodCalls_EachSharedMiddleEvaluatedOnce: the N-ary
generalisation with both B and C as shared middles. Verifies that
each operand is evaluated at most once, in source order, and that
short-circuit skips the rest when any link is false.
Also fix a stray `&&` on the chain-shape condition in
Binder_Operators.cs that a local edit left behind when merging the
two separate `left is ... && left.Type is { ... }` checks into a
single extended property pattern
`left is BoundBinaryOperator { Type.SpecialType: ... } leftBinaryOperator`.
The consolidated form is cleaner - nothing else changes.
Rule 2(b) of §11.11.13 has two failure modes: the isolated `Y op B` overload resolution could find no applicable operator at all (already covered by ChainFallback_OuterLinkHasNoApplicableOperator_ReportsCS9379), OR it could find one that returns a non-bool type. Only the first mode was tested. Add ChainFallback_OuterLinkResolvesToNonBoolOperator_ReportsCS9379: struct S with bool-returning `operator <(S, S)` (so the chain shape exists) plus a non-bool `operator <(S, int)` returning S. Isolated resolution on `S < int` finds the non-bool overload, so the chain must be rejected with ERR_NoChainedRelationalComparison rather than any of CS0019, CS0029, or a silent wrong-typed bound node. 24 tests total; all pass.
Previous rounds called these "too contrived" / "not worth the complexity to document" and skipped them. Pinning both now so future compiler-team review doesn't have to ask why they were skipped. - ChainFallback_OuterLinkResolutionIsAmbiguous_ReportsCS9379: the third rule-2(b) failure mode, where isolated `Y op B` overload resolution finds multiple applicable user-defined operators with no tiebreaker. Construction mirrors the CS0034 ambiguity pattern from SemanticErrorTests (implicit conversions to two unrelated wrapper types, each of which has its own user-defined `<`). The chain feature must produce ERR_NoChainedRelationalComparison for this path, not CS0034 - the ambiguity is unfamiliar to a user trying to write a chain. - ChainFallback_OnOlderLanguageVersion_ReportsBothPreviewAndRule2Errors: documents the deliberate current behaviour where the preview-gating call (unconditional) AND the chain-rule-2(b) failure both report. Each describes a distinct problem; the test will need updating if LDM later prefers to suppress the rule-2(b) diagnostic when preview is already failing. 26 tests total; all pass.
…a chain Previous coverage pinned exactly one permutation (short<int<long). The other five can behave quite differently at the IL level depending on whether the middle operand's type is strictly "in between" the other two - in that case the inner link's operator is narrower than the temp's type and the current lowering emits unverifiable IL, whereas in the other four permutations all operators end up on the same width and IL-verify passes. Cover every arrangement as a Theory + InlineData with an `ilVerifies` discriminator. Each row asserts single-evaluation of A/B/C (important: asymmetric widening must not double-evaluate) and a True chain result for the chosen values (1, 5, 10). Expected IL-verify outcome per permutation: short < int < long FAILS (inner int<int, temp long) short < long < int PASSES (inner long<long, temp long) int < short< long FAILS (inner int<int, temp long) int < long < short PASSES (inner long<long, temp long) long < short< int PASSES (inner long<long, temp long) long < int < short PASSES (inner long<long, temp long) When the "keep temp at Y's inner type" fix for Option A lands, the two FAILS rows flip to Passes and the parameter can be removed. 32 tests total; all pass.
… types Add ChainFallback_InnerConversionShapesYAwayFromCompatibleRawType_ReportsCS9379: a scenario where B has both an `implicit -> int` and a bool-returning `operator <(B, string)`, used in `int a, B b, string c; a < b < c`. - The inner link resolves `int < B` to predefined `int < int` (via B's implicit conversion to int); Y is therefore stored as `(int)b`, type int. - The outer isolated `int < string` has no applicable operator anywhere, so rule 2(b) fails and CS9379 fires. - Crucially, a hypothetical "look at raw b" interpretation would have succeeded via B.operator<(B, string). Spec §11.11.13's "Conversions on the shared middle operand" explicitly says we use Y's post-inner classification, so we reject this case. The test pins current spec + current implementation. If LDM later adjusts the spec to reconsider Y's raw type, this test gets updated.
The features/labeled-break-continue branch reserves 9379 for `ERR_NoBreakOrContId`. To avoid a merge collision when either branch lands, shift our two codes: ERR_NoChainedRelationalComparison: 9379 -> 9380 ERR_ExpressionTreeContainsChainedRelationalComparison: 9380 -> 9381 Update all test-file comments referencing the CS#### numbers. Resource strings are keyed by name, not number, so no .resx/.xlf change needed. No behaviour change; all 33 tests still pass.
…erator.Update The Update(BinaryOperatorKind, ConstantValue?, MethodSymbol?, TypeSymbol?, ...) overload rebuilds UncommonData via CreateIfNeeded, which preserves the four primary UncommonData fields but has no way to preserve ChainedRelationalLeftOperand. Calling it on a chained node would silently drop the chain marker and produce a wrongly-shaped bound tree. All current callers (SpillSequenceSpiller, ExtensionMethodReferenceRewriter, Optimizer) run post-lowering, where chained nodes have already been rewritten to a short-circuit && chain - so the assert is a backstop, not a fix for observed breakage. If it ever fires, the right response is to switch the caller to Update(UncommonData) (preserves Data exactly) or to extend this overload to forward ChainedRelationalLeftOperand. No behaviour change; all 33 tests pass.
…walker BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator exists to avoid stack overflow on deep left-leaning binary-operator trees by flattening the left spine into an explicit visit order. It does this indiscriminately, which means `a < b < c < d` (chained relational) or `a && b && c && d` (conditional logical) have their left spines visited straight-through, losing the short-circuit control-flow information. For ref-safety analysis (the primary consumer) this was conservative but not clearly wrong: every operand is visited, so every escape is considered. But philosophically the walker was silently lying about the tree's evaluation order, and chained relational nodes made this more visible because their operands routinely carry non-trivial conversions and shared-middle temp semantics. Stop the flatten whenever the spine hits a short-circuit node - both when node.Left is itself short-circuiting AND when the spine walk discovers one mid-loop. In both cases fall back to the base walker's default Left/Right visit, which preserves the tree structure and is still stack-guarded by BoundTreeWalkerWithStackGuard. No observable behaviour change in existing tests (853 CSharp15 tests still pass including the 33 chained-relational tests); the fix is about correctness of control-flow for future analyses built on top of this walker.
…binary ops MethodInvocationInfo.FromBinaryOperator built argument-to-parameter pairs by zipping [binary.Left, binary.Right] against the operator method's parameters. For a chained relational BoundBinaryOperator, that's wrong: the outer node's `Left` is the bool-typed inner BoundBinaryOperator, not the semantic first operand Y. The operator method (selected for `Y op Right`) has its first parameter typed for Y, so pairing it with the inner bool gave ref-safety analysis a mismatched argument that could produce wrong escape-scope conclusions for user-defined chained relationals whose operator parameters carry refs. Use binary.ChainedRelationalLeftOperand (the converted Y, non-null iff IsChainedRelational) as the first arg for chained nodes, and keep binary.Left for classical nodes. The corresponding built-in (no-method) intersect path at lines 4714-4715 / 5519 walks into the inner BoundBinaryOperator recursively; for bool-returning chains the final result is bool so the conservative intersection is harmless. That path is left unchanged. All 33 chained-relational tests still pass; the broader CSharp15 test suite (853 tests) still passes.
…eferred)
Attempted to thread ChainedRelationalLeftOperand (Y) through
afterLeftChildOfBoundBinaryOperatorHasBeenVisited so that:
(a) InferResultNullability sees Y's TypeWithState instead of the
inner bool's, and
(b) the `<`/`<=`/`>`/`>=` relational null-refinement that
AfterLeftChildHasBeenVisited applies also runs for chained nodes
(so e.g. `int x; s?.Length; if (x < x + 1 < s?.Length) s.Length`
wouldn't warn on `s.Length`).
That fix tripped DebugVerifier: ChainedRelationalLeftOperand lives on
UncommonData, not as a regular bound-tree child, and the verifier
validates every analyzed node against its own bound-tree walk. A
straightforward VisitWithoutDiagnostics(ChainedRelationalLeftOperand)
adds to the analyzed-nullability map without the verifier walking it,
so counts diverge.
Teaching DebugVerifier about UncommonData-embedded expressions is the
correct long-term fix but is non-trivial and out of scope for the
binding-foundation PR. For (a) in particular, §11.11.13 rule 2(b)
forces chained's result type to `bool`, so the wrong `leftType` has no
observable effect on return-type nullability; it is a theoretical
concern only. For (b), it means NRT refinement through a chained
outer link is currently missing, which can produce spurious
"possibly-null dereference" warnings in bodies gated on chained
conditions. Neither is a regression from prior behaviour (the feature
didn't exist), and neither is user-visible incorrect output.
Add a detailed TODO at the chained-branch in NullableWalker describing
the two missed refinements, the DebugVerifier constraint that blocks
the direct fix, and where a future fix should land.
…nd SpillSequenceSpiller Both passes run post-LocalRewriter, where LocalRewriter_ChainedRelationalOperator has already rewritten every chained BoundBinaryOperator into a BoundSequence containing a short-circuit && chain. But both passes also call `node.Update(..., ConstantValueOpt, BinaryOperatorMethod, ...)` on whatever binary node they do see, and that overload rebuilds UncommonData via CreateIfNeeded - silently dropping ChainedRelationalLeftOperand if the assert added earlier in this branch were ever removed from the Update overload itself. These asserts give a second, pass-local backstop. Optimizer additionally uses (operatorKind.Logical) to emit branch cookies / stack scheduling - which is wrong for chained nodes since they short-circuit but aren't IsLogical. If this assert were to fire, the path would need to treat IsChainedRelational as a short-circuit boundary the way the pre-lowering walkers do. Same pattern as the existing backstop in CodeGen/EmitOperators.EmitBinaryOperatorExpression. No behaviour change; all tests pass.
Six additions flagged by the subagent synthesis as medium-priority: 1. Chain_MiddleOperandEvaluatedOnce tightened: previously covered only the inner-true path (result=True, calls=1). Now also covers inner-true outer-false (different runtime path through the lowering) and inner- false outer-skipped (short-circuit). A bug that double-evaluated the middle operand on the inner-false path would slip past the old single- assertion version; the new version catches it. 2. Chain_MatchesHandWrittenAndChain_SingleEvaluationPinned: explicit spec-example coverage. Prints `0 <= M() <= 10` and `0 <= M() && M() <= 10` side by side, pinning that the chain evaluates M once and the naive && rewrite evaluates M twice. Previously we only pinned the chain count. 3. Chain_SameTypeEquivalence_MatchesHandWrittenAndChain: complement to the existing AsymmetricConversion_EquivalentToHandWritten... grid test. Exercises the "identity conversion everywhere" lowering path (all three operands int), a 64-case grid compared against the hand-written && form. Catches any drift in same-type behaviour. 4. ChainFallback_OuterLinkError_ReportsCS9380_ForEveryChainableOperator ([Theory, InlineData]): previously only `<` was tested at the CS9380 diagnostic path; this covers `<=`, `>`, `>=` routing through the same diagnostic with correct OperatorToken text and argument substitution. Location-independent (each op has a different source column).
… ?:, bool?, Func<bool>) Four new tests covering operand shapes the existing suite didn't touch: - Chain_EmbeddedInStringConcat_LowersExactlyOnce: chain as the right operand of string concatenation. Exercises LocalRewriter_BinaryOperator's left-spine stack walker now stopping on `current.IsChainedRelational` - a regression there would flatten the chain into the + spine. - Chain_ResultConvertedToNullableBool_CompilesAndRuns: `bool? b = chain;`. Exercises the implicit boxing conversion sitting atop the chain's bool result in the bound tree. - Chain_UsedAsGenericArgumentOfFuncTakingBool_Works: pass chain as a `bool` argument and capture it in `Func<bool>`. Defensive against binder-side interactions between the speculative chain-fallback attempt and type inference. - Chain_WithOperandShapes_NullCoalescingAndConditionalAccess_Works: operands are `??`, `?.`, and `arr.Length`. Marked `FailsILVerify` because `int < int? < int` is an asymmetric-conversion chain, same deferred IL issue the existing asymmetric tests pin. Needs `#nullable enable` to use `int[]?`. - Chain_WithOperandShapes_ConditionalExpression_Works: `cond ? A() : B()` as the middle operand. Pins that the chain's temp capture doesn't interfere with the conditional-operator lowering of a chain operand. 44 tests total; all pass.
added 5 commits
April 20, 2026 15:16
…ned theory Every row of MixedSignedUnsigned_ChainsThatClassicalBindingAccepts_Fold evaluates to True by design (operands chosen to satisfy a < b < c), so the per-row "True" parameter was dead weight. Hard-code the uniform expectation in the test body and drop the column.
…alue Each row now carries the chained form, the equivalent hand-written expansion `(op1) && (op2) && ...`, and the expected bool value. Both forms are placed into separate `const bool` initializers in the same compilation, so CS0133 fires if either form fails to fold, and a silent value drift between the chained fold and the classical fold surfaces as a mismatched expected output. N-ary chain rows (1<2<3<4 and 1<2<3<4<5) expand to three and four `&&` terms respectively.
…ntFoldingTests Turn `#nullable disable` into `#nullable enable` on the constant-folding test file so it matches Roslyn's modern-file default. No code changes needed - the tests only use non-null strings and value types locally, so nullable annotations require no adjustment. All 352 chained-relational tests still pass.
…s other The original theory carried both the chained form and the hand-written expansion as full expression strings. For the large majority of rows - a single relational operator appearing at both links of a three-operand chain - that's duplication of the operator AND the operand values; the expansion can be synthesized from the parts. - ThreeOperandChain_ChainedAndExpandedFoldAgree takes (a, b, c, op, expected) and builds both `a op b op c` and `(a op b) && (b op c)` in the test body. Covers 13 rows: basic int, <= / >=, boundary-equal, negative/ overflow-adjacent, and the four asymmetric-conversion shapes (int/long widening, short widening). - MixedOperatorsOrNAryChain_ChainedAndExpandedFoldAgree handles the 4 rows that don't fit the template: `1 < 2 > 0` (mixed direction), and the three n-ary chains of length 4 / 5. These keep the full (chain, expanded, expected) shape since there's no uniform operator to factor out. Total row count unchanged; behaviour unchanged; 373 tests pass.
…rows (IDE0055) Same IDE0055 cleanup as the parent-branch commit, applied to ThreeOperandChain_ChainedAndExpandedFoldAgree, MixedOperatorsOrNAryChain_ChainedAndExpandedFoldAgree, and MixedSignedUnsigned_ChainsThatClassicalBindingAccepts_Fold. Runs ./eng/build.sh --runAnalyzers --warnAsError on the CSharp15 test project now produces 0 warnings across the whole crc feature area. All 373 chained-relational tests still pass.
CyrusNajmabadi
force-pushed
the
crc/constant-folding
branch
from
b777799 to
463196c
Compare
added 12 commits
April 20, 2026 15:32
…ted nullable, n-ary, and user-defined chains Five new VerifyIL tests (six total with the pre-existing Chain_IL_CanonicalBoundsCheckShape) covering the emit scenarios the user flagged as highly relevant: - Chain_IL_SameTypeInt_TempReuseAndShortCircuit: `int a, int b, int c => a < b < c`. Pins the single-evaluation guarantee (b stored to V_0 once, loaded twice), and the bge.s-style short-circuit that skips c entirely when the inner link fails. - Chain_IL_AsymmetricShortIntLong_ConversionOnTempLoad: `short a, int b, long c => a < b < c`. Pins that the temp V_0 is declared `int` (the inner link's type, not the outer's wider long), and that `conv.i8` at IL_0007 widens the temp on its OUTER-LINK load - not at store time. This is the IL-level guarantee of verifiable IL for asymmetric-widening chains. - Chain_IL_NullableInt_HasValueAndShortCircuit: `int? a, int? b, int? c => a < b < c`. Shows the standard lifted-relational lowering: GetValueOrDefault + HasValue.get + `and` on both sides, per §11.4.8 "lifted returns true iff both non-null AND underlying compare is true", then `brfalse.s` on the inner result to short-circuit the outer. - Chain_IL_NAry_NestedShortCircuits: 4-operand chain `a < b < c < d`. Two temps (one each for b and c), two `bge.s` short-circuits both targeting the same false-tail, three compares total. Confirms the stack-walker emits consistent temp-reuse across arbitrary lengths. - Chain_IL_UserDefinedOperator_CallsBothOperators: struct S with `operator <(S, S)`. Shows the chain lowers to two `call S.op_LessThan(S, S)` instructions separated by `brfalse.s`, with b's temp loaded exactly twice. Note: a constant-folded chain (e.g. `0 < 5 < 10`) would emit the same shape today because the fold result is not consulted at lowering time; that optimisation is a follow-up on crc/constant-folding. All tests pass under `--runAnalyzers --warnAsError`. Total CRC tests: 296.
…EmitTests file Move all six IL-pinning tests (CanonicalBoundsCheckShape, SameTypeInt_TempReuseAndShortCircuit, AsymmetricShortIntLong_ConversionOnTempLoad, NullableInt_HasValueAndShortCircuit, NAry_NestedShortCircuits, UserDefinedOperator_CallsBothOperators) out of the main ChainedRelationalComparisonTests file into a new ChainedRelationalComparisonEmitTests file. Drops the `Chain_IL_` name prefix from each test since the class now carries that context. Totals unchanged: 290 tests in the main class + 6 tests in the emit class = 296 total, same as before the split.
…nedRelationalComparisonEmitTests
…ace operators
Chained relational comparison has no explicit awareness of generic math, but
its binding path already threads ConstrainedToTypeOpt through BinaryOperatorSignature
at each link, so chains over a type parameter constrained to a curiously-recurring
interface with static abstract relational operators "just work". These tests pin
that it does: both binding + emit + runtime behavior for three constraint shapes
(interface-only, class + interface, struct + interface), each with T and T?
operands where meaningful.
Runtime tests (ChainedRelationalComparisonTests.cs):
- 7 tests covering the full matrix of constraint shapes x {T, T?}, exercising
both reference and value-type instantiations, short-circuit semantics, null
handling for Nullable<T>, and single-evaluation of the shared middle operand.
IL pins (ChainedRelationalComparisonEmitTests.cs):
- 5 tests pinning the generated IL: constrained. T + call dispatch is uniform
across all three constraint shapes for plain T; T? on a class-constrained T
produces IL byte-identical to T (no lifting); T? on a struct-constrained T
composes the lifted-relational HasValue-gated pattern on top of the
constrained call on each link.
The five happy-path Fact tests for the matrix of (constraint shape) x (T vs T?)
were sharing nearly identical test bodies with only the `class, ` / `struct, `
prefix and the `?` suffix differing. Fold them into one [Theory] per file with
[InlineData] rows that parameterize the same source template, so adding another
constraint shape is a one-line change and the shared IL / runtime invariants are
expressed once.
ChainedRelationalComparisonEmitTests.cs:
- 4 Facts (InterfaceOnly_T, InterfaceAndClass_T, InterfaceAndClass_NullableT,
InterfaceAndStruct_T) collapse into one Theory with 5 rows, all asserting
the same canonical `constrained. T` + `call` IL.
- The one-off `InterfaceAndStruct_NullableT_Lifted` Fact stays separate since
it pins uniquely different IL (lifted-over-constrained pattern).
- `#nullable enable` is hoisted into the GenericILHarness constant so the
per-row source template doesn't need per-row preprocessor tweaks.
ChainedRelationalComparisonTests.cs:
- 5 Facts for the unlifted runtime happy-path collapse into one Theory with
6 rows (adds an extra row that exercises an unconstrained T against both
RefImpl and ValImpl instantiations, since that shape uniquely accepts both).
- The two struct-`T?` Facts (null-scenario coverage and single-evaluation)
stay separate because they assert behaviours the unlifted rows can't cover.
Total generic-constrained test count: 12 -> 14 (5 IL-pin rows + 1 IL fact +
6 runtime rows + 2 runtime facts). Same coverage, +2 rows net, -231 source
lines.
…Data rows (IDE0055) The new Theory rows introduced in the previous commit used column-aligned whitespace between the string arguments for readability; --runAnalyzers --warnAsError flags that as IDE0055 (Fix formatting), consistent with the earlier fix on the nullable-test rows. Collapse to single spaces - the accompanying comment block still documents what each row's arguments mean, so readability isn't lost.
…trained InlineData rows (IDE0055)
…tional nodes GetSymbolsAndResultKind used BoundBinaryOperator.Left.Type as the synthesized intrinsic operator's left-parameter type, but for a chained relational comparison node that Left is the inner link's bool result, not an operand of the isolated `Y op Right` operator this node represents. As a result GetSymbolInfo returned signatures like `bool operator <(bool, int)` - a symbol the `TestOperationVisitor.CheckOperators` IOperation test hook then fails to round-trip through Compilation.CreateBuiltinOperator. Use ChainedRelationalLeftConvertedType (the outer link's left-operand type chosen by overload resolution) as the left-parameter type for chained nodes, matching what overload resolution actually resolved.
…ained relational nodes
CyrusNajmabadi
changed the title
This was referenced Apr 20, 2026
added 5 commits
April 21, 2026 00:27
Drop narrative test-prose that restated the spec, referenced implementation paths, or enumerated "a wrong impl would X" hypotheticals. Keep at most one brief line where the test's behaviour isn't self-evident from its name and source.
…g/emit tests # Conflicts: # src/Compilers/CSharp/Test/CSharp15/ChainedRelationalComparisonTests.cs
Same trim pass as on crc/binding: drop spec restatements, implementation-path name-drops, and "what a wrong impl would show" hypotheticals.
Those tests compile against TargetFramework.NetCoreApp to access static- abstract interface members. The emitted binary then references System.Runtime 7.0, which desktop (net472) runs can't load - producing FileLoadException / BadImageFormatException on Test_Windows_Desktop_*. Switch the three GenericConstraint_* tests in ChainedRelationalComparisonTests and the two in ChainedRelationalComparisonEmitTests from [Fact] / [Theory] to [ConditionalFact(typeof(CoreClrOnly))] / [ConditionalTheory(typeof(CoreClrOnly))] so they're simply skipped on desktop runs. The language feature under test is a .NET 7+ runtime feature; exercising it on desktop was never meaningful.
CyrusNajmabadi
changed the title
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
1 participant
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.
Follow-up to #83241. Teaches the binder's constant folding to recognise a chained relational comparison as a constant expression when every operand is constant and every link's isolated
X op' Y/Y op Zfold is constant. MatchesX && Ybehaviour: chains appear inconstinitializers,switchcase labels, attribute arguments, and anywhere elseConstantValueis consulted.Nullable-typed operands (
int?etc.) intentionally do not fold, matching existing relational-operator behaviour.Stacked: