add_custom_command: Validate arguments more rigorously

Add a new CMP0175 policy to preserve backward compatibility
for projects that were using unsupported keywords or arguments.

Fixes: #26096, #21089, #18976

Source/cmAddCustomCommandCommand.cxx

@@ -2,11 +2,15 @@
    file Copyright.txt or https://cmake.org/licensing for details.  */
 #include "cmAddCustomCommandCommand.h"
 
+#include <algorithm>
+#include <iterator>
+#include <set>
 #include <sstream>
 #include <unordered_set>
 #include <utility>
 
 #include <cm/memory>
+#include <cmext/string_view>
 
 #include "cmCustomCommand.h"
 #include "cmCustomCommandLines.h"
@@ -140,11 +144,82 @@ bool cmAddCustomCommandCommand(std::vector<std::string> const& args,
     keyDEPENDS_EXPLICIT_ONLY,
     keyCODEGEN
   };
+  /* clang-format off */
+  static std::set<std::string> const supportedTargetKeywords{
+    keyARGS,
+    keyBYPRODUCTS,
+    keyCOMMAND,
+    keyCOMMAND_EXPAND_LISTS,
+    keyCOMMENT,
+    keyPOST_BUILD,
+    keyPRE_BUILD,
+    keyPRE_LINK,
+    keyTARGET,
+    keyVERBATIM,
+    keyWORKING_DIRECTORY
+  };
+  /* clang-format on */
+  static std::set<std::string> const supportedOutputKeywords{
+    keyAPPEND,
+    keyARGS,
+    keyBYPRODUCTS,
+    keyCODEGEN,
+    keyCOMMAND,
+    keyCOMMAND_EXPAND_LISTS,
+    keyCOMMENT,
+    keyDEPENDS,
+    keyDEPENDS_EXPLICIT_ONLY,
+    keyDEPFILE,
+    keyIMPLICIT_DEPENDS,
+    keyJOB_POOL,
+    keyJOB_SERVER_AWARE,
+    keyMAIN_DEPENDENCY,
+    keyOUTPUT,
+    keyUSES_TERMINAL,
+    keyVERBATIM,
+    keyWORKING_DIRECTORY
+  };
+  /* clang-format off */
+  static std::set<std::string> const supportedAppendKeywords{
+    keyAPPEND,
+    keyARGS,
+    keyCOMMAND,
+    keyCOMMENT,           // Allowed but ignored
+    keyDEPENDS,
+    keyIMPLICIT_DEPENDS,
+    keyMAIN_DEPENDENCY,   // Allowed but ignored
+    keyOUTPUT,
+    keyWORKING_DIRECTORY  // Allowed but ignored
+  };
+  /* clang-format on */
+  std::set<std::string> keywordsSeen;
+  std::string const* keywordExpectingValue = nullptr;
+  auto const cmp0175 = mf.GetPolicyStatus(cmPolicies::CMP0175);
 
   for (std::string const& copy : args) {
     if (keywords.count(copy)) {
+      // Check if a preceding keyword expected a value but there wasn't one
+      if (keywordExpectingValue) {
+        std::string const msg =
+          cmStrCat("Keyword ", *keywordExpectingValue,
+                   " requires a value, but none was given.");
+        if (cmp0175 == cmPolicies::NEW) {
+          mf.IssueMessage(MessageType::FATAL_ERROR, msg);
+          return false;
+        }
+        if (cmp0175 == cmPolicies::WARN) {
+          mf.IssueMessage(
+            MessageType::AUTHOR_WARNING,
+            cmStrCat(msg, '\n',
+                     cmPolicies::GetPolicyWarning(cmPolicies::CMP0175)));
+        }
+      }
+      keywordExpectingValue = nullptr;
+      keywordsSeen.insert(copy);
+
       if (copy == keySOURCE) {
         doing = doing_source;
+        keywordExpectingValue = &keySOURCE;
       } else if (copy == keyCOMMAND) {
         doing = doing_command;
 
@@ -173,6 +248,7 @@ bool cmAddCustomCommandCommand(std::vector<std::string> const& args,
         codegen = true;
       } else if (copy == keyTARGET) {
         doing = doing_target;
+        keywordExpectingValue = &keyTARGET;
       } else if (copy == keyARGS) {
         // Ignore this old keyword.
       } else if (copy == keyDEPENDS) {
@@ -181,16 +257,20 @@ bool cmAddCustomCommandCommand(std::vector<std::string> const& args,
         doing = doing_outputs;
       } else if (copy == keyOUTPUT) {
         doing = doing_output;
+        keywordExpectingValue = &keyOUTPUT;
       } else if (copy == keyBYPRODUCTS) {
         doing = doing_byproducts;
       } else if (copy == keyWORKING_DIRECTORY) {
         doing = doing_working_directory;
+        keywordExpectingValue = &keyWORKING_DIRECTORY;
       } else if (copy == keyMAIN_DEPENDENCY) {
         doing = doing_main_dependency;
+        keywordExpectingValue = &keyMAIN_DEPENDENCY;
       } else if (copy == keyIMPLICIT_DEPENDS) {
         doing = doing_implicit_depends_lang;
       } else if (copy == keyCOMMENT) {
         doing = doing_comment;
+        keywordExpectingValue = &keyCOMMENT;
       } else if (copy == keyDEPFILE) {
         doing = doing_depfile;
         if (!mf.GetGlobalGenerator()->SupportsCustomCommandDepfile()) {
@@ -198,12 +278,16 @@ bool cmAddCustomCommandCommand(std::vector<std::string> const& args,
                                    mf.GetGlobalGenerator()->GetName()));
           return false;
         }
+        keywordExpectingValue = &keyDEPFILE;
       } else if (copy == keyJOB_POOL) {
         doing = doing_job_pool;
+        keywordExpectingValue = &keyJOB_POOL;
       } else if (copy == keyJOB_SERVER_AWARE) {
         doing = doing_job_server_aware;
+        keywordExpectingValue = &keyJOB_SERVER_AWARE;
       }
     } else {
+      keywordExpectingValue = nullptr; // Value is being processed now
       std::string filename;
       switch (doing) {
         case doing_output:
@@ -288,6 +372,21 @@ bool cmAddCustomCommandCommand(std::vector<std::string> const& args,
           byproducts.push_back(filename);
           break;
         case doing_comment:
+          if (!comment_buffer.empty()) {
+            std::string const msg =
+              "COMMENT requires exactly one argument, but multiple values "
+              "or COMMENT keywords have been given.";
+            if (cmp0175 == cmPolicies::NEW) {
+              mf.IssueMessage(MessageType::FATAL_ERROR, msg);
+              return false;
+            }
+            if (cmp0175 == cmPolicies::WARN) {
+              mf.IssueMessage(
+                MessageType::AUTHOR_WARNING,
+                cmStrCat(msg, '\n',
+                         cmPolicies::GetPolicyWarning(cmPolicies::CMP0175)));
+            }
+          }
           comment_buffer = copy;
           comment = comment_buffer.c_str();
           break;
@@ -351,6 +450,27 @@ bool cmAddCustomCommandCommand(std::vector<std::string> const& args,
 
   // Check for an append request.
   if (append) {
+    std::vector<std::string> unsupportedKeywordsUsed;
+    std::set_difference(keywordsSeen.begin(), keywordsSeen.end(),
+                        supportedAppendKeywords.begin(),
+                        supportedAppendKeywords.end(),
+                        std::back_inserter(unsupportedKeywordsUsed));
+    if (!unsupportedKeywordsUsed.empty()) {
+      std::string const msg =
+        cmJoin(unsupportedKeywordsUsed, ", "_s,
+               "The following keywords are not supported when using "
+               "APPEND with add_custom_command(OUTPUT): "_s);
+      if (cmp0175 == cmPolicies::NEW) {
+        mf.IssueMessage(MessageType::FATAL_ERROR, msg);
+        return false;
+      }
+      if (cmp0175 == cmPolicies::WARN) {
+        mf.IssueMessage(
+          MessageType::AUTHOR_WARNING,
+          cmStrCat(msg, ".\n",
+                   cmPolicies::GetPolicyWarning(cmPolicies::CMP0175)));
+      }
+    }
     mf.AppendCustomCommandToOutput(output[0], depends, implicit_depends,
                                    commandLines);
     return true;
@@ -376,9 +496,92 @@ bool cmAddCustomCommandCommand(std::vector<std::string> const& args,
   cc->SetDependsExplicitOnly(depends_explicit_only);
   if (source.empty() && output.empty()) {
     // Source is empty, use the target.
+    if (commandLines.empty()) {
+      std::string const msg = "At least one COMMAND must be given.";
+      if (cmp0175 == cmPolicies::NEW) {
+        mf.IssueMessage(MessageType::FATAL_ERROR, msg);
+        return false;
+      }
+      if (cmp0175 == cmPolicies::WARN) {
+        mf.IssueMessage(
+          MessageType::AUTHOR_WARNING,
+          cmStrCat(msg, '\n',
+                   cmPolicies::GetPolicyWarning(cmPolicies::CMP0175)));
+      }
+    }
+
+    std::vector<std::string> unsupportedKeywordsUsed;
+    std::set_difference(keywordsSeen.begin(), keywordsSeen.end(),
+                        supportedTargetKeywords.begin(),
+                        supportedTargetKeywords.end(),
+                        std::back_inserter(unsupportedKeywordsUsed));
+    if (!unsupportedKeywordsUsed.empty()) {
+      std::string const msg =
+        cmJoin(unsupportedKeywordsUsed, ", "_s,
+               "The following keywords are not supported when using "
+               "add_custom_command(TARGET): "_s);
+      if (cmp0175 == cmPolicies::NEW) {
+        mf.IssueMessage(MessageType::FATAL_ERROR, msg);
+        return false;
+      }
+      if (cmp0175 == cmPolicies::WARN) {
+        mf.IssueMessage(
+          MessageType::AUTHOR_WARNING,
+          cmStrCat(msg, ".\n",
+                   cmPolicies::GetPolicyWarning(cmPolicies::CMP0175)));
+      }
+    }
+    auto const prePostCount = keywordsSeen.count(keyPRE_BUILD) +
+      keywordsSeen.count(keyPRE_LINK) + keywordsSeen.count(keyPOST_BUILD);
+    if (prePostCount != 1) {
+      std::string msg =
+        "Exactly one of PRE_BUILD, PRE_LINK, or POST_BUILD must be given.";
+      if (cmp0175 == cmPolicies::NEW) {
+        mf.IssueMessage(MessageType::FATAL_ERROR, msg);
+        return false;
+      }
+      if (cmp0175 == cmPolicies::WARN) {
+        msg += " Assuming ";
+        switch (cctype) {
+          case cmCustomCommandType::PRE_BUILD:
+            msg += "PRE_BUILD";
+            break;
+          case cmCustomCommandType::PRE_LINK:
+            msg += "PRE_LINK";
+            break;
+          case cmCustomCommandType::POST_BUILD:
+            msg += "POST_BUILD";
+        }
+        mf.IssueMessage(
+          MessageType::AUTHOR_WARNING,
+          cmStrCat(msg, " to preserve backward compatibility.\n",
+                   cmPolicies::GetPolicyWarning(cmPolicies::CMP0175)));
+      }
+    }
     mf.AddCustomCommandToTarget(target, cctype, std::move(cc));
   } else if (target.empty()) {
     // Target is empty, use the output.
+    std::vector<std::string> unsupportedKeywordsUsed;
+    std::set_difference(keywordsSeen.begin(), keywordsSeen.end(),
+                        supportedOutputKeywords.begin(),
+                        supportedOutputKeywords.end(),
+                        std::back_inserter(unsupportedKeywordsUsed));
+    if (!unsupportedKeywordsUsed.empty()) {
+      std::string const msg =
+        cmJoin(unsupportedKeywordsUsed, ", "_s,
+               "The following keywords are not supported when using "
+               "add_custom_command(OUTPUT): "_s);
+      if (cmp0175 == cmPolicies::NEW) {
+        mf.IssueMessage(MessageType::FATAL_ERROR, msg);
+        return false;
+      }
+      if (cmp0175 == cmPolicies::WARN) {
+        mf.IssueMessage(
+          MessageType::AUTHOR_WARNING,
+          cmStrCat(msg, ".\n",
+                   cmPolicies::GetPolicyWarning(cmPolicies::CMP0175)));
+      }
+    }
     cc->SetOutputs(output);
     cc->SetMainDependency(main_dependency);
     cc->SetDepends(depends);

Source/cmPolicies.h

@@ -537,6 +537,8 @@ class cmMakefile;
   SELECT(POLICY, CMP0174,                                                     \
          "cmake_parse_arguments(PARSE_ARGV) defines a variable for an empty " \
          "string after a single-value keyword.",                              \
+         3, 31, 0, cmPolicies::WARN)                                          \
+  SELECT(POLICY, CMP0175, "add_custom_command() rejects invalid arguments.",  \
          3, 31, 0, cmPolicies::WARN)
 
 #define CM_SELECT_ID(F, A1, A2, A3, A4, A5, A6) F(A1)