[main] optparse: don't treat = sign as separator for prefix flags

Author: silverweed

main/src/optparse.hxx

@@ -70,8 +70,9 @@
 /// ~~~
 ///
 /// (see EFlagTreatment for more details). This will **disable** flag grouping globally, but allows the parser to
-/// interpret flags and arguments that are not separated by spaces.
-/// Note that this only makes sense for flags with arguments.
+/// interpret flags and arguments that are not separated by spaces. If prefix arg is used, the equal sign will *not*
+/// be considered as the key-value separator (`-Dfoo=bar` if `-D` is a prefix arg flag will be parsed as `"-D",
+/// "foo=bar"`). Note that this option only makes sense for flags with arguments.
 ///
 /// \author Giacomo Parolini <giacomo.parolini@cern.ch>
 /// \date 2025-10-09
@@ -218,6 +219,10 @@ public:
          throw std::invalid_argument("Flag `" + std::string(*aliases.begin()) +
                                      "` has option kFlagPrefixArg but it's a Switch, so the option makes no sense.");
 
+      std::vector<RExpectedFlag> flagsToBeAdded;
+      flagsToBeAdded.reserve(aliases.size());
+
+      const auto nCurrentFlags = fExpectedFlags.size();
       int aliasIdx = -1;
       for (auto f : aliases) {
          auto prefixLen = f.find_first_not_of('-');
@@ -229,13 +234,17 @@ public:
 
          auto flagName = f.substr(prefixLen);
 
-         // Check that we're not introducing ambiguities with prefix flags. While we're at it, also check that none
-         // of the given aliases were already added.
+         // Check that we're not introducing ambiguities with prefix flags.
+         // While we're at it, also check that none of the given aliases were already added. Note that it is valid to
+         // add the same flag name as both a long and short flag (like "-foo" and "--foo") but only if they are aliases
+         // of the same flag.
          for (const auto &expFlag : fExpectedFlags) {
-            // NOTE: we're checking against the full string, not just the flag name, to allow cases like:
-            // AddFlag({"-foo", "--foo"}).
-            if (expFlag.AsStr() == f)
-               throw std::invalid_argument("Flag `" + expFlag.AsStr() + "` was added multiple times.");
+            if (expFlag.fName == flagName) {
+               throw std::invalid_argument(
+                  "Flag `" + std::string(flagName) +
+                  "` was added multiple times. Note that adding flags with the same name but different number of `-` "
+                  "can only be done as aliases of the same call to AddFlag().");
+            }
 
             if (!(flagOpts & kFlagPrefixArg) && !(expFlag.fOpts & kFlagPrefixArg))
                continue;
@@ -249,6 +258,15 @@ public:
             }
          }
 
+         // Check that the user didn't pass the very same flag multiple times in the same AddFlag invocation, as that's
+         // likely a mistake. This is different from the check done in the previous loop as that one only concerns
+         // flags that were already added before this AddFlag invocation.
+         for (const auto &expFlag : flagsToBeAdded) {
+            if (expFlag.AsStr() == f)
+               throw std::invalid_argument("The same flag `" + expFlag.AsStr() +
+                                           "` was passed multiple times to AddFlag().");
+         }
+
          bool disallowsGrouping = (prefixLen == 1 && (f.size() > 2 || (flagOpts & kFlagPrefixArg)));
          if (disallowsGrouping) {
             if (fSettings.fFlagTreatment == EFlagTreatment::kDefault) {
@@ -268,10 +286,12 @@ public:
          expected.fAlias = aliasIdx;
          expected.fShort = prefixLen == 1;
          expected.fOpts = flagOpts;
-         fExpectedFlags.push_back(expected);
+         flagsToBeAdded.push_back(expected);
          if (aliasIdx < 0)
-            aliasIdx = fExpectedFlags.size() - 1;
+            aliasIdx = nCurrentFlags + flagsToBeAdded.size() - 1;
       }
+
+      fExpectedFlags.insert(fExpectedFlags.end(), flagsToBeAdded.begin(), flagsToBeAdded.end());
    }
 
    /// If `name` refers to a previously-defined switch (i.e. a boolean flag), returns how many times
