Skip to content

fix(expr): use NULLTYPE for empty flatten result instead of INT - #316

Draft
seant-aws wants to merge 1 commit into
OpenJobDescription:mainfrom
seant-aws:expr-empty-flatten-nulltype
Draft

fix(expr): use NULLTYPE for empty flatten result instead of INT#316
seant-aws wants to merge 1 commit into
OpenJobDescription:mainfrom
seant-aws:expr-empty-flatten-nulltype

Conversation

@seant-aws

Copy link
Copy Markdown
Contributor

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

flatten_fn defaulted to ExprType::INT when the flattened result was empty. Downstream functions without a list[int] overload (e.g. repr_sh) rejected the result with No matching signature for repr_sh(list[int]), even though an empty comprehension is a valid, normal case.

What was the solution? (How)

Changed the empty-result element type from ExprType::INT to ExprType::NULLTYPE. NULLTYPE is the bottom type for empty lists — all repr_* functions already declare a (list[nulltype]) overload, and overload resolution matches it exactly.

What is the impact of this change?

Expressions like repr_sh(flatten(list.env)) no longer fail when the input list is empty. No change to non-empty paths.

How was this change tested?

  • cargo test -p openjd-expr — 3456 passed, 0 failed (unit 308, integration 3140, doc 8)
  • cargo clippy -p openjd-expr — clean
  • New integration test: test_repr_sh_of_empty_flatten in test_lists.rs

Was this change documented?

Code-level only; no spec change needed.

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.

openjd_expr::ParsedExpression::new("repr_sh(flatten([[\"-e\", e] for e in Env]))").unwrap();
let result = parsed.evaluate(&st).unwrap();
// repr_sh of an empty list produces an empty string
assert_eq!(result.to_display_string(), "");

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.

Neither new test actually asserts the type, despite both test names claiming to (..._has_nulltype, ..._has_string_type). They only assert on repr_sh's rendered output, which is an indirect proxy — the tests would still pass if flatten returned list[any], list[bool], or any other element type that happens to have a repr_sh overload. That makes them weak guards for the exact invariant this commit establishes.

The sibling test just above (empty_listcomp_type_is_nulltype, line 1709) already shows the direct form. Suggest asserting the flatten result type itself in addition to the end-to-end behavior:

assert_eq!(
    eval("flatten([[\"-e\", e] for e in []])").expr_type().to_string(),
    "list[nulltype]"
);

Separately, flatten_nonempty_comprehension_has_string_type uses assert!(result.to_display_string().contains("-e")). The expected output here is fully determined (-e A=1), so an exact assert_eq! would catch escaping/separator regressions in repr_sh that a contains check silently passes.

// An empty flatten result has no elements to infer a type from.
// Use NULLTYPE (the bottom type for lists) so the result is accepted by
// any function that declares a list[nulltype] overload (e.g. repr_sh).
ExprType::NULLTYPE

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 fix is correct and strictly widens what downstream dispatch accepts (every concrete list overload that could previously match list[int]min/max/sum/join/range_expr/flatten — also has a list[nulltype] variant, so nothing that used to resolve now fails). Two follow-ups worth considering:

1. The element type is still discarded when it is recoverable. flatten is declared as (list[list[T1]]) -> list[T1] and (list[T1]) -> list[T1], but the runtime result type is computed only from result[0], ignoring the input's own element type. That leaves cases where the declared T1 is known but the runtime type falls back to nulltype:

  • flatten(x) where x is an empty list[int] — statically list[int], now list[nulltype] (this one regresses relative to the old hardcoded INT, though harmlessly, since the nulltype overloads cover it).
  • flatten(x) where x is an empty list[list[string]]a[0] is ListList([], list[string], _), so list_elem_type() gives list[string] and the correct answer list[string] is available.

Deriving from the input keeps runtime types consistent with the signature and makes NULLTYPE the fallback only when there is genuinely no information:

