On 4/25/21 3:23 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> get_expr can return many things, depending on where it is used. In the
>> outer parsing loop, we expect and require it to return a dict.
>>
>> (It's (maybe) a bit involved to teach mypy that when nested is False,
>> this is already always True. I'll look into it later, maybe.)
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qapi/parser.py | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>> index c75434e75a5..6b443b1247e 100644
>> --- a/scripts/qapi/parser.py
>> +++ b/scripts/qapi/parser.py
>> @@ -78,6 +78,8 @@ def _parse(self):
>> continue
>>
>> expr = self.get_expr(False)
>> + assert isinstance(expr, dict) # Guaranteed when nested=False
>> +
>> if 'include' in expr:
>> self.reject_expr_doc(cur_doc)
>> if len(expr) != 1:
>> @@ -278,6 +280,7 @@ def get_values(self):
>> self.accept()
>>
>> def get_expr(self, nested):
>> + # TODO: Teach mypy that nested=False means the retval is a Dict.
>> if self.tok != '{' and not nested:
>> raise QAPIParseError(self, "expected '{'")
>> if self.tok == '{':
>
> The better place to assert a post condition would be ...
>
> self.accept()
> expr = self.get_members()
> elif self.tok == '[':
> self.accept()
> expr = self.get_values()
> elif self.tok in "'tf":
> expr = self.val
> self.accept()
> else:
> raise QAPIParseError(
> self, "expected '{', '[', string, or boolean")
>
> ... here.
>
> return expr
>
> But then it may not help mypy over the hump, which is the whole point of
> the patch.
>
Right, the problem is that 'expr' here actually doesn't have to be a
Dict. It can be a List, str, or bool too.
The type narrowing occurs only when you pass nested=False.
> Alternative ways to skin this cat:
>
> * Split get_object() off get_expr().
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index ca5e8e18e0..c79b3c7d08 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -262,9 +262,12 @@ def get_values(self):
> raise QAPIParseError(self, "expected ',' or ']'")
> self.accept()
>
> - def get_expr(self, nested):
> - if self.tok != '{' and not nested:
> + def get_object(self):
> + if self.tok != '{':
> raise QAPIParseError(self, "expected '{'")
> + return self.get_expr()
> +
> + def get_expr(self):
> if self.tok == '{':
> self.accept()
> expr = self.get_members()
>
That'd work well. no @overload.
> * Shift "top-level expression must be dict" up:
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index ca5e8e18e0..ee8cbf3531 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -68,7 +68,10 @@ def __init__(self, fname, previously_included=None, incl_info=None):
> self.docs.append(cur_doc)
> continue
>
> - expr = self.get_expr(False)
> + expr = self.get_expr()
> + if not isinstance(expr, OrderedDict):
> + raise QAPISemError(
> + info, "top-level expression must be an object")
Also works. As a benefit (to both previous suggestions), it leaves
get_expr completely generic and expresses the grammatical constraint up
here in the parseloop. It leaves the JSON parsing more generic and
further consolidates QAPI Schema specific stuff to this region.
> if 'include' in expr:
> self.reject_expr_doc(cur_doc)
> if len(expr) != 1:
> @@ -262,9 +265,7 @@ def get_values(self):
> raise QAPIParseError(self, "expected ',' or ']'")
> self.accept()
>
> - def get_expr(self, nested):
> - if self.tok != '{' and not nested:
> - raise QAPIParseError(self, "expected '{'")
> + def get_expr(self):
> if self.tok == '{':
> self.accept()
> expr = self.get_members()
>
> * Shift it further, into expr.py:
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 496f7e0333..0a83c493a0 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -600,7 +600,10 @@ def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
> """
> for expr_elem in exprs:
> # Expression
> - assert isinstance(expr_elem['expr'], dict)
> + if not isinstance(expr_elem['expr'], dict):
> + raise QAPISemError(
> + info, "top-level expression must be an object")
> +
> for key in expr_elem['expr'].keys():
> assert isinstance(key, str)
> expr: _JSONObject = expr_elem['expr']
>
> Shifting it up would be closer to qapi-code-gen.txt than what we have
> now.
>
This is also pretty nice, as it furthers the splitting of the JSON
syntax from the abstract QAPI syntax, which is a distinct end-goal I have.
A slight downside is that the type of a value now needs to follow
outside of parser.py, which will warrant a type name.
> All observations, no demands.
>