From a9df60ff1cd81fa5f24f96ea0dde35b508c574c2 Mon Sep 17 00:00:00 2001 From: vincent-rg Date: Mon, 5 Feb 2024 21:47:09 +0100 Subject: [PATCH] AK: Update OptionParser::m_arg_index by substracting skipped args On argument swapping to put positional ones toward the end, m_arg_index was pointing at "last arg index" + "skipped args" + "consumed args" and thus was pointing ahead of the skipped ones. m_arg_index now points after the current parsed option arguments. --- AK/OptionParser.cpp | 8 +- Meta/Lagom/CMakeLists.txt | 2 + Tests/AK/CMakeLists.txt | 1 + Tests/AK/TestOptionParser.cpp | 174 +++++++++++++++++++++ Tests/LibCore/TestLibCoreArgsParser.cpp | 199 +++++++++++++++++++++++- 5 files changed, 382 insertions(+), 2 deletions(-) create mode 100644 Tests/AK/TestOptionParser.cpp diff --git a/AK/OptionParser.cpp b/AK/OptionParser.cpp index 681e24fa26e..2aa29fbe9df 100644 --- a/AK/OptionParser.cpp +++ b/AK/OptionParser.cpp @@ -239,7 +239,7 @@ void OptionParser::shift_argv() { // We've just parsed an option (which perhaps has a value). // Put the option (along with its value, if any) in front of other arguments. - if (m_consumed_args == 0 || m_skipped_arguments == 0) { + if (m_consumed_args == 0 && m_skipped_arguments == 0) { // Nothing to do! return; } @@ -253,6 +253,12 @@ void OptionParser::shift_argv() m_args.slice(m_arg_index, m_consumed_args).copy_to(buffer_bytes); m_args.slice(m_arg_index - m_skipped_arguments, m_skipped_arguments).copy_to(m_args.slice(m_arg_index + m_consumed_args - m_skipped_arguments)); buffer_bytes.slice(0, m_consumed_args).copy_to(m_args.slice(m_arg_index - m_skipped_arguments, m_consumed_args)); + + // m_arg_index took into account m_skipped_arguments (both are inc in find_next_option) + // so now we have to make m_arg_index point to the beginning of skipped arguments + m_arg_index -= m_skipped_arguments; + // and let's forget about skipped arguments + m_skipped_arguments = 0; } bool OptionParser::find_next_option() diff --git a/Meta/Lagom/CMakeLists.txt b/Meta/Lagom/CMakeLists.txt index d1bd533a97d..d5b2e4e05af 100644 --- a/Meta/Lagom/CMakeLists.txt +++ b/Meta/Lagom/CMakeLists.txt @@ -688,6 +688,8 @@ if (BUILD_LAGOM) endif() # LibCore + lagom_test(../../Tests/LibCore/TestLibCoreArgsParser.cpp) + if ((LINUX OR APPLE) AND NOT EMSCRIPTEN) lagom_test(../../Tests/LibCore/TestLibCoreFileWatcher.cpp) lagom_test(../../Tests/LibCore/TestLibCorePromise.cpp LIBS LibThreading) diff --git a/Tests/AK/CMakeLists.txt b/Tests/AK/CMakeLists.txt index 5391c1dacd9..845415b7026 100644 --- a/Tests/AK/CMakeLists.txt +++ b/Tests/AK/CMakeLists.txt @@ -56,6 +56,7 @@ set(AK_TEST_SOURCES TestNonnullRefPtr.cpp TestNumberFormat.cpp TestOptional.cpp + TestOptionParser.cpp TestOwnPtr.cpp TestPrint.cpp TestQueue.cpp diff --git a/Tests/AK/TestOptionParser.cpp b/Tests/AK/TestOptionParser.cpp new file mode 100644 index 00000000000..663ab4bce70 --- /dev/null +++ b/Tests/AK/TestOptionParser.cpp @@ -0,0 +1,174 @@ +/* + * Copyright (c) 2021, the SerenityOS developers. + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +#include + +#include +#include +#include +#include + +TEST_CASE(string_option) +{ + ByteString short_options = ""; + int index_of_found_long_option = -1; + Vector long_options; + long_options.append( + { "string_opt"sv, + OptionParser::ArgumentRequirement::HasRequiredArgument, + &index_of_found_long_option, + 0 }); + + Array argument_array({ "app"sv, "--string_opt"sv, "string_opt_value"sv }); + Span arguments(argument_array); + size_t next_argument_index = 1; + + OptionParser parser; + auto result = parser.getopt(arguments.slice(1), short_options, long_options, {}); + + // found a long option + EXPECT_EQ(result.result, 0); + // found long option at index 0 + EXPECT_EQ(index_of_found_long_option, 0); + // 2 args consumed: option name and value + EXPECT_EQ(result.consumed_args, static_cast(2)); + // option has a value + EXPECT_EQ(result.optarg_value, "string_opt_value"); + + next_argument_index += result.consumed_args; + // we are past the end + EXPECT_EQ(next_argument_index, static_cast(3)); +} + +TEST_CASE(string_option_then_positional) +{ + ByteString short_options = ""; + int index_of_found_long_option = -1; + Vector long_options; + long_options.append( + { "string_opt"sv, + OptionParser::ArgumentRequirement::HasRequiredArgument, + &index_of_found_long_option, + 0 }); + + Array argument_array({ "app"sv, "--string_opt"sv, "string_opt_value"sv, "positional"sv }); + Span arguments(argument_array); + size_t next_argument_index = 1; + + OptionParser parser; + auto result = parser.getopt(arguments.slice(1), short_options, long_options, {}); + + // found a long option + EXPECT_EQ(result.result, 0); + // found long option at index 0 + EXPECT_EQ(index_of_found_long_option, 0); + // 2 args consumed: option name and value + EXPECT_EQ(result.consumed_args, static_cast(2)); + // option has a value + EXPECT_EQ(result.optarg_value, "string_opt_value"); + + next_argument_index += result.consumed_args; + // we are at "positional" index of arguments vector + EXPECT_EQ(next_argument_index, static_cast(3)); + EXPECT_EQ(arguments[next_argument_index], "positional"); + + result = parser.getopt(arguments.slice(1), short_options, long_options, {}); + // there's no more options + EXPECT_EQ(result.result, -1); +} + +TEST_CASE(positional_then_string_option) +{ + ByteString short_options = ""; + int index_of_found_long_option = -1; + Vector long_options; + long_options.append( + { "string_opt"sv, + OptionParser::ArgumentRequirement::HasRequiredArgument, + &index_of_found_long_option, + 0 }); + + Array argument_array({ "app"sv, "positional"sv, "--string_opt"sv, "string_opt_value"sv }); + Span arguments(argument_array); + size_t next_argument_index = 1; + + OptionParser parser; + auto result = parser.getopt(arguments.slice(1), short_options, long_options, {}); + + // found a long option + EXPECT_EQ(result.result, 0); + // found long option at index 0 + EXPECT_EQ(index_of_found_long_option, 0); + // 2 args consumed: option name and value + EXPECT_EQ(result.consumed_args, static_cast(2)); + // option has a value + EXPECT_EQ(result.optarg_value, "string_opt_value"); + + next_argument_index += result.consumed_args; + // we are at "positional" index of arguments vector + EXPECT_EQ(next_argument_index, static_cast(3)); + EXPECT_EQ(arguments[next_argument_index], "positional"); + + result = parser.getopt(arguments.slice(1), short_options, long_options, {}); + // there's no more options + EXPECT_EQ(result.result, -1); +} + +TEST_CASE(positional_then_string_option_then_bool_option) +{ + // #22759: Positional arguments were sometimes incorrectly not shifted, leading to an incorrect parse. + + ByteString short_options = ""; + int index_of_found_long_option = -1; + Vector long_options; + long_options.append( + { "string_opt"sv, + OptionParser::ArgumentRequirement::HasRequiredArgument, + &index_of_found_long_option, + 0 }); + long_options.append( + { "bool_opt"sv, + OptionParser::ArgumentRequirement::NoArgument, + &index_of_found_long_option, + 1 }); + + Array argument_array({ "app"sv, "positional"sv, "--string_opt"sv, "string_opt_value"sv, "--bool_opt"sv }); + Span arguments(argument_array); + size_t next_argument_index = 1; + + OptionParser parser; + auto result = parser.getopt(arguments.slice(1), short_options, long_options, {}); + // found a long option + EXPECT_EQ(result.result, 0); + // found long option at index 0 + EXPECT_EQ(index_of_found_long_option, 0); + // 2 args consumed: option name and value + EXPECT_EQ(result.consumed_args, static_cast(2)); + // option has a value + EXPECT_EQ(result.optarg_value, "string_opt_value"); + + next_argument_index += result.consumed_args; + EXPECT_EQ(next_argument_index, static_cast(3)); + // positional argument has been shifted here + EXPECT_EQ(arguments[next_argument_index], "positional"); + + result = parser.getopt(arguments.slice(1), short_options, long_options, {}); + // found another long option + EXPECT_EQ(result.result, 0); + // found long option at index 1 + EXPECT_EQ(index_of_found_long_option, 1); + // 1 arg consumed: option name + EXPECT_EQ(result.consumed_args, static_cast(1)); + + next_argument_index += result.consumed_args; + // "positional" argument has been shifted here + EXPECT_EQ(next_argument_index, static_cast(4)); + EXPECT_EQ(arguments[next_argument_index], "positional"); + + result = parser.getopt(arguments.slice(1), short_options, long_options, {}); + // there's no more options + EXPECT_EQ(result.result, -1); +} diff --git a/Tests/LibCore/TestLibCoreArgsParser.cpp b/Tests/LibCore/TestLibCoreArgsParser.cpp index c4bd89792cb..c9753c6d677 100644 --- a/Tests/LibCore/TestLibCoreArgsParser.cpp +++ b/Tests/LibCore/TestLibCoreArgsParser.cpp @@ -5,10 +5,10 @@ */ #include +#include #include #include #include -#include class ParserResult { public: @@ -112,6 +112,40 @@ TEST_CASE(bool_option) EXPECT_EQ(force, true); } +TEST_CASE(string_option) +{ + ByteString string_option; + + // short option + auto parser_result = run_parser({ "app"sv, "-d"sv, "foo"sv }, [&](auto& parser) { + parser.add_option(string_option, "dummy", nullptr, 'd', "DUMMY"); + }); + EXPECT_EQ(parser_result.result, true); + EXPECT_EQ(string_option, "foo"); + + // short option, not given + string_option = ""; + parser_result = run_parser({ "app"sv, "-d"sv }, [&](auto& parser) { + parser.add_option(string_option, "dummy", nullptr, 'd', "DUMMY"); + }); + EXPECT_EQ(parser_result.result, false); + + // long option + string_option = ""; + parser_result = run_parser({ "app"sv, "--dummy"sv, "foo"sv }, [&](auto& parser) { + parser.add_option(string_option, "dummy", "dummy", {}, "DUMMY"); + }); + EXPECT_EQ(parser_result.result, true); + EXPECT_EQ(string_option, "foo"); + + // long option, not given + string_option = ""; + parser_result = run_parser({ "app"sv, "--dummy"sv }, [&](auto& parser) { + parser.add_option(string_option, "dummy", "dummy", {}, "DUMMY"); + }); + EXPECT_EQ(parser_result.result, false); +} + TEST_CASE(positional_string_argument) { // Single required string argument @@ -316,6 +350,169 @@ TEST_CASE(combination_of_bool_options_with_positional_vector_string) EXPECT_EQ(parser_result.result, false); } +TEST_CASE(combination_of_bool_and_string_short_options_with_positional_vector_string) +{ + // #22759: Positional arguments were sometimes incorrectly not shifted, leading to an incorrect parse. + + // Bool options (given), string options (given) and one positional argument (given) + // Expected: all arguments fill as given + bool bool_opt1 = false; + bool bool_opt2 = false; + ByteString string_opt1; + ByteString string_opt2; + Vector positionals; + + auto parser_result = run_parser({ "app"sv, "-b"sv, "-c"sv, "-d"sv, "foo"sv, "-e"sv, "bar"sv, "one"sv }, [&](auto& parser) { + parser.add_option(bool_opt1, "bool_opt1", nullptr, 'b'); + parser.add_option(bool_opt2, "bool_opt2", nullptr, 'c'); + parser.add_option(string_opt1, "string_opt1", nullptr, 'd', "D"); + parser.add_option(string_opt2, "string_opt2", nullptr, 'e', "E"); + parser.add_positional_argument(positionals, "pos", "pos", Core::ArgsParser::Required::No); + }); + EXPECT_EQ(parser_result.result, true); + EXPECT_EQ(bool_opt1, true); + EXPECT_EQ(bool_opt2, true); + EXPECT_EQ(string_opt1, "foo"); + EXPECT_EQ(string_opt2, "bar"); + EXPECT_EQ(positionals.size(), 1u); + if (positionals.size() == 1u) { + EXPECT_EQ(positionals[0], "one"); + } + + // Bool options (given), string options (given) and one positional argument (given) + // one bool option after positional + // Expected: all arguments fill as given + bool_opt1 = false; + bool_opt2 = false; + string_opt1 = ""; + string_opt2 = ""; + positionals = {}; + + parser_result = run_parser({ "app"sv, "-c"sv, "-d"sv, "foo"sv, "-e"sv, "bar"sv, "one"sv, "-b"sv }, [&](auto& parser) { + parser.add_option(bool_opt1, "bool_opt1", nullptr, 'b'); + parser.add_option(bool_opt2, "bool_opt2", nullptr, 'c'); + parser.add_option(string_opt1, "string_opt1", nullptr, 'd', "D"); + parser.add_option(string_opt2, "string_opt2", nullptr, 'e', "E"); + parser.add_positional_argument(positionals, "pos", "pos", Core::ArgsParser::Required::No); + }); + EXPECT_EQ(parser_result.result, true); + EXPECT_EQ(bool_opt1, true); + EXPECT_EQ(bool_opt2, true); + EXPECT_EQ(string_opt1, "foo"); + EXPECT_EQ(string_opt2, "bar"); + EXPECT_EQ(positionals.size(), 1u); + if (positionals.size() == 1u) { + EXPECT_EQ(positionals[0], "one"); + } + + // Bool options (given), string options (given) and one positional argument (given) + // one string and one bool option after positional + // Expected: all arguments fill as given + bool_opt1 = false; + bool_opt2 = false; + string_opt1 = ""; + string_opt2 = ""; + positionals = {}; + + parser_result = run_parser({ "app"sv, "-c"sv, "-e"sv, "bar"sv, "one"sv, "-d"sv, "foo"sv, "-b"sv }, [&](auto& parser) { + parser.add_option(bool_opt1, "bool_opt1", nullptr, 'b'); + parser.add_option(bool_opt2, "bool_opt2", nullptr, 'c'); + parser.add_option(string_opt1, "string_opt1", nullptr, 'd', "D"); + parser.add_option(string_opt2, "string_opt2", nullptr, 'e', "E"); + parser.add_positional_argument(positionals, "pos", "pos", Core::ArgsParser::Required::No); + }); + EXPECT_EQ(parser_result.result, true); + EXPECT_EQ(bool_opt1, true); + EXPECT_EQ(bool_opt2, true); + EXPECT_EQ(string_opt1, "foo"); + EXPECT_EQ(string_opt2, "bar"); + EXPECT_EQ(positionals.size(), 1u); + if (positionals.size() == 1u) { + EXPECT_EQ(positionals[0], "one"); + } + + // Bool options (given), string options (given) and two positional arguments (given) + // positional arguments are separated by options + // Expected: all arguments fill as given + bool_opt1 = false; + bool_opt2 = false; + string_opt1 = ""; + string_opt2 = ""; + positionals = {}; + + parser_result = run_parser({ "app"sv, "-b"sv, "-d"sv, "foo"sv, "one"sv, "-c"sv, "-e"sv, "bar"sv, "two"sv }, [&](auto& parser) { + parser.add_option(bool_opt1, "bool_opt1", nullptr, 'b'); + parser.add_option(bool_opt2, "bool_opt2", nullptr, 'c'); + parser.add_option(string_opt1, "string_opt1", nullptr, 'd', "D"); + parser.add_option(string_opt2, "string_opt2", nullptr, 'e', "E"); + parser.add_positional_argument(positionals, "pos", "pos", Core::ArgsParser::Required::No); + }); + EXPECT_EQ(parser_result.result, true); + EXPECT_EQ(bool_opt1, true); + EXPECT_EQ(bool_opt2, true); + EXPECT_EQ(string_opt1, "foo"); + EXPECT_EQ(string_opt2, "bar"); + EXPECT_EQ(positionals.size(), 2u); + if (positionals.size() == 2u) { + EXPECT_EQ(positionals[0], "one"); + EXPECT_EQ(positionals[1], "two"); + } + + // Bool options (given), string options (given) and two positional arguments (given) + // positional arguments are separated and followed by options + // Expected: all arguments fill as given + bool_opt1 = false; + bool_opt2 = false; + string_opt1 = ""; + string_opt2 = ""; + positionals = {}; + + parser_result = run_parser({ "app"sv, "one"sv, "-b"sv, "-d"sv, "foo"sv, "two"sv, "-c"sv, "-e"sv, "bar"sv }, [&](auto& parser) { + parser.add_option(bool_opt1, "bool_opt1", nullptr, 'b'); + parser.add_option(bool_opt2, "bool_opt2", nullptr, 'c'); + parser.add_option(string_opt1, "string_opt1", nullptr, 'd', "D"); + parser.add_option(string_opt2, "string_opt2", nullptr, 'e', "E"); + parser.add_positional_argument(positionals, "pos", "pos", Core::ArgsParser::Required::No); + }); + EXPECT_EQ(parser_result.result, true); + EXPECT_EQ(bool_opt1, true); + EXPECT_EQ(bool_opt2, true); + EXPECT_EQ(string_opt1, "foo"); + EXPECT_EQ(string_opt2, "bar"); + EXPECT_EQ(positionals.size(), 2u); + if (positionals.size() == 2u) { + EXPECT_EQ(positionals[0], "one"); + EXPECT_EQ(positionals[1], "two"); + } + + // Bool options (given), string options (given) and two positional arguments (given) + // positional arguments are separated and followed by options, variation on options order + // Expected: all arguments fill as given + bool_opt1 = false; + bool_opt2 = false; + string_opt1 = ""; + string_opt2 = ""; + positionals = {}; + + parser_result = run_parser({ "app"sv, "one"sv, "-d"sv, "foo"sv, "-b"sv, "two"sv, "-e"sv, "bar"sv, "-c"sv }, [&](auto& parser) { + parser.add_option(bool_opt1, "bool_opt1", nullptr, 'b'); + parser.add_option(bool_opt2, "bool_opt2", nullptr, 'c'); + parser.add_option(string_opt1, "string_opt1", nullptr, 'd', "D"); + parser.add_option(string_opt2, "string_opt2", nullptr, 'e', "E"); + parser.add_positional_argument(positionals, "pos", "pos", Core::ArgsParser::Required::No); + }); + EXPECT_EQ(parser_result.result, true); + EXPECT_EQ(bool_opt1, true); + EXPECT_EQ(bool_opt2, true); + EXPECT_EQ(string_opt1, "foo"); + EXPECT_EQ(string_opt2, "bar"); + EXPECT_EQ(positionals.size(), 2u); + if (positionals.size() == 2u) { + EXPECT_EQ(positionals[0], "one"); + EXPECT_EQ(positionals[1], "two"); + } +} + TEST_CASE(stop_on_first_non_option) { // Do not stop on first non-option; arguments in correct order