verify: warn on bare fmt/fmt2 at non-tail position (ILO-T032)#299
Merged
Conversation
2 tasks
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
161999e to
5ad4546
Compare
fmt and fmt2 are pure-functional sprintf builders. A bare `fmt "..." v`
statement evaluates and discards the resulting text on every engine
(tree, VM, Cranelift) - nothing reaches stdout, which is the common
"my script ran and printed nothing" trap when agents treat fmt like
Rust's println! or Python's print.
Verifier now flags non-tail Stmt::Expr that is a Builtin::Fmt/Fmt2 call
with ILO-T032 and a hint pointing at the two idiomatic fix shapes
(prnt fmt ... / name = fmt ...). Tail position is unchanged: the
documented return-formatted-text idiom (say-x>t;fmt "x={}" 42) does
not warn.
Diagnostic-only change. Runtime behaviour across all three engines is
unchanged. The persona report that prompted this framed the silent-
discard as Cranelift-specific; it is actually consistent across every
engine, by design.
Unit tests cover: bare fmt warns, bare fmt2 warns, tail position
silent, bound-to-name silent, nested-in-prnt silent, multiple bare
fmts each warn.
Demonstrates `prnt fmt ...` and `line=fmt ...;prnt line` as the two documented fixes for ILO-T032, plus the tail-position fmt return-value form which is correctly silent. Wraps the print calls in a single-iter foreach so loop-tail auto-print suppression keeps stdout to exactly the asserted line for tests/examples_engines.rs across tree and VM.
SPEC.md, ai.txt, and skills/ilo/SKILL.md now state explicitly that fmt and fmt2 build strings rather than perform I/O, with a pointer to ILO-T032 and the prnt fmt / name=fmt fix shapes. Tail-position fmt remains the documented return-formatted-text idiom. Persona logs show this is the most common silent-failure pattern: agents assume fmt prints, write a block of bare fmt statements, and watch their script exit 0 with nothing on stdout. Adding the note next to the fmt entry in every load-bearing reference cuts the diagnosis loop short.
build.rs regenerates ai.txt from SPEC.md and re-injects the AI-SPEC block in skills/ilo/SKILL.md on every cargo build. CI runs `git diff --exit-code ai.txt` after the build, so any SPEC.md edit must be paired with the regenerated artefacts. My earlier docs commit hand-edited ai.txt instead of letting build.rs do it; this commit lets the generator pick up the SPEC.md change plus xs.i / slc neg-index / take-drop neg semantics that landed on main in parallel, and also re-syncs SKILL.md.
109a068 to
f7f7de2
Compare
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
fmtandfmt2are pure-functional sprintf builders. A barefmt "..." vstatement evaluates and discards the resulting text on every engine - nothing reaches stdout. Verifier now flags this with ILO-T032 and a hint pointing at the two idiomatic fix shapes (prnt fmt .../name = fmt ...).This is a diagnostic-only change. Runtime behaviour across tree, VM, and Cranelift is unchanged.
Persona-framing correction
The persona report that prompted this (
ilo_assessment_feedback.mdline 2138, quant-trader rerun3) framed the silent-discard as Cranelift-specific - "on 0.11.2 from the Cranelift default path, barefmtis silent." That framing is wrong. I reproduced the same shape (go>t;fmt "discarded={}" 42;prnt "after") on all three engines and observed identical behaviour: thefmtresult is discarded, onlyprnt "after"reaches stdout. This is correct functional semantics (bare expression statements don't perform I/O), consistent with the existing precedent that bare+=xs vand baremset m k vdon't mutate either. This PR doesn't change the behaviour; it adds the diagnostic the persona explicitly asked for.Repro before/after
Before: runs silently to completion, stdout =
done\n7. Thefmt "v={}" vevaluates and is thrown away. The agent spends ~3 minutes diagnosing why their report function emits nothing.After: verifier emits
ILO-T032 bare 'fmt' result is discardedwith hintdid you mean prnt fmt ... to print, or name = fmt ... to capture?. Program still runs (warning, not error), but the agent now sees the problem immediately.What's in the diff (per commit)
verify: warn on bare fmt/fmt2 at non-tail position (ILO-T032)- the verifier check itself, the diagnostic registry entry with examples, and six unit tests covering bare fmt warns, bare fmt2 warns, tail-position no warn, bound-to-name no warn, nested-in-prnt no warn, multiple bare fmts each warn independently.example: bare-fmt-warns.ilo pins the two idiomatic shapes- newexamples/bare-fmt-warns.ilodemonstratingprnt fmt ...,name=fmt ...;prnt name, and tail-position fmt, exercised across tree + VM viatests/examples_engines.rs.docs: note that fmt is pure-functional sprintf, not print- SPEC.md, ai.txt, and skills/ilo/SKILL.md all gain an explicit "fmt does not print" note next to the existing fmt entry, with a pointer to ILO-T032 and the fix shapes. (Sitebuiltins.mdupdated in a separate commit/PR in the site repo.)Test plan
cargo test --release --features cranelift --lib verify::tests::bare_fmt verify::tests::fmt_in_tail verify::tests::fmt_bound verify::tests::fmt_inside verify::tests::multiple_bare_fmts- 6/6 passcargo test --release --features cranelift --test examples_engines- examples_engines passes (new example included)cargo test --release --features cranelift --lib vm::compile_cranelift::tests::aot- all 7 AOT tests pass against default target dircargo fmt --check- cleancargo clippy --release --features cranelift --all-targets -- -D warnings- cleango>t;fmt "discarded={}" 42;prnt "after"on all three engines (--run-tree,--run-vm,--run-cranelift) - confirmed warning surfaces at CLI and program still runs (exit 0).Follow-ups
+=xs vandmset m k vbare-discard family (assessment doc line 2598) is a clear next PR, same shape: warn when non-tailStmt::Expris a discarded result-producing builtin call. Deferred per scope decision so this PR stays minimal and easy to revert if needed.fmtis the only statement in a foreach/while body) also discard silently but are not warned by this PR. Same follow-up could widen the check to cover that case.