-
Notifications
You must be signed in to change notification settings - Fork 79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core: Improve error message for unknown block successor #3762
base: main
Are you sure you want to change the base?
Conversation
"test.op"() [^unknown_successor]: () -> () | ||
} | ||
|
||
// CHECK: Unknown location of span region ends with missing block declarations for block(s) unknown_successor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"span" is parser terminology, no? Same with "unknown location".
The block name could be in quotes - maybe have a look how we do it in other block references within error messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error message is definitely not correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't quite debug what's gone wrong but the error message should be at the end of the region and should point to each time a block is used without being defined.
@@ -100,7 +100,7 @@ def __init__( | |||
super().__init__(ParserState(MLIRLexer(Input(input, name))), ctx) | |||
self.ssa_values = dict() | |||
self.blocks = dict() | |||
self.forward_block_references = dict() | |||
self.forward_block_references = defaultdict(list) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this change do?
@@ -145,7 +145,7 @@ def parse_module(self, allow_implicit_module: bool = True) -> ModuleOp: | |||
value_names = ", ".join( | |||
"%" + name for name in self.forward_ssa_references.keys() | |||
) | |||
if len(self.forward_block_references.keys()) > 1: | |||
if len(self.forward_ssa_references.keys()) > 1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a separate change but I don't mind including it here.
@@ -564,7 +564,7 @@ def parse_optional_region( | |||
region.add_block(block) | |||
|
|||
# Finally, check that all forward block references have been resolved. | |||
if len(self.forward_block_references) > 0: | |||
if self.forward_block_references: | |||
pos = self.lexer.pos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be self.pos
?
Make the error message more specific where an unknown successor is encountered.