Add #[must_use] to the transform and source combinators - #871
Open
0-jake-0 wants to merge 1 commit into
Open
Conversation
Dropping a combinator's result is not a no-op in this engine. `Stream::wire` has already registered the node with the shared `Builder`, and nothing in `src/` prunes unreachable nodes, so `s.map(f);` with no binding leaves a node wired that cycles every tick for the whole run producing a value nobody reads — a silent logic bug plus a permanent per-cycle cost, with no warning. Annotate the value-returning declarations in `StreamOps`, `SourceOps`, `StatisticsOps`, the inherent `Stream`/`GraphBuilder` combinators, and the hand-written `Signal` ones, all with one shared reason naming the actual consequence. Scope is transforms and sources only. `for_each` / `for_each_mut` / `print` / `logged` / `inspect` / `timed` / `finally` are called as bare statements throughout this tree and their side effect happens regardless of the handle; `feedback` closes its loop whether or not the pass-through is kept; `spawn` / `spawn_bounded` / `spawn_map` / `spawn_map_bounded` legitimately run a side-effecting sub-graph on a worker thread. Those stay unannotated — a false positive would train people to ignore the warning that matters. The derive follows the same split. `expand_signal`'s methods are inherent, so they carry the attribute; a new `#[op(sink)]` flag suppresses it for the five sink ops. `expand_fluent`'s cannot: its expansion lands in a trait `impl`, where `#[must_use]` is inert (rustc resolves a method call to the trait's item) and rustc warns `unused_attributes` — a future hard error. That is why the fluent attribute lives on the hand-written declaration, and it is now recorded next to both generators. `tests/trybuild/must_use_combinators.rs` pins it: `#![deny(unused_must_use)]` turns the warning into compiler output trybuild can compare, covering both the hand-written and generated halves, and exercising every sink under the same `deny` so a mis-scoped attribute breaks the fixture. Closes #830 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GRyBUej7RQrqxvrSjvqByM
4 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.
What this changes
Every value-returning transform and source in the fluent surface now
carries
#[must_use], with one shared reason:#[must_use = "a dropped stream stays wired and cycles every tick, producing an unread value"]126 methods in total — 95 hand-written declarations plus 31 generated ones:
StreamOps(fluent.rs)SourceOps(fluent.rs)StatisticsOps(adapters/statistics.rs)GraphBuilder::combine/replay_results,Stream::collapse_accumulate/split/filter_noneSignalcombinators (signal.rs)Signalcombinators (expand_signal)Why
Dropping a combinator's result is not a no-op here.
Stream::wire(
fluent.rs:739-752) has already registered the node with the sharedBuilder, and there is no reachability pruning anywhere insrc/, sos.map(f);with no binding leaves a node wired that cycles every tick for thewhole run producing a value nobody reads. That is a silent logic bug — the
author believes they transformed the stream — plus a permanent per-cycle cost,
and nothing warned about it.
latency.rsand ~50 adapter methods already usedthe attribute;
fluent.rshad zero.Closes #830
How it was verified
cargo fmt --allcargo lintandcargo lint-all— both clean. This is the load-bearingcheck for scope: they run clippy
-D warningsover--all-targets,which is every example, bench and test in the tree. An over-wide
annotation would have failed here on the repo's own code.
cargo test -p wingfoil --all-features— all suites pass except the*_integrationones, which need Docker/live services and failidentically on
mainin this sandbox.cargo test -p wingfoil-deriveThe compile test
crates/wingfoil/tests/trybuild/must_use_combinators.rs, picked up by theexisting
t.compile_fail("tests/trybuild/*.rs")harness.#[must_use]produces a warning, which an ordinary test cannot see, so the fixture
#[deny(unused_must_use)]s — the warning becomes compiler output trybuildcompares against a checked-in
.stderr.It pins both directions:
StreamOps/SourceOpsdeclarations and through the derive-generatedSignalones,so both halves of the change are covered;
deny, soif one of them ever grows the attribute the fixture stops compiling.
Regenerate with
TRYBUILD=overwriteafter an intentional message change.Notes for the reviewer
The derive: only one of the two generators can carry it
The issue asked for the attribute in both
expand_fluentandexpand_signal.Only
expand_signalcan have it, and the reason is worth recording:expand_fluent's expansion lands inside a traitimpl.#[must_use]there is inert — rustc resolves a method call to the trait's item, not the
impl's — and it also warns
unused_attributes("cannot be used on traitmethods in impl blocks", already flagged as a future hard error), which
under
-D warningswould break every downstream op author. So the fluentattribute lives on the hand-written declaration, which is where the trait's
documented public surface already lives.
expand_signal's methods are inherent, so they carry it themselves.Both generators now say so in their doc comments.
New
#[op(sink)]flagexpand_signalgenerates a method for every#[op(fluent)]op, sinksincluded, so an unconditional attribute would have annotated
print/for_each/inspect/timed/finally. Those five are now declared#[op(build = …, fluent, sink)], which suppresses it. The flag is documentedon
OpArgs, in/new-op(new step 4c) and indocs/adding-an-op.md.Deliberately left OFF — sinks
Their side effect happens regardless of what you do with the returned handle,
and they are called as bare statements in this very tree
(
examples/adapters/fix/main.rs:73,81; the kdb examples likewise):for_each,for_each_mutprint,loggedinspect,timedfinallyDeliberately left OFF — ambiguous, per the "a false positive is worse than
a miss" rule
StreamOps::feedbacks.feedback(&sink);as a statement closes the loop correctly — the handle is optionalSourceOps::spawn,spawn_boundedg.spawn(|wg| wg.ticker(p).for_each(..));is a legitimate "run this side-effecting graph on a thread"StreamOps::spawn_map,spawn_map_boundedGraphBuilder::custom_nodecycleclosure may exist purely for its side effectGraphBuilder::source,Stream::wireNot in scope, but noticed
GraphBuilder::build/Stream::buildhave the same "silently does nothing"shape — a dropped
Runnermeans the graph never runs — andGraphBuilder::snapshotis pure. Both are outside "combinators", and
buildin particular needs acheck for
#[should_panic]tests that call it bare for its call-onceprecondition, so I left them for a follow-up rather than widening this diff.
Generated by Claude Code