Page MenuHomePhabricator

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

Authored by richard-uhler on Sep 30 2020, 1:10 PM.

Details

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.

Diff Detail

Event Timeline

richard-uhler created this revision.Sep 30 2020, 1:10 PM
Herald added a project: Restricted Project. · View Herald Transcript
richard-uhler requested review of this revision.Sep 30 2020, 1:10 PM
mehdi_amini added inline comments.Oct 3 2020, 10:49 AM
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.

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.

richard-uhler marked an inline comment as done.Oct 5 2020, 10:07 AM

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

rriddle accepted this revision.Oct 5 2020, 1:58 PM

LGTM after resolving comments.

mlir/lib/Parser/TypeParser.cpp
317

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

This revision is now accepted and ready to land.Oct 5 2020, 1:58 PM
mehdi_amini added inline comments.Oct 5 2020, 2:15 PM
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 added inline comments.Oct 6 2020, 11:09 AM
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 added inline comments.Oct 6 2020, 11:35 AM
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 added inline comments.Oct 6 2020, 4:08 PM
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 retitled this revision from Help user more if they forget to use ! for a dialect type. to Add more context to 'expected non-function type' error message..
richard-uhler edited the summary of this revision. (Show Details)

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

rriddle requested changes to this revision.Oct 7 2020, 11:53 AM
rriddle added inline comments.
mlir/lib/Parser/TypeParser.cpp
315

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

This revision now requires changes to proceed.Oct 7 2020, 11:53 AM
richard-uhler marked 3 inline comments as done.Oct 7 2020, 11:53 AM
richard-uhler added inline comments.
mlir/lib/Parser/TypeParser.cpp
317

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