[FileCheck] Fix numeric variable redefinition

[thopre]

Summary

Commit r365249 changed usage of FileCheckNumericVariable to have one
instance of that class per variable as opposed to one instance per
definition of a given variable as was done before. However, it retained
the safety check in setValue that it should only be called with the
variable unset, even after r365625.

However this causes assert failure when a non-pseudo variable is being
redefined. And while redefinition of @LINE at each CHECK line work in
the general case, it caused problem when a substitution failed (fixed in
r365624) and still causes problem when a CHECK line does not match since
@LINE's value is cleared after substitutions in match() happened but
printSubstitutions also attempts a substitution.

This commit solves the root of the problem by changing setValue to set a
new value regardless of whether a value was set or not, thus fixing all
the aforementioned issues.

[jhenderson]

I am wondering why we don't just have setValue call clearValue first in all cases?

[thopre]

In D64882#1590970, @jhenderson wrote:

I am wondering why we don't just have setValue call clearValue first in all cases?

That's a remnant when each variable definition had its own variable and thus setting it twice didn't make sense and indicated an error. I think the right thing to do now is to change setValue to not check if the value is already set or not.

[jhenderson]

In D64882#1590997, @thopre wrote:

In D64882#1590970, @jhenderson wrote:

I am wondering why we don't just have setValue call clearValue first in all cases?

That's a remnant when each variable definition had its own variable and thus setting it twice didn't make sense and indicated an error. I think the right thing to do now is to change setValue to not check if the value is already set or not.

That seems reasonable to me. I think that would make this patch largely redundant, right?

[thopre]

Pursue different approach

[jhenderson]

LGTM.

[thopre]

In D64882#1591158, @jhenderson wrote:

LGTM.

I think I'll add unit tests as well.

[thopre]

In D64882#1591189, @thopre wrote:

In D64882#1591158, @jhenderson wrote:

LGTM.

I think I'll add unit tests as well.

Nvm, one issue is an assert and the other needs to test the diagnostic message, both aspects being difficult to test in unit testing. There's already tests for those so I'll leave it like that.

llvm/include/llvm/Support/FileCheck.h

FileCheckNumericVariable(StringRef Name,
       : Name(Name), DefLineNumber(DefLineNumber) {}
 
   /// \returns name of this numeric variable.
   StringRef getName() const { return Name; }
 
   /// \returns this variable's value.
   Optional<uint64_t> getValue() const { return Value; }
 
-  /// Sets value of this numeric variable, if undefined. Triggers an assertion
-  /// failure if the variable is actually defined.
-  void setValue(uint64_t Value);
+  /// Sets value of this numeric variable to \p NewValue.
+  void setValue(uint64_t NewValue) { Value = NewValue; }
 
   /// Clears value of this numeric variable, regardless of whether it is
   /// currently defined or not.
-  void clearValue();
+  void clearValue() { Value = None; }
 
   /// \returns the line number where this variable is defined, if any, or None
   /// if defined before input is parsed.
   Optional<size_t> getDefLineNumber() { return DefLineNumber; }
 };
 
 /// Class representing the use of a numeric variable in the AST of an
 /// expression.

llvm/lib/Support/FileCheck.cpp

 #include <cstdint>
 #include <list>
 #include <map>
 #include <tuple>
 #include <utility>
 
 using namespace llvm;
 
