[MLIR Parser] Improve QoI for "expected token" errors

[lattner]

Summary

A typical problem with missing a token is that the missing
token is at the end of a line. The problem with this is that
the error message gets reported on the start of the following
line (which is where the next / invalid token is) which can
be confusing.

Handle this by noticing this case and backing up to the end of
the previous line.

[bzcheeseman]

Just a couple questions, thanks for knocking this out!

mlir/lib/Parser/Parser.cpp

180

Would something like this work and simplify the loop here?

const char *bufferStart = state.lex.getBufferBegin();
const char *curPtr = loc.getPointer();

// Take the current buffer and back up over empty lines and the last newline if one exists.
auto trimmed = StringRef(bufferStart, curPtr).rtrim(" \t\n");
return emitError(SMLoc::getFromPointer(trimmed.end() - 1), message);

187

OOC why not just curPtr - 1 for these and below?

[rriddle]

LGTM with @bzcheesemans comments addressed. Would love a TODO to look through comments though, otherwise we are going to be emitting diagnostics that point into comments with expected tokens.

[lattner]

Use fancy stringref trick; ignore comments in lines.

mlir/lib/Parser/Parser.h

public:
   // they returned a failure.  The second class fills in by-reference arguments
   // as the results of their action.
 
   //===--------------------------------------------------------------------===//
   // Error Handling
   //===--------------------------------------------------------------------===//
 
   /// Emit an error and return failure.
-  InFlightDiagnostic emitError(const Twine &message = {}) {
-    // If the error is to be emitted at EOF, move it back one character.
-    if (state.curToken.is(Token::eof)) {
-      return emitError(
-          SMLoc::getFromPointer(state.curToken.getLoc().getPointer() - 1),
-          message);
-    }
-    return emitError(state.curToken.getLoc(), message);
-  }
+  InFlightDiagnostic emitError(const Twine &message = {});
   InFlightDiagnostic emitError(SMLoc loc, const Twine &message = {});
 
