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

[John Snow]

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.