Improve error message when declarativeAssembly contains invalid literals

[rriddle]

mlir/tools/mlir-tblgen/FormatGen.cpp

213–214

Is there a test for this?

mlir/tools/mlir-tblgen/FormatGen.h

151

Is the llvm:: necessary on these?

[mehdi_amini]

Remove spurious llvm:: namespace qualifier

[mehdi_amini]

mlir/tools/mlir-tblgen/FormatGen.cpp

213–214

No, I haven't been able to hit this case from the existing callers.

It is handled here: https://github.com/llvm/llvm-project/blob/main/mlir/tools/mlir-tblgen/OpFormatGen.cpp#L2716

mlir/test/mlir-tblgen/op-format-spec.td

def DirectiveTypeZOperandInvalidI : TestFormat_Op<[{
   type($result) type($result)
 }]>, Results<(outs I64:$result)>;
 
 //===----------------------------------------------------------------------===//
 // Literals
 //===----------------------------------------------------------------------===//
 
 // Test all of the valid literals.
-// CHECK: error: expected valid literal
+// CHECK: error: expected valid literal but got 'a:': keywords should contain only alphanum, '_', '$', or '.' characters
 def LiteralInvalidA : TestFormat_Op<[{
+  `a:`
+}]>;
+// CHECK: error: expected valid literal but got '1': single character literal must be a letter or one of '_:,=<>()[]{}?+*'
+def LiteralInvalidB : TestFormat_Op<[{
   `1`
 }]>;
+// CHECK: error: expected valid literal but got ':abc': valid keyword starts with a letter or '_'
+def LiteralInvalidC : TestFormat_Op<[{
+  `:abc`
+}]>;
+
 // CHECK: error: unexpected end of file in literal
 // CHECK: error: expected directive, literal, variable, or optional group
