Re: [PATCH 12/22] qapi/parser: add type hint annotations

[John Snow]

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