-void FileCheckNumericVariable::setValue(uint64_t NewValue) {
-  assert(!Value && "Overwriting numeric variable's value is not allowed");
-  Value = NewValue;
-}
-
-void FileCheckNumericVariable::clearValue() {
-  if (!Value)
-    return;
-  Value = None;
-}
-
 Expected<uint64_t> FileCheckNumericVariableUse::eval() const {
   Optional<uint64_t> Value = NumericVariable->getValue();
   if (Value)
     return *Value;
   return make_error<FileCheckUndefVarError>(Name);
 }
 
 Expected<uint64_t> FileCheckASTBinop::eval() const {
if (!Substitutions.empty()) {
 
     size_t InsertOffset = 0;
     // Substitute all string variables and expressions whose values are only
     // now known. Use of string variables defined on the same line are handled
     // by back-references.
     for (const auto &Substitution : Substitutions) {
       // Substitute and check for failure (e.g. use of undefined variable).
       Expected<std::string> Value = Substitution->getResult();
-      if (!Value) {
-        Context->LineVariable->clearValue();
+      if (!Value)
         return Value.takeError();
-      }
 
       // Plop it into the regex at the adjusted offset.
       TmpStr.insert(TmpStr.begin() + Substitution->getIndex() + InsertOffset,
                     Value->begin(), Value->end());
       InsertOffset += Value->size();
     }
 
     // Match the newly constructed regex.
     RegExToMatch = TmpStr;
-    Context->LineVariable->clearValue();
   }
 
   SmallVector<StringRef, 4> MatchInfo;
   if (!Regex(RegExToMatch, Regex::Newline).match(Buffer, &MatchInfo))
     return make_error<FileCheckNotFoundError>();
 
   // Successful regex match.
   assert(!MatchInfo.empty() && "Didn't get any match");

llvm/test/FileCheck/line-count.txt

 50 ERR9: line-count.txt:[[#@LINE-1]]:17: error: unsupported operation '*'
 51
 52 BAD10: [[@LINE-x]]
 53 ERR10: line-count.txt:[[#@LINE-1]]:19: error: invalid operand format 'x'
 54
 55 BAD11: [[@LINE-1x]]
 56 ERR11: line-count.txt:[[#@LINE-1]]:20: error: unexpected characters at end of expression 'x'
 57
-58 CHECK: [[#@LINE]] CHECK
-59 CHECK: [[# @LINE]] CHECK
-60 CHECK: [[# @LINE ]] CHECK
-61
-62 CHECK: [[#@LINE-1]]
-63 CHECK: [[# @LINE-1]] CHECK
-64 CHECK: [[# @LINE -1]] CHECK
-65 CHECK: [[# @LINE - 1]] CHECK
-66 CHECK: [[# @LINE - 1 ]] CHECK
+; RUN: not FileCheck -check-prefix BAD12 -input-file %s %s 2>&1 \
+; RUN:   | FileCheck -check-prefix ERR12 %s
+60
+61 BAD12: [[#@LINE-1]] NOT HERE
+62 ERR12: note: with "@LINE-1" equal to "60"
+63
+64 CHECK: [[#@LINE]] CHECK
+65 CHECK: [[# @LINE]] CHECK
+66 CHECK: [[# @LINE ]] CHECK
+67
+68 CHECK: [[#@LINE-1]]
+69 CHECK: [[# @LINE-1]] CHECK
+70 CHECK: [[# @LINE -1]] CHECK
+71 CHECK: [[# @LINE - 1]] CHECK
+72 CHECK: [[# @LINE - 1 ]] CHECK

llvm/test/FileCheck/numeric-expression.txt

 RUN: FileCheck --input-file %s %s
 
 ; We use CHECK-NEXT directives to force a match on all lines with digits.
 
 ; Numeric variable definition without spaces.
 DEF NO SPC
-11
+10
 CHECK-LABEL: DEF NO SPC
 CHECK-NEXT: [[#VAR1:]]
 
 ; Numeric variable definition with different spacing.
 DEF SPC
 11
 11
 11
 CHECK-LABEL: DEF SPC
 CHECK-NEXT: [[# VAR1a:]]
 CHECK-NEXT: [[# VAR1b :]]
 CHECK-NEXT: [[# VAR1c : ]]
 
+; Numeric variable redefinition.
+REDEF NO SPC
+11
+CHECK-LABEL: REDEF
+CHECK-NEXT: [[#VAR1:]]
+
 ; Numeric expressions using variables defined on other lines without spaces.
 USE NO SPC
 11
 12
 10
 11
 11
 11

llvm/unittests/Support/FileCheckTest.cpp

 }
 
 static void expectUndefError(const Twine &ExpectedUndefVarName, Error Err) {
   expectUndefErrors({ExpectedUndefVarName.str()}, std::move(Err));
 }
 
 TEST_F(FileCheckTest, NumericVariable) {
   // Undefined variable: getValue and eval fail, error returned by eval holds
-  // the name of the undefined variable and setValue does not trigger assert.
+  // the name of the undefined variable.
   FileCheckNumericVariable FooVar = FileCheckNumericVariable("FOO", 1);
   EXPECT_EQ("FOO", FooVar.getName());
   FileCheckNumericVariableUse FooVarUse =
       FileCheckNumericVariableUse("FOO", &FooVar);
   EXPECT_FALSE(FooVar.getValue());
   Expected<uint64_t> EvalResult = FooVarUse.eval();
   EXPECT_FALSE(EvalResult);
   expectUndefError("FOO", EvalResult.takeError());
+
   FooVar.setValue(42);
 
   // Defined variable: getValue and eval return value set.
   Optional<uint64_t> Value = FooVar.getValue();
   EXPECT_TRUE(bool(Value));
   EXPECT_EQ(42U, *Value);
   EvalResult = FooVarUse.eval();
   EXPECT_TRUE(bool(EvalResult));