Noticed while reviewing any_dep_changed's operand handling for #365. Not part of that fix.
Symptom
extra_outputs — the rule syntax for declaring outputs beyond %o — appears to be parsed and threaded into the graph builder, then never read. If that is right, a Tupfile declaring extra outputs is accepted silently and the declaration has no effect.
Evidence
Every occurrence of the name in the tree:
include/pup/parser/ast.hpp:99 Vec<PathPattern> extra_outputs; // declaration
include/pup/parser/ast.hpp:113 Vec<PathPattern> extra_outputs; // declaration
include/pup/graph/builder.hpp:50 Vec<parser::PathPattern> extra_outputs; // declaration
src/parser/parser.cpp:35 Vec<PathPattern> extra_outputs; // declaration
src/parser/parser.cpp:570 body.extra_outputs.push_back(...) // write
src/parser/parser.cpp:464 rule.extra_outputs = std::move(body->extra_outputs); // write
src/parser/parser.cpp:516 macro.extra_outputs = std::move(body->extra_outputs); // write
src/graph/builder.cpp:1578 def.extra_outputs = macro.extra_outputs; // write
Four declarations, four writes, zero reads. Nothing downstream of RuleDef::extra_outputs consumes it — no edge creation, no output-node registration, no serialization.
This is a grep-level finding and should be confirmed against the parse path before any fix (a read through a structured binding or a whole-struct copy would not match the name).
Why it matters beyond the dead field
Silently-accepted, inert syntax is the failure class this project keeps designing out: the user gets no error, the build looks correct, and the declared outputs simply are not tracked. If a declaration cannot be honoured it should be rejected at parse time, not absorbed.
It also has a second-order consequence that surfaced in #365's design. The disposition there rests on the invariant that a command's outputs have no dependency edges outside its operand vector — which is why any_dep_changed needs no edges_from leg (that leg would iterate to a guaranteed false). If extra_outputs were ever given semantics, that invariant would break and the missing leg would become a real defect. So this inert field is the change most likely to trip that invariant unnoticed. The invariant is being recorded as a one-line comment at any_dep_changed in the #365 PR; whoever implements extra_outputs must read it.
Possible dispositions
- Reject at parse time with a clear diagnostic, if the feature is not intended.
- Delete the syntax and the plumbing, if it was never intended.
- Implement it, in which case the
any_dep_changed invariant above must be revisited in the same change.
Which of these is right depends on whether tup itself supports the syntax and whether putup means to be conformant there — worth checking against the tup source before deciding.
Noticed while reviewing
any_dep_changed's operand handling for #365. Not part of that fix.Symptom
extra_outputs— the rule syntax for declaring outputs beyond%o— appears to be parsed and threaded into the graph builder, then never read. If that is right, a Tupfile declaring extra outputs is accepted silently and the declaration has no effect.Evidence
Every occurrence of the name in the tree:
Four declarations, four writes, zero reads. Nothing downstream of
RuleDef::extra_outputsconsumes it — no edge creation, no output-node registration, no serialization.This is a grep-level finding and should be confirmed against the parse path before any fix (a read through a structured binding or a whole-struct copy would not match the name).
Why it matters beyond the dead field
Silently-accepted, inert syntax is the failure class this project keeps designing out: the user gets no error, the build looks correct, and the declared outputs simply are not tracked. If a declaration cannot be honoured it should be rejected at parse time, not absorbed.
It also has a second-order consequence that surfaced in #365's design. The disposition there rests on the invariant that a command's outputs have no dependency edges outside its operand vector — which is why
any_dep_changedneeds noedges_fromleg (that leg would iterate to a guaranteedfalse). Ifextra_outputswere ever given semantics, that invariant would break and the missing leg would become a real defect. So this inert field is the change most likely to trip that invariant unnoticed. The invariant is being recorded as a one-line comment atany_dep_changedin the #365 PR; whoever implementsextra_outputsmust read it.Possible dispositions
any_dep_changedinvariant above must be revisited in the same change.Which of these is right depends on whether tup itself supports the syntax and whether putup means to be conformant there — worth checking against the tup source before deciding.