core: Improve error message for unknown block successor #3762
Conversation
compor
changed the title
| "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.
"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.
This error message is definitely not correct.
There was a problem hiding this comment.
why is it a problem to use parser terminology here (since it's a parse error)?
There was a problem hiding this comment.
how would I know what the correct error might be? (this one is the one that's currently being printed when the code is run) - or is this the debugging you refer to in the next comment, that is proving difficult?
| @@ -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.
What does this change do?
There was a problem hiding this comment.
it creates automatic values for non-existent keys so you don't get a key error
There was a problem hiding this comment.
This seems reasonably now that I understand the code a bit more.
| @@ -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.
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.
This should probably be self.pos?
|
@emmau678 will you have time this week to address the comments on this PR? |
I will attempt it today, I’m finding it a bit hard to identify how to fix it though. |
|
Please message with questions if anything isn't clear, it's much better to iterate quickly, otherwise we don't know if our comment was too vague. It would be great to reply to the comments with either the answer or a clarifying question, for example. |
| @@ -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 | |||
| raise MultipleSpansParseError( | |||
| Span(pos, pos + 1, self.lexer.input), | |||
There was a problem hiding this comment.
@emmau678 This is the error message that should be triggered, and it doesn't line up with the error message being printed. An actual span should be printed instead of "Unknown location" and there should be the following messages about "dangling block references:"
There was a problem hiding this comment.
ok, thank you @alexarice. I will make some updates this afternoon (I am mostly busy until then)
Make the error message more specific where an unknown successor is encountered.