Pairs under an if / unless in a jbuilder template are emitted instead of dropped - #361
Conversation
A statement of an object template that is not a `json.*` call was
Unknown, so a block `if … else … end`, an `unless` and the `if` /
`unless` modifiers became an empty append and the pairs inside them
were dropped.
An `If` statement is now `Cond`: the same `if`, each branch's pairs
appended inside it. The pair loop moves into `emit_pairs`, which threads
a three-state comma (`First`, `After`, `Unknown`) through the
statements and both branches. Only after a conditional whose branches
disagree is the comma decided when the template runs, by asking whether
the accumulator still ends in the object's `{`. Templates without a
conditional emit what they did before.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughJbuilder lowering now recognizes conditional statements and modifiers. It emits conditional pairs while tracking comma placement across branches. Integration tests check generated Ruby syntax and verify rendered JSON for conditional branches and modifiers. ChangesConditional Jbuilder pairs
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant ExprNodeIf
participant classify
participant emit_pairs
participant RubyView
ExprNodeIf->>classify: Recognize conditional expression
classify->>emit_pairs: Pass condition and branches as JbStmt::Cond
emit_pairs->>emit_pairs: Emit branch pairs with separator state
emit_pairs->>RubyView: Generate conditional Ruby pairs
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change emits pairs inside 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Probed with
roundhouse 2026.9.18 (7fad14c0), Linux x86-64, CRuby 4.0.5.In a jbuilder object template, a statement under a condition is dropped with no diagnostic: a block
if … else … end, anunless, and theif/unlessmodifiers. An optional field (json.label x if …) or a branch between two values is the usual way a template writes either.Widget 1 has
name: "b", size: 5, widget 2name: "a", size: nil:branch{"id":1,"big":true},{"id":2,"big":false}{"id":1},{"id":2}modifier{"id":1,"label":"b","note":"b"},{"id":2}{"id":1},{"id":2}first_pair{"big":true,"id":1},{"id":2}{"id":1},{"id":2}classify(src/lower/jbuilder_to_library/mod.rs) only looks atjson.*sends, so theIf(ingest givesunlessthe same node with the branches swapped) isUnknownand becomesio << "".Fix
A new statement kind,
Cond, for anIfstatement of an object template. It emits the sameif, each branch's pairs appended inside it:The commas needed one change.
emit_objectcounted pairs statically (emitted > 0); after a conditional with a pair in one branch only, the next pair can't know at lowering time whether it is the first. The pair loop moves intoemit_pairs, which threads a three-stateSep(First,After,Unknown) through the statements and both branches: a branch starts from the state before theif, and the state after it is the branches' common state, orUnknownwhen they differ. Only in theUnknownstate is the comma decided at run time, by asking the accumulator (first_pairabove):That is exact because an object's pairs are the only thing appended after its
{, and no complete JSON value ends in{. Templates without a conditional emit exactly what they did before (the comma state isFirstorAfterthroughout). Anunless(anIfwith an empty then-branch after ingest) is emitted asif !(cond), so it doesn't print an empty branch.Tests
tests/jbuilder_conditional_pairs.rs: the three templates above, ingested in memory. It checks the emitted Ruby (every view parses; both branches carry their pair; noio << ""left), then writes the emitted views next toruntime/ruby/json_builder.rb, renders them on CRuby for both widgets with Structs for the rows, and compares with the Rails answers in the table.first_pairis the case that takes the run-time comma. Without the change, 2 of its 3 tests fail (the parse test passes); the render test gets the main column.Full suite,
cargo test --release --no-fail-faston this machine (fixtures/real-bloggenerated withbin/rh fixture):7fad14c0: 3247 passed, 2 failed, 116 ignoredThe 2 failures are the same on both (
resource_and_unit_batch_helpers_preserve_failures_and_contracts,the_store_fixture_checks_clean).Sibling PRs that also touch
src/lower/jbuilder_to_library/mod.rs: #355, #356, #359. Each is independent ofmain; whichever lands second rebases. This one moves the pair loop into a function, so its conflicts with the others are the arms they add to that loop.Found while compiling a Rails API app with
--target spinel.🤖 Generated with Claude Code
Summary by CodeRabbit