fix(expr): stop target_type leaking into operand evaluation; accept 'any' target

[mwiebe]

Fixes: #291

What was the problem/requirement? (What/Why)

When a host application evaluates an expression, it can pass a target_type
to say what kind of value it wants back. For example, a template field that
holds a timeout wants an int, so the host evaluates that field's expression
with target_type=int, and the evaluator converts the result to an int if it
can. The type applies to the expression's final answer — "whatever this
computes, I need it as an int at the end."

The bug: instead of applying the target type only to the final answer, the
evaluator applied it to every intermediate value along the way. Most
intermediate values have nothing to do with the final answer's type. Take
[10, 20, 30][0] with target_type=int: the answer is 10, which is
already an int. But the evaluator first tried to convert the list
[10, 20, 30] itself to an int — a conversion that makes no sense and
fails — so a perfectly valid expression errored out the moment a target
type was supplied.

Issue #291 reported three cases of this, found by differential testing
against an independent implementation:

• A. Subscripts: [10, 20, 30][0] with target_type=int tried to
  coerce the list (and the index) instead of the result.
• B. and/or: these operators return one of their operands, and only
  the returned one matters. null or 7 with target_type=int should return
  7, but the evaluator tried to coerce the discarded null and failed.
  This breaks the common Param.X or "fallback" null-coalescing pattern.
• C. An explicit any target — defined by the spec as "matches
  anything" — was rejected for every value, because the coercion function
  had no rule for it.

While confirming the report we found the same leak in two more places:
function-call arguments (len('abc') with target_type=int tried to parse
'abc' as an int) and slice bounds ('hello'[1:3] with
target_type=string turned the bounds into strings so no slice signature
matched).

RFC 0005 already specifies the correct behavior in its "Target Type
Propagation Rules" table: operands evaluate unconstrained, and only a few
specific slots (the branches of a ternary, the elements of a list literal)
inherit or derive a target from their parent. Our spec transcription of that
table in specs/expr/evaluator.md had the wrong entries for BoolOp,
Call, Subscript, and ListComp — and the code faithfully implemented
the wrong table.

What was the solution? (How)

Two independent fixes:

1. Propagation is now structural rather than enumerated. Previously the
   target type lived in a mutable field on the evaluator that every node
   implicitly saw, and node kinds that needed to shield their operands each
   had to remember to temporarily clear it — subscripts, and/or, call
   arguments, and slice bounds were the ones that forgot. Now the target is
   an explicit parameter threaded through the recursion (eval_node(node, target)): each node applies its target to its own result only, and
   children receive no target unless the node explicitly passes one. Only
   the RFC-sanctioned slots do — ternary branches inherit the parent's
   target, and list-literal elements derive the element type from a
   list[T] target. A node kind added in the future is therefore safe by
   default instead of inheriting the caller's target by accident.

2. coerce treats any as a no-op. A value asked to become any is
   returned unchanged, matching the spec's definition of any as the
   unconstrained type.

