Re: [PATCH 10/22] qapi/parser: Fix typing of token membership tests

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> On 4/25/21 3:59 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> When the token can be None, we can't use 'x in "abc"' style membership
>>> tests to group types of tokens together, because 'None in "abc"' is a
>>> TypeError.
>>>
>>> Easy enough to fix, if not a little ugly.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   scripts/qapi/parser.py | 5 +++--
>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>>> index 7f3c009f64b..16fd36f8391 100644
>>> --- a/scripts/qapi/parser.py
>>> +++ b/scripts/qapi/parser.py
>>> @@ -272,7 +272,7 @@ def get_values(self):
>>>           if self.tok == ']':
>>>               self.accept()
>>>               return expr
>>> -        if self.tok not in "{['tf":
>>> +        if self.tok is None or self.tok not in "{['tf":
>>>               raise QAPIParseError(
>>>                   self, "expected '{', '[', ']', string, or boolean")
>>>           while True:
>>> @@ -294,7 +294,8 @@ def get_expr(self, nested):
>>>           elif self.tok == '[':
>>>               self.accept()
>>>               expr = self.get_values()
>>> -        elif self.tok in "'tf":
>>> +        elif self.tok and self.tok in "'tf":
>>> +            assert isinstance(self.val, (str, bool))
>>>               expr = self.val
>>>               self.accept()
>>>           else:
>> 
>> How can self.tok be None?
>> 
>> I suspect this is an artifact of PATCH 04.  Before, self.tok is
>> initialized to the first token, then set to subsequent tokens (all str)
>> in turn.  After, it's initialized to None, then set to tokens in turn.
>> 
>
> Actually, it's set to None to represent EOF. See here:
>
>              elif self.tok == '\n':
> 	        if self.cursor == len(self.src):
>                      self.tok = None
>                      return

Alright, then this is actually a bug fix:

    $ echo -n "{'key': " | python3 scripts/qapi-gen.py /dev/stdin
    Traceback (most recent call last):
      File "scripts/qapi-gen.py", line 19, in <module>
        sys.exit(main.main())
      File "/work/armbru/qemu/scripts/qapi/main.py", line 93, in main
        generate(args.schema,
      File "/work/armbru/qemu/scripts/qapi/main.py", line 50, in generate
        schema = QAPISchema(schema_file)
      File "/work/armbru/qemu/scripts/qapi/schema.py", line 852, in __init__
        parser = QAPISchemaParser(fname)
      File "/work/armbru/qemu/scripts/qapi/parser.py", line 59, in __init__
        self._parse()
      File "/work/armbru/qemu/scripts/qapi/parser.py", line 81, in _parse
        expr = self.get_expr(False)
      File "/work/armbru/qemu/scripts/qapi/parser.py", line 293, in get_expr
        expr = self.get_members()
      File "/work/armbru/qemu/scripts/qapi/parser.py", line 260, in get_members
        expr[key] = self.get_expr(True)
      File "/work/armbru/qemu/scripts/qapi/parser.py", line 297, in get_expr
        elif self.tok in "'tf":
    TypeError: 'in <string>' requires string as left operand, not NoneType

Likewise, the other hunk:

    $ echo -n "{'key': [" | python3 scripts/qapi-gen.py /dev/stdin
    Traceback (most recent call last):
      File "scripts/qapi-gen.py", line 19, in <module>
        sys.exit(main.main())
      File "/work/armbru/qemu/scripts/qapi/main.py", line 89, in main
        generate(args.schema,
      File "/work/armbru/qemu/scripts/qapi/main.py", line 51, in generate
        schema = QAPISchema(schema_file)
      File "/work/armbru/qemu/scripts/qapi/schema.py", line 860, in __init__
        parser = QAPISchemaParser(fname)
      File "/work/armbru/qemu/scripts/qapi/parser.py", line 71, in __init__
        expr = self.get_expr(False)
      File "/work/armbru/qemu/scripts/qapi/parser.py", line 270, in get_expr
        expr = self.get_members()
      File "/work/armbru/qemu/scripts/qapi/parser.py", line 238, in get_members
        expr[key] = self.get_expr(True)
      File "/work/armbru/qemu/scripts/qapi/parser.py", line 273, in get_expr
        expr = self.get_values()
      File "/work/armbru/qemu/scripts/qapi/parser.py", line 253, in get_values
        if self.tok not in "{['tf":
    TypeError: 'in <string>' requires string as left operand, not NoneType

Please add test cases.  I recommend adding them in a separate patch, so
this one's diff shows clearly what's being fixed.

There's a similar one in accept(), but it's safe:

            self.tok = self.src[self.cursor]
            ...
            elif self.tok in '{}:,[]':
                return

Regarding "if not a little ugly": instead of

    self.tok is None or self.tok not in "{['tf"

we could use

    self.tok not in tuple("{['tf")

> A more pythonic idiom would be to create a lexer class that behaves as 
> an iterator, yielding Token class objects, and eventually, raising 
> StopIteration.
>
> (Not suggesting I do that now. I have thought about it though, yes.)

Yes, let's resist the temptation to improve things into too many
directions at once.

Aside: using exceptions for perfectly unexceptional things like loop
termination is in questionable taste, but we gotta go with the Python
flow.