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

[PATCH v3 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/`

V3:
 - Squashed a bunch of patches into the QAPIExpression patch
 - Added a few 'info' locals whenever there were >= 3 usages to help
   minimize some churn
 - Removed some type redundancy from docstrings
 - Brought along the two patches from pt0 that I want merged.
 - Removed 'pexpr' entirely, there's no intermediate state where it's
   needed now.
 - Minor style issues.

John Snow (7):
  qapi: Update flake8 config
  qapi: update pylint configuration
  qapi/expr: Split check_expr out from check_exprs
  qapi/expr: add typing workaround for AbstractSet
  qapi/parser: add QAPIExpression type
  qapi: remove _JSONObject
  qapi: remove JSON value FIXME

 scripts/qapi/.flake8   |   3 +-
 scripts/qapi/expr.py   | 231 +++++++++++++++++++----------------------
 scripts/qapi/parser.py |  49 +++++----
 scripts/qapi/pylintrc  |   1 +
 scripts/qapi/schema.py |  72 +++++++------
 5 files changed, 181 insertions(+), 175 deletions(-)

-- 
2.39.0

[PATCH v3 1/7] qapi: Update flake8 config

[John Snow]

New versions of flake8 don't like same-line comments. (It's a version
newer than what fc37 ships, but it still makes my life easier to fix it
now.)

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/.flake8 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/qapi/.flake8 b/scripts/qapi/.flake8
index 6b158c68b84..a873ff67309 100644
--- a/scripts/qapi/.flake8
+++ b/scripts/qapi/.flake8
@@ -1,2 +1,3 @@
 [flake8]
-extend-ignore = E722  # Prefer pylint's bare-except checks to flake8's
+# Prefer pylint's bare-except checks to flake8's
+extend-ignore = E722
-- 
2.39.0

[PATCH v3 2/7] qapi: update pylint configuration

[John Snow]

Newer versions of pylint disable the "no-self-use" message by
default. Older versions don't, though. If we leave the suppressions in,
pylint yelps about useless options. Just tell pylint to shush.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/pylintrc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
index a7246282030..90546df5345 100644
--- a/scripts/qapi/pylintrc
+++ b/scripts/qapi/pylintrc
@@ -23,6 +23,7 @@ disable=fixme,
         too-many-statements,
         too-many-instance-attributes,
         consider-using-f-string,
+        useless-option-value,
 
 [REPORTS]
 
-- 
2.39.0

[PATCH v3 3/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..b56353bdf84 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, The expression 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

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

[John Snow]

mypy can only narrow the type of `Mapping[str, ...].keys() & Set[str]`
to `AbstractSet[str]` and not a `Set[str]`. As a result, if the type of
an expression is changed to a Mapping[], mypy is unsure if the .pop() is
safe.

A forthcoming commit does exactly that, so wrap the expression in a
set() constructor to force the intermediate expression to be resolved as
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 b56353bdf84..af802367eff 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -622,8 +622,8 @@ def check_expr(expr_elem: _JSONObject) -> 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 v3 5/7] qapi/parser: add QAPIExpression type

[John Snow]

This patch creates a new type, QAPIExpression, which represents a parsed
expression complete with QAPIDoc and QAPISourceInfo.

This patch turns parser.exprs into a list of QAPIExpression 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.

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]"

NB2: Python 3.6 has an oversight for typing UserDict that makes it
impossible to type for both runtime and analysis time. The problem is
described in detail at https://github.com/python/typing/issues/60 - this
workaround can be safely removed when 3.7 is our minimum version.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/expr.py   | 89 +++++++++++++++++-------------------------
 scripts/qapi/parser.py | 54 ++++++++++++++++---------
 scripts/qapi/schema.py | 72 ++++++++++++++++++----------------
 3 files changed, 110 insertions(+), 105 deletions(-)

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index af802367eff..01cfc13fc3a 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 QAPIDoc
+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: _JSONObject, info: QAPISourceInfo) -> None:
+def check_flags(expr: QAPIExpression) -> None:
     """
     Ensure flag members (if present) have valid values.
 
     :param expr: The expression to validate.
-    :param info: QAPI schema source file information.
 
     :raise QAPISemError:
         When certain flags have an invalid value, or when
@@ -243,18 +242,18 @@ def check_flags(expr: _JSONObject, 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: _JSONObject, info: QAPISourceInfo) -> None:
+def check_enum(expr: QAPIExpression) -> None:
     """
     Normalize and validate this expression 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.
@@ -460,6 +458,7 @@ def check_enum(expr: _JSONObject, info: QAPISourceInfo) -> None:
     name = expr['enum']
     members = expr['data']
     prefix = expr.get('prefix')
+    info = expr.info
 
     if not isinstance(members, list):
         raise QAPISemError(info, "'data' must be an array")
@@ -486,12 +485,11 @@ 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: QAPIExpression) -> None:
     """
     Normalize and validate this expression 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 +497,15 @@ def check_struct(expr: _JSONObject, 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: _JSONObject, info: QAPISourceInfo) -> None:
+def check_union(expr: QAPIExpression) -> None:
     """
     Normalize and validate this expression 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.
@@ -517,6 +514,7 @@ def check_union(expr: _JSONObject, info: QAPISourceInfo) -> None:
     base = expr['base']
     discriminator = expr['discriminator']
     members = expr['data']
+    info = expr.info
 
     check_type(base, info, "'base'", allow_dict=name)
     check_name_is_str(discriminator, info, "'discriminator'")
@@ -531,17 +529,17 @@ 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: QAPIExpression) -> None:
     """
     Normalize and validate this expression 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.
     """
     members = expr['data']
+    info = expr.info
 
     if not members:
         raise QAPISemError(info, "'data' must not be empty")
@@ -557,12 +555,11 @@ 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: QAPIExpression) -> None:
     """
     Normalize and validate this expression 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 +569,16 @@ def check_command(expr: _JSONObject, 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: _JSONObject, info: QAPISourceInfo) -> None:
+def check_event(expr: QAPIExpression) -> None:
     """
     Normalize and validate this expression 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,37 +587,23 @@ def check_event(expr: _JSONObject, 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(expr_elem: _JSONObject) -> None:
+def check_expr(expr: QAPIExpression) -> None:
     """
     Validate and normalize a parsed QAPI schema expression.
 
-    :param expr_elem: 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, The expression 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
 
+    info = expr.info
     metas = set(expr.keys() & {
         'enum', 'struct', 'union', 'alternate', 'command', 'event'})
     if len(metas) != 1:
@@ -637,6 +619,7 @@ def check_expr(expr_elem: _JSONObject) -> None:
     info.set_defn(meta, name)
     check_defn_name_str(name, info, meta)
 
+    doc = expr.doc
     if doc:
         if doc.symbol != name:
             raise QAPISemError(
@@ -649,24 +632,24 @@ def check_expr(expr_elem: _JSONObject) -> 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'],
@@ -674,21 +657,21 @@ def check_expr(expr_elem: _JSONObject) -> 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[_JSONObject]) -> List[_JSONObject]:
+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 1b006cdc133..87b46db7fba 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -14,13 +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,
+    Mapping,
     Optional,
     Set,
     Union,
@@ -37,15 +38,30 @@
     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.
+
+
+# 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):
@@ -100,7 +116,7 @@ def __init__(self,
         self.line_pos = 0
 
         # Parser output:
-        self.exprs: List[Dict[str, object]] = []
+        self.exprs: List[QAPIExpression] = []
         self.docs: List[QAPIDoc] = []
 
         # Showtime!
@@ -147,8 +163,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 +180,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: Mapping[str, object],
+                  info: QAPISourceInfo,
+                  doc: Optional['QAPIDoc'] = None) -> None:
+        self.exprs.append(QAPIExpression(expr, info, doc))
+
     @staticmethod
     def reject_expr_doc(doc: Optional['QAPIDoc']) -> None:
         if doc and doc.symbol:
@@ -784,7 +800,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 cd8661125cd..207e4d71f39 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,14 +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'))
+        info = expr.info
         features = self._make_features(expr.get('features'), info)
         self._def_entity(QAPISchemaEnumType(
-            name, info, doc, ifcond, features,
+            name, info, expr.doc, ifcond, features,
             self._make_enum_members(data, info), prefix))
 
     def _make_member(self, name, typ, ifcond, features, info):
@@ -1072,14 +1074,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']
+        info = expr.info
         ifcond = QAPISchemaIfCond(expr.get('if'))
         features = self._make_features(expr.get('features'), info)
         self._def_entity(QAPISchemaObjectType(
-            name, info, doc, ifcond, features, base,
+            name, info, expr.doc, ifcond, features, base,
             self._make_members(data, info),
             None))
 
@@ -1089,11 +1092,13 @@ 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)
+        info = expr.info
         ifcond = QAPISchemaIfCond(expr.get('if'))
         features = self._make_features(expr.get('features'), info)
         if isinstance(base, dict):
@@ -1105,17 +1110,19 @@ def _def_union_type(self, expr, info, doc):
                                QAPISchemaIfCond(value.get('if')),
                                info)
             for (key, value) in data.items()]
-        members = []
+        members: List[QAPISchemaObjectTypeMember] = []
         self._def_entity(
-            QAPISchemaObjectType(name, info, doc, ifcond, features,
+            QAPISchemaObjectType(name, info, expr.doc, ifcond, features,
                                  base, members,
                                  QAPISchemaVariants(
                                      tag_name, 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'))
+        info = expr.info
         features = self._make_features(expr.get('features'), info)
         variants = [
             self._make_variant(key, value['type'],
@@ -1124,11 +1131,11 @@ def _def_alternate_type(self, expr, info, doc):
             for (key, value) in data.items()]
         tag_member = QAPISchemaObjectTypeMember('type', info, 'QType', False)
         self._def_entity(
-            QAPISchemaAlternateType(name, info, doc, ifcond, features,
-                                    QAPISchemaVariants(
-                                        None, info, tag_member, variants)))
+            QAPISchemaAlternateType(
+                name, info, expr.doc, ifcond, features,
+                QAPISchemaVariants(None, 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,6 +1146,7 @@ def _def_command(self, expr, info, doc):
         allow_preconfig = expr.get('allow-preconfig', False)
         coroutine = expr.get('coroutine', False)
         ifcond = QAPISchemaIfCond(expr.get('if'))
+        info = expr.info
         features = self._make_features(expr.get('features'), info)
         if isinstance(data, OrderedDict):
             data = self._make_implicit_object_type(
@@ -1147,44 +1155,42 @@ def _def_command(self, expr, info, doc):
         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,
+        self._def_entity(QAPISchemaCommand(name, 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'))
+        info = expr.info
         features = self._make_features(expr.get('features'), 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))
+        self._def_entity(QAPISchemaEvent(name, 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.get('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

[PATCH v3 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 01cfc13fc3a..8ae9a1e1986 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 87b46db7fba..c165bd3912c 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 v3 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 c165bd3912c..b5afdd703e7 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 v3 3/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.

I don't get this sentence, I'm afraid.  It could tip the balance...

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

Is reducing the nesting level worth sacrificing git-blame?

If we decide it is: there are two "Checked in check_exprs" comments in
need of an update.

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

Harmless accident :)

Re: [PATCH v3 3/7] qapi/expr: Split check_expr out from check_exprs

[Markus Armbruster]

Another observation...

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

[...]

>  def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
>      """
>      Validate and normalize a list of parsed QAPI schema expressions.

The typing is kind of wrong.

The list is the value of QAPISchemaParser.exprs, which is (losely) typed
as List[Dict[str, object]].  It is actually a dict mapping

   'expr' -> _ExprValue
   'info' -> QAPISourceInfo
   'doc'  -> Optional[QAPIDoc]

Thet's not what _JSONObject is!  Its doc string describes it as
"Deserialized JSON objects as returned by the parser", i.e. the same as
_ExprValue.

Works only because _JSONObject is a dict mapping arbitrary keys to
_JSONObject, str or bool.

This patch spreads the flawed typing to the new function.

Gets healed later in the series.

[...]

Re: [PATCH v3 3/7] qapi/expr: Split check_expr out from check_exprs

[John Snow]

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

On Fri, Feb 10, 2023, 7:33 AM Markus Armbruster <armbru@redhat.com> wrote:

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

What I mean is: If you want to handle exceptions without this patch, a
try/catch will add another nesting level to this hundred line function.

By splitting it, you can use the outer looping function as a choke point to
write the handler.

I had a branch once where I use this fact to stop routing "info" into 99%
of expr.py. I did this to use an off the shelf JSON validator while
preserving your custom source info error reporting that identifies the
precise location of the error.

I took that out for now, in the interest of staying focused on the minimum
viable to achieve strict typing, but found this change to be helpful anyway.

Admit that it does muddy the waters with git blame, but... my calculus on
that might just be different from yours.

(To me, git blame is almost always something I have to trace back through
several refactors anyway, so I see the battle as lost before I started.)

I can omit this patch for now if you'd like, it isn't crucial. I just think
I'd still "kinda like to have it". I'll leave it up to you.

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

Whoops, script doesn't understand when I add notes.

> ---
> >  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..b56353bdf84 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:
>
> [...]
>
> >  def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
> >      """
> >      Validate and normalize a list of parsed QAPI schema expressions.
>
> The typing is kind of wrong.
>
> The list is the value of QAPISchemaParser.exprs, which is (losely) typed
> as List[Dict[str, object]].  It is actually a dict mapping
>
>    'expr' -> _ExprValue
>    'info' -> QAPISourceInfo
>    'doc'  -> Optional[QAPIDoc]
>
> Thet's not what _JSONObject is!  Its doc string describes it as
> "Deserialized JSON objects as returned by the parser", i.e. the same as
> _ExprValue.
>
> Works only because _JSONObject is a dict mapping arbitrary keys to
> _JSONObject, str or bool.
>
> This patch spreads the flawed typing to the new function.
>
> Gets healed later in the series.
>

Oops...!

Symptom of patch reordering that happened some time ago, I think. Mea
culpa. Will fix.

(Possibly with some ugly intermediate type that goes away by end of series.)



> [...]
>
>

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

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> mypy can only narrow the type of `Mapping[str, ...].keys() & Set[str]`
> to `AbstractSet[str]` and not a `Set[str]`. As a result, if the type of
> an expression is changed to a Mapping[], mypy is unsure if the .pop() is
> safe.
>
> A forthcoming commit does exactly that, so wrap the expression in a
> set() constructor to force the intermediate expression to be resolved as
> 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 b56353bdf84..af802367eff 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -622,8 +622,8 @@ def check_expr(expr_elem: _JSONObject) -> 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,
                   "expression must have exactly one key"
                   " 'enum', 'struct', 'union', 'alternate',"
                   " 'command', 'event'")
           meta = metas.pop()

I'm mildly surprised that the result of operator & is considered
immutable.  How could it *not* be a freshly created set?  Oh well.

Passing a set to set() is a no-op, so

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Regardless, the code feels a bit too clever to me.  It actually did
already when I wrote it, but I the less clever ways I could think of
were also much more verbose.

The code checks that exactly one of a set of keys is present, and which
one it is.  Any ideas?

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

[John Snow]

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

On Fri, Feb 10, 2023, 10:44 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > mypy can only narrow the type of `Mapping[str, ...].keys() & Set[str]`
> > to `AbstractSet[str]` and not a `Set[str]`. As a result, if the type of
> > an expression is changed to a Mapping[], mypy is unsure if the .pop() is
> > safe.
> >
> > A forthcoming commit does exactly that, so wrap the expression in a
> > set() constructor to force the intermediate expression to be resolved as
> > 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 b56353bdf84..af802367eff 100644
> > --- a/scripts/qapi/expr.py
> > +++ b/scripts/qapi/expr.py
> > @@ -622,8 +622,8 @@ def check_expr(expr_elem: _JSONObject) -> 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,
>                    "expression must have exactly one key"
>                    " 'enum', 'struct', 'union', 'alternate',"
>                    " 'command', 'event'")
>            meta = metas.pop()
>
> I'm mildly surprised that the result of operator & is considered
> immutable.  How could it *not* be a freshly created set?  Oh well.
>

I think because it's up to the LHS type to return the result.

Wait, maybe I can just switch the operand order, lol.


> Passing a set to set() is a no-op, so
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> Regardless, the code feels a bit too clever to me.  It actually did
> already when I wrote it, but I the less clever ways I could think of
> were also much more verbose.
>
> The code checks that exactly one of a set of keys is present, and which
> one it is.  Any ideas?
>

Yeah, it feels too clever but I'm not sure I know something more elegant,
really. I'll consider it while I address your feedback and prepare a respin.

>

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

Re: [PATCH v3 6/7] qapi: remove _JSONObject

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

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

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v3 7/7] qapi: remove JSON value FIXME

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> With the two major JSON-ish type hierarchies clarified for distinct
> purposes; QAPIExpression for parsed expressions and JSONValue for

The comment you remove talks about _ExprValue, not QAPIExpression.

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

I think there's a weightier counter-argument struggling to get out.

As I explained under v2's cover letter, the two types represent things
that are only superficially similar.

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

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.

JSONValue is an annotated JSON abstract syntax tree.  QAPIExpression and
_ExprValue are related to it only insofar as the QAPI schema language is
(rather loosely) patterned after JSON.

Moreover, the two ASTs serve different purposes.  QAPIExpression and
_ExprValue represent *input*: they are produced by a parser and consumed
by a semantic analyzer.  JSONValue represents *output*: it is produced
within a backend so we can separate the JSON text formatting aspect.

Consolidating these two ASTs makes no sense.

Suggest to work this argument into your commit message.

> So, declare this "done enough for now".

Agree.

> 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 c165bd3912c..b5afdd703e7 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]

Re: [PATCH v3 5/7] qapi/parser: add QAPIExpression type

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This patch creates a new type, QAPIExpression, which represents a parsed
> expression complete with QAPIDoc and QAPISourceInfo.
>
> This patch turns parser.exprs into a list of QAPIExpression 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.
>
> 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]"
>
> NB2: Python 3.6 has an oversight for typing UserDict that makes it
> impossible to type for both runtime and analysis time. The problem is
> described in detail at https://github.com/python/typing/issues/60 - this
> workaround can be safely removed when 3.7 is our minimum version.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---

Looks good to me, just one question:

[...]

> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 1b006cdc133..87b46db7fba 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -14,13 +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,
> +    Mapping,
>      Optional,
>      Set,
>      Union,
> @@ -37,15 +38,30 @@
>      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.
> +
> +
> +# 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):

Can we subclass dict instead?

> +    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):

[...]

Re: [PATCH v3 3/7] qapi/expr: Split check_expr out from check_exprs

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> --0000000000003b01fe05f45a096a
> Content-Type: text/plain; charset="UTF-8"
>
> On Fri, Feb 10, 2023, 7:33 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> Another observation...
>>
>> 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.
>>
>
> What I mean is: If you want to handle exceptions without this patch, a
> try/catch will add another nesting level to this hundred line function.
>
> By splitting it, you can use the outer looping function as a choke point to
> write the handler.
>
> I had a branch once where I use this fact to stop routing "info" into 99%
> of expr.py. I did this to use an off the shelf JSON validator while
> preserving your custom source info error reporting that identifies the
> precise location of the error.

When you have to reindent anyway, the drawback "makes git-blame less
useful" evaporates.  When the reindent is due to another level of
nesting, the benefit "reduces nesting" becomes more valuable.

> I took that out for now, in the interest of staying focused on the minimum
> viable to achieve strict typing, but found this change to be helpful anyway.
>
> Admit that it does muddy the waters with git blame, but... my calculus on
> that might just be different from yours.
>
> (To me, git blame is almost always something I have to trace back through
> several refactors anyway, so I see the battle as lost before I started.)
>
> I can omit this patch for now if you'd like, it isn't crucial. I just think
> I'd still "kinda like to have it". I'll leave it up to you.

I'd prefer to shelve it for now.  Bring it back when we have to reindent
anyway.

>> > 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>
>>
>
> Whoops, script doesn't understand when I add notes.
>
>> ---
>> >  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..b56353bdf84 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:
>>
>> [...]
>>
>> >  def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
>> >      """
>> >      Validate and normalize a list of parsed QAPI schema expressions.
>>
>> The typing is kind of wrong.
>>
>> The list is the value of QAPISchemaParser.exprs, which is (losely) typed
>> as List[Dict[str, object]].  It is actually a dict mapping
>>
>>    'expr' -> _ExprValue
>>    'info' -> QAPISourceInfo
>>    'doc'  -> Optional[QAPIDoc]
>>
>> Thet's not what _JSONObject is!  Its doc string describes it as
>> "Deserialized JSON objects as returned by the parser", i.e. the same as
>> _ExprValue.
>>
>> Works only because _JSONObject is a dict mapping arbitrary keys to
>> _JSONObject, str or bool.
>>
>> This patch spreads the flawed typing to the new function.
>>
>> Gets healed later in the series.
>>
>
> Oops...!
>
> Symptom of patch reordering that happened some time ago, I think. Mea
> culpa. Will fix.
>
> (Possibly with some ugly intermediate type that goes away by end of series.)

Only if it's dead easy.

Simply have the commit message point out there's problem being fixed?

>> [...]

Re: [PATCH v3 5/7] qapi/parser: add QAPIExpression type

[John Snow]

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

On Sat, Feb 11, 2023, 1:49 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > This patch creates a new type, QAPIExpression, which represents a parsed
> > expression complete with QAPIDoc and QAPISourceInfo.
> >
> > This patch turns parser.exprs into a list of QAPIExpression 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.
> >
> > 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]"
> >
> > NB2: Python 3.6 has an oversight for typing UserDict that makes it
> > impossible to type for both runtime and analysis time. The problem is
> > described in detail at https://github.com/python/typing/issues/60 - this
> > workaround can be safely removed when 3.7 is our minimum version.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
>
> Looks good to me, just one question:
>
> [...]
>
> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> > index 1b006cdc133..87b46db7fba 100644
> > --- a/scripts/qapi/parser.py
> > +++ b/scripts/qapi/parser.py
> > @@ -14,13 +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,
> > +    Mapping,
> >      Optional,
> >      Set,
> >      Union,
> > @@ -37,15 +38,30 @@
> >      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.
> > +
> > +
> > +# 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):
>
> Can we subclass dict instead?
>

...apparently yes?

class QAPIExpression(Dict[str, object]): ...

Only question I have is what the init signature ought to be, though keeping
it as what I already have here apparently passes the type checker.

I think we lose the ability to do kwargs init in that case, but mypy
doesn't give us a lyskov warning, so maybe it's fine? I'll have to play
around with it. dict is implemented at the cpython level, so I'm not as
sure of how subclassing it works. They document three distinct signatures
for its initializer, which is also a python built-in.

I can rename that initialdata parameter too, pending what I discover above.


> > +    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):
>
> [...]
>
>

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

Re: [PATCH v3 3/7] qapi/expr: Split check_expr out from check_exprs

[John Snow]

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

On Sat, Feb 11, 2023, 5:06 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > --0000000000003b01fe05f45a096a
> > Content-Type: text/plain; charset="UTF-8"
> >
> > On Fri, Feb 10, 2023, 7:33 AM Markus Armbruster <armbru@redhat.com>
> wrote:
> >
> >> Another observation...
> >>
> >> 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.
> >>
> >
> > What I mean is: If you want to handle exceptions without this patch, a
> > try/catch will add another nesting level to this hundred line function.
> >
> > By splitting it, you can use the outer looping function as a choke point
> to
> > write the handler.
> >
> > I had a branch once where I use this fact to stop routing "info" into 99%
> > of expr.py. I did this to use an off the shelf JSON validator while
> > preserving your custom source info error reporting that identifies the
> > precise location of the error.
>
> When you have to reindent anyway, the drawback "makes git-blame less
> useful" evaporates.  When the reindent is due to another level of
> nesting, the benefit "reduces nesting" becomes more valuable.
>
> > I took that out for now, in the interest of staying focused on the
> minimum
> > viable to achieve strict typing, but found this change to be helpful
> anyway.
> >
> > Admit that it does muddy the waters with git blame, but... my calculus on
> > that might just be different from yours.
> >
> > (To me, git blame is almost always something I have to trace back through
> > several refactors anyway, so I see the battle as lost before I started.)
> >
> > I can omit this patch for now if you'd like, it isn't crucial. I just
> think
> > I'd still "kinda like to have it". I'll leave it up to you.
>
> I'd prefer to shelve it for now.  Bring it back when we have to reindent
> anyway.
>

OK. Let me peek ahead to some of my other pending patches and just confirm
there's no stronger reason to keep it...

(/me still ~kiiiinda~ wants it, but admits its not seemingly crucial atm)


> >> > 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>
> >>
> >
> > Whoops, script doesn't understand when I add notes.
> >
> >> ---
> >> >  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..b56353bdf84 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:
> >>
> >> [...]
> >>
> >> >  def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
> >> >      """
> >> >      Validate and normalize a list of parsed QAPI schema expressions.
> >>
> >> The typing is kind of wrong.
> >>
> >> The list is the value of QAPISchemaParser.exprs, which is (losely) typed
> >> as List[Dict[str, object]].  It is actually a dict mapping
> >>
> >>    'expr' -> _ExprValue
> >>    'info' -> QAPISourceInfo
> >>    'doc'  -> Optional[QAPIDoc]
> >>
> >> Thet's not what _JSONObject is!  Its doc string describes it as
> >> "Deserialized JSON objects as returned by the parser", i.e. the same as
> >> _ExprValue.
> >>
> >> Works only because _JSONObject is a dict mapping arbitrary keys to
> >> _JSONObject, str or bool.
> >>
> >> This patch spreads the flawed typing to the new function.
> >>
> >> Gets healed later in the series.
> >>
> >
> > Oops...!
> >
> > Symptom of patch reordering that happened some time ago, I think. Mea
> > culpa. Will fix.
> >
> > (Possibly with some ugly intermediate type that goes away by end of
> series.)
>
> Only if it's dead easy.
>
> Simply have the commit message point out there's problem being fixed?
>
> >> [...]
>
>

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

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

[John Snow]

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

On Fri, Feb 10, 2023, 11:26 AM John Snow <jsnow@redhat.com> wrote:

>
>
> On Fri, Feb 10, 2023, 10:44 AM Markus Armbruster <armbru@redhat.com>
> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > mypy can only narrow the type of `Mapping[str, ...].keys() & Set[str]`
>> > to `AbstractSet[str]` and not a `Set[str]`. As a result, if the type of
>> > an expression is changed to a Mapping[], mypy is unsure if the .pop() is
>> > safe.
>> >
>> > A forthcoming commit does exactly that, so wrap the expression in a
>> > set() constructor to force the intermediate expression to be resolved as
>> > 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 b56353bdf84..af802367eff 100644
>> > --- a/scripts/qapi/expr.py
>> > +++ b/scripts/qapi/expr.py
>> > @@ -622,8 +622,8 @@ def check_expr(expr_elem: _JSONObject) -> 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,
>>                    "expression must have exactly one key"
>>                    " 'enum', 'struct', 'union', 'alternate',"
>>                    " 'command', 'event'")
>>            meta = metas.pop()
>>
>> I'm mildly surprised that the result of operator & is considered
>> immutable.  How could it *not* be a freshly created set?  Oh well.
>>
>
> I think because it's up to the LHS type to return the result.
>
> Wait, maybe I can just switch the operand order, lol.
>

Good news, I can just switch the operand order. Looks cleaner.


>
>> Passing a set to set() is a no-op, so
>>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>
>> Regardless, the code feels a bit too clever to me.  It actually did
>> already when I wrote it, but I the less clever ways I could think of
>> were also much more verbose.
>>
>> The code checks that exactly one of a set of keys is present, and which
>> one it is.  Any ideas?
>>
>
> Yeah, it feels too clever but I'm not sure I know something more elegant,
> really. I'll consider it while I address your feedback and prepare a respin.
>

Still don't have brighter ideas. A problem for another time.

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