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

@@ -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()

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)

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

Tests/AK/TestOptionParser.cpp

@@ -0,0 +1,174 @@
+/*
+ * Copyright (c) 2021, the SerenityOS developers.
+ *
+ * SPDX-License-Identifier: BSD-2-Clause
+ */
+
+#include <LibTest/TestCase.h>
+
+#include <AK/Array.h>
+#include <AK/OptionParser.h>
+#include <AK/String.h>
+#include <AK/Vector.h>
+
+TEST_CASE(string_option)
+{
+    ByteString short_options = "";
+    int index_of_found_long_option = -1;
+    Vector<OptionParser::Option> long_options;
+    long_options.append(
+        { "string_opt"sv,
+            OptionParser::ArgumentRequirement::HasRequiredArgument,
+            &index_of_found_long_option,
+            0 });
+
+    Array<StringView, 3> argument_array({ "app"sv, "--string_opt"sv, "string_opt_value"sv });
+    Span<StringView> 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<size_t>(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<size_t>(3));
+}
+
+TEST_CASE(string_option_then_positional)
+{
+    ByteString short_options = "";
+    int index_of_found_long_option = -1;
+    Vector<OptionParser::Option> long_options;
+    long_options.append(
+        { "string_opt"sv,
+            OptionParser::ArgumentRequirement::HasRequiredArgument,
+            &index_of_found_long_option,
+            0 });
+
+    Array<StringView, 4> argument_array({ "app"sv, "--string_opt"sv, "string_opt_value"sv, "positional"sv });
+    Span<StringView> 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<size_t>(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<size_t>(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<OptionParser::Option> long_options;
+    long_options.append(
+        { "string_opt"sv,
+            OptionParser::ArgumentRequirement::HasRequiredArgument,
+            &index_of_found_long_option,
+            0 });
+
+    Array<StringView, 4> argument_array({ "app"sv, "positional"sv, "--string_opt"sv, "string_opt_value"sv });
+    Span<StringView> 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<size_t>(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<size_t>(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<OptionParser::Option> 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<StringView, 5> argument_array({ "app"sv, "positional"sv, "--string_opt"sv, "string_opt_value"sv, "--bool_opt"sv });
+    Span<StringView> 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<size_t>(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<size_t>(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<size_t>(1));
+
+    next_argument_index += result.consumed_args;
+    // "positional" argument has been shifted here
+    EXPECT_EQ(next_argument_index, static_cast<size_t>(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);
+}

Tests/LibCore/TestLibCoreArgsParser.cpp

@@ -5,10 +5,10 @@
  */
 
 #include <AK/Function.h>
+#include <AK/String.h>
 #include <AK/Vector.h>
 #include <LibCore/ArgsParser.h>
 #include <LibTest/TestCase.h>
-#include <string.h>
 
 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<StringView> 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