Support multiple space-separated values in display statements - #636
Conversation
`display` parsed only the first value and silently dropped any tokens that followed on the same line, so `display "user age is " user age` printed just "user age is " with the variable thrown away — silent, partial output with no error. `display` now folds additional space-separated values into a single left-associative concatenation, matching `display a with b with c`. Quoted text is a string literal; anything else is a variable/expression. Values are joined directly (no separator), so spaces come from the quotes. Backward compatible: direct index access (`display numbers 0`) is absorbed by the first expression before the fold, a line break ends the statement because `Eol` is not a value start, and single values are unchanged. Only programs that previously dropped trailing values change behavior. - Add Parser::is_value_start and rewrite parse_display_statement to fold trailing value-start tokens into a Concatenation. - Tests: parser unit tests (string/var interleavings, three-value fold, index-access and single-value regressions, no-leak-into-next-statement) and TestPrograms/display_multiple_values.wfl. - Docs: "Display Several Values at Once" in hello-world.md plus a manifest-tracked, 5-layer-validated example; Dev Diary entry. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KUmoGX1Eyt6H1BcXKg34Vu
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe ChangesMulti-value display
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa599932f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let (cat_line, cat_column) = match self.cursor.peek() { | ||
| Some(token) if Self::is_value_start(&token.token) => (token.line, token.column), |
There was a problem hiding this comment.
When the next display item starts with a valid expression keyword rather than an identifier/literal, this guard stops folding and leaves the tokens as a separate statement. For example, display "number: " call get number prints only number: and then evaluates the action result separately, and display "count is " count inside a count loop fails because count is parsed as a new count statement. Since the new docs describe subsequent items as variables or expressions, include the keyword expression starters that parse_primary_expression accepts (e.g. call, current, contextual count, etc.) here instead of only is_value_start's current literals/identifiers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds parser support for display statements to accept multiple space-separated values on the same line by folding them into a left-associative Concatenation, fixing prior behavior where trailing values could be silently ignored. This change sits in the WFL parser and is backed by new parser tests, docs, and example programs.
Changes:
- Refactors
parse_display_statementto parse the first expression and then fold subsequent value-start expressions intoExpression::Concatenation. - Introduces
Parser::is_value_startto conservatively detect when another space-separated value begins. - Adds parser unit tests plus docs + manifest-tracked examples and an end-to-end TestPrograms example.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/parser/stmt/io.rs |
Implements the multi-value display fold and anchors statement position at display. |
src/parser/helpers.rs |
Adds Parser::is_value_start token predicate used by display folding. |
src/parser/tests.rs |
Adds unit tests for concatenation folding and regressions (index access, single value, statement boundary). |
Docs/02-getting-started/hello-world.md |
Documents the new space-separated display form with examples and guidance. |
TestPrograms/display_multiple_values.wfl |
Adds an end-to-end example program covering common and regression scenarios. |
TestPrograms/docs_examples/basic_syntax/display_multiple_01.wfl |
Adds a manifest-tracked docs example for the new docs section. |
TestPrograms/docs_examples/_meta/manifest.json |
Registers the new docs example for validation. |
Dev diary/2026-07-18-display-multiple-values.md |
Records the motivation, root cause, design constraints, and tests/docs added. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| You don't have to write `with` between every piece. A `display` can list | ||
| several values separated by spaces — quoted text is shown as-is, and anything | ||
| else (a variable or an expression) is evaluated first: |
| // Anchor the statement at the `display` keyword itself. | ||
| let (line, column) = self | ||
| .bump_sync() // Consume "display" | ||
| .map_or((0, 0), |token| (token.line, token.column)); |
Addresses automated review on #636: the first cut of multi-value `display` only folded literals, identifiers and `(`, so a value that *starts* with a keyword was still dropped — or worse. `display "count is " count` in a count loop failed to parse (`count` reached the statement parser and was treated as a new count loop), and `display "number: " call get_number` dropped the call. - Extend `Parser::is_value_start` to also accept `call`, `count`, and `current` — the keyword-led primary expressions that are not also binary operators (so they reach the fold instead of dangling). Keywords the binary parser already consumes after a value (`with`, `find`, `replace`, `split`, `matches`, arithmetic) never reach the check; a leading `-` parses as subtraction. - `parse_display_statement` now `expect`s the `display` token instead of defaulting the statement position to a misleading (0, 0); it is only dispatched on `display`. - Docs: describe display values honestly (variable, number, action call, or arithmetic expression) instead of promising arbitrary expression forms. - Tests: parser folds for the `count` variable and a `call` action; E2E example gains a folded `call` and a `count`-loop display. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KUmoGX1Eyt6H1BcXKg34Vu
| pub(crate) fn is_value_start(token: &Token) -> bool { | ||
| matches!( | ||
| token, | ||
| Token::StringLiteral(_) | ||
| | Token::IntLiteral(_) | ||
| | Token::FloatLiteral(_) | ||
| | Token::BooleanLiteral(_) | ||
| | Token::NothingLiteral | ||
| | Token::Identifier(_) | ||
| | Token::LeftParen | ||
| | Token::KeywordCall | ||
| | Token::KeywordCount | ||
| | Token::KeywordCurrent | ||
| ) | ||
| } |
logbie
left a comment
logbie
left a comment
There was a problem hiding this comment.
Deep-review verdict: do not merge yet. Reviewed at 498a2c3e. The focused parser tests and full CI are green, but the feature still has observable correctness and grammar problems that the current tests do not exercise.
Merge-blocking findings
-
The space-separated form is not semantically equivalent to
with.parse_display_statementbuilds a left-deep concatenation atsrc/parser/stmt/io.rs:423-429, while explicitwithparses its right-hand side recursively atsrc/parser/expr/binary.rs:346-355. Because concatenation evaluates the left value, then the right value, and only then stringifies them, mutation makes this observable.With two identical lists initially containing
beforeandafter:display left_items "" pop of left_items display right_items with "" with pop of right_itemsproduces:
[before, after]after [before]afterThis contradicts the docs' “exactly the same” / No-Unlearning claim. The left fold also repeatedly copies a large leading text value, adding roughly
k * Lcopying forksuffixes and anL-byte prefix. Please either build the same association aswithor introduce a flat concatenation representation with deliberately specified evaluation/stringification semantics. -
is_value_startis still incomplete.The second commit correctly added
call,count, andcurrent, butsrc/parser/helpers.rs:259-272still omits valid starters accepted byparse_primary_expression, includingnot,pattern,output,file,directory,process,header,list,read, and contextual-keyword variables.Confirmed current-head behavior:
display "not: " not yes // prints only "not: " display "files: " list files in "." // prints only the label display "content: " read content from source // parse failurePlease centralize the “can start a primary expression” decision instead of maintaining a display-specific subset.
-
find/replace/splitbeing consumed by the binary parser does not make them valid subsequent display values.display "parts: " split "a,b" by ","prints
[a, b], notparts: [a, b]. Thesplitbranch atsrc/parser/expr/binary.rs:524-549replaces/discards the preceding left expression.findsimilarly loses the label;replacecan hard-fail. This must be resolved at the operand-boundary/grammar level; adding tokens tois_value_startafter the firstparse_expressionhas already consumed them is insufficient. -
The advertised whitespace-separator contract conflicts with existing lexical grammar.
display a b clexes as oneIdentifier("a b c")atsrc/lexer/mod.rs:259-273, not three values, despite the implementation comments, test program, and diary claiming equivalence withdisplay a with b with c. Other ambiguous cases includeage 0(direct index),"list: " [1,2](postfix index), and"n: " -5(subtraction).Please either choose an unambiguous separator, or narrow the public contract and explicitly require
withfor ambiguous pairs. -
The tests do not verify runtime output.
The new unit tests inspect AST shape, and the executable examples place expected output only in comments. The CI program runner redirects stdout/stderr to
/dev/nulland manifest validation checks only exit status. Add interpreter/CLI tests asserting exact stdout, including the mutation regression and prefix-expression cases above.
The second commit did fix the named call/count/current cases and replaced the misleading (0, 0) fallback with an invariant expect; those changes look good.
@claude Please independently validate the current head and report back on all five findings. In particular, exercise multi-value display with WFL user-defined action calls and container values/properties/methods, including mutable lists/containers and side effects. Compare exact stdout between the space-separated and with forms; an exit code of zero is not sufficient. Please run targeted parser plus interpreter/CLI tests, inspect the completed GitHub Actions results, and identify the minimal parser/AST approach that preserves existing action/container behavior and with semantics.
|
Claude finished @logbie's task in 4m 54s —— View job Independent validation of @logbie's deep-review findings (head
|
|
@claude Please proceed with the implementation pass on the current PR branch now. The goal is a merge-ready version of this feature, not another review-only report:
You are explicitly authorized for this task to run Standing communication rule: post every material uncertainty, grammar/design choice, failed test, permission problem, or question to this PR. If a choice could change language semantics, ask here before committing to it. |
|
Claude finished @logbie's task in 26m 21s —— View job Implementation pass — pushed; most CI green,
|
Addresses the maintainer deep-review of the multi-value `display` feature: - Fold space-separated display values right-associatively (a with (b with c)), matching explicit `with`'s AST shape, evaluation order, and stringification order exactly. The previous left-associative fold stringified an earlier value before a later value's side effects (e.g. a list `pop`) ran, so `display a b c` could observably diverge from `display a with b with c` whenever a value was a shared mutable reference (list/container). - Expand `is_value_start` with the keyword-led value starters flagged by review as safe and unambiguous: `not`, `pattern`, `output`, `file`, `directory`, `process`, `header`, `list`, `read`. Left out `loop`/`exit`/ `repeat`/`try`/`when` (statement/block openers) and `back`/`error` (unflagged, left for a follow-up) — see the doc comment for the reasoning. - `find`/`replace`/`split` remain excluded from folding: the bug where they discard the preceding value is in the general binary-expression grammar (expr/binary.rs), not `display`-specific, and out of scope for this pass. - Corrected docs/diary/example comments that overclaimed space-separated values are unambiguous — multi-word identifiers, direct indexing, and unary/binary operators all claim the space before folding ever runs. - Added exact-stdout regression coverage (tests/display_multiple_values_stdout_test.rs) covering documented happy paths, action return values/arguments/side effects, container instance/property/method access, and — the key regression — byte-for-byte equivalence between space-separated `display` and `with` for a mutating list, both as a plain variable and as a container property. Co-authored-by: logbie <1138960+logbie@users.noreply.github.com>
CI's Check formatting job flagged 3 test call sites exceeding the line-width limit; reformatted to match rustfmt's expected multi-line call style (same form already used by the adjacent directory_exists/header/read_content tests).
| /// - `not` and unary `-` (`Minus`) as *binary*-operator continuations don't | ||
| /// apply here, but `Minus` specifically can never be the head of a fresh | ||
| /// display value either way: `parse_binary_expression` has no precedence | ||
| /// guard on subtraction, so a leading `-` after the first value is always | ||
| /// consumed as arithmetic *inside* that first `parse_expression()` call |
| // Fold right-associatively — the same tree shape, evaluation order, and | ||
| // stringification order as explicit `with` (`a with b with c` parses as | ||
| // `a with (b with c)`, see the `with` handling in expr/binary.rs). This |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Dev` diary/2026-07-18-display-multiple-values.md:
- Around line 119-123: Update the parser unit-test summary in the “three-value”
description to say it verifies a right-associative fold instead of a
left-associative fold; leave the other listed test coverage unchanged.
In `@Docs/02-getting-started/hello-world.md`:
- Around line 172-181: Update the explanation around the “space-separated
values” rule to state that only keyword-led primary expressions recognized by
is_value_start create a boundary; do not claim that every keyword starts another
display value, and explicitly exclude binary operators and statement/block
starters.
In `@src/parser/tests.rs`:
- Around line 2594-2603: Update the parse_display helper to call parser.parse()
instead of parse_statement(), then match the resulting single-statement program
and extract the DisplayStatement value. Require exactly one statement,
preserving the existing panic behavior for parse errors or unexpected statement
kinds so trailing tokens are rejected.
In `@TestPrograms/display_multiple_values.wfl`:
- Line 34: Update the action declaration using the canonical parameter syntax:
in the doubled action definition, replace the parameter name n with the required
parameters x form while preserving the action name and declaration structure.
In `@tests/display_multiple_values_stdout_test.rs`:
- Around line 16-42: Update run_wfl to reuse test_helpers::run_wfl_program
instead of constructing a temporary script and invoking Command::output
directly. Preserve the helper’s stdout-returning behavior by converting its
returned output to the String expected by callers, and remove the now-unneeded
local process and filesystem handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f33de007-c313-46bf-a077-6662fcf8a28e
📒 Files selected for processing (7)
Dev diary/2026-07-18-display-multiple-values.mdDocs/02-getting-started/hello-world.mdTestPrograms/display_multiple_values.wflsrc/parser/helpers.rssrc/parser/stmt/io.rssrc/parser/tests.rstests/display_multiple_values_stdout_test.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/parser/helpers.rs
- src/parser/stmt/io.rs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/parser/stmt/io.rs:432
- PR description says the space-separated fold is "left-associative", but the implementation here (and the tests/docs) intentionally fold right-associatively to match
with's AST shape and evaluation order.
Please update the PR description to avoid misleading reviewers/readers about the actual shipped semantics.
// Fold right-associatively — the same tree shape, evaluation order, and
// stringification order as explicit `with` (`a with b with c` parses as
// `a with (b with c)`, see the `with` handling in expr/binary.rs). This
// matters beyond cosmetics: `Concatenation` evaluates left, then right,
// then stringifies both, so a left-associative fold would stringify an
// earlier value *before* a later value's side effects (e.g. a list
// mutation) run, while this matches `with` exactly, value for value.
let mut value = values.pop().expect("at least one value was parsed");
| let binary = test_helpers::get_wfl_binary_path(); | ||
| let dir = tempfile::tempdir().expect("create temp dir"); | ||
| let script: PathBuf = dir.path().join("program.wfl"); | ||
| fs::write(&script, program).expect("write program"); | ||
|
|
||
| let output = Command::new(binary) | ||
| .arg(&script) | ||
| .output() | ||
| .expect("run wfl binary"); |
The prior implementation pass was pushed without a local toolchain, so three issues only surfaced when the tests actually ran here: - The container stdout test used `describe` — a reserved test-framework keyword — as a container method name, failing to parse and breaking Integration Tests and Build/Test/Clippy. Renamed the method to `summarize`. - The `pattern` and `header` keyword-fold parser tests used inputs that never lex/parse: `pattern "\d+"` (`\d` is not a valid WFL string escape) and `header "..." of response` (`response` is a reserved keyword). Switched to `pattern "abc"` and `... of req`; both fold correctly (verified via --parse). Review follow-ups addressed in the same pass: - parser/tests.rs `parse_display` helper now parses the whole program and requires exactly one statement, so a fold that leaves dangling tokens fails loudly (CodeRabbit). - stdout test `run_wfl` reuses `test_helpers::run_wfl_program`, which enforces a 30s timeout instead of a bare `Command::output()` that could hang CI (CodeRabbit). - Docs/Dev Diary: corrected the stale "left-associative" wording to right-associative, and narrowed the keyword-boundary claim in hello-world.md to the value-starting keywords (not every keyword; operators and statement/block starters excluded) (CodeRabbit/Copilot). - TestPrograms example uses the canonical `with parameters n` action syntax. - Reworded the `is_value_start` doc comment: `-` after a value is consumed as the binary subtraction operator (not "no precedence guard") (Copilot). Verified locally: cargo fmt --check, clippy -D warnings, full cargo test (display stdout 16/16, parser display 18/18; only the pre-existing flaky subprocess-timeout test hiccups, passes 3/3 in isolation), and TestPrograms/display_multiple_values.wfl runs clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KUmoGX1Eyt6H1BcXKg34Vu
|
Pushed
Both folds are actually correct with valid inputs (verified via Also folded in the review nits in the same commit: Verified locally on Two things still need a maintainer call:
Generated by Claude Code |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/parser/stmt/io.rs:429
- PR description says the fold is left-associative, but the implementation (and tests/docs) intentionally fold right-associatively to match
withevaluation/stringification. To avoid confusion for reviewers/users, the PR description should be updated to say right-associative (or “same aswith”).
// Fold right-associatively — the same tree shape, evaluation order, and
// stringification order as explicit `with` (`a with b with c` parses as
// `a with (b with c)`, see the `with` handling in expr/binary.rs). This
// matters beyond cosmetics: `Concatenation` evaluates left, then right,
// then stringifies both, so a left-associative fold would stringify an
| loop { | ||
| let (cat_line, cat_column) = match self.cursor.peek() { | ||
| Some(token) if Self::is_value_start(&token.token) => (token.line, token.column), | ||
| _ => break, | ||
| }; |
|
@claude New-head follow-up for
Finding 3 ( |
| "display_multiple_values_stdout_does_not_exist", | ||
| ) | ||
| .with_extension("missing"); | ||
| let missing_path = missing_path.to_str().expect("path should be valid UTF-8"); |
|
@claude New-head review for
The Separate status: the Claude review workflow failure is repository policy ( |
|
Claude finished @logbie's task in 17m 41s —— View job New-head correction pass for
|
…coupling at runtime - Reformat two call sites (src/parser/tests.rs, tests/display_multiple_values_stdout_test.rs) to match rustfmt's actual output, per CI run 29660025888. - Escape backslashes before embedding a temp-dir path in a WFL string literal, since WFL only recognizes a fixed escape set and a bare Windows path would fail to lex. - Replace the sample-based can_start_primary_expression/parse_primary_expression coupling test with a real runtime check: parse_primary_expression now wraps its dispatch (renamed parse_primary_expression_dispatch) and asserts the two agree on every parse, in every debug build, not just a hand-picked sample. Co-authored-by: logbie <1138960+logbie@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/parser/expr/primary.rs`:
- Around line 23-74: Compile the entire invariant-checking flow out of release
builds, not just its assertions. In the primary-expression parsing method
surrounding parse_primary_expression_dispatch, place both the leading-token
capture and all predicted_can_start verification logic behind a
#[cfg(debug_assertions)] block, while leaving result creation and return
behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dbd1ae6a-91d8-4e4e-aee1-4796209a2f50
📒 Files selected for processing (7)
Dev diary/2026-07-18-display-multiple-values.mdDocs/02-getting-started/hello-world.mdsrc/parser/expr/primary.rssrc/parser/helpers.rssrc/parser/stmt/io.rssrc/parser/tests.rstests/display_multiple_values_stdout_test.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/parser/helpers.rs
- Docs/02-getting-started/hello-world.md
- src/parser/stmt/io.rs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
Docs/02-getting-started/hello-world.md:178
- The list of boundaries that split space-separated
displayvalues is presented as exhaustive, but it omits boolean/nothing literals (e.g.yes,no,nothing), which also start fresh values. Tighten the wording to avoid implying only numbers can start a literal value.
outside a `display`. Space-separated values only split apart where the grammar
already has a boundary: a quote, a number, a parenthesis, or one of the
keywords that begins a value on its own (such as `not`, `file exists`, or an
action `call`).
src/parser/stmt/io.rs:438
- PR description says the multi-value
displayfold is left-associative, but the implementation (and tests/docs) are explicitly right-associative to matchwith(a with (b with c)). Please update the PR description to match the shipped behavior.
// Fold right-associatively — the same tree shape, evaluation order, and
// stringification order as explicit `with` (`a with b with c` parses as
// `a with (b with c)`, see the `with` handling in expr/binary.rs). This
// matters beyond cosmetics: `Concatenation` evaluates left, then right,
// then stringifies both, so a left-associative fold would stringify an
// earlier value *before* a later value's side effects (e.g. a list
// mutation) run, while this matches `with` exactly, value for value.
let mut value = values.pop().expect("at least one value was parsed");
The `parse_primary_expression` wrapper that keeps `can_start_primary_expression` in sync with the real dispatch is a debug-only invariant, but only its `debug_assert!` was stripped in release — the leading-token clone, reclassification, and error inspection still ran on every primary-expression parse, which is a hot path. Gate the whole check (leading capture + verification block) behind `#[cfg(debug_assertions)]` so release builds pay nothing for it. Debug behavior (panic on predicate/dispatch drift) is unchanged. Verified locally: cargo fmt --check, cargo build (debug), cargo check --release, cargo clippy --all-targets --all-features -D warnings, cargo test (616 lib + 17 stdout integration), and TestPrograms comprehensive programs — all green with the debug_assert coupling active. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KUmoGX1Eyt6H1BcXKg34Vu
Verified
|
| // joining them explicitly with `with` (`display a b c` parses identically to | ||
| // `display a with b with c`). See parse_display_statement in stmt/io.rs. |
…t comment Three bare words with no separator lex as a single multi-word identifier, not three display values, so `display a b c` does not demonstrate the fold. Use a mixed `display "x" y "z"` example (and note the multi-word-identifier caveat) so the comment matches shipped behavior. Comment-only; no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KUmoGX1Eyt6H1BcXKg34Vu
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/parser/stmt/io.rs:437
- The PR description says multi-value
displayfolds values into a left-associative concatenation, but the implementation (and tests/docs) deliberately fold right-associatively to matchwithevaluation order. Please update the PR description to avoid confusion for reviewers and future archaeology.
// Fold right-associatively — the same tree shape, evaluation order, and
// stringification order as explicit `with` (`a with b with c` parses as
// `a with (b with c)`, see the `with` handling in expr/binary.rs). This
// matters beyond cosmetics: `Concatenation` evaluates left, then right,
// then stringifies both, so a left-associative fold would stringify an
// earlier value *before* a later value's side effects (e.g. a list
// mutation) run, while this matches `with` exactly, value for value.
|
✅ CI green on Head
CodeRabbit's latest review found no actionable comments and auto-paused. Ready for your review/merge. Still maintainer-owned, as you flagged: the PR description still says "left-associative" (code/tests/docs are all right-associative — say the word and I'll edit it), and Finding 3 ( Generated by Claude Code |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/parser/stmt/io.rs:434
- PR description says space-separated
displayvalues are folded left-associatively, but the implementation and tests here explicitly fold right-associatively to matchwith(which is important for evaluation/stringification order). Please update the PR description to match the actual behavior so reviewers/users aren’t misled.
// Fold right-associatively — the same tree shape, evaluation order, and
// stringification order as explicit `with` (`a with b with c` parses as
// `a with (b with c)`, see the `with` handling in expr/binary.rs). This
// matters beyond cosmetics: `Concatenation` evaluates left, then right,
| > **Tip:** `with` and the space-separated form do the same job. Use whichever | ||
| > reads more clearly — mix them freely if you like. |
…ed freely The getting-started tip said to "mix them freely", but mixing the two within one `display` is not equivalent to either pure form: `display a with b c` parses the `with` into the first value and then folds, giving `(a with b) with c` (left-grouped), whereas pure `display a b c` / `display a with b with c` give `a with (b with c)` (right-grouped). With a side-effecting later value (e.g. a list pop) the grouping changes the observed evaluation/stringification order — confirmed at runtime: mixed prints `[before, after]after` vs `[before]after` for both pure forms. Tighten the tip to recommend one form per statement, per the docs-honesty policy (no overclaiming runtime behavior). Prose-only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KUmoGX1Eyt6H1BcXKg34Vu
|
Heads-up: pushed It resolves Copilot's docs-honesty finding on Verified the merged tree locally before pushing: Copilot's other note (PR description still says "left-associative") is the same description-wording item you're holding — yours unless you'd like me to flip it. Generated by Claude Code |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/parser/stmt/io.rs:437
- The PR description says multi-value
displayfolds values into a left-associative concatenation, but the implementation here explicitly folds right-associatively to matchwith(and the added tests also assert right-associative nesting). Please update the PR description to match the actual semantics so reviewers/users aren’t misled.
// Fold right-associatively — the same tree shape, evaluation order, and
// stringification order as explicit `with` (`a with b with c` parses as
// `a with (b with c)`, see the `with` handling in expr/binary.rs). This
// matters beyond cosmetics: `Concatenation` evaluates left, then right,
// then stringifies both, so a left-associative fold would stringify an
// earlier value *before* a later value's side effects (e.g. a list
// mutation) run, while this matches `with` exactly, value for value.
Summary
The
displaystatement now accepts multiple space-separated values, folding them into a single left-associative concatenation. This resolves a usability issue where trailing values were silently dropped, and aligns with the natural-language principle that users expectdisplay "label" valueto work intuitively.Changes
Parser (
src/parser/stmt/io.rs)parse_display_statementto:displaykeyword itself (preserving line/column info)(), folding each additional value into a left-associativeConcatenationParser::is_value_starthelper (src/parser/helpers.rs) to identify tokens that begin a fresh value, ensuring:display numbers 0) remains a singleIndexAccess(the0is absorbed by the first expression)Eolis not a value-start)ConcatenationBackward Compatibility
Documentation & Examples
Docs/02-getting-started/hello-world.md): Added "Display Several Values at Once" section with examples and a tip comparing space-separated andwithformsTestPrograms/display_multiple_values.wfl): Comprehensive example covering the original report,withequivalence, mixed values, expressions, index access, and conditional blocksTestPrograms/docs_examples/basic_syntax/display_multiple_01.wfl): 5-layer validated backing the new docs sectionTests
src/parser/tests.rs): 5 new tests covering:display numbers 0stays a single value)Concatenationwrapper)Dev Diary
Dev diary/2026-07-18-display-multiple-values.mddocumenting the bug report, root cause, implementation rationale, backward compatibility, spacing semantics, and test coverageImplementation Details
The fold uses the same semantics as
with— values are joined directly with no separator inserted. Spaces come from the quotes:display "I am " age " years old"producesI am 25 years old. This upholds the No-Unlearning Invariant: the space-separated form and thewithform are the same form, with nothing to unlearn.The implementation is conservative: only tokens that clearly begin a value trigger the fold. Operators, keywords, and statement boundaries do not, preserving existing behavior for edge cases and keeping the language predictable.
https://claude.ai/code/session_01KUmoGX1Eyt6H1BcXKg34Vu
Summary by CodeRabbit
displaynow accepts multiple space-separated values in one statement, desugaring like chainedwith(including keyword-led value forms such ascall,count from,not, and file/directory/process/header/list/read constructs) with right-associative folding.scores 0) remains a single value.display ... count/read ...patterns from being incorrectly folded.displayand spacing rules (no added spaces; spaces come from quoted text).displayvswithequivalence) plus a new test program.