cmake_parse_arguments: Restore capture of value after repeated keyword

When a single-value keyword is repeated, and the first instance is
missing a value, it prevents the value from the second instance from
being stored in a variable. This was a regression introduced by
commit ceeea4e511 (cmake_parse_arguments: Set variable if empty string
given after keyword, 2024-08-18). That change also didn't create a
variable if the keyword was given but without a value. The purpose
of the change was to always define a variable if a keyword was given.
Lastly, that change didn't protect the CMP0174 logic to make it only
apply to the PARSE_ARGV form.

The first two of the above problems are fixed here by tracking the
keywords given instead of checking which keywords were missing
values. The third problem is also fixed here, being tightly coupled
to the same logic as the first two problems.

Fixes: #26397

Help/policy/CMP0174.rst

@@ -10,21 +10,25 @@ One of the main reasons for using the ``PARSE_ARGV`` form of the
 :command:`cmake_parse_arguments` command is to more robustly handle corner
 cases related to empty values.  The non-``PARSE_ARGV`` form doesn't preserve
 empty arguments, but the ``PARSE_ARGV`` form does.  For each single-value
-keyword given, a variable should be defined if the keyword is followed by a
-value.  Prior to CMake 3.31, no variable would be defined if the value given
-was an empty string.  This meant the code could not detect the difference
-between the keyword not being given, and it being given but with an empty
-value, except by iterating over all the arguments and checking if the keyword
-is present.
+keyword given, a variable should be defined if the keyword is present, even
+if it is followed by an empty string.
+
+Prior to CMake 3.31, no variable would be defined if the value given after a
+single-value keyword was an empty string.  This meant the code could not detect
+the difference between the keyword not being given, and it being given but with
+an empty value, except by iterating over all the arguments and checking if the
+keyword is present.
 
 For the ``OLD`` behavior of this policy,
 :command:`cmake_parse_arguments(PARSE_ARGV)` does not define a variable for a
-single-value keyword followed by an empty string.
+single-value keyword followed by an empty string, or followed by no value at
+all.
+
 For the ``NEW`` behavior, :command:`cmake_parse_arguments(PARSE_ARGV)` always
 defines a variable for each keyword given in the arguments, even a single-value
-keyword with an empty string as its value.  With the ``NEW`` behavior, the
-code can robustly check if a single-value keyword was given with any value
-using just ``if(DEFINED <prefix>_<keyword>)``.
+keyword with an empty string as its value or no value at all.  With the
+``NEW`` behavior, the code can robustly check if a single-value keyword was
+given using just ``if(DEFINED <prefix>_<keyword>)``.
 
 .. |INTRODUCED_IN_CMAKE_VERSION| replace:: 3.31
 .. |WARNS_OR_DOES_NOT_WARN| replace:: warns

Source/cmParseArgumentsCommand.cxx

@@ -6,7 +6,6 @@
 #include <set>
 #include <utility>
 
-#include <cm/optional>
 #include <cm/string_view>
 
 #include "cmArgumentParser.h"
@@ -43,7 +42,7 @@ std::string JoinList(std::vector<std::string> const& arg, bool escape)
 }
 
 using options_map = std::map<std::string, bool>;
-using single_map = std::map<std::string, cm::optional<std::string>>;
+using single_map = std::map<std::string, std::string>;
 using multi_map =
   std::map<std::string, ArgumentParser::NonEmpty<std::vector<std::string>>>;
 using options_set = std::set<cm::string_view>;