+  /// Emit an error about a "wrong token".  If the current token is at the
+  /// start of a source line, this will apply heuristics to back up and report
+  /// the error at the end of the previous line, which is where the expected
+  /// token is supposed to be.
+  InFlightDiagnostic emitWrongTokenError(const Twine &message = {});
+
   /// Encode the specified source location information into an attribute for
   /// attachment to the IR.
   Location getEncodedSourceLocation(SMLoc loc) {
     // If there are no active nested parsers, we can get the encoded source
     // location directly.
     if (state.parserDepth == 0)
       return state.lex.getEncodedSourceLocation(loc);
     // Otherwise, we need to re-encode it to point to the top level buffer.

mlir/lib/Parser/Parser.cpp

 ///
 ParseResult
 Parser::parseCommaSeparatedListUntil(Token::Kind rightToken,
                                      function_ref<ParseResult()> parseElement,
                                      bool allowEmptyList) {
   // Handle the empty case.
   if (getToken().is(rightToken)) {
     if (!allowEmptyList)
-      return emitError("expected list element");
+      return emitWrongTokenError("expected list element");
     consumeToken(rightToken);
     return success();
   }
 
   if (parseCommaSeparatedList(parseElement) ||
       parseToken(rightToken, "expected ',' or '" +
                                  Token::getTokenSpelling(rightToken) + "'"))
     return failure();
 
   return success();
 }
 
+InFlightDiagnostic Parser::emitError(const Twine &message) {
+  auto loc = state.curToken.getLoc();
+  if (state.curToken.isNot(Token::eof))
+    return emitError(loc, message);
+
+  // If the error is to be emitted at EOF, move it back one character.
+  return emitError(SMLoc::getFromPointer(loc.getPointer() - 1), message);
+}
+
 InFlightDiagnostic Parser::emitError(SMLoc loc, const Twine &message) {
   auto diag = mlir::emitError(getEncodedSourceLocation(loc), message);
 
   // If we hit a parse error in response to a lexer error, then the lexer
   // already reported the error.
   if (getToken().is(Token::error))
     diag.abandon();
   return diag;
 }
 
+/// Emit an error about a "wrong token".  If the current token is at the
+/// start of a source line, this will apply heuristics to back up and report
+/// the error at the end of the previous line, which is where the expected
+/// token is supposed to be.
+InFlightDiagnostic Parser::emitWrongTokenError(const Twine &message) {
+  auto loc = state.curToken.getLoc();
+
+  // If the error is to be emitted at EOF, move it back one character.
+  if (state.curToken.is(Token::eof))
+    loc = SMLoc::getFromPointer(loc.getPointer() - 1);
+
+  // Determine if the token is at the start of the current line.
+  const char *bufferStart = state.lex.getBufferBegin();
+  const char *curPtr = loc.getPointer();
+
+  // Back up over entirely blank lines.
+  while (1) {
+    // Back up until we see a \n, but don't look past the buffer start.
+    curPtr = StringRef(bufferStart, curPtr - bufferStart).rtrim(" \t").end();
+
+    // For tokens with no preceding source line, just emit at the original
+    // location.
+    if (curPtr == bufferStart || curPtr[-1] != '\n')
+      return emitError(loc, message);
+
+    // Check to see if the preceding line has a comment on it.  We assume that a
+    // `//` is the start of a comment, which is mostly correct.
+    // TODO: This will do the wrong thing for // in a string literal.
+    --curPtr;
+    auto prevLine = StringRef(bufferStart, curPtr - bufferStart);
+    size_t newLineIndex = prevLine.rfind('\n');
+    if (newLineIndex != StringRef::npos)
+      prevLine = prevLine.drop_front(newLineIndex);
+    size_t commentStart = prevLine.find("//");
+    if (commentStart != StringRef::npos)
+      curPtr = prevLine.begin() + commentStart;
+
+    // Otherwise, we can move backwards at least this line.
+    loc = SMLoc::getFromPointer(curPtr);
+  }
+}
+
 /// Consume the specified token if present and return success.  On failure,
 /// output a diagnostic and return failure.
 ParseResult Parser::parseToken(Token::Kind expectedToken,
                                const Twine &message) {
   if (consumeIf(expectedToken))
     return success();
-  return emitError(message);
+  return emitWrongTokenError(message);
 }
 
 /// Parse an optional integer value from the stream.
 OptionalParseResult Parser::parseOptionalInteger(APInt &result) {
   Token curToken = getToken();
   if (curToken.isNot(Token::integer, Token::minus))
     return llvm::None;
 

 ParseResult OperationParser::parseOperation() {
   auto loc = getToken().getLoc();
   SmallVector<ResultRecord, 1> resultIDs;
   size_t numExpectedResults = 0;
   if (getToken().is(Token::percent_identifier)) {
     // Parse the group of result ids.
     auto parseNextResult = [&]() -> ParseResult {
       // Parse the next result id.
-      if (!getToken().is(Token::percent_identifier))
-        return emitError("expected valid ssa identifier");
-
       Token nameTok = getToken();
-      consumeToken(Token::percent_identifier);
+      if (parseToken(Token::percent_identifier,
+                     "expected valid ssa identifier"))
+        return failure();
 
       // If the next token is a ':', we parse the expected result count.
       size_t expectedSubResults = 1;
       if (consumeIf(Token::colon)) {
         // Check that the next token is an integer.
         if (!getToken().is(Token::integer))
-          return emitError("expected integer number of results");
+          return emitWrongTokenError("expected integer number of results");
 
         // Check that number of results is > 0.
         auto val = getToken().getUInt64IntegerValue();
         if (!val.hasValue() || val.getValue() < 1)
-          return emitError("expected named operation to have atleast 1 result");
+          return emitError(
+              "expected named operation to have at least 1 result");
         consumeToken(Token::integer);
         expectedSubResults = *val;
       }
 
       resultIDs.emplace_back(nameTok.getSpelling(), expectedSubResults,
                              nameTok.getLoc());
       numExpectedResults += expectedSubResults;
       return success();
     };
     if (parseCommaSeparatedList(parseNextResult))
       return failure();
 
     if (parseToken(Token::equal, "expected '=' after SSA name"))
       return failure();
   }
 
   Operation *op;
   Token nameTok = getToken();
   if (nameTok.is(Token::bare_identifier) || nameTok.isKeyword())
     op = parseCustomOperation(resultIDs);
   else if (nameTok.is(Token::string))
     op = parseGenericOperation();
   else
-    return emitError("expected operation name in quotes");
+    return emitWrongTokenError("expected operation name in quotes");
 
   // If parsing of the basic operation failed, then this whole thing fails.
   if (!op)
     return failure();
 
   // If the operation had a name, register it.
   if (!resultIDs.empty()) {
     if (op->getNumResults() == 0)

 
 /// Parse a single operation successor.
 ///
 ///   successor ::= block-id
 ///
 ParseResult OperationParser::parseSuccessor(Block *&dest) {
   // Verify branch is identifier and get the matching block.
   if (!getToken().is(Token::caret_identifier))
-    return emitError("expected block name");
+    return emitWrongTokenError("expected block name");
   dest = getBlockNamed(getTokenSpelling(), getToken().getLoc());
   consumeToken();
   return success();
 }
 
 /// Parse a comma-separated list of operation successors in brackets.
 ///
 ///   successor-list ::= `[` successor (`,` successor )* `]`
public:
   }
 
   /// Parse zero or more SSA comma-separated operand references with a specified
   /// surrounding delimiter, and an optional required operand count.
   ParseResult parseOperandList(SmallVectorImpl<UnresolvedOperand> &result,
                                Delimiter delimiter = Delimiter::None,
                                bool allowResultNumber = true,
                                int requiredOperandCount = -1) override {
-    auto startLoc = parser.getToken().getLoc();
-
     // The no-delimiter case has some special handling for better diagnostics.
     if (delimiter == Delimiter::None) {
       // parseCommaSeparatedList doesn't handle the missing case for "none",
       // so we handle it custom here.
       if (parser.getToken().isNot(Token::percent_identifier)) {
         // If we didn't require any operands or required exactly zero (weird)
         // then this is success.
         if (requiredOperandCount == -1 || requiredOperandCount == 0)
           return success();
 
         // Otherwise, try to produce a nice error message.
-        if (parser.getToken().is(Token::l_paren) ||
-            parser.getToken().is(Token::l_square))
-          return emitError(startLoc, "unexpected delimiter");
-        return emitError(startLoc, "invalid operand");
+        if (parser.getToken().isAny(Token::l_paren, Token::l_square))
+          return parser.emitError("unexpected delimiter");
+        return parser.emitWrongTokenError("expected operand");
       }
     }
 
     auto parseOneOperand = [&]() -> ParseResult {
       return parseOperand(result.emplace_back(), allowResultNumber);
     };
 
+    auto startLoc = parser.getToken().getLoc();
     if (parseCommaSeparatedList(delimiter, parseOneOperand, " in operand list"))
       return failure();
 
     // Check that we got the expected # of elements.
     if (requiredOperandCount != -1 &&
         result.size() != static_cast<size_t>(requiredOperandCount))
       return emitError(startLoc, "expected ")
              << requiredOperandCount << " operands";

mlir/test/Dialect/ControlFlow/invalid.mlir

^bb3(%bb3arg : i32):
     return
 }
 
 // -----
 
 func.func @switch_missing_comma(%flag : i32, %caseOperand : i32) {
   cf.switch %flag : i32, [
     default: ^bb1(%caseOperand : i32),
-    45: ^bb2(%caseOperand : i32)
     // expected-error@+1 {{expected ']'}}
+    45: ^bb2(%caseOperand : i32)
     43: ^bb3(%caseOperand : i32)
   ]
 
   ^bb1(%bb1arg : i32):
     return
   ^bb2(%bb2arg : i32):
     return
   ^bb3(%bb3arg : i32):

mlir/test/Dialect/LLVMIR/global.mlir

 // expected-error @+1 {{expected zero or one type}}
 llvm.mlir.global internal @more_than_one_type(0) : i64, i32
 
 // -----
 
 llvm.mlir.global internal @foo(0: i32) : i32
 
 func.func @bar() {
-  // expected-error @+2{{expected ':'}}
+  // expected-error @+1{{expected ':'}}
   llvm.mlir.addressof @foo
 }
 
 // -----
 
 func.func @foo() {
   // The attribute parser will consume the first colon-type, so we put two of
   // them to trigger the attribute type mismatch error.

mlir/test/Dialect/Linalg/invalid.mlir

^bb(%0: f32):
       linalg.index -1 : index
       linalg.yield %0 : f32
   }
 }
 
 // -----
 
 func.func @generic_no_region(%arg0: memref<f32>) {
-  // expected-error @+5 {{expected '{' to begin a region}}
+  // expected-error @+4 {{expected '{' to begin a region}}
   linalg.generic {
     indexing_maps =  [ affine_map<() -> (0)> ],
     iterator_types = []
   } ins(%arg0 : memref<f32>)
 }
 
 // -----
 

mlir/test/Dialect/SPIRV/IR/control-flow-ops.mlir

func.func @branch_argument() -> () {
   spv.Branch ^next(%zero, %zero: i32, i32)
 ^next(%arg0: i32, %arg1: i32):
   spv.Return
 }
 
 // -----
 
 func.func @missing_accessor() -> () {
-  // expected-error @+2 {{expected block name}}
+  // expected-error @+1 {{expected block name}}
   spv.Branch
 }
 
 // -----
 
 func.func @wrong_accessor_count() -> () {
   %true = spv.Constant true
   // expected-error @+1 {{requires 1 successor but found 2}}

mlir/test/Dialect/SPIRV/IR/logical-ops.mlir

func.func @logicalBinary2(%arg0 : vector<4xi1>, %arg1 : vector<4xi1>)
   %0 = spv.LogicalAnd %arg0, %arg1 : vector<4xi1>
   return
 }
 
 // -----
 
 func.func @logicalBinary(%arg0 : i1, %arg1 : i1)
 {
-  // expected-error @+2 {{expected ':'}}
+  // expected-error @+1 {{expected ':'}}
   %0 = spv.LogicalAnd %arg0, %arg1
   return
 }
 
 // -----
 
 func.func @logicalBinary(%arg0 : i1, %arg1 : i1)
 {
func.func @logicalUnary2(%arg0 : vector<4xi1>)
   %0 = spv.LogicalNot %arg0 : vector<4xi1>
   return
 }
 
 // -----
 
 func.func @logicalUnary(%arg0 : i1)
 {
-  // expected-error @+2 {{expected ':'}}
+  // expected-error @+1 {{expected ':'}}
   %0 = spv.LogicalNot %arg0
   return
 }
 
 // -----
 
 func.func @logicalUnary(%arg0 : i1)
 {
func.func @select_op_vec_condn_vec(%arg0: vector<3xi1>) -> () {
   return
 }
 
 // -----
 
 func.func @select_op(%arg0: i1) -> () {
   %0 = spv.Constant 2 : i32
   %1 = spv.Constant 3 : i32
-  // expected-error @+2 {{expected ','}}
+  // expected-error @+1 {{expected ','}}
   %2 = spv.Select %arg0, %0, %1 : i1
   return
 }
 
 // -----
 
 func.func @select_op(%arg1: vector<3xi1>) -> () {
   %0 = spv.Constant 2 : i32

mlir/test/Dialect/SPIRV/IR/memory-ops.mlir

func.func @simple_load_missing_operand() -> () {
   %1 = spv.Load "Function" : f32
   return
 }
 
 // -----
 
 func.func @simple_load_missing_rettype() -> () {
   %0 = spv.Variable : !spv.ptr<f32, Function>
-  // expected-error @+2 {{expected ':'}}
+  // expected-error @+1 {{expected ':'}}
   %1 = spv.Load "Function" %0
   return
 }
 
 // -----
 
 func.func @volatile_load_missing_lbrace() -> () {
   %0 = spv.Variable : !spv.ptr<f32, Function>
func.func @simple_store_missing_ptr_type(%arg0 : f32) -> () {
   spv.Store  %0, %arg0 : f32
   return
 }
 
 // -----
 
 func.func @simple_store_missing_operand(%arg0 : f32) -> () {
   %0 = spv.Variable : !spv.ptr<f32, Function>
-  // expected-error @+1 {{custom op 'spv.Store' invalid operand}} : f32
+  // expected-error @+1 {{expected operand}}
   spv.Store  "Function" , %arg0 : f32
   return
 }
 
 // -----
 
 func.func @simple_store_missing_operand(%arg0 : f32) -> () {
   %0 = spv.Variable : !spv.ptr<f32, Function>

mlir/test/Dialect/SPIRV/IR/misc-ops.mlir

func.func @undef() -> () {
   // expected-error @+2{{expected non-function type}}
   %0 = spv.Undef :
   spv.Return
 }
 
 // -----
 
 func.func @undef() -> () {
-  // expected-error @+2{{expected ':'}}
+  // expected-error @+1{{expected ':'}}
   %0 = spv.Undef
   spv.Return
 }
 
 // -----
 
 func.func @assume_true(%arg : i1) -> () {
   // CHECK: spv.AssumeTrueKHR %{{.*}}

mlir/test/IR/invalid-locations.mlir

 // RUN: mlir-opt %s -split-input-file -verify-diagnostics
 
 // -----
 
 func.func @location_missing_l_paren() {
 ^bb:
   return loc) // expected-error {{expected '(' in location}}
 }
 
 // -----
 
 func.func @location_missing_r_paren() {
 ^bb:
-  return loc(unknown // expected-error@+1 {{expected ')' in location}}
+  return loc(unknown // expected-error {{expected ')' in location}}
 }
 
 // -----
 
 func.func @location_invalid_instance() {
 ^bb:
   return loc() // expected-error {{expected location instance}}
 }

 ^bb:
   return loc(callsite(unknown at )  // expected-error {{expected location instance}}
 }
 
 // -----
 
 func.func @location_callsite_missing_r_paren() {
 ^bb:
-  return loc(callsite( unknown at unknown  // expected-error@+1 {{expected ')' in callsite location}}
+  return loc(callsite( unknown at unknown  // expected-error {{expected ')' in callsite location}}
 }
 
 // -----
 
 func.func @location_fused_missing_greater() {
 ^bb:
   return loc(fused<true [unknown]) // expected-error {{expected '>' after fused location metadata}}
 }

mlir/test/IR/invalid.mlir

 ^bb42 (%0): // expected-error {{expected ':' and type for SSA operand}}
   return
 }
 
 // -----
 
 func.func @block_arg_no_close_paren() {
 ^bb42:
-  cf.br ^bb2( // expected-error@+1 {{expected ':'}}
+  cf.br ^bb2( // expected-error {{expected ':'}}
   return
 }
 
 // -----
 
 func.func @block_first_has_predecessor() {
 // expected-error@-1 {{entry block of region may not have predecessors}}
 ^bb42:
func.func @malformed_for_to() {
   affine.for %i = 1 too 10 { // expected-error {{expected 'to' between bounds}}
   }
 }
 
 // -----
 
 func.func @incomplete_for() {
   affine.for %i = 1 to 10 step 2
-}        // expected-error {{expected '{' to begin a region}}
+}        // expected-error @-1 {{expected '{' to begin a region}}
 
 // -----
 
 #map0 = affine_map<(d0) -> (d0 floordiv 4)>
 
 func.func @reference_to_iv_in_bound() {
   // expected-error@+2 {{region entry argument '%i0' is already in use}}
   // expected-note@+1 {{previously referenced here}}
func.func @multi_result_missing_count() {
   // expected-error@+1 {{expected integer number of results}}
   %0: = "foo" () : () -> (i32, i32)
   return
 }
 
 // -----
 
 func.func @multi_result_zero_count() {
-  // expected-error@+1 {{expected named operation to have atleast 1 result}}
+  // expected-error@+1 {{expected named operation to have at least 1 result}}
   %0:0 = "foo" () : () -> (i32, i32)
   return
 }
 
 // -----
 
 func.func @multi_result_invalid_identifier() {
   // expected-error@+1 {{expected valid ssa identifier}}

 
 // -----
 
 // expected-error@+1 {{expected valid attribute name}}
 "t"(){""}
 
 // -----
 
-// expected-error@+2 {{expected ']'}}
+// expected-error@+1 {{expected ']'}}
 "f"() { b = [@m:
 
+// -----
+
+// This makes sure we emit an error at the end of the correct line, the : is
+// expected at the end of foo, not on the return line.
+func.func @error_at_end_of_line() {
+  // expected-error@+1 {{expected ':' followed by operation type}}
+  %0 = "foo"() 
+  return
+}
+
+// -----
+
+// This makes sure we emit an error at the end of the correct line, the : is
+// expected at the end of foo, not on the return line.
+func.func @error_at_end_of_line() {
+  %0 = "foo"() 
+  // expected-error@-1 {{expected ':' followed by operation type}}
+
+  // This is a comment and so is the thing above.
+  return
+}