@@ -428,18 +448,35 @@ public:
          if (!isFlag) {
             // positional argument
             fArgs.push_back(arg);
-         } else {
+            continue;
+         }
+
+         ++arg; // eat first `-`.
+
+         // Parse long or short flag and its argument into `argStr` / `nxtArgStr`.
+         // Note that `argStr` may contain multiple flags in case of grouped short flags (in which case nxtArgStr
+         // refers only to the last one).
+         argStr.clear();
+         std::string_view nxtArgStr;
+         // If this is false `nxtArgStr` *must* refer to the next arg, otherwise it might or might not be.
+         bool nxtArgIsTentative = true;
+         if (arg[0] == '-') {
+            // long flag
             ++arg;
-            // Parse long or short flag and its argument into `argStr` / `nxtArgStr`.
-            // Note that `argStr` may contain multiple flags in case of grouped short flags (in which case nxtArgStr
-            // refers only to the last one).
-            argStr.clear();
-            std::string_view nxtArgStr;
-            // If this is false `nxtArgStr` *must* refer to the next arg, otherwise it might or might not be.
-            bool nxtArgIsTentative = true;
-            if (arg[0] == '-') {
-               // long flag
-               ++arg;
+
+            // if we have prefix flags we need to check them first, as in that case the `=` sign should not be treated
+            // as the key-value separator.
+            const auto argLen = strlen(arg);
+            for (const auto &flag : fExpectedFlags) {
+               if ((flag.fOpts & kFlagPrefixArg) && flag.fName.size() < argLen &&
+                   strncmp(arg, flag.fName.c_str(), flag.fName.size()) == 0) {
+                  argStr.push_back(std::string_view(arg, flag.fName.size()));
+                  nxtArgStr = std::string_view(arg + flag.fName.size(), argLen - flag.fName.size());
+                  break;
+               }
+            }
+
+            if (argStr.empty()) {
                const char *eq = strchr(arg, '=');
                if (eq) {
                   argStr.push_back(std::string_view(arg, eq - arg));
@@ -452,100 +489,102 @@ public:
                      ++i;
                   }
                }
-            } else {
-               // short flag.
-               // If flag grouping is active, all flags except the last one will have an implicitly empty argument.
-               auto argLen = strlen(arg);
-               while (fSettings.fFlagTreatment == EFlagTreatment::kGrouped && argLen > 1) {
-                  argStr.push_back(std::string_view{arg, 1});
-                  ++arg, --argLen;
-               }
+            }
+         } else {
+            // short flag.
+            // If flag grouping is active, all flags except the last one will have an implicitly empty argument.
+            auto argLen = strlen(arg);
+            while (fSettings.fFlagTreatment == EFlagTreatment::kGrouped && argLen > 1) {
+               argStr.push_back(std::string_view{arg, 1});
+               ++arg, --argLen;
+            }
 
-               argStr.push_back(std::string_view(arg));
-               if (i < nArgs - 1 && args[i + 1][0] != '-') {
-                  nxtArgStr = args[i + 1];
-                  ++i;
-               }
+            argStr.push_back(std::string_view(arg));
+            if (i < nArgs - 1 && args[i + 1][0] != '-') {
+               nxtArgStr = args[i + 1];
+               ++i;
             }
+         }
 
