From 06443726ff0f96bb38f6d4e0654056dde8b61fea Mon Sep 17 00:00:00 2001
From: Jonas Rembser <jonas.rembser@cern.ch>
Date: Sun, 13 Mar 2022 17:21:11 +0100
Subject: [PATCH] [RF] Correctly treat variables with integer names in
 RooFormula

In case of named arguments, the RooFormula will replace the argument
names with` x[0]` to `x[n]`. There are two things that can go wrong if
RooFormula is not implemented right. First, if there is a variable named
"x" it should only be substituted if the matching substring is not
followed by "[", to not replace existing x[i]. Second, variables with
integer names like "0" should only be substituted if the match is not
followed by a "]", again to avoid replacing x[i]. This test checks that
these cases are handled correctly.

The second case was so far not dealt with correctly, but with this
commit it is. A corresponding unit test was also implemented.

The preprocessor commands in `RooFormula` were also reorganized
slightly, such that one can test the `TPRegexp` backend simply by
commenting out the `define ROOFORMULA_HAVE_STD_REGEX`.
---
 roofit/roofitcore/src/RooFormula.cxx      | 12 ++++++++----
 roofit/roofitcore/test/testRooFormula.cxx | 17 +++++++++++++++++
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/roofit/roofitcore/src/RooFormula.cxx b/roofit/roofitcore/src/RooFormula.cxx
index ca0de6673a87a..0cd115ae90d77 100644
--- a/roofit/roofitcore/src/RooFormula.cxx
+++ b/roofit/roofitcore/src/RooFormula.cxx
@@ -148,6 +148,10 @@ RooFormula::RooFormula(const RooFormula& other, const char* name) :
 #ifndef _MSC_VER
 #if !defined(__GNUC__) || defined(__clang__) || (__GNUC__ > 4) || ( __GNUC__ == 4 && __GNUC_MINOR__ > 8)
 #define ROOFORMULA_HAVE_STD_REGEX
+#endif //GCC < 4.9 Check
+#endif //_MSC_VER
+
+#ifdef ROOFORMULA_HAVE_STD_REGEX
 ////////////////////////////////////////////////////////////////////////////////
 /// Process given formula by replacing all ordinal and name references by
 /// `x[i]`, where `i` matches the position of the argument in `_origList`.
@@ -208,6 +212,7 @@ std::string RooFormula::processFormula(std::string formula) const {
     auto regex = std::string{"\\b"} + var.GetName();
     regex = std::regex_replace(regex, std::regex("([\\[\\]\\{\\}])"), "\\$1"); // The name might contain [, ], {, or }.
     regex += "\\b(?!\\[)"; // Veto '[' as next character. If the variable is called `x`, this might otherwise replace `x[0]`.
+    regex += "\\b(?!\\])"; // Veto ']' as next character. If the variable is called `0`, this might otherwise replace `x[0]`.
     std::regex findParameterRegex(regex);
 
     std::stringstream replacement;
@@ -270,8 +275,7 @@ std::string RooFormula::reconstructFormula(std::string internalRepr) const {
 
   return internalRepr;
 }
-#endif //GCC < 4.9 Check
-#endif //_MSC_VER
+#endif //ROOFORMULA_HAVE_STD_REGEX
 
 
 
@@ -612,7 +616,7 @@ std::string RooFormula::processFormula(std::string formula) const {
     const auto& var = _origList[i];
     TString regex = "\\b";
     regex += var.GetName();
-    regex += "\\b([^[]|$)"; //Negative lookahead. If the variable is called `x`, this might otherwise replace `x[0]`.
+    regex += "\\b([^\\[\\]]|$)"; //Negative lookahead. If the variable is called `x` or `0`, this might otherwise replace `x[0]`.
     TPRegexp findParameterRegex(regex);
 
     std::stringstream replacement;
@@ -687,4 +691,4 @@ std::string RooFormula::reconstructFormula(std::string internalRepr) const {
 
   return internalReprT.Data();
 }
-#endif //GCC < 4.9 Check
+#endif //ROOFORMULA_HAVE_STD_REGEX
diff --git a/roofit/roofitcore/test/testRooFormula.cxx b/roofit/roofitcore/test/testRooFormula.cxx
index 30c72fb39f3de..827e639b264db 100644
--- a/roofit/roofitcore/test/testRooFormula.cxx
+++ b/roofit/roofitcore/test/testRooFormula.cxx
@@ -40,3 +40,20 @@ TEST(RooFormula, TestInvalidFormulae) {
   EXPECT_FLOAT_EQ(form6->eval(nullptr), 1.337 - 1.);
   delete form6;
 }
+
+// In case of named arguments, the RooFormula will replace the argument names
+// with x[0] to x[n]. There are two things that can go wrong if RooFormula is
+// not implemented right. First, if there is a variable named "x" it should
+// only be substituted if the matching substring is not followed by "[", to not
+// replace existing x[i]. Second, variables with integer names like "0" should
+// only be substituted if the match is not followed by a "]", again to avoid
+// replacing x[i]. This test checks that these cases are handled correctly.
+TEST(RooFormula, TestDangerousVariableNames) {
+  RooRealVar dt("dt", "dt", -10, 10);
+  RooRealVar x("x", "x", 1.547);
+  RooRealVar zero("0", "0", 0);
+
+  // Create the formula, triggers an error if the formula doesn't compile
+  // correctly because the dangerous variable names haven't been treated right.
+  RooFormula formula("formula", "exp(-abs(@0)/@1)*cos(@0*@2)", {dt, x, zero});
+}