The propagation table in specs/expr/evaluator.md was corrected to match
RFC 0005, and now documents the three slots where we deliberately deviate
(using no target plus an explicit type check, because the explicit checks
produce friendlier error messages — e.g. "Condition must be a boolean,
got int" instead of a generic coercion failure). The upstream RFC and wiki
were checked and are already correct; no spec-repo changes are needed.

What is the impact of this change?

Expressions that evaluate correctly without a target_type now also
evaluate correctly with one. This matters in ordinary use, not just edge
cases: the spec's own rule for template args items gives them a target of
T? | list[T], so any host following the spec hit case A for every
subscript and case B for every or-based fallback in an args field.

No public API changed. The Evaluator type is crate-private; the public
with_target_type builder methods and their signatures are untouched.

How was this change tested?

• 12 new regression tests in
  tests/integration/test_target_type_propagation.rs covering all five
  expressions from the issue, the call-argument and slice-bound variants,
  the realistic T? | list[T] args-target scenarios, and one
  full-message assertion (message + expression + caret) confirming that a
  genuinely uncoercible result still errors correctly.
• cargo test --workspace: all tests pass, including the 3,141-test expr
  integration binary. (Two openjd-sessions Windows cross-user tests fail
  on the development machine with a domain-unavailable environment error,
  0x8007051F, unrelated to this change — they exercise LogonUserW.)
• cargo clippy --all-features --all-targets --workspace -- -D warnings: clean.
• Full OpenJD conformance suite: the failure set is byte-identical between
  main and this branch (41 pre-existing failures in newer let/timeout
  validation tests, unrelated to expressions), confirming no regressions.

Was this change documented?

Yes. Doc comments on eval_node and ExprValue::coerce describe the new
propagation contract. specs/expr/evaluator.md (propagation table,
BoolOp/Call/Subscript sections) and specs/expr/values.md (coercion rules)
were updated in the same commit. specs/expr/public-api.md needed no
changes because the public API is unchanged.

Is this a breaking change?

No. The public API surface is identical. Behavior changes only for
expression + target_type combinations that previously returned spurious
errors; those now succeed as the spec requires.

Does this change impact security?

No. It does not create files, change permissions, or cross any trust
boundary. Evaluation resource limits (memory, operation count, recursion
depth) are unaffected; the single recursion chokepoint that enforces the
depth limit is preserved in eval_node.

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[crowecawcaw]

Nice!

[leongdl]

Reviewed by tracing the evaluation call stack end to end and confirming each finding empirically in a worktree at 618e26d (probe tests run, then removed; full openjd-expr suite green at 3,454 tests).

The reframing from "each node remembers to clear a mutable field" to "the target is a parameter, None by default" is the right fix — the four sites that had the bug were exactly the four that forgot to clear the field, and that failure mode no longer exists. Keeping the recursion-depth chokepoint intact through the rewrite is easy to get wrong and wasn't. Correcting the spec transcription in the same commit matters, since the wrong table is why the bug existed at all.

Call stack for reference (paths under crates/openjd-expr/src/, lines at 618e26d):

host → format_string.rs:191 resolve_inner (threads target_type)
     → eval/parse.rs:388     EvalBuilder::evaluate
     → evaluator.rs:234      evaluate(root)  ── target_type consumed HERE, once ──
     → evaluator.rs:261      eval_node(node, target)
                               ├─ depth counter (+1), the single chokepoint
                               ├─ evaluate_inner(node, target)
                               └─ evaluator.rs:298  val.coerce(target)  ◄── the ONLY coercion site
     → evaluator.rs:320      evaluate_inner: target forwarded to exactly 4 arms
                               If / List / ListComp / Call; every other arm passes None
     → evaluator.rs:957      call_arg_targets (per-arg target from candidate signatures)
     → evaluator.rs:446      dispatch_with_node → function_library multiple dispatch
     → value.rs:746          ExprValue::coerce (any=no-op → match-first → union members)

Two must fix items and two non-blocking ones, inline. Summary:

1. Must fix — an unresolved result plus a scalar target still errors (evaluator.rs:298). Same defect class as #291; union targets survive only incidentally.
2. Must fix — eval_listcomp applies the new element target on only one of its two paths (evaluator.rs:1442), so static validation now rejects a comprehension that runs fine.
3. Non-blocking — any is unconstrained in coerce but constraining in call_arg_targets (evaluator.rs:965).
4. Non-blocking — coerce is now called mid-evaluation, and its own comment states it assumes it is not (evaluator.rs:1051).

Refuted, so nobody re-investigates: I checked all 160 registered signatures for overload sets where the caller's target selects a subset with different parameter types, looking for a case where argument targeting changes a result. There isn't one in the default library — the arithmetic/conversion families (max, min, round, ceil, abs, zfill, int, float) all resolve to a union containing the value's own type, so match-first returns it unchanged, and the generic families (len, sorted, string, flatten, reversed, unique) have symbolic parameters and stay unconstrained. Verified empirically for max(1, 2.5), min(1, 2.5), zfill(1.5, 6), sum([1, 2]), all([]), int('7') — identical with and without a target. The sticky None in per_pos is also correct as written (a later candidate can't resurrect a position a symbolic candidate cleared), and depth accounting still counts the root exactly once.

Filed as a comment rather than request changes so the existing approval isn't dismissed — treat items 1 and 2 as blocking regardless.

[leongdl]

MUST FIX — an unresolved result plus a scalar target_type still errors. Same defect class as #291, still live after this PR.

ExprValue::coerce has no arm for ExprValue::Unresolved, so it falls through to _ => Err(...). Because this line coerces unconditionally on the Ok path, any expression whose value is unresolved fails the moment a non-union target is supplied:

expression                  no target            target=int                                target=int?|list[int]
Task.Param.X                OK unresolved[int]   ERR Cannot coerce unresolved[int] to int   OK unresolved[int]
Task.Param.X + 1            OK unresolved[int]   ERR Cannot coerce unresolved[int] to int   OK unresolved[int]
1 if Task.Param.B else 2    OK unresolved[int]   ERR Cannot coerce unresolved[int] to int   OK unresolved[int]
min([Task.Param.X, 3])      OK unresolved[int]   ERR Cannot coerce unresolved[int] to int   OK unresolved[int]

(Measured on this branch with ExprValue::unresolved(ExprType::INT) in the symbol table.)

Union targets survive only incidentally: ExprType::match_type unwraps Unresolved and delegates to the constraint, so the match-first branch in coerce returns early. The scalar path has no comparable check. Note this also means the unresolved-IfExp path at ~line 883 can't succeed under a scalar target — it coerces both branches, rebuilds an Unresolved(t), and then this line coerces that wrapper again and fails.

Why it matters: this is the PR's own thesis ("expressions that evaluate correctly without a target_type now also evaluate correctly with one") failing for the entire static-validation path. FormatString::validate is documented as taking unresolved(T) for symbols whose values aren't yet known, so a host validating timeout: "{{ Task.Param.X }}" with target_type=int gets a spurious error at check time — and per the PR description, spec-following hosts supply a target for args items as a matter of course.

To be fair: this is pre-existing, not a regression — the old code ran the identical val.coerce(...) at the root. But it lives in the function this PR rewrites and it's the same bug shape, so closing it here seems right rather than leaving a second target_type-makes-valid-expressions-fail case behind.

Suggested fix: an unresolved value carries no concrete value to convert, so coercion should be deferred, not attempted. Either guard here (if val.is_unresolved() { return Ok(val) }) or add an Unresolved arm in coerce that checks the constraint against the target with match_type and returns the value unchanged — the latter keeps genuinely-incompatible cases (unresolved[list[int]] → int) erroring, which the blunt guard would not.

[leongdl]

MUST FIX — elem_target is computed here but applied on only one of eval_listcomp's two paths.

