Merge topic 'foreach-loop-variable'

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

Acked-by: Kitware Robot <kwrobot@kitware.com>
Acked-by: Ben Boeckel <ben.boeckel@kitware.com>
Acked-by: Michael Hirsch <michael@scivision.dev>
Merge-request: !6044

4 files changed, 46 insertions, 11 deletions

diff --git a/Source/cmForEachCommand.cxx b/Source/cmForEachCommand.cxx
index bcacb15f9c..4845a6d091 100644
--- a/Source/cmForEachCommand.cxx
+++ b/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;
diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx
index 40a67a399d..dba856037f 100644
--- a/Source/cmMakefile.cxx
+++ b/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);
diff --git a/Source/cmMakefile.h b/Source/cmMakefile.h
index 71d765c71f..3c07808170 100644
--- a/Source/cmMakefile.h
+++ b/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;
   /**
diff --git a/Source/cmPolicies.h b/Source/cmPolicies.h
index 0a5bb4b100..0ff12d57b1 100644
--- a/Source/cmPolicies.h
+++ b/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)                                         \