[PATCH v2 0/7] qapi: static typing conversion, pt5c

[PATCH v2 0/7] qapi: static typing conversion, pt5c

[John Snow]

This is part five (c), and focuses on sharing strict types between
parser.py and expr.py.

gitlab: https://gitlab.com/jsnow/qemu/-/commits/python-qapi-cleanup-pt5c

Every commit should pass with:
 - `isort -c qapi/`
 - `flake8 qapi/`
 - `pylint --rcfile=qapi/pylintrc qapi/`
 - `mypy --config-file=qapi/mypy.ini qapi/`

Some notes on this series:

Patches 2 and 3 are almost entirely superseded by patch 5, but I wasn't
as confident that Markus would like patch 5, so these patches aren't
squashed quite as tightly as they could be -- I recommend peeking ahead
at the cover letters before reviewing the actual patch diffs.

By the end of this series, the only JSON-y types we have left are:

(A) QAPIExpression,
(B) JSONValue,
(C) _ExprValue.

The argument I'm making here is that QAPIExpression and JSONValue are
distinct enough to warrant having both types (for now, at least); and
that _ExprValue is specialized enough to also warrant its inclusion.

(Brutal honesty: my attempts at unifying this even further had even more
hacks and unsatisfying conclusions, and fully unifying these types
should probably wait until we're allowed to rely on some fairly modern
Python versions.)

John Snow (7):
  qapi/expr: Split check_expr out from check_exprs
  qapi/parser.py: add ParsedExpression type
  qapi/expr: Use TopLevelExpr where appropriate
  qapi/expr: add typing workaround for AbstractSet
  qapi/parser: [RFC] add QAPIExpression
  qapi: remove _JSONObject
  qapi: remove JSON value FIXME

 scripts/qapi/expr.py   | 282 +++++++++++++++++++----------------------
 scripts/qapi/parser.py |  51 +++++---
 scripts/qapi/schema.py | 105 +++++++--------
 3 files changed, 218 insertions(+), 220 deletions(-)

-- 
2.39.0

[PATCH v2 1/7] qapi/expr: Split check_expr out from check_exprs

[John Snow]

Primarily, this reduces a nesting level of a particularly long
block. It's mostly code movement, but a new docstring is created.

It also has the effect of creating a fairly convenient "catch point" in
check_exprs for exception handling without making the nesting level even
worse.

Signed-off-by: John Snow <jsnow@redhat.com>

---

This patch was originally written as part of my effort to factor out
QAPISourceInfo from this file by having expr.py raise a simple
exception, then catch and wrap it at the higher level.

This series doesn't do that anymore, but reducing the nesting level
still seemed subjectively nice. It's not crucial.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/expr.py | 179 +++++++++++++++++++++++--------------------
 1 file changed, 95 insertions(+), 84 deletions(-)

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 5a1782b57ea..d01543006d8 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -595,6 +595,99 @@ def check_event(expr: _JSONObject, info: QAPISourceInfo) -> None:
     check_type(args, info, "'data'", allow_dict=not boxed)
 
 
+def check_expr(expr_elem: _JSONObject) -> None:
+    """
+    Validate and normalize a parsed QAPI schema expression.
+
+    :param expr_elem: The parsed expression to normalize and validate.
+
+    :raise QAPISemError: When this expression fails validation.
+    :return: None, ``expr`` is normalized in-place as needed.
+    """
+    # Expression
+    assert isinstance(expr_elem['expr'], dict)
+    for key in expr_elem['expr'].keys():
+        assert isinstance(key, str)
+    expr: _JSONObject = expr_elem['expr']
+
+    # QAPISourceInfo
+    assert isinstance(expr_elem['info'], QAPISourceInfo)
+    info: QAPISourceInfo = expr_elem['info']
+
+    # Optional[QAPIDoc]
+    tmp = expr_elem.get('doc')
+    assert tmp is None or isinstance(tmp, QAPIDoc)
+    doc: Optional[QAPIDoc] = tmp
+
+    if 'include' in expr:
+        return
+
+    metas = expr.keys() & {'enum', 'struct', 'union', 'alternate',
+                           'command', 'event'}
+    if len(metas) != 1:
+        raise QAPISemError(
+            info,
+            "expression must have exactly one key"
+            " 'enum', 'struct', 'union', 'alternate',"
+            " 'command', 'event'")
+    meta = metas.pop()
+
+    check_name_is_str(expr[meta], info, "'%s'" % meta)
+    name = cast(str, expr[meta])
+    info.set_defn(meta, name)
+    check_defn_name_str(name, info, meta)
+
+    if doc:
+        if doc.symbol != name:
+            raise QAPISemError(
+                info, "documentation comment is for '%s'" % doc.symbol)
+        doc.check_expr(expr)
+    elif info.pragma.doc_required:
+        raise QAPISemError(info,
+                           "documentation comment required")
+
+    if meta == 'enum':
+        check_keys(expr, info, meta,
+                   ['enum', 'data'], ['if', 'features', 'prefix'])
+        check_enum(expr, info)
+    elif meta == 'union':
+        check_keys(expr, info, meta,
+                   ['union', 'base', 'discriminator', 'data'],
+                   ['if', 'features'])
+        normalize_members(expr.get('base'))
+        normalize_members(expr['data'])
+        check_union(expr, info)
+    elif meta == 'alternate':
+        check_keys(expr, info, meta,
+                   ['alternate', 'data'], ['if', 'features'])
+        normalize_members(expr['data'])
+        check_alternate(expr, info)
+    elif meta == 'struct':
+        check_keys(expr, info, meta,
+                   ['struct', 'data'], ['base', 'if', 'features'])
+        normalize_members(expr['data'])
+        check_struct(expr, info)
+    elif meta == 'command':
+        check_keys(expr, info, meta,
+                   ['command'],
+                   ['data', 'returns', 'boxed', 'if', 'features',
+                    'gen', 'success-response', 'allow-oob',
+                    'allow-preconfig', 'coroutine'])
+        normalize_members(expr.get('data'))
+        check_command(expr, info)
+    elif meta == 'event':
+        check_keys(expr, info, meta,
+                   ['event'], ['data', 'boxed', 'if', 'features'])
+        normalize_members(expr.get('data'))
+        check_event(expr, info)
+    else:
+        assert False, 'unexpected meta type'
+
+    check_if(expr, info, meta)
+    check_features(expr.get('features'), info)
+    check_flags(expr, info)
+
+
 def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
     """
     Validate and normalize a list of parsed QAPI schema expressions.
@@ -607,88 +700,6 @@ def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
     :raise QAPISemError: When any expression fails validation.
     :return: The same list of expressions (now modified).
     """
-    for expr_elem in exprs:
-        # Expression
-        assert isinstance(expr_elem['expr'], dict)
-        for key in expr_elem['expr'].keys():
-            assert isinstance(key, str)
-        expr: _JSONObject = expr_elem['expr']
-
-        # QAPISourceInfo
-        assert isinstance(expr_elem['info'], QAPISourceInfo)
-        info: QAPISourceInfo = expr_elem['info']
-
-        # Optional[QAPIDoc]
-        tmp = expr_elem.get('doc')
-        assert tmp is None or isinstance(tmp, QAPIDoc)
-        doc: Optional[QAPIDoc] = tmp
-
-        if 'include' in expr:
-            continue
-
-        metas = expr.keys() & {'enum', 'struct', 'union', 'alternate',
-                               'command', 'event'}
-        if len(metas) != 1:
-            raise QAPISemError(
-                info,
-                "expression must have exactly one key"
-                " 'enum', 'struct', 'union', 'alternate',"
-                " 'command', 'event'")
-        meta = metas.pop()
-
-        check_name_is_str(expr[meta], info, "'%s'" % meta)
-        name = cast(str, expr[meta])
-        info.set_defn(meta, name)
-        check_defn_name_str(name, info, meta)
-
-        if doc:
-            if doc.symbol != name:
-                raise QAPISemError(
-                    info, "documentation comment is for '%s'" % doc.symbol)
-            doc.check_expr(expr)
-        elif info.pragma.doc_required:
-            raise QAPISemError(info,
-                               "documentation comment required")
-
-        if meta == 'enum':
-            check_keys(expr, info, meta,
-                       ['enum', 'data'], ['if', 'features', 'prefix'])
-            check_enum(expr, info)
-        elif meta == 'union':
-            check_keys(expr, info, meta,
-                       ['union', 'base', 'discriminator', 'data'],
-                       ['if', 'features'])
-            normalize_members(expr.get('base'))
-            normalize_members(expr['data'])
-            check_union(expr, info)
-        elif meta == 'alternate':
-            check_keys(expr, info, meta,
-                       ['alternate', 'data'], ['if', 'features'])
-            normalize_members(expr['data'])
-            check_alternate(expr, info)
-        elif meta == 'struct':
-            check_keys(expr, info, meta,
-                       ['struct', 'data'], ['base', 'if', 'features'])
-            normalize_members(expr['data'])
-            check_struct(expr, info)
-        elif meta == 'command':
-            check_keys(expr, info, meta,
-                       ['command'],
-                       ['data', 'returns', 'boxed', 'if', 'features',
-                        'gen', 'success-response', 'allow-oob',
-                        'allow-preconfig', 'coroutine'])
-            normalize_members(expr.get('data'))
-            check_command(expr, info)
-        elif meta == 'event':
-            check_keys(expr, info, meta,
-                       ['event'], ['data', 'boxed', 'if', 'features'])
-            normalize_members(expr.get('data'))
-            check_event(expr, info)
-        else:
-            assert False, 'unexpected meta type'
-
-        check_if(expr, info, meta)
-        check_features(expr.get('features'), info)
-        check_flags(expr, info)
-
+    for expr in exprs:
+        check_expr(expr)
     return exprs
-- 
2.39.0

Re: [PATCH v2 1/7] qapi/expr: Split check_expr out from check_exprs

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Primarily, this reduces a nesting level of a particularly long
> block. It's mostly code movement, but a new docstring is created.
>
> It also has the effect of creating a fairly convenient "catch point" in
> check_exprs for exception handling without making the nesting level even
> worse.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
>
> ---
>
> This patch was originally written as part of my effort to factor out
> QAPISourceInfo from this file by having expr.py raise a simple
> exception, then catch and wrap it at the higher level.
>
> This series doesn't do that anymore, but reducing the nesting level
> still seemed subjectively nice. It's not crucial.

Drawback: git-blame can't see through the move, and blames *you* for
that.

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/expr.py | 179 +++++++++++++++++++++++--------------------
>  1 file changed, 95 insertions(+), 84 deletions(-)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 5a1782b57ea..d01543006d8 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -595,6 +595,99 @@ def check_event(expr: _JSONObject, info: QAPISourceInfo) -> None:
>      check_type(args, info, "'data'", allow_dict=not boxed)
>  
>  
> +def check_expr(expr_elem: _JSONObject) -> None:
> +    """
> +    Validate and normalize a parsed QAPI schema expression.
> +
> +    :param expr_elem: The parsed expression to normalize and validate.
> +
> +    :raise QAPISemError: When this expression fails validation.
> +    :return: None, ``expr`` is normalized in-place as needed.

``expr`` is not defined here.  Suggest ", the expression is normalized".

> +    """
> +    # Expression
> +    assert isinstance(expr_elem['expr'], dict)
> +    for key in expr_elem['expr'].keys():
> +        assert isinstance(key, str)
> +    expr: _JSONObject = expr_elem['expr']

[...]

[PATCH v2 2/7] qapi/parser.py: add ParsedExpression type

[John Snow]

This is an immutable, named, typed tuple. It's arguably nicer than
arbitrary dicts for passing data around when using strict typing.

This patch turns parser.exprs into a list of ParsedExpressions instead,
and adjusts expr.py to match.

This allows the types we specify in parser.py to be "remembered" all the
way through expr.py and into schema.py. Several assertions around
packing and unpacking this data can be removed as a result.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/expr.py   | 29 +++++++++--------------------
 scripts/qapi/parser.py | 29 ++++++++++++++++++-----------
 scripts/qapi/schema.py |  6 +++---
 3 files changed, 30 insertions(+), 34 deletions(-)

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index d01543006d8..293f830fe9d 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -44,7 +44,7 @@
 
 from .common import c_name
 from .error import QAPISemError
-from .parser import QAPIDoc
+from .parser import ParsedExpression
 from .source import QAPISourceInfo
 
 
@@ -595,29 +595,17 @@ def check_event(expr: _JSONObject, info: QAPISourceInfo) -> None:
     check_type(args, info, "'data'", allow_dict=not boxed)
 
 
-def check_expr(expr_elem: _JSONObject) -> None:
+def check_expr(pexpr: ParsedExpression) -> None:
     """
-    Validate and normalize a parsed QAPI schema expression.
+    Validate and normalize a `ParsedExpression`.
 
-    :param expr_elem: The parsed expression to normalize and validate.
+    :param pexpr: The parsed expression to normalize and validate.
 
     :raise QAPISemError: When this expression fails validation.
-    :return: None, ``expr`` is normalized in-place as needed.
+    :return: None, ``pexpr`` is normalized in-place as needed.
     """
-    # Expression
-    assert isinstance(expr_elem['expr'], dict)
-    for key in expr_elem['expr'].keys():
-        assert isinstance(key, str)
-    expr: _JSONObject = expr_elem['expr']
-
-    # QAPISourceInfo
-    assert isinstance(expr_elem['info'], QAPISourceInfo)
-    info: QAPISourceInfo = expr_elem['info']
-
-    # Optional[QAPIDoc]
-    tmp = expr_elem.get('doc')
-    assert tmp is None or isinstance(tmp, QAPIDoc)
-    doc: Optional[QAPIDoc] = tmp
+    expr = pexpr.expr
+    info = pexpr.info
 
     if 'include' in expr:
         return
@@ -637,6 +625,7 @@ def check_expr(expr_elem: _JSONObject) -> None:
     info.set_defn(meta, name)
     check_defn_name_str(name, info, meta)
 
