[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 10/22] qapi/parser: Fix typing of token membership tests
From: |
Markus Armbruster |
Subject: |
Re: [PATCH 10/22] qapi/parser: Fix typing of token membership tests |
Date: |
Tue, 27 Apr 2021 09:00:38 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
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.
- Re: [PATCH 11/22] qapi/parser: Rework _check_pragma_list_of_str as a TypeGuard, (continued)
[PATCH 15/22] qapi/parser: allow 'ch' variable name, John Snow, 2021/04/21
[PATCH 16/22] qapi/parser: add docstrings, John Snow, 2021/04/21
[PATCH 10/22] qapi/parser: Fix typing of token membership tests, John Snow, 2021/04/21
[PATCH 14/22] qapi/parser: Remove superfluous list constructor, John Snow, 2021/04/21
[PATCH 17/22] CHECKPOINT, John Snow, 2021/04/21
[PATCH 19/22] qapi: [WIP] Add type ignores for qapidoc.py, John Snow, 2021/04/21
[PATCH 20/22] qapi: [WIP] Import QAPIDoc from qapidoc Signed-off-by: John Snow <address@hidden>, John Snow, 2021/04/21
[PATCH 22/22] qapi: [WIP] Enable linters on parser.py, John Snow, 2021/04/21
[PATCH 21/22] qapi: [WIP] Add QAPIDocError, John Snow, 2021/04/21
[PATCH 13/22] qapi/parser: [RFC] overload the return type of get_expr, John Snow, 2021/04/21
[PATCH 18/22] qapi: [WIP] Rip QAPIDoc out of parser.py, John Snow, 2021/04/21