fix(expr): preserve compatible unresolved targets - #300
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
0c287d1 to
8643642
Compare
Review summaryWhat was reviewed: Both commits at tip 8643642 — the type-level coercion table ( Angles worked:
Finding (blocker): The union-source branch requires every union member to coerce to the target ( Non-blocking, real:
Verdict: do not ship as-is — one branch rewrite away from ship (see the inline comment on the union-source branch). Corrections to my own prior review: my #297 prescription (a |
bd6c3ec to
1357dd1
Compare
Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
Two divergences between the concrete coercion table and the type-level table used for unresolved placeholders: The `range_expr` arm matched on a bare `list` target while ignoring the element type, so it returned a `list[int]` for any `list[T]` target, breaking the postcondition that a coerced value satisfies the target it was coerced to. Materialize, then widen element-wise, and reject targets whose element type `int` cannot reach. The unresolved table now accepts `range_expr -> list[T]` on the same condition instead of only `list[int]`. A type-variable target was accepted for unresolved values but has no arm in the concrete table, so a concrete value always fails against one. Rejecting both keeps validation from passing an expression that can only fail once the value is known. Unresolved coercion may accept a pair the concrete value later rejects, since the deciding information is a payload the placeholder does not carry. It must never reject a pair the concrete value would accept: that fails a template at validation time that would have run correctly. Record that asymmetry in the docstring and specs/expr/values.md. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
1357dd1 to
a421788
Compare
The materialize-then-widen behavior introduced in the previous commit overshot: RFC 0005's implicit coercion table lists list[int] as the only list type a range_expr coerces to, and implicit rules do not chain. The widening belongs to the explicit list() conversion (RFC 0006), whose list[int] result the list[T] -> list[U] rule can then apply to. Restore the postcondition the right way: reject a list[T] target with any element type other than int, on both the concrete path and the unresolved type-level table, instead of widening element-wise toward the target. The target check now runs before the materialization size cap so an invalid target reports the type error rather than a size error. Update specs/expr/values.md and the affected tests to match. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
Two review findings on the coercion tables, both rejections in the direction the asymmetry rule forbids (failing validation for a value that would have run correctly): The range_expr element-type guard compared against int by strict equality, rejecting a list[any] target even though a list[int] value already satisfies list[any] — no widening involved. Accept an int or any element target, on both the concrete path and the type-level table (which was also internally inconsistent: unresolved[list[int]] coerced to list[any] but unresolved[range_expr] did not). The type-level list/list rule required the source element type to reach the target's, but the concrete path accepts an empty list of any element type against any list[U] (element-wise over zero elements). Since an unresolved[list[S]] could resolve to the empty list, accept every list/list pair and defer the element compatibility check until the payload is known — consistent with how unresolved[string] -> int defers the parse check. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
| // element target is accepted because a `list[int]` value | ||
| // already satisfies `list[any]` — no widening involved. | ||
| if let Some(elem) = target.params().first() { | ||
| if elem != &ExprType::INT && elem.code() != TypeCode::Any { |
There was a problem hiding this comment.
The new element guard rejects element types that a list[int] value already satisfies, which contradicts the rationale given two lines above ("An any element target is accepted because a list[int] value already satisfies list[any] — no widening involved").
elem != &ExprType::INT && elem.code() != TypeCode::Any also rejects:
list[int | string]— alist[int]satisfies this by union membershiplist[int?](i.e.list[int | nulltype]) — samelist[T]/list[T1]… — a type-variable element binds toint
For all of these the materialized list[int] would satisfy the target with no widening, so by the stated reasoning they should be accepted. Only genuinely-widening element targets (list[float], list[string], list[bool], list[path]) should be rejected.
The satisfaction test the codebase already has expresses this directly:
if let Some(elem) = target.params().first() {
if elem.match_type(&ExprType::INT).is_none() {
return Err(...);
}
}match_type covers int, any, int | string, int?, and type variables in one check, and drops the special-case code() == TypeCode::Any branch.
The same condition is duplicated in has_range_list_rule (unresolved_coercion_result, ~line 985), so the type-level table under-accepts identically. That one is the more consequential half: by the invariant this PR documents ("it must never reject one the concrete value would accept"), an unresolved[range_expr] against a list[int | string] target fails validation for a template that would have run correctly once resolved.
|
Non-blocking nit (single-step accept/reject behavior is correct, so not gating): In for member in target.params() {
...
if let Some(result_type) = Self::unresolved_coercion_result(source, member) {
return Some(result_type);
}
}But the semantics this PR adds to Example (union params sort alphabetically, so member order is deterministic):
So Suggested fix: collect results from all succeeding members and return (Cross-checked against RFC 0005 §"Implicit Type Coercion" while reviewing — separately, the |
|
For other reviewers to understand — a plain-language walkthrough of what this PR does and why: The setting: templates are validated before all values existTemplates contain expressions like What coercion isEvery field has an expected target type. If an expression produces an The bug this PR fixesBefore this PR, Coercion said "I have no actual number to convert" and rejected it — a valid template failed validation just because the value hadn't arrived yet. The fixApply the same coercion table at the type level:
One asymmetry is deliberate: some conversions depend on the payload ( (For those who've seen abstract interpretation: |
Complexity baseline for the modified call chainMeasured with Call stack reaching the modified codeSecondary call site into the same modified function: Measurements
AssessmentThe complexity here is of the dispatch-table kind — long flat sequences of early-return type rules mirroring the RFC coercion table, not nested state machines — so each rule is independently readable and the raw CCN overstates the reading difficulty. Caveat: lizard counts match arms, which inflates CCN for large Rust The structural observation worth tracking: |
| if target.code() == TypeCode::Union { | ||
| if target.match_type(source).is_some() { | ||
| return Some(source.clone()); | ||
| } | ||
| for member in target.params() { | ||
| if matches!( | ||
| member.code(), | ||
| TypeCode::NullType | TypeCode::List | TypeCode::Union | ||
| ) { | ||
| continue; | ||
| } | ||
| if let Some(result_type) = Self::unresolved_coercion_result(source, member) { | ||
| return Some(result_type); | ||
| } | ||
| } | ||
| return None; | ||
| } |
There was a problem hiding this comment.
Ran into this same thing independently — maybe worth a follow-up?
The concrete path's member choice is payload-dependent: from_str_coerce("abc", Bool) fails and it falls through, where the type-level path succeeds on that member. Collecting all viable members instead matches the spec text this PR adds ("the union of the result types from all successful possibilities") and makes this branch symmetric with the source-union branch after the follow-up commits.
if target.code() == TypeCode::Union {
if target.match_type(source).is_some() {
return Some(source.clone());
}
let result_types: Vec<_> = target
.params()
.iter()
.filter(|m| !matches!(m.code(), TypeCode::NullType | TypeCode::List | TypeCode::Union))
.filter_map(|member| Self::unresolved_coercion_result(source, member))
.collect();
return if result_types.is_empty() {
None
} else {
Some(ExprType::union(result_types))
};
}
|
Ok, went over @leongdl's comments as well. I agree with most things, seems like that one followup is valuable. The rest seems good, I think makes sense. Good catch! |
3 participants
Follow-up to review comments on #297
What was the problem/requirement? (What/Why)
OpenJD validates expressions before every runtime value is necessarily
available. For example, validation may know that
Param.Framewill be aninteger even though the actual frame number is supplied later. The expression
evaluator represents this as an unresolved value: a typed placeholder that
means "the value is not known yet, but its type is."
Expressions can also be evaluated with a target type, which describes the type
the surrounding template field expects. If an unresolved integer reached an
integer target, coercion incorrectly rejected it because there was no concrete
number to convert. This caused valid templates to fail validation even though
the known type already met the requirement.
What was the solution? (How)
Treat coercion as already satisfied when the placeholder's known type matches
the target, and return the unresolved value unchanged. There is no concrete
payload to convert yet, and none is needed to prove that the types are
compatible.
If the known type does not match, coercion still fails. For example, an
unresolved
list[int]cannot satisfy aninttarget, so genuine type errorsremain visible during validation.
What is the impact of this change?
Valid templates no longer fail merely because a runtime value has not been
supplied yet. Validation still catches incompatible types as early as before.
How was this change tested?
cargo test -p openjd-expr --test integration unresolved.Was this change documented?
specs/expr/values.mdand source documentation.Is this a breaking change?
No.
Does this change impact security?
No.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.