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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> 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 ;)

I should have; sorry about that.  I need to avoid distractions to stay
productive, and web browsers are basically gatling guns firing
armor-piercing distraction rounds at me.

> 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.

TopLevelExpr?  Matches qapi-code-gen.txt's grammar:

    SCHEMA = TOP-LEVEL-EXPR...

    TOP-LEVEL-EXPR = DIRECTIVE | DEFINITION

    DIRECTIVE = INCLUDE | PRAGMA
    DEFINITION = ENUM | STRUCT | UNION | ALTERNATE | COMMAND | EVENT

>>> +
>>> +# 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.

Another candidate: frozenset.

> 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.

True.  I missed that, and thought we'd need None just as an initial
value here.

>>>           self.pos = 0
>>>           self.cursor = 0
>>> -        self.val = None
>>> +        self.val: Optional[Union[bool, str]] = None
>>>           self.line_pos = 0

[...]