+    doc = pexpr.doc
     if doc:
         if doc.symbol != name:
             raise QAPISemError(
@@ -688,7 +677,7 @@ def check_expr(expr_elem: _JSONObject) -> None:
     check_flags(expr, info)
 
 
-def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
+def check_exprs(exprs: List[ParsedExpression]) -> List[ParsedExpression]:
     """
     Validate and normalize a list of parsed QAPI schema expressions.
 
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 1b006cdc133..f897dffbfd4 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -21,6 +21,7 @@
     TYPE_CHECKING,
     Dict,
     List,
+    NamedTuple,
     Optional,
     Set,
     Union,
@@ -48,6 +49,12 @@
 # several modules.
 
 
+class ParsedExpression(NamedTuple):
+    expr: TopLevelExpr
+    info: QAPISourceInfo
+    doc: Optional['QAPIDoc']
+
+
 class QAPIParseError(QAPISourceError):
     """Error class for all QAPI schema parsing errors."""
     def __init__(self, parser: 'QAPISchemaParser', msg: str):
@@ -100,7 +107,7 @@ def __init__(self,
         self.line_pos = 0
 
         # Parser output:
-        self.exprs: List[Dict[str, object]] = []
+        self.exprs: List[ParsedExpression] = []
         self.docs: List[QAPIDoc] = []
 
         # Showtime!
@@ -147,8 +154,7 @@ def _parse(self) -> None:
                                        "value of 'include' must be a string")
                 incl_fname = os.path.join(os.path.dirname(self._fname),
                                           include)
-                self.exprs.append({'expr': {'include': incl_fname},
-                                   'info': info})
+                self._add_expr(OrderedDict({'include': incl_fname}), info)
                 exprs_include = self._include(include, info, incl_fname,
                                               self._included)
                 if exprs_include:
@@ -165,17 +171,18 @@ def _parse(self) -> None:
                 for name, value in pragma.items():
                     self._pragma(name, value, info)
             else:
-                expr_elem = {'expr': expr,
-                             'info': info}
-                if cur_doc:
-                    if not cur_doc.symbol:
-                        raise QAPISemError(
-                            cur_doc.info, "definition documentation required")
-                    expr_elem['doc'] = cur_doc
-                self.exprs.append(expr_elem)
+                if cur_doc and not cur_doc.symbol:
+                    raise QAPISemError(
+                        cur_doc.info, "definition documentation required")
+                self._add_expr(expr, info, cur_doc)
             cur_doc = None
         self.reject_expr_doc(cur_doc)
 
+    def _add_expr(self, expr: TopLevelExpr,
+                  info: QAPISourceInfo,
+                  doc: Optional['QAPIDoc'] = None) -> None:
+        self.exprs.append(ParsedExpression(expr, info, doc))
+
     @staticmethod
     def reject_expr_doc(doc: Optional['QAPIDoc']) -> None:
         if doc and doc.symbol:
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index cd8661125cd..89f8e60db36 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -1168,9 +1168,9 @@ def _def_event(self, expr, info, doc):
 
     def _def_exprs(self, exprs):
         for expr_elem in exprs:
-            expr = expr_elem['expr']
-            info = expr_elem['info']
-            doc = expr_elem.get('doc')
+            expr = expr_elem.expr
+            info = expr_elem.info
+            doc = expr_elem.doc
             if 'enum' in expr:
                 self._def_enum_type(expr, info, doc)
             elif 'struct' in expr:
-- 
2.39.0

Re: [PATCH v2 2/7] qapi/parser.py: add ParsedExpression type

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This is an immutable, named, typed tuple. It's arguably nicer than
> arbitrary dicts for passing data around when using strict typing.
>
> This patch turns parser.exprs into a list of ParsedExpressions instead,
> and adjusts expr.py to match.
>
> This allows the types we specify in parser.py to be "remembered" all the
> way through expr.py and into schema.py. Several assertions around
> packing and unpacking this data can be removed as a result.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/expr.py   | 29 +++++++++--------------------
>  scripts/qapi/parser.py | 29 ++++++++++++++++++-----------
>  scripts/qapi/schema.py |  6 +++---
>  3 files changed, 30 insertions(+), 34 deletions(-)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index d01543006d8..293f830fe9d 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -44,7 +44,7 @@
>  
>  from .common import c_name
>  from .error import QAPISemError
> -from .parser import QAPIDoc
> +from .parser import ParsedExpression
>  from .source import QAPISourceInfo
>  
>  
> @@ -595,29 +595,17 @@ def check_event(expr: _JSONObject, info: QAPISourceInfo) -> None:
>      check_type(args, info, "'data'", allow_dict=not boxed)
>  
>  
> -def check_expr(expr_elem: _JSONObject) -> None:
> +def check_expr(pexpr: ParsedExpression) -> None:
>      """
> -    Validate and normalize a parsed QAPI schema expression.
> +    Validate and normalize a `ParsedExpression`.

Doc comment style question: what's our attitude towards repeating (parts
of) the function signature in its doc comment?

In general, I dislike comments that merely rephrase code as prose.

Untyped Python builds a habit of mentioning the types in the doc
comment.  That's additional information.  In typed Python, it's
rephrased information.

Keeping the old line would be just fine with me.

>  
> -    :param expr_elem: The parsed expression to normalize and validate.
> +    :param pexpr: The parsed expression to normalize and validate.
>  
>      :raise QAPISemError: When this expression fails validation.
> -    :return: None, ``expr`` is normalized in-place as needed.
> +    :return: None, ``pexpr`` is normalized in-place as needed.
>      """
> -    # Expression
> -    assert isinstance(expr_elem['expr'], dict)
> -    for key in expr_elem['expr'].keys():
> -        assert isinstance(key, str)
> -    expr: _JSONObject = expr_elem['expr']
> -
> -    # QAPISourceInfo
> -    assert isinstance(expr_elem['info'], QAPISourceInfo)
> -    info: QAPISourceInfo = expr_elem['info']
> -
> -    # Optional[QAPIDoc]
> -    tmp = expr_elem.get('doc')
> -    assert tmp is None or isinstance(tmp, QAPIDoc)
> -    doc: Optional[QAPIDoc] = tmp
> +    expr = pexpr.expr
> +    info = pexpr.info
>  
>      if 'include' in expr:
>          return
> @@ -637,6 +625,7 @@ def check_expr(expr_elem: _JSONObject) -> None:
>      info.set_defn(meta, name)
>      check_defn_name_str(name, info, meta)
>  
> +    doc = pexpr.doc
>      if doc:
>          if doc.symbol != name:
>              raise QAPISemError(
> @@ -688,7 +677,7 @@ def check_expr(expr_elem: _JSONObject) -> None:
>      check_flags(expr, info)
>  
>  
> -def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
> +def check_exprs(exprs: List[ParsedExpression]) -> List[ParsedExpression]:
>      """
>      Validate and normalize a list of parsed QAPI schema expressions.
>  
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 1b006cdc133..f897dffbfd4 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -21,6 +21,7 @@
>      TYPE_CHECKING,
>      Dict,
>      List,
> +    NamedTuple,
>      Optional,
>      Set,
>      Union,
> @@ -48,6 +49,12 @@
>  # several modules.
>  
>  
> +class ParsedExpression(NamedTuple):
> +    expr: TopLevelExpr
> +    info: QAPISourceInfo
> +    doc: Optional['QAPIDoc']
> +
> +
>  class QAPIParseError(QAPISourceError):
>      """Error class for all QAPI schema parsing errors."""
>      def __init__(self, parser: 'QAPISchemaParser', msg: str):
> @@ -100,7 +107,7 @@ def __init__(self,
>          self.line_pos = 0
>  
>          # Parser output:
> -        self.exprs: List[Dict[str, object]] = []
> +        self.exprs: List[ParsedExpression] = []
>          self.docs: List[QAPIDoc] = []
>  
>          # Showtime!
> @@ -147,8 +154,7 @@ def _parse(self) -> None:
>                                         "value of 'include' must be a string")
>                  incl_fname = os.path.join(os.path.dirname(self._fname),
>                                            include)
> -                self.exprs.append({'expr': {'include': incl_fname},
> -                                   'info': info})
> +                self._add_expr(OrderedDict({'include': incl_fname}), info)
>                  exprs_include = self._include(include, info, incl_fname,
>                                                self._included)
>                  if exprs_include:
> @@ -165,17 +171,18 @@ def _parse(self) -> None:
>                  for name, value in pragma.items():
>                      self._pragma(name, value, info)
>              else:
> -                expr_elem = {'expr': expr,
> -                             'info': info}
> -                if cur_doc:
> -                    if not cur_doc.symbol:
> -                        raise QAPISemError(
> -                            cur_doc.info, "definition documentation required")
> -                    expr_elem['doc'] = cur_doc
> -                self.exprs.append(expr_elem)
> +                if cur_doc and not cur_doc.symbol:
> +                    raise QAPISemError(
> +                        cur_doc.info, "definition documentation required")
> +                self._add_expr(expr, info, cur_doc)
>              cur_doc = None
>          self.reject_expr_doc(cur_doc)
>  
> +    def _add_expr(self, expr: TopLevelExpr,
> +                  info: QAPISourceInfo,
> +                  doc: Optional['QAPIDoc'] = None) -> None:
> +        self.exprs.append(ParsedExpression(expr, info, doc))
> +
>      @staticmethod
>      def reject_expr_doc(doc: Optional['QAPIDoc']) -> None:
>          if doc and doc.symbol:
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index cd8661125cd..89f8e60db36 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -1168,9 +1168,9 @@ def _def_event(self, expr, info, doc):
>  
>      def _def_exprs(self, exprs):
>          for expr_elem in exprs:

Rename @expr_elem to @pexpr for consistency with check_expr()?

> -            expr = expr_elem['expr']
> -            info = expr_elem['info']
> -            doc = expr_elem.get('doc')
> +            expr = expr_elem.expr
> +            info = expr_elem.info
> +            doc = expr_elem.doc
>              if 'enum' in expr:
>                  self._def_enum_type(expr, info, doc)
>              elif 'struct' in expr:

Re: [PATCH v2 2/7] qapi/parser.py: add ParsedExpression type

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 8134 bytes --]

