Skip to content

fix(expr): preserve compatible unresolved targets #300

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mwiebe merged 6 commits into from
Aug 12, 2026
Merged

fix(expr): preserve compatible unresolved targets #300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a6952f5
fix(expr): preserve compatible unresolved targets
mwiebe Aug 7, 2026
a6863eb
fix(expr): coerce unresolved values by type
mwiebe Aug 8, 2026
17fc0aa
fix(expr): accept partially coercible unresolved unions
mwiebe Aug 10, 2026
a421788
fix(expr): make coerce results satisfy their target type
mwiebe Aug 10, 2026
e1348cb
fix(expr): restrict implicit range_expr coercion to list[int]
mwiebe Aug 10, 2026
9af43ab
fix(expr): accept list[any] range targets and defer list element checks
mwiebe Aug 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
146 changes: 146 additions & 0 deletions crates/openjd-expr/src/value.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -727,6 +727,14 @@ impl ExprValue {
/// Coercion is non-destructive: only conversions that don't lose
/// information are attempted (`int → float`, `int → string`, etc).
///
/// Unresolved values apply the same table at the type level and return
/// an unresolved value constrained to the result type. Checks that need
/// a concrete payload, such as parsing a string as an integer, are
/// deferred until resolution. The type-level table may therefore accept
/// a pair that the concrete value later rejects, but it must never
/// reject one the concrete value would accept — that would fail a
/// template during validation that would have run correctly.
///
/// An `any` target is unconstrained: every value is returned
/// unchanged.
///
Expand All @@ -749,6 +757,17 @@ impl ExprValue {
if target.code() == TypeCode::Any {
return Ok(self);
}
// Unresolved values have no payload, so apply the concrete coercion
// table at the type level and defer payload-dependent checks until
// resolution. The result constraint must satisfy the target just as
// a concrete result's type would.
if let ExprValue::Unresolved(constraint) = &self {
Comment thread
mwiebe marked this conversation as resolved.
return if let Some(result_type) = Self::unresolved_coercion_result(constraint, target) {
Ok(ExprValue::unresolved(result_type))
} else {
Err(format!("Cannot coerce {constraint} to {target}"))
};
}
// Match-first: also accepts the case where the target is a union
// and the value's type is one of its members. Falls back to the
// existing strict-equality behavior for non-union targets.
Expand Down Expand Up @@ -813,6 +832,22 @@ impl ExprValue {
}
(ExprValue::RangeExpr(r), TypeCode::String) => Ok(ExprValue::String(r.to_string())),
(ExprValue::RangeExpr(r), TypeCode::List) => {
// RFC 0005: `list[int]` is the only list type a range_expr
// implicitly coerces to. Implicit rules do not chain, so the
// materialized `list[int]` is never widened element-wise
// toward another target; templates use the explicit `list()`
// conversion (RFC 0006) when they want that. An `any`
// 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 {

@github-actions github-actions Bot Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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] — a list[int] satisfies this by union membership
  • list[int?] (i.e. list[int | nulltype]) — same
  • list[T] / list[T1]… — a type-variable element binds to int

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.

return Err(format!(
"Cannot coerce range_expr to {target}: a range \
expression only implicitly coerces to list[int] \
(use list() for an explicit conversion)"
));
}
}
// coerce() runs outside any EvalContext (post-evaluation
// target-type hook, public API), so no operation or memory
// budget applies here. Cap the materialization at the
Expand Down Expand Up @@ -846,6 +881,117 @@ impl ExprValue {
}
}

fn unresolved_coercion_result(source: &ExprType, target: &ExprType) -> Option<ExprType> {
if target.code() == TypeCode::Any {
return Some(source.clone());
}
if source.code() == TypeCode::Unresolved {
return source
.params()
.first()
.and_then(|inner| Self::unresolved_coercion_result(inner, target));
}
if source.code() == TypeCode::Union {
let result_types: Vec<_> = source
.params()
.iter()
.filter_map(|member| Self::unresolved_coercion_result(member, target))
.collect();
return if result_types.is_empty() {
None
} else {
Some(ExprType::union(result_types))
};
}
if source.code() == TypeCode::Any
|| matches!(
source.code(),
TypeCode::TypeVarT
| TypeCode::TypeVarT1
| TypeCode::TypeVarT2
| TypeCode::TypeVarT3
)
{
return Some(target.clone());
}
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;
}
Comment on lines +917 to +933

@seant-aws seant-aws Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))
    };
}

// Note there is deliberately no rule for a type-variable *target*:
// the concrete table has no arm for one either, so a concrete value
// always fails against it. Accepting the unresolved case would let
// validation pass an expression that can only fail at runtime.
if source == target {
return Some(source.clone());
}
if source.code() == TypeCode::List
&& source.params().len() == 1
&& target.code() == TypeCode::List
&& target.params().len() == 1
{
// Any list/list pair is accepted: an `unresolved[list[S]]`
// could resolve to the empty list, which the concrete path
// coerces to any `list[U]` (element-wise over zero elements).
// Rejecting here would fail a template during validation that
// could have run correctly. The element compatibility check is
// deferred until the value resolves with a non-empty payload.
return Some(target.clone());
}

let has_scalar_rule = matches!(
(source.code(), target.code()),
(TypeCode::Int, TypeCode::Float)
| (TypeCode::Float, TypeCode::Int)
| (
TypeCode::Bool
| TypeCode::Int
| TypeCode::Float
| TypeCode::Path
| TypeCode::RangeExpr,
TypeCode::String
)
| (
TypeCode::String,
TypeCode::NullType
| TypeCode::Bool
| TypeCode::Int
| TypeCode::Float
| TypeCode::Path
| TypeCode::RangeExpr
)
);
// `list[int]` is the only list type a `range_expr` implicitly
// coerces to (RFC 0005); implicit rules do not chain, so no
// element-wise widening applies — mirroring the concrete path.
// An `any` element target is accepted because a `list[int]` value
// already satisfies `list[any]`.
let has_range_list_rule = source.code() == TypeCode::RangeExpr
&& target.code() == TypeCode::List
&& match target.params().first() {
Some(elem) => elem == &ExprType::INT || elem.code() == TypeCode::Any,
None => true,
};
if has_scalar_rule || has_range_list_rule {
Some(target.clone())
} else {
None
}
}

/// Python-style repr: `ExprValue(42)`, `ExprValue('hello')`, `ExprValue([1, 2], type='list[int]')`.
pub fn repr_python(&self) -> String {
match self {
Expand Down
Loading
Loading