From: John Snow <jsnow@redhat.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: Michael Roth <michael.roth@amd.com>,
qemu-devel@nongnu.org, Eduardo Habkost <ehabkost@redhat.com>,
Cleber Rosa <crosa@redhat.com>
Subject: Re: [PATCH 12/22] qapi/parser: add type hint annotations
Date: Mon, 26 Apr 2021 14:00:34 -0400 [thread overview]
Message-ID: <b19c350f-19f0-9f68-7531-d0a1db8e410e@redhat.com> (raw)
In-Reply-To: <87zgxm5yrp.fsf@dusky.pond.sub.org>
On 4/25/21 8:34 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Annotations do not change runtime behavior.
>> This commit *only* adds annotations.
>>
>> (Annotations for QAPIDoc are in a later commit.)
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qapi/parser.py | 61 ++++++++++++++++++++++++++++--------------
>> 1 file changed, 41 insertions(+), 20 deletions(-)
>>
>> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>> index d02a134aae9..f2b57d5642a 100644
>> --- a/scripts/qapi/parser.py
>> +++ b/scripts/qapi/parser.py
>> @@ -17,16 +17,29 @@
>> from collections import OrderedDict
>> import os
>> import re
>> -from typing import List
>> +from typing import (
>> + Dict,
>> + List,
>> + Optional,
>> + Set,
>> + Union,
>> +)
>>
>> from .common import match_nofail
>> from .error import QAPISemError, QAPISourceError
>> from .source import QAPISourceInfo
>>
>>
>> +#: Represents a parsed JSON object; semantically: one QAPI schema expression.
>> +Expression = Dict[str, object]
>
> I believe you use this for what qapi-code-gen.txt calls a top-level
> expression. TopLevelExpression is rather long, but it's used just once,
> and once more in RFC PATCH 13. What do you think?
>
Yeah, I left a comment on gitlab about this -- Sorry for splitting the
stream, I didn't expect you to reply on-list without at least clicking
the link ;)
You're right, this is TOP LEVEL EXPR. I actually do mean to start using
it in expr.py as well too, in what will become (I think) pt5c:
non-immediately-necessary parser cleanups.
I can use TopLevelExpression as the type name if you'd like, but if you
have a suggestion for something shorter I am open to suggestions if
"Expression" is way too overloaded/confusing.
>> +
>> +# Return value alias for get_expr().
>> +_ExprValue = Union[List[object], Dict[str, object], str, bool]
>
> This is essentially a node in our pidgin-JSON parser's abstract syntax
> tree. Tree roots use the Dict branch of this Union.
>
> See also my review of PATCH 06.
>
OK, I skimmed that one for now but I'll get back to it.
>> +
>> +
>> class QAPIParseError(QAPISourceError):
>> """Error class for all QAPI schema parsing errors."""
>> - def __init__(self, parser, msg):
>> + def __init__(self, parser: 'QAPISchemaParser', msg: str):
>
> Forward reference needs quotes. Can't be helped.
>
>> col = 1
>> for ch in parser.src[parser.line_pos:parser.pos]:
>> if ch == '\t':
>> @@ -38,7 +51,10 @@ def __init__(self, parser, msg):
>>
>> class QAPISchemaParser:
>>
>> - def __init__(self, fname, previously_included=None, incl_info=None):
>> + def __init__(self,
>> + fname: str,
>> + previously_included: Optional[Set[str]] = None,
>
> This needs to be Optional[] because using the empty set as default
> parameter value would be a dangerous trap. Python's choice to evaluate
> the default parameter value just once has always been iffy. Stirring
> static typing into the language makes it iffier. Can't be helped.
>
We could force it to accept a tuple and convert it into a set
internally. It's just that we seem to use it for sets now.
Or ... in pt5c I float the idea of just passing the parent parser in,
and I reach up and grab the previously-included stuff directly.
>> + incl_info: Optional[QAPISourceInfo] = None):
>> self._fname = fname
>> self._included = previously_included or set()
>> self._included.add(os.path.abspath(self._fname))
>> @@ -46,20 +62,20 @@ def __init__(self, fname, previously_included=None, incl_info=None):
>>
>> # Lexer state (see `accept` for details):
>> self.info = QAPISourceInfo(self._fname, incl_info)
>> - self.tok = None
>> + self.tok: Optional[str] = None
>
> Would
>
> self.tok: str
>
> work?
>
Not without modifications, because the Token being None is used to
represent EOF.
>> self.pos = 0
>> self.cursor = 0
>> - self.val = None
>> + self.val: Optional[Union[bool, str]] = None
>> self.line_pos = 0
>>
>> # Parser output:
>> - self.exprs = []
>> - self.docs = []
>> + self.exprs: List[Expression] = []
>> + self.docs: List[QAPIDoc] = []
>>
>> # Showtime!
>> self._parse()
>>
>> - def _parse(self):
>> + def _parse(self) -> None:
>> cur_doc = None
>>
>> with open(self._fname, 'r', encoding='utf-8') as fp:
>> @@ -122,7 +138,7 @@ def _parse(self):
>> self.reject_expr_doc(cur_doc)
>>
>> @staticmethod
>> - def reject_expr_doc(doc):
>> + def reject_expr_doc(doc: Optional['QAPIDoc']) -> None:
>> if doc and doc.symbol:
>> raise QAPISemError(
>> doc.info,
>> @@ -130,10 +146,14 @@ def reject_expr_doc(doc):
>> % doc.symbol)
>>
>> @staticmethod
>> - def _include(include, info, incl_fname, previously_included):
>> + def _include(include: str,
>> + info: QAPISourceInfo,
>> + incl_fname: str,
>> + previously_included: Set[str]
>> + ) -> Optional['QAPISchemaParser']:
>> incl_abs_fname = os.path.abspath(incl_fname)
>> # catch inclusion cycle
>> - inf = info
>> + inf: Optional[QAPISourceInfo] = info
>> while inf:
>> if incl_abs_fname == os.path.abspath(inf.fname):
>> raise QAPISemError(info, "inclusion loop for %s" % include)
>> @@ -152,9 +172,9 @@ def _include(include, info, incl_fname, previously_included):
>> ) from err
>>
>> @staticmethod
>> - def _pragma(name, value, info):
>> + def _pragma(name: str, value: object, info: QAPISourceInfo) -> None:
>
> value: object isn't wrong, but why not _ExprValue?
>
I forget; admit this one slipped through from an earlier revision.
Right now: because _ExprValue is too broad. It really wants Dict[str,
object] but the typing on get_expr() is challenging.
I'll revisit this with better excuses after I digest your patch 6 review.
>>
>> - def _check(name, value) -> List[str]:
>> + def _check(name: str, value: object) -> List[str]:
>> if (not isinstance(value, list) or
>> any([not isinstance(elt, str) for elt in value])):
>> raise QAPISemError(
>> @@ -176,7 +196,7 @@ def _check(name, value) -> List[str]:
>> else:
>> raise QAPISemError(info, "unknown pragma '%s'" % name)
>>
>> - def accept(self, skip_comment=True):
>> + def accept(self, skip_comment: bool = True) -> None:
>> while True:
>> self.tok = self.src[self.cursor]
>> self.pos = self.cursor
>> @@ -240,8 +260,8 @@ def accept(self, skip_comment=True):
>> self.src[self.cursor-1:])
>> raise QAPIParseError(self, "stray '%s'" % match.group(0))
>>
>> - def get_members(self):
>> - expr = OrderedDict()
>> + def get_members(self) -> 'OrderedDict[str, object]':
>> + expr: 'OrderedDict[str, object]' = OrderedDict()
>> if self.tok == '}':
>> self.accept()
>> return expr
>> @@ -267,8 +287,8 @@ def get_members(self):
>> if self.tok != "'":
>> raise QAPIParseError(self, "expected string")
>>
>> - def get_values(self):
>> - expr = []
>> + def get_values(self) -> List[object]:
>> + expr: List[object] = []
>> if self.tok == ']':
>> self.accept()
>> return expr
>> @@ -284,8 +304,9 @@ def get_values(self):
>> raise QAPIParseError(self, "expected ',' or ']'")
>> self.accept()
>>
>> - def get_expr(self, nested):
>> + def get_expr(self, nested: bool = False) -> _ExprValue:
>> # TODO: Teach mypy that nested=False means the retval is a Dict.
>> + expr: _ExprValue
>> if self.tok != '{' and not nested:
>> raise QAPIParseError(self, "expected '{'")
>> if self.tok == '{':
>> @@ -303,7 +324,7 @@ def get_expr(self, nested):
>> self, "expected '{', '[', string, or boolean")
>> return expr
>>
>> - def get_doc(self, info):
>> + def get_doc(self, info: QAPISourceInfo) -> List['QAPIDoc']:
>> if self.val != '##':
>> raise QAPIParseError(
>> self, "junk after '##' at start of documentation comment")
Thanks!
--js
next prev parent reply other threads:[~2021-04-26 18: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
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 [this message]
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=b19c350f-19f0-9f68-7531-d0a1db8e410e@redhat.com \
--to=jsnow@redhat.com \
--cc=armbru@redhat.com \
--cc=crosa@redhat.com \
--cc=ehabkost@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).