On Wed, Feb 8, 2023, 11:17 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > This is an immutable, named, typed tuple. It's arguably nicer than
> > arbitrary dicts for passing data around when using strict typing.
> >
> > This patch turns parser.exprs into a list of ParsedExpressions instead,
> > and adjusts expr.py to match.
> >
> > This allows the types we specify in parser.py to be "remembered" all the
> > way through expr.py and into schema.py. Several assertions around
> > packing and unpacking this data can be removed as a result.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  scripts/qapi/expr.py   | 29 +++++++++--------------------
> >  scripts/qapi/parser.py | 29 ++++++++++++++++++-----------
> >  scripts/qapi/schema.py |  6 +++---
> >  3 files changed, 30 insertions(+), 34 deletions(-)
> >
> > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> > index d01543006d8..293f830fe9d 100644
> > --- a/scripts/qapi/expr.py
> > +++ b/scripts/qapi/expr.py
> > @@ -44,7 +44,7 @@
> >
> >  from .common import c_name
> >  from .error import QAPISemError
> > -from .parser import QAPIDoc
> > +from .parser import ParsedExpression
> >  from .source import QAPISourceInfo
> >
> >
> > @@ -595,29 +595,17 @@ def check_event(expr: _JSONObject, info:
> QAPISourceInfo) -> None:
> >      check_type(args, info, "'data'", allow_dict=not boxed)
> >
> >
> > -def check_expr(expr_elem: _JSONObject) -> None:
> > +def check_expr(pexpr: ParsedExpression) -> None:
> >      """
> > -    Validate and normalize a parsed QAPI schema expression.
> > +    Validate and normalize a `ParsedExpression`.
>
> Doc comment style question: what's our attitude towards repeating (parts
> of) the function signature in its doc comment?
>
> In general, I dislike comments that merely rephrase code as prose.
>

Me too, I've never found a doc system that gets it exactly right... I don't
like the duplication.

For the sake of tooltips and help docs that might be removed from code, I
often feel compelled to write something. Sometimes I value the consistency
over the DRY violation.

I dunno. I struggle with it.


> Untyped Python builds a habit of mentioning the types in the doc
> comment.  That's additional information.  In typed Python, it's
> rephrased information.
>
> Keeping the old line would be just fine with me.
>

The only reason I have this habit is because I have an obsession with
cross-references, really. I love clickables. love 'em!

Though it's probably true that the type signature would suffice for giving
you a clickable.

Looking at
https://qemu.readthedocs.io/projects/python-qemu-qmp/en/latest/overview.html
... yeah, the signatures are clickable. OK, I can cut it out. probably. I
guess. perhaps.


> >
> > -    :param expr_elem: The parsed expression to normalize and validate.
> > +    :param pexpr: The parsed expression to normalize and validate.
> >
> >      :raise QAPISemError: When this expression fails validation.
> > -    :return: None, ``expr`` is normalized in-place as needed.
> > +    :return: None, ``pexpr`` is normalized in-place as needed.
> >      """
> > -    # Expression
> > -    assert isinstance(expr_elem['expr'], dict)
> > -    for key in expr_elem['expr'].keys():
> > -        assert isinstance(key, str)
> > -    expr: _JSONObject = expr_elem['expr']
> > -
> > -    # QAPISourceInfo
> > -    assert isinstance(expr_elem['info'], QAPISourceInfo)
> > -    info: QAPISourceInfo = expr_elem['info']
> > -
> > -    # Optional[QAPIDoc]
> > -    tmp = expr_elem.get('doc')
> > -    assert tmp is None or isinstance(tmp, QAPIDoc)
> > -    doc: Optional[QAPIDoc] = tmp
> > +    expr = pexpr.expr
> > +    info = pexpr.info
> >
> >      if 'include' in expr:
> >          return
> > @@ -637,6 +625,7 @@ def check_expr(expr_elem: _JSONObject) -> None:
> >      info.set_defn(meta, name)
> >      check_defn_name_str(name, info, meta)
> >
> > +    doc = pexpr.doc
> >      if doc:
> >          if doc.symbol != name:
> >              raise QAPISemError(
> > @@ -688,7 +677,7 @@ def check_expr(expr_elem: _JSONObject) -> None:
> >      check_flags(expr, info)
> >
> >
> > -def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
> > +def check_exprs(exprs: List[ParsedExpression]) ->
> List[ParsedExpression]:
> >      """
> >      Validate and normalize a list of parsed QAPI schema expressions.
> >
> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> > index 1b006cdc133..f897dffbfd4 100644
> > --- a/scripts/qapi/parser.py
> > +++ b/scripts/qapi/parser.py
> > @@ -21,6 +21,7 @@
> >      TYPE_CHECKING,
> >      Dict,
> >      List,
> > +    NamedTuple,
> >      Optional,
> >      Set,
> >      Union,
> > @@ -48,6 +49,12 @@
> >  # several modules.
> >
> >
> > +class ParsedExpression(NamedTuple):
> > +    expr: TopLevelExpr
> > +    info: QAPISourceInfo
> > +    doc: Optional['QAPIDoc']
> > +
> > +
> >  class QAPIParseError(QAPISourceError):
> >      """Error class for all QAPI schema parsing errors."""
> >      def __init__(self, parser: 'QAPISchemaParser', msg: str):
> > @@ -100,7 +107,7 @@ def __init__(self,
> >          self.line_pos = 0
> >
> >          # Parser output:
> > -        self.exprs: List[Dict[str, object]] = []
> > +        self.exprs: List[ParsedExpression] = []
> >          self.docs: List[QAPIDoc] = []
> >
> >          # Showtime!
> > @@ -147,8 +154,7 @@ def _parse(self) -> None:
> >                                         "value of 'include' must be a
> string")
> >                  incl_fname = os.path.join(os.path.dirname(self._fname),
> >                                            include)
> > -                self.exprs.append({'expr': {'include': incl_fname},
> > -                                   'info': info})
> > +                self._add_expr(OrderedDict({'include': incl_fname}),
> info)
> >                  exprs_include = self._include(include, info, incl_fname,
> >                                                self._included)
> >                  if exprs_include:
> > @@ -165,17 +171,18 @@ def _parse(self) -> None:
> >                  for name, value in pragma.items():
> >                      self._pragma(name, value, info)
> >              else:
> > -                expr_elem = {'expr': expr,
> > -                             'info': info}
> > -                if cur_doc:
> > -                    if not cur_doc.symbol:
> > -                        raise QAPISemError(
> > -                            cur_doc.info, "definition documentation
> required")
> > -                    expr_elem['doc'] = cur_doc
> > -                self.exprs.append(expr_elem)
> > +                if cur_doc and not cur_doc.symbol:
> > +                    raise QAPISemError(
> > +                        cur_doc.info, "definition documentation
> required")
> > +                self._add_expr(expr, info, cur_doc)
> >              cur_doc = None
> >          self.reject_expr_doc(cur_doc)
> >
> > +    def _add_expr(self, expr: TopLevelExpr,
> > +                  info: QAPISourceInfo,
> > +                  doc: Optional['QAPIDoc'] = None) -> None:
> > +        self.exprs.append(ParsedExpression(expr, info, doc))
> > +
> >      @staticmethod
> >      def reject_expr_doc(doc: Optional['QAPIDoc']) -> None:
> >          if doc and doc.symbol:
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index cd8661125cd..89f8e60db36 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -1168,9 +1168,9 @@ def _def_event(self, expr, info, doc):
> >
> >      def _def_exprs(self, exprs):
> >          for expr_elem in exprs:
>
> Rename @expr_elem to @pexpr for consistency with check_expr()?
>

OK, will address when reordering/squashing


> > -            expr = expr_elem['expr']
> > -            info = expr_elem['info']
> > -            doc = expr_elem.get('doc')
> > +            expr = expr_elem.expr
> > +            info = expr_elem.info
> > +            doc = expr_elem.doc
> >              if 'enum' in expr:
> >                  self._def_enum_type(expr, info, doc)
> >              elif 'struct' in expr:
>
>

[-- Attachment #2: Type: text/html, Size: 11947 bytes --]

[PATCH v2 3/7] qapi/expr: Use TopLevelExpr where appropriate

[John Snow]

Remove most usages of _JSONObject with a more semantically meaningful
alias. Note that this is only a semantic alias; the distinction is not
enforced by the type system. This is merely a benefit for the human:
instead of check_xyz functions operating on a representation of some
"JSON Object", we can document them as operating on QAPI's Top Level
Expressions directly; it's more semantically meaningful.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/expr.py | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 293f830fe9d..338c9ea4131 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -44,7 +44,7 @@
 
 from .common import c_name
 from .error import QAPISemError
-from .parser import ParsedExpression
+from .parser import ParsedExpression, TopLevelExpr
 from .source import QAPISourceInfo
 
 
@@ -229,11 +229,11 @@ def pprint(elems: Iterable[str]) -> str:
                pprint(unknown), pprint(allowed)))
 
 
-def check_flags(expr: _JSONObject, info: QAPISourceInfo) -> None:
+def check_flags(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
     """
     Ensure flag members (if present) have valid values.
 
-    :param expr: The expression to validate.
+    :param expr: The `TopLevelExpr` to validate.
     :param info: QAPI schema source file information.
 
     :raise QAPISemError:
@@ -447,9 +447,9 @@ def check_features(features: Optional[object],
         check_if(feat, info, source)
 
 
-def check_enum(expr: _JSONObject, info: QAPISourceInfo) -> None:
+def check_enum(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
     """
-    Normalize and validate this expression as an ``enum`` definition.
+    Normalize and validate this `TopLevelExpr` as an ``enum`` definition.
 
     :param expr: The expression to validate.
     :param info: QAPI schema source file information.
@@ -486,9 +486,9 @@ def check_enum(expr: _JSONObject, info: QAPISourceInfo) -> None:
         check_features(member.get('features'), info)
 
 
-def check_struct(expr: _JSONObject, info: QAPISourceInfo) -> None:
+def check_struct(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
     """
-    Normalize and validate this expression as a ``struct`` definition.
+    Normalize and validate this `TopLevelExpr` as a ``struct`` definition.
 
     :param expr: The expression to validate.
     :param info: QAPI schema source file information.
@@ -503,9 +503,9 @@ def check_struct(expr: _JSONObject, info: QAPISourceInfo) -> None:
     check_type(expr.get('base'), info, "'base'")
 
 
-def check_union(expr: _JSONObject, info: QAPISourceInfo) -> None:
+def check_union(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
     """
-    Normalize and validate this expression as a ``union`` definition.
+    Normalize and validate this `TopLevelExpr` as a ``union`` definition.
 
     :param expr: The expression to validate.
     :param info: QAPI schema source file information.
@@ -531,9 +531,9 @@ def check_union(expr: _JSONObject, info: QAPISourceInfo) -> None:
         check_type(value['type'], info, source, allow_array=not base)
 
 
-def check_alternate(expr: _JSONObject, info: QAPISourceInfo) -> None:
+def check_alternate(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
     """
-    Normalize and validate this expression as an ``alternate`` definition.
+    Normalize and validate this `TopLevelExpr` as an ``alternate`` definition.
 
     :param expr: The expression to validate.
     :param info: QAPI schema source file information.
@@ -557,9 +557,9 @@ def check_alternate(expr: _JSONObject, info: QAPISourceInfo) -> None:
         check_type(value['type'], info, source, allow_array=True)
 
 
-def check_command(expr: _JSONObject, info: QAPISourceInfo) -> None:
+def check_command(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
     """
-    Normalize and validate this expression as a ``command`` definition.
+    Normalize and validate this `TopLevelExpr` as a ``command`` definition.
 
     :param expr: The expression to validate.
     :param info: QAPI schema source file information.
@@ -577,9 +577,9 @@ def check_command(expr: _JSONObject, info: QAPISourceInfo) -> None:
     check_type(rets, info, "'returns'", allow_array=True)
 
 
-def check_event(expr: _JSONObject, info: QAPISourceInfo) -> None:
+def check_event(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
     """
-    Normalize and validate this expression as an ``event`` definition.
+    Normalize and validate this `TopLevelExpr` as an ``event`` definition.
 
     :param expr: The expression to validate.
     :param info: QAPI schema source file information.
-- 
2.39.0

Re: [PATCH v2 3/7] qapi/expr: Use TopLevelExpr where appropriate

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Remove most usages of _JSONObject with a more semantically meaningful
> alias. Note that this is only a semantic alias; the distinction is not
> enforced by the type system. This is merely a benefit for the human:
> instead of check_xyz functions operating on a representation of some
> "JSON Object", we can document them as operating on QAPI's Top Level
> Expressions directly; it's more semantically meaningful.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/expr.py | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 293f830fe9d..338c9ea4131 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -44,7 +44,7 @@
>  
>  from .common import c_name
>  from .error import QAPISemError
> -from .parser import ParsedExpression
> +from .parser import ParsedExpression, TopLevelExpr
>  from .source import QAPISourceInfo
>  
>  
> @@ -229,11 +229,11 @@ def pprint(elems: Iterable[str]) -> str:
>                 pprint(unknown), pprint(allowed)))
>  
>  
> -def check_flags(expr: _JSONObject, info: QAPISourceInfo) -> None:
> +def check_flags(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
>      """
>      Ensure flag members (if present) have valid values.
>  
> -    :param expr: The expression to validate.
> +    :param expr: The `TopLevelExpr` to validate.
>      :param info: QAPI schema source file information.
>  
>      :raise QAPISemError:
> @@ -447,9 +447,9 @@ def check_features(features: Optional[object],
>          check_if(feat, info, source)
>  
>  
> -def check_enum(expr: _JSONObject, info: QAPISourceInfo) -> None:
> +def check_enum(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
>      """
> -    Normalize and validate this expression as an ``enum`` definition.
> +    Normalize and validate this `TopLevelExpr` as an ``enum`` definition.

Doc comment style question raised in review of PATCH 2 applies.

More of the same below.

>  
>      :param expr: The expression to validate.
>      :param info: QAPI schema source file information.

[...]

[PATCH v2 4/7] qapi/expr: add typing workaround for AbstractSet

[John Snow]

As part of attempting to unify the JSON types, I discovered that mypy
believes that `Mapping[str, ...].keys() & Set[str]` produces an
`AbstractSet[str]` and not a `Set[str]`.

As a result, mypy is unsure if the .pop() is safe.

Eh, fine, just wrap the expression in a set() constructor to force it to
be a mutable type.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/expr.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 338c9ea4131..95a25758fed 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -610,8 +610,8 @@ def check_expr(pexpr: ParsedExpression) -> None:
     if 'include' in expr:
         return
 
-    metas = expr.keys() & {'enum', 'struct', 'union', 'alternate',
-                           'command', 'event'}
+    metas = set(expr.keys() & {
+        'enum', 'struct', 'union', 'alternate', 'command', 'event'})
     if len(metas) != 1:
         raise QAPISemError(
             info,
-- 
2.39.0

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

[John Snow]

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.

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>
---
 scripts/qapi/expr.py   | 133 +++++++++++++++++++----------------------
 scripts/qapi/parser.py |  43 ++++++++-----
 scripts/qapi/schema.py | 105 ++++++++++++++++----------------
 3 files changed, 142 insertions(+), 139 deletions(-)

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 95a25758fed..83fa7a85b06 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -34,9 +34,9 @@
 import re
 from typing import (
     Collection,
-    Dict,
     Iterable,
     List,
+    Mapping,
     Optional,
     Union,
     cast,
@@ -44,7 +44,7 @@
 
 from .common import c_name
 from .error import QAPISemError
-from .parser import ParsedExpression, TopLevelExpr
+from .parser import QAPIExpression
 from .source import QAPISourceInfo
 
 
@@ -53,7 +53,7 @@
 # here (and also not practical as long as mypy lacks recursive
 # types), because the purpose of this module is to interrogate that
 # type.
-_JSONObject = Dict[str, object]
+_JSONObject = Mapping[str, object]
 
 
 # See check_name_str(), below.
@@ -229,12 +229,11 @@ def pprint(elems: Iterable[str]) -> str:
                pprint(unknown), pprint(allowed)))
 
 
-def check_flags(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
+def check_flags(expr: QAPIExpression) -> None:
     """
     Ensure flag members (if present) have valid values.
 
-    :param expr: The `TopLevelExpr` to validate.
-    :param info: QAPI schema source file information.
+    :param expr: The `QAPIExpression` to validate.
 
     :raise QAPISemError:
         When certain flags have an invalid value, or when
@@ -243,18 +242,18 @@ def check_flags(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
     for key in ('gen', 'success-response'):
         if key in expr and expr[key] is not False:
             raise QAPISemError(
-                info, "flag '%s' may only use false value" % key)
+                expr.info, "flag '%s' may only use false value" % key)
     for key in ('boxed', 'allow-oob', 'allow-preconfig', 'coroutine'):
         if key in expr and expr[key] is not True:
             raise QAPISemError(
-                info, "flag '%s' may only use true value" % key)
+                expr.info, "flag '%s' may only use true value" % key)
     if 'allow-oob' in expr and 'coroutine' in expr:
         # This is not necessarily a fundamental incompatibility, but
         # we don't have a use case and the desired semantics isn't
         # obvious.  The simplest solution is to forbid it until we get
         # a use case for it.
-        raise QAPISemError(info, "flags 'allow-oob' and 'coroutine' "
-                                 "are incompatible")
+        raise QAPISemError(
+            expr.info, "flags 'allow-oob' and 'coroutine' are incompatible")
 
 
 def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None:
@@ -447,12 +446,11 @@ def check_features(features: Optional[object],
         check_if(feat, info, source)
 
 
-def check_enum(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
+def check_enum(expr: QAPIExpression) -> None:
     """
-    Normalize and validate this `TopLevelExpr` as an ``enum`` definition.
+    Normalize and validate this `QAPIExpression` as an ``enum`` definition.
 
     :param expr: The expression to validate.
-    :param info: QAPI schema source file information.
 
     :raise QAPISemError: When ``expr`` is not a valid ``enum``.
     :return: None, ``expr`` is normalized in-place as needed.
@@ -462,36 +460,35 @@ def check_enum(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
     prefix = expr.get('prefix')
 
     if not isinstance(members, list):
-        raise QAPISemError(info, "'data' must be an array")
+        raise QAPISemError(expr.info, "'data' must be an array")
     if prefix is not None and not isinstance(prefix, str):
-        raise QAPISemError(info, "'prefix' must be a string")
+        raise QAPISemError(expr.info, "'prefix' must be a string")
 
-    permissive = name in info.pragma.member_name_exceptions
+    permissive = name in expr.info.pragma.member_name_exceptions
 
     members[:] = [m if isinstance(m, dict) else {'name': m}
                   for m in members]
     for member in members:
         source = "'data' member"
-        check_keys(member, info, source, ['name'], ['if', 'features'])
+        check_keys(member, expr.info, source, ['name'], ['if', 'features'])
         member_name = member['name']
-        check_name_is_str(member_name, info, source)
+        check_name_is_str(member_name, expr.info, source)
         source = "%s '%s'" % (source, member_name)
         # Enum members may start with a digit
         if member_name[0].isdigit():
             member_name = 'd' + member_name  # Hack: hide the digit
-        check_name_lower(member_name, info, source,
+        check_name_lower(member_name, expr.info, source,
                          permit_upper=permissive,
                          permit_underscore=permissive)
-        check_if(member, info, source)
-        check_features(member.get('features'), info)
+        check_if(member, expr.info, source)
+        check_features(member.get('features'), expr.info)
 
 
-def check_struct(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
+def check_struct(expr: QAPIExpression) -> None:
     """
-    Normalize and validate this `TopLevelExpr` as a ``struct`` definition.
+    Normalize and validate this `QAPIExpression` as a ``struct`` definition.
 
     :param expr: The expression to validate.
-    :param info: QAPI schema source file information.
 
     :raise QAPISemError: When ``expr`` is not a valid ``struct``.
     :return: None, ``expr`` is normalized in-place as needed.
@@ -499,16 +496,15 @@ def check_struct(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
     name = cast(str, expr['struct'])  # Checked in check_exprs
     members = expr['data']
 
-    check_type(members, info, "'data'", allow_dict=name)
-    check_type(expr.get('base'), info, "'base'")
+    check_type(members, expr.info, "'data'", allow_dict=name)
+    check_type(expr.get('base'), expr.info, "'base'")
 
 
-def check_union(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
+def check_union(expr: QAPIExpression) -> None:
     """
-    Normalize and validate this `TopLevelExpr` as a ``union`` definition.
+    Normalize and validate this `QAPIExpression` as a ``union`` definition.
 
     :param expr: The expression to validate.
-    :param info: QAPI schema source file information.
 
     :raise QAPISemError: when ``expr`` is not a valid ``union``.
     :return: None, ``expr`` is normalized in-place as needed.
@@ -518,25 +514,24 @@ def check_union(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
     discriminator = expr['discriminator']
     members = expr['data']
 
-    check_type(base, info, "'base'", allow_dict=name)
-    check_name_is_str(discriminator, info, "'discriminator'")
+    check_type(base, expr.info, "'base'", allow_dict=name)
+    check_name_is_str(discriminator, expr.info, "'discriminator'")
 
     if not isinstance(members, dict):
-        raise QAPISemError(info, "'data' must be an object")
+        raise QAPISemError(expr.info, "'data' must be an object")
 
     for (key, value) in members.items():
         source = "'data' member '%s'" % key
-        check_keys(value, info, source, ['type'], ['if'])
-        check_if(value, info, source)
-        check_type(value['type'], info, source, allow_array=not base)
+        check_keys(value, expr.info, source, ['type'], ['if'])
+        check_if(value, expr.info, source)
+        check_type(value['type'], expr.info, source, allow_array=not base)
 
 
-def check_alternate(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
+def check_alternate(expr: QAPIExpression) -> None:
     """
-    Normalize and validate this `TopLevelExpr` as an ``alternate`` definition.
+    Normalize and validate a `QAPIExpression` as an ``alternate`` definition.
 
     :param expr: The expression to validate.
-    :param info: QAPI schema source file information.
 
     :raise QAPISemError: When ``expr`` is not a valid ``alternate``.
     :return: None, ``expr`` is normalized in-place as needed.
@@ -544,25 +539,24 @@ def check_alternate(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
     members = expr['data']
 
     if not members:
-        raise QAPISemError(info, "'data' must not be empty")
+        raise QAPISemError(expr.info, "'data' must not be empty")
 
     if not isinstance(members, dict):
-        raise QAPISemError(info, "'data' must be an object")
+        raise QAPISemError(expr.info, "'data' must be an object")
 
     for (key, value) in members.items():
         source = "'data' member '%s'" % key
-        check_name_lower(key, info, source)
-        check_keys(value, info, source, ['type'], ['if'])
-        check_if(value, info, source)
-        check_type(value['type'], info, source, allow_array=True)
+        check_name_lower(key, expr.info, source)
+        check_keys(value, expr.info, source, ['type'], ['if'])
+        check_if(value, expr.info, source)
+        check_type(value['type'], expr.info, source, allow_array=True)
 
 
-def check_command(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
+def check_command(expr: QAPIExpression) -> None:
     """
-    Normalize and validate this `TopLevelExpr` as a ``command`` definition.
+    Normalize and validate this `QAPIExpression` as a ``command`` definition.
 
     :param expr: The expression to validate.
-    :param info: QAPI schema source file information.
 
     :raise QAPISemError: When ``expr`` is not a valid ``command``.
     :return: None, ``expr`` is normalized in-place as needed.
@@ -572,17 +566,16 @@ def check_command(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
     boxed = expr.get('boxed', False)
 
     if boxed and args is None:
-        raise QAPISemError(info, "'boxed': true requires 'data'")
-    check_type(args, info, "'data'", allow_dict=not boxed)
-    check_type(rets, info, "'returns'", allow_array=True)
+        raise QAPISemError(expr.info, "'boxed': true requires 'data'")
+    check_type(args, expr.info, "'data'", allow_dict=not boxed)
+    check_type(rets, expr.info, "'returns'", allow_array=True)
 
 
-def check_event(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
+def check_event(expr: QAPIExpression) -> None:
     """
-    Normalize and validate this `TopLevelExpr` as an ``event`` definition.
+    Normalize and validate this `QAPIExpression` as an ``event`` definition.
 
     :param expr: The expression to validate.
-    :param info: QAPI schema source file information.
 
     :raise QAPISemError: When ``expr`` is not a valid ``event``.
     :return: None, ``expr`` is normalized in-place as needed.
@@ -591,25 +584,23 @@ def check_event(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
     boxed = expr.get('boxed', False)
 
     if boxed and args is None:
-        raise QAPISemError(info, "'boxed': true requires 'data'")
-    check_type(args, info, "'data'", allow_dict=not boxed)
+        raise QAPISemError(expr.info, "'boxed': true requires 'data'")
+    check_type(args, expr.info, "'data'", allow_dict=not boxed)
 
 
-def check_expr(pexpr: ParsedExpression) -> None:
+def check_expr(expr: QAPIExpression) -> None:
     """
-    Validate and normalize a `ParsedExpression`.
+    Validate and normalize a `QAPIExpression`.
 
-    :param pexpr: The parsed expression to normalize and validate.
+    :param expr: The parsed expression to normalize and validate.
 
     :raise QAPISemError: When this expression fails validation.
-    :return: None, ``pexpr`` is normalized in-place as needed.
+    :return: None, ``expr`` is normalized in-place as needed.
     """
-    expr = pexpr.expr
-    info = pexpr.info
-
     if 'include' in expr:
         return
 
+    info = expr.info
     metas = set(expr.keys() & {
         'enum', 'struct', 'union', 'alternate', 'command', 'event'})
     if len(metas) != 1:
@@ -625,7 +616,7 @@ def check_expr(pexpr: ParsedExpression) -> None:
     info.set_defn(meta, name)
     check_defn_name_str(name, info, meta)
 
-    doc = pexpr.doc
+    doc = expr.doc
     if doc:
         if doc.symbol != name:
             raise QAPISemError(
@@ -638,24 +629,24 @@ def check_expr(pexpr: ParsedExpression) -> None:
     if meta == 'enum':
         check_keys(expr, info, meta,
                    ['enum', 'data'], ['if', 'features', 'prefix'])
-        check_enum(expr, info)
+        check_enum(expr)
     elif meta == 'union':
         check_keys(expr, info, meta,
                    ['union', 'base', 'discriminator', 'data'],
                    ['if', 'features'])
         normalize_members(expr.get('base'))
         normalize_members(expr['data'])
-        check_union(expr, info)
+        check_union(expr)
     elif meta == 'alternate':
         check_keys(expr, info, meta,
                    ['alternate', 'data'], ['if', 'features'])
         normalize_members(expr['data'])
-        check_alternate(expr, info)
+        check_alternate(expr)
     elif meta == 'struct':
         check_keys(expr, info, meta,
                    ['struct', 'data'], ['base', 'if', 'features'])
         normalize_members(expr['data'])
-        check_struct(expr, info)
+        check_struct(expr)
     elif meta == 'command':
         check_keys(expr, info, meta,
                    ['command'],
@@ -663,21 +654,21 @@ def check_expr(pexpr: ParsedExpression) -> None:
                     'gen', 'success-response', 'allow-oob',
                     'allow-preconfig', 'coroutine'])
         normalize_members(expr.get('data'))
-        check_command(expr, info)
+        check_command(expr)
     elif meta == 'event':
         check_keys(expr, info, meta,
                    ['event'], ['data', 'boxed', 'if', 'features'])
         normalize_members(expr.get('data'))
-        check_event(expr, info)
+        check_event(expr)
     else:
         assert False, 'unexpected meta type'
 
     check_if(expr, info, meta)
     check_features(expr.get('features'), info)
-    check_flags(expr, info)
+    check_flags(expr)
 
 
-def check_exprs(exprs: List[ParsedExpression]) -> List[ParsedExpression]:
+def check_exprs(exprs: List[QAPIExpression]) -> List[QAPIExpression]:
     """
     Validate and normalize a list of parsed QAPI schema expressions.
 
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
+
+
+class QAPIExpression(_UserDict):
+    def __init__(
+        self,
+        initialdata: Mapping[str, object],
+        info: QAPISourceInfo,
+        doc: Optional['QAPIDoc'] = None,
+    ):
+        super().__init__(initialdata)
+        self.info = info
+        self.doc: Optional['QAPIDoc'] = doc
 
 
 class QAPIParseError(QAPISourceError):
@@ -107,7 +118,7 @@ def __init__(self,
         self.line_pos = 0
 
         # Parser output:
-        self.exprs: List[ParsedExpression] = []
+        self.exprs: List[QAPIExpression] = []
         self.docs: List[QAPIDoc] = []
 
         # Showtime!
@@ -178,10 +189,10 @@ def _parse(self) -> None:
             cur_doc = None
         self.reject_expr_doc(cur_doc)
 
-    def _add_expr(self, expr: TopLevelExpr,
+    def _add_expr(self, expr: Mapping[str, object],
                   info: QAPISourceInfo,
                   doc: Optional['QAPIDoc'] = None) -> None:
-        self.exprs.append(ParsedExpression(expr, info, doc))
+        self.exprs.append(QAPIExpression(expr, info, doc))
 
     @staticmethod
     def reject_expr_doc(doc: Optional['QAPIDoc']) -> None:
@@ -791,7 +802,7 @@ def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
                                % feature.name)
         self.features[feature.name].connect(feature)
 
-    def check_expr(self, expr: TopLevelExpr) -> None:
+    def check_expr(self, expr: QAPIExpression) -> None:
         if self.has_section('Returns') and 'command' not in expr:
             raise QAPISemError(self.info,
                                "'Returns:' is only valid for commands")
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 89f8e60db36..295c21eab22 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -17,7 +17,7 @@
 from collections import OrderedDict
 import os
 import re
-from typing import Optional
+from typing import List, Optional
 
 from .common import (
     POINTER_SUFFIX,
@@ -29,7 +29,7 @@
 )
 from .error import QAPIError, QAPISemError, QAPISourceError
 from .expr import check_exprs
-from .parser import QAPISchemaParser
+from .parser import QAPIExpression, QAPISchemaParser
 
 
 class QAPISchemaIfCond:
@@ -964,10 +964,11 @@ def module_by_fname(self, fname):
         name = self._module_name(fname)
         return self._module_dict[name]
 
-    def _def_include(self, expr, info, doc):
+    def _def_include(self, expr: QAPIExpression):
         include = expr['include']
-        assert doc is None
-        self._def_entity(QAPISchemaInclude(self._make_module(include), info))
+        assert expr.doc is None
+        self._def_entity(
+            QAPISchemaInclude(self._make_module(include), expr.info))
 
     def _def_builtin_type(self, name, json_type, c_type):
         self._def_entity(QAPISchemaBuiltinType(name, json_type, c_type))
@@ -1045,15 +1046,15 @@ def _make_implicit_object_type(self, name, info, ifcond, role, members):
                 name, info, None, ifcond, None, None, members, None))
         return name
 
-    def _def_enum_type(self, expr, info, doc):
+    def _def_enum_type(self, expr: QAPIExpression):
         name = expr['enum']
         data = expr['data']
         prefix = expr.get('prefix')
         ifcond = QAPISchemaIfCond(expr.get('if'))
-        features = self._make_features(expr.get('features'), info)
+        features = self._make_features(expr.get('features'), expr.info)
         self._def_entity(QAPISchemaEnumType(
-            name, info, doc, ifcond, features,
-            self._make_enum_members(data, info), prefix))
+            name, expr.info, expr.doc, ifcond, features,
+            self._make_enum_members(data, expr.info), prefix))
 
     def _make_member(self, name, typ, ifcond, features, info):
         optional = False
@@ -1072,15 +1073,15 @@ def _make_members(self, data, info):
                                   value.get('features'), info)
                 for (key, value) in data.items()]
 
-    def _def_struct_type(self, expr, info, doc):
+    def _def_struct_type(self, expr: QAPIExpression):
         name = expr['struct']
         base = expr.get('base')
         data = expr['data']
         ifcond = QAPISchemaIfCond(expr.get('if'))
-        features = self._make_features(expr.get('features'), info)
+        features = self._make_features(expr.get('features'), expr.info)
         self._def_entity(QAPISchemaObjectType(
-            name, info, doc, ifcond, features, base,
-            self._make_members(data, info),
+            name, expr.info, expr.doc, ifcond, features, base,
+            self._make_members(data, expr.info),
             None))
 
     def _make_variant(self, case, typ, ifcond, info):
@@ -1089,46 +1090,49 @@ def _make_variant(self, case, typ, ifcond, info):
             typ = self._make_array_type(typ[0], info)
         return QAPISchemaVariant(case, info, typ, ifcond)
 
-    def _def_union_type(self, expr, info, doc):
+    def _def_union_type(self, expr: QAPIExpression):
         name = expr['union']
         base = expr['base']
         tag_name = expr['discriminator']
         data = expr['data']
+        assert isinstance(data, dict)
         ifcond = QAPISchemaIfCond(expr.get('if'))
-        features = self._make_features(expr.get('features'), info)
+        features = self._make_features(expr.get('features'), expr.info)
         if isinstance(base, dict):
             base = self._make_implicit_object_type(
-                name, info, ifcond,
-                'base', self._make_members(base, info))
+                name, expr.info, ifcond,
+                'base', self._make_members(base, expr.info))
         variants = [
             self._make_variant(key, value['type'],
                                QAPISchemaIfCond(value.get('if')),
-                               info)
+                               expr.info)
             for (key, value) in data.items()]
-        members = []
+        members: List[QAPISchemaObjectTypeMember] = []
         self._def_entity(
-            QAPISchemaObjectType(name, info, doc, ifcond, features,
+            QAPISchemaObjectType(name, expr.info, expr.doc, ifcond, features,
                                  base, members,
                                  QAPISchemaVariants(
-                                     tag_name, info, None, variants)))
+                                     tag_name, expr.info, None, variants)))
 
-    def _def_alternate_type(self, expr, info, doc):
+    def _def_alternate_type(self, expr: QAPIExpression):
         name = expr['alternate']
         data = expr['data']
+        assert isinstance(data, dict)
         ifcond = QAPISchemaIfCond(expr.get('if'))
-        features = self._make_features(expr.get('features'), info)
+        features = self._make_features(expr.get('features'), expr.info)
         variants = [
             self._make_variant(key, value['type'],
                                QAPISchemaIfCond(value.get('if')),
-                               info)
+                               expr.info)
             for (key, value) in data.items()]
-        tag_member = QAPISchemaObjectTypeMember('type', info, 'QType', False)
+        tag_member = QAPISchemaObjectTypeMember(
+            'type', expr.info, 'QType', False)
         self._def_entity(
-            QAPISchemaAlternateType(name, info, doc, ifcond, features,
-                                    QAPISchemaVariants(
-                                        None, info, tag_member, variants)))
+            QAPISchemaAlternateType(
+                name, expr.info, expr.doc, ifcond, features,
+                QAPISchemaVariants(None, expr.info, tag_member, variants)))
 
-    def _def_command(self, expr, info, doc):
+    def _def_command(self, expr: QAPIExpression):
         name = expr['command']
         data = expr.get('data')
         rets = expr.get('returns')
@@ -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
 
-- 
2.39.0

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

[Markus Armbruster]

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.

> 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>
> ---
>  scripts/qapi/expr.py   | 133 +++++++++++++++++++----------------------
>  scripts/qapi/parser.py |  43 ++++++++-----
>  scripts/qapi/schema.py | 105 ++++++++++++++++----------------
>  3 files changed, 142 insertions(+), 139 deletions(-)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 95a25758fed..83fa7a85b06 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -34,9 +34,9 @@
>  import re
>  from typing import (
>      Collection,
> -    Dict,
>      Iterable,
>      List,
> +    Mapping,
>      Optional,
>      Union,
>      cast,
> @@ -44,7 +44,7 @@
>  
>  from .common import c_name
>  from .error import QAPISemError
> -from .parser import ParsedExpression, TopLevelExpr
> +from .parser import QAPIExpression
>  from .source import QAPISourceInfo
>  
>  
> @@ -53,7 +53,7 @@
>  # here (and also not practical as long as mypy lacks recursive
>  # types), because the purpose of this module is to interrogate that
>  # type.
> -_JSONObject = Dict[str, object]
> +_JSONObject = Mapping[str, object]
>  
>  
>  # See check_name_str(), below.
> @@ -229,12 +229,11 @@ def pprint(elems: Iterable[str]) -> str:
>                 pprint(unknown), pprint(allowed)))
>  
>  
> -def check_flags(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> +def check_flags(expr: QAPIExpression) -> None:
>      """
>      Ensure flag members (if present) have valid values.
>  
> -    :param expr: The `TopLevelExpr` to validate.
> -    :param info: QAPI schema source file information.
> +    :param expr: The `QAPIExpression` to validate.
>  
>      :raise QAPISemError:
>          When certain flags have an invalid value, or when
> @@ -243,18 +242,18 @@ def check_flags(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
>      for key in ('gen', 'success-response'):
>          if key in expr and expr[key] is not False:
>              raise QAPISemError(
> -                info, "flag '%s' may only use false value" % key)
> +                expr.info, "flag '%s' may only use false value" % key)
>      for key in ('boxed', 'allow-oob', 'allow-preconfig', 'coroutine'):
>          if key in expr and expr[key] is not True:
>              raise QAPISemError(
> -                info, "flag '%s' may only use true value" % key)
> +                expr.info, "flag '%s' may only use true value" % key)
>      if 'allow-oob' in expr and 'coroutine' in expr:
>          # This is not necessarily a fundamental incompatibility, but
>          # we don't have a use case and the desired semantics isn't
>          # obvious.  The simplest solution is to forbid it until we get
>          # a use case for it.
> -        raise QAPISemError(info, "flags 'allow-oob' and 'coroutine' "
> -                                 "are incompatible")
> +        raise QAPISemError(
> +            expr.info, "flags 'allow-oob' and 'coroutine' are incompatible")
>  
>  
>  def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None:
> @@ -447,12 +446,11 @@ def check_features(features: Optional[object],
>          check_if(feat, info, source)
>  
>  
> -def check_enum(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> +def check_enum(expr: QAPIExpression) -> None:
>      """
> -    Normalize and validate this `TopLevelExpr` as an ``enum`` definition.
> +    Normalize and validate this `QAPIExpression` as an ``enum`` definition.
>  
>      :param expr: The expression to validate.
> -    :param info: QAPI schema source file information.
>  
>      :raise QAPISemError: When ``expr`` is not a valid ``enum``.
>      :return: None, ``expr`` is normalized in-place as needed.
> @@ -462,36 +460,35 @@ def check_enum(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
>      prefix = expr.get('prefix')
>  
>      if not isinstance(members, list):
> -        raise QAPISemError(info, "'data' must be an array")
> +        raise QAPISemError(expr.info, "'data' must be an array")
>      if prefix is not None and not isinstance(prefix, str):
> -        raise QAPISemError(info, "'prefix' must be a string")
> +        raise QAPISemError(expr.info, "'prefix' must be a string")
>  
> -    permissive = name in info.pragma.member_name_exceptions
> +    permissive = name in expr.info.pragma.member_name_exceptions
>  
>      members[:] = [m if isinstance(m, dict) else {'name': m}
>                    for m in members]
>      for member in members:
>          source = "'data' member"
> -        check_keys(member, info, source, ['name'], ['if', 'features'])
> +        check_keys(member, expr.info, source, ['name'], ['if', 'features'])
>          member_name = member['name']
> -        check_name_is_str(member_name, info, source)
> +        check_name_is_str(member_name, expr.info, source)
>          source = "%s '%s'" % (source, member_name)
>          # Enum members may start with a digit
>          if member_name[0].isdigit():
>              member_name = 'd' + member_name  # Hack: hide the digit
> -        check_name_lower(member_name, info, source,
> +        check_name_lower(member_name, expr.info, source,
>                           permit_upper=permissive,
>                           permit_underscore=permissive)
> -        check_if(member, info, source)
> -        check_features(member.get('features'), info)
> +        check_if(member, expr.info, source)
> +        check_features(member.get('features'), expr.info)
>  
>  
> -def check_struct(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> +def check_struct(expr: QAPIExpression) -> None:
>      """
> -    Normalize and validate this `TopLevelExpr` as a ``struct`` definition.
> +    Normalize and validate this `QAPIExpression` as a ``struct`` definition.
>  
>      :param expr: The expression to validate.
> -    :param info: QAPI schema source file information.
>  
>      :raise QAPISemError: When ``expr`` is not a valid ``struct``.
>      :return: None, ``expr`` is normalized in-place as needed.
> @@ -499,16 +496,15 @@ def check_struct(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
>      name = cast(str, expr['struct'])  # Checked in check_exprs
>      members = expr['data']
>  
> -    check_type(members, info, "'data'", allow_dict=name)
> -    check_type(expr.get('base'), info, "'base'")
> +    check_type(members, expr.info, "'data'", allow_dict=name)
> +    check_type(expr.get('base'), expr.info, "'base'")
>  
>  
> -def check_union(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> +def check_union(expr: QAPIExpression) -> None:
>      """
> -    Normalize and validate this `TopLevelExpr` as a ``union`` definition.
> +    Normalize and validate this `QAPIExpression` as a ``union`` definition.
>  
>      :param expr: The expression to validate.
> -    :param info: QAPI schema source file information.
>  
>      :raise QAPISemError: when ``expr`` is not a valid ``union``.
>      :return: None, ``expr`` is normalized in-place as needed.
> @@ -518,25 +514,24 @@ def check_union(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
>      discriminator = expr['discriminator']
>      members = expr['data']
>  
> -    check_type(base, info, "'base'", allow_dict=name)
> -    check_name_is_str(discriminator, info, "'discriminator'")
> +    check_type(base, expr.info, "'base'", allow_dict=name)
> +    check_name_is_str(discriminator, expr.info, "'discriminator'")
>  
>      if not isinstance(members, dict):
> -        raise QAPISemError(info, "'data' must be an object")
> +        raise QAPISemError(expr.info, "'data' must be an object")
>  
>      for (key, value) in members.items():
>          source = "'data' member '%s'" % key
> -        check_keys(value, info, source, ['type'], ['if'])
> -        check_if(value, info, source)
> -        check_type(value['type'], info, source, allow_array=not base)
> +        check_keys(value, expr.info, source, ['type'], ['if'])
> +        check_if(value, expr.info, source)
> +        check_type(value['type'], expr.info, source, allow_array=not base)
>  
>  
> -def check_alternate(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> +def check_alternate(expr: QAPIExpression) -> None:
>      """
> -    Normalize and validate this `TopLevelExpr` as an ``alternate`` definition.
> +    Normalize and validate a `QAPIExpression` as an ``alternate`` definition.
>  
>      :param expr: The expression to validate.
> -    :param info: QAPI schema source file information.
>  
>      :raise QAPISemError: When ``expr`` is not a valid ``alternate``.
>      :return: None, ``expr`` is normalized in-place as needed.
> @@ -544,25 +539,24 @@ def check_alternate(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
>      members = expr['data']
>  
>      if not members:
> -        raise QAPISemError(info, "'data' must not be empty")
> +        raise QAPISemError(expr.info, "'data' must not be empty")
>  
>      if not isinstance(members, dict):
> -        raise QAPISemError(info, "'data' must be an object")
> +        raise QAPISemError(expr.info, "'data' must be an object")
>  
>      for (key, value) in members.items():
>          source = "'data' member '%s'" % key
> -        check_name_lower(key, info, source)
> -        check_keys(value, info, source, ['type'], ['if'])
> -        check_if(value, info, source)
> -        check_type(value['type'], info, source, allow_array=True)
> +        check_name_lower(key, expr.info, source)
> +        check_keys(value, expr.info, source, ['type'], ['if'])
> +        check_if(value, expr.info, source)
> +        check_type(value['type'], expr.info, source, allow_array=True)
>  
>  
> -def check_command(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> +def check_command(expr: QAPIExpression) -> None:
>      """
> -    Normalize and validate this `TopLevelExpr` as a ``command`` definition.
> +    Normalize and validate this `QAPIExpression` as a ``command`` definition.
>  
>      :param expr: The expression to validate.
> -    :param info: QAPI schema source file information.
>  
>      :raise QAPISemError: When ``expr`` is not a valid ``command``.
>      :return: None, ``expr`` is normalized in-place as needed.
> @@ -572,17 +566,16 @@ def check_command(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
>      boxed = expr.get('boxed', False)
>  
>      if boxed and args is None:
> -        raise QAPISemError(info, "'boxed': true requires 'data'")
> -    check_type(args, info, "'data'", allow_dict=not boxed)
> -    check_type(rets, info, "'returns'", allow_array=True)
> +        raise QAPISemError(expr.info, "'boxed': true requires 'data'")
> +    check_type(args, expr.info, "'data'", allow_dict=not boxed)
> +    check_type(rets, expr.info, "'returns'", allow_array=True)
>  
>  
> -def check_event(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> +def check_event(expr: QAPIExpression) -> None:
>      """
> -    Normalize and validate this `TopLevelExpr` as an ``event`` definition.
> +    Normalize and validate this `QAPIExpression` as an ``event`` definition.
>  
>      :param expr: The expression to validate.
> -    :param info: QAPI schema source file information.
>  
>      :raise QAPISemError: When ``expr`` is not a valid ``event``.
>      :return: None, ``expr`` is normalized in-place as needed.
> @@ -591,25 +584,23 @@ def check_event(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
>      boxed = expr.get('boxed', False)
>  
>      if boxed and args is None:
> -        raise QAPISemError(info, "'boxed': true requires 'data'")
> -    check_type(args, info, "'data'", allow_dict=not boxed)
> +        raise QAPISemError(expr.info, "'boxed': true requires 'data'")
> +    check_type(args, expr.info, "'data'", allow_dict=not boxed)
>  
>  
> -def check_expr(pexpr: ParsedExpression) -> None:
> +def check_expr(expr: QAPIExpression) -> None:
>      """
> -    Validate and normalize a `ParsedExpression`.
> +    Validate and normalize a `QAPIExpression`.
>  
> -    :param pexpr: The parsed expression to normalize and validate.
> +    :param expr: The parsed expression to normalize and validate.
>  
>      :raise QAPISemError: When this expression fails validation.
> -    :return: None, ``pexpr`` is normalized in-place as needed.
> +    :return: None, ``expr`` is normalized in-place as needed.
>      """
> -    expr = pexpr.expr
> -    info = pexpr.info
> -
>      if 'include' in expr:
>          return
>  
> +    info = expr.info
>      metas = set(expr.keys() & {
>          'enum', 'struct', 'union', 'alternate', 'command', 'event'})
>      if len(metas) != 1:
> @@ -625,7 +616,7 @@ def check_expr(pexpr: ParsedExpression) -> None:
>      info.set_defn(meta, name)
>      check_defn_name_str(name, info, meta)
>  
> -    doc = pexpr.doc
> +    doc = expr.doc
>      if doc:
>          if doc.symbol != name:
>              raise QAPISemError(
> @@ -638,24 +629,24 @@ def check_expr(pexpr: ParsedExpression) -> None:
>      if meta == 'enum':
>          check_keys(expr, info, meta,
>                     ['enum', 'data'], ['if', 'features', 'prefix'])
> -        check_enum(expr, info)
> +        check_enum(expr)
>      elif meta == 'union':
>          check_keys(expr, info, meta,
>                     ['union', 'base', 'discriminator', 'data'],
>                     ['if', 'features'])
>          normalize_members(expr.get('base'))
>          normalize_members(expr['data'])
> -        check_union(expr, info)
> +        check_union(expr)
>      elif meta == 'alternate':
>          check_keys(expr, info, meta,
>                     ['alternate', 'data'], ['if', 'features'])
>          normalize_members(expr['data'])
> -        check_alternate(expr, info)
> +        check_alternate(expr)
>      elif meta == 'struct':
>          check_keys(expr, info, meta,
>                     ['struct', 'data'], ['base', 'if', 'features'])
>          normalize_members(expr['data'])
> -        check_struct(expr, info)
> +        check_struct(expr)
>      elif meta == 'command':
>          check_keys(expr, info, meta,
>                     ['command'],
> @@ -663,21 +654,21 @@ def check_expr(pexpr: ParsedExpression) -> None:
>                      'gen', 'success-response', 'allow-oob',
>                      'allow-preconfig', 'coroutine'])
>          normalize_members(expr.get('data'))
> -        check_command(expr, info)
> +        check_command(expr)
>      elif meta == 'event':
>          check_keys(expr, info, meta,
>                     ['event'], ['data', 'boxed', 'if', 'features'])
>          normalize_members(expr.get('data'))
> -        check_event(expr, info)
> +        check_event(expr)
>      else:
>          assert False, 'unexpected meta type'
>  
>      check_if(expr, info, meta)
>      check_features(expr.get('features'), info)
> -    check_flags(expr, info)
> +    check_flags(expr)
>  
>  
> -def check_exprs(exprs: List[ParsedExpression]) -> List[ParsedExpression]:
> +def check_exprs(exprs: List[QAPIExpression]) -> List[QAPIExpression]:
>      """
>      Validate and normalize a list of parsed QAPI schema expressions.
>  
> 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
:)

> +
> +
> +class QAPIExpression(_UserDict):
> +    def __init__(
> +        self,
> +        initialdata: Mapping[str, object],

I'd prefer to separate words: initial_data.

> +        info: QAPISourceInfo,
> +        doc: Optional['QAPIDoc'] = None,
> +    ):
> +        super().__init__(initialdata)
> +        self.info = info
> +        self.doc: Optional['QAPIDoc'] = doc
>  
>  
>  class QAPIParseError(QAPISourceError):
> @@ -107,7 +118,7 @@ def __init__(self,
>          self.line_pos = 0
>  
>          # Parser output:
> -        self.exprs: List[ParsedExpression] = []
> +        self.exprs: List[QAPIExpression] = []
>          self.docs: List[QAPIDoc] = []
>  
>          # Showtime!
> @@ -178,10 +189,10 @@ def _parse(self) -> None:
>              cur_doc = None
>          self.reject_expr_doc(cur_doc)
>  
> -    def _add_expr(self, expr: TopLevelExpr,
> +    def _add_expr(self, expr: Mapping[str, object],
>                    info: QAPISourceInfo,
>                    doc: Optional['QAPIDoc'] = None) -> None:
> -        self.exprs.append(ParsedExpression(expr, info, doc))
> +        self.exprs.append(QAPIExpression(expr, info, doc))
>  
>      @staticmethod
>      def reject_expr_doc(doc: Optional['QAPIDoc']) -> None:
> @@ -791,7 +802,7 @@ def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
>                                 % feature.name)
>          self.features[feature.name].connect(feature)
>  
> -    def check_expr(self, expr: TopLevelExpr) -> None:
> +    def check_expr(self, expr: QAPIExpression) -> None:
>          if self.has_section('Returns') and 'command' not in expr:
>              raise QAPISemError(self.info,
>                                 "'Returns:' is only valid for commands")
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 89f8e60db36..295c21eab22 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -17,7 +17,7 @@
>  from collections import OrderedDict
>  import os
>  import re
> -from typing import Optional
> +from typing import List, Optional
>  
>  from .common import (
>      POINTER_SUFFIX,
> @@ -29,7 +29,7 @@
>  )
>  from .error import QAPIError, QAPISemError, QAPISourceError
>  from .expr import check_exprs
> -from .parser import QAPISchemaParser
> +from .parser import QAPIExpression, QAPISchemaParser
>  
>  
>  class QAPISchemaIfCond:
> @@ -964,10 +964,11 @@ def module_by_fname(self, fname):
>          name = self._module_name(fname)
>          return self._module_dict[name]
>  
> -    def _def_include(self, expr, info, doc):
> +    def _def_include(self, expr: QAPIExpression):
>          include = expr['include']
> -        assert doc is None
> -        self._def_entity(QAPISchemaInclude(self._make_module(include), info))
> +        assert expr.doc is None
> +        self._def_entity(
> +            QAPISchemaInclude(self._make_module(include), expr.info))
>  
>      def _def_builtin_type(self, name, json_type, c_type):
>          self._def_entity(QAPISchemaBuiltinType(name, json_type, c_type))
> @@ -1045,15 +1046,15 @@ def _make_implicit_object_type(self, name, info, ifcond, role, members):
>                  name, info, None, ifcond, None, None, members, None))
>          return name
>  
> -    def _def_enum_type(self, expr, info, doc):
> +    def _def_enum_type(self, expr: QAPIExpression):
>          name = expr['enum']
>          data = expr['data']
>          prefix = expr.get('prefix')
>          ifcond = QAPISchemaIfCond(expr.get('if'))
> -        features = self._make_features(expr.get('features'), info)
> +        features = self._make_features(expr.get('features'), expr.info)
>          self._def_entity(QAPISchemaEnumType(
> -            name, info, doc, ifcond, features,
> -            self._make_enum_members(data, info), prefix))
> +            name, expr.info, expr.doc, ifcond, features,
> +            self._make_enum_members(data, expr.info), prefix))
>  
>      def _make_member(self, name, typ, ifcond, features, info):
>          optional = False
> @@ -1072,15 +1073,15 @@ def _make_members(self, data, info):
>                                    value.get('features'), info)
>                  for (key, value) in data.items()]
>  
> -    def _def_struct_type(self, expr, info, doc):
> +    def _def_struct_type(self, expr: QAPIExpression):
>          name = expr['struct']
>          base = expr.get('base')
>          data = expr['data']
>          ifcond = QAPISchemaIfCond(expr.get('if'))
> -        features = self._make_features(expr.get('features'), info)
> +        features = self._make_features(expr.get('features'), expr.info)
>          self._def_entity(QAPISchemaObjectType(
> -            name, info, doc, ifcond, features, base,
> -            self._make_members(data, info),
> +            name, expr.info, expr.doc, ifcond, features, base,
> +            self._make_members(data, expr.info),
>              None))
>  
>      def _make_variant(self, case, typ, ifcond, info):
> @@ -1089,46 +1090,49 @@ def _make_variant(self, case, typ, ifcond, info):
>              typ = self._make_array_type(typ[0], info)
>          return QAPISchemaVariant(case, info, typ, ifcond)
>  
> -    def _def_union_type(self, expr, info, doc):
> +    def _def_union_type(self, expr: QAPIExpression):
>          name = expr['union']
>          base = expr['base']
>          tag_name = expr['discriminator']
>          data = expr['data']
> +        assert isinstance(data, dict)
>          ifcond = QAPISchemaIfCond(expr.get('if'))
> -        features = self._make_features(expr.get('features'), info)
> +        features = self._make_features(expr.get('features'), expr.info)
>          if isinstance(base, dict):
>              base = self._make_implicit_object_type(
> -                name, info, ifcond,
> -                'base', self._make_members(base, info))
> +                name, expr.info, ifcond,
> +                'base', self._make_members(base, expr.info))
>          variants = [
>              self._make_variant(key, value['type'],
>                                 QAPISchemaIfCond(value.get('if')),
> -                               info)
> +                               expr.info)
>              for (key, value) in data.items()]
> -        members = []
> +        members: List[QAPISchemaObjectTypeMember] = []
>          self._def_entity(
> -            QAPISchemaObjectType(name, info, doc, ifcond, features,
> +            QAPISchemaObjectType(name, expr.info, expr.doc, ifcond, features,
>                                   base, members,
>                                   QAPISchemaVariants(
> -                                     tag_name, info, None, variants)))
> +                                     tag_name, expr.info, None, variants)))
>  
> -    def _def_alternate_type(self, expr, info, doc):
> +    def _def_alternate_type(self, expr: QAPIExpression):
>          name = expr['alternate']
>          data = expr['data']
> +        assert isinstance(data, dict)
>          ifcond = QAPISchemaIfCond(expr.get('if'))
> -        features = self._make_features(expr.get('features'), info)
> +        features = self._make_features(expr.get('features'), expr.info)
>          variants = [
>              self._make_variant(key, value['type'],
>                                 QAPISchemaIfCond(value.get('if')),
> -                               info)
> +                               expr.info)
>              for (key, value) in data.items()]
> -        tag_member = QAPISchemaObjectTypeMember('type', info, 'QType', False)
> +        tag_member = QAPISchemaObjectTypeMember(
> +            'type', expr.info, 'QType', False)
>          self._def_entity(
> -            QAPISchemaAlternateType(name, info, doc, ifcond, features,
> -                                    QAPISchemaVariants(
> -                                        None, info, tag_member, variants)))
> +            QAPISchemaAlternateType(
> +                name, expr.info, expr.doc, ifcond, features,
> +                QAPISchemaVariants(None, expr.info, tag_member, variants)))
>  
> -    def _def_command(self, expr, info, doc):
> +    def _def_command(self, expr: QAPIExpression):
>          name = expr['command']
>          data = expr.get('data')
>          rets = expr.get('returns')
> @@ -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.

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

