Re: [PATCH v2 5/7] qapi/parser: [RFC] add QAPIExpression

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Wed, Feb 8, 2023 at 11:28 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > The idea here is to combine 'TopLevelExpr' and 'ParsedExpression' into
>> > one type that accomplishes the purposes of both types;
>> >
>> > 1. TopLevelExpr is meant to represent a JSON Object, but only those that
>> > represent what qapi-schema calls a TOP-LEVEL-EXPR, i.e. definitions,
>> > pragmas, and includes.
>> >
>> > 2. ParsedExpression is meant to represent a container around the above
>> > type, alongside QAPI-specific metadata -- the QAPISourceInfo and QAPIDoc
>> > objects.
>> >
>> > We can actually just roll these up into one type: A python mapping that
>> > has the metadata embedded directly inside of it.
>>
>> On the general idea: yes, please!  Gets rid of "TopLevelExpr and
>> _JSONObject are the same, except semantically, but nothing checks that",
>> which I never liked.
>
> I prefer them to be checked/enforced too; I'll admit to trying to do
> "a little less" to try and invoke less magic, especially in Python
> 3.6. The best magic comes in later versions.
>
>>
>> > NB: This necessitates a change of typing for check_if() and
>> > check_keys(), because mypy does not believe UserDict[str, object] ⊆
>> > Dict[str, object]. It will, however, accept Mapping or
>> > MutableMapping. In this case, the immutable form is preferred as an
>> > input parameter because we don't actually mutate the input.
>> >
>> > Without this change, we will observe:
>> > qapi/expr.py:631: error: Argument 1 to "check_keys" has incompatible
>> > type "QAPIExpression"; expected "Dict[str, object]"
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>

[...]

>> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>> > index f897dffbfd4..88f6fdfa67b 100644
>> > --- a/scripts/qapi/parser.py
>> > +++ b/scripts/qapi/parser.py
>> > @@ -14,14 +14,14 @@
>> >  # This work is licensed under the terms of the GNU GPL, version 2.
>> >  # See the COPYING file in the top-level directory.
>> >
>> > -from collections import OrderedDict
>> > +from collections import OrderedDict, UserDict
>> >  import os
>> >  import re
>> >  from typing import (
>> >      TYPE_CHECKING,
>> >      Dict,
>> >      List,
>> > -    NamedTuple,
>> > +    Mapping,
>> >      Optional,
>> >      Set,
>> >      Union,
>> > @@ -38,21 +38,32 @@
>> >      from .schema import QAPISchemaFeature, QAPISchemaMember
>> >
>> >
>> > -#: Represents a single Top Level QAPI schema expression.
>> > -TopLevelExpr = Dict[str, object]
>> > -
>> >  # Return value alias for get_expr().
>> >  _ExprValue = Union[List[object], Dict[str, object], str, bool]
>> >
>> > -# FIXME: Consolidate and centralize definitions for TopLevelExpr,
>> > -# _ExprValue, _JSONValue, and _JSONObject; currently scattered across
>> > -# several modules.
>> >
>> > +# FIXME: Consolidate and centralize definitions for _ExprValue,
>> > +# JSONValue, and _JSONObject; currently scattered across several
>> > +# modules.
>> >
>> > -class ParsedExpression(NamedTuple):
>> > -    expr: TopLevelExpr
>> > -    info: QAPISourceInfo
>> > -    doc: Optional['QAPIDoc']
>> > +
>> > +# 3.6 workaround: can be removed when Python 3.7+ is our required version.
>> > +if TYPE_CHECKING:
>> > +    _UserDict = UserDict[str, object]
>> > +else:
>> > +    _UserDict = UserDict
>>
>> Worth mentioning in the commit message?  Genuine question; I'm not sure
>> :)
>>
>
> If you please! My only consideration here was leaving a comment with
> both "3.6" and "3.7" so that when I git grep to upgrade from 3.6 to
> 3.7, there's a shining spotlight on this particular wart.
>
> The problem here is that Python 3.6 does not believe that you can
> subscript UserDict, because UserDict is not generic in its
> *implementation*, it's only generic in its type stub. Short-sighted
> problem that was corrected for 3.7; here's a bug filed by Papa Guido
> heself: https://github.com/python/typing/issues/60
>
> (This bug is where I found this workaround from.)
>
>> > +
>> > +
>> > +class QAPIExpression(_UserDict):
>> > +    def __init__(
>> > +        self,
>> > +        initialdata: Mapping[str, object],
>>
>> I'd prefer to separate words: initial_data.
>>
>
> Wasn't my choice:
> https://docs.python.org/3/library/collections.html#collections.UserDict

Alright then.

>> > +        info: QAPISourceInfo,
>> > +        doc: Optional['QAPIDoc'] = None,
>> > +    ):
>> > +        super().__init__(initialdata)
>> > +        self.info = info
>> > +        self.doc: Optional['QAPIDoc'] = doc
>> >
>> >
>> >  class QAPIParseError(QAPISourceError):

[...]

>> > @@ -1139,52 +1143,49 @@ def _def_command(self, expr, info, doc):
>> >          allow_preconfig = expr.get('allow-preconfig', False)
>> >          coroutine = expr.get('coroutine', False)
>> >          ifcond = QAPISchemaIfCond(expr.get('if'))
>> > -        features = self._make_features(expr.get('features'), info)
>> > +        features = self._make_features(expr.get('features'), expr.info)
>> >          if isinstance(data, OrderedDict):
>> >              data = self._make_implicit_object_type(
>> > -                name, info, ifcond,
>> > -                'arg', self._make_members(data, info))
>> > +                name, expr.info, ifcond,
>> > +                'arg', self._make_members(data, expr.info))
>> >          if isinstance(rets, list):
>> >              assert len(rets) == 1
>> > -            rets = self._make_array_type(rets[0], info)
>> > -        self._def_entity(QAPISchemaCommand(name, info, doc, ifcond, 
>> > features,
>> > -                                           data, rets,
>> > +            rets = self._make_array_type(rets[0], expr.info)
>> > +        self._def_entity(QAPISchemaCommand(name, expr.info, expr.doc, 
>> > ifcond,
>> > +                                           features, data, rets,
>> >                                             gen, success_response,
>> >                                             boxed, allow_oob, 
>> > allow_preconfig,
>> >                                             coroutine))
>> >
>> > -    def _def_event(self, expr, info, doc):
>> > +    def _def_event(self, expr: QAPIExpression):
>> >          name = expr['event']
>> >          data = expr.get('data')
>> >          boxed = expr.get('boxed', False)
>> >          ifcond = QAPISchemaIfCond(expr.get('if'))
>> > -        features = self._make_features(expr.get('features'), info)
>> > +        features = self._make_features(expr.get('features'), expr.info)
>> >          if isinstance(data, OrderedDict):
>> >              data = self._make_implicit_object_type(
>> > -                name, info, ifcond,
>> > -                'arg', self._make_members(data, info))
>> > -        self._def_entity(QAPISchemaEvent(name, info, doc, ifcond, 
>> > features,
>> > -                                         data, boxed))
>> > +                name, expr.info, ifcond,
>> > +                'arg', self._make_members(data, expr.info))
>> > +        self._def_entity(QAPISchemaEvent(name, expr.info, expr.doc, 
>> > ifcond,
>> > +                                         features, data, boxed))
>> >
>> >      def _def_exprs(self, exprs):
>> > -        for expr_elem in exprs:
>> > -            expr = expr_elem.expr
>> > -            info = expr_elem.info
>> > -            doc = expr_elem.doc
>> > +        for expr in exprs:
>> >              if 'enum' in expr:
>> > -                self._def_enum_type(expr, info, doc)
>> > +                self._def_enum_type(expr)
>> >              elif 'struct' in expr:
>> > -                self._def_struct_type(expr, info, doc)
>> > +                self._def_struct_type(expr)
>> >              elif 'union' in expr:
>> > -                self._def_union_type(expr, info, doc)
>> > +                self._def_union_type(expr)
>> >              elif 'alternate' in expr:
>> > -                self._def_alternate_type(expr, info, doc)
>> > +                self._def_alternate_type(expr)
>> >              elif 'command' in expr:
>> > -                self._def_command(expr, info, doc)
>> > +                self._def_command(expr)
>> >              elif 'event' in expr:
>> > -                self._def_event(expr, info, doc)
>> > +                self._def_event(expr)
>> >              elif 'include' in expr:
>> > -                self._def_include(expr, info, doc)
>> > +                self._def_include(expr)
>> >              else:
>> >                  assert False
>>
>> The insertion of expr. makes the patch a bit tiresome to review.  I
>> only skimmed it for now.
>
> There's indeed a lot of mechanical churn. It appears to work via
> testing; both make check and the full CI job.
> "It compiles, how wrong could it be!?"
>
> *ducks*

A few local variables could reduce churn.  Not sure we want them in the
final code, though.  Use your judgement.