John Snow <js...@redhat.com> writes:
> On 4/25/21 8:34 AM, Markus Armbruster wrote:
>> John Snow <js...@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 <js...@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
[...]
