fix: improve unrecognized arg handling in parser #24984
Conversation
WalkthroughThe changes modify unrecognized argument and flag handling in the streaming parser. Error messages are updated to distinguish between flags and arguments with values. The parser progression logic is simplified to always advance after emitting diagnostics. Index initialization is adjusted for short-flag handling. Changes
Pre-merge checks✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/deps/zig-clap/clap/streaming.zig(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: test/js/sql/sql.test.ts:195-202
Timestamp: 2025-09-25T22:07:13.851Z
Learning: PR oven-sh/bun#22946: JSON/JSONB result parsing updates (e.g., returning parsed arrays instead of legacy strings) are out of scope for this PR; tests keep current expectations with a TODO. Handle parsing fixes in a separate PR.
| // unrecognized command | ||
| // if flag else arg | ||
| if (arg_info.kind == .long or arg_info.kind == .short) { | ||
| if (warn_on_unrecognized_flag) { | ||
| Output.warn("unrecognized flag: {s}{s}\n", .{ if (arg_info.kind == .long) "--" else "-", name }); | ||
| Output.flush(); | ||
| } | ||
|
|
||
| // continue parsing after unrecognized flag | ||
| return parser.next(); | ||
| } | ||
|
|
||
| if (warn_on_unrecognized_flag) { | ||
| Output.warn("unrecognized argument: {s}\n", .{name}); | ||
| if (maybe_value) |v| { | ||
| Output.warn("unrecognized argument: --{s}={s}\n", .{ name, v }); | ||
| } else { | ||
| Output.warn("unrecognized flag: --{s}\n", .{name}); | ||
| } | ||
| Output.flush(); | ||
| } | ||
| return null; | ||
| return parser.next(); | ||
| }, |
There was a problem hiding this comment.
Unrecognized long options now silently drop InvalidArgument errors
With this change, an unrecognized long argument (e.g. --q or --q=1) never raises error.InvalidArgument anymore:
- When
warn_on_unrecognized_flagisfalse(the default, and whattest "errors"uses), the code just falls through theifand thenreturn parser.next();, effectively skipping the unknown flag with no diagnostic. - This contradicts the expectations in
test "errors"for--q/--q=1and diverges from short/positional handling which still reportInvalidArgument.
To keep the new, more precise warning text while preserving prior error semantics, consider gating the “warn & skip” behavior behind the flag and otherwise returning an error, e.g.:
- // unrecognized command
- if (warn_on_unrecognized_flag) {
- if (maybe_value) |v| {
- Output.warn("unrecognized argument: --{s}={s}\n", .{ name, v });
- } else {
- Output.warn("unrecognized flag: --{s}\n", .{name});
- }
- Output.flush();
- }
- return parser.next();
+ // unrecognized command
+ if (warn_on_unrecognized_flag) {
+ if (maybe_value) |v| {
+ Output.warn("unrecognized argument: --{s}={s}\n", .{ name, v });
+ } else {
+ Output.warn("unrecognized flag: --{s}\n", .{ name });
+ }
+ Output.flush();
+ // Skip unknown flag and keep parsing further args.
+ return parser.next();
+ }
+ // Preserve existing error semantics when warnings are disabled.
+ return parser.err(arg, .{ .long = name }, error.InvalidArgument);That keeps the improved warning messages for the Bun CLI use‑case, while ensuring the StreamingClap tests and existing callers relying on InvalidArgument continue to behave as before.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // unrecognized command | |
| // if flag else arg | |
| if (arg_info.kind == .long or arg_info.kind == .short) { | |
| if (warn_on_unrecognized_flag) { | |
| Output.warn("unrecognized flag: {s}{s}\n", .{ if (arg_info.kind == .long) "--" else "-", name }); | |
| Output.flush(); | |
| } | |
| // continue parsing after unrecognized flag | |
| return parser.next(); | |
| } | |
| if (warn_on_unrecognized_flag) { | |
| Output.warn("unrecognized argument: {s}\n", .{name}); | |
| if (maybe_value) |v| { | |
| Output.warn("unrecognized argument: --{s}={s}\n", .{ name, v }); | |
| } else { | |
| Output.warn("unrecognized flag: --{s}\n", .{name}); | |
| } | |
| Output.flush(); | |
| } | |
| return null; | |
| return parser.next(); | |
| }, | |
| // unrecognized command | |
| if (warn_on_unrecognized_flag) { | |
| if (maybe_value) |v| { | |
| Output.warn("unrecognized argument: --{s}={s}\n", .{ name, v }); | |
| } else { | |
| Output.warn("unrecognized flag: --{s}\n", .{ name }); | |
| } | |
| Output.flush(); | |
| // Skip unknown flag and keep parsing further args. | |
| return parser.next(); | |
| } | |
| // Preserve existing error semantics when warnings are disabled. | |
| return parser.err(arg, .{ .long = name }, error.InvalidArgument); |
🤖 Prompt for AI Agents
In src/deps/zig-clap/clap/streaming.zig around lines 89 to 99, unrecognized long
options currently always fall through to parser.next() and silently drop
error.InvalidArgument; change the logic so that if warn_on_unrecognized_flag is
true you keep the existing warn/flush/return parser.next() behavior, but if it
is false you return error.InvalidArgument (preserving the prior error semantics
used by tests and short/positional handling); ensure the returned
InvalidArgument includes the option name (and value variant when present)
consistent with existing error construction elsewhere in this file.
1 participant
What does this PR do?
Parser now distinguishes between unrecognized long args with and without values.
How did you verify your code works?
Tested the changes to ensure correct warning messages are displayed for various input scenarios.