Skip to content

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

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
seant-aws merged 1 commit into from
Aug 13, 2026
+42 −1
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 4 additions & 1 deletion crates/openjd-expr/src/functions/list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,10 @@ pub fn flatten_fn(ctx: Ctx, a: &[ExprValue]) -> R {
}
}
let et = if result.is_empty() {
ExprType::INT
// An empty flatten result has no elements to infer a type from.

@github-actions github-actions Bot Aug 12, 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.

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

// 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

@github-actions github-actions Bot Aug 12, 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 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.

@github-actions github-actions Bot Aug 13, 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 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).

} else {
result[0].expr_type()
};
Expand Down
38 changes: 38 additions & 0 deletions crates/openjd-expr/tests/integration/test_lists.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1714,3 +1714,41 @@ fn empty_listcomp_type_is_nulltype() {
"list[nulltype]"
);
}

#[test]
fn flatten_empty_comprehension_has_nulltype() {
// Regression: flatten over an empty comprehension result must yield list[nulltype],
// not list[int], so that functions like repr_sh (which has a list[nulltype] overload)
// accept the result.
let mut st = SymbolTable::new();
st.set(
"Env",
ExprValue::make_list(vec![], ExprType::STRING).unwrap(),
)
.unwrap();
let parsed =
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(), "");

@github-actions github-actions Bot Aug 12, 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.

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.

}

#[test]
fn flatten_nonempty_comprehension_has_string_type() {
// Non-empty case: flatten over a string comprehension must still yield list[string].
let mut st = SymbolTable::new();
st.set(
"Env",
ExprValue::make_list(vec![ExprValue::String("A=1".to_string())], ExprType::STRING).unwrap(),
)
.unwrap();
let parsed =
openjd_expr::ParsedExpression::new("repr_sh(flatten([[\"-e\", e] for e in Env]))").unwrap();
let result = parsed.evaluate(&st).unwrap();
// repr_sh on a list of strings should produce shell-escaped space-separated values
assert!(
result.to_display_string().contains("-e"),
"expected repr_sh output to contain -e, got: {}",
result.to_display_string()
);
}
Loading