Merge topic 'while-regression'

eae125ace5 Refactor: Get rid of `isTrue` variable in the `while` block execution
4c1cdfd8f0 Refactor: Keep `cmWhileFunctionBlocker` members private
d22f68d019 Refactor: Transform `while` loop into `for`
e97e714f0d Fix: `while()` reports an error the same way as `if()`
880ca66b51 Fix: `while()` can silently ignore incorrect condition
61b33c3f4e Fix: Regression in the `cmConditionEvaluator::HandleLevel0`

Acked-by: Kitware Robot <kwrobot@kitware.com>
Merge-request: !6442

Source/cmConditionEvaluator.cxx

@@ -260,11 +260,17 @@ bool cmConditionEvaluator::IsTrue(
   for (auto fn : handlers) {
     // Call the reducer 'till there is anything to reduce...
     // (i.e., if after an iteration the size becomes smaller)
+    auto levelResult = true;
     for (auto beginSize = newArgs.size();
-         (this->*fn)(newArgs, errorString, status) &&
+         (levelResult = (this->*fn)(newArgs, errorString, status)) &&
          newArgs.size() < beginSize;
          beginSize = newArgs.size()) {
     }
+
+    if (!levelResult) {
+      // NOTE `errorString` supposed to be set already
+      return false;
+    }
   }
 
   // now at the end there should only be one argument left

Source/cmWhileCommand.cxx

@@ -16,13 +16,14 @@
 #include "cmListFileCache.h"
 #include "cmMakefile.h"
 #include "cmMessageType.h"
+#include "cmOutputConverter.h"
 #include "cmSystemTools.h"
 #include "cmake.h"
 
 class cmWhileFunctionBlocker : public cmFunctionBlocker
 {
 public:
-  cmWhileFunctionBlocker(cmMakefile* mf);
+  cmWhileFunctionBlocker(cmMakefile* mf, std::vector<cmListFileArgument> args);
   ~cmWhileFunctionBlocker() override;
 
   cm::string_view StartCommandName() const override { return "while"_s; }
@@ -34,14 +35,15 @@ public:
   bool Replay(std::vector<cmListFileFunction> functions,
               cmExecutionStatus& inStatus) override;
 
-  std::vector<cmListFileArgument> Args;
-
 private:
   cmMakefile* Makefile;
+  std::vector<cmListFileArgument> Args;
 };
 
-cmWhileFunctionBlocker::cmWhileFunctionBlocker(cmMakefile* mf)
-  : Makefile(mf)
+cmWhileFunctionBlocker::cmWhileFunctionBlocker(
+  cmMakefile* const mf, std::vector<cmListFileArgument> args)
+  : Makefile{ mf }
+  , Args{ std::move(args) }
 {
   this->Makefile->PushLoopBlock();
 }
@@ -60,39 +62,29 @@ bool cmWhileFunctionBlocker::ArgumentsMatch(cmListFileFunction const& lff,
 bool cmWhileFunctionBlocker::Replay(std::vector<cmListFileFunction> functions,
                                     cmExecutionStatus& inStatus)
 {
-  cmMakefile& mf = inStatus.GetMakefile();
-  std::string errorString;
-
-  std::vector<cmExpandedCommandArgument> expandedArguments;
-  mf.ExpandArguments(this->Args, expandedArguments);
-  MessageType messageType;
+  auto& mf = inStatus.GetMakefile();
 
   cmListFileBacktrace whileBT =
     mf.GetBacktrace().Push(this->GetStartingContext());
-  cmConditionEvaluator conditionEvaluator(mf, whileBT);
 
-  bool isTrue =
-    conditionEvaluator.IsTrue(expandedArguments, errorString, messageType);
+  std::vector<cmExpandedCommandArgument> expandedArguments;
+  // At least same size expected for `expandedArguments` as `Args`
+  expandedArguments.reserve(this->Args.size());
 
-  while (isTrue) {
-    if (!errorString.empty()) {
-      std::string err = "had incorrect arguments: ";
-      for (cmListFileArgument const& arg : this->Args) {
-        err += (arg.Delim ? "\"" : "");
-        err += arg.Value;
-        err += (arg.Delim ? "\"" : "");
-        err += " ";
-      }
-      err += "(";
-      err += errorString;
-      err += ").";
-      mf.GetCMakeInstance()->IssueMessage(messageType, err, whileBT);
-      if (messageType == MessageType::FATAL_ERROR) {
-        cmSystemTools::SetFatalErrorOccured();
-        return true;
-      }
-    }
+  auto expandArgs = [&mf](std::vector<cmListFileArgument> const& args,
+                          std::vector<cmExpandedCommandArgument>& out)
+    -> std::vector<cmExpandedCommandArgument>& {
+    out.clear();
+    mf.ExpandArguments(args, out);
+    return out;
+  };
 
+  std::string errorString;
+  MessageType messageType;
+
+  for (cmConditionEvaluator conditionEvaluator(mf, whileBT);
+       conditionEvaluator.IsTrue(expandArgs(this->Args, expandedArguments),
+                                 errorString, messageType);) {
     // Invoke all the functions that were collected in the block.
     for (cmListFileFunction const& fn : functions) {
       cmExecutionStatus status(mf);
@@ -111,11 +103,22 @@ bool cmWhileFunctionBlocker::Replay(std::vector<cmListFileFunction> functions,
         return true;
       }
     }
-    expandedArguments.clear();
-    mf.ExpandArguments(this->Args, expandedArguments);
-    isTrue =
-      conditionEvaluator.IsTrue(expandedArguments, errorString, messageType);
   }
+
+  if (!errorString.empty()) {
+    std::string err = "had incorrect arguments:\n ";
+    for (auto const& i : expandedArguments) {
+      err += " ";
+      err += cmOutputConverter::EscapeForCMake(i.GetValue());
+    }
+    err += "\n";
+    err += errorString;
+    mf.GetCMakeInstance()->IssueMessage(messageType, err, whileBT);
+    if (messageType == MessageType::FATAL_ERROR) {
+      cmSystemTools::SetFatalErrorOccured();
+    }
+  }
+
   return true;
 }
 
@@ -128,11 +131,9 @@ bool cmWhileCommand(std::vector<cmListFileArgument> const& args,
   }
 
   // create a function blocker
-  {
-    cmMakefile& makefile = status.GetMakefile();
-    auto fb = cm::make_unique<cmWhileFunctionBlocker>(&makefile);
-    fb->Args = args;
-    makefile.AddFunctionBlocker(std::move(fb));
-  }
+  auto& makefile = status.GetMakefile();
+  makefile.AddFunctionBlocker(
+    cm::make_unique<cmWhileFunctionBlocker>(&makefile, args));
+
   return true;
 }

Tests/RunCMake/if/RunCMakeTest.cmake

@@ -9,6 +9,8 @@ run_cmake(duplicate-else-after-elseif)
 run_cmake(elseif-message)
 run_cmake(misplaced-elseif)
 
+run_cmake(unbalanced-parenthesis)
+
 run_cmake(MatchesSelf)
 run_cmake(IncompleteMatches)
 run_cmake(IncompleteMatchesFail)

Tests/RunCMake/if/unbalanced-parenthesis-result.txt

@@ -0,0 +1 @@
+1

Tests/RunCMake/if/unbalanced-parenthesis-stderr.txt

@@ -0,0 +1,8 @@
+CMake Error at unbalanced-parenthesis\.cmake:[0-9]+ \(if\):
+  if given arguments:
+
+    "NOT" "\(" "IN_LIST" "some_list"
+
+  mismatched parenthesis in condition
+Call Stack \(most recent call first\):
+  CMakeLists\.txt:[0-9]+ \(include\)

Tests/RunCMake/if/unbalanced-parenthesis.cmake

@@ -0,0 +1,8 @@
+set(var_with_paren "(")
+set(some_list "")
+
+if(NOT ${var_with_paren} IN_LIST some_list)
+  message(STATUS "Never prints")
+else()
+  message(STATUS "Never prints")
+endif()

Tests/RunCMake/while/RunCMakeTest.cmake

@@ -5,3 +5,5 @@ run_cmake(EndMissing)
 run_cmake(EndMismatch)
 run_cmake(EndAlone)
 run_cmake(EndAloneArgs)
+
+run_cmake(unbalanced-parenthesis)

Tests/RunCMake/while/unbalanced-parenthesis-result.txt

@@ -0,0 +1 @@
+1

Tests/RunCMake/while/unbalanced-parenthesis-stderr.txt

@@ -0,0 +1,8 @@
+CMake Error at unbalanced-parenthesis.cmake:[0-9]+ \(while\):
+  had incorrect arguments:
+
+    "NOT" "\(" "IN_LIST" "some_list"
+
+  mismatched parenthesis in condition
+Call Stack \(most recent call first\):
+  CMakeLists\.txt:[0-9]+ \(include\)

Tests/RunCMake/while/unbalanced-parenthesis.cmake

@@ -0,0 +1,8 @@
+set(var_with_paren "(")
+set(some_list "")
+
+while(NOT ${var_with_paren} IN_LIST some_list)
+  message(STATUS "Never prints")
+endwhile()
+
+message(STATUS "Never prints")