From fe5a753e3bddd7d92a8c8d3bc0582c33ce88ec2f Mon Sep 17 00:00:00 2001 From: Aaron Aranda <73726359+aaronaranda@users.noreply.github.com> Date: Sun, 14 Jun 2026 10:48:37 -0600 Subject: [PATCH] Better token classification. Moved required option/positional checks. Handle unknown options and excess positionals --- CHANGELOG.md | 8 ++ src/cli/parse.hpp | 232 +++++++++++++++++-------------------- tests/test_positionals.cpp | 82 +++++++++++++ 3 files changed, 196 insertions(+), 126 deletions(-) create mode 100644 tests/test_positionals.cpp diff --git a/CHANGELOG.md b/CHANGELOG.md index 06c8548..74ff4d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- Option values are no longer mis-collected as positional arguments. + `parse_into` now walks argv in a single left-to-right pass with a shared + cursor, so a flag's value (e.g. the `foo.h` in `--include foo.h target.cpp`) + can't also be assigned to a positional. +- Unknown options and excess positionals are now reported as errors instead + of being silently ignored. + ### Changed - Parser rearchitected around a compile-time **parse plan**. `cli::build()` produces a `program_descriptor` (flat `option_descriptor` / `positional_descriptor` diff --git a/src/cli/parse.hpp b/src/cli/parse.hpp index 12503d1..9cc49e4 100644 --- a/src/cli/parse.hpp +++ b/src/cli/parse.hpp @@ -12,11 +12,9 @@ #include #include #include -#include #include #include #include -#include #include namespace cli::detail { @@ -63,10 +61,45 @@ inline auto extract_value(std::span args, std::size_t po return {.value = {}, .extra_consumed = 0, .found = false}; } +// Find the option descriptor matching an option token, or nullptr. +// Handles long (`--name`, `--name=v`), short (`-x`), and clustered counter +// short forms (`-vvv`). Clustered different shorts (`-abc`) are not yet +// supported (see P2). +inline auto match_option(std::span options, std::string_view arg) + -> const option_descriptor * { + for (const auto &opt : options) { + if (matches_long(arg, opt.long_name) || matches_short(arg, opt.short_name)) { + return &opt; + } + } + // clustered counter short form + if (std::size(arg) > 2 && arg[0] == '-' && arg[1] != '-') { + for (const auto &opt : options) { + if (opt.kind != option_kind::counter || opt.short_name != arg[1]) { + continue; + } + bool all_same = true; + for (std::size_t k = 1; k < std::size(arg); ++k) { + if (arg[k] != opt.short_name) { + all_same = false; + break; + } + } + if (all_same) { + return &opt; + } + } + } + return nullptr; +} + // single struct parser template auto parse_into(Opts &opts, std::string_view argv0, std::span args) -> void { + static constexpr auto plan = cli::build(); + + // Built-in --help / -h wins from any position, regardless of other errors. for (auto arg : args) { if (arg == "--help" || arg == "-h") { cli::print_help(argv0); @@ -74,140 +107,88 @@ auto parse_into(Opts &opts, std::string_view argv0, std::span positionals{}; - positionals.reserve(std::size(args)); - for (auto arg : args) { - if (!arg.starts_with('-') || arg == "-") { - positionals.push_back(arg); - } - } - + // classify tokens from left to right as an option, positional, or unknown. + std::vector option_seen(std::size(plan.options), false); + std::vector positional_filled(std::size(plan.positionals), false); std::size_t positional_idx{0}; - // switch over option_kind for matching and use the descriptor's invoke for parsing - static constexpr auto plan = cli::build(); - for (const auto &opt : plan.options) { - switch (opt.kind) { - case option_kind::help: - { - for (auto arg : args) { - if (matches_long(arg, opt.long_name) || matches_short(arg, opt.short_name)) { - cli::print_help(argv0); - std::exit(EXIT_SUCCESS); - } - } - break; - } - case option_kind::version: - { - for (auto arg : args) { - if (matches_long(arg, opt.long_name) || matches_short(arg, opt.short_name)) { - std::println("{}", plan.version != nullptr ? plan.version : "(no version set)"); - std::exit(EXIT_SUCCESS); - } - } - break; - } - case option_kind::bool_flag: - { - for (auto arg : args) { - if (matches_long(arg, opt.long_name) || matches_short(arg, opt.short_name)) { - opt.invoke(&opts, ""); - break; - } - } + for (std::size_t i = 0; i < std::size(args); ++i) { + auto arg = args[i]; + bool is_option = arg.starts_with('-') && arg != "-"; + + if (is_option) { + const option_descriptor *opt = match_option(plan.options, arg); + if (opt == nullptr) { + std::println(stderr, "Unknown option {}", arg); + cli::print_help(argv0); + std::exit(EXIT_FAILURE); + } + option_seen[static_cast(opt - std::data(plan.options))] = true; + + switch (opt->kind) { + case option_kind::help: + cli::print_help(argv0); + std::exit(EXIT_SUCCESS); + case option_kind::version: + std::println("{}", plan.version != nullptr ? plan.version : "(no version set)"); + std::exit(EXIT_SUCCESS); + case option_kind::bool_flag: + opt->invoke(&opts, ""); break; - } - case option_kind::counter: - { - for (auto arg : args) { - if (matches_long(arg, opt.long_name)) { - opt.invoke(&opts, ""); - } else if (arg.starts_with('-') && !arg.starts_with("--")) { - for (std::size_t k = 1; k < std::size(arg); ++k) { - if (arg[k] == opt.short_name) { - opt.invoke(&opts, ""); - } - } + case option_kind::counter: + { + std::size_t times = matches_long(arg, opt->long_name) ? 1 : (std::size(arg) - 1); + for (std::size_t t = 0; t < times; ++t) { + opt->invoke(&opts, ""); } + break; } - break; - } - case option_kind::scalar: - { - bool matched{false}; - for (std::size_t i = 0; i < std::size(args); ++i) { - if (matches_long(args[i], opt.long_name) || matches_short(args[i], opt.short_name)) { - auto vl = extract_value(args, i); - if (!vl.found) { - std::println(stderr, "Option {} is missing a value", args[i]); - std::exit(EXIT_FAILURE); - } - if (!opt.invoke(&opts, vl.value)) { - std::println(stderr, "Failed to parse `{}` value \"{}\"", args[i], vl.value); - std::exit(EXIT_FAILURE); - } - matched = true; - break; + case option_kind::scalar: + case option_kind::vector: + { + auto vl = extract_value(args, i); + if (!vl.found) { + std::println(stderr, "Option {} is missing a value", arg); + std::exit(EXIT_FAILURE); } - } - if (!matched && opt.is_required) { - std::println(stderr, "Missing required option --{}", opt.long_name); - cli::print_help(argv0); - std::exit(EXIT_FAILURE); - } - break; - } - case option_kind::vector: - { - for (std::size_t i = 0; i < std::size(args); ++i) { - if (matches_long(args[i], opt.long_name) || matches_short(args[i], opt.short_name)) { - auto vl = extract_value(args, i); - if (!vl.found) { - std::println(stderr, "Option {} is missing a value", args[i]); - std::exit(EXIT_FAILURE); - } - if (!opt.invoke(&opts, vl.value)) { - std::println(stderr, "Failed to parse `{}` value \"{}\"", args[i], vl.value); - std::exit(EXIT_FAILURE); - } - i += vl.extra_consumed; + if (!opt->invoke(&opts, vl.value)) { + std::println(stderr, "Failed to parse `{}` value \"{}\"", arg, vl.value); + std::exit(EXIT_FAILURE); } + i += vl.extra_consumed; + break; } - break; - } - } - } - - // Plan-driven dispatch for positionals. Vector positionals drain all - // remaining slots; scalar positionals take the next slot in declaration - // order. - for (const auto &pos : plan.positionals) { - if (pos.is_vector) { - while (positional_idx < std::size(positionals)) { - if (!pos.invoke(&opts, positionals[positional_idx])) { - std::println( - stderr, - "Failed to parse positional `{}` from \"{}\"", - pos.name, - positionals[positional_idx]); - std::exit(EXIT_FAILURE); - } - ++positional_idx; } - } else if (positional_idx < std::size(positionals)) { - if (!pos.invoke(&opts, positionals[positional_idx])) { - std::println( - stderr, - "Failed to parse positional `{}` from \"{}\"", - pos.name, - positionals[positional_idx]); + } else { + // handle positional tokens + if (positional_idx >= std::size(plan.positionals)) { + std::println(stderr, "Unexpected argument \"{}\"", arg); + cli::print_help(argv0); std::exit(EXIT_FAILURE); } - ++positional_idx; - } else if (pos.is_required) { - std::println(stderr, "Missing required positional argument `{}`", pos.name); + const auto &pos = plan.positionals[positional_idx]; + if (!pos.invoke(&opts, arg)) { + std::println(stderr, "Failed to parse positional `{}` from \"{}\"", pos.name, arg); + std::exit(EXIT_FAILURE); + } + positional_filled[positional_idx] = true; + if (!pos.is_vector) { + ++positional_idx; + } + } + } + + // required options/positionals + for (std::size_t o = 0; o < std::size(plan.options); ++o) { + if (plan.options[o].is_required && !option_seen[o]) { + std::println(stderr, "Missing required option --{}", plan.options[o].long_name); + cli::print_help(argv0); + std::exit(EXIT_FAILURE); + } + } + for (std::size_t p = 0; p < std::size(plan.positionals); ++p) { + if (plan.positionals[p].is_required && !positional_filled[p]) { + std::println(stderr, "Missing required positional argument `{}`", plan.positionals[p].name); cli::print_help(argv0); std::exit(EXIT_FAILURE); } @@ -216,7 +197,6 @@ auto parse_into(Opts &opts, std::string_view argv0, std::span +#include + +#include +#include +#include +#include +#include + +namespace { + +struct [[=cli::program("test")]] Opts { + [[=cli::desc("Source file"), =cli::positional, =cli::required]] + std::string source; + + [[=cli::desc("Include dirs"), =cli::short_name('I'), =cli::long_name("include")]] + std::vector includes; + + [[=cli::desc("Port"), =cli::short_name('p')]] + int port = 0; + + [[=cli::desc("Verbose"), =cli::short_name('v')]] + bool verbose = false; +}; + +template +auto parse(std::array const &args) -> Opts { + return cli::parse(std::string_view{"test"}, std::span{args.data(), args.size()}); +} + +} // namespace + +// The core B1 bug: `--include foo.h target.cpp` must put target.cpp in the +// positional and foo.h in the vector option — not foo.h in both. +TEST(positionals, flag_value_not_collected_as_positional) { + auto opts = parse(std::array{"--include", "foo.h", "target.cpp"}); + EXPECT_EQ(opts.source, "target.cpp"); + ASSERT_EQ(opts.includes.size(), 1u); + EXPECT_EQ(opts.includes[0], "foo.h"); +} + +// Scalar flag value (a number) must not be grabbed as the positional. +TEST(positionals, scalar_value_not_collected_as_positional) { + auto opts = parse(std::array{"--port", "9090", "main.cpp"}); + EXPECT_EQ(opts.source, "main.cpp"); + EXPECT_EQ(opts.port, 9090); +} + +// Positional before the flag. +TEST(positionals, positional_before_flag) { + auto opts = parse(std::array{"main.cpp", "--port", "9090"}); + EXPECT_EQ(opts.source, "main.cpp"); + EXPECT_EQ(opts.port, 9090); +} + +// Bool flag interleaved with positional — bool consumes no value. +TEST(positionals, bool_flag_does_not_eat_positional) { + auto opts = parse(std::array{"--verbose", "main.cpp"}); + EXPECT_TRUE(opts.verbose); + EXPECT_EQ(opts.source, "main.cpp"); +} + +// --name=value form leaves the following token as the positional. +TEST(positionals, equals_form_leaves_positional) { + auto opts = parse(std::array{"--port=9090", "main.cpp"}); + EXPECT_EQ(opts.port, 9090); + EXPECT_EQ(opts.source, "main.cpp"); +} + +// Multiple repeated vector flags plus a trailing positional. +TEST(positionals, repeated_vector_flag_then_positional) { + auto opts = + parse(std::array{"-I", "a", "--include", "b", "main.cpp"}); + EXPECT_EQ(opts.source, "main.cpp"); + ASSERT_EQ(opts.includes.size(), 2u); + EXPECT_EQ(opts.includes[0], "a"); + EXPECT_EQ(opts.includes[1], "b"); +}