fix(expr)!: Type-check a membership item against the list's element type by leongdl · Pull Request #396 · OpenJobDescription/openjd-rs

leongdl · 2026-09-15T04:20:45Z

Fixes: openjd-specifications fixture EXPR/job_templates/proposed/expr2.1.3--membership-element-type-mismatch.invalid.yaml on OpenJobDescription/openjd-specifications#164 (no openjd-rs issue is filed for this)

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

The Expression Language spec (§2.1.3) gives list membership one type variable, __contains__(list: list[T], item: T), and the range overload a concrete item type, (range_expr, int). openjd-expr registered (list[T1], T2) and (range_expr, T1), so any item type matched any container and membership was decided by a loop over ==, which the spec makes total across types (§1.2.5). "a" in [1, 2] therefore evaluated to false where it has no signature at all.

A sweep of the six-by-six item/element type grid found the same for every one of the thirty mismatched pairs, not in returned true for all thirty, and it held through a comprehension and one level down a nested list. Every neighbouring operation does check: [1, "a"], [1, 2] + ["a"], "a" < 1 and 1 in "abc" are all refused. The list overload was the outlier.

The mistake is silent. A mistyped membership test is an ordinary authoring error, and it produced a running job instead of a diagnostic. Worse, eval_compare returned Unresolved(BOOL) without dispatching as soon as either operand was a parameter, so even a correct signature would have fired for literals only: Param.S in [1, 2] with a STRING parameter would still pass openjd check and fail on the worker.

What was the solution? (How)

Two commits.

1. fix(expr)!: Type-check a membership item against the list's element type. Register the spec's signatures and teach generic dispatch to reconcile a type variable that two parameters bind. Changing the signature string alone was measured first and breaks 1 in [1.0, 2.0], 1.0 in [1, 2], both path/string pairs and 1 in [], because try_coerce_types passes a symbolic parameter through unchanged and the second match_call conflicts again. So the fix is types::unify_binding, used by match_call, match_type and match_signature_recursive:

Bindings	Result
identical types	that type
`int` / `float`	`float`
`path` / `string`	`string`
`range_expr` / `list[int]`	`list[int]`
`list[A]` / `list[B]`	`list[unify(A, B)]`; a `nulltype` element (the empty list) yields
a union and a type any member unifies with	the union
anything else, incl. a bare `nulltype` against a scalar	conflict

list[<var>] against list[nulltype] binds nothing, so [] never pins T. The coercible pairs are the language's non-destructive implicit coercions (§1.2.3). The range overload becomes (range_expr, int | float), keeping the exact int/float rule the equality operators use so 1.0 in range_expr('1-3') agrees with 1.0 in [1, 2, 3].

2. fix(expr): Type-check comparisons whose operand is a parameter. eval_compare dispatches even when an operand is unresolved. call_inner already matches an unresolved value's constraint type against the signatures and returns Unresolved(<ret>) on success, so a pair with no signature is refused at validation and a pair with one still defers only its value. The ordering operators are (T1, T2) and keep their run-time cross-type refusal in do_compare; tightening that is a separate change.

What is the impact of this change?

Refused at validation, with the caret on the comparison:

"a" in [1, 2]              Cannot use 'in' operator with list[int] and string
1 in ["a", "b"]            ... list[string] and int
true in [1, 2]             ... list[int] and bool
null in [1, 2]             ... list[int] and nulltype
["a"] in [[1], [2]]        ... list[list[int]] and list[string]
"3" in range_expr("1-5")   ... range_expr and string
Param.S in [1, 2]          (STRING parameter) ... list[int] and string, at check

Still accepted and decided by value: 1 in [1.0, 2.0] (true), 3 in [1.0, 2.0] (false), 1.0 in [1, 2], 1.0 in range_expr('1-3'), path(['/a']) in ['/a'], '/a' in [path(['/a'])], [1] in [[1.0]], 1 in [], [] in [[1]].

No public API signature changes. unify_binding is pub(crate).

How was this change tested?