@@ -76,7 +75,7 @@ struct UserArgumentParser : public cmArgumentParser<void>
 static void PassParsedArguments(
   const std::string& prefix, cmMakefile& makefile, const options_map& options,
   const single_map& singleValArgs, const multi_map& multiValArgs,
-  const std::vector<std::string>& unparsed,
+  const std::vector<std::string>& unparsed, const options_set& keywordsSeen,
   const options_set& keywordsMissingValues, bool parseFromArgV)
 {
   for (auto const& iter : options) {
@@ -87,29 +86,26 @@ static void PassParsedArguments(
   const cmPolicies::PolicyStatus cmp0174 =
     makefile.GetPolicyStatus(cmPolicies::CMP0174);
   for (auto const& iter : singleValArgs) {
-    if (iter.second.has_value()) {
-      // So far, we only know that the keyword was given, not whether a value
-      // was provided after the keyword
-      if (keywordsMissingValues.find(iter.first) ==
-          keywordsMissingValues.end()) {
-        // A (possibly empty) value was given
-        if (cmp0174 == cmPolicies::NEW || !iter.second->empty()) {
-          makefile.AddDefinition(cmStrCat(prefix, iter.first), *iter.second);
-          continue;
-        }
-        if (cmp0174 == cmPolicies::WARN) {
-          makefile.IssueMessage(
-            MessageType::AUTHOR_WARNING,
-            cmStrCat("An empty string was given as the value after the ",
-                     iter.first,
-                     " keyword. Policy CMP0174 is not set, so "
-                     "cmake_parse_arguments() will unset the ",
-                     prefix, iter.first,
-                     " variable rather than setting it to an empty string."));
-        }
+    if (keywordsSeen.find(iter.first) == keywordsSeen.end()) {
+      makefile.RemoveDefinition(cmStrCat(prefix, iter.first));
+    } else if ((parseFromArgV && cmp0174 == cmPolicies::NEW) ||
+               !iter.second.empty()) {
+      makefile.AddDefinition(cmStrCat(prefix, iter.first), iter.second);
+    } else {
+      // The OLD policy behavior doesn't define a variable for an empty or
+      // missing value, and we can't differentiate between those two cases.
+      if (parseFromArgV && (cmp0174 == cmPolicies::WARN)) {
+        makefile.IssueMessage(
+          MessageType::AUTHOR_WARNING,
+          cmStrCat("The ", iter.first,
+                   " keyword was followed by an empty string or no value at "
+                   "all. Policy CMP0174 is not set, so "
+                   "cmake_parse_arguments() will unset the ",
+                   prefix, iter.first,
+                   " variable rather than setting it to an empty string."));
       }
+      makefile.RemoveDefinition(cmStrCat(prefix, iter.first));
     }
-    makefile.RemoveDefinition(prefix + iter.first);
   }
 
   for (auto const& iter : multiValArgs) {
@@ -237,6 +233,14 @@ bool cmParseArgumentsCommand(std::vector<std::string> const& args,
     }
   }
 
+  std::vector<cm::string_view> keywordsSeen;
+  parser.BindParsedKeywords(keywordsSeen);
+
+  // For single-value keywords, only the last instance matters, since it
+  // determines the value stored. But if a keyword is repeated, it will be
+  // added to this vector if _any_ instance is missing a value. If one of the
+  // earlier instances is missing a value but the last one isn't, its presence
+  // in this vector will be misleading.
   std::vector<cm::string_view> keywordsMissingValues;
   parser.BindKeywordsMissingValue(keywordsMissingValues);
 
@@ -244,7 +248,7 @@ bool cmParseArgumentsCommand(std::vector<std::string> const& args,
 
   PassParsedArguments(
     prefix, status.GetMakefile(), options, singleValArgs, multiValArgs,
-    unparsed,
+    unparsed, options_set(keywordsSeen.begin(), keywordsSeen.end()),
     options_set(keywordsMissingValues.begin(), keywordsMissingValues.end()),
     parseFromArgV);
 

Tests/RunCMake/cmake_parse_arguments/CornerCasesArgvN-stderr.txt

@@ -2,7 +2,7 @@ Testing CMP0174 = NEW
 Testing CMP0174 = OLD
 Testing CMP0174 = WARN
 CMake Warning \(dev\) at CornerCasesArgvN\.cmake:[0-9]+ \(cmake_parse_arguments\):
-  An empty string was given as the value after the P1 keyword\.  Policy
+  The P1 keyword was followed by an empty string or no value at all\.  Policy
   CMP0174 is not set, so cmake_parse_arguments\(\) will unset the arg_P1
   variable rather than setting it to an empty string\.
 Call Stack \(most recent call first\):

Tests/RunCMake/cmake_parse_arguments/CornerCasesArgvN.cmake

@@ -60,7 +60,7 @@ block(SCOPE_FOR POLICIES)
     cmake_parse_arguments(PARSE_ARGV 0 arg "" "P1;P2" "")
     TEST(arg_KEYWORDS_MISSING_VALUES "P2")
     TEST(arg_P1 "")
-    TEST(arg_P2 "UNDEFINED")
+    TEST(arg_P2 "")
   endfunction()
   test_cmp0174_new_missing(P1 "" P2)
 
@@ -71,28 +71,56 @@ block(SCOPE_FOR POLICIES)
     TEST(arg_P2 "UNDEFINED")
   endfunction()
   test_cmp0174_new_no_missing(P1 "")
+
+  # For repeated keywords, the keyword will be reported as missing a value if
+  # any one of them is missing a value.
+  function(test_cmp0174_new_repeated_arg)
+    cmake_parse_arguments(PARSE_ARGV 0 arg "" "P1;P2" "")
+    TEST(arg_KEYWORDS_MISSING_VALUES "P1")
+    TEST(arg_P1 "abc")
+    TEST(arg_P2 "UNDEFINED")
+  endfunction()
+  test_cmp0174_new_repeated_arg(P1 P1 abc)
 endblock()
 
 message(NOTICE "Testing CMP0174 = OLD")
 block(SCOPE_FOR POLICIES)
   cmake_policy(SET CMP0174 OLD)
   function(test_cmp0174_old)
-    cmake_parse_arguments(PARSE_ARGV 0 arg "" "P1;P2" "")
+    cmake_parse_arguments(PARSE_ARGV 0 arg "" "P1;P2;P3" "")
     TEST(arg_KEYWORDS_MISSING_VALUES "P2")
     TEST(arg_P1 "UNDEFINED")
     TEST(arg_P2 "UNDEFINED")
+    TEST(arg_P3 "UNDEFINED")
   endfunction()
   test_cmp0174_old(P1 "" P2)
+
+  function(test_cmp0174_old_repeated_arg)
+    cmake_parse_arguments(PARSE_ARGV 0 arg "" "P1;P2" "")
+    TEST(arg_KEYWORDS_MISSING_VALUES "P1")
+    TEST(arg_P1 "abc")
+    TEST(arg_P2 "UNDEFINED")
+  endfunction()
+  test_cmp0174_old_repeated_arg(P1 P1 abc)
 endblock()
 
 message(NOTICE "Testing CMP0174 = WARN")
 block(SCOPE_FOR POLICIES)
   cmake_policy(VERSION 3.30)  # WARN
   function(test_cmp0174_warn)
-    cmake_parse_arguments(PARSE_ARGV 0 arg "" "P1;P2" "")
+    cmake_parse_arguments(PARSE_ARGV 0 arg "" "P1;P2;P3" "")
     TEST(arg_KEYWORDS_MISSING_VALUES "P2")
     TEST(arg_P1 "UNDEFINED")
     TEST(arg_P2 "UNDEFINED")
+    TEST(arg_P3 "UNDEFINED")
   endfunction()
   test_cmp0174_warn(P1 "" P2)
+
+  function(test_cmp0174_warn_repeated_arg)
+    cmake_parse_arguments(PARSE_ARGV 0 arg "" "P1;P2" "")
+    TEST(arg_KEYWORDS_MISSING_VALUES "P1")
+    TEST(arg_P1 "abc")
+    TEST(arg_P2 "UNDEFINED")
+  endfunction()
+  test_cmp0174_warn_repeated_arg(P1 P1 abc)
 endblock()