fix(hydro_lang): still materialize handoff refs when only captured by for_each closures#3044
Merged
Merged
Conversation
…losures `HydroRoot::ForEach`'s DFIR emission only *looked up* captured handoff references (by_ref/by_mut) in `built_tees`, assuming some node-level operator had already emitted the reference node. When a `for_each` closure was the sole consumer of a reference — e.g. the common accumulator pattern `stream.for_each(q!(|x| *acc_mut += x))` — graph compilation panicked with "ForEach singleton ref not found in built_tees — ref node was not emitted". This path was never validated end-to-end: the original commit adding for_each ref-capture support only included compile-only tests that stop at `flow.finalize()`, which does not emit DFIR. Fix: `ForEach` now emits each captured `Reference` node via `emit_core` (deduplicated through `built_tees` by the `Reference` arm), mirroring how node-level operators (map/filter/etc.) materialize their closures' captured references during their bottom-up traversal. The emission runs in both the Builders and Callback paths so statement IDs stay consistent between them. Statement-ID assignment is unchanged for programs without ref-capturing for_each (the loop is a no-op when `singleton_refs` is empty). Tests: added two focused regression tests in handoff_ref.rs (`singleton_by_ref_for_each_sole_consumer_emits`, `singleton_by_mut_for_each_sole_consumer_emits`) where the for_each closure is the only consumer of the reference. They drive the flow through full DFIR emission via `DeployFlow::preview_compile()` (which `flow.finalize()` alone does not reach). Both fail with the built_tees panic before the fix and pass after. The tests use tick-scoped references so they remain valid if top-level bounded references are later restricted (see #3010). Verified: full hydro_lang suite 187/187 pass (deploy+sim), clippy clean, cargo +nightly fmt applied. Co-authored-by: Infinity 🤖 <infinity@hydro.run> PR: #3044
Deploying hydro with
|
| Latest commit: |
fc1fdc3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b8a914f3.hydroflow.pages.dev |
| Branch Preview URL: | https://mingwei-fix-foreach-refcaptu.hydroflow.pages.dev |
shadaj
reviewed
Jul 17, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a DFIR emission panic when a HydroRoot::ForEach closure is the only consumer of a captured handoff reference (by_ref/by_mut), by ensuring the captured HydroNode::Reference nodes are materialized during ForEach emission (matching behavior of node-level operators).
Changes:
- Update
HydroRoot::ForEachDFIR emission toemit_coreeach captured singleton reference (deduped viabuilt_tees) before generatingfor_each(...). - Add deploy-gated regression tests that drive the flow through full DFIR emission via
preview_compile()to cover the previously-unreached path.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| hydro_lang/src/compile/ir/mod.rs | Materializes for_each-captured handoff references during DFIR emission to prevent built_tees lookup panics. |
| hydro_lang/src/handoff_ref.rs | Adds regression tests covering the “for_each is sole consumer of ref/by_mut” DFIR emission case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…liced! in tests Two review comments on the for_each ref-capture fix PR: 1. copilot-pull-request-reviewer: in HydroRoot::ForEach, stmt_id was minted before emitting the captured refs, so the Callback path's leaf_callback no longer observed "most recently allocated ID == this root's ID", unlike other HydroRoot variants. The concern is latent (traverse_dfir / the Callback path has no in-tree callers today), but the invariant is worth preserving. Moved the stmt_id allocation to after the ref-emission loop, so statement IDs follow emission order (refs first, then for_each) and the callback invariant matches the other root variants. 2. shadaj: rewrote the two regression tests to use sliced! instead of direct .tick()/.batch() calls (direct tick usage is being deprecated). The tests now slice the input stream via `use(items, nondet!(...))`, build the tick-local singleton via `items.location().source_iter(...).fold(...)` (matching the existing sim-test pattern), and end the block with the for_each call — possible because `()` is Unslicable — keeping the for_each closure as the sole consumer of the reference. Re-verified red/green: with the fix reverted, both sliced!-based tests still fail with the "ForEach singleton ref not found in built_tees" panic; with the fix they pass. Full hydro_lang suite 187/187 pass (deploy+sim), clippy clean, fmt applied. Co-authored-by: Infinity 🤖 <infinity@hydro.run> PR: #3044
MingweiSamuel
force-pushed
the
mingwei/fix-foreach-refcapture
branch
from
July 20, 2026 16:34
bf5d9f7 to
fc1fdc3
Compare
shadaj
approved these changes
Jul 20, 2026
MingweiSamuel
changed the title
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.
HydroRoot::ForEach's DFIR emission only looked up captured handoff references(by_ref/by_mut) in
built_tees, assuming some node-level operator had alreadyemitted the reference node. When a
for_eachclosure was the sole consumer of areference — e.g. the common accumulator pattern
stream.for_each(q!(|x| *acc_mut += x))— graph compilation panicked with"ForEach singleton ref not found in built_tees — ref node was not emitted".
This path was never validated end-to-end: the original commit adding for_each
ref-capture support only included compile-only tests that stop at
flow.finalize(),which does not emit DFIR.
Fix:
ForEachnow emits each capturedReferencenode viaemit_core(deduplicated through
built_teesby theReferencearm), mirroring hownode-level operators (map/filter/etc.) materialize their closures' captured
references during their bottom-up traversal. The emission runs in both the
Builders and Callback paths so statement IDs stay consistent between them.
Statement-ID assignment is unchanged for programs without ref-capturing
for_each (the loop is a no-op when
singleton_refsis empty).Tests: added two focused regression tests in handoff_ref.rs
(
singleton_by_ref_for_each_sole_consumer_emits,singleton_by_mut_for_each_sole_consumer_emits) where the for_each closure isthe only consumer of the reference. They drive the flow through full DFIR
emission via
DeployFlow::preview_compile()(whichflow.finalize()alone doesnot reach). Both fail with the built_tees panic before the fix and pass after.
The tests use tick-scoped references so they remain valid if top-level bounded
references are later restricted (see #3010).
Verified: full hydro_lang suite 187/187 pass (deploy+sim), clippy clean,
cargo +nightly fmt applied.