cargo test --workspace on the tree rebased onto be91939: 7680 passed, 0 failed. cargo clippy --all-features --all-targets --workspace -- -D warnings and cargo fmt --check clean; cargo doc no warnings.
New tests: unify_binding unit tests per row of the table above; match_call on a repeated variable; integration tests for the refused pairs, the preserved coercions, the empty list, the caret span (in test_error_formatting.rs), unresolved items of matching and mismatching type; and two openjd-model tests asserting field path + message through decode_job_template, one with a literal and one with a STRING parameter.
Three existing tests pinned the old false (list_containment_with_different_types_returns_false, float_in_range_matches_python's string case, and range_and_list_containment_agree_at_boundary, which passes unchanged) and are replaced or kept.
Mutation check: five mutants each fail at least one named test — old signature (4 tests), unresolved short-circuit restored (2), int/float unify dropped (4), [] pins T (1), nulltype yields at top level (1).
Full OpenJD conformance suite at openjd-specifications 37c8353: 1182 passed, 0 failed, on the rebased tree. The parked fixture expr2.1.3--membership-element-type-mismatch.invalid.yaml on openjd-specifications#164 now passes.

Was this change documented?

Yes. specs/expr/function-library.md records the new signatures, the unification step in three-phase dispatch, and retires the paragraph that documented (list[T1], T2) as deliberate. specs/expr/type-system.md gains a "Type variable unification" table. specs/expr/evaluator.md documents that unresolved comparison operands still dispatch. specs/expr/public-api.md's match_call doc names the rule. Doc comments on unify_binding, the registration site, range_contains and eval_compare explain why.

Is this a breaking change?

Yes, commit 1 carries a BREAKING CHANGE footer. item in list / item not in list now fail signature resolution when the item's type is neither the element type nor implicitly coercible to it (int/float, path/string, range_expr/list[int]); they previously evaluated to false/true. item in range_expr accepts only int or float items; a string, bool or list item is now an error rather than false. Commit 2 moves the failure for Param.X in [...] type mismatches from run time to validation time.

Templates that relied on a cross-type membership test evaluating to false must compare types that can match. Under the spec such a template never type-checked.

Does this change impact security?

No.

Open question for upstream, not blocking

The spec does not say which of §1.2.3's conversions may reach a T already bound by another argument. This PR uses the non-destructive pairs (int/float, path/string, range_expr/list[int]), which is the same set §2.1.4 names as "compatible pairs" for ordering and §2.1.3 uses for list concatenation. Under RFC 0005's can_coerce table, which also has int/float/bool → string, 1 in ["1", "2"] would be true; here it is an error, consistent with startswith(5, "5") being refused today. Worth a clarification issue on openjd-specifications.

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

…anics Three findings from the automated review of OpenJobDescription#396, all in the new type variable unification, all reproduced before changing anything. 1. `unify_binding(int, any)` conflicted. openjd-model rebinds a failed `let` as `unresolved[any]` so later bindings do not cascade, and with `(list[T], T)` a membership test against such a binding added a second, misleading error: `Cannot use 'in' operator with list[int] and any` under the real one. Reproduced with `Foo = 1 + 'x'` then `Bar = Foo in [1, 2]`. `any` now reconciles with anything, as it already does in `match_type`. 2. The `match_type` fast path that made `list[<var>]` bind nothing against `list[nulltype]` was not scoped to membership. For the one-parameter list generics whose return mentions the variable (`sorted`, `reversed`, `unique`, and `list * int`) an unresolved empty-list argument then resolved to `unresolved[list[T1]]`, a leaked type variable, where it had been `list[nulltype]`. Reproduced through `resolve_call` and through `sorted(E)` with `E` unresolved. The fast path is gone. Instead `match_call` and the recursive matcher treat such a match as a *weak* binding: any other parameter's binding wins outright (`1 in []` binds `T = int`), and a variable nothing else binds is `nulltype` as before. 3. `unify_inner` indexed `params[0]` on `List`-coded types without checking the list has an element type. `ExprType::new(TypeCode::List, vec![])` is constructible and the repo treats it as a real input elsewhere; two arms could panic on it. They now use `params.first()` / slice patterns and unify to `None`. Tests: unit tests for each rule in types.rs, an integration test for the `any` item and for the empty-list return types, and an openjd-model test for the exact `let` cascade. Four mutants (Any arms removed; weak bindings never applied; `[]` binds strongly; unguarded list index) each fail a named test. Spec docs updated to say the weak-binding rule applies to every signature. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>

Review finding on OpenJobDescription#396, reproduced: membership dispatches container-first, the reverse of the source order, so 'a' in [1, 2] reported "with list[int] and string", and [1, 2] in [1, 2] reported "with list[int] and list[int]", two identical types that read as a bug in the type checker. That expression evaluated to false before this PR, so the message is the primary user-visible artifact of the change. When no __contains__ signature matches and the container is a list, the error now reads: Cannot use 'in' operator: item of type list[int] is not compatible with the element type int of list[int]. A range_expr container gets the same shape. A string haystack keeps the generic message, which this PR did not change. The seven integration and two model tests asserting the old wording are updated, and [1, 2] in [1, 2] is added as the case that motivated it. Restoring the generic path fails both caret tests and the mismatch table. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>

mwiebe · 2026-09-15T15:04:30Z

Nit: this error message has the types in the wrong order for the inpt example data: Cannot use 'in' operator with list[int] and string EDIT: this was already addressed in the code change, problem was just in PR description.

The Expression Language spec gives list membership one type variable, `__contains__(list: list[T], item: T)`, and the range overload a concrete item type, `(range_expr, int)`. openjd-expr registered `(list[T1], T2)` and `(range_expr, T1)`, so any item type matched any container and membership was decided by a loop over `==`, which the spec makes total across types. `"a" in [1, 2]` therefore evaluated to `false` where it has no signature at all, and so did every one of the thirty mismatched item/element pairs a sweep of the six-by-six type grid found. The mistake is silent: a mistyped membership test produces a running job instead of a diagnostic. Register the spec's signatures, and teach generic dispatch to reconcile a type variable that two parameters bind. `types::unify_binding` accepts the same type or one of the language's non-destructive coercible pairs, int/float, path/string and range_expr/list[int], binding the wider type; lists reconcile element-wise, with a nulltype element (the empty list) yielding to the other side; a union agrees if any member does; anything else is a conflict. `list[<var>]` against `list[nulltype]` binds nothing, so `[]` never pins T for the other argument. The range overload becomes `(range_expr, int | float)`, keeping the exact int/float rule the equality operators already use so that `1.0 in range_expr('1-3')` agrees with `1.0 in [1, 2, 3]`. Changing the signature string alone was measured first and breaks `1 in [1.0, 2.0]`, `1.0 in [1, 2]`, both path/string pairs and `1 in []`, because try_coerce_types passes a symbolic parameter through unchanged and the second match_call conflicts again. That is why the fix is in binding reconciliation and not in the registration. Now refused at validation, with the caret on the comparison: "a" in [1, 2] Cannot use 'in' operator with list[int] and string 1 in ["a", "b"] ... list[string] and int true in [1, 2] ... list[int] and bool null in [1, 2] ... list[int] and nulltype ["a"] in [[1], [2]] ... list[list[int]] and list[string] "3" in range_expr("1-5") ... range_expr and string Still accepted and decided by value: `1 in [1.0, 2.0]` (true), `3 in [1.0, 2.0]` (false), `1.0 in [1, 2]`, `path(['/a']) in ['/a']`, `'/a' in [path(['/a'])]`, `[1] in [[1.0]]`, `1 in []` and `[] in [[1]]`. Tests: unify_binding unit tests per rule, match_call on a repeated variable, and integration tests for the refused pairs, the preserved coercions, the empty list, and the caret span. Three existing tests pinned the old `false` and are replaced. Five mutants (old signature; no int/float unify; `[]` pins T; nulltype yields at top level; unresolved short-circuit) each fail at least one named test. Full conformance suite at openjd-specifications 37c8353: 1182 passed, 0 failed, and the parked fixture EXPR/job_templates/proposed/expr2.1.3--membership-element-type-mismatch.invalid.yaml on openjd-specifications#164 now passes. The specs (function-library.md, type-system.md, public-api.md) record the new signatures and the unification table, and retire the paragraph that documented `(list[T1], T2)` as deliberate. BREAKING CHANGE: `item in list` and `item not in list` now fail signature resolution when the item's type is neither the list's element type nor implicitly coercible to it (int/float, path/string, range_expr/list[int]); they previously evaluated to `false`/`true`. `item in range_expr` accepts only int or float items; a string, bool or list item is now an error rather than `false`. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>

eval_compare returned Unresolved(BOOL) as soon as either operand was unresolved, without dispatching. So the membership check landed by the previous commit fired for literals only: `"a" in [1, 2]` failed at `openjd check`, but `Param.S in [1, 2]` with a STRING parameter passed check and failed on the worker with the same message. Dispatch the operands anyway. `call_inner` already matches the constraint type of an unresolved value against the operator's signatures and returns Unresolved(<return type>) on success, so a pair that has no signature is refused at validation time and a pair that has one still defers only its value. A chain with an unresolved link evaluates to Unresolved(BOOL) unless an earlier link is statically false, as before. This reaches every comparison operator, and changes the outcome only where a signature is missing. `==`, `!=` and the ordering operators are registered `(T1, T2)`, so they keep type-checking as they did; a cross-type ordering such as `Param.S < 1` is still refused only at run time inside do_compare, which is a separate change. The string overload of `in`, `(string, string)`, is now enforced statically too: `Param.N in 'abc'` with an INT parameter fails check. Tests: unresolved[int] and unresolved[string] items against list[int] in openjd-expr, and the same through decode_job_template in openjd-model with the field path. Reverting the dispatch fails both. evaluator.md documents the new rule. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>

On a Windows runner the host path format renders path(['/a']) as \a, so path(['/a']) in ['/a', '/b'] is false there. That is the value comparison answering correctly for its platform, not the signature check under test. Evaluate the path/string cases under an explicit POSIX format, and add the Windows-format twin so both renderings are pinned. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>

…anics Three findings from the automated review of OpenJobDescription#396, all in the new type variable unification, all reproduced before changing anything. 1. `unify_binding(int, any)` conflicted. openjd-model rebinds a failed `let` as `unresolved[any]` so later bindings do not cascade, and with `(list[T], T)` a membership test against such a binding added a second, misleading error: `Cannot use 'in' operator with list[int] and any` under the real one. Reproduced with `Foo = 1 + 'x'` then `Bar = Foo in [1, 2]`. `any` now reconciles with anything, as it already does in `match_type`. 2. The `match_type` fast path that made `list[<var>]` bind nothing against `list[nulltype]` was not scoped to membership. For the one-parameter list generics whose return mentions the variable (`sorted`, `reversed`, `unique`, and `list * int`) an unresolved empty-list argument then resolved to `unresolved[list[T1]]`, a leaked type variable, where it had been `list[nulltype]`. Reproduced through `resolve_call` and through `sorted(E)` with `E` unresolved. The fast path is gone. Instead `match_call` and the recursive matcher treat such a match as a *weak* binding: any other parameter's binding wins outright (`1 in []` binds `T = int`), and a variable nothing else binds is `nulltype` as before. 3. `unify_inner` indexed `params[0]` on `List`-coded types without checking the list has an element type. `ExprType::new(TypeCode::List, vec![])` is constructible and the repo treats it as a real input elsewhere; two arms could panic on it. They now use `params.first()` / slice patterns and unify to `None`. Tests: unit tests for each rule in types.rs, an integration test for the `any` item and for the empty-list return types, and an openjd-model test for the exact `let` cascade. Four mutants (Any arms removed; weak bindings never applied; `[]` binds strongly; unguarded list index) each fail a named test. Spec docs updated to say the weak-binding rule applies to every signature. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>

The section was inserted between the is_concrete() and satisfies() rows, so the last three rows rendered as literal pipe text. It now follows the sig_return() row, ahead of the crate-internal helpers. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>

Review finding on OpenJobDescription#396, reproduced: membership dispatches container-first, the reverse of the source order, so 'a' in [1, 2] reported "with list[int] and string", and [1, 2] in [1, 2] reported "with list[int] and list[int]", two identical types that read as a bug in the type checker. That expression evaluated to false before this PR, so the message is the primary user-visible artifact of the change. When no __contains__ signature matches and the container is a list, the error now reads: Cannot use 'in' operator: item of type list[int] is not compatible with the element type int of list[int]. A range_expr container gets the same shape. A string haystack keeps the generic message, which this PR did not change. The seven integration and two model tests asserting the old wording are updated, and [1, 2] in [1, 2] is added as the case that motivated it. Restoring the generic path fails both caret tests and the mismatch table. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>

leongdl requested a review from a team as a code owner September 15, 2026 04:20

github-actions Bot added the waiting-on-maintainers label Sep 15, 2026