-def LiteralInvalidB : TestFormat_Op<[{
+def LiteralInvalidD : TestFormat_Op<[{
   `
 }]>;
 // CHECK-NOT: error
 def LiteralValid : TestFormat_Op<[{
   `_` `:` `,` `=` `<` `>` `(` `)` `[` `]` `?` `+` `*` ` ` `` `->` `\n` `abc$._`
   attr-dict
 }]>;
 

mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp

 FormatParser::parseLiteral(ParserContext ctx) {
   if (ctx != TopLevelContext) {
     return emitError(
         "literals may only be used in the top-level section of the format");
   }
 
   /// Get the literal spelling without the surrounding "`".
   auto value = curToken.getSpelling().drop_front().drop_back();
-  if (!isValidLiteral(value))
-    return emitError("literal '" + value + "' is not valid");
+  if (!isValidLiteral(value, [&](Twine diag) {
+        (void)emitError("expected valid literal but got '" + value +
+                        "': " + diag);
+      }))
+    return failure();
 
   consumeToken();
   return {std::make_unique<LiteralElement>(value)};
 }
 
 FailureOr<std::unique_ptr<Element>>
 FormatParser::parseVariable(ParserContext ctx) {
   /// Get the parameter name without the preceding "$".

mlir/tools/mlir-tblgen/FormatGen.h

 };
 
 /// Whether a space needs to be emitted before a literal. E.g., two keywords
 /// back-to-back require a space separator, but a keyword followed by '<' does
 /// not require a space.
 bool shouldEmitSpaceBefore(StringRef value, bool lastWasPunctuation);
 
 /// Returns true if the given string can be formatted as a keyword.
-bool canFormatStringAsKeyword(StringRef value);
+bool canFormatStringAsKeyword(StringRef value,
+                              function_ref<void(Twine)> emitError = nullptr);
 
 /// Returns true if the given string is valid format literal element.
-bool isValidLiteral(StringRef value);
+/// If `emitError` is provided, it is invoked with the reason for the failure.
+bool isValidLiteral(StringRef value,
+                    function_ref<void(Twine)> emitError = nullptr);
 
 /// Whether a failure in parsing the assembly format should be a fatal error.
 extern llvm::cl::opt<bool> formatErrorIsFatal;
 
 } // end namespace tblgen
 } // end namespace mlir
 
 #endif // MLIR_TOOLS_MLIRTBLGEN_FORMATGEN_H_

mlir/tools/mlir-tblgen/FormatGen.cpp

bool mlir::tblgen::shouldEmitSpaceBefore(StringRef value,
                                          bool lastWasPunctuation) {
   if (value.size() != 1 && value != "->")
     return true;
   if (lastWasPunctuation)
     return !StringRef(">)}],").contains(value.front());
   return !StringRef("<>(){}[],").contains(value.front());
 }
 
-bool mlir::tblgen::canFormatStringAsKeyword(StringRef value) {
-  if (!isalpha(value.front()) && value.front() != '_')
+bool mlir::tblgen::canFormatStringAsKeyword(
+    StringRef value, function_ref<void(Twine)> emitError) {
+  if (!isalpha(value.front()) && value.front() != '_') {
+    if (emitError)
+      emitError("valid keyword starts with a letter or '_'");
     return false;
-  return llvm::all_of(value.drop_front(), [](char c) {
+  }
+  if (!llvm::all_of(value.drop_front(), [](char c) {
         return isalnum(c) || c == '_' || c == '$' || c == '.';
-  });
+      })) {
+    if (emitError)
+      emitError(
+          "keywords should contain only alphanum, '_', '$', or '.' characters");
+    return false;
+  }
+  return true;
 }
 
-bool mlir::tblgen::isValidLiteral(StringRef value) {
-  if (value.empty())
+bool mlir::tblgen::isValidLiteral(StringRef value,
+                                  function_ref<void(Twine)> emitError) {
+  if (value.empty()) {
+    if (emitError)
+      emitError("literal can't be empty");
     return false;
+  }
   char front = value.front();
 
   // If there is only one character, this must either be punctuation or a
   // single character bare identifier.
-  if (value.size() == 1)
-    return isalpha(front) || StringRef("_:,=<>()[]{}?+*").contains(front);
-
+  if (value.size() == 1) {
+    StringRef bare = "_:,=<>()[]{}?+*";
+    if (isalpha(front) || bare.contains(front))
+      return true;
+    if (emitError)
+      emitError("single character literal must be a letter or one of '" + bare +
+                "'");
+    return false;
+  }
   // Check the punctuation that are larger than a single character.
   if (value == "->")
     return true;
 
   // Otherwise, this must be an identifier.
-  return canFormatStringAsKeyword(value);
+  return canFormatStringAsKeyword(value, emitError);
 }
 
 //===----------------------------------------------------------------------===//
 // Commandline Options
 //===----------------------------------------------------------------------===//
 
 llvm::cl::opt<bool> mlir::tblgen::formatErrorIsFatal(
     "asmformat-error-is-fatal",
     llvm::cl::desc("Emit a fatal error if format parsing fails"),
     llvm::cl::init(true));

mlir/tools/mlir-tblgen/OpFormatGen.cpp

LogicalResult FormatParser::parseLiteral(std::unique_ptr<Element> &element,
   }
   // The parsed literal is a newline element.
   if (value == "\\n") {
     element = std::make_unique<NewlineElement>();
     return ::mlir::success();
   }
 
   // Check that the parsed literal is valid.
-  if (!isValidLiteral(value))
-    return emitError(literalTok.getLoc(), "expected valid literal");
-
+  if (!isValidLiteral(value, [&](Twine diag) {
+        (void)emitError(literalTok.getLoc(),
+                        "expected valid literal but got '" + value +
+                            "': " + diag);
+      }))
+    return failure();
   element = std::make_unique<LiteralElement>(value);
   return ::mlir::success();
 }
 
 LogicalResult FormatParser::parseOptional(std::unique_ptr<Element> &element,
                                           ParserContext context) {
   llvm::SMLoc curLoc = curToken.getLoc();
   if (context != TopLevelContext)