-            for (auto j = 0u; j < argStr.size(); ++j) {
-               std::string_view argS = argStr[j];
+         assert(argStr.size() < 2 || fSettings.fFlagTreatment == EFlagTreatment::kGrouped);
 
-               const auto *exp = GetExpectedFlag(argS);
-               if (!exp) {
+         for (auto j = 0u; j < argStr.size(); ++j) {
+            std::string_view argS = argStr[j];
+
+            const auto *exp = GetExpectedFlag(argS);
+            if (!exp) {
+               fErrors.push_back(std::string("Unknown flag: ") + argOrig);
+               break;
+            }
+
+            // In Prefix mode, check if the returned expected flag is shorter than `argS`. This can mean two things:
+            // - if `nxtArgIsTentative == false` then this flag was followed by an equal sign, and in that case
+            //   the intention is interpreted as "I want this flag's argument to be whatever follows the equal sign",
+            //   which means we treat this as an unknown flag;
+            // - otherwise, we use the rest of `argS` as the argument to the flag.
+            // More concretely: if the user added flag "-D" and argS is "-Dfoo=bar", we parse it as
+            // {flag: "-Dfoo", arg: "bar"}, rather than {flag: "-D", arg: "foo=bar"}.
+            if ((exp->fOpts & kFlagPrefixArg) && argS.size() > exp->fName.size()) {
+               if (nxtArgIsTentative) {
+                  i -= !nxtArgStr.empty(); // if we had already picked a candidate next arg, undo that.
+                  nxtArgStr = argS.substr(exp->fName.size());
+                  nxtArgIsTentative = false;
+               } else {
                   fErrors.push_back(std::string("Unknown flag: ") + argOrig);
                   break;
                }
+            } else {
+               assert(exp->fName.size() == argS.size());
+            }
 
-               // In Prefix mode, check if the returned expected flag is shorter than `argS`. This can mean two things:
-               // - if `nxtArgIsTentative == false` then this flag was followed by an equal sign, and in that case
-               //   the intention is interpreted as "I want this flag's argument to be whatever follows the equal sign",
-               //   which means we treat this as an unknown flag;
-               // - otherwise, we use the rest of `argS` as the argument to the flag.
-               // More concretely: if the user added flag "-D" and argS is "-Dfoo=bar", we parse it as
-               // {flag: "-Dfoo", arg: "bar"}, rather than {flag: "-D", arg: "foo=bar"}.
-               if ((exp->fOpts & kFlagPrefixArg) && argS.size() > exp->fName.size()) {
-                  if (nxtArgIsTentative) {
-                     i -= !nxtArgStr.empty(); // if we had already picked a candidate next arg, undo that.
-                     nxtArgStr = argS.substr(exp->fName.size());
-                     nxtArgIsTentative = false;
-                  } else {
-                     fErrors.push_back(std::string("Unknown flag: ") + argOrig);
-                     break;
-                  }
-               } else {
-                  assert(exp->fName.size() == argS.size());
-               }
+            std::string_view nxtArg = (j == argStr.size() - 1) ? nxtArgStr : "";
 
-               std::string_view nxtArg = (j == argStr.size() - 1) ? nxtArgStr : "";
-
-               RCmdLineOpts::RFlag flag;
-               flag.fHelp = exp->fHelp;
-               // If the flag is an alias (e.g. long version of a short one), save its name as the aliased one, so we
-               // can fetch the value later by using any of the aliases.
-               if (exp->fAlias < 0)
-                  flag.fName = exp->fName;
-               else
-                  flag.fName = fExpectedFlags[exp->fAlias].fName;
-
-               // Check for duplicate flags
-               if (!(exp->fOpts & kFlagAllowMultiple)) {
-                  auto existingIt = std::find_if(fFlags.begin(), fFlags.end(),
-                                                 [&flag](const auto &f) { return f.fName == flag.fName; });
-                  if (existingIt != fFlags.end()) {
-                     std::string err = std::string("Flag ") + exp->AsStr() + " appeared more than once";
-                     if (exp->fFlagType == RCmdLineOpts::EFlagType::kWithArg)
-                        err += " with the value: " + existingIt->fValue;
-                     fErrors.push_back(err);
-                     break;
-                  }
+            RCmdLineOpts::RFlag flag;
+            flag.fHelp = exp->fHelp;
+            // If the flag is an alias (e.g. long version of a short one), save its name as the aliased one, so we
+            // can fetch the value later by using any of the aliases.
+            if (exp->fAlias < 0)
+               flag.fName = exp->fName;
+            else
+               flag.fName = fExpectedFlags[exp->fAlias].fName;
+
+            // Check for duplicate flags
+            if (!(exp->fOpts & kFlagAllowMultiple)) {
+               auto existingIt =
+                  std::find_if(fFlags.begin(), fFlags.end(), [&flag](const auto &f) { return f.fName == flag.fName; });
+               if (existingIt != fFlags.end()) {
+                  std::string err = std::string("Flag ") + exp->AsStr() + " appeared more than once";
+                  if (exp->fFlagType == RCmdLineOpts::EFlagType::kWithArg)
+                     err += " with the value: " + existingIt->fValue;
+                  fErrors.push_back(err);
+                  break;
                }
+            }
 
-               // Check that arguments are what we expect.
-               if (exp->fFlagType == RCmdLineOpts::EFlagType::kWithArg) {
-                  if (!nxtArg.empty()) {
-                     flag.fValue = nxtArg;
-                  } else {
-                     fErrors.push_back("Missing argument for flag " + exp->AsStr());
-                  }
+            // Check that arguments are what we expect.
+            if (exp->fFlagType == RCmdLineOpts::EFlagType::kWithArg) {
+               if (!nxtArg.empty()) {
+                  flag.fValue = nxtArg;
                } else {
-                  if (!nxtArg.empty()) {
-                     if (nxtArgIsTentative)
-                        --i;
-                     else
-                        fErrors.push_back("Flag " + exp->AsStr() + " does not expect an argument");
-                  }
+                  fErrors.push_back("Missing argument for flag " + exp->AsStr());
+               }
+            } else {
+               if (!nxtArg.empty()) {
+                  if (nxtArgIsTentative)
+                     --i;
+                  else
+                     fErrors.push_back("Flag " + exp->AsStr() + " does not expect an argument");
                }
-
-               if (!fErrors.empty())
-                  break;
-
-               fFlags.push_back(flag);
             }
 
             if (!fErrors.empty())
                break;
+
+            fFlags.push_back(flag);
          }
+
+         if (!fErrors.empty())
+            break;
       }
    }
 };

main/test/optparse_test.cxx

@@ -664,7 +664,7 @@ TEST(OptParse, PrefixMultiple)
    opts.Parse(args, std::size(args));
 
    EXPECT_TRUE(opts.GetErrors().empty());
-   EXPECT_EQ(opts.GetFlagValues("D"), std::vector<std::string_view>({"name", "name", "name"}));
+   EXPECT_EQ(opts.GetFlagValues("D"), std::vector<std::string_view>({"name", "=name", "name"}));
 }
 
 TEST(OptParse, PrefixWithEqual)
