| Message ID | 20210422030720.3685766-6-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: static typing conversion, pt5a | expand |
John Snow <jsnow@redhat.com> writes: > The type checker can't narrow the type of the token value to string, > because it's only loosely correlated with the return token. > > We know that a token of '#' should always have a "str" value. > Add an assertion. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/parser.py | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py > index f519518075e..c75434e75a5 100644 > --- a/scripts/qapi/parser.py > +++ b/scripts/qapi/parser.py > @@ -303,6 +303,7 @@ def get_doc(self, info): > cur_doc = QAPIDoc(self, info) > self.accept(False) > while self.tok == '#': > + assert isinstance(self.val, str), "Expected str value" > if self.val.startswith('##'): > # End of doc comment > if self.val != '##': The second operand of assert provides no additional information. Please drop it.
On 4/24/21 4:33 AM, Markus Armbruster wrote: > The second operand of assert provides no additional information. Please > drop it. I don't agree with "no additional information", strictly. I left you a comment on gitlab before you started reviewing on-list. What I wrote there: "Markus: I know you're not a fan of these, but I wanted a suggestion on how to explain why this must be true in case it wasn't obvious to someone else in the future." --js
John Snow <jsnow@redhat.com> writes: > On 4/24/21 4:33 AM, Markus Armbruster wrote: >> The second operand of assert provides no additional information. Please >> drop it. > > I don't agree with "no additional information", strictly. > > I left you a comment on gitlab before you started reviewing on-list. > What I wrote there: > > "Markus: I know you're not a fan of these, but I wanted a suggestion on > how to explain why this must be true in case it wasn't obvious to > someone else in the future." But the second operand doesn't explain anything. Look: diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index f519518075e..c75434e75a5 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -303,6 +303,7 @@ def get_doc(self, info): cur_doc = QAPIDoc(self, info) self.accept(False) while self.tok == '#': + assert isinstance(self.val, str), "Expected str value" if self.val.startswith('##'): # End of doc comment if self.val != '##': The second operand paraphrases the first one in prose rather than code. An actual *explanation* would instead tell me why the first operand must be true. To do that, I'd point to self.accept()'s postcondition. Which (informally) is self.tok in ('#', '{', ... ) and self.tok == '#' implies self.val is a str and self.tok == '{' implies self.val is None ... I believe this is required working knowledge for understanding the parser. Your PATCH 16 puts it in a doc string, so readers don't have to extract it from code. Makes sense. It's not going to fit into a workable second operand here, I'm afraid. I assume you need this assertion for mypy. If yes, let's get the job done with minimal fuss. If no, please drop the assertion entirely.
On 4/27/21 8:30 AM, Markus Armbruster wrote: > I assume you need this assertion for mypy. If yes, let's get the job > done with minimal fuss. If no, please drop the assertion entirely. Yep, needed for mypy. You are right that these assertions are for clarifying postconditions of accept() that tie together the value of .tok with the type of .val. I'll replace the message with a better comment, but we do still need either: (1) A way to make the return from accept() statically type-safe, or (2) The assertion. As with most of the patches after part one of this series, I've opted for the quicker thing to speed us along to a clean mypy baseline. (Though I have spent some time prototyping solutions for #1...) --js
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index f519518075e..c75434e75a5 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -303,6 +303,7 @@ def get_doc(self, info): cur_doc = QAPIDoc(self, info) self.accept(False) while self.tok == '#': + assert isinstance(self.val, str), "Expected str value" if self.val.startswith('##'): # End of doc comment if self.val != '##':
The type checker can't narrow the type of the token value to string, because it's only loosely correlated with the return token. We know that a token of '#' should always have a "str" value. Add an assertion. Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/parser.py | 1 + 1 file changed, 1 insertion(+)