[Mlir-commits] [mlir] 48fb79e - Improve error message when declarativeAssembly contains invalid literals

[Mehdi Amini]

Author: Mehdi Amini
Date: 2021-12-04T00:27:32Z
New Revision: 48fb79effb65cb786e8c7264d86af77154bf53c8

URL: https://github.com/llvm/llvm-project/commit/48fb79effb65cb786e8c7264d86af77154bf53c8
DIFF: https://github.com/llvm/llvm-project/commit/48fb79effb65cb786e8c7264d86af77154bf53c8.diff

LOG: Improve error message when declarativeAssembly contains invalid literals

Differential Revision: https://reviews.llvm.org/D115085

Added: 
    

Modified: 
    mlir/test/mlir-tblgen/op-format-spec.td
    mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp
    mlir/tools/mlir-tblgen/FormatGen.cpp
    mlir/tools/mlir-tblgen/FormatGen.h
    mlir/tools/mlir-tblgen/OpFormatGen.cpp

Removed: 
    


################################################################################
diff  --git a/mlir/test/mlir-tblgen/op-format-spec.td b/mlir/test/mlir-tblgen/op-format-spec.td
index e4d0f9e3b21f8..5b79efc704ec5 100644
--- a/mlir/test/mlir-tblgen/op-format-spec.td
+++ b/mlir/test/mlir-tblgen/op-format-spec.td
@@ -320,13 +320,22 @@ def DirectiveTypeZOperandInvalidI : TestFormat_Op<[{
 //===----------------------------------------------------------------------===//
 
 // 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

diff  --git a/mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp b/mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp
index 3c6035b7baf5b..01ae08e57be9e 100644
--- a/mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp
+++ b/mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp
@@ -606,8 +606,11 @@ FormatParser::parseLiteral(ParserContext ctx) {
 
   /// 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)};

diff  --git a/mlir/tools/mlir-tblgen/FormatGen.cpp b/mlir/tools/mlir-tblgen/FormatGen.cpp
index fa6c0603ac7e1..b4bad2aef0c3c 100644
--- a/mlir/tools/mlir-tblgen/FormatGen.cpp
+++ b/mlir/tools/mlir-tblgen/FormatGen.cpp
@@ -189,30 +189,50 @@ bool mlir::tblgen::shouldEmitSpaceBefore(StringRef value,
   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) {
-    return isalnum(c) || c == '_' || c == '$' || 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);
 }
 
 //===----------------------------------------------------------------------===//

diff  --git a/mlir/tools/mlir-tblgen/FormatGen.h b/mlir/tools/mlir-tblgen/FormatGen.h
index f061d1ed5c678..3a434765b29ec 100644
--- a/mlir/tools/mlir-tblgen/FormatGen.h
+++ b/mlir/tools/mlir-tblgen/FormatGen.h
@@ -147,10 +147,13 @@ class FormatLexer {
 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;

diff  --git a/mlir/tools/mlir-tblgen/OpFormatGen.cpp b/mlir/tools/mlir-tblgen/OpFormatGen.cpp
index dfe671c9b7a93..0827146da180f 100644
--- a/mlir/tools/mlir-tblgen/OpFormatGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpFormatGen.cpp
@@ -2724,9 +2724,12 @@ LogicalResult FormatParser::parseLiteral(std::unique_ptr<Element> &element,
   }
 
   // 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();
 }