It's used at line 1547 (the resolved-iterable loop), but the unresolved-iterable probe path at line 1486 still calls child.evaluate(&lc.elt), which reads the child evaluator's own target_type field — always None, set at line 1394 in child_evaluator. So the element expression is unconstrained there. Same expression, same target, different verdicts:

[[x, '2'] for x in [1]]            target list[list[int]] → OK  ListList([ListInt([1, 2])])
[[x, '2'] for x in Task.Param.L]   target list[list[int]] → ERR "List literal contains
                                                                incompatible types: int, string"

(Task.Param.L = unresolved(list[int]); the second is the exact expression from the new listcomp_element_derives_target_from_list_target test with an unresolved iterable substituted.)

Why it matters: static validation rejects a template that evaluates fine at runtime — the reverse of what validation is for. Unlike the finding on line 298, this asymmetry is introduced by this PR: before it neither path had an element target, so both failed consistently.

Suggested fix: pass elem_target.as_ref() at line 1486 as well. While there, the three remaining child.evaluate(...) calls (1467, 1486, 1531) are the last places in the evaluator that take their target from the implicit field rather than an explicit argument — exactly the pattern this PR removes everywhere else. Switching them to child.eval_node(node, ...) closes the loop and spares the reader having to go check what child_evaluator sets target_type to. A regression test with an unresolved iterable would pin it; there's currently no new test that exercises the unresolved-iterable branch at all.

[leongdl]

Non-blocking, but the two halves of this PR disagree here: any is unconstrained in coerce (the value.rs change) yet constraining in call_arg_targets.

match_type returns Some whenever either side is Any, so with target_type=any every arity-matching signature passes the return-type filter, and this function produces concrete per-position unions. any therefore behaves differently from None:

join([1, 2], '-')   no target   → ERR "No matching signature for join(list[int], string)"
join([1, 2], '-')   target=any  → ERR "Cannot coerce list[int] to list[nulltype] | list[path] | list[string]"

I could not find a case in the default library where this turns success into failure — the argument unions are always wide enough, and the any-is-a-no-op fix is in fact what keeps any-typed parameters (bool(any), and any host signature with an any param) working through argument targeting. So the observable damage today is only the worse error message. But the rules contradict each other, and the coupling is invisible: if the any no-op were ever narrowed, argument evaluation would break at a distance with no test pointing back here.

Suggested fix: early-return unconstrained when target.code() == TypeCode::Any, next to the existing method-call and dunder guards, so any and None are the same thing everywhere.

[leongdl]

Non-blocking — this call makes ExprValue::coerce run during evaluation, and coerce documents an assumption that it doesn't.

The RangeExpr → List arm in value.rs justifies an unbudgeted materialization with: "coerce() runs outside any EvalContext (post-evaluation target-type hook, public API), so no operation or memory budget applies here", capping at DEFAULT_OPERATION_LIMIT in lieu of a real budget. With signature-derived argument targets that premise no longer holds: coercion can now fire once per argument per node, including inside a comprehension body.

Reachability — plausible, not confirmed. Not in the default library: every list-typed argument position resolves to a union that either includes range_expr (max, min, sum, so match-first returns the range untouched) or skips List members entirely (join, all, which error instead). It becomes reachable for a host that registers a single (list[int]) -> int overload, where the argument target is a bare list[int] and a range_expr argument materializes up to DEFAULT_OPERATION_LIMIT i64s outside the memory limit — repeatedly, if the call sits in a comprehension body.

The PR states "evaluation resource limits are unaffected", and that's still true in practice — but it now rests on a property of the default library's signature set rather than on the code. At minimum the comment in coerce should stop asserting the old premise; better would be threading the budget through the coercion eval_node performs.

[leongdl]

Follow-up: I mutation-tested the new tests to check they actually pin the fix. Method — snapshot evaluator.rs + value.rs, apply one mutant at a time, rebuild (a mutant that fails to compile is discarded rather than counted as caught), run first the 14 new tests and then the full 3,454-test openjd-expr suite, restore from snapshot and verify by checksum. Baseline green both times.

Headline: the fix itself is well pinned. The new machinery is not pinned at all.

Mutant	New tests	Full suite (3,454)
Revert both src files to main	12 of 14 fail	—
coerce: remove the any no-op	caught (2)	—
eval_listcomp: drop element target	caught (1)	—
eval_list: drop element target	survived	caught by test_lists::nested_mixed_list_with_int_target
eval_node: skip result coercion entirely	caught (3)	—
call_arg_targets: symbolic params no longer unconstrain	caught (1)	—
call_arg_targets: drop the arity filter	caught (1)	—
call_arg_targets: drop the method/dunder guard	survived	SURVIVED
call_arg_targets: drop the return-type candidate filter	survived	SURVIVED
eval_ifexp: branches no longer inherit the parent target	survived	SURVIVED
call_arg_targets: always return unconstrained (whole feature reverted)	survived	SURVIVED

Three things fall out of that, detailed inline:

1. call_arg_targets (~100 lines) is entirely unobservable to the test suite. Replace its body with vec![None; n_args] and all 3,454 tests still pass. The two mutants that were caught (symbolic, arity) were caught by the "must stay unconstrained" tests noticing the function doing something wrong — nothing anywhere asserts it doing something right.
2. IfExp branch target inheritance has zero coverage, despite being an RFC-mandated rule this PR preserves and documents in the propagation table.
3. Two of the 14 new tests pass against unfixed main — and they're the two the PR presents as the realistic host scenario.