@@ -673,11 +673,24 @@ TEST(OptParse, PrefixWithEqual)
    opts.AddFlag({"--D"}, ROOT::RCmdLineOpts::EFlagType::kWithArg, "",
                 ROOT::RCmdLineOpts::kFlagAllowMultiple | ROOT::RCmdLineOpts::kFlagPrefixArg);
 
-   const char *args[] = {"somename", "--Df=a"};
+   const char *args[] = {"somename", "--Df=a", "--D", "f=a"};
    opts.Parse(args, std::size(args));
 
-   EXPECT_EQ(opts.GetErrors().size(), 1);
-   EXPECT_TRUE(opts.GetFlagValues("D").empty());
+   EXPECT_TRUE(opts.GetErrors().empty());
+   EXPECT_EQ(opts.GetFlagValues("D"), std::vector<std::string_view>({"f=a", "f=a"}));
+}
+
+TEST(OptParse, PrefixShortMulticharacter)
+{
+   ROOT::RCmdLineOpts opts;
+   opts.AddFlag({"-includeI"}, ROOT::RCmdLineOpts::EFlagType::kWithArg, "",
+                ROOT::RCmdLineOpts::kFlagAllowMultiple | ROOT::RCmdLineOpts::kFlagPrefixArg);
+
+   const char *args[] = {"somename", "-includeI/foo/bar", "-includeI", "/foo/bar"};
+   opts.Parse(args, std::size(args));
+
+   EXPECT_TRUE(opts.GetErrors().empty());
+   EXPECT_EQ(opts.GetFlagValues("includeI"), std::vector<std::string_view>({"/foo/bar", "/foo/bar"}));
 }
 
 TEST(OptParse, PrefixDisablesGrouping)
@@ -724,6 +737,37 @@ TEST(OptParse, DuplicateFlag)
       opts.AddFlag({"-a"}, ROOT::RCmdLineOpts::EFlagType::kSwitch);
       FAIL() << "adding the same flag twice should fail";
    } catch (const std::invalid_argument &ex) {
-      EXPECT_STREQ(ex.what(), "Flag `-a` was added multiple times.");
+      EXPECT_STREQ(ex.what(), "Flag `a` was added multiple times. Note that adding flags with the same name but "
+                              "different number of `-` can only be done as aliases of the same call to AddFlag().");
+   }
+}
+
+TEST(OptParse, DuplicateFlagLongShort)
+{
+   ROOT::RCmdLineOpts opts;
+   opts.AddFlag({"-a"}, ROOT::RCmdLineOpts::EFlagType::kSwitch);
+   try {
+      opts.AddFlag({"--a"}, ROOT::RCmdLineOpts::EFlagType::kSwitch);
+      FAIL() << "adding the same flag twice should fail";
+   } catch (const std::invalid_argument &ex) {
+      EXPECT_STREQ(ex.what(), "Flag `a` was added multiple times. Note that adding flags with the same name but "
+                              "different number of `-` can only be done as aliases of the same call to AddFlag().");
+   }
+}
+
+TEST(OptParse, DuplicateFlagLongShortOk)
+{
+   ROOT::RCmdLineOpts opts;
+   opts.AddFlag({"-a", "--a"}, ROOT::RCmdLineOpts::EFlagType::kSwitch);
+}
+
+TEST(OptParse, DuplicateFlagAlias)
+{
+   ROOT::RCmdLineOpts opts;
+   try {
+      opts.AddFlag({"--a", "--a"}, ROOT::RCmdLineOpts::EFlagType::kSwitch);
+      FAIL() << "adding the same flag twice should fail";
+   } catch (const std::invalid_argument &ex) {
+      EXPECT_STREQ(ex.what(), "The same flag `--a` was passed multiple times to AddFlag().");
    }
 }