Re: [PATCH 06/22] qapi/parser: assert get_expr returns object in outer loop

[John Snow <jsnow@redhat.com>]

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