cmArgumentParser: Improve bad argument handling

Tweak ArgumentParser::ParseResult to store errors as a set, rather than
concatenating them. Add a new method that a) also optionally checks for
unknown arguments, and b) reports errors using the `SetError` method of
`cmExecutionStatus`, which allows callers to `return false`, which is
less surprising when an error occurs. This improves consistency at call
sites, reduces duplication by moving the common task of complaining
about unknown arguments to a reusable method, and also produces somewhat
more concise messages in the case that multiple errors occurred.

Note that, for some reason, the parser is sometimes generating duplicate
errors, hence the use of a set rather than a list.

Source/cmArgumentParser.cxx

@@ -5,6 +5,7 @@
 #include <algorithm>
 
 #include "cmArgumentParserTypes.h"
+#include "cmExecutionStatus.h"
 #include "cmMakefile.h"
 #include "cmMessageType.h"
 #include "cmStringAlgorithms.h"
@@ -214,11 +215,41 @@ bool ParseResult::MaybeReportError(cmMakefile& mf) const
     return false;
   }
   std::string e;
-  for (auto const& ke : this->KeywordErrors) {
-    e = cmStrCat(e, "Error after keyword \"", ke.first, "\":\n", ke.second);
+  for (auto const& kel : this->KeywordErrors) {
+    e = cmStrCat(e, "Error after keyword \"", kel.first, "\":\n");
+    for (auto const& ke : kel.second) {
+      e += ke;
+    }
   }
   mf.IssueMessage(MessageType::FATAL_ERROR, e);
   return true;
 }
 
+bool ParseResult::Check(cm::string_view context,
+                        std::vector<std::string> const* unparsedArguments,
+                        cmExecutionStatus& status) const
+{
+  if (unparsedArguments && !unparsedArguments->empty()) {
+    status.SetError(cmStrCat(context, " given unknown argument: \""_s,
+                             unparsedArguments->front(), "\"."_s));
+    return false;
+  }
+
+  if (!this->KeywordErrors.empty()) {
+    std::string msg = cmStrCat(
+      context, (context.empty() ? ""_s : " "_s), "given invalid "_s,
+      (this->KeywordErrors.size() > 1 ? "arguments:"_s : "argument:"_s));
+    for (auto const& kel : this->KeywordErrors) {
+      for (auto const& ke : kel.second) {
+        msg =
+          cmStrCat(msg, "\n  "_s, kel.first, ": "_s, cmStripWhitespace(ke));
+      }
+    }
+    status.SetError(msg);
+    return false;
+  }
+
+  return true;
+}
+
 } // namespace ArgumentParser

Source/cmArgumentParser.h

@@ -8,6 +8,7 @@
 #include <cstddef>
 #include <functional>
 #include <map>
+#include <set>
 #include <string>
 #include <utility>
 #include <vector>
@@ -23,13 +24,17 @@
 template <typename Result>
 class cmArgumentParser; // IWYU pragma: keep
 
+class cmExecutionStatus;
 class cmMakefile;
 
 namespace ArgumentParser {
 
 class ParseResult
 {
-  std::map<cm::string_view, std::string> KeywordErrors;
+  using KeywordErrorList = std::set<std::string>;
+  using KeywordErrorMap = std::map<cm::string_view, KeywordErrorList>;
+
+  KeywordErrorMap KeywordErrors;
 
 public:
   explicit operator bool() const { return this->KeywordErrors.empty(); }
@@ -37,15 +42,21 @@ public:
   void AddKeywordError(cm::string_view key, cm::string_view text)
 
   {
-    this->KeywordErrors[key] += text;
+    this->KeywordErrors[key].emplace(text);
   }
 
-  std::map<cm::string_view, std::string> const& GetKeywordErrors() const
+  KeywordErrorMap const& GetKeywordErrors() const
   {
     return this->KeywordErrors;
   }
 
   bool MaybeReportError(cmMakefile& mf) const;
+
+  /// Check if argument parsing succeeded. Return \c false and set an error if
+  /// any errors were encountered, or if \p unparsedArguments is non-empty.
+  bool Check(cm::string_view context,
+             std::vector<std::string> const* unparsedArguments,
+             cmExecutionStatus& status) const;
 };
 
 template <typename Result>

Tests/CMakeLib/testArgumentParser.cxx

@@ -2,6 +2,7 @@
    file LICENSE.rst or https://cmake.org/licensing for details.  */
 
 #include <map>
+#include <set>
 #include <string>
 #include <utility>
 #include <vector>
@@ -294,7 +295,8 @@ bool verifyResult(Result const& result,
   for (auto const& ke : result.GetKeywordErrors()) {
     auto const ki = keywordErrors.find(ke.first);
     ASSERT_TRUE(ki != keywordErrors.end());
-    ASSERT_TRUE(ke.second == ki->second);
+    ASSERT_TRUE(ke.second.size() == 1);
+    ASSERT_TRUE(*ke.second.begin() == ki->second);
   }
 
   return true;