[John Snow]

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>
> > ---
> >  scripts/qapi/expr.py   | 133 +++++++++++++++++++----------------------
> >  scripts/qapi/parser.py |  43 ++++++++-----
> >  scripts/qapi/schema.py | 105 ++++++++++++++++----------------
> >  3 files changed, 142 insertions(+), 139 deletions(-)
> >
> > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> > index 95a25758fed..83fa7a85b06 100644
> > --- a/scripts/qapi/expr.py
> > +++ b/scripts/qapi/expr.py
> > @@ -34,9 +34,9 @@
> >  import re
> >  from typing import (
> >      Collection,
> > -    Dict,
> >      Iterable,
> >      List,
> > +    Mapping,
> >      Optional,
> >      Union,
> >      cast,
> > @@ -44,7 +44,7 @@
> >
> >  from .common import c_name
> >  from .error import QAPISemError
> > -from .parser import ParsedExpression, TopLevelExpr
> > +from .parser import QAPIExpression
> >  from .source import QAPISourceInfo
> >
> >
> > @@ -53,7 +53,7 @@
> >  # here (and also not practical as long as mypy lacks recursive
> >  # types), because the purpose of this module is to interrogate that
> >  # type.
> > -_JSONObject = Dict[str, object]
> > +_JSONObject = Mapping[str, object]
> >
> >
> >  # See check_name_str(), below.
> > @@ -229,12 +229,11 @@ def pprint(elems: Iterable[str]) -> str:
> >                 pprint(unknown), pprint(allowed)))
> >
> >
> > -def check_flags(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> > +def check_flags(expr: QAPIExpression) -> None:
> >      """
> >      Ensure flag members (if present) have valid values.
> >
> > -    :param expr: The `TopLevelExpr` to validate.
> > -    :param info: QAPI schema source file information.
> > +    :param expr: The `QAPIExpression` to validate.
> >
> >      :raise QAPISemError:
> >          When certain flags have an invalid value, or when
> > @@ -243,18 +242,18 @@ def check_flags(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> >      for key in ('gen', 'success-response'):
> >          if key in expr and expr[key] is not False:
> >              raise QAPISemError(
> > -                info, "flag '%s' may only use false value" % key)
> > +                expr.info, "flag '%s' may only use false value" % key)
> >      for key in ('boxed', 'allow-oob', 'allow-preconfig', 'coroutine'):
> >          if key in expr and expr[key] is not True:
> >              raise QAPISemError(
> > -                info, "flag '%s' may only use true value" % key)
> > +                expr.info, "flag '%s' may only use true value" % key)
> >      if 'allow-oob' in expr and 'coroutine' in expr:
> >          # This is not necessarily a fundamental incompatibility, but
> >          # we don't have a use case and the desired semantics isn't
> >          # obvious.  The simplest solution is to forbid it until we get
> >          # a use case for it.
> > -        raise QAPISemError(info, "flags 'allow-oob' and 'coroutine' "
> > -                                 "are incompatible")
> > +        raise QAPISemError(
> > +            expr.info, "flags 'allow-oob' and 'coroutine' are incompatible")
> >
> >
> >  def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None:
> > @@ -447,12 +446,11 @@ def check_features(features: Optional[object],
> >          check_if(feat, info, source)
> >
> >
> > -def check_enum(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> > +def check_enum(expr: QAPIExpression) -> None:
> >      """
> > -    Normalize and validate this `TopLevelExpr` as an ``enum`` definition.
> > +    Normalize and validate this `QAPIExpression` as an ``enum`` definition.
> >
> >      :param expr: The expression to validate.
> > -    :param info: QAPI schema source file information.
> >
> >      :raise QAPISemError: When ``expr`` is not a valid ``enum``.
> >      :return: None, ``expr`` is normalized in-place as needed.
> > @@ -462,36 +460,35 @@ def check_enum(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> >      prefix = expr.get('prefix')
> >
> >      if not isinstance(members, list):
> > -        raise QAPISemError(info, "'data' must be an array")
> > +        raise QAPISemError(expr.info, "'data' must be an array")
> >      if prefix is not None and not isinstance(prefix, str):
> > -        raise QAPISemError(info, "'prefix' must be a string")
> > +        raise QAPISemError(expr.info, "'prefix' must be a string")
> >
> > -    permissive = name in info.pragma.member_name_exceptions
> > +    permissive = name in expr.info.pragma.member_name_exceptions
> >
> >      members[:] = [m if isinstance(m, dict) else {'name': m}
> >                    for m in members]
> >      for member in members:
> >          source = "'data' member"
> > -        check_keys(member, info, source, ['name'], ['if', 'features'])
> > +        check_keys(member, expr.info, source, ['name'], ['if', 'features'])
> >          member_name = member['name']
> > -        check_name_is_str(member_name, info, source)
> > +        check_name_is_str(member_name, expr.info, source)
> >          source = "%s '%s'" % (source, member_name)
> >          # Enum members may start with a digit
> >          if member_name[0].isdigit():
> >              member_name = 'd' + member_name  # Hack: hide the digit
> > -        check_name_lower(member_name, info, source,
> > +        check_name_lower(member_name, expr.info, source,
> >                           permit_upper=permissive,
> >                           permit_underscore=permissive)
> > -        check_if(member, info, source)
> > -        check_features(member.get('features'), info)
> > +        check_if(member, expr.info, source)
> > +        check_features(member.get('features'), expr.info)
> >
> >
> > -def check_struct(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> > +def check_struct(expr: QAPIExpression) -> None:
> >      """
> > -    Normalize and validate this `TopLevelExpr` as a ``struct`` definition.
> > +    Normalize and validate this `QAPIExpression` as a ``struct`` definition.
> >
> >      :param expr: The expression to validate.
> > -    :param info: QAPI schema source file information.
> >
> >      :raise QAPISemError: When ``expr`` is not a valid ``struct``.
> >      :return: None, ``expr`` is normalized in-place as needed.
> > @@ -499,16 +496,15 @@ def check_struct(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> >      name = cast(str, expr['struct'])  # Checked in check_exprs
> >      members = expr['data']
> >
> > -    check_type(members, info, "'data'", allow_dict=name)
> > -    check_type(expr.get('base'), info, "'base'")
> > +    check_type(members, expr.info, "'data'", allow_dict=name)
> > +    check_type(expr.get('base'), expr.info, "'base'")
> >
> >
> > -def check_union(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> > +def check_union(expr: QAPIExpression) -> None:
> >      """
> > -    Normalize and validate this `TopLevelExpr` as a ``union`` definition.
> > +    Normalize and validate this `QAPIExpression` as a ``union`` definition.
> >
> >      :param expr: The expression to validate.
> > -    :param info: QAPI schema source file information.
> >
> >      :raise QAPISemError: when ``expr`` is not a valid ``union``.
> >      :return: None, ``expr`` is normalized in-place as needed.
> > @@ -518,25 +514,24 @@ def check_union(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> >      discriminator = expr['discriminator']
> >      members = expr['data']
> >
> > -    check_type(base, info, "'base'", allow_dict=name)
> > -    check_name_is_str(discriminator, info, "'discriminator'")
> > +    check_type(base, expr.info, "'base'", allow_dict=name)
> > +    check_name_is_str(discriminator, expr.info, "'discriminator'")
> >
> >      if not isinstance(members, dict):
> > -        raise QAPISemError(info, "'data' must be an object")
> > +        raise QAPISemError(expr.info, "'data' must be an object")
> >
> >      for (key, value) in members.items():
> >          source = "'data' member '%s'" % key
> > -        check_keys(value, info, source, ['type'], ['if'])
> > -        check_if(value, info, source)
> > -        check_type(value['type'], info, source, allow_array=not base)
> > +        check_keys(value, expr.info, source, ['type'], ['if'])
> > +        check_if(value, expr.info, source)
> > +        check_type(value['type'], expr.info, source, allow_array=not base)
> >
> >
> > -def check_alternate(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> > +def check_alternate(expr: QAPIExpression) -> None:
> >      """
> > -    Normalize and validate this `TopLevelExpr` as an ``alternate`` definition.
> > +    Normalize and validate a `QAPIExpression` as an ``alternate`` definition.
> >
> >      :param expr: The expression to validate.
> > -    :param info: QAPI schema source file information.
> >
> >      :raise QAPISemError: When ``expr`` is not a valid ``alternate``.
> >      :return: None, ``expr`` is normalized in-place as needed.
> > @@ -544,25 +539,24 @@ def check_alternate(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> >      members = expr['data']
> >
> >      if not members:
> > -        raise QAPISemError(info, "'data' must not be empty")
> > +        raise QAPISemError(expr.info, "'data' must not be empty")
> >
> >      if not isinstance(members, dict):
> > -        raise QAPISemError(info, "'data' must be an object")
> > +        raise QAPISemError(expr.info, "'data' must be an object")
> >
> >      for (key, value) in members.items():
> >          source = "'data' member '%s'" % key
> > -        check_name_lower(key, info, source)
> > -        check_keys(value, info, source, ['type'], ['if'])
> > -        check_if(value, info, source)
> > -        check_type(value['type'], info, source, allow_array=True)
> > +        check_name_lower(key, expr.info, source)
> > +        check_keys(value, expr.info, source, ['type'], ['if'])
> > +        check_if(value, expr.info, source)
> > +        check_type(value['type'], expr.info, source, allow_array=True)
> >
> >
> > -def check_command(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> > +def check_command(expr: QAPIExpression) -> None:
> >      """
> > -    Normalize and validate this `TopLevelExpr` as a ``command`` definition.
> > +    Normalize and validate this `QAPIExpression` as a ``command`` definition.
> >
> >      :param expr: The expression to validate.
> > -    :param info: QAPI schema source file information.
> >
> >      :raise QAPISemError: When ``expr`` is not a valid ``command``.
> >      :return: None, ``expr`` is normalized in-place as needed.
> > @@ -572,17 +566,16 @@ def check_command(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> >      boxed = expr.get('boxed', False)
> >
> >      if boxed and args is None:
> > -        raise QAPISemError(info, "'boxed': true requires 'data'")
> > -    check_type(args, info, "'data'", allow_dict=not boxed)
> > -    check_type(rets, info, "'returns'", allow_array=True)
> > +        raise QAPISemError(expr.info, "'boxed': true requires 'data'")
> > +    check_type(args, expr.info, "'data'", allow_dict=not boxed)
> > +    check_type(rets, expr.info, "'returns'", allow_array=True)
> >
> >
> > -def check_event(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> > +def check_event(expr: QAPIExpression) -> None:
> >      """
> > -    Normalize and validate this `TopLevelExpr` as an ``event`` definition.
> > +    Normalize and validate this `QAPIExpression` as an ``event`` definition.
> >
> >      :param expr: The expression to validate.
> > -    :param info: QAPI schema source file information.
> >
> >      :raise QAPISemError: When ``expr`` is not a valid ``event``.
> >      :return: None, ``expr`` is normalized in-place as needed.
> > @@ -591,25 +584,23 @@ def check_event(expr: TopLevelExpr, info: QAPISourceInfo) -> None:
> >      boxed = expr.get('boxed', False)
> >
> >      if boxed and args is None:
> > -        raise QAPISemError(info, "'boxed': true requires 'data'")
> > -    check_type(args, info, "'data'", allow_dict=not boxed)
> > +        raise QAPISemError(expr.info, "'boxed': true requires 'data'")
> > +    check_type(args, expr.info, "'data'", allow_dict=not boxed)
> >
> >
> > -def check_expr(pexpr: ParsedExpression) -> None:
> > +def check_expr(expr: QAPIExpression) -> None:
> >      """
> > -    Validate and normalize a `ParsedExpression`.
> > +    Validate and normalize a `QAPIExpression`.
> >
> > -    :param pexpr: The parsed expression to normalize and validate.
> > +    :param expr: The parsed expression to normalize and validate.
> >
> >      :raise QAPISemError: When this expression fails validation.
> > -    :return: None, ``pexpr`` is normalized in-place as needed.
> > +    :return: None, ``expr`` is normalized in-place as needed.
> >      """
> > -    expr = pexpr.expr
> > -    info = pexpr.info
> > -
> >      if 'include' in expr:
> >          return
> >
> > +    info = expr.info
> >      metas = set(expr.keys() & {
> >          'enum', 'struct', 'union', 'alternate', 'command', 'event'})
> >      if len(metas) != 1:
> > @@ -625,7 +616,7 @@ def check_expr(pexpr: ParsedExpression) -> None:
> >      info.set_defn(meta, name)
> >      check_defn_name_str(name, info, meta)
> >
> > -    doc = pexpr.doc
> > +    doc = expr.doc
> >      if doc:
> >          if doc.symbol != name:
> >              raise QAPISemError(
> > @@ -638,24 +629,24 @@ def check_expr(pexpr: ParsedExpression) -> None:
> >      if meta == 'enum':
> >          check_keys(expr, info, meta,
> >                     ['enum', 'data'], ['if', 'features', 'prefix'])
> > -        check_enum(expr, info)
> > +        check_enum(expr)
> >      elif meta == 'union':
> >          check_keys(expr, info, meta,
> >                     ['union', 'base', 'discriminator', 'data'],
> >                     ['if', 'features'])
> >          normalize_members(expr.get('base'))
> >          normalize_members(expr['data'])
> > -        check_union(expr, info)
> > +        check_union(expr)
> >      elif meta == 'alternate':
> >          check_keys(expr, info, meta,
> >                     ['alternate', 'data'], ['if', 'features'])
> >          normalize_members(expr['data'])
> > -        check_alternate(expr, info)
> > +        check_alternate(expr)
> >      elif meta == 'struct':
> >          check_keys(expr, info, meta,
> >                     ['struct', 'data'], ['base', 'if', 'features'])
> >          normalize_members(expr['data'])
> > -        check_struct(expr, info)
> > +        check_struct(expr)
> >      elif meta == 'command':
> >          check_keys(expr, info, meta,
> >                     ['command'],
> > @@ -663,21 +654,21 @@ def check_expr(pexpr: ParsedExpression) -> None:
> >                      'gen', 'success-response', 'allow-oob',
> >                      'allow-preconfig', 'coroutine'])
> >          normalize_members(expr.get('data'))
> > -        check_command(expr, info)
> > +        check_command(expr)
> >      elif meta == 'event':
> >          check_keys(expr, info, meta,
> >                     ['event'], ['data', 'boxed', 'if', 'features'])
> >          normalize_members(expr.get('data'))
> > -        check_event(expr, info)
> > +        check_event(expr)
> >      else:
> >          assert False, 'unexpected meta type'
> >
> >      check_if(expr, info, meta)
> >      check_features(expr.get('features'), info)
> > -    check_flags(expr, info)
> > +    check_flags(expr)
> >
> >
> > -def check_exprs(exprs: List[ParsedExpression]) -> List[ParsedExpression]:
> > +def check_exprs(exprs: List[QAPIExpression]) -> List[QAPIExpression]:
> >      """
> >      Validate and normalize a list of parsed QAPI schema expressions.
> >
> > 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

> > +        info: QAPISourceInfo,
> > +        doc: Optional['QAPIDoc'] = None,
> > +    ):
> > +        super().__init__(initialdata)
> > +        self.info = info
> > +        self.doc: Optional['QAPIDoc'] = doc
> >
> >
> >  class QAPIParseError(QAPISourceError):
> > @@ -107,7 +118,7 @@ def __init__(self,
> >          self.line_pos = 0
> >
> >          # Parser output:
> > -        self.exprs: List[ParsedExpression] = []
> > +        self.exprs: List[QAPIExpression] = []
> >          self.docs: List[QAPIDoc] = []
> >
> >          # Showtime!
> > @@ -178,10 +189,10 @@ def _parse(self) -> None:
> >              cur_doc = None
> >          self.reject_expr_doc(cur_doc)
> >
> > -    def _add_expr(self, expr: TopLevelExpr,
> > +    def _add_expr(self, expr: Mapping[str, object],
> >                    info: QAPISourceInfo,
> >                    doc: Optional['QAPIDoc'] = None) -> None:
> > -        self.exprs.append(ParsedExpression(expr, info, doc))
> > +        self.exprs.append(QAPIExpression(expr, info, doc))
> >
> >      @staticmethod
> >      def reject_expr_doc(doc: Optional['QAPIDoc']) -> None:
> > @@ -791,7 +802,7 @@ def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
> >                                 % feature.name)
> >          self.features[feature.name].connect(feature)
> >
> > -    def check_expr(self, expr: TopLevelExpr) -> None:
> > +    def check_expr(self, expr: QAPIExpression) -> None:
> >          if self.has_section('Returns') and 'command' not in expr:
> >              raise QAPISemError(self.info,
> >                                 "'Returns:' is only valid for commands")
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index 89f8e60db36..295c21eab22 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -17,7 +17,7 @@
> >  from collections import OrderedDict
> >  import os
> >  import re
> > -from typing import Optional
> > +from typing import List, Optional
> >
> >  from .common import (
> >      POINTER_SUFFIX,
> > @@ -29,7 +29,7 @@
> >  )
> >  from .error import QAPIError, QAPISemError, QAPISourceError
> >  from .expr import check_exprs
> > -from .parser import QAPISchemaParser
> > +from .parser import QAPIExpression, QAPISchemaParser
> >
> >
> >  class QAPISchemaIfCond:
> > @@ -964,10 +964,11 @@ def module_by_fname(self, fname):
> >          name = self._module_name(fname)
> >          return self._module_dict[name]
> >
> > -    def _def_include(self, expr, info, doc):
> > +    def _def_include(self, expr: QAPIExpression):
> >          include = expr['include']
> > -        assert doc is None
> > -        self._def_entity(QAPISchemaInclude(self._make_module(include), info))
> > +        assert expr.doc is None
> > +        self._def_entity(
> > +            QAPISchemaInclude(self._make_module(include), expr.info))
> >
> >      def _def_builtin_type(self, name, json_type, c_type):
> >          self._def_entity(QAPISchemaBuiltinType(name, json_type, c_type))
> > @@ -1045,15 +1046,15 @@ def _make_implicit_object_type(self, name, info, ifcond, role, members):
> >                  name, info, None, ifcond, None, None, members, None))
> >          return name
> >
> > -    def _def_enum_type(self, expr, info, doc):
> > +    def _def_enum_type(self, expr: QAPIExpression):
> >          name = expr['enum']
> >          data = expr['data']
> >          prefix = expr.get('prefix')
> >          ifcond = QAPISchemaIfCond(expr.get('if'))
> > -        features = self._make_features(expr.get('features'), info)
> > +        features = self._make_features(expr.get('features'), expr.info)
> >          self._def_entity(QAPISchemaEnumType(
> > -            name, info, doc, ifcond, features,
> > -            self._make_enum_members(data, info), prefix))
> > +            name, expr.info, expr.doc, ifcond, features,
> > +            self._make_enum_members(data, expr.info), prefix))
> >
> >      def _make_member(self, name, typ, ifcond, features, info):
> >          optional = False
> > @@ -1072,15 +1073,15 @@ def _make_members(self, data, info):
> >                                    value.get('features'), info)
> >                  for (key, value) in data.items()]
> >
> > -    def _def_struct_type(self, expr, info, doc):
> > +    def _def_struct_type(self, expr: QAPIExpression):
> >          name = expr['struct']
> >          base = expr.get('base')
> >          data = expr['data']
> >          ifcond = QAPISchemaIfCond(expr.get('if'))
> > -        features = self._make_features(expr.get('features'), info)
> > +        features = self._make_features(expr.get('features'), expr.info)
> >          self._def_entity(QAPISchemaObjectType(
> > -            name, info, doc, ifcond, features, base,
> > -            self._make_members(data, info),
> > +            name, expr.info, expr.doc, ifcond, features, base,
> > +            self._make_members(data, expr.info),
> >              None))
> >
> >      def _make_variant(self, case, typ, ifcond, info):
> > @@ -1089,46 +1090,49 @@ def _make_variant(self, case, typ, ifcond, info):
> >              typ = self._make_array_type(typ[0], info)
> >          return QAPISchemaVariant(case, info, typ, ifcond)
> >
> > -    def _def_union_type(self, expr, info, doc):
> > +    def _def_union_type(self, expr: QAPIExpression):
> >          name = expr['union']
> >          base = expr['base']
> >          tag_name = expr['discriminator']
> >          data = expr['data']
> > +        assert isinstance(data, dict)
> >          ifcond = QAPISchemaIfCond(expr.get('if'))
> > -        features = self._make_features(expr.get('features'), info)
> > +        features = self._make_features(expr.get('features'), expr.info)
> >          if isinstance(base, dict):
> >              base = self._make_implicit_object_type(
> > -                name, info, ifcond,
> > -                'base', self._make_members(base, info))
> > +                name, expr.info, ifcond,
> > +                'base', self._make_members(base, expr.info))
> >          variants = [
> >              self._make_variant(key, value['type'],
> >                                 QAPISchemaIfCond(value.get('if')),
> > -                               info)
> > +                               expr.info)
> >              for (key, value) in data.items()]
> > -        members = []
> > +        members: List[QAPISchemaObjectTypeMember] = []
> >          self._def_entity(
> > -            QAPISchemaObjectType(name, info, doc, ifcond, features,
> > +            QAPISchemaObjectType(name, expr.info, expr.doc, ifcond, features,
> >                                   base, members,
> >                                   QAPISchemaVariants(
> > -                                     tag_name, info, None, variants)))
> > +                                     tag_name, expr.info, None, variants)))
> >
> > -    def _def_alternate_type(self, expr, info, doc):
> > +    def _def_alternate_type(self, expr: QAPIExpression):
> >          name = expr['alternate']
> >          data = expr['data']
> > +        assert isinstance(data, dict)
> >          ifcond = QAPISchemaIfCond(expr.get('if'))
> > -        features = self._make_features(expr.get('features'), info)
> > +        features = self._make_features(expr.get('features'), expr.info)
> >          variants = [
> >              self._make_variant(key, value['type'],
> >                                 QAPISchemaIfCond(value.get('if')),
> > -                               info)
> > +                               expr.info)
> >              for (key, value) in data.items()]
> > -        tag_member = QAPISchemaObjectTypeMember('type', info, 'QType', False)
> > +        tag_member = QAPISchemaObjectTypeMember(
> > +            'type', expr.info, 'QType', False)
> >          self._def_entity(
> > -            QAPISchemaAlternateType(name, info, doc, ifcond, features,
> > -                                    QAPISchemaVariants(
> > -                                        None, info, tag_member, variants)))
> > +            QAPISchemaAlternateType(
> > +                name, expr.info, expr.doc, ifcond, features,
> > +                QAPISchemaVariants(None, expr.info, tag_member, variants)))
> >
> > -    def _def_command(self, expr, info, doc):
> > +    def _def_command(self, expr: QAPIExpression):
> >          name = expr['command']
> >          data = expr.get('data')
> >          rets = expr.get('returns')
> > @@ -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*

