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 != '##':
-- 
2.30.2

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