foreach(): loop variables are only available in the loop scope

Fixes: #20553

Help/command/foreach.rst

@@ -14,9 +14,12 @@ semicolon or whitespace.
 All commands between ``foreach`` and the matching ``endforeach`` are recorded
 without being invoked.  Once the ``endforeach`` is evaluated, the recorded
 list of commands is invoked once for each item in ``<items>``.
-At the beginning of each iteration the variable ``loop_var`` will be set
+At the beginning of each iteration the variable ``<loop_var>`` will be set
 to the value of the current item.
 
+The scope of ``<loop_var>`` is restricted to the loop scope. See policy
+:policy:`CMP0124` for details.
+
 The commands :command:`break` and :command:`continue` provide means to
 escape from the normal control flow.
 

Help/manual/cmake-policies.7.rst

@@ -57,6 +57,7 @@ Policies Introduced by CMake 3.21
 .. toctree::
    :maxdepth: 1
 
+   CMP0124: foreach() loop variables are only available in the loop scope. </policy/CMP0124>
    CMP0123: ARMClang cpu/arch compile and link flags must be set explicitly. </policy/CMP0123>
    CMP0122: UseSWIG use standard library name conventions for csharp language. </policy/CMP0122>
    CMP0121: The list command detects invalid indicies. </policy/CMP0121>

Help/policy/CMP0124.rst

@@ -0,0 +1,20 @@
+CMP0124
+-------
+
+.. versionadded:: 3.21
+
+The loop variables created by :command:`foreach` command have now their scope
+restricted to the loop scope.
+
+Starting with CMake 3.21, the :command:`foreach` command ensures that the loop
+variables have their scope restricted to the loop scope.
+
+The ``OLD`` behavior for this policy let the loop variables to exist, with an
+empty value, in the outer scope of loop scope.
+
+This policy was introduced in CMake version 3.21. Use the
+:command:`cmake_policy` command to set it to ``OLD`` or ``NEW`` explicitly.
+Unlike many policies, CMake version |release| does *not* warn when the policy
+is not set and simply uses ``OLD`` behavior.
+
+.. include:: DEPRECATED.txt

Help/release/dev/foreach-variable-scope.rst

@@ -0,0 +1,5 @@
+foreach-variable-scope
+----------------------
+
+* The :command:`foreach` command restrict loop variables to the loop scope.
+  See policy :policy:`CMP0124` for details.

Source/cmForEachCommand.cxx

@@ -17,6 +17,7 @@
 #include <utility>
 
 #include <cm/memory>
+#include <cm/optional>
 #include <cm/string_view>
 #include <cmext/string_view>
 
@@ -25,7 +26,7 @@
 #include "cmListFileCache.h"
 #include "cmMakefile.h"
 #include "cmMessageType.h"
-#include "cmProperty.h"
+#include "cmPolicies.h"
 #include "cmRange.h"
 #include "cmStringAlgorithms.h"
 #include "cmSystemTools.h"
