[FileCheck] Don't diagnose undef vars at parse time

[thopre]

Summary

Diagnosing use of undefined variables takes place in
parseNumericVariableUse() and printSubstitutions() for numeric variables
but only takes place in printSubstitutions() for string variables. The
reason for the split location of diagnostics is that parsing is not
aware of the clearing of variables due to --enable-var-scope and thus
use of variables cleared in this way can only be catched by
printSubstitutions().

Beyond the code level inconsistency, there is also a user facing
inconsistency since diagnostics look different between the two
functions. While the diagnostic in printSubstitutions is more verbose,
doing the diagnostic there allows to diagnose all undefined variables
rather than just the first one and error out.

This patch create dummy variable definition when encountering a use of
undefined variable so that parsing can proceed and be diagnosed by
printSubstitutions() later. Tests that were testing whether parsing
fails in such case are thus modified accordingly.

[jhenderson]

llvm/lib/Support/FileCheck.cpp

160 ↗

(On Diff #208095)

use -> uses

161 ↗

(On Diff #208095)

when -> during (?)

llvm/test/FileCheck/numeric-expression.txt

72 ↗

(On Diff #208095)

Have you lost test coverage here? It looks to me like you're no longer checking the error message printed at all, and I don't understand why from your comment.

llvm/unittests/Support/FileCheckTest.cpp

474 ↗

(On Diff #208095)

If EXPECT_TRUE doesn't work with Expression, can you call the boolean operator of Expression here? I think that would be nicer than a static_cast.

480 ↗

(On Diff #208095)

store -> stored

[thopre]

Address review comments

[thopre]

llvm/test/FileCheck/numeric-expression.txt

72 ↗

(On Diff #208095)

I checked llvm/test/FileCheck/verbose.txt and thought it was already tested there but looking at it again today it's clearly not the case. Shouldn't have submitted those patches so late. The codepath is tested via llvm/test/FileCheck/var-scope.txt but (1) not the full diagnostic is checked for (only the error message) and (2) this only checks undefined variable due to --enable-var-scope.

I've recycled the test for undefined numeric variables, I'll submit a separate patch later to add a test for undefined string variables.

[jhenderson]

LGTM.

[thopre]

Use relative line number in test

llvm/trunk/lib/Support/FileCheck.cpp

if (IsPseudo && !Name.equals("@LINE"))
     return FileCheckErrorDiagnostic::get(
         SM, Name, "invalid pseudo numeric variable '" + Name + "'");
 
   // Numeric variable definitions and uses are parsed in the order in which
   // they appear in the CHECK patterns. For each definition, the pointer to the
   // class instance of the corresponding numeric variable definition is stored
   // in GlobalNumericVariableTable in parsePattern. Therefore, the pointer we
   // get below is for the class instance corresponding to the last definition
-  // of this variable use.
+  // of this variable use. If we don't find a variable definition we create a
+  // dummy one so that parsing can continue. All uses of undefined variables,
+  // whether string or numeric, are then diagnosed in printSubstitutions()
+  // after failing to match.
   auto VarTableIter = Context->GlobalNumericVariableTable.find(Name);
-  if (VarTableIter == Context->GlobalNumericVariableTable.end())
-    return FileCheckErrorDiagnostic::get(
-        SM, Name, "using undefined numeric variable '" + Name + "'");
+  FileCheckNumericVariable *NumericVariable;
+  if (VarTableIter != Context->GlobalNumericVariableTable.end())
+    NumericVariable = VarTableIter->second;
+  else {
+    NumericVariable = Context->makeNumericVariable(0, Name);
+    Context->GlobalNumericVariableTable[Name] = NumericVariable;
+  }
 
-  FileCheckNumericVariable *NumericVariable = VarTableIter->second;
   if (!IsPseudo && NumericVariable->getDefLineNumber() == LineNumber)
     return FileCheckErrorDiagnostic::get(
         SM, Name,
         "numeric variable '" + Name + "' defined on the same line as used");
 
   return NumericVariable;
 }
 

llvm/trunk/test/FileCheck/numeric-expression.txt

 ; Numeric expression using undefined variable.
 RUN: not FileCheck --check-prefix UNDEF-USE --input-file %s %s 2>&1 \
 RUN:   | FileCheck --strict-whitespace --check-prefix UNDEF-USE-MSG %s
 
 UNDEF VAR USE
 UNDEFVAR: 11
 UNDEF-USE-LABEL: UNDEF VAR USE
 UNDEF-USE-NEXT: UNDEFVAR: [[#UNDEFVAR]]
-UNDEF-USE-MSG: numeric-expression.txt:[[#@LINE-1]]:30: error: using undefined numeric variable 'UNDEFVAR'
+UNDEF-USE-MSG: numeric-expression.txt:[[#@LINE-1]]:17: error: {{U}}NDEF-USE-NEXT: expected string not found in input
 UNDEF-USE-MSG-NEXT: {{U}}NDEF-USE-NEXT: UNDEFVAR: {{\[\[#UNDEFVAR\]\]}}
-UNDEF-USE-MSG-NEXT: {{^                             \^$}}
+UNDEF-USE-MSG-NEXT: {{^                \^$}}
+UNDEF-USE-MSG-NEXT: numeric-expression.txt:[[#@LINE-6]]:1: note: scanning from here
+UNDEF-USE-MSG-NEXT: UNDEFVAR: 11
+UNDEF-USE-MSG-NEXT: {{^\^$}}
+UNDEF-USE-MSG-NEXT: numeric-expression.txt:[[#@LINE-9]]:1: note: uses undefined variable "UNDEFVAR"
+UNDEF-USE-MSG-NEXT: UNDEFVAR: 11
+UNDEF-USE-MSG-NEXT: {{^\^$}}
 
 ; Numeric expression with unsupported operator.
 RUN: not FileCheck -D#NUMVAR=10 --check-prefix INVAL-OP --input-file %s %s 2>&1 \
 RUN:   | FileCheck --strict-whitespace --check-prefix INVAL-OP-MSG %s
 
 INVALID OPERATOR
 NUMVAR*2: 22
 INVAL-OP-LABEL: INVALID OPERATOR

llvm/trunk/unittests/Support/FileCheckTest.cpp

TEST_F(FileCheckTest, ParseExpr) {
   EXPECT_FALSE(Tester.parseSubstExpect("VAR3  :"));
   EXPECT_FALSE(Tester.parseSubstExpect("VAR3:  "));
 
   // Numeric expression.
 
   // Unacceptable variable.
   EXPECT_TRUE(Tester.parseSubstExpect("10VAR"));
   EXPECT_TRUE(Tester.parseSubstExpect("@FOO"));
-  EXPECT_TRUE(Tester.parseSubstExpect("UNDEF"));
 
   // Only valid variable.
   EXPECT_FALSE(Tester.parseSubstExpect("@LINE"));
   EXPECT_FALSE(Tester.parseSubstExpect("FOO"));
+  EXPECT_FALSE(Tester.parseSubstExpect("UNDEF"));
 
   // Use variable defined on same line.
   EXPECT_FALSE(Tester.parsePatternExpect("[[#LINE1VAR:]]"));
   EXPECT_TRUE(Tester.parseSubstExpect("LINE1VAR"));
 
   // Unsupported operator.
   EXPECT_TRUE(Tester.parseSubstExpect("@LINE/2"));
 
TEST_F(FileCheckTest, FileCheckContext) {
   // Check a numeric expression's evaluation fails if called after clearing of
   // local variables, if it was created before. This is important because local
   // variable clearing due to --enable-var-scope happens after numeric
   // expressions are linked to the numeric variables they use.
   EXPECT_TRUE(errorToBool((*Expression)->eval().takeError()));
   P = FileCheckPattern(Check::CheckPlain, &Cxt, 2);
   Expression = P.parseNumericSubstitutionBlock(LocalNumVarRef,
                                                DefinedNumericVariable, SM);
-  EXPECT_TRUE(errorToBool(Expression.takeError()));
+  EXPECT_TRUE(bool(Expression));
+  ExpressionVal = (*Expression)->eval();
+  EXPECT_TRUE(errorToBool(ExpressionVal.takeError()));
   EmptyVar = Cxt.getPatternVarValue(EmptyVarStr);
   EXPECT_TRUE(errorToBool(EmptyVar.takeError()));
+  // Clear again because parseNumericSubstitutionBlock would have created a
+  // dummy variable and stored it in GlobalNumericVariableTable.
+  Cxt.clearLocalVars();
 
   // Redefine global variables and check variables are defined again.
   GlobalDefines.emplace_back(std::string("$GlobalVar=BAR"));
   GlobalDefines.emplace_back(std::string("#$GlobalNumVar=36"));
   EXPECT_FALSE(errorToBool(Cxt.defineCmdlineVariables(GlobalDefines, SM)));
   StringRef GlobalVarStr = "$GlobalVar";
   StringRef GlobalNumVarRef = bufferize(SM, "$GlobalNumVar");
   Expected<StringRef> GlobalVar = Cxt.getPatternVarValue(GlobalVarStr);