Skip to content

extra_outputs is parsed and threaded into RuleDef but never read — accepted-but-inert rule syntax #370

Description

@typeless

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

  1. Reject at parse time with a clear diagnostic, if the feature is not intended.
  2. Delete the syntax and the plumbing, if it was never intended.
  3. 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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions