From 61b33c3f4eae3ce81df36c79ec69630cd9fcefdc Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Mon, 9 Aug 2021 20:35:56 +0300 Subject: Fix: Regression in the `cmConditionEvaluator::HandleLevel0` As reported in the BUG #22524, mismatched parenthesis reported differently for `while()` and `if()`. The problem was in the double loop (over "handlers" and the arguments), where the outer loop didn't check the result of the running handler. --- Source/cmConditionEvaluator.cxx | 8 +++++++- Tests/RunCMake/if/RunCMakeTest.cmake | 2 ++ Tests/RunCMake/if/unbalanced-parenthesis-result.txt | 1 + Tests/RunCMake/if/unbalanced-parenthesis-stderr.txt | 8 ++++++++ Tests/RunCMake/if/unbalanced-parenthesis.cmake | 8 ++++++++ Tests/RunCMake/while/RunCMakeTest.cmake | 2 ++ Tests/RunCMake/while/unbalanced-parenthesis.cmake | 8 ++++++++ 7 files changed, 36 insertions(+), 1 deletion(-) create mode 100644 Tests/RunCMake/if/unbalanced-parenthesis-result.txt create mode 100644 Tests/RunCMake/if/unbalanced-parenthesis-stderr.txt create mode 100644 Tests/RunCMake/if/unbalanced-parenthesis.cmake create mode 100644 Tests/RunCMake/while/unbalanced-parenthesis.cmake diff --git a/Source/cmConditionEvaluator.cxx b/Source/cmConditionEvaluator.cxx index 49189d8..68bc4d8 100644 --- a/Source/cmConditionEvaluator.cxx +++ b/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 diff --git a/Tests/RunCMake/if/RunCMakeTest.cmake b/Tests/RunCMake/if/RunCMakeTest.cmake index 239c167..6baa840 100644 --- a/Tests/RunCMake/if/RunCMakeTest.cmake +++ b/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) diff --git a/Tests/RunCMake/if/unbalanced-parenthesis-result.txt b/Tests/RunCMake/if/unbalanced-parenthesis-result.txt new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/Tests/RunCMake/if/unbalanced-parenthesis-result.txt @@ -0,0 +1 @@ +1 diff --git a/Tests/RunCMake/if/unbalanced-parenthesis-stderr.txt b/Tests/RunCMake/if/unbalanced-parenthesis-stderr.txt new file mode 100644 index 0000000..770ccb8 --- /dev/null +++ b/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\) diff --git a/Tests/RunCMake/if/unbalanced-parenthesis.cmake b/Tests/RunCMake/if/unbalanced-parenthesis.cmake new file mode 100644 index 0000000..c51c755 --- /dev/null +++ b/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() diff --git a/Tests/RunCMake/while/RunCMakeTest.cmake b/Tests/RunCMake/while/RunCMakeTest.cmake index 7da80ac..bb9b991 100644 --- a/Tests/RunCMake/while/RunCMakeTest.cmake +++ b/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) diff --git a/Tests/RunCMake/while/unbalanced-parenthesis.cmake b/Tests/RunCMake/while/unbalanced-parenthesis.cmake new file mode 100644 index 0000000..4b6a5cd --- /dev/null +++ b/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 "It prints but in fact `while()` have to fail") -- cgit v0.12 From 880ca66b51551d5ee732c7b81349c1a5f724d093 Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Mon, 9 Aug 2021 20:58:27 +0300 Subject: Fix: `while()` can silently ignore incorrect condition When `conditionEvaluator.IsTrue(...)` returns `false` it just didn't print the error occured. --- Source/cmWhileCommand.cxx | 36 +++++++++++----------- .../while/unbalanced-parenthesis-result.txt | 1 + .../while/unbalanced-parenthesis-stderr.txt | 5 +++ Tests/RunCMake/while/unbalanced-parenthesis.cmake | 2 +- 4 files changed, 25 insertions(+), 19 deletions(-) create mode 100644 Tests/RunCMake/while/unbalanced-parenthesis-result.txt create mode 100644 Tests/RunCMake/while/unbalanced-parenthesis-stderr.txt diff --git a/Source/cmWhileCommand.cxx b/Source/cmWhileCommand.cxx index 327c1c7..1363386 100644 --- a/Source/cmWhileCommand.cxx +++ b/Source/cmWhileCommand.cxx @@ -75,24 +75,6 @@ bool cmWhileFunctionBlocker::Replay(std::vector functions, conditionEvaluator.IsTrue(expandedArguments, errorString, messageType); 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; - } - } - // Invoke all the functions that were collected in the block. for (cmListFileFunction const& fn : functions) { cmExecutionStatus status(mf); @@ -116,6 +98,24 @@ bool cmWhileFunctionBlocker::Replay(std::vector functions, isTrue = conditionEvaluator.IsTrue(expandedArguments, errorString, messageType); } + + if (!isTrue && !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; } diff --git a/Tests/RunCMake/while/unbalanced-parenthesis-result.txt b/Tests/RunCMake/while/unbalanced-parenthesis-result.txt new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/Tests/RunCMake/while/unbalanced-parenthesis-result.txt @@ -0,0 +1 @@ +1 diff --git a/Tests/RunCMake/while/unbalanced-parenthesis-stderr.txt b/Tests/RunCMake/while/unbalanced-parenthesis-stderr.txt new file mode 100644 index 0000000..33ac54a --- /dev/null +++ b/Tests/RunCMake/while/unbalanced-parenthesis-stderr.txt @@ -0,0 +1,5 @@ +CMake Error at unbalanced-parenthesis.cmake:[0-9]+ \(while\): + had incorrect arguments: NOT \$\{var_with_paren\} IN_LIST some_list + \(mismatched parenthesis in condition\). +Call Stack \(most recent call first\): + CMakeLists\.txt:[0-9]+ \(include\) diff --git a/Tests/RunCMake/while/unbalanced-parenthesis.cmake b/Tests/RunCMake/while/unbalanced-parenthesis.cmake index 4b6a5cd..7a12701 100644 --- a/Tests/RunCMake/while/unbalanced-parenthesis.cmake +++ b/Tests/RunCMake/while/unbalanced-parenthesis.cmake @@ -5,4 +5,4 @@ while(NOT ${var_with_paren} IN_LIST some_list) message(STATUS "Never prints") endwhile() -message(STATUS "It prints but in fact `while()` have to fail") +message(STATUS "Never prints") -- cgit v0.12 From e97e714f0d59809fcefdc94feaadedb319d252c6 Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Mon, 9 Aug 2021 21:48:24 +0300 Subject: Fix: `while()` reports an error the same way as `if()` With arguments list expanded. --- Source/cmWhileCommand.cxx | 12 +++++------- Tests/RunCMake/while/unbalanced-parenthesis-stderr.txt | 7 +++++-- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/Source/cmWhileCommand.cxx b/Source/cmWhileCommand.cxx index 1363386..8938663 100644 --- a/Source/cmWhileCommand.cxx +++ b/Source/cmWhileCommand.cxx @@ -16,6 +16,7 @@ #include "cmListFileCache.h" #include "cmMakefile.h" #include "cmMessageType.h" +#include "cmOutputConverter.h" #include "cmSystemTools.h" #include "cmake.h" @@ -100,16 +101,13 @@ bool cmWhileFunctionBlocker::Replay(std::vector functions, } if (!isTrue && !errorString.empty()) { - std::string err = "had incorrect arguments: "; - for (cmListFileArgument const& arg : this->Args) { - err += (arg.Delim ? "\"" : ""); - err += arg.Value; - err += (arg.Delim ? "\"" : ""); + std::string err = "had incorrect arguments:\n "; + for (cmExpandedCommandArgument const& i : expandedArguments) { err += " "; + err += cmOutputConverter::EscapeForCMake(i.GetValue()); } - err += "("; + err += "\n"; err += errorString; - err += ")."; mf.GetCMakeInstance()->IssueMessage(messageType, err, whileBT); if (messageType == MessageType::FATAL_ERROR) { cmSystemTools::SetFatalErrorOccured(); diff --git a/Tests/RunCMake/while/unbalanced-parenthesis-stderr.txt b/Tests/RunCMake/while/unbalanced-parenthesis-stderr.txt index 33ac54a..9d4132c 100644 --- a/Tests/RunCMake/while/unbalanced-parenthesis-stderr.txt +++ b/Tests/RunCMake/while/unbalanced-parenthesis-stderr.txt @@ -1,5 +1,8 @@ CMake Error at unbalanced-parenthesis.cmake:[0-9]+ \(while\): - had incorrect arguments: NOT \$\{var_with_paren\} IN_LIST some_list - \(mismatched parenthesis in condition\). + had incorrect arguments: + + "NOT" "\(" "IN_LIST" "some_list" + + mismatched parenthesis in condition Call Stack \(most recent call first\): CMakeLists\.txt:[0-9]+ \(include\) -- cgit v0.12 From d22f68d01998ff3cbbdebaec2f1071148b82d588 Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Tue, 10 Aug 2021 00:41:55 +0300 Subject: Refactor: Transform `while` loop into `for` And reduce scope for some variables + use some more `auto`. --- Source/cmWhileCommand.cxx | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/Source/cmWhileCommand.cxx b/Source/cmWhileCommand.cxx index 8938663..1285baf 100644 --- a/Source/cmWhileCommand.cxx +++ b/Source/cmWhileCommand.cxx @@ -61,21 +61,31 @@ bool cmWhileFunctionBlocker::ArgumentsMatch(cmListFileFunction const& lff, bool cmWhileFunctionBlocker::Replay(std::vector functions, cmExecutionStatus& inStatus) { - cmMakefile& mf = inStatus.GetMakefile(); - std::string errorString; - - std::vector 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 expandedArguments; + // At least same size expected for `expandedArguments` as `Args` + expandedArguments.reserve(this->Args.size()); + + auto expandArgs = [&mf](std::vector const& args, + std::vector& out) + -> std::vector& { + out.clear(); + mf.ExpandArguments(args, out); + return out; + }; + + std::string errorString; + MessageType messageType; + auto isTrue = true; - while (isTrue) { + for (cmConditionEvaluator conditionEvaluator(mf, whileBT); + (isTrue = + 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); @@ -94,15 +104,11 @@ bool cmWhileFunctionBlocker::Replay(std::vector functions, return true; } } - expandedArguments.clear(); - mf.ExpandArguments(this->Args, expandedArguments); - isTrue = - conditionEvaluator.IsTrue(expandedArguments, errorString, messageType); } if (!isTrue && !errorString.empty()) { std::string err = "had incorrect arguments:\n "; - for (cmExpandedCommandArgument const& i : expandedArguments) { + for (auto const& i : expandedArguments) { err += " "; err += cmOutputConverter::EscapeForCMake(i.GetValue()); } @@ -127,7 +133,7 @@ bool cmWhileCommand(std::vector const& args, // create a function blocker { - cmMakefile& makefile = status.GetMakefile(); + auto& makefile = status.GetMakefile(); auto fb = cm::make_unique(&makefile); fb->Args = args; makefile.AddFunctionBlocker(std::move(fb)); -- cgit v0.12 From 4c1cdfd8f09182d8a6c87772b7fc0946c691e18b Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Tue, 10 Aug 2021 01:04:32 +0300 Subject: Refactor: Keep `cmWhileFunctionBlocker` members private Particularly `Args`. --- Source/cmWhileCommand.cxx | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/Source/cmWhileCommand.cxx b/Source/cmWhileCommand.cxx index 1285baf..7d9eec0 100644 --- a/Source/cmWhileCommand.cxx +++ b/Source/cmWhileCommand.cxx @@ -23,7 +23,7 @@ class cmWhileFunctionBlocker : public cmFunctionBlocker { public: - cmWhileFunctionBlocker(cmMakefile* mf); + cmWhileFunctionBlocker(cmMakefile* mf, std::vector args); ~cmWhileFunctionBlocker() override; cm::string_view StartCommandName() const override { return "while"_s; } @@ -35,14 +35,15 @@ public: bool Replay(std::vector functions, cmExecutionStatus& inStatus) override; - std::vector Args; - private: cmMakefile* Makefile; + std::vector Args; }; -cmWhileFunctionBlocker::cmWhileFunctionBlocker(cmMakefile* mf) - : Makefile(mf) +cmWhileFunctionBlocker::cmWhileFunctionBlocker( + cmMakefile* const mf, std::vector args) + : Makefile{ mf } + , Args{ std::move(args) } { this->Makefile->PushLoopBlock(); } @@ -132,11 +133,9 @@ bool cmWhileCommand(std::vector const& args, } // create a function blocker - { - auto& makefile = status.GetMakefile(); - auto fb = cm::make_unique(&makefile); - fb->Args = args; - makefile.AddFunctionBlocker(std::move(fb)); - } + auto& makefile = status.GetMakefile(); + makefile.AddFunctionBlocker( + cm::make_unique(&makefile, args)); + return true; } -- cgit v0.12 From eae125ace54bd82ed5be14cfee50f41a07b10619 Mon Sep 17 00:00:00 2001 From: Alex Turbov Date: Tue, 10 Aug 2021 01:19:56 +0300 Subject: Refactor: Get rid of `isTrue` variable in the `while` block execution --- Source/cmWhileCommand.cxx | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/Source/cmWhileCommand.cxx b/Source/cmWhileCommand.cxx index 7d9eec0..b8297ce 100644 --- a/Source/cmWhileCommand.cxx +++ b/Source/cmWhileCommand.cxx @@ -81,12 +81,10 @@ bool cmWhileFunctionBlocker::Replay(std::vector functions, std::string errorString; MessageType messageType; - auto isTrue = true; for (cmConditionEvaluator conditionEvaluator(mf, whileBT); - (isTrue = - conditionEvaluator.IsTrue(expandArgs(this->Args, expandedArguments), - errorString, messageType));) { + 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); @@ -107,7 +105,7 @@ bool cmWhileFunctionBlocker::Replay(std::vector functions, } } - if (!isTrue && !errorString.empty()) { + if (!errorString.empty()) { std::string err = "had incorrect arguments:\n "; for (auto const& i : expandedArguments) { err += " "; -- cgit v0.12