Add more context to 'expected non-function type' error message.

[richard-uhler]

Summary

Include what kind of token was found in the error message.

For motivation, imagine a new MLIR user defines a custom dialect type for
booleans and writes .mlir code:

func @xor(%a: bool, %b: bool) -> (bool) { ... }

They will get an error message saying "expected non-function type",
pointing to bool. This can be confusing to a new user who doesn't
realize the issue isn't that the parser thinks "bool" is a function
type, it's that the parser thinks "bool" isn't a type at all.

With this change, the error message reported is now:

"expected non-function type, but found bare-id"

Hopefully that gives a better hint to the user that bare-id isn't a type.

[mehdi_amini]

mlir/lib/Parser/TypeParser.cpp

317

I don't think this is necessarily correct in this context: you can expect a type in various context where hitting a "bare identifier" does not mean it is a custom operation I believe.

[richard-uhler]

Change text from "custom operation" to "bare-id".

And add more context on the problem this CL is trying to solve to the commit
message.

[mehdi_amini]

LGTM: can you write a test for this? (I think mlir/test/IR/invalid.mlir is the usual place for this)

[rriddle]

LGTM after resolving comments.

mlir/lib/Parser/TypeParser.cpp

317

Can you use a note for this instead of lumping it with the error?

[mehdi_amini]

mlir/lib/Parser/TypeParser.cpp

317

We could even be smarter: look if there is a . in the identifier and check if the prefix is an available registered dialect before emitting the note.

[richard-uhler]

mlir/lib/Parser/TypeParser.cpp

317

There are a maybe a dozen test cases in mlir/test/IR/invalid.mlir and other files that hit this case. If I attach a note, all of the tests start to fail because there is an unexpected note. Adding the note to all the test cases feels like over-specifying in the test cases. Do you have any suggestions for how to deal with this? Should I add the note to all the test cases, or is there some way to specify in the test cases that a note is acceptable but not required?

317

The motivating example for me to make this change was where I accidentally used "bool" as the type instead of "!mydialect.bool". Being smarter wouldn't help for this case.

I tried adding my motivating example to the commit message. Can you see that? I don't see the revised commit message showing up at https://reviews.llvm.org/D88611.

[mehdi_amini]

mlir/lib/Parser/TypeParser.cpp

317

If you change your commit locally, your need to add --verbatim when you use arc if you want to update Phabricator to match your local commit message.

You're saying that many tests are hitting this case, why is this a problem only with the note and not with this change already?

The motivating example for me to make this change was where I accidentally used "bool" as the type instead of "!mydialect.bool". Being smarter wouldn't help for this case.

Seems like the message isn't correct then, it only suggests to add ! in front of your bool

[richard-uhler]

mlir/lib/Parser/TypeParser.cpp

317

The change didn't cause problems to existing tests as is because the tests check for a match on a subset of the existing error message: "expected non-function type" still exists in the error message. Adding the note changes the number of messages, not just the content of the messages.

I'll see what I can come up with for the tests and improving the note message if you don't have any alternative suggestions.

[richard-uhler]

Remove note, add description for token types other than bare-id, and update
test cases that hit this case.

[rriddle]

mlir/lib/Parser/TypeParser.cpp

315

This makes this message inconsistent w.r.t all of the other "expected" messages.

[richard-uhler]

mlir/lib/Parser/TypeParser.cpp

317

I took a slightly different approach. What do you think about it now?

mlir/lib/Parser/TypeParser.cpp

 ///   index-type ::= `index`
 ///   float-type ::= `f16` | `bf16` | `f32` | `f64`
 ///   none-type ::= `none`
 ///
 Type Parser::parseNonFunctionType() {
   switch (getToken().getKind()) {
   default:
     return (emitError("expected non-function type"), nullptr);
+  case Token::bare_identifier:
+    return (emitError("expected non-function type,"
+                      " but found start of custom operation."
+                      " Did you mean to prefix the identifier with '!'"
+                      " to refer to a dialect type?"),
+            nullptr);
   case Token::kw_memref:
     return parseMemRefType();
   case Token::kw_tensor:
     return parseTensorType();
   case Token::kw_complex:
     return parseComplexType();
   case Token::kw_tuple:
     return parseTupleType();