What the mutation run says is missing

Ordered by value. The first two would also pin the must-fix findings from my earlier review.

Pins for the open findings

• Unresolved result + scalar target: Task.Param.X + 1 with target=int must not error (currently does).
• Listcomp with an unresolved iterable + list[list[int]] target — the only listcomp test uses a concrete iterable, so the divergent path at line 1486 is untested entirely.

Behaviours with no coverage at all (from the surviving mutants)

• Something that demonstrates what call_arg_targets buys. A host-registered library with a single concrete (list[int]) -> int overload is the cleanest: the argument target is then a bare list[int] and the coercion is observable.
• A method call under a target — 'abc'.upper() with target=string, and a UFCS form — to pin the is_method_call guard.
• A dunder under a target, to pin the __x__ half of the same guard.
• Two overloads with different return types where the target selects one, to pin the return-type candidate filter.
• IfExp branch inheritance: 1 if true else 'x' with target=string → "1"; plus the unresolved-test variant, where both branches are coerced.

any coverage is int/string only. Add null, bool, float, list[int], path, range_expr, and any as an argument target (finding 3) — that last one is where any and None currently differ.

Boolean-operator edges — every and/or test returns the second operand. Missing: returning the first (5 or 0 with target=string → "5", false and 3 → "false"), 3+ operand chains (null or null or 7), and an unresolved operand mid-chain with a target.

Subscript / slice min-max-edge — negative index ([10,20,30][-1], 'hello'[-1]), out-of-bounds index, negative and omitted slice bounds ([1,2,3][-2:], 'hello'[::2]), zero step. Each with a target, and asserting the error provenance: an out-of-bounds or zero-step failure must still report the index/step error, not a coercion error. That distinction is the whole point of the three deliberate RFC deviations, and nothing currently pins it.

Numeric and empty boundaries — int target on a non-integral float element ([1.5, 2.5][0] → "not a whole number", and the caret should name the element, not the list), int target on a result outside i64 (the float_fits_i64 overflow message), i64::MAX/MIN, empty list literal with a list[int] target (element type is NULLTYPE when empty — worth pinning), single-element list, empty slice [][0:0].

Error-caret provenance for nested targeted nodes — uncoercible_result_still_errors_with_caret is a good test but only covers the root. A coercion failure in a list element or an IfExp branch now also carries a caret, and no test says where it points.

Union-target negatives — a value matching no member and coercible to no member, to pin the union error message; and the RangeExpr → list[int] coercion, which is the one argument path that materializes memory outside the budget (my finding 4).

[leongdl]