--js

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.

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

[Markus Armbruster]

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

[...]

> @@ -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
> +
> +
> +class QAPIExpression(_UserDict):
> +    def __init__(
> +        self,
> +        initialdata: Mapping[str, object],
> +        info: QAPISourceInfo,
> +        doc: Optional['QAPIDoc'] = None,
> +    ):

Style nitpick:

           doc: Optional['QAPIDoc'] = None):

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

[...]

[PATCH v2 6/7] qapi: remove _JSONObject

[John Snow]

We can remove this alias as it only has two usages now, and no longer
pays for the confusion of "yet another type".

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/expr.py   | 13 +++----------
 scripts/qapi/parser.py |  5 ++---
 2 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 83fa7a85b06..0a74feab92f 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -48,14 +48,6 @@
 from .source import QAPISourceInfo
 
 
-# Deserialized JSON objects as returned by the parser.
-# The values of this mapping are not necessary to exhaustively type
-# here (and also not practical as long as mypy lacks recursive
-# types), because the purpose of this module is to interrogate that
-# type.
-_JSONObject = Mapping[str, object]
-
-
 # See check_name_str(), below.
 valid_name = re.compile(r'(__[a-z0-9.-]+_)?'
                         r'(x-)?'
@@ -192,7 +184,7 @@ def check_defn_name_str(name: str, info: QAPISourceInfo, meta: str) -> None:
                 info, "%s name should not end in 'List'" % meta)
 
 