@@ -113,9 +114,11 @@ bool cmForEachFunctionBlocker::ReplayItems(
 
   // At end of for each execute recorded commands
   // store the old value
-  std::string oldDef;
-  if (cmProp d = mf.GetDefinition(this->Args.front())) {
-    oldDef = *d;
+  cm::optional<std::string> oldDef;
+  if (mf.GetPolicyStatus(cmPolicies::CMP0124) != cmPolicies::NEW) {
+    oldDef = mf.GetSafeDefinition(this->Args.front());
+  } else if (mf.IsNormalDefinitionSet(this->Args.front())) {
+    oldDef = *mf.GetDefinition(this->Args.front());
   }
 
   auto restore = false;
@@ -131,9 +134,14 @@ bool cmForEachFunctionBlocker::ReplayItems(
   }
 
   if (restore) {
-    // restore the variable to its prior value
-    mf.AddDefinition(this->Args.front(), oldDef);
+    if (oldDef) {
+      // restore the variable to its prior value
+      mf.AddDefinition(this->Args.front(), *oldDef);
+    } else {
+      mf.RemoveDefinition(this->Args.front());
+    }
   }
+
   return true;
 }
 
@@ -185,10 +193,15 @@ bool cmForEachFunctionBlocker::ReplayZipLists(
   assert("Sanity check" && iterationVars.size() == values.size());
 
   // Store old values for iteration variables
-  std::map<std::string, std::string> oldDefs;
+  std::map<std::string, cm::optional<std::string>> oldDefs;
   for (auto i = 0u; i < values.size(); ++i) {
-    if (cmProp d = mf.GetDefinition(iterationVars[i])) {
-      oldDefs.emplace(iterationVars[i], *d);
+    const auto& varName = iterationVars[i];
+    if (mf.GetPolicyStatus(cmPolicies::CMP0124) != cmPolicies::NEW) {
+      oldDefs.emplace(varName, mf.GetSafeDefinition(varName));
+    } else if (mf.IsNormalDefinitionSet(varName)) {
+      oldDefs.emplace(varName, *mf.GetDefinition(varName));
+    } else {
+      oldDefs.emplace(varName, cm::nullopt);
     }
   }
 
@@ -226,7 +239,11 @@ bool cmForEachFunctionBlocker::ReplayZipLists(
   // Restore the variables to its prior value
   if (restore) {
     for (auto const& p : oldDefs) {
-      mf.AddDefinition(p.first, p.second);
+      if (p.second) {
+        mf.AddDefinition(p.first, *p.second);
+      } else {
+        mf.RemoveDefinition(p.first);
+      }
     }
   }
   return true;

Source/cmMakefile.cxx

@@ -2507,6 +2507,20 @@ bool cmMakefile::IsDefinitionSet(const std::string& name) const
   return def != nullptr;
 }
 
+bool cmMakefile::IsNormalDefinitionSet(const std::string& name) const
+{
+  cmProp def = this->StateSnapshot.GetDefinition(name);
+#ifndef CMAKE_BOOTSTRAP
+  if (cmVariableWatch* vv = this->GetVariableWatch()) {
+    if (!def) {
+      vv->VariableAccessed(
+        name, cmVariableWatch::UNKNOWN_VARIABLE_DEFINED_ACCESS, nullptr, this);
+    }
+  }
+#endif
+  return def != nullptr;
+}
+
 cmProp cmMakefile::GetDefinition(const std::string& name) const
 {
   cmProp def = this->StateSnapshot.GetDefinition(name);

Source/cmMakefile.h

@@ -486,6 +486,7 @@ public:
   const std::string& GetSafeDefinition(const std::string&) const;
   const std::string& GetRequiredDefinition(const std::string& name) const;
   bool IsDefinitionSet(const std::string&) const;
+  bool IsNormalDefinitionSet(const std::string&) const;
   bool GetDefExpandList(const std::string& name, std::vector<std::string>& out,
                         bool emptyArgs = false) const;
   /**

Source/cmPolicies.h

@@ -369,7 +369,10 @@ class cmMakefile;
     21, 0, cmPolicies::WARN)                                                  \
   SELECT(POLICY, CMP0123,                                                     \
          "ARMClang cpu/arch compile and link flags must be set explicitly.",  \
-         3, 21, 0, cmPolicies::WARN)
+         3, 21, 0, cmPolicies::WARN)                                          \
+  SELECT(POLICY, CMP0124,                                                     \
+         "foreach() loop variables are only available in the loop scope.", 3, \
+         21, 0, cmPolicies::WARN)
 
 #define CM_SELECT_ID(F, A1, A2, A3, A4, A5, A6) F(A1)
 #define CM_FOR_EACH_POLICY_ID(POLICY)                                         \

Tests/RunCMake/foreach/RunCMakeTest.cmake

@@ -20,3 +20,5 @@ run_cmake(foreach-RANGE-non-int-test-3-2)
 run_cmake(foreach-RANGE-non-int-test-3-3)
 run_cmake(foreach-RANGE-invalid-test)
 run_cmake(foreach-RANGE-out-of-range-test)
+run_cmake(foreach-var-scope-CMP0124-OLD)
+run_cmake(foreach-var-scope-CMP0124-NEW)

Tests/RunCMake/foreach/foreach-var-scope-CMP0124-NEW.cmake

@@ -0,0 +1,51 @@
+
+cmake_policy(SET CMP0124 NEW)
+
+foreach(VAR a b c)
+endforeach()
+if (DEFINED VAR)
+  message(SEND_ERROR "Variable 'VAR' unexpectedly defined.")
+endif()
+
+set(LIST1 a b c)
+set(LIST2 x y z)
+foreach(VAR1_1 VAR1_2 IN ZIP_LISTS LIST1 LIST2)
+endforeach()
+if (DEFINED VAR1_1 OR DEFINED VAR1_2)
+  message(SEND_ERROR "Variables 'VAR1_1' or 'VAR1_2' unexpectedly defined.")
+endif()
+
+
+set (VAR2 OLD)
+foreach(VAR2 a b c)
+endforeach()
+if (NOT DEFINED VAR2 OR NOT VAR2 STREQUAL "OLD")
+  message(SEND_ERROR "Variable 'VAR2' not defined or wrong value.")
+endif()
+
+set (VAR2_2 OLD)
+foreach(VAR2_1 VAR2_2 IN ZIP_LISTS LIST1 LIST2)
+endforeach()
+if (DEFINED VAR2_1 OR NOT DEFINED VAR2_2)
+  message(SEND_ERROR "Variable 'VAR2_1' unexpectedly defined or variable 'VAR2_2' not defined.")
+endif()
+
+
+set (VAR3 OLD CACHE STRING "")
+foreach(VAR3 a b c)
+endforeach()
+# check that only cache variable is defined
+set(OLD_VALUE "${VAR3}")
+unset(VAR3 CACHE)
+if (DEFINED VAR3 OR NOT OLD_VALUE STREQUAL "OLD")
+  message(SEND_ERROR "Variable 'VAR3' wrongly defined or wrong value.")
+endif()
+
+set (VAR3_2 OLD CACHE STRING "")
+foreach(VAR3_1 VAR3_2 IN ZIP_LISTS LIST1 LIST2)
+endforeach()
+set(OLD_VALUE "${VAR3_2}")
+unset(VAR3_2 CACHE)
+if (DEFINED VAR3_1 OR DEFINED VAR3_2 OR NOT OLD_VALUE STREQUAL "OLD")
+  message(SEND_ERROR "Variable 'VAR3_1' unexpectedly defined or variable 'VAR2_2' wrongly defined or wrong value.")
+endif()

Tests/RunCMake/foreach/foreach-var-scope-CMP0124-OLD.cmake

@@ -0,0 +1,53 @@
+
+cmake_policy(SET CMP0124 OLD)
+
+foreach(VAR a b c)
+endforeach()
+if (NOT DEFINED VAR OR NOT VAR STREQUAL "")
+  message(SEND_ERROR "Variable 'VAR' not defined or wrong value.")
+endif()
+
+set(LIST1 a b c)
+set(LIST2 x y z)
+foreach(VAR1_1 VAR1_2 IN ZIP_LISTS LIST1 LIST2)
+endforeach()
+if (NOT DEFINED VAR1_1 OR NOT VAR1_1 STREQUAL ""
+    OR NOT DEFINED VAR1_2 OR NOT VAR1_2 STREQUAL "")
+  message(SEND_ERROR "Variables 'VAR1_1' or 'VAR1_2' not defined or wrong value.")
+endif()
+
+
+set (VAR2 OLD)
+foreach(VAR2 a b c)
+endforeach()
+if (NOT DEFINED VAR2 OR NOT VAR2 STREQUAL "OLD")
+  message(SEND_ERROR "Variable 'VAR2' not defined or wrong value.")
+endif()
+
+set (VAR2_2 OLD)
+foreach(VAR2_1 VAR2_2 IN ZIP_LISTS LIST1 LIST2)
+endforeach()
+if (NOT DEFINED VAR2_1 OR NOT VAR2_1 STREQUAL ""
+    OR NOT DEFINED VAR2_2 OR NOT VAR2_2 STREQUAL "OLD")
+  message(SEND_ERROR "Variables 'VAR2_1' or 'VAR2_2' not defined or wrong value.")
+endif()
+
+
+set (VAR3 OLD CACHE STRING "")
+foreach(VAR3 a b c)
+endforeach()
+# a normal variable is defined, holding cache variable value
+unset(VAR3 CACHE)
+if (NOT DEFINED VAR3 OR NOT VAR3 STREQUAL "OLD")
+  message(SEND_ERROR "Variable 'VAR3' not defined or wrong value.")
+endif()
+
+set (VAR3_2 OLD CACHE STRING "")
+foreach(VAR3_1 VAR3_2 IN ZIP_LISTS LIST1 LIST2)
+endforeach()
+# a normal variable is defined, holding cache variable value
+unset(VAR3_2 CACHE)
+if (NOT DEFINED VAR3_1 OR NOT VAR3_1 STREQUAL ""
+    OR NOT DEFINED VAR3_2 OR NOT VAR3_2 STREQUAL "OLD")
+  message(SEND_ERROR "Variables 'VAR3_1' or 'VAR3_2' not defined or wrong value.")
+endif()