Test gap, must fix — this whole function is unobservable to the test suite. I replaced its body with an unconditional return vec![None; n_args] (reverting the entire argument-targeting feature, i.e. both of the last two commits' behaviour) and all 3,454 openjd-expr tests still pass, including all 14 new ones.

The two internal mutants that were caught are worth reading carefully, because of what they imply:

• removing the arity filter → caught by call_arguments_not_constrained_by_caller_target
• removing the symbolic-parameter check → caught by generic_call_arguments_stay_unconstrained

Both of those tests assert an argument is unconstrained. They fail when this function starts computing a wrong target, not when it stops computing a right one. So the suite pins "argument targeting does no harm" and never "argument targeting does something". The method/dunder guard and the return-type candidate filter survive against the full suite too — nothing pins either.

This matters more than a normal coverage gap because I could not find a single expression in the default library where argument targeting changes a result (I checked all 160 registered signatures; details in my other review). Every list-typed argument position resolves to a union containing the value's own type, so match-first returns it unchanged. The one behaviour difference I did find is a worse error message: join([1, 2], '-') reports "No matching signature for join(list[int], string)" with no target, but "Cannot coerce list[int] to list[nulltype] | list[path] | list[string]" with target=string.

I'm not suggesting deleting it — it's RFC 0005 compliance and a host with its own signatures can observe it. But ~100 lines of new dispatch logic with zero pinning tests is how the original bug survived: the propagation table was wrong and nothing caught it. Suggest at least one test where the computed argument target is observable. The cleanest is a host-registered library with a single concrete (list[int]) -> int overload, so the argument target is a bare list[int] rather than a union that matches anyway — that also exercises the RangeExpr→list materialization path. Failing that, a comment recording that the function is currently a no-op for the default library's signature set would at least stop the next reader assuming it's load-bearing.

[leongdl]

Zero coverage — I changed both of these to None (so branches no longer inherit the parent target) and all 3,454 openjd-expr tests still passed. Same for the concrete-test path at lines ~918/~921.

This is an RFC-mandated rule that the PR preserves, documents in the propagation table, and calls out by name in the eval_node doc comment as one of only four slots that receive a non-None target — so it's load-bearing by design and unpinned in practice. Pre-existing gap, not introduced here, but this PR is where the rule got written down.

1 if true else 'x' with target=string → "1" covers the concrete path in one line. The unresolved-test path is the more interesting one, since it coerces both branches (lines 864-865) and then rebuilds an Unresolved(t) from the coerced type — which, per my other review, this line's result can no longer coerce at the outer eval_node. A test with an unresolved condition and a scalar target would pin both behaviours at once.

[leongdl]

These two tests pass against unfixed main. I restored evaluator.rs and value.rs from 89591ea while keeping this whole test file: 12 of the 14 new tests fail, and the two survivors are args_style_union_target_with_subscript and args_style_union_target_with_or_fallback.

The reason is coerce's match-first union rule. With a target of int? | list[int], the leaked receiver [10, 20, 30] is list[int] — already a union member — so the old code's spurious coercion was a no-op and returned it unchanged. Same for the or case: null matches the nulltype member of string? | list[string] and 'fallback' matches string. The union target is exactly what made case A and case B harmless.

That's worth reconciling with the impact section, which says the spec's T? | list[T] rule for args items means "any host following the spec hit case A for every subscript and case B for every or-based fallback in an args field". These two tests are the evidence for that claim and they demonstrate the opposite. The genuinely broken cases were the scalar targets — int for a timeout, path, string — and those are well covered by the other 12.

Not arguing for deleting them; they're useful characterization tests for the union path and they'd catch a future regression in match-first. But as written they cannot fail for this fix, so they shouldn't be counted as regression coverage, and the impact paragraph probably wants narrowing to scalar targets.

[leongdl]

Escalating the must-fix items from my two earlier reviews to a blocking state. Everything here is already detailed in the inline threads — linked rather than repeated, to avoid duplicating the walls of text. The design of the fix is right; these are three specific gaps in it.

1. An unresolved result plus a scalar target_type still errors — evaluator.rs:298

ExprValue::coerce has no Unresolved arm, so it falls through to _ => Err(...), and this line coerces unconditionally on the Ok path. Measured on this branch:

Task.Param.X               no target → OK unresolved[int]   target=int → ERR "Cannot coerce unresolved[int] to int"
Task.Param.X + 1           no target → OK unresolved[int]   target=int → ERR
1 if Task.Param.B else 2   no target → OK unresolved[int]   target=int → ERR
min([Task.Param.X, 3])     no target → OK unresolved[int]   target=int → ERR

Union targets survive only incidentally — match_type unwraps Unresolved and delegates to the constraint, so match-first returns early. The scalar path has no equivalent check.

This is the PR's own thesis failing for the whole static-validation path: FormatString::validate is documented as taking unresolved(T) for not-yet-known symbols, so validating timeout: "{{ Task.Param.X }}" with target_type=int errors at check time. Pre-existing rather than a regression — the old code ran the identical val.coerce(...) at the root — but it's the same defect class as #291 and it lives in the function this PR rewrites. Fix: guard on val.is_unresolved() here, or add an Unresolved arm to coerce that checks the constraint with match_type (the latter keeps unresolved[list[int]] → int erroring, which the blunt guard would not).

2. eval_listcomp applies the new element target on only one of its two paths — evaluator.rs:1442

elem_target is used at line 1547 but not at line 1486, where the unresolved-iterable probe path still calls child.evaluate(&lc.elt) and picks up the child's own always-None field:

[[x, '2'] for x in [1]]            target list[list[int]] → OK
[[x, '2'] for x in Task.Param.L]   target list[list[int]] → ERR "List literal contains incompatible types: int, string"

Static validation therefore rejects a comprehension that runs fine. This asymmetry is introduced by this PR — before it neither path had an element target, so both failed consistently. Fix: pass elem_target.as_ref() at 1486, and while there convert the three remaining child.evaluate(...) calls (1467, 1486, 1531) to child.eval_node(node, ...) — they are the last places in the evaluator taking a target from the implicit field, which is the pattern this PR removes everywhere else.

3. call_arg_targets is unobservable to the test suite — evaluator.rs:964

Replacing its body with an unconditional return vec![None; n_args] — reverting the entire argument-targeting feature — leaves all 3,454 openjd-expr tests passing, including all 14 new ones. The two internal mutants that were caught (arity filter, symbolic check) were caught by tests asserting an argument is unconstrained; they fire when the function computes a wrong target, never when it stops computing a right one. The method/dunder guard and the return-type candidate filter survive the full suite too.

Blocking rather than a normal coverage note for two reasons. First, ~100 lines of new dispatch logic with no pinning test is exactly how the original bug survived — the propagation table was wrong and nothing caught it. Second, I could not find a single default-library expression where argument targeting changes a result (checked all 160 registered signatures), and the one behaviour difference I did find is a worse error message: join([1, 2], '-') reports No matching signature for join(list[int], string) with no target but Cannot coerce list[int] to list[nulltype] | list[path] | list[string] with target=string. One test where the computed argument target is observable — a host-registered library with a single concrete (list[int]) -> int overload is cleanest — would settle both.

Not blocking, but worth reconciling

args_style_union_target_with_subscript and args_style_union_target_with_or_fallback pass against unfixed main (12 of 14 new tests fail; those two don't). A union target is what made cases A and B harmless, so the impact paragraph's claim about args items is contradicted by the two tests offered as its evidence. The genuinely broken cases were scalar targets, and those are well covered.

Happy to send a PR against your branch with the fixes and mutation-verified regression tests if that's useful — say the word.

[leongdl]

Correction to my earlier review, up front. I wrote that I had checked all 160 registered signatures and that "there isn't one in the default library" where argument targeting changes a result. That was wrong, and I'd rather flag it prominently than have it quietly discount what follows. I reasoned that every argument position resolves to a union containing the value's own type — true for the arithmetic families I probed, but I missed two shapes: a value whose type is in no member (a bool or float in a string parameter slot), and accumulating functions where coercing the arguments first changes the arithmetic. Both are reachable with the default library.

I attributed each case below by running it twice — once on the branch as-is, once with call_arg_targets stubbed to always return unconstrained — so "caused by this PR" vs "pre-existing in coerce" is measured, not inferred.

---

A. Argument targeting silently widens the type system for string parameter slots — caused by this PR

Because a per-position union like float | int | string is tried in member order and string is reachable by coercion from bool, float and path, a value that matches no member gets stringified. Expressions that are type errors without a target succeed with one:

expression	no target	with target
upper(true)	ERR No matching signature for upper(bool)	OK "TRUE"
lower(true)	ERR	OK "true"
ljust(true, 6)	ERR	OK "true "
zfill(true, 6)	ERR	OK "00true"
center(1.5, 7)	ERR	OK " 1.5 "
startswith(true, 't')	ERR	OK true
split(true)	ERR	OK ["true"]
replace(true, 't', 'T')	ERR	OK "True"

(target=string, or bool for startswith; every one of these reverts to the error with call_arg_targets stubbed out.) len(true) correctly still errors, because len's (list[T1]) overload is symbolic and unconstrains the position — the design working as intended.

"00true" as a zero-padded value is the clearest illustration: the host asked for a string result and got a nonsense one instead of a type error. Note this also means the reachability caveat I put on my coerce-runs-mid-evaluation comment was too generous — argument coercion is observable in the default library after all, so that finding is confirmed rather than plausible.

B. Argument targeting can swallow an integer-overflow error — caused by this PR

sum([9223372036854775807, 1])   no target    → ERR "Integer overflow: result is outside the 64-bit signed range"
sum([9223372036854775807, 1])   target=float → OK  9.223372036854776e+18
sum([9223372036854775807, 1])   target=int   → ERR "Integer overflow..."   (guard still applies)

With target=float the only candidate is (list[float]) -> float, so the argument is coerced to list[float] before summing and the accumulation happens in f64, where there is no overflow to detect. The target_type decides whether the overflow guard runs. 9223372036854775807 + 1 is unaffected — __add__ is a dunder and the guard skips it.

Same mechanism, non-overflow flavour, also caused by this PR:

sum([9007199254740993, 1])   no target → 9007199254740994    (exact i64 sum, then coerced)
sum([9007199254740993, 1])   float     → 9007199254740992.0  (each arg → f64 first, +1 vanishes)

Off by 2. This is the invariant call_arg_targets' own doc comment states — "targets guide coercion of results, they must not change the computation" — being violated. You already found and fixed the sorted instance of exactly this category; these are two more instances of it, on the concrete-parameter path rather than the generic one. Skipping coercion for a position whose candidate types the value doesn't match (leaving dispatch to decide, as it did before) would close A and B together.

C. int → float loses precision, contradicting the documented invariant — pre-existing

The doc comment on this function says coercion is non-destructive and names int → float as an example. It isn't, above 2^53:

9007199254740993        target=float → 9007199254740992.0      (off by one)
9223372036854775807     target=float → 9.223372036854776e+18   (i64::MAX)
[9007199254740993]      target=list[float] → [9007199254740992.0]
max(9007199254740993, 1) target=float → 9007199254740992.0

Identical with call_arg_targets stubbed out, so this is the root/result coercion, not argument targeting — pre-existing and out of scope for this PR. But the comment is a claim under test and currently false; either narrow it (int → float is lossy above 2^53) or reject the conversion when i as f64 as i64 != i.

D. Union coercion order is alphabetical, so the answer depends on member spelling — pre-existing

normalize_union sorts members by to_string(), and the per-member loop in coerce returns the first success. So the tried order is bool → float → int → path → range_expr → string, regardless of how the host wrote the union:

'1'      target=bool | int       → Bool(true)      (not Int(1))
'0'      target=bool | int       → Bool(false)
'on'     target=bool | int       → Bool(true)
'10'     target=float | int      → Float(10.0)
'10'     target=int | float      → Float(10.0)     (spelling ignored; float sorts first)
'10'     target=int              → Int(10)
'2-5'    target=int | range_expr → RangeExpr(2-5)
'/tmp/x' target=int | path       → Path("/tmp/x")

The bool cases are the sharp edge: from_str_coerce's bool branch accepts 1/0/on/off/yes/no, so a host asking for bool | int gets true for the string "1" rather than the integer. Only reachable when the value's own type is in no member (match-first handles the rest), and I found no default-library argument position that produces a bool-containing union — so this is latent rather than live. Worth a sentence in specs/expr/values.md since it's currently undocumented and a host composing a union target would not guess it.

What I checked and found correct

float_fits_i64 is right, and it's the kind of check that is usually subtly wrong — the half-open (-2^63 .. 2^63) range means -9223372036854775808.0 coerces to exactly i64::MIN while 9223372036854775807.0 correctly reports overflow rather than saturating. 2.5 → int gives "is not a whole number", 1e308 * 10 is caught by the infinity guard before coercion, string→int uses parse::<i64> so it errors rather than wrapping, string→float rejects inf/NaN, and an empty list literal under a list[int] target coerces cleanly despite its nulltype element type. None of the odd cases above come from a missing bounds check; they all come from which conversion gets chosen.

[leongdl]

This is where findings A and B land — and where my earlier claim that argument targeting changes no result in the default library turned out to be wrong. Two shapes I missed, both measured by comparing the branch against a build with this function stubbed to return vec![None; n_args]:

A. A value whose type is in no member of the position's union gets coerced anyway. zfill's pos0 union is float | int | string; a bool matches none, so the loop reaches string and stringifies it. That makes type errors succeed:

upper(true)             none=ERR No matching signature for upper(bool)   target=string → OK "TRUE"
zfill(true, 6)          none=ERR                                          target=string → OK "00true"
center(1.5, 7)          none=ERR                                          target=string → OK "  1.5  "
split(true)             none=ERR                                          target=list[string] → OK ["true"]
replace(true,'t','T')   none=ERR                                          target=string → OK "True"
startswith(true, 't')   none=ERR                                          target=bool → OK true

All of them revert to the error with this function stubbed out. len(true) correctly still errors — (list[T1]) is symbolic so the position is unconstrained, which is the design working.

B. Coercing arguments before the call changes the arithmetic, including whether an overflow is detected.

sum([9223372036854775807, 1])   none=ERR "Integer overflow..."   target=float → OK 9.223372036854776e+18
sum([9223372036854775807, 1])                                    target=int   → ERR "Integer overflow..."
sum([9007199254740993, 1])      none=OK 9007199254740994          target=float → OK 9007199254740992.0

With target=float the only candidate is (list[float]) -> float, so each element becomes f64 before the accumulation and the i64 overflow check never runs. The target_type decides whether the guard applies. Both revert with the function stubbed.

This is the invariant stated in this function's own doc comment — "targets guide coercion of results, they must not change the computation" — being violated on the concrete-parameter path. You already found and fixed the generic-parameter instance (sorted reordering); these are the same category, two doors down.

Suggested fix: skip the coercion when the value's type matches none of the position's candidate types, and let dispatch decide as it did before — dispatch's own signature-driven coercion is both narrower and where the "No matching signature" diagnostic comes from. That closes A, B, and the error-message degradation on join/all/any in one change, and it makes the earlier any-vs-None inconsistency moot for most cases.

[leongdl]

Non-blocking, pre-existing — but this comment is a claim under test and it's false above 2^53. int → float is named here as an example of a conversion that doesn't lose information; the arm at line 781 is a bare *i as f64:

9007199254740993       target=float → 9007199254740992.0     (off by one)
9223372036854775807    target=float → 9.223372036854776e+18  (i64::MAX)
[9007199254740993]     target=list[float] → [9007199254740992.0]

Identical with call_arg_targets stubbed out, so this is the result coercion rather than anything this PR introduced — flagging it because the PR touches this doc comment's function and the comment will now be read as authoritative on the new argument-coercion path too, where a silently-wrong large integer is harder to notice.

Either narrow the sentence (int → float is lossy above 2^53) or make the arm reject when i as f64 as i64 != i. The latter is consistent with how the reverse direction already behaves — float → int refuses 2.5 rather than truncating, and float_fits_i64 is careful about exactly this class of boundary.

While in this function: normalize_union sorts members by to_string(), so the per-member coercion loop below tries them alphabetically — bool, float, int, path, range_expr, string. That makes the result depend on member names rather than on the order the host wrote them ('10' with int | float yields Float(10.0), and '1' with bool | int yields Bool(true) because from_str_coerce accepts 1/0/on/yes as bools). Latent rather than live — no default-library argument position produces a bool-containing union — but it's undocumented, and specs/expr/values.md is the natural place for a sentence about it.

[leongdl]

I went looking for Python↔Rust conversion loss, since that's where a precision bug would be invisible. Two things came out of it: the boundary itself is in good shape, and I need to refine my attribution of finding A — it was too broad, and the accurate version is more useful.

Measured against the released extension (openjd-model 0.11.1, i.e. pre-PR openjd-rs) using evaluate_expression(..., target_type=...).

First: the released Python confirms #291 is live breakage

[10, 20, 30][0]   target=int → ExpressionError: Cannot coerce list[int] to int
null or 7         target=int → ExpressionError: Cannot coerce nulltype to int
1                 target=any → ExpressionError: Cannot coerce int to any

All three fixed by this PR. Worth having in the thread — the fix isn't theoretical.

Correction to finding A: this PR fixes half the leak, and the surviving half changes errors into wrong answers

I said the bool-in-a-string-slot table was "caused by this PR". That's wrong for the single-argument functions — the released code produces the same output by the cruder route:

expression	released (pre-PR)	PR branch	verdict
upper(true)	'TRUE'	'TRUE'	pre-existing, preserved
lower(true)	'true'	'true'	pre-existing, preserved
replace(true, 't', 'T')	'True'	'True'	pre-existing, preserved
ljust(true, 6)	ERR No matching signature for ljust(string, string)	'true '	new success
zfill(true, 6)	ERR ... zfill(string, string)	'00true'	new success
center(1.5, 7)	ERR ... center(string, string)	' 1.5 '	new success
startswith(true, 't')	ERR Cannot convert 't' to bool	true	new success
split(true)	ERR Cannot coerce bool to list[string]	["true"]	new success
sum([9223372036854775807, 1]) (float)	ERR Cannot coerce list[int] to float	9.223372036854776e+18	new — overflow guard bypassed
sum([9007199254740993, 1]) (float)	ERR	9007199254740992.0	new — off by 2

The mechanism is clearer than "this PR broke it". The old leak applied the caller's target to every argument, so on a multi-argument function it mangled the numeric second argument too and the call died at dispatch with a loud "No matching signature". Per-position targeting correctly fixes the second argument — and thereby unmasks the first argument's stringification, which was always there. A loud error becomes a quiet wrong answer.

So the honest framing: the operand-leak fix eliminated this class for subscripts, boolops and slices, but for call arguments it narrowed the leak rather than closing it. Finishing the job is the same one-line suggestion as before — don't coerce a position whose candidate types the value matches none of; leave it to dispatch, which is where "No matching signature" comes from. That turns all ten rows into errors and closes the sum cases at the same time.

The boundary itself: no trampoline needed for the numeric path

I read rust-bindings/src/expr/expr_value.rs in openjd-model-for-python and exercised it. The things that usually go wrong here are handled:

• PyBool is cast before PyInt. Python's bool is an int subclass, so the naive order silently turns True into 1. Confirmed correct: ExprValue(True).type == 'bool' and .item() returns a Python bool.
• Python int → i64 raises rather than wrapping or saturating. 2**63, -(2**63)-1 and 10**30 all give ExpressionError: Integer overflow: result is outside the 64-bit signed range, and the binding deliberately rewrites PyO3's OverflowError text to that canonical message. 2**63-1 and -(2**63) round-trip exactly.
• NaN and ±infinity are rejected on ingestion, for float and Decimal alike, with the reference's messages.
• Decimal is detected with a real isinstance, so subclasses work and unrelated same-named classes don't.
• Unresolved raises on the way out instead of producing a junk Python value — consistent with unresolved values being validation-time only.
• openjd.expr is a pure re-export of the extension, and there's no try_coerce_nondestructive left in the Python tree. There is exactly one coercion implementation today, so there's no Python-vs-Rust coercion parity risk to reconcile — which is the main reason no trampolining is required.

Two places where trampolining would help (bindings repo, not this PR)

1. Decimal is one-way. expr_value_to_py does ExprValue::Float(f) => f.value(), dropping Float64::original. The original text survives for display but not for extraction:

str(ExprValue(Decimal('0.10')))                     → '0.10'
ExprValue(Decimal('0.10')).item()                   → 0.1
ExprValue(Decimal('12345678901234567890.5')).item() → 1.2345678901234567e+19

A host passing Decimal to preserve exact decimal text gets a lossy float back. Format-string resolution is fine because it goes through the display path — it's only native extraction that loses it. Returning a Decimal when original is present, or exposing the original text on ExprValue, would make it round-trip safe.

2. path is one-way in both directions. pathlib.Path isn't an accepted input (ExprValue(Path('/tmp/x')) → TypeError: Cannot convert PosixPath to ExprValue), and a path value extracts as a plain str, dropping PathFormat. So a host that round-trips a path value through Python turns it into a string, after which __truediv__(path, string), .name, .parent and friends no longer match. Accepting os.PathLike on ingestion is a small, safe addition; the extraction side is arguably intentional but worth documenting.

The one precision loss that does cross the boundary silently

Finding C, seen from Python — the host gets a plain float with no signal that a digit was dropped:

evaluate_expression('9007199254740993', target_type='float') → 9007199254740992.0
evaluate_expression('9007199254740993', target_type='int')   → 9007199254740993
evaluate_expression('9223372036854775807', target_type='float') → 9.223372036854776e+18

Pre-existing in coerce, not this PR — but it's the one case where the careful i64 range checking at the boundary is undone afterwards by an unchecked as f64. A host that validated its integer fits in 64 bits, and then asked for a float, silently gets a different number.

[leongdl]

Refining my earlier attribution here, now that I've compared against the released extension (openjd-model 0.11.1) rather than only against a stubbed build of this function.

For single-argument string functions the stringification is pre-existing, not introduced here — released and PR branch agree:

upper(true)              target=string  released='TRUE'   PR='TRUE'
lower(true)              target=string  released='true'   PR='true'
replace(true,'t','T')    target=string  released='True'   PR='True'

For multi-argument functions the PR changes the outcome, and not in the good direction:

ljust(true, 6)                 released=ERR No matching signature for ljust(string, string)  PR='true  '
zfill(true, 6)                 released=ERR ... zfill(string, string)                        PR='00true'
center(1.5, 7)                 released=ERR ... center(string, string)                       PR='  1.5  '
startswith(true, 't')          released=ERR Cannot convert 't' to bool                       PR=true
split(true)                    released=ERR Cannot coerce bool to list[string]               PR=["true"]
sum([9223372036854775807, 1])  released=ERR Cannot coerce list[int] to float                 PR=9.223372036854776e+18

The mechanism is worth stating plainly because it reframes the fix. The old leak applied the caller's target to every argument, so on a two-argument call it also mangled the numeric second argument and the call died loudly at dispatch. Per-position targeting correctly fixes the second argument — and in doing so unmasks the first argument's stringification, which was there all along. The net effect is that a "No matching signature" error becomes a silently wrong value, and in the sum case an "Integer overflow" error becomes a silently inexact float.

So this isn't "the new function broke it" — it's "the new function fixes half of it". The operand-leak fix closed this class for subscripts, boolops and slices; for call arguments it narrowed the leak instead of closing it.

The suggestion is unchanged and now covers all of it: don't coerce a position when the value's type matches none of that position's candidate types — leave it to dispatch, which is both narrower and the source of the "No matching signature" diagnostic. That turns every row above back into an error, closes the sum overflow and off-by-2 cases, and makes the earlier any-vs-None inconsistency mostly moot. Positions where the value does match a candidate type keep working exactly as they do now.

[mwiebe]

I've filed the follow-up spec PR OpenJobDescription/openjd-specifications#168