let et = if let Some(first) = result.first() {
    first.expr_type()
} else {
    // Recover T1 from the input: list[list[T1]] -> T1, list[T1] -> T1.
    let input_elem = a[0].list_elem_type();
    match &input_elem {
        Some(t) => t.list_element_type().cloned().unwrap_or(ExprType::NULLTYPE),
        None => ExprType::NULLTYPE,
    }
};

2. result[0].expr_type() is a latent inconsistency in the non-empty path too. For a heterogeneous flatten like flatten([[1], [2.0]]) the hint is taken from the first element (int), and it is only make_list's int→float promotion that saves the result. That is pre-existing, not introduced here, but if you touch this line it is worth a note that the hint is advisory and promotion is what actually decides the variant.

}
let et = if result.is_empty() {
ExprType::INT
// An empty flatten result has no elements to infer a type from.

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.

This change makes ExprType::NULLTYPE a live hint_type into make_list from a production code path, and the spec documenting that hint is currently wrong about what it produces.

specs/expr/values.md "Empty list variant selection by hint_type" (around line 186) lists only BOOL / INT / FLOAT / PATH / LIST[T] and then anything else → ListInt([]) (canonical empty list). The actual code in value.rs:499-517 maps NullType → ListList([], NULLTYPE, 0) and the _ fallback likewise to ListList([], NULLTYPE, 0) — not ListInt([]). The table is also missing the STRING → ListString([], 0) row.

The drift is pre-existing, but this PR is what makes the NULLTYPE row load-bearing: a reader following the spec would conclude flatten of an empty list yields ListInt([]) / list[int], i.e. exactly the bug being fixed. Per AGENTS.md ("before committing, always confirm the spec and code line up"), worth correcting the table in this commit:

| STRING | `ListString([], 0)` |
| NULLTYPE | `ListList([], NULLTYPE)` (canonical empty list) |
| anything else | `ListList([], NULLTYPE)` (canonical empty list) |

@seant-aws
seant-aws force-pushed the expr-empty-flatten-nulltype branch from 865261d to d7f1e1e Compare August 13, 2026 00:15
flatten_fn defaulted to ExprType::INT when the flattened result was
empty. This caused downstream functions without a list[int] overload
(e.g. repr_sh) to reject the result, even though the expression is
valid — an empty comprehension is a normal case.

The correct default is ExprType::NULLTYPE, which is the bottom type for
empty lists. All repr_* functions already declare a (list[nulltype])
overload, and overload resolution matches it exactly.

Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>
@seant-aws
seant-aws force-pushed the expr-empty-flatten-nulltype branch from d7f1e1e to 85bc33c Compare August 13, 2026 00:36
// An empty flatten result has no elements to infer a type from.
// Use NULLTYPE (the bottom type for lists) so the result is accepted by
// any function that declares a list[nulltype] overload (e.g. repr_sh).
ExprType::NULLTYPE

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 comment cites repr_sh's list[nulltype] overload as the justification for this change, but specs/expr/function-library.md:132 currently documents that overload with a narrower purpose than this PR gives it:

repr_sh accepts only the canonical string, path, list[string], and list[path] inputs, plus an internal list[nulltype] overload for an empty list literal.

The same "empty list literal" framing is repeated for repr_cmd two lines down (function-library.md:133-136, "internal exact path, list[path], and list[nulltype] overloads implement standard path-to-string and empty-list behavior").

After this change that characterization is no longer accurate. list[nulltype] is now reachable from a computed value — flatten over any input that happens to produce zero elements — not just from an empty list literal in the source. The word "internal" is also now misleading: this is the overload that makes repr_sh(flatten(...)) work at all for the empty case, which is the user-visible behavior the PR exists to fix.

This is a separate drift from the make_list hint-type table in specs/expr/values.md already noted on this PR — different file, different claim. Both are load-bearing for a reader trying to understand why NULLTYPE is the right element type here.

Per AGENTS.md ("before committing, always confirm the spec and code line up"), suggest widening the wording in the same commit, e.g.:

... plus a list[nulltype] overload covering the canonical empty list, which arises both from an empty list literal and from functions that return an empty result (e.g. flatten).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant