From: Markus Armbruster <armbru@redhat.com>
To: John Snow <jsnow@redhat.com>
Cc: Michael Roth <michael.roth@amd.com>,
Cleber Rosa <crosa@redhat.com>,
qemu-devel@nongnu.org, Eduardo Habkost <ehabkost@redhat.com>
Subject: Re: [PATCH 10/22] qapi/parser: Fix typing of token membership tests
Date: Tue, 27 Apr 2021 09:00:38 +0200 [thread overview]
Message-ID: <87k0oo6wkp.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <78cf87ce-ce02-d9d6-0922-84a328b6b9da@redhat.com> (John Snow's message of "Mon, 26 Apr 2021 13:51:34 -0400")
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.
next prev parent reply other threads:[~2021-04-27 7:02 UTC|newest]
Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-22 3:06 [PATCH 00/22] qapi: static typing conversion, pt5a John Snow
2021-04-22 3:06 ` [PATCH 01/22] qapi/parser: Don't try to handle file errors John Snow
2021-04-23 15:46 ` Markus Armbruster
2021-04-23 19:20 ` John Snow
2021-04-27 13:47 ` Markus Armbruster
2021-04-27 17:58 ` John Snow
2021-04-28 5:48 ` Markus Armbruster
2021-04-22 3:07 ` [PATCH 02/22] qapi/source: [RFC] add "with_column" contextmanager John Snow
2021-04-27 9:33 ` Markus Armbruster
2021-04-22 3:07 ` [PATCH 03/22] qapi/source: Remove line number from QAPISourceInfo initializer John Snow
2021-04-24 6:38 ` Markus Armbruster
2021-04-26 17:39 ` John Snow
2021-04-26 23:14 ` John Snow
2021-04-27 6:07 ` Markus Armbruster
2021-04-22 3:07 ` [PATCH 04/22] qapi/parser: factor parsing routine into method John Snow
2021-04-22 3:07 ` [PATCH 05/22] qapi/parser: Assert lexer value is a string John Snow
2021-04-24 8:33 ` Markus Armbruster
2021-04-26 17:43 ` John Snow
2021-04-27 12:30 ` Markus Armbruster
2021-04-27 13:58 ` John Snow
2021-04-22 3:07 ` [PATCH 06/22] qapi/parser: assert get_expr returns object in outer loop John Snow
2021-04-25 7:23 ` Markus Armbruster
2021-04-27 15:03 ` John Snow
2021-04-22 3:07 ` [PATCH 07/22] qapi/parser: assert object keys are strings John Snow
2021-04-25 7:27 ` Markus Armbruster
2021-04-26 17:46 ` John Snow
2021-04-27 6:13 ` Markus Armbruster
2021-04-27 14:15 ` John Snow
2021-04-22 3:07 ` [PATCH 08/22] qapi/parser: Use @staticmethod where appropriate John Snow
2021-04-22 3:07 ` [PATCH 09/22] qapi: add match_nofail helper John Snow
2021-04-25 7:54 ` Markus Armbruster
2021-04-26 17:48 ` John Snow
2021-04-22 3:07 ` [PATCH 10/22] qapi/parser: Fix typing of token membership tests John Snow
2021-04-25 7:59 ` Markus Armbruster
2021-04-26 17:51 ` John Snow
2021-04-27 7:00 ` Markus Armbruster [this message]
2021-05-04 1:01 ` John Snow
2021-05-05 6:29 ` Markus Armbruster
2021-04-22 3:07 ` [PATCH 11/22] qapi/parser: Rework _check_pragma_list_of_str as a TypeGuard John Snow
2021-04-25 12:32 ` Markus Armbruster
2021-04-26 23:48 ` John Snow
2021-04-27 7:15 ` Markus Armbruster
2021-05-05 19:09 ` John Snow
2021-04-22 3:07 ` [PATCH 12/22] qapi/parser: add type hint annotations John Snow
2021-04-25 12:34 ` Markus Armbruster
2021-04-26 18:00 ` John Snow
2021-04-27 8:21 ` Markus Armbruster
2021-04-26 23:55 ` John Snow
2021-04-27 8:43 ` Markus Armbruster
2021-05-06 1:49 ` John Snow
2021-05-06 1:27 ` John Snow
2021-04-22 3:07 ` [PATCH 13/22] qapi/parser: [RFC] overload the return type of get_expr John Snow
2021-04-22 3:07 ` [PATCH 14/22] qapi/parser: Remove superfluous list constructor John Snow
2021-04-22 3:07 ` [PATCH 15/22] qapi/parser: allow 'ch' variable name John Snow
2021-04-22 3:07 ` [PATCH 16/22] qapi/parser: add docstrings John Snow
2021-04-25 13:27 ` Markus Armbruster
2021-04-26 18:26 ` John Snow
2021-04-27 9:03 ` Markus Armbruster
2021-05-06 2:08 ` John Snow
2021-05-07 1:34 ` John Snow
2021-05-07 8:25 ` Markus Armbruster
2021-04-22 3:07 ` [PATCH 17/22] CHECKPOINT John Snow
2021-04-22 3:07 ` [PATCH 18/22] qapi: [WIP] Rip QAPIDoc out of parser.py John Snow
2021-04-22 3:07 ` [PATCH 19/22] qapi: [WIP] Add type ignores for qapidoc.py John Snow
2021-04-22 3:07 ` [PATCH 20/22] qapi: [WIP] Import QAPIDoc from qapidoc Signed-off-by: John Snow <jsnow@redhat.com> John Snow
2021-04-22 3:07 ` [PATCH 21/22] qapi: [WIP] Add QAPIDocError John Snow
2021-04-22 3:07 ` [PATCH 22/22] qapi: [WIP] Enable linters on parser.py John Snow
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87k0oo6wkp.fsf@dusky.pond.sub.org \
--to=armbru@redhat.com \
--cc=crosa@redhat.com \
--cc=ehabkost@redhat.com \
--cc=jsnow@redhat.com \
--cc=michael.roth@amd.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).