Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions derive/src/codegen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1304,10 +1304,6 @@ fn flag_arm(cli: &Cli, i: usize, field: &Field) -> TokenStream {
}
}

/// Whether another occurrence is a command-line mistake rather than another value.
///
/// Counts and collections repeat by definition, and `var` explicitly opts a value-taking flag
/// into repetition. Every other flag matches clap's default of one occurrence.
/// Whether a repeat of this flag is only a duplicate *within one command*.
///
/// A `global` flag is in scope for every descendant, and clap lets it be given again on a
Expand All @@ -1331,6 +1327,26 @@ fn reset_per_level(cli: &Cli) -> TokenStream {
quote!(#(#resets)*)
}

/// Whether another occurrence is a command-line mistake rather than another value.
///
/// Counts and collections repeat by definition, and `var` explicitly opts a value-taking flag
/// into repetition. Every other flag matches clap's default of one occurrence.
///
/// **This is stricter than the grammar, deliberately.** The corpus specifies the opposite for a
/// *spec-driven* parse — `long-repeated-keeps-the-last` says "a repeat is a correction, typically
/// a wrapper appending to a command line it did not write" — and usage-argv's parser and
/// usage-lib both honour that. The rule lives here, in the derive's post-binding layer, and
/// nowhere else: `Error::DuplicateFlag` is constructed at one call site, in this file.
///
/// Kept, because the two cases are not the same case. A spec parsed at run time may well be
/// describing someone else's command line, which is the wrapper the corpus has in mind. A
/// derived CLI is an authored, fixed surface, where `--jobs 1 --jobs 2` is far likelier to be a
/// mistake than an amendment — and it is what clap does, so an adopter replacing clap sees no
/// change. Dropping it would make a derived binary quietly accept a line clap rejects today,
/// which is the direction that costs an adopter rather than the one that helps.
///
/// Recorded because it looks like an inconsistency and is not one. `differential.rs` in the gate
/// carries the same note from the other end.
fn rejects_duplicate(field: &Field) -> bool {
matches!(field.kind, Kind::Flag { .. })
&& !matches!(field.shape, Shape::Count | Shape::Many)
Expand Down
30 changes: 30 additions & 0 deletions tasks/perf-shadow.sh
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,19 @@ verify() {
fi
}

# The ratio against clap this file expects to hold, and below which it says so.
#
# Not a gate — the workflow runs this with `|| true`, and a shared runner is no place to fail a
# build on an instruction count. It is an annotation, because the alternative turned out to be
# nobody noticing: the number went from 117x to 97x across a run of feature work, unremarked,
# because the only place it appeared was one line of a report nobody diffs. clap is flat, so the
# movement was ours.
#
# 80 rather than 117: the point is to catch a slide, not to pin the current figure. A parse that
# is only 80x cheaper than clap's has stopped being the thing this project claims, and that is
# worth a sentence in the log before it is worth an argument about the target.
CLAP_RATIO_FLOOR=80

# Two significant figures either side of the decimal point, so a framework within a factor
# of usage reads as 1.5x rather than 1x and one three orders away still reads cleanly.
ratio() {
Expand Down Expand Up @@ -119,3 +132,20 @@ bpaf_cold=$(cold parse-n-bpaf)
} >"$tmp"

cat "$tmp" >"$out"

# Said after the report is written, so a failure here cannot leave the comment truncated — the
# same reason the table is built in a temporary file.
#
# `::warning::` when running under GitHub Actions, a plain line otherwise, so the same script is
# useful at a terminal.
clap_ratio=$(awk -v a="$clap_cold" -v b="$usage_cold" 'BEGIN { printf "%d", a / b }')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Integer formatting obscures low ratios

The new warning always formats clap_ratio as an integer, while the existing report preserves one decimal below 10x. A ratio below 1 therefore becomes 0x, producing a misleading claim that usage is “0x cheaper than clap” precisely when the warning needs to explain a severe regression.

Knowledge Base Used: Compiled argv parsing and derives

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code

if [ "$clap_ratio" -lt "$CLAP_RATIO_FLOOR" ]; then
message="the parse is ${clap_ratio}x cheaper than clap's, under the ${CLAP_RATIO_FLOOR}x this\
file expects. clap is flat, so a fall here is usage getting more expensive rather than clap\
getting cheaper. See tasks/perf-shadow.sh."
if [ -n "${GITHUB_ACTIONS:-}" ]; then
echo "::warning title=Shadow ratio below floor::$message"
else
echo "warning: $message" >&2
fi
fi