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

[Markus Armbruster <armbru@redhat.com>]

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.

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

* 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")
                 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.

All observations, no demands.