-def check_keys(value: _JSONObject,
+def check_keys(value: Mapping[str, object],
                info: QAPISourceInfo,
                source: str,
                required: Collection[str],
@@ -256,7 +248,8 @@ def check_flags(expr: QAPIExpression) -> None:
             expr.info, "flags 'allow-oob' and 'coroutine' are incompatible")
 
 
-def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None:
+def check_if(expr: Mapping[str, object],
+             info: QAPISourceInfo, source: str) -> None:
     """
     Validate the ``if`` member of an object.
 
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 88f6fdfa67b..315660e8671 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -42,9 +42,8 @@
 _ExprValue = Union[List[object], Dict[str, object], str, bool]
 
 
-# FIXME: Consolidate and centralize definitions for _ExprValue,
-# JSONValue, and _JSONObject; currently scattered across several
-# modules.
+# FIXME: Consolidate and centralize definitions for _ExprValue and
+# JSONValue; currently scattered across several modules.
 
 
 # 3.6 workaround: can be removed when Python 3.7+ is our required version.
-- 
2.39.0

[PATCH v2 7/7] qapi: remove JSON value FIXME

[John Snow]

With the two major JSON-ish type hierarchies clarified for distinct
purposes; QAPIExpression for parsed expressions and JSONValue for
introspection data, remove this FIXME as no longer an action item.

In theory, it may be possible to define a completely agnostic
one-size-fits-all JSON type hierarchy that any other user could borrow -
in practice, it's tough to wrangle the differences between invariant,
covariant and contravariant types: input and output parameters demand
different properties of such a structure. As such, it's simply more
trouble than it's worth.

So, declare this "done enough for now".

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/parser.py | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 315660e8671..556092f37b1 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -42,10 +42,6 @@
 _ExprValue = Union[List[object], Dict[str, object], str, bool]
 
 
-# FIXME: Consolidate and centralize definitions for _ExprValue and
-# JSONValue; currently scattered across several modules.
-
-
 # 3.6 workaround: can be removed when Python 3.7+ is our required version.
 if TYPE_CHECKING:
     _UserDict = UserDict[str, object]
-- 
2.39.0

Re: [PATCH v2 0/7] qapi: static typing conversion, pt5c

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This is part five (c), and focuses on sharing strict types between
> parser.py and expr.py.
>
> gitlab: https://gitlab.com/jsnow/qemu/-/commits/python-qapi-cleanup-pt5c
>
> Every commit should pass with:
>  - `isort -c qapi/`
>  - `flake8 qapi/`
>  - `pylint --rcfile=qapi/pylintrc qapi/`
>  - `mypy --config-file=qapi/mypy.ini qapi/`
>
> Some notes on this series:
>
> Patches 2 and 3 are almost entirely superseded by patch 5, but I wasn't
> as confident that Markus would like patch 5, so these patches aren't
> squashed quite as tightly as they could be -- I recommend peeking ahead
> at the cover letters before reviewing the actual patch diffs.

Yes, you're taking a somewhat roundabout path there.

I think I like PATCH 5 well enough.  Do you have a tighter squash in
mind?

> By the end of this series, the only JSON-y types we have left are:
>
> (A) QAPIExpression,
> (B) JSONValue,
> (C) _ExprValue.
>
> The argument I'm making here is that QAPIExpression and JSONValue are
> distinct enough to warrant having both types (for now, at least); and
> that _ExprValue is specialized enough to also warrant its inclusion.
>
> (Brutal honesty: my attempts at unifying this even further had even more
> hacks and unsatisfying conclusions, and fully unifying these types
> should probably wait until we're allowed to rely on some fairly modern
> Python versions.)

Feels okay to me.

Re: [PATCH v2 0/7] qapi: static typing conversion, pt5c

[John Snow]

On Wed, Feb 8, 2023 at 11:31 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> John Snow <jsnow@redhat.com> writes:
>
> > This is part five (c), and focuses on sharing strict types between
> > parser.py and expr.py.
> >
> > gitlab: https://gitlab.com/jsnow/qemu/-/commits/python-qapi-cleanup-pt5c
> >
> > Every commit should pass with:
> >  - `isort -c qapi/`
> >  - `flake8 qapi/`
> >  - `pylint --rcfile=qapi/pylintrc qapi/`
> >  - `mypy --config-file=qapi/mypy.ini qapi/`
> >
> > Some notes on this series:
> >
> > Patches 2 and 3 are almost entirely superseded by patch 5, but I wasn't
> > as confident that Markus would like patch 5, so these patches aren't
> > squashed quite as tightly as they could be -- I recommend peeking ahead
> > at the cover letters before reviewing the actual patch diffs.
>
> Yes, you're taking a somewhat roundabout path there.

The result of trying 10 different things and seeing what was feasible
through trial and error, and rather less the product of an intentional
design. In the name of just getting the ball rolling again, I sent it
out instead of hemming and hawing over perfection. Publish early,
Publish often! ... is what people doing the publishing say. Apologies
to the reviewer.

>
> I think I like PATCH 5 well enough.  Do you have a tighter squash in
> mind?

Not directly. I could essentially just squash them, but that becomes a
pretty big patch.

>
> > By the end of this series, the only JSON-y types we have left are:
> >
> > (A) QAPIExpression,
> > (B) JSONValue,
> > (C) _ExprValue.
> >
> > The argument I'm making here is that QAPIExpression and JSONValue are
> > distinct enough to warrant having both types (for now, at least); and
> > that _ExprValue is specialized enough to also warrant its inclusion.
> >
> > (Brutal honesty: my attempts at unifying this even further had even more
> > hacks and unsatisfying conclusions, and fully unifying these types
> > should probably wait until we're allowed to rely on some fairly modern
> > Python versions.)
>
> Feels okay to me.

Sorry, best I could do with reasonable effort. I will try to improve
the situation when we bump the Python version!

Re: [PATCH v2 0/7] qapi: static typing conversion, pt5c

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Wed, Feb 8, 2023 at 11:31 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > This is part five (c), and focuses on sharing strict types between
>> > parser.py and expr.py.
>> >
>> > gitlab: https://gitlab.com/jsnow/qemu/-/commits/python-qapi-cleanup-pt5c
>> >
>> > Every commit should pass with:
>> >  - `isort -c qapi/`
>> >  - `flake8 qapi/`
>> >  - `pylint --rcfile=qapi/pylintrc qapi/`
>> >  - `mypy --config-file=qapi/mypy.ini qapi/`
>> >
>> > Some notes on this series:
>> >
>> > Patches 2 and 3 are almost entirely superseded by patch 5, but I wasn't
>> > as confident that Markus would like patch 5, so these patches aren't
>> > squashed quite as tightly as they could be -- I recommend peeking ahead
>> > at the cover letters before reviewing the actual patch diffs.
>>
>> Yes, you're taking a somewhat roundabout path there.
>
> The result of trying 10 different things and seeing what was feasible
> through trial and error, and rather less the product of an intentional
> design. In the name of just getting the ball rolling again, I sent it
> out instead of hemming and hawing over perfection. Publish early,
> Publish often! ... is what people doing the publishing say. Apologies
> to the reviewer.

The series was easy enough to review, in good part thanks to your cover
letter.

>> I think I like PATCH 5 well enough.  Do you have a tighter squash in
>> mind?
>
> Not directly. I could essentially just squash them, but that becomes a
> pretty big patch.

Worth a try.

>> > By the end of this series, the only JSON-y types we have left are:
>> >
>> > (A) QAPIExpression,
>> > (B) JSONValue,
>> > (C) _ExprValue.
>> >
>> > The argument I'm making here is that QAPIExpression and JSONValue are
>> > distinct enough to warrant having both types (for now, at least); and
>> > that _ExprValue is specialized enough to also warrant its inclusion.
>> >
>> > (Brutal honesty: my attempts at unifying this even further had even more
>> > hacks and unsatisfying conclusions, and fully unifying these types
>> > should probably wait until we're allowed to rely on some fairly modern
>> > Python versions.)
>>
>> Feels okay to me.
>
> Sorry, best I could do with reasonable effort. I will try to improve
> the situation when we bump the Python version!

Pretty low priority as far as I'm concerned.

mypy is (surprisingly, inexplicably, inexcusably, take your pick) bad at
recursive types.  We've sunk quite a bit of effort into getting the most
out of it around these fundamentally recursive data structures anyway.
I'd like to remind you that my gut feeling was "alright, let's just not
type them then."  You persuaded me to go this far.  No regrets.

The three types you mentioned are indeed distinct.

_ExprValue is the obvious stupid abstract syntax tree for the QAPI
schema language, with str and bool leaves (QAPI doesn't support
floating-point numbers), OrderedDict and list inner nodes.  It is used
for parser output.

Note that the type definition says Dict[str, object].  It's really
OrderedDict.  Plain dict should do for us since 3.6 made it ordered.

QAPIExpression augments _ExprValue, adding a QAPISourceInfo (identifying
the expression's source) and a QAPIDoc (the expressions documentation).
It is used to represent QAPI top-level expressions.  Two observations.

One, having source information only at the top-level is lazy, and leads
to sub-optimal error messages.  I'm not asking for improvement there; we
have bigger fish to fry.

Two, the fact that it wraps around _ExprValue is less than obvious.  The
type doesn't mention _ExprValue.  QAPISchemaParser._add_expr(), the
function we use turn an _ExprValue into a QAPIExpression doesn't mention
it either.  Both work on Mapping[str, object] instead.  Also not a
request for improvement.

JSONValue is an annotated JSON abstract syntax tree.  It's related to
the other two only insofar as JSON is related to the QAPI language.
Unifying plain syntax trees for these separate languages feels like a
dubious proposition to me.  Unifying *annotated* syntax trees feels
worse.