Skip to content

The legacy pattern_registry path is unreachable in production but still taxes every scanner-interface change #408

Description

@typeless

Recommended by the architect during the dep-scan cluster triage, after a correction to that triage established that this path is live code rather than the test-only surface it was first taken for. Evidence below re-verified independently at the line.

The path is unreachable in production

RulePatternRegistry reaches the builder through one option field, and nothing ever sets it:

  • include/pup/cli/context.hpp:61graph::RulePatternRegistry* pattern_registry = nullptr;
  • include/pup/graph/builder.hpp:37RulePatternRegistry const* pattern_registry = nullptr;
  • src/cli/context.cpp:970.pattern_registry = ctx_opts.pattern_registry, is the only occurrence of the field on a left-hand side anywhere in src/ or include/, and it is a pass-through of the default.

So the fallback arm at src/graph/builder.cpp:2136:

} else if (ctx.options.pattern_registry && !ctx.options.pattern_registry->empty()) {
    generated_rules = ctx.options.pattern_registry->match_and_generate(cmd_info);
}

can never be entered by a real build. RulePatternRegistry appears in exactly five files: its own implementation and header, the two option declarations above, and test/unit/test_rule_pattern.cpp — which constructs the registry directly rather than through the option.

This is a dead path, not dead code: the symbols are linked and the tests pass, which is why it has stayed invisible.

What keeping it costs

  1. A duplicate predicate spelling. rule_pattern.cpp:43 binds .matches = scanners::matches_gcc_compile, the std::string_view overload that tokenizes internally — a second way to ask the same question that exec::parse_command is a dead third lexical normalizer with its own escape semantics #346 deleted elsewhere.
  2. A surviving tokenize-twice. Inside this path matches tokenizes and .generate tokenizes again. Dep-scan check tokenizes each matched compile twice; the second product exists only to be tested with .empty() #363 removed that shape from the check path; this copy outlived it, and is evidence for this issue rather than grounds to reopen that one.
  3. A lockstep tax on every scanner-interface change. rule_pattern.cpp's .generate lambda is a third caller of build_dep_scans, so any change to the DepScan shape must move it too — a surface that has to be remembered rather than one the compiler leads you to. This already changes the file list for build_dep_command interns a scan string the unscanned-compile check discards, ~98M instructions and permanent pool residency #362.
  4. The string-entry overloads themselves. matches_gcc_compile(std::string_view) and its clang_cl twin exist to serve this path and its tests.

Deleting the path retires all four at once.

The one design question

Not every test in test_rule_pattern.cpp is about the legacy path — some pin behaviour that the scanner registry also owes. Deciding which of those pins re-home onto the scanner registry and which die with the path is the substantive part of this work; the deletion itself is mechanical. A pin that re-homes is behaviour we still promise, and losing it silently would be the real cost of this change.

Not in scope

The scanner registry is the live path and is not touched. This is removal of an unreachable alternative, not a redesign of scanner dispatch.

Refs #346, #362, #363.

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