[PATCH 00/16] qapi: static typing conversion, pt2

[PATCH 00/16] qapi: static typing conversion, pt2

[John Snow]

based-on: <20200922210101.4081073-1-jsnow@redhat.com>
          [PATCH v2 00/38] qapi: static typing conversion, pt1

Hi, this series adds static type hints to the QAPI module.
This is part two!

Part 2: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt2
Everything: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt6

- Requires Python 3.6+
- Requires mypy 0.770 or newer (for type analysis only)
- Requires pylint 2.6.0 or newer (for lint checking only)

This part of the series focuses on just expr.py.

Type hints are added in patches that add *only* type hints and change no
other behavior. Any necessary changes to behavior to accommodate typing
are split out into their own tiny patches.

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

John Snow (16):
  qapi/expr.py: Remove 'info' argument from nested check_if_str
  qapi/expr.py: Check for dict instead of OrderedDict
  qapi/expr.py: constrain incoming expression types
  qapi/expr.py: Add assertion for union type 'check_dict'
  qapi/expr.py: move string check upwards in check_type
  qapi/expr.py: Check type of 'data' member
  qapi/expr.py: Add casts in a few select cases
  qapi/expr.py: add type hint annotations
  qapi/expr.py: rewrite check_if
  qapi/expr.py: Remove single-letter variable
  qapi/expr.py: enable pylint checks
  qapi/expr.py: Add docstrings
  qapi/expr.py: Modify check_keys to accept any Iterable
  qapi/expr.py: Use tuples instead of lists for static data
  qapi/expr.py: move related checks inside check_xxx functions
  qapi/expr.py: Use an expression checker dispatch table

 scripts/qapi/expr.py  | 440 +++++++++++++++++++++++++++++++-----------
 scripts/qapi/mypy.ini |   5 -
 scripts/qapi/pylintrc |   1 -
 3 files changed, 327 insertions(+), 119 deletions(-)

-- 
2.26.2

[PATCH 01/16] qapi/expr.py: Remove 'info' argument from nested check_if_str

[John Snow]

The function can just use the argument from the scope above. Otherwise,
we get shadowed argument errors because the parameter name clashes with
the name of a variable already in-scope.

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

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 03b31ecfc1..de659a54ad 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -96,7 +96,7 @@ def check_flags(expr, info):
 
 def check_if(expr, info, source):
 
-    def check_if_str(ifcond, info):
+    def check_if_str(ifcond):
         if not isinstance(ifcond, str):
             raise QAPISemError(
                 info,
@@ -116,9 +116,9 @@ def check_if_str(ifcond, info):
             raise QAPISemError(
                 info, "'if' condition [] of %s is useless" % source)
         for elt in ifcond:
-            check_if_str(elt, info)
+            check_if_str(elt)
     else:
-        check_if_str(ifcond, info)
+        check_if_str(ifcond)
         expr['if'] = [ifcond]
 
 
-- 
2.26.2

[PATCH 02/16] qapi/expr.py: Check for dict instead of OrderedDict

[John Snow]

OrderedDict is a subtype of dict, so we can check for a more general form.

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

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index de659a54ad..1872a8a3cc 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -15,7 +15,7 @@
 # See the COPYING file in the top-level directory.
 
 import re
-from collections import OrderedDict
+
 from .common import c_name
 from .error import QAPISemError
 
@@ -123,7 +123,7 @@ def check_if_str(ifcond):
 
 
 def normalize_members(members):
-    if isinstance(members, OrderedDict):
+    if isinstance(members, dict):
         for key, arg in members.items():
             if isinstance(arg, dict):
                 continue
@@ -154,7 +154,7 @@ def check_type(value, info, source,
     if not allow_dict:
         raise QAPISemError(info, "%s should be a type name" % source)
 
-    if not isinstance(value, OrderedDict):
+    if not isinstance(value, dict):
         raise QAPISemError(info,
                            "%s should be an object or type name" % source)
 
-- 
2.26.2

[PATCH 03/16] qapi/expr.py: constrain incoming expression types

[John Snow]

mypy does not know the types of values stored in Dicts that masquerade
as objects. Help the type checker out by constraining the type.

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

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 1872a8a3cc..f6b55a87c1 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -15,9 +15,17 @@
 # See the COPYING file in the top-level directory.
 
 import re
+from typing import MutableMapping, Optional
 
 from .common import c_name
 from .error import QAPISemError
+from .parser import QAPIDoc
+from .source import QAPISourceInfo
+
+
+# Expressions in their raw form are JSON-like structures with arbitrary forms.
+# Minimally, their top-level form must be a mapping of strings to values.
+Expression = MutableMapping[str, object]
 
 
 # Names must be letters, numbers, -, and _.  They must start with letter,
@@ -280,9 +288,20 @@ def check_event(expr, info):
 
 def check_exprs(exprs):
     for expr_elem in exprs:
-        expr = expr_elem['expr']
-        info = expr_elem['info']
-        doc = expr_elem.get('doc')
+        # Expression
+        assert isinstance(expr_elem['expr'], dict)
+        expr: Expression = expr_elem['expr']
+        for key in expr.keys():
+            assert isinstance(key, str)
+
+        # 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
-- 
2.26.2

[PATCH 04/16] qapi/expr.py: Add assertion for union type 'check_dict'

[John Snow]

mypy isn't fond of allowing you to check for bool membership in a
collection of str elements. Guard this lookup for precisely when we were
given a name.

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

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index f6b55a87c1..67892502e9 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -166,7 +166,9 @@ def check_type(value, info, source,
         raise QAPISemError(info,
                            "%s should be an object or type name" % source)
 
-    permit_upper = allow_dict in info.pragma.name_case_whitelist
+    permit_upper = False
+    if isinstance(allow_dict, str):
+        permit_upper = allow_dict in info.pragma.name_case_whitelist
 
     # value is a dictionary, check that each member is okay
     for (key, arg) in value.items():
-- 
2.26.2

[PATCH 05/16] qapi/expr.py: move string check upwards in check_type

[John Snow]

It's a simple case, shimmy the early return upwards.

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

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 67892502e9..3f5af5f5e4 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -143,6 +143,10 @@ def check_type(value, info, source,
     if value is None:
         return
 
+    # Type name
+    if isinstance(value, str):
+        return
+
     # Array type
     if isinstance(value, list):
         if not allow_array:
@@ -153,10 +157,6 @@ def check_type(value, info, source,
                                source)
         return
 
-    # Type name
-    if isinstance(value, str):
-        return
-
     # Anonymous type
 
     if not allow_dict:
-- 
2.26.2

[PATCH 06/16] qapi/expr.py: Check type of 'data' member

[John Snow]

Iterating over the members of data isn't going to work if it's not some
form of dict anyway, but for type safety, formalize it.

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

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 3f5af5f5e4..633f9b9172 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -247,6 +247,9 @@ def check_union(expr, info):
             raise QAPISemError(info, "'discriminator' requires 'base'")
         check_name_is_str(discriminator, info, "'discriminator'")
 
+    if not isinstance(members, dict):
+        raise QAPISemError(info, "'data' must be an object")
+
     for (key, value) in members.items():
         source = "'data' member '%s'" % key
         check_name_str(key, info, source)
@@ -260,6 +263,10 @@ def check_alternate(expr, info):
 
     if not members:
         raise QAPISemError(info, "'data' must not be empty")
+
+    if not isinstance(members, dict):
+        raise QAPISemError(info, "'data' must be an object")
+
     for (key, value) in members.items():
         source = "'data' member '%s'" % key
         check_name_str(key, info, source)
-- 
2.26.2

[PATCH 07/16] qapi/expr.py: Add casts in a few select cases

[John Snow]

Casts are instructions to the type checker only, they aren't "safe" and
should probably be avoided in general. In this case, when we perform
type checking on a nested structure, the type of each field does not
"stick".

We don't need to assert that something is a str if we've already checked
that it is -- use a cast instead for these cases.

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

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 633f9b9172..4a3352a2bd 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -15,7 +15,11 @@
 # See the COPYING file in the top-level directory.
 
 import re
-from typing import MutableMapping, Optional
+from typing import (
+    MutableMapping,
+    Optional,
+    cast,
+)
 
 from .common import c_name
 from .error import QAPISemError
@@ -225,7 +229,7 @@ def check_enum(expr, info):
 
 
 def check_struct(expr, info):
-    name = expr['struct']
+    name = cast(str, expr['struct'])  # Asserted in check_exprs
     members = expr['data']
 
     check_type(members, info, "'data'", allow_dict=name)
@@ -233,7 +237,7 @@ def check_struct(expr, info):
 
 
 def check_union(expr, info):
-    name = expr['union']
+    name = cast(str, expr['union'])  # Asserted in check_exprs
     base = expr.get('base')
     discriminator = expr.get('discriminator')
     members = expr['data']
@@ -330,7 +334,7 @@ def check_exprs(exprs):
         else:
             raise QAPISemError(info, "expression is missing metatype")
 
-        name = expr[meta]
+        name = cast(str, expr[meta])  # asserted right below:
         check_name_is_str(name, info, "'%s'" % meta)
         info.set_defn(meta, name)
         check_defn_name_str(name, info, meta)
-- 
2.26.2

[PATCH 08/16] qapi/expr.py: add type hint annotations

[John Snow]

Annotations do not change runtime behavior.
This commit *only* adds annotations.

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

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 4a3352a2bd..6b064a2138 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -16,8 +16,11 @@
 
 import re
 from typing import (
+    Iterable,
+    List,
     MutableMapping,
     Optional,
+    Union,
     cast,
 )
 
@@ -27,10 +30,11 @@
 from .source import QAPISourceInfo
 
 
-# Expressions in their raw form are JSON-like structures with arbitrary forms.
-# Minimally, their top-level form must be a mapping of strings to values.
-Expression = MutableMapping[str, object]
+# Arbitrary form for a JSON-like object.
+_JSObject = MutableMapping[str, object]
 
+# Expressions in their raw form are (just) JSON-like objects.
+Expression = _JSObject
 
 # Names must be letters, numbers, -, and _.  They must start with letter,
 # except for downstream extensions which must start with __RFQDN_.
@@ -39,14 +43,19 @@
                         '[a-zA-Z][a-zA-Z0-9_-]*$')
 
 
-def check_name_is_str(name, info, source):
+def check_name_is_str(name: object,
+                      info: QAPISourceInfo,
+                      source: str) -> None:
     if not isinstance(name, str):
         raise QAPISemError(info, "%s requires a string name" % source)
 
 
-def check_name_str(name, info, source,
-                   allow_optional=False, enum_member=False,
-                   permit_upper=False):
+def check_name_str(name: str,
+                   info: QAPISourceInfo,
+                   source: str,
+                   allow_optional: bool = False,
+                   enum_member: bool = False,
+                   permit_upper: bool = False) -> None:
     membername = name
 
     if allow_optional and name.startswith('*'):
@@ -66,16 +75,20 @@ def check_name_str(name, info, source,
     assert not membername.startswith('*')
 
 
-def check_defn_name_str(name, info, meta):
+def check_defn_name_str(name: str, info: QAPISourceInfo, meta: str) -> None:
     check_name_str(name, info, meta, permit_upper=True)
     if name.endswith('Kind') or name.endswith('List'):
         raise QAPISemError(
             info, "%s name should not end in '%s'" % (meta, name[-4:]))
 
 
-def check_keys(value, info, source, required, optional):
+def check_keys(value: _JSObject,
+               info: QAPISourceInfo,
+               source: str,
+               required: List[str],
+               optional: List[str]) -> None:
 
-    def pprint(elems):
+    def pprint(elems: Iterable[str]) -> str:
         return ', '.join("'" + e + "'" for e in sorted(elems))
 
     missing = set(required) - set(value)
@@ -95,7 +108,7 @@ def pprint(elems):
                pprint(unknown), pprint(allowed)))
 
 
-def check_flags(expr, info):
+def check_flags(expr: Expression, info: QAPISourceInfo) -> None:
     for key in ['gen', 'success-response']:
         if key in expr and expr[key] is not False:
             raise QAPISemError(
@@ -106,9 +119,9 @@ def check_flags(expr, info):
                 info, "flag '%s' may only use true value" % key)
 
 
-def check_if(expr, info, source):
+def check_if(expr: _JSObject, info: QAPISourceInfo, source: str) -> None:
 
-    def check_if_str(ifcond):
+    def check_if_str(ifcond: object) -> None:
         if not isinstance(ifcond, str):
             raise QAPISemError(
                 info,
@@ -134,7 +147,7 @@ def check_if_str(ifcond):
         expr['if'] = [ifcond]
 
 
-def normalize_members(members):
+def normalize_members(members: object) -> None:
     if isinstance(members, dict):
         for key, arg in members.items():
             if isinstance(arg, dict):
@@ -142,8 +155,11 @@ def normalize_members(members):
             members[key] = {'type': arg}
 
 
-def check_type(value, info, source,
-               allow_array=False, allow_dict=False):
+def check_type(value: Optional[object],
+               info: QAPISourceInfo,
+               source: str,
+               allow_array: bool = False,
+               allow_dict: Union[bool, str] = False) -> None:
     if value is None:
         return
 
@@ -187,7 +203,8 @@ def check_type(value, info, source,
         check_type(arg['type'], info, key_source, allow_array=True)
 
 
-def check_features(features, info):
+def check_features(features: Optional[object],
+                   info: QAPISourceInfo) -> None:
     if features is None:
         return
     if not isinstance(features, list):
@@ -204,7 +221,7 @@ def check_features(features, info):
         check_if(f, info, source)
 
 
-def check_enum(expr, info):
+def check_enum(expr: Expression, info: QAPISourceInfo) -> None:
     name = expr['enum']
     members = expr['data']
     prefix = expr.get('prefix')
@@ -228,7 +245,7 @@ def check_enum(expr, info):
         check_if(member, info, source)
 
 
-def check_struct(expr, info):
+def check_struct(expr: Expression, info: QAPISourceInfo) -> None:
     name = cast(str, expr['struct'])  # Asserted in check_exprs
     members = expr['data']
 
@@ -236,7 +253,7 @@ def check_struct(expr, info):
     check_type(expr.get('base'), info, "'base'")
 
 
-def check_union(expr, info):
+def check_union(expr: Expression, info: QAPISourceInfo) -> None:
     name = cast(str, expr['union'])  # Asserted in check_exprs
     base = expr.get('base')
     discriminator = expr.get('discriminator')
@@ -262,7 +279,7 @@ def check_union(expr, info):
         check_type(value['type'], info, source, allow_array=not base)
 
 
-def check_alternate(expr, info):
+def check_alternate(expr: Expression, info: QAPISourceInfo) -> None:
     members = expr['data']
 
     if not members:
@@ -279,7 +296,7 @@ def check_alternate(expr, info):
         check_type(value['type'], info, source)
 
 
-def check_command(expr, info):
+def check_command(expr: Expression, info: QAPISourceInfo) -> None:
     args = expr.get('data')
     rets = expr.get('returns')
     boxed = expr.get('boxed', False)
@@ -290,7 +307,7 @@ def check_command(expr, info):
     check_type(rets, info, "'returns'", allow_array=True)
 
 
-def check_event(expr, info):
+def check_event(expr: Expression, info: QAPISourceInfo) -> None:
     args = expr.get('data')
     boxed = expr.get('boxed', False)
 
@@ -299,7 +316,7 @@ def check_event(expr, info):
     check_type(args, info, "'data'", allow_dict=not boxed)
 
 
-def check_exprs(exprs):
+def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
     for expr_elem in exprs:
         # Expression
         assert isinstance(expr_elem['expr'], dict)
diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
index 5ab3433c5f..0d0111930f 100644
--- a/scripts/qapi/mypy.ini
+++ b/scripts/qapi/mypy.ini
@@ -14,11 +14,6 @@ disallow_untyped_defs = False
 disallow_incomplete_defs = False
 check_untyped_defs = False
 
-[mypy-qapi.expr]
-disallow_untyped_defs = False
-disallow_incomplete_defs = False
-check_untyped_defs = False
-
 [mypy-qapi.parser]
 disallow_untyped_defs = False
 disallow_incomplete_defs = False
-- 
2.26.2

[PATCH 09/16] qapi/expr.py: rewrite check_if

[John Snow]

This is a only minor rewrite to address some minor style nits.  Don't
compare against the empty list to check for the empty condition, and
move the normalization forward to unify the check on the now-normalized
structure.

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

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 6b064a2138..5d5c3d050d 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -136,15 +136,15 @@ def check_if_str(ifcond: object) -> None:
     ifcond = expr.get('if')
     if ifcond is None:
         return
-    if isinstance(ifcond, list):
-        if ifcond == []:
-            raise QAPISemError(
-                info, "'if' condition [] of %s is useless" % source)
-        for elt in ifcond:
-            check_if_str(elt)
-    else:
-        check_if_str(ifcond)
-        expr['if'] = [ifcond]
+
+    if not isinstance(ifcond, list):
+        ifcond = [ifcond]
+        expr['if'] = ifcond
+    if not ifcond:
+        raise QAPISemError(
+            info, "'if' condition [] of %s is useless" % source)
+    for elt in ifcond:
+        check_if_str(elt)
 
 
 def normalize_members(members: object) -> None:
-- 
2.26.2

[PATCH 10/16] qapi/expr.py: Remove single-letter variable

[John Snow]

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

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 5d5c3d050d..f244e9648c 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -211,14 +211,14 @@ def check_features(features: Optional[object],
         raise QAPISemError(info, "'features' must be an array")
     features[:] = [f if isinstance(f, dict) else {'name': f}
                    for f in features]
-    for f in features:
+    for feature in features:
         source = "'features' member"
-        assert isinstance(f, dict)
-        check_keys(f, info, source, ['name'], ['if'])
-        check_name_is_str(f['name'], info, source)
-        source = "%s '%s'" % (source, f['name'])
-        check_name_str(f['name'], info, source)
-        check_if(f, info, source)
+        assert isinstance(feature, dict)
+        check_keys(feature, info, source, ['name'], ['if'])
+        check_name_is_str(feature['name'], info, source)
+        source = "%s '%s'" % (source, feature['name'])
+        check_name_str(feature['name'], info, source)
+        check_if(feature, info, source)
 
 
 def check_enum(expr: Expression, info: QAPISourceInfo) -> None:
-- 
2.26.2

[PATCH 11/16] qapi/expr.py: enable pylint checks

[John Snow]

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

diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
index 581755351b..d7adb2ba33 100644
--- a/scripts/qapi/pylintrc
+++ b/scripts/qapi/pylintrc
@@ -4,7 +4,6 @@
 # The regex matches against base names, not paths.
 ignore-patterns=doc.py,
                 error.py,
-                expr.py,
                 parser.py,
                 schema.py,
 
-- 
2.26.2

[PATCH 12/16] qapi/expr.py: Add docstrings

[John Snow]

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

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index f244e9648c..4bba09f6e5 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -1,7 +1,5 @@
 # -*- coding: utf-8 -*-
 #
-# Check (context-free) QAPI schema expression structure
-#
 # Copyright IBM, Corp. 2011
 # Copyright (c) 2013-2019 Red Hat Inc.
 #
@@ -14,6 +12,25 @@
 # This work is licensed under the terms of the GNU GPL, version 2.
 # See the COPYING file in the top-level directory.
 
+"""
+Normalize and validate (context-free) QAPI schema expression structures.
+
+After QAPI expressions are parsed from disk, they are stored in
+recursively nested Python data structures using Dict, List, str, bool,
+and int. This module ensures that those nested structures have the
+correct type(s) and key(s) where appropriate for the QAPI context-free
+grammar.
+
+The QAPI schema expression language also allows for syntactic sugar;
+this module also handles the normalization process of these nested
+structures.
+
+See `check_exprs` for the main entry point.
+
+See `schema.QAPISchema` for processing into native Python data
+structures and contextual semantic validation.
+"""
+
 import re
 from typing import (
     Iterable,
@@ -46,6 +63,7 @@
 def check_name_is_str(name: object,
                       info: QAPISourceInfo,
                       source: str) -> None:
+    """Ensures that `name` is a string. [Const]"""
     if not isinstance(name, str):
         raise QAPISemError(info, "%s requires a string name" % source)
 
@@ -56,6 +74,24 @@ def check_name_str(name: str,
                    allow_optional: bool = False,
                    enum_member: bool = False,
                    permit_upper: bool = False) -> None:
+    """
+    Ensures a string is a legal name. [Const]
+
+    A name is legal in the default case when:
+    - It matches ``(__[a-z0-9.-]+_)?[a-z][a-z0-9_-]*``
+    - It does not start with ``q_`` or ``q-``
+
+    :param name:           Name to check.
+    :param info:           QAPI source file information.
+    :param source:         Human-readable str describing "what" this name is.
+    :param allow_optional: Allow the very first character to be ``*``.
+                           (Cannot be used with `enum_member`)
+    :param enum_member:    Allow the very first character to be a digit.
+                           (Cannot be used with `allow_optional`)
+    :param permit_upper:   Allows upper-case characters wherever
+                           lower-case characters are allowed.
+    """
+    assert not (allow_optional and enum_member)
     membername = name
 
     if allow_optional and name.startswith('*'):
@@ -76,6 +112,17 @@ def check_name_str(name: str,
 
 
 def check_defn_name_str(name: str, info: QAPISourceInfo, meta: str) -> None:
+    """
+    Ensures a name is a legal definition name. [Const]
+
+    A legal definition name:
+     - Adheres to the criteria in `check_name_str`, with uppercase permitted
+     - Does not end with ``Kind`` or ``List``.
+
+    :param name: Name to check.
+    :param info: QAPI source file information.
+    :param meta: Type name of the QAPI expression.
+    """
     check_name_str(name, info, meta, permit_upper=True)
     if name.endswith('Kind') or name.endswith('List'):
         raise QAPISemError(
@@ -87,6 +134,15 @@ def check_keys(value: _JSObject,
                source: str,
                required: List[str],
                optional: List[str]) -> None:
+    """
+    Ensures an object has a specific set of keys. [Const]
+
+    :param value:    The object to check.
+    :param info:     QAPI source file information.
+    :param source:   Human-readable str describing "what" this object is.
+    :param required: Keys that *must* be present.
+    :param optional: Keys that *may* be present.
+    """
 
     def pprint(elems: Iterable[str]) -> str:
         return ', '.join("'" + e + "'" for e in sorted(elems))
@@ -109,6 +165,12 @@ def pprint(elems: Iterable[str]) -> str:
 
 
 def check_flags(expr: Expression, info: QAPISourceInfo) -> None:
+    """
+    Ensures common fields in an Expression are correct. [Const]
+
+    :param expr: Expression to validate.
+    :param info: QAPI source file information.
+    """
     for key in ['gen', 'success-response']:
         if key in expr and expr[key] is not False:
             raise QAPISemError(
@@ -120,6 +182,18 @@ def check_flags(expr: Expression, info: QAPISourceInfo) -> None:
 
 
 def check_if(expr: _JSObject, info: QAPISourceInfo, source: str) -> None:
+    """
+    Syntactically validate and normalize the ``if`` field of an object. [RW]
+
+    The ``if`` field may be either a `str` or a `List[str]`.
+    A `str` element will be normalized to `List[str]`.
+
+    Sugared: `Union[str, List[str]]`
+    Ifcond: `List[str]`
+
+    :param expr: A `dict`; the ``if`` field, if present, will be validated.
+    :param info: QAPI source file information.
+    """
 
     def check_if_str(ifcond: object) -> None:
         if not isinstance(ifcond, str):
@@ -148,6 +222,16 @@ def check_if_str(ifcond: object) -> None:
 
 
 def normalize_members(members: object) -> None:
+    """
+    Normalize a "members" value. [RW]
+
+    If `members` is an object, for every value in that object, if that
+    value is not itself already an object, normalize it to
+    ``{'type': value}``.
+
+    Sugared: `Dict[str, Union[str, TypeRef]]`
+    Members: `Dict[str, TypeRef]`
+    """
     if isinstance(members, dict):
         for key, arg in members.items():
             if isinstance(arg, dict):
@@ -160,6 +244,18 @@ def check_type(value: Optional[object],
                source: str,
                allow_array: bool = False,
                allow_dict: Union[bool, str] = False) -> None:
+    """
+    Check the QAPI type of `value`. [RW]
+
+    Python types of `str` or `None` are always allowed.
+
+    :param value:       The value to check.
+    :param info:        QAPI Source file information.
+    :param source:      Human readable string describing "what" the value is.
+    :param allow_array: Allow a `List[str]` of length 1,
+                        which indicates an Array<T> type.
+    :param allow_dict:  Allow a dict, treated as an anonymous type.
+    """
     if value is None:
         return
 
@@ -205,6 +301,15 @@ def check_type(value: Optional[object],
 
 def check_features(features: Optional[object],
                    info: QAPISourceInfo) -> None:
+    """
+    Syntactically validate and normalize the "features" field. [RW]
+
+    `features` may be a List of either `str` or `dict`.
+    Any `str` element will be normalized to `{'name': element}`.
+
+    Sugared: `List[Union[str, Feature]]`
+    Features: `List[Feature]`
+    """
     if features is None:
         return
     if not isinstance(features, list):
@@ -222,6 +327,12 @@ def check_features(features: Optional[object],
 
 
 def check_enum(expr: Expression, info: QAPISourceInfo) -> None:
+    """
+    Validate this `Expression` as an ``enum`` expression. [RW]
+
+    :param expr: `Expression` to validate.
+    :param info: QAPI source file information.
+    """
     name = expr['enum']
     members = expr['data']
     prefix = expr.get('prefix')
@@ -246,6 +357,12 @@ def check_enum(expr: Expression, info: QAPISourceInfo) -> None:
 
 
 def check_struct(expr: Expression, info: QAPISourceInfo) -> None:
+    """
+    Validate this `Expression` as a ``struct`` expression. [RW]
+
+    :param expr: `Expression` to validate.
+    :param info: QAPI source file information.
+    """
     name = cast(str, expr['struct'])  # Asserted in check_exprs
     members = expr['data']
 
@@ -254,6 +371,12 @@ def check_struct(expr: Expression, info: QAPISourceInfo) -> None:
 
 
 def check_union(expr: Expression, info: QAPISourceInfo) -> None:
+    """
+    Validate this `Expression` as a ``union`` expression. [RW]
+
+    :param expr: `Expression` to validate.
+    :param info: QAPI source file information.
+    """
     name = cast(str, expr['union'])  # Asserted in check_exprs
     base = expr.get('base')
     discriminator = expr.get('discriminator')
@@ -280,6 +403,12 @@ def check_union(expr: Expression, info: QAPISourceInfo) -> None:
 
 
 def check_alternate(expr: Expression, info: QAPISourceInfo) -> None:
+    """
+    Validate this `Expression` as an ``alternate`` expression. [RW]
+
+    :param expr: Expression to validate.
+    :param info: QAPI source file information.
+    """
     members = expr['data']
 
     if not members:
@@ -297,6 +426,12 @@ def check_alternate(expr: Expression, info: QAPISourceInfo) -> None:
 
 
 def check_command(expr: Expression, info: QAPISourceInfo) -> None:
+    """
+    Validate this `Expression` as a ``command`` expression. [RW]
+
+    :param expr: `Expression` to validate.
+    :param info: QAPI source file information.
+    """
     args = expr.get('data')
     rets = expr.get('returns')
     boxed = expr.get('boxed', False)
@@ -308,6 +443,16 @@ def check_command(expr: Expression, info: QAPISourceInfo) -> None:
 
 
 def check_event(expr: Expression, info: QAPISourceInfo) -> None:
+    """
+    Normalize and syntactically validate the ``event`` expression. [RW]
+
+    Event:
+        event:    `str`
+        data:     `Optional[Dict[str, Member]]`
+        boxed:    `Optional[bool]`
+        if:       `Optional[Ifcond]`
+        features: `Optional[Features]`
+    """
     args = expr.get('data')
     boxed = expr.get('boxed', False)
 
@@ -317,6 +462,14 @@ def check_event(expr: Expression, info: QAPISourceInfo) -> None:
 
 
 def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
+    """
+    Validate and normalize a list of parsed QAPI schema expressions. [RW]
+
+    This function accepts a list of expressions + metadta as returned by
+    the parser. It destructively normalizes the expressions in-place.
+
+    :param exprs: The list of expressions to normalize/validate.
+    """
     for expr_elem in exprs:
         # Expression
         assert isinstance(expr_elem['expr'], dict)
-- 
2.26.2

[PATCH 13/16] qapi/expr.py: Modify check_keys to accept any Iterable

[John Snow]

This is a very minor adjustment.

a + b is list-specific behavior, but we can accept a wider variety of
types in a more pythonic fashion if we avoid that behavior.

Typing it this way allows callers to use things like dict.keys() and
other iterables that are not their own discrete lists.

Including it just as a statement of practice if nothing else: It's nice
to use the least-specific type possible as function input and use the
most-specific type for returns.

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

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 4bba09f6e5..2a1f37ca88 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -132,8 +132,8 @@ def check_defn_name_str(name: str, info: QAPISourceInfo, meta: str) -> None:
 def check_keys(value: _JSObject,
                info: QAPISourceInfo,
                source: str,
-               required: List[str],
-               optional: List[str]) -> None:
+               required: Iterable[str] = (),
+               optional: Iterable[str] = ()) -> None:
     """
     Ensures an object has a specific set of keys. [Const]
 
@@ -154,7 +154,7 @@ def pprint(elems: Iterable[str]) -> str:
             "%s misses key%s %s"
             % (source, 's' if len(missing) > 1 else '',
                pprint(missing)))
-    allowed = set(required + optional)
+    allowed = set(required) | set(optional)
     unknown = set(value) - allowed
     if unknown:
         raise QAPISemError(
-- 
2.26.2

[PATCH 14/16] qapi/expr.py: Use tuples instead of lists for static data

[John Snow]

It is -- maybe -- possibly -- three nanoseconds faster.

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 2a1f37ca88..69ee9e3f18 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -171,11 +171,11 @@ def check_flags(expr: Expression, info: QAPISourceInfo) -> None:
     :param expr: Expression to validate.
     :param info: QAPI source file information.
     """
-    for key in ['gen', 'success-response']:
+    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)
-    for key in ['boxed', 'allow-oob', 'allow-preconfig']:
+    for key in ('boxed', 'allow-oob', 'allow-preconfig'):
         if key in expr and expr[key] is not True:
             raise QAPISemError(
                 info, "flag '%s' may only use true value" % key)
-- 
2.26.2

[PATCH 15/16] qapi/expr.py: move related checks inside check_xxx functions

[John Snow]

There's not a big obvious difference between the types of checks that
happen in the main function versus the kind that happen in the
functions. Now they're in one place for each of the main types.

As part of the move, spell out the required and optional keywords so
they're obvious at a glance.

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

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 69ee9e3f18..74b2681ef8 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -333,6 +333,10 @@ def check_enum(expr: Expression, info: QAPISourceInfo) -> None:
     :param expr: `Expression` to validate.
     :param info: QAPI source file information.
     """
+    check_keys(expr, info, 'enum',
+               required=('enum', 'data'),
+               optional=('if', 'features', 'prefix'))
+
     name = expr['enum']
     members = expr['data']
     prefix = expr.get('prefix')
@@ -363,6 +367,11 @@ def check_struct(expr: Expression, info: QAPISourceInfo) -> None:
     :param expr: `Expression` to validate.
     :param info: QAPI source file information.
     """
+    check_keys(expr, info, 'struct',
+               required=('struct', 'data'),
+               optional=('base', 'if', 'features'))
+    normalize_members(expr['data'])
+
     name = cast(str, expr['struct'])  # Asserted in check_exprs
     members = expr['data']
 
@@ -377,6 +386,13 @@ def check_union(expr: Expression, info: QAPISourceInfo) -> None:
     :param expr: `Expression` to validate.
     :param info: QAPI source file information.
     """
+    check_keys(expr, info, 'union',
+               required=('union', 'data'),
+               optional=('base', 'discriminator', 'if', 'features'))
+
+    normalize_members(expr.get('base'))
+    normalize_members(expr['data'])
+
     name = cast(str, expr['union'])  # Asserted in check_exprs
     base = expr.get('base')
     discriminator = expr.get('discriminator')
@@ -409,6 +425,11 @@ def check_alternate(expr: Expression, info: QAPISourceInfo) -> None:
     :param expr: Expression to validate.
     :param info: QAPI source file information.
     """
+    check_keys(expr, info, 'alternate',
+               required=('alternate', 'data'),
+               optional=('if', 'features'))
+    normalize_members(expr['data'])
+
     members = expr['data']
 
     if not members:
@@ -432,6 +453,13 @@ def check_command(expr: Expression, info: QAPISourceInfo) -> None:
     :param expr: `Expression` to validate.
     :param info: QAPI source file information.
     """
+    check_keys(expr, info, 'command',
+               required=['command'],
+               optional=('data', 'returns', 'boxed', 'if', 'features',
+                         'gen', 'success-response', 'allow-oob',
+                         'allow-preconfig'))
+    normalize_members(expr.get('data'))
+
     args = expr.get('data')
     rets = expr.get('returns')
     boxed = expr.get('boxed', False)
@@ -453,6 +481,11 @@ def check_event(expr: Expression, info: QAPISourceInfo) -> None:
         if:       `Optional[Ifcond]`
         features: `Optional[Features]`
     """
+    normalize_members(expr.get('data'))
+    check_keys(expr, info, 'event',
+               required=['event'],
+               optional=('data', 'boxed', 'if', 'features'))
+
     args = expr.get('data')
     boxed = expr.get('boxed', False)
 
@@ -519,38 +552,16 @@ def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
                                "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', 'data'],
-                       ['base', 'discriminator', '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'])
-            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'
-- 
2.26.2

[PATCH 16/16] qapi/expr.py: Use an expression checker dispatch table

[John Snow]

This enforces a type signature against all of the top-level expression
check routines without necessarily needing to create some
overcomplicated class hierarchy for them.

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

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 74b2681ef8..cfd342aa04 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -31,8 +31,11 @@
 structures and contextual semantic validation.
 """
 
+from enum import Enum
 import re
 from typing import (
+    Callable,
+    Dict,
     Iterable,
     List,
     MutableMapping,
@@ -494,6 +497,26 @@ def check_event(expr: Expression, info: QAPISourceInfo) -> None:
     check_type(args, info, "'data'", allow_dict=not boxed)
 
 
+class ExpressionType(str, Enum):
+    INCLUDE = 'include'
+    ENUM = 'enum'
+    UNION = 'union'
+    ALTERNATE = 'alternate'
+    STRUCT = 'struct'
+    COMMAND = 'command'
+    EVENT = 'event'
+
+
+_CHECK_FN: Dict[str, Callable[[Expression, QAPISourceInfo], None]] = {
+    'enum': check_enum,
+    'union': check_union,
+    'alternate': check_alternate,
+    'struct': check_struct,
+    'command': check_command,
+    'event': check_event,
+}
+
+
 def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
     """
     Validate and normalize a list of parsed QAPI schema expressions. [RW]
@@ -519,28 +542,20 @@ def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
         assert tmp is None or isinstance(tmp, QAPIDoc)
         doc: Optional[QAPIDoc] = tmp
 
-        if 'include' in expr:
-            continue
-
-        if 'enum' in expr:
-            meta = 'enum'
-        elif 'union' in expr:
-            meta = 'union'
-        elif 'alternate' in expr:
-            meta = 'alternate'
-        elif 'struct' in expr:
-            meta = 'struct'
-        elif 'command' in expr:
-            meta = 'command'
-        elif 'event' in expr:
-            meta = 'event'
+        for kind in ExpressionType:
+            if kind in expr:
+                meta = kind
+                break
         else:
             raise QAPISemError(info, "expression is missing metatype")
 
+        if meta == ExpressionType.INCLUDE:
+            continue
+
         name = cast(str, expr[meta])  # asserted right below:
-        check_name_is_str(name, info, "'%s'" % meta)
-        info.set_defn(meta, name)
-        check_defn_name_str(name, info, meta)
+        check_name_is_str(name, info, "'%s'" % meta.value)
+        info.set_defn(meta.value, name)
+        check_defn_name_str(name, info, meta.value)
 
         if doc:
             if doc.symbol != name:
@@ -551,22 +566,8 @@ def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
             raise QAPISemError(info,
                                "documentation comment required")
 
-        if meta == 'enum':
-            check_enum(expr, info)
-        elif meta == 'union':
-            check_union(expr, info)
-        elif meta == 'alternate':
-            check_alternate(expr, info)
-        elif meta == 'struct':
-            check_struct(expr, info)
-        elif meta == 'command':
-            check_command(expr, info)
-        elif meta == 'event':
-            check_event(expr, info)
-        else:
-            assert False, 'unexpected meta type'
-
-        check_if(expr, info, meta)
+        _CHECK_FN[meta](expr, info)
+        check_if(expr, info, meta.value)
         check_features(expr.get('features'), info)
         check_flags(expr, info)
 
-- 
2.26.2

Re: [PATCH 01/16] qapi/expr.py: Remove 'info' argument from nested check_if_str

[Eduardo Habkost]

On Tue, Sep 22, 2020 at 05:12:58PM -0400, John Snow wrote:
> The function can just use the argument from the scope above. Otherwise,
> we get shadowed argument errors because the parameter name clashes with
> the name of a variable already in-scope.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [PATCH 02/16] qapi/expr.py: Check for dict instead of OrderedDict

[Eduardo Habkost]

On Tue, Sep 22, 2020 at 05:12:59PM -0400, John Snow wrote:
> OrderedDict is a subtype of dict, so we can check for a more general form.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [PATCH 03/16] qapi/expr.py: constrain incoming expression types

[Eduardo Habkost]

On Tue, Sep 22, 2020 at 05:13:00PM -0400, John Snow wrote:
> mypy does not know the types of values stored in Dicts that masquerade
> as objects. Help the type checker out by constraining the type.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/expr.py | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 1872a8a3cc..f6b55a87c1 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -15,9 +15,17 @@
>  # See the COPYING file in the top-level directory.
>  
>  import re
> +from typing import MutableMapping, Optional
>  
>  from .common import c_name
>  from .error import QAPISemError
> +from .parser import QAPIDoc
> +from .source import QAPISourceInfo
> +
> +
> +# Expressions in their raw form are JSON-like structures with arbitrary forms.
> +# Minimally, their top-level form must be a mapping of strings to values.
> +Expression = MutableMapping[str, object]
>  
>  
>  # Names must be letters, numbers, -, and _.  They must start with letter,
> @@ -280,9 +288,20 @@ def check_event(expr, info):
>  
>  def check_exprs(exprs):
>      for expr_elem in exprs:
> -        expr = expr_elem['expr']
> -        info = expr_elem['info']
> -        doc = expr_elem.get('doc')
> +        # Expression
> +        assert isinstance(expr_elem['expr'], dict)
> +        expr: Expression = expr_elem['expr']
> +        for key in expr.keys():
> +            assert isinstance(key, str)

mypy doesn't seem to require the key type asserts, on my testing.

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

Do you need a separate variable here?  This seems to work too:

        doc = expr_elem.get('doc')
        assert doc is None or isinstance(doc, QAPIDoc)

after the assert, mypy will infer the type of doc to be
Optional[QAPIDoc].

None of this should block a useful 120-patch cleanup series, so:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

>  
>          if 'include' in expr:
>              continue
> -- 
> 2.26.2
> 

-- 
Eduardo

Re: [PATCH 04/16] qapi/expr.py: Add assertion for union type 'check_dict'

[Eduardo Habkost]

On Tue, Sep 22, 2020 at 05:13:01PM -0400, John Snow wrote:
> mypy isn't fond of allowing you to check for bool membership in a
> collection of str elements. Guard this lookup for precisely when we were
> given a name.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/expr.py | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index f6b55a87c1..67892502e9 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -166,7 +166,9 @@ def check_type(value, info, source,
>          raise QAPISemError(info,
>                             "%s should be an object or type name" % source)
>  
> -    permit_upper = allow_dict in info.pragma.name_case_whitelist
> +    permit_upper = False
> +    if isinstance(allow_dict, str):
> +        permit_upper = allow_dict in info.pragma.name_case_whitelist

Well, this keeps existing behavior, so:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

But: what exactly is the meaning of allow_dict=False,
allow_dict=True, and allow_dict being a string?


>  
>      # value is a dictionary, check that each member is okay
>      for (key, arg) in value.items():
> -- 
> 2.26.2
> 

-- 
Eduardo

Re: [PATCH 05/16] qapi/expr.py: move string check upwards in check_type

[Eduardo Habkost]

On Tue, Sep 22, 2020 at 05:13:02PM -0400, John Snow wrote:
> It's a simple case, shimmy the early return upwards.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [PATCH 06/16] qapi/expr.py: Check type of 'data' member

[Eduardo Habkost]

On Tue, Sep 22, 2020 at 05:13:03PM -0400, John Snow wrote:
> Iterating over the members of data isn't going to work if it's not some
> form of dict anyway, but for type safety, formalize it.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [PATCH 07/16] qapi/expr.py: Add casts in a few select cases

[Eduardo Habkost]

On Tue, Sep 22, 2020 at 05:13:04PM -0400, John Snow wrote:
> Casts are instructions to the type checker only, they aren't "safe" and
> should probably be avoided in general. In this case, when we perform
> type checking on a nested structure, the type of each field does not
> "stick".
> 
> We don't need to assert that something is a str if we've already checked
> that it is -- use a cast instead for these cases.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [PATCH 08/16] qapi/expr.py: add type hint annotations

[Eduardo Habkost]

On Tue, Sep 22, 2020 at 05:13:05PM -0400, John Snow wrote:
> Annotations do not change runtime behavior.
> This commit *only* adds annotations.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [PATCH 09/16] qapi/expr.py: rewrite check_if

[Eduardo Habkost]

On Tue, Sep 22, 2020 at 05:13:06PM -0400, John Snow wrote:
> This is a only minor rewrite to address some minor style nits.  Don't
> compare against the empty list to check for the empty condition, and
> move the normalization forward to unify the check on the now-normalized
> structure.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [PATCH 10/16] qapi/expr.py: Remove single-letter variable

[Eduardo Habkost]

On Tue, Sep 22, 2020 at 05:13:07PM -0400, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [PATCH 11/16] qapi/expr.py: enable pylint checks

[Eduardo Habkost]

On Tue, Sep 22, 2020 at 05:13:08PM -0400, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>

Tested-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [PATCH 12/16] qapi/expr.py: Add docstrings

[Eduardo Habkost]

On Tue, Sep 22, 2020 at 05:13:09PM -0400, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
[...]
> @@ -160,6 +244,18 @@ def check_type(value: Optional[object],
>                 source: str,
>                 allow_array: bool = False,
>                 allow_dict: Union[bool, str] = False) -> None:
> +    """
> +    Check the QAPI type of `value`. [RW]
> +
> +    Python types of `str` or `None` are always allowed.
> +
> +    :param value:       The value to check.
> +    :param info:        QAPI Source file information.
> +    :param source:      Human readable string describing "what" the value is.
> +    :param allow_array: Allow a `List[str]` of length 1,
> +                        which indicates an Array<T> type.
> +    :param allow_dict:  Allow a dict, treated as an anonymous type.

I was hoping the docstring would explain what happens when
allow_dict is a string.

-- 
Eduardo

Re: [PATCH 13/16] qapi/expr.py: Modify check_keys to accept any Iterable

[Eduardo Habkost]

On Tue, Sep 22, 2020 at 05:13:10PM -0400, John Snow wrote:
> This is a very minor adjustment.
> 
> a + b is list-specific behavior, but we can accept a wider variety of
> types in a more pythonic fashion if we avoid that behavior.
> 
> Typing it this way allows callers to use things like dict.keys() and
> other iterables that are not their own discrete lists.
> 
> Including it just as a statement of practice if nothing else: It's nice
> to use the least-specific type possible as function input and use the
> most-specific type for returns.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [PATCH 14/16] qapi/expr.py: Use tuples instead of lists for static data

[Eduardo Habkost]

On Tue, Sep 22, 2020 at 05:13:11PM -0400, John Snow wrote:
> It is -- maybe -- possibly -- three nanoseconds faster.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [PATCH 15/16] qapi/expr.py: move related checks inside check_xxx functions

[Eduardo Habkost]

On Tue, Sep 22, 2020 at 05:13:12PM -0400, John Snow wrote:
> There's not a big obvious difference between the types of checks that
> happen in the main function versus the kind that happen in the
> functions. Now they're in one place for each of the main types.
> 
> As part of the move, spell out the required and optional keywords so
> they're obvious at a glance.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/expr.py | 55 ++++++++++++++++++++++++++------------------
>  1 file changed, 33 insertions(+), 22 deletions(-)
> 
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 69ee9e3f18..74b2681ef8 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -333,6 +333,10 @@ def check_enum(expr: Expression, info: QAPISourceInfo) -> None:
>      :param expr: `Expression` to validate.
>      :param info: QAPI source file information.
>      """
> +    check_keys(expr, info, 'enum',
> +               required=('enum', 'data'),
> +               optional=('if', 'features', 'prefix'))
> +
>      name = expr['enum']
>      members = expr['data']
>      prefix = expr.get('prefix')
> @@ -363,6 +367,11 @@ def check_struct(expr: Expression, info: QAPISourceInfo) -> None:
>      :param expr: `Expression` to validate.
>      :param info: QAPI source file information.
>      """
> +    check_keys(expr, info, 'struct',
> +               required=('struct', 'data'),
> +               optional=('base', 'if', 'features'))
> +    normalize_members(expr['data'])
> +
>      name = cast(str, expr['struct'])  # Asserted in check_exprs
>      members = expr['data']
>  
> @@ -377,6 +386,13 @@ def check_union(expr: Expression, info: QAPISourceInfo) -> None:
>      :param expr: `Expression` to validate.
>      :param info: QAPI source file information.
>      """
> +    check_keys(expr, info, 'union',
> +               required=('union', 'data'),
> +               optional=('base', 'discriminator', 'if', 'features'))
> +
> +    normalize_members(expr.get('base'))
> +    normalize_members(expr['data'])
> +
>      name = cast(str, expr['union'])  # Asserted in check_exprs
>      base = expr.get('base')
>      discriminator = expr.get('discriminator')
> @@ -409,6 +425,11 @@ def check_alternate(expr: Expression, info: QAPISourceInfo) -> None:
>      :param expr: Expression to validate.
>      :param info: QAPI source file information.
>      """
> +    check_keys(expr, info, 'alternate',
> +               required=('alternate', 'data'),
> +               optional=('if', 'features'))
> +    normalize_members(expr['data'])
> +
>      members = expr['data']
>  
>      if not members:
> @@ -432,6 +453,13 @@ def check_command(expr: Expression, info: QAPISourceInfo) -> None:
>      :param expr: `Expression` to validate.
>      :param info: QAPI source file information.
>      """
> +    check_keys(expr, info, 'command',
> +               required=['command'],
> +               optional=('data', 'returns', 'boxed', 'if', 'features',
> +                         'gen', 'success-response', 'allow-oob',
> +                         'allow-preconfig'))
> +    normalize_members(expr.get('data'))
> +
>      args = expr.get('data')
>      rets = expr.get('returns')
>      boxed = expr.get('boxed', False)
> @@ -453,6 +481,11 @@ def check_event(expr: Expression, info: QAPISourceInfo) -> None:
>          if:       `Optional[Ifcond]`
>          features: `Optional[Features]`
>      """
> +    normalize_members(expr.get('data'))
> +    check_keys(expr, info, 'event',
> +               required=['event'],
> +               optional=('data', 'boxed', 'if', 'features'))

Why is the order reversed here?  The other functions call
check_keys() before normalize_members().


> +
>      args = expr.get('data')
>      boxed = expr.get('boxed', False)
>  
> @@ -519,38 +552,16 @@ def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
>                                 "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', 'data'],
> -                       ['base', 'discriminator', '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'])
> -            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'
> -- 
> 2.26.2
> 

-- 
Eduardo

Re: [PATCH 16/16] qapi/expr.py: Use an expression checker dispatch table

[Eduardo Habkost]

On Tue, Sep 22, 2020 at 05:13:13PM -0400, John Snow wrote:
> This enforces a type signature against all of the top-level expression
> check routines without necessarily needing to create some
> overcomplicated class hierarchy for them.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/expr.py | 69 ++++++++++++++++++++++----------------------
>  1 file changed, 35 insertions(+), 34 deletions(-)
> 
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 74b2681ef8..cfd342aa04 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -31,8 +31,11 @@
>  structures and contextual semantic validation.
>  """
>  
> +from enum import Enum
>  import re
>  from typing import (
> +    Callable,
> +    Dict,
>      Iterable,
>      List,
>      MutableMapping,
> @@ -494,6 +497,26 @@ def check_event(expr: Expression, info: QAPISourceInfo) -> None:
>      check_type(args, info, "'data'", allow_dict=not boxed)
>  
>  
> +class ExpressionType(str, Enum):
> +    INCLUDE = 'include'
> +    ENUM = 'enum'
> +    UNION = 'union'
> +    ALTERNATE = 'alternate'
> +    STRUCT = 'struct'
> +    COMMAND = 'command'
> +    EVENT = 'event'
> +
> +
> +_CHECK_FN: Dict[str, Callable[[Expression, QAPISourceInfo], None]] = {
> +    'enum': check_enum,
> +    'union': check_union,
> +    'alternate': check_alternate,
> +    'struct': check_struct,
> +    'command': check_command,
> +    'event': check_event,
> +}
> +
> +
>  def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
>      """
>      Validate and normalize a list of parsed QAPI schema expressions. [RW]
> @@ -519,28 +542,20 @@ def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
>          assert tmp is None or isinstance(tmp, QAPIDoc)
>          doc: Optional[QAPIDoc] = tmp
>  
> -        if 'include' in expr:
> -            continue
> -
> -        if 'enum' in expr:
> -            meta = 'enum'
> -        elif 'union' in expr:
> -            meta = 'union'
> -        elif 'alternate' in expr:
> -            meta = 'alternate'
> -        elif 'struct' in expr:
> -            meta = 'struct'
> -        elif 'command' in expr:
> -            meta = 'command'
> -        elif 'event' in expr:
> -            meta = 'event'
> +        for kind in ExpressionType:
> +            if kind in expr:
> +                meta = kind

I see lots of meta.value expressions below.  Maybe setting
  meta = kind.value
will make the code more readable?

I don't think this should block an obvious improvement to the
code, so:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

> +                break
>          else:
>              raise QAPISemError(info, "expression is missing metatype")
>  
> +        if meta == ExpressionType.INCLUDE:
> +            continue
> +
>          name = cast(str, expr[meta])  # asserted right below:
> -        check_name_is_str(name, info, "'%s'" % meta)
> -        info.set_defn(meta, name)
> -        check_defn_name_str(name, info, meta)
> +        check_name_is_str(name, info, "'%s'" % meta.value)
> +        info.set_defn(meta.value, name)
> +        check_defn_name_str(name, info, meta.value)
>  
>          if doc:
>              if doc.symbol != name:
> @@ -551,22 +566,8 @@ def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
>              raise QAPISemError(info,
>                                 "documentation comment required")
>  
> -        if meta == 'enum':
> -            check_enum(expr, info)
> -        elif meta == 'union':
> -            check_union(expr, info)
> -        elif meta == 'alternate':
> -            check_alternate(expr, info)
> -        elif meta == 'struct':
> -            check_struct(expr, info)
> -        elif meta == 'command':
> -            check_command(expr, info)
> -        elif meta == 'event':
> -            check_event(expr, info)
> -        else:
> -            assert False, 'unexpected meta type'
> -
> -        check_if(expr, info, meta)
> +        _CHECK_FN[meta](expr, info)
> +        check_if(expr, info, meta.value)
>          check_features(expr.get('features'), info)
>          check_flags(expr, info)
>  
> -- 
> 2.26.2
> 

-- 
Eduardo

Re: [PATCH 01/16] qapi/expr.py: Remove 'info' argument from nested check_if_str

[Cleber Rosa]

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

On Tue, Sep 22, 2020 at 05:12:58PM -0400, John Snow wrote:
> The function can just use the argument from the scope above. Otherwise,
> we get shadowed argument errors because the parameter name clashes with
> the name of a variable already in-scope.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 02/16] qapi/expr.py: Check for dict instead of OrderedDict

[Cleber Rosa]

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

On Tue, Sep 22, 2020 at 05:12:59PM -0400, John Snow wrote:
> OrderedDict is a subtype of dict, so we can check for a more general form.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 03/16] qapi/expr.py: constrain incoming expression types

[Cleber Rosa]

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

On Wed, Sep 23, 2020 at 03:42:24PM -0400, Eduardo Habkost wrote:
> On Tue, Sep 22, 2020 at 05:13:00PM -0400, John Snow wrote:
> > mypy does not know the types of values stored in Dicts that masquerade
> > as objects. Help the type checker out by constraining the type.
> > 
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  scripts/qapi/expr.py | 25 ++++++++++++++++++++++---
> >  1 file changed, 22 insertions(+), 3 deletions(-)
> > 
> > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> > index 1872a8a3cc..f6b55a87c1 100644
> > --- a/scripts/qapi/expr.py
> > +++ b/scripts/qapi/expr.py
> > @@ -15,9 +15,17 @@
> >  # See the COPYING file in the top-level directory.
> >  
> >  import re
> > +from typing import MutableMapping, Optional
> >  
> >  from .common import c_name
> >  from .error import QAPISemError
> > +from .parser import QAPIDoc
> > +from .source import QAPISourceInfo
> > +
> > +
> > +# Expressions in their raw form are JSON-like structures with arbitrary forms.
> > +# Minimally, their top-level form must be a mapping of strings to values.
> > +Expression = MutableMapping[str, object]
> >  
> >  
> >  # Names must be letters, numbers, -, and _.  They must start with letter,
> > @@ -280,9 +288,20 @@ def check_event(expr, info):
> >  
> >  def check_exprs(exprs):
> >      for expr_elem in exprs:
> > -        expr = expr_elem['expr']
> > -        info = expr_elem['info']
> > -        doc = expr_elem.get('doc')
> > +        # Expression
> > +        assert isinstance(expr_elem['expr'], dict)
> > +        expr: Expression = expr_elem['expr']
> > +        for key in expr.keys():
> > +            assert isinstance(key, str)
> 
> mypy doesn't seem to require the key type asserts, on my testing.
>

Do you mean that mypy actually takes notice of the type assert?  And
includes that as source of information for the type check or am I
misinterpreting you?

BTW, what I understood from this assert is that a more specific
type than the MutableMapping is desirable here.  Did I get that
right John?

- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 03/16] qapi/expr.py: constrain incoming expression types

[Cleber Rosa]

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

On Tue, Sep 22, 2020 at 05:13:00PM -0400, John Snow wrote:
> mypy does not know the types of values stored in Dicts that masquerade
> as objects. Help the type checker out by constraining the type.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 04/16] qapi/expr.py: Add assertion for union type 'check_dict'

[Cleber Rosa]

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

On Tue, Sep 22, 2020 at 05:13:01PM -0400, John Snow wrote:
> mypy isn't fond of allowing you to check for bool membership in a
> collection of str elements. Guard this lookup for precisely when we were
> given a name.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 05/16] qapi/expr.py: move string check upwards in check_type

[Cleber Rosa]

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

On Tue, Sep 22, 2020 at 05:13:02PM -0400, John Snow wrote:
> It's a simple case, shimmy the early return upwards.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

I'm only acking this because of the very graphical "shimmy dance"
movie that played in my mind.

Reviewed-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 06/16] qapi/expr.py: Check type of 'data' member

[Cleber Rosa]

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

On Tue, Sep 22, 2020 at 05:13:03PM -0400, John Snow wrote:
> Iterating over the members of data isn't going to work if it's not some
> form of dict anyway, but for type safety, formalize it.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/expr.py | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 3f5af5f5e4..633f9b9172 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -247,6 +247,9 @@ def check_union(expr, info):
>              raise QAPISemError(info, "'discriminator' requires 'base'")
>          check_name_is_str(discriminator, info, "'discriminator'")
>  
> +    if not isinstance(members, dict):
> +        raise QAPISemError(info, "'data' must be an object")
> +

Don't you mean "must be a dict" ?

>      for (key, value) in members.items():
>          source = "'data' member '%s'" % key
>          check_name_str(key, info, source)
> @@ -260,6 +263,10 @@ def check_alternate(expr, info):
>  
>      if not members:
>          raise QAPISemError(info, "'data' must not be empty")
> +
> +    if not isinstance(members, dict):
> +        raise QAPISemError(info, "'data' must be an object")
> +

Same here?

- Cleber.

>      for (key, value) in members.items():
>          source = "'data' member '%s'" % key
>          check_name_str(key, info, source)
> -- 
> 2.26.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 07/16] qapi/expr.py: Add casts in a few select cases

[Cleber Rosa]

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

On Tue, Sep 22, 2020 at 05:13:04PM -0400, John Snow wrote:
> Casts are instructions to the type checker only, they aren't "safe" and
> should probably be avoided in general. In this case, when we perform
> type checking on a nested structure, the type of each field does not
> "stick".
> 
> We don't need to assert that something is a str if we've already checked
> that it is -- use a cast instead for these cases.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 03/16] qapi/expr.py: constrain incoming expression types

[John Snow]

On 9/23/20 3:42 PM, Eduardo Habkost wrote:
> On Tue, Sep 22, 2020 at 05:13:00PM -0400, John Snow wrote:
>> mypy does not know the types of values stored in Dicts that masquerade
>> as objects. Help the type checker out by constraining the type.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qapi/expr.py | 25 ++++++++++++++++++++++---
>>   1 file changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index 1872a8a3cc..f6b55a87c1 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -15,9 +15,17 @@
>>   # See the COPYING file in the top-level directory.
>>   
>>   import re
>> +from typing import MutableMapping, Optional
>>   
>>   from .common import c_name
>>   from .error import QAPISemError
>> +from .parser import QAPIDoc
>> +from .source import QAPISourceInfo
>> +
>> +
>> +# Expressions in their raw form are JSON-like structures with arbitrary forms.
>> +# Minimally, their top-level form must be a mapping of strings to values.
>> +Expression = MutableMapping[str, object]
>>   
>>   
>>   # Names must be letters, numbers, -, and _.  They must start with letter,
>> @@ -280,9 +288,20 @@ def check_event(expr, info):
>>   
>>   def check_exprs(exprs):
>>       for expr_elem in exprs:
>> -        expr = expr_elem['expr']
>> -        info = expr_elem['info']
>> -        doc = expr_elem.get('doc')
>> +        # Expression
>> +        assert isinstance(expr_elem['expr'], dict)
>> +        expr: Expression = expr_elem['expr']
>> +        for key in expr.keys():
>> +            assert isinstance(key, str)
> 
> mypy doesn't seem to require the key type asserts, on my testing.
> 

Strictly no. This code is removed somewhere in part 5 when I introduce a 
typed structure to carry this data from the Parser to the Expression 
checker.

(Sometimes, these asserts were for my own sake.)

>> +
>> +        # 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
> 
> Do you need a separate variable here?  This seems to work too:
> 
>          doc = expr_elem.get('doc')
>          assert doc is None or isinstance(doc, QAPIDoc)
> 
> after the assert, mypy will infer the type of doc to be
> Optional[QAPIDoc].
> 

In full honesty, I don't recall... but this code does get replaced by 
the end of this marathon.

> None of this should block a useful 120-patch cleanup series, so:
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 

Thanks!

Re: [PATCH 03/16] qapi/expr.py: constrain incoming expression types

[John Snow]

On 9/24/20 8:05 PM, Cleber Rosa wrote:
> On Wed, Sep 23, 2020 at 03:42:24PM -0400, Eduardo Habkost wrote:
>> On Tue, Sep 22, 2020 at 05:13:00PM -0400, John Snow wrote:
>>> mypy does not know the types of values stored in Dicts that masquerade
>>> as objects. Help the type checker out by constraining the type.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   scripts/qapi/expr.py | 25 ++++++++++++++++++++++---
>>>   1 file changed, 22 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>>> index 1872a8a3cc..f6b55a87c1 100644
>>> --- a/scripts/qapi/expr.py
>>> +++ b/scripts/qapi/expr.py
>>> @@ -15,9 +15,17 @@
>>>   # See the COPYING file in the top-level directory.
>>>   
>>>   import re
>>> +from typing import MutableMapping, Optional
>>>   
>>>   from .common import c_name
>>>   from .error import QAPISemError
>>> +from .parser import QAPIDoc
>>> +from .source import QAPISourceInfo
>>> +
>>> +
>>> +# Expressions in their raw form are JSON-like structures with arbitrary forms.
>>> +# Minimally, their top-level form must be a mapping of strings to values.
>>> +Expression = MutableMapping[str, object]
>>>   
>>>   
>>>   # Names must be letters, numbers, -, and _.  They must start with letter,
>>> @@ -280,9 +288,20 @@ def check_event(expr, info):
>>>   
>>>   def check_exprs(exprs):
>>>       for expr_elem in exprs:
>>> -        expr = expr_elem['expr']
>>> -        info = expr_elem['info']
>>> -        doc = expr_elem.get('doc')
>>> +        # Expression
>>> +        assert isinstance(expr_elem['expr'], dict)
>>> +        expr: Expression = expr_elem['expr']
>>> +        for key in expr.keys():
>>> +            assert isinstance(key, str)
>>
>> mypy doesn't seem to require the key type asserts, on my testing.
>>
> 
> Do you mean that mypy actually takes notice of the type assert?  And
> includes that as source of information for the type check or am I
> misinterpreting you?
> 
> BTW, what I understood from this assert is that a more specific
> type than the MutableMapping is desirable here.  Did I get that
> right John?
> 

Yes, we do want a more specific type. We'll get one somewhere in part 5 
when parser.py gets a workout.

> - Cleber.
> 

mypy takes notice of assert isinstance(x, FooType) because below this 
line, it is not possible for x to be anything other than a FooType.

You can use this to "downcast" types.

you can use cast() too, but those are "unsafe", in that they don't 
actually check. assert *will* check.

You can also constrain types by doing a simple:

if isinstance(x, FooType):
     x.FooMethod()

Re: [PATCH 08/16] qapi/expr.py: add type hint annotations

[Cleber Rosa]

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

On Tue, Sep 22, 2020 at 05:13:05PM -0400, John Snow wrote:
> Annotations do not change runtime behavior.
> This commit *only* adds annotations.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 04/16] qapi/expr.py: Add assertion for union type 'check_dict'

[John Snow]

On 9/23/20 3:53 PM, Eduardo Habkost wrote:
> On Tue, Sep 22, 2020 at 05:13:01PM -0400, John Snow wrote:
>> mypy isn't fond of allowing you to check for bool membership in a
>> collection of str elements. Guard this lookup for precisely when we were
>> given a name.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qapi/expr.py | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index f6b55a87c1..67892502e9 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -166,7 +166,9 @@ def check_type(value, info, source,
>>           raise QAPISemError(info,
>>                              "%s should be an object or type name" % source)
>>   
>> -    permit_upper = allow_dict in info.pragma.name_case_whitelist
>> +    permit_upper = False
>> +    if isinstance(allow_dict, str):
>> +        permit_upper = allow_dict in info.pragma.name_case_whitelist
> 
> Well, this keeps existing behavior, so:
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> But: what exactly is the meaning of allow_dict=False,
> allow_dict=True, and allow_dict being a string?
> 
> 

allow_dict = True -- allows the type to be an object describing the type.

allow_dict: str -- allows the type to be an object (like True), but also 
passes a name in for the purposes of validating the name with the pragma 
whitelist(!)

>>   
>>       # value is a dictionary, check that each member is okay
>>       for (key, arg) in value.items():
>> -- 
>> 2.26.2
>>
> 

Re: [PATCH 06/16] qapi/expr.py: Check type of 'data' member

[John Snow]

On 9/24/20 8:31 PM, Cleber Rosa wrote:
> On Tue, Sep 22, 2020 at 05:13:03PM -0400, John Snow wrote:
>> Iterating over the members of data isn't going to work if it's not some
>> form of dict anyway, but for type safety, formalize it.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qapi/expr.py | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index 3f5af5f5e4..633f9b9172 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -247,6 +247,9 @@ def check_union(expr, info):
>>               raise QAPISemError(info, "'discriminator' requires 'base'")
>>           check_name_is_str(discriminator, info, "'discriminator'")
>>   
>> +    if not isinstance(members, dict):
>> +        raise QAPISemError(info, "'data' must be an object")
>> +
> 
> Don't you mean "must be a dict" ?
> 

This error is speaking JSON-ese! json objects become python dicts, so if 
we didn't get a python dict here, we didn't get a json object.

Re: [PATCH 09/16] qapi/expr.py: rewrite check_if

[Cleber Rosa]

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

On Tue, Sep 22, 2020 at 05:13:06PM -0400, John Snow wrote:
> This is a only minor rewrite to address some minor style nits.  Don't
> compare against the empty list to check for the empty condition, and
> move the normalization forward to unify the check on the now-normalized
> structure.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Cleber Rosa <crosa@redhat.com>

And just to make sure there was not change in the logic:

Tested-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 10/16] qapi/expr.py: Remove single-letter variable

[Cleber Rosa]

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

On Tue, Sep 22, 2020 at 05:13:07PM -0400, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 12/16] qapi/expr.py: Add docstrings

[John Snow]

On 9/23/20 4:16 PM, Eduardo Habkost wrote:
> On Tue, Sep 22, 2020 at 05:13:09PM -0400, John Snow wrote:
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
> [...]
>> @@ -160,6 +244,18 @@ def check_type(value: Optional[object],
>>                  source: str,
>>                  allow_array: bool = False,
>>                  allow_dict: Union[bool, str] = False) -> None:
>> +    """
>> +    Check the QAPI type of `value`. [RW]
>> +
>> +    Python types of `str` or `None` are always allowed.
>> +
>> +    :param value:       The value to check.
>> +    :param info:        QAPI Source file information.
>> +    :param source:      Human readable string describing "what" the value is.
>> +    :param allow_array: Allow a `List[str]` of length 1,
>> +                        which indicates an Array<T> type.
>> +    :param allow_dict:  Allow a dict, treated as an anonymous type.
> 
> I was hoping the docstring would explain what happens when
> allow_dict is a string.
> 

Reasonable request!

Re: [PATCH 11/16] qapi/expr.py: enable pylint checks

[Cleber Rosa]

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

On Tue, Sep 22, 2020 at 05:13:08PM -0400, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Cleber Rosa <crosa@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 15/16] qapi/expr.py: move related checks inside check_xxx functions

[John Snow]

On 9/23/20 4:25 PM, Eduardo Habkost wrote:
> On Tue, Sep 22, 2020 at 05:13:12PM -0400, John Snow wrote:
>> There's not a big obvious difference between the types of checks that
>> happen in the main function versus the kind that happen in the
>> functions. Now they're in one place for each of the main types.
>>
>> As part of the move, spell out the required and optional keywords so
>> they're obvious at a glance.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qapi/expr.py | 55 ++++++++++++++++++++++++++------------------
>>   1 file changed, 33 insertions(+), 22 deletions(-)
>>
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index 69ee9e3f18..74b2681ef8 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -333,6 +333,10 @@ def check_enum(expr: Expression, info: QAPISourceInfo) -> None:
>>       :param expr: `Expression` to validate.
>>       :param info: QAPI source file information.
>>       """
>> +    check_keys(expr, info, 'enum',
>> +               required=('enum', 'data'),
>> +               optional=('if', 'features', 'prefix'))
>> +
>>       name = expr['enum']
>>       members = expr['data']
>>       prefix = expr.get('prefix')
>> @@ -363,6 +367,11 @@ def check_struct(expr: Expression, info: QAPISourceInfo) -> None:
>>       :param expr: `Expression` to validate.
>>       :param info: QAPI source file information.
>>       """
>> +    check_keys(expr, info, 'struct',
>> +               required=('struct', 'data'),
>> +               optional=('base', 'if', 'features'))
>> +    normalize_members(expr['data'])
>> +
>>       name = cast(str, expr['struct'])  # Asserted in check_exprs
>>       members = expr['data']
>>   
>> @@ -377,6 +386,13 @@ def check_union(expr: Expression, info: QAPISourceInfo) -> None:
>>       :param expr: `Expression` to validate.
>>       :param info: QAPI source file information.
>>       """
>> +    check_keys(expr, info, 'union',
>> +               required=('union', 'data'),
>> +               optional=('base', 'discriminator', 'if', 'features'))
>> +
>> +    normalize_members(expr.get('base'))
>> +    normalize_members(expr['data'])
>> +
>>       name = cast(str, expr['union'])  # Asserted in check_exprs
>>       base = expr.get('base')
>>       discriminator = expr.get('discriminator')
>> @@ -409,6 +425,11 @@ def check_alternate(expr: Expression, info: QAPISourceInfo) -> None:
>>       :param expr: Expression to validate.
>>       :param info: QAPI source file information.
>>       """
>> +    check_keys(expr, info, 'alternate',
>> +               required=('alternate', 'data'),
>> +               optional=('if', 'features'))
>> +    normalize_members(expr['data'])
>> +
>>       members = expr['data']
>>   
>>       if not members:
>> @@ -432,6 +453,13 @@ def check_command(expr: Expression, info: QAPISourceInfo) -> None:
>>       :param expr: `Expression` to validate.
>>       :param info: QAPI source file information.
>>       """
>> +    check_keys(expr, info, 'command',
>> +               required=['command'],
>> +               optional=('data', 'returns', 'boxed', 'if', 'features',
>> +                         'gen', 'success-response', 'allow-oob',
>> +                         'allow-preconfig'))
>> +    normalize_members(expr.get('data'))
>> +
>>       args = expr.get('data')
>>       rets = expr.get('returns')
>>       boxed = expr.get('boxed', False)
>> @@ -453,6 +481,11 @@ def check_event(expr: Expression, info: QAPISourceInfo) -> None:
>>           if:       `Optional[Ifcond]`
>>           features: `Optional[Features]`
>>       """
>> +    normalize_members(expr.get('data'))
>> +    check_keys(expr, info, 'event',
>> +               required=['event'],
>> +               optional=('data', 'boxed', 'if', 'features'))
> 
> Why is the order reversed here?  The other functions call
> check_keys() before normalize_members().
> 

Oops! This was from an earlier experiment. I am dabbling with factoring 
out all of the normalization into a step that occurs discretely before 
data shape validation.

I have deep, ulterior reasons for doing this.

...But they shouldn't have leaked into this patch series, so I'll fix 
that. Thanks!

--js

Re: [PATCH 16/16] qapi/expr.py: Use an expression checker dispatch table

[John Snow]

On 9/23/20 4:36 PM, Eduardo Habkost wrote:
> I see lots of meta.value expressions below.  Maybe setting
>    meta = kind.value
> will make the code more readable?
> 

I can do that.

> I don't think this should block an obvious improvement to the
> code, so:
> 
> Reviewed-by: Eduardo Habkost<ehabkost@redhat.com>

Thanks!

--js

Re: [PATCH 12/16] qapi/expr.py: Add docstrings

[Cleber Rosa]

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

On Tue, Sep 22, 2020 at 05:13:09PM -0400, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/expr.py | 157 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 155 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index f244e9648c..4bba09f6e5 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -1,7 +1,5 @@
>  # -*- coding: utf-8 -*-
>  #
> -# Check (context-free) QAPI schema expression structure
> -#
>  # Copyright IBM, Corp. 2011
>  # Copyright (c) 2013-2019 Red Hat Inc.
>  #
> @@ -14,6 +12,25 @@
>  # This work is licensed under the terms of the GNU GPL, version 2.
>  # See the COPYING file in the top-level directory.
>  
> +"""
> +Normalize and validate (context-free) QAPI schema expression structures.
> +
> +After QAPI expressions are parsed from disk, they are stored in
> +recursively nested Python data structures using Dict, List, str, bool,
> +and int. This module ensures that those nested structures have the
> +correct type(s) and key(s) where appropriate for the QAPI context-free
> +grammar.
> +
> +The QAPI schema expression language also allows for syntactic sugar;
> +this module also handles the normalization process of these nested
> +structures.
> +
> +See `check_exprs` for the main entry point.
> +
> +See `schema.QAPISchema` for processing into native Python data
> +structures and contextual semantic validation.
> +"""
> +
>  import re
>  from typing import (
>      Iterable,
> @@ -46,6 +63,7 @@
>  def check_name_is_str(name: object,
>                        info: QAPISourceInfo,
>                        source: str) -> None:
> +    """Ensures that `name` is a string. [Const]"""
>      if not isinstance(name, str):
>          raise QAPISemError(info, "%s requires a string name" % source)
>  
> @@ -56,6 +74,24 @@ def check_name_str(name: str,
>                     allow_optional: bool = False,
>                     enum_member: bool = False,
>                     permit_upper: bool = False) -> None:
> +    """
> +    Ensures a string is a legal name. [Const]
> +
> +    A name is legal in the default case when:
> +    - It matches ``(__[a-z0-9.-]+_)?[a-z][a-z0-9_-]*``
> +    - It does not start with ``q_`` or ``q-``
> +
> +    :param name:           Name to check.
> +    :param info:           QAPI source file information.
> +    :param source:         Human-readable str describing "what" this name is.
> +    :param allow_optional: Allow the very first character to be ``*``.
> +                           (Cannot be used with `enum_member`)
> +    :param enum_member:    Allow the very first character to be a digit.
> +                           (Cannot be used with `allow_optional`)
> +    :param permit_upper:   Allows upper-case characters wherever
> +                           lower-case characters are allowed.
> +    """
> +    assert not (allow_optional and enum_member)
>      membername = name
>  
>      if allow_optional and name.startswith('*'):
> @@ -76,6 +112,17 @@ def check_name_str(name: str,
>  
>  
>  def check_defn_name_str(name: str, info: QAPISourceInfo, meta: str) -> None:
> +    """
> +    Ensures a name is a legal definition name. [Const]
> +
> +    A legal definition name:
> +     - Adheres to the criteria in `check_name_str`, with uppercase permitted
> +     - Does not end with ``Kind`` or ``List``.
> +
> +    :param name: Name to check.
> +    :param info: QAPI source file information.
> +    :param meta: Type name of the QAPI expression.
> +    """
>      check_name_str(name, info, meta, permit_upper=True)
>      if name.endswith('Kind') or name.endswith('List'):
>          raise QAPISemError(
> @@ -87,6 +134,15 @@ def check_keys(value: _JSObject,
>                 source: str,
>                 required: List[str],
>                 optional: List[str]) -> None:
> +    """
> +    Ensures an object has a specific set of keys. [Const]
> +
> +    :param value:    The object to check.
> +    :param info:     QAPI source file information.
> +    :param source:   Human-readable str describing "what" this object is.
> +    :param required: Keys that *must* be present.
> +    :param optional: Keys that *may* be present.
> +    """
>  
>      def pprint(elems: Iterable[str]) -> str:
>          return ', '.join("'" + e + "'" for e in sorted(elems))
> @@ -109,6 +165,12 @@ def pprint(elems: Iterable[str]) -> str:
>  
>  
>  def check_flags(expr: Expression, info: QAPISourceInfo) -> None:
> +    """
> +    Ensures common fields in an Expression are correct. [Const]
> +
> +    :param expr: Expression to validate.
> +    :param info: QAPI source file information.
> +    """
>      for key in ['gen', 'success-response']:
>          if key in expr and expr[key] is not False:
>              raise QAPISemError(
> @@ -120,6 +182,18 @@ def check_flags(expr: Expression, info: QAPISourceInfo) -> None:
>  
>  
>  def check_if(expr: _JSObject, info: QAPISourceInfo, source: str) -> None:
> +    """
> +    Syntactically validate and normalize the ``if`` field of an object. [RW]
> +
> +    The ``if`` field may be either a `str` or a `List[str]`.
> +    A `str` element will be normalized to `List[str]`.
> +
> +    Sugared: `Union[str, List[str]]`
> +    Ifcond: `List[str]`
> +
> +    :param expr: A `dict`; the ``if`` field, if present, will be validated.
> +    :param info: QAPI source file information.
> +    """
>  
>      def check_if_str(ifcond: object) -> None:
>          if not isinstance(ifcond, str):
> @@ -148,6 +222,16 @@ def check_if_str(ifcond: object) -> None:
>  
>  
>  def normalize_members(members: object) -> None:
> +    """
> +    Normalize a "members" value. [RW]
> +
> +    If `members` is an object, for every value in that object, if that
> +    value is not itself already an object, normalize it to
> +    ``{'type': value}``.
> +
> +    Sugared: `Dict[str, Union[str, TypeRef]]`
> +    Members: `Dict[str, TypeRef]`
> +    """
>      if isinstance(members, dict):
>          for key, arg in members.items():
>              if isinstance(arg, dict):
> @@ -160,6 +244,18 @@ def check_type(value: Optional[object],
>                 source: str,
>                 allow_array: bool = False,
>                 allow_dict: Union[bool, str] = False) -> None:
> +    """
> +    Check the QAPI type of `value`. [RW]
> +
> +    Python types of `str` or `None` are always allowed.
> +
> +    :param value:       The value to check.
> +    :param info:        QAPI Source file information.
> +    :param source:      Human readable string describing "what" the value is.
> +    :param allow_array: Allow a `List[str]` of length 1,
> +                        which indicates an Array<T> type.
> +    :param allow_dict:  Allow a dict, treated as an anonymous type.
> +    """
>      if value is None:
>          return
>  
> @@ -205,6 +301,15 @@ def check_type(value: Optional[object],
>  
>  def check_features(features: Optional[object],
>                     info: QAPISourceInfo) -> None:
> +    """
> +    Syntactically validate and normalize the "features" field. [RW]
> +
> +    `features` may be a List of either `str` or `dict`.
> +    Any `str` element will be normalized to `{'name': element}`.
> +
> +    Sugared: `List[Union[str, Feature]]`
> +    Features: `List[Feature]`
> +    """
>      if features is None:
>          return
>      if not isinstance(features, list):
> @@ -222,6 +327,12 @@ def check_features(features: Optional[object],
>  
>  
>  def check_enum(expr: Expression, info: QAPISourceInfo) -> None:
> +    """
> +    Validate this `Expression` as an ``enum`` expression. [RW]
> +
> +    :param expr: `Expression` to validate.
> +    :param info: QAPI source file information.
> +    """
>      name = expr['enum']
>      members = expr['data']
>      prefix = expr.get('prefix')
> @@ -246,6 +357,12 @@ def check_enum(expr: Expression, info: QAPISourceInfo) -> None:
>  
>  
>  def check_struct(expr: Expression, info: QAPISourceInfo) -> None:
> +    """
> +    Validate this `Expression` as a ``struct`` expression. [RW]
> +
> +    :param expr: `Expression` to validate.
> +    :param info: QAPI source file information.
> +    """
>      name = cast(str, expr['struct'])  # Asserted in check_exprs
>      members = expr['data']
>  
> @@ -254,6 +371,12 @@ def check_struct(expr: Expression, info: QAPISourceInfo) -> None:
>  
>  
>  def check_union(expr: Expression, info: QAPISourceInfo) -> None:
> +    """
> +    Validate this `Expression` as a ``union`` expression. [RW]
> +
> +    :param expr: `Expression` to validate.
> +    :param info: QAPI source file information.
> +    """
>      name = cast(str, expr['union'])  # Asserted in check_exprs
>      base = expr.get('base')
>      discriminator = expr.get('discriminator')
> @@ -280,6 +403,12 @@ def check_union(expr: Expression, info: QAPISourceInfo) -> None:
>  
>  
>  def check_alternate(expr: Expression, info: QAPISourceInfo) -> None:
> +    """
> +    Validate this `Expression` as an ``alternate`` expression. [RW]
> +
> +    :param expr: Expression to validate.
> +    :param info: QAPI source file information.
> +    """
>      members = expr['data']
>  
>      if not members:
> @@ -297,6 +426,12 @@ def check_alternate(expr: Expression, info: QAPISourceInfo) -> None:
>  
>  
>  def check_command(expr: Expression, info: QAPISourceInfo) -> None:
> +    """
> +    Validate this `Expression` as a ``command`` expression. [RW]
> +
> +    :param expr: `Expression` to validate.
> +    :param info: QAPI source file information.
> +    """
>      args = expr.get('data')
>      rets = expr.get('returns')
>      boxed = expr.get('boxed', False)
> @@ -308,6 +443,16 @@ def check_command(expr: Expression, info: QAPISourceInfo) -> None:
>  
>  
>  def check_event(expr: Expression, info: QAPISourceInfo) -> None:
> +    """
> +    Normalize and syntactically validate the ``event`` expression. [RW]
> +
> +    Event:
> +        event:    `str`
> +        data:     `Optional[Dict[str, Member]]`
> +        boxed:    `Optional[bool]`
> +        if:       `Optional[Ifcond]`
> +        features: `Optional[Features]`
> +    """
>      args = expr.get('data')
>      boxed = expr.get('boxed', False)
>  
> @@ -317,6 +462,14 @@ def check_event(expr: Expression, info: QAPISourceInfo) -> None:
>  
>  
>  def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
> +    """
> +    Validate and normalize a list of parsed QAPI schema expressions. [RW]
> +
> +    This function accepts a list of expressions + metadta as returned by
> +    the parser. It destructively normalizes the expressions in-place.
> +
> +    :param exprs: The list of expressions to normalize/validate.
> +    """

This is a huge improvement already, but maybe also take the
opportunity to add ":return:" too?  Anyway,

Reviewed-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 13/16] qapi/expr.py: Modify check_keys to accept any Iterable

[Cleber Rosa]

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

On Tue, Sep 22, 2020 at 05:13:10PM -0400, John Snow wrote:
> This is a very minor adjustment.
> 
> a + b is list-specific behavior, but we can accept a wider variety of
> types in a more pythonic fashion if we avoid that behavior.
> 
> Typing it this way allows callers to use things like dict.keys() and
> other iterables that are not their own discrete lists.
> 
> Including it just as a statement of practice if nothing else: It's nice
> to use the least-specific type possible as function input and use the
> most-specific type for returns.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 14/16] qapi/expr.py: Use tuples instead of lists for static data

[Cleber Rosa]

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

On Tue, Sep 22, 2020 at 05:13:11PM -0400, John Snow wrote:
> It is -- maybe -- possibly -- three nanoseconds faster.
>

Kevin O'Leary would clearly say here: "stop the madness!" :)

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

Reviewed-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 04/16] qapi/expr.py: Add assertion for union type 'check_dict'

[Eduardo Habkost]

On Thu, Sep 24, 2020 at 08:47:31PM -0400, John Snow wrote:
> On 9/23/20 3:53 PM, Eduardo Habkost wrote:
> > On Tue, Sep 22, 2020 at 05:13:01PM -0400, John Snow wrote:
> > > mypy isn't fond of allowing you to check for bool membership in a
> > > collection of str elements. Guard this lookup for precisely when we were
> > > given a name.
> > > 
> > > Signed-off-by: John Snow <jsnow@redhat.com>
> > > ---
> > >   scripts/qapi/expr.py | 4 +++-
> > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> > > index f6b55a87c1..67892502e9 100644
> > > --- a/scripts/qapi/expr.py
> > > +++ b/scripts/qapi/expr.py
> > > @@ -166,7 +166,9 @@ def check_type(value, info, source,
> > >           raise QAPISemError(info,
> > >                              "%s should be an object or type name" % source)
> > > -    permit_upper = allow_dict in info.pragma.name_case_whitelist
> > > +    permit_upper = False
> > > +    if isinstance(allow_dict, str):
> > > +        permit_upper = allow_dict in info.pragma.name_case_whitelist
> > 
> > Well, this keeps existing behavior, so:
> > 
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > 
> > But: what exactly is the meaning of allow_dict=False,
> > allow_dict=True, and allow_dict being a string?
> > 
> > 
> 
> allow_dict = True -- allows the type to be an object describing the type.
> 
> allow_dict: str -- allows the type to be an object (like True), but also
> passes a name in for the purposes of validating the name with the pragma
> whitelist(!)

What.

-- 
Eduardo

Re: [PATCH 15/16] qapi/expr.py: move related checks inside check_xxx functions

[Cleber Rosa]

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

On Tue, Sep 22, 2020 at 05:13:12PM -0400, John Snow wrote:
> There's not a big obvious difference between the types of checks that
> happen in the main function versus the kind that happen in the
> functions. Now they're in one place for each of the main types.
> 
> As part of the move, spell out the required and optional keywords so
> they're obvious at a glance.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 12/16] qapi/expr.py: Add docstrings

[John Snow]

On 9/24/20 8:59 PM, Cleber Rosa wrote:
> On Tue, Sep 22, 2020 at 05:13:09PM -0400, John Snow wrote:
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qapi/expr.py | 157 ++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 155 insertions(+), 2 deletions(-)
>>
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index f244e9648c..4bba09f6e5 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -1,7 +1,5 @@
>>   # -*- coding: utf-8 -*-
>>   #
>> -# Check (context-free) QAPI schema expression structure
>> -#
>>   # Copyright IBM, Corp. 2011
>>   # Copyright (c) 2013-2019 Red Hat Inc.
>>   #
>> @@ -14,6 +12,25 @@
>>   # This work is licensed under the terms of the GNU GPL, version 2.
>>   # See the COPYING file in the top-level directory.
>>   
>> +"""
>> +Normalize and validate (context-free) QAPI schema expression structures.
>> +
>> +After QAPI expressions are parsed from disk, they are stored in
>> +recursively nested Python data structures using Dict, List, str, bool,
>> +and int. This module ensures that those nested structures have the
>> +correct type(s) and key(s) where appropriate for the QAPI context-free
>> +grammar.
>> +
>> +The QAPI schema expression language also allows for syntactic sugar;
>> +this module also handles the normalization process of these nested
>> +structures.
>> +
>> +See `check_exprs` for the main entry point.
>> +
>> +See `schema.QAPISchema` for processing into native Python data
>> +structures and contextual semantic validation.
>> +"""
>> +
>>   import re
>>   from typing import (
>>       Iterable,
>> @@ -46,6 +63,7 @@
>>   def check_name_is_str(name: object,
>>                         info: QAPISourceInfo,
>>                         source: str) -> None:
>> +    """Ensures that `name` is a string. [Const]"""
>>       if not isinstance(name, str):
>>           raise QAPISemError(info, "%s requires a string name" % source)
>>   
>> @@ -56,6 +74,24 @@ def check_name_str(name: str,
>>                      allow_optional: bool = False,
>>                      enum_member: bool = False,
>>                      permit_upper: bool = False) -> None:
>> +    """
>> +    Ensures a string is a legal name. [Const]
>> +
>> +    A name is legal in the default case when:
>> +    - It matches ``(__[a-z0-9.-]+_)?[a-z][a-z0-9_-]*``
>> +    - It does not start with ``q_`` or ``q-``
>> +
>> +    :param name:           Name to check.
>> +    :param info:           QAPI source file information.
>> +    :param source:         Human-readable str describing "what" this name is.
>> +    :param allow_optional: Allow the very first character to be ``*``.
>> +                           (Cannot be used with `enum_member`)
>> +    :param enum_member:    Allow the very first character to be a digit.
>> +                           (Cannot be used with `allow_optional`)
>> +    :param permit_upper:   Allows upper-case characters wherever
>> +                           lower-case characters are allowed.
>> +    """
>> +    assert not (allow_optional and enum_member)
>>       membername = name
>>   
>>       if allow_optional and name.startswith('*'):
>> @@ -76,6 +112,17 @@ def check_name_str(name: str,
>>   
>>   
>>   def check_defn_name_str(name: str, info: QAPISourceInfo, meta: str) -> None:
>> +    """
>> +    Ensures a name is a legal definition name. [Const]
>> +
>> +    A legal definition name:
>> +     - Adheres to the criteria in `check_name_str`, with uppercase permitted
>> +     - Does not end with ``Kind`` or ``List``.
>> +
>> +    :param name: Name to check.
>> +    :param info: QAPI source file information.
>> +    :param meta: Type name of the QAPI expression.
>> +    """
>>       check_name_str(name, info, meta, permit_upper=True)
>>       if name.endswith('Kind') or name.endswith('List'):
>>           raise QAPISemError(
>> @@ -87,6 +134,15 @@ def check_keys(value: _JSObject,
>>                  source: str,
>>                  required: List[str],
>>                  optional: List[str]) -> None:
>> +    """
>> +    Ensures an object has a specific set of keys. [Const]
>> +
>> +    :param value:    The object to check.
>> +    :param info:     QAPI source file information.
>> +    :param source:   Human-readable str describing "what" this object is.
>> +    :param required: Keys that *must* be present.
>> +    :param optional: Keys that *may* be present.
>> +    """
>>   
>>       def pprint(elems: Iterable[str]) -> str:
>>           return ', '.join("'" + e + "'" for e in sorted(elems))
>> @@ -109,6 +165,12 @@ def pprint(elems: Iterable[str]) -> str:
>>   
>>   
>>   def check_flags(expr: Expression, info: QAPISourceInfo) -> None:
>> +    """
>> +    Ensures common fields in an Expression are correct. [Const]
>> +
>> +    :param expr: Expression to validate.
>> +    :param info: QAPI source file information.
>> +    """
>>       for key in ['gen', 'success-response']:
>>           if key in expr and expr[key] is not False:
>>               raise QAPISemError(
>> @@ -120,6 +182,18 @@ def check_flags(expr: Expression, info: QAPISourceInfo) -> None:
>>   
>>   
>>   def check_if(expr: _JSObject, info: QAPISourceInfo, source: str) -> None:
>> +    """
>> +    Syntactically validate and normalize the ``if`` field of an object. [RW]
>> +
>> +    The ``if`` field may be either a `str` or a `List[str]`.
>> +    A `str` element will be normalized to `List[str]`.
>> +
>> +    Sugared: `Union[str, List[str]]`
>> +    Ifcond: `List[str]`
>> +
>> +    :param expr: A `dict`; the ``if`` field, if present, will be validated.
>> +    :param info: QAPI source file information.
>> +    """
>>   
>>       def check_if_str(ifcond: object) -> None:
>>           if not isinstance(ifcond, str):
>> @@ -148,6 +222,16 @@ def check_if_str(ifcond: object) -> None:
>>   
>>   
>>   def normalize_members(members: object) -> None:
>> +    """
>> +    Normalize a "members" value. [RW]
>> +
>> +    If `members` is an object, for every value in that object, if that
>> +    value is not itself already an object, normalize it to
>> +    ``{'type': value}``.
>> +
>> +    Sugared: `Dict[str, Union[str, TypeRef]]`
>> +    Members: `Dict[str, TypeRef]`
>> +    """
>>       if isinstance(members, dict):
>>           for key, arg in members.items():
>>               if isinstance(arg, dict):
>> @@ -160,6 +244,18 @@ def check_type(value: Optional[object],
>>                  source: str,
>>                  allow_array: bool = False,
>>                  allow_dict: Union[bool, str] = False) -> None:
>> +    """
>> +    Check the QAPI type of `value`. [RW]
>> +
>> +    Python types of `str` or `None` are always allowed.
>> +
>> +    :param value:       The value to check.
>> +    :param info:        QAPI Source file information.
>> +    :param source:      Human readable string describing "what" the value is.
>> +    :param allow_array: Allow a `List[str]` of length 1,
>> +                        which indicates an Array<T> type.
>> +    :param allow_dict:  Allow a dict, treated as an anonymous type.
>> +    """
>>       if value is None:
>>           return
>>   
>> @@ -205,6 +301,15 @@ def check_type(value: Optional[object],
>>   
>>   def check_features(features: Optional[object],
>>                      info: QAPISourceInfo) -> None:
>> +    """
>> +    Syntactically validate and normalize the "features" field. [RW]
>> +
>> +    `features` may be a List of either `str` or `dict`.
>> +    Any `str` element will be normalized to `{'name': element}`.
>> +
>> +    Sugared: `List[Union[str, Feature]]`
>> +    Features: `List[Feature]`
>> +    """
>>       if features is None:
>>           return
>>       if not isinstance(features, list):
>> @@ -222,6 +327,12 @@ def check_features(features: Optional[object],
>>   
>>   
>>   def check_enum(expr: Expression, info: QAPISourceInfo) -> None:
>> +    """
>> +    Validate this `Expression` as an ``enum`` expression. [RW]
>> +
>> +    :param expr: `Expression` to validate.
>> +    :param info: QAPI source file information.
>> +    """
>>       name = expr['enum']
>>       members = expr['data']
>>       prefix = expr.get('prefix')
>> @@ -246,6 +357,12 @@ def check_enum(expr: Expression, info: QAPISourceInfo) -> None:
>>   
>>   
>>   def check_struct(expr: Expression, info: QAPISourceInfo) -> None:
>> +    """
>> +    Validate this `Expression` as a ``struct`` expression. [RW]
>> +
>> +    :param expr: `Expression` to validate.
>> +    :param info: QAPI source file information.
>> +    """
>>       name = cast(str, expr['struct'])  # Asserted in check_exprs
>>       members = expr['data']
>>   
>> @@ -254,6 +371,12 @@ def check_struct(expr: Expression, info: QAPISourceInfo) -> None:
>>   
>>   
>>   def check_union(expr: Expression, info: QAPISourceInfo) -> None:
>> +    """
>> +    Validate this `Expression` as a ``union`` expression. [RW]
>> +
>> +    :param expr: `Expression` to validate.
>> +    :param info: QAPI source file information.
>> +    """
>>       name = cast(str, expr['union'])  # Asserted in check_exprs
>>       base = expr.get('base')
>>       discriminator = expr.get('discriminator')
>> @@ -280,6 +403,12 @@ def check_union(expr: Expression, info: QAPISourceInfo) -> None:
>>   
>>   
>>   def check_alternate(expr: Expression, info: QAPISourceInfo) -> None:
>> +    """
>> +    Validate this `Expression` as an ``alternate`` expression. [RW]
>> +
>> +    :param expr: Expression to validate.
>> +    :param info: QAPI source file information.
>> +    """
>>       members = expr['data']
>>   
>>       if not members:
>> @@ -297,6 +426,12 @@ def check_alternate(expr: Expression, info: QAPISourceInfo) -> None:
>>   
>>   
>>   def check_command(expr: Expression, info: QAPISourceInfo) -> None:
>> +    """
>> +    Validate this `Expression` as a ``command`` expression. [RW]
>> +
>> +    :param expr: `Expression` to validate.
>> +    :param info: QAPI source file information.
>> +    """
>>       args = expr.get('data')
>>       rets = expr.get('returns')
>>       boxed = expr.get('boxed', False)
>> @@ -308,6 +443,16 @@ def check_command(expr: Expression, info: QAPISourceInfo) -> None:
>>   
>>   
>>   def check_event(expr: Expression, info: QAPISourceInfo) -> None:
>> +    """
>> +    Normalize and syntactically validate the ``event`` expression. [RW]
>> +
>> +    Event:
>> +        event:    `str`
>> +        data:     `Optional[Dict[str, Member]]`
>> +        boxed:    `Optional[bool]`
>> +        if:       `Optional[Ifcond]`
>> +        features: `Optional[Features]`
>> +    """
>>       args = expr.get('data')
>>       boxed = expr.get('boxed', False)
>>   
>> @@ -317,6 +462,14 @@ def check_event(expr: Expression, info: QAPISourceInfo) -> None:
>>   
>>   
>>   def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
>> +    """
>> +    Validate and normalize a list of parsed QAPI schema expressions. [RW]
>> +
>> +    This function accepts a list of expressions + metadta as returned by
>> +    the parser. It destructively normalizes the expressions in-place.
>> +
>> +    :param exprs: The list of expressions to normalize/validate.
>> +    """
> 
> This is a huge improvement already, but maybe also take the
> opportunity to add ":return:" too?  Anyway,
> 
> Reviewed-by: Cleber Rosa <crosa@redhat.com>
> 

Yeah, I can do that. The only reason I added docstrings here -- and 
basically nowhere else -- was I found this code hardest to understand, 
because of the highly dynamic type structures it's dealing with.

So I wrote a bunch of notes for myself, and ... may as well share those.

Thanks!

Re: [PATCH 14/16] qapi/expr.py: Use tuples instead of lists for static data

[John Snow]

On 9/24/20 9:03 PM, Cleber Rosa wrote:
> On Tue, Sep 22, 2020 at 05:13:11PM -0400, John Snow wrote:
>> It is -- maybe -- possibly -- three nanoseconds faster.
>>
> 
> Kevin O'Leary would clearly say here: "stop the madness!" :)
> 
>> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> Reviewed-by: Cleber Rosa <crosa@redhat.com>
> 

I can explain!

When I sat down to write this series, I just "cleaned" in general 
without regard to what was strictly necessary and what wasn't.

Once I had to split them out, brick-by-brick, some things looked much 
more frivolous than others, but I had already written and tested them, so...

Well. There are definitely a few unimportant patches here.

--js

Re: [PATCH 16/16] qapi/expr.py: Use an expression checker dispatch table

[Cleber Rosa]

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

On Tue, Sep 22, 2020 at 05:13:13PM -0400, John Snow wrote:
> This enforces a type signature against all of the top-level expression
> check routines without necessarily needing to create some
> overcomplicated class hierarchy for them.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/expr.py | 69 ++++++++++++++++++++++----------------------
>  1 file changed, 35 insertions(+), 34 deletions(-)
> 
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 74b2681ef8..cfd342aa04 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -31,8 +31,11 @@
>  structures and contextual semantic validation.
>  """
>  
> +from enum import Enum
>  import re
>  from typing import (
> +    Callable,
> +    Dict,
>      Iterable,
>      List,
>      MutableMapping,
> @@ -494,6 +497,26 @@ def check_event(expr: Expression, info: QAPISourceInfo) -> None:
>      check_type(args, info, "'data'", allow_dict=not boxed)
>  
>  
> +class ExpressionType(str, Enum):
> +    INCLUDE = 'include'
> +    ENUM = 'enum'
> +    UNION = 'union'
> +    ALTERNATE = 'alternate'
> +    STRUCT = 'struct'
> +    COMMAND = 'command'
> +    EVENT = 'event'
> +
> +
> +_CHECK_FN: Dict[str, Callable[[Expression, QAPISourceInfo], None]] = {
> +    'enum': check_enum,
> +    'union': check_union,
> +    'alternate': check_alternate,
> +    'struct': check_struct,
> +    'command': check_command,
> +    'event': check_event,
> +}
> +
> +
>  def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
>      """
>      Validate and normalize a list of parsed QAPI schema expressions. [RW]
> @@ -519,28 +542,20 @@ def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
>          assert tmp is None or isinstance(tmp, QAPIDoc)
>          doc: Optional[QAPIDoc] = tmp
>  
> -        if 'include' in expr:
> -            continue
> -
> -        if 'enum' in expr:
> -            meta = 'enum'
> -        elif 'union' in expr:
> -            meta = 'union'
> -        elif 'alternate' in expr:
> -            meta = 'alternate'
> -        elif 'struct' in expr:
> -            meta = 'struct'
> -        elif 'command' in expr:
> -            meta = 'command'
> -        elif 'event' in expr:
> -            meta = 'event'
> +        for kind in ExpressionType:
> +            if kind in expr:
> +                meta = kind
> +                break
>          else:
>              raise QAPISemError(info, "expression is missing metatype")
>  
> +        if meta == ExpressionType.INCLUDE:
> +            continue
> +
>          name = cast(str, expr[meta])  # asserted right below:
> -        check_name_is_str(name, info, "'%s'" % meta)
> -        info.set_defn(meta, name)
> -        check_defn_name_str(name, info, meta)
> +        check_name_is_str(name, info, "'%s'" % meta.value)
> +        info.set_defn(meta.value, name)
> +        check_defn_name_str(name, info, meta.value)
>  
>          if doc:
>              if doc.symbol != name:
> @@ -551,22 +566,8 @@ def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
>              raise QAPISemError(info,
>                                 "documentation comment required")
>  
> -        if meta == 'enum':
> -            check_enum(expr, info)
> -        elif meta == 'union':
> -            check_union(expr, info)
> -        elif meta == 'alternate':
> -            check_alternate(expr, info)
> -        elif meta == 'struct':
> -            check_struct(expr, info)
> -        elif meta == 'command':
> -            check_command(expr, info)
> -        elif meta == 'event':
> -            check_event(expr, info)
> -        else:
> -            assert False, 'unexpected meta type'
> -
> -        check_if(expr, info, meta)
> +        _CHECK_FN[meta](expr, info)

I have to say the style of this line bothers me, but it's just that,
style. So,

Reviewed-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 04/16] qapi/expr.py: Add assertion for union type 'check_dict'

[John Snow]

On 9/24/20 9:08 PM, Eduardo Habkost wrote:
> On Thu, Sep 24, 2020 at 08:47:31PM -0400, John Snow wrote:
>> On 9/23/20 3:53 PM, Eduardo Habkost wrote:
>>> On Tue, Sep 22, 2020 at 05:13:01PM -0400, John Snow wrote:
>>>> mypy isn't fond of allowing you to check for bool membership in a
>>>> collection of str elements. Guard this lookup for precisely when we were
>>>> given a name.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>    scripts/qapi/expr.py | 4 +++-
>>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>>>> index f6b55a87c1..67892502e9 100644
>>>> --- a/scripts/qapi/expr.py
>>>> +++ b/scripts/qapi/expr.py
>>>> @@ -166,7 +166,9 @@ def check_type(value, info, source,
>>>>            raise QAPISemError(info,
>>>>                               "%s should be an object or type name" % source)
>>>> -    permit_upper = allow_dict in info.pragma.name_case_whitelist
>>>> +    permit_upper = False
>>>> +    if isinstance(allow_dict, str):
>>>> +        permit_upper = allow_dict in info.pragma.name_case_whitelist
>>>
>>> Well, this keeps existing behavior, so:
>>>
>>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>>>
>>> But: what exactly is the meaning of allow_dict=False,
>>> allow_dict=True, and allow_dict being a string?
>>>
>>>
>>
>> allow_dict = True -- allows the type to be an object describing the type.
>>
>> allow_dict: str -- allows the type to be an object (like True), but also
>> passes a name in for the purposes of validating the name with the pragma
>> whitelist(!)
> 
> What.

(lol)

> 

What's going on here is that when you pass in a name, bool(allow_dict) 
is True -- so we will allow the object being checked here to be a dict.

Also, when you pass in a name, that name is looked up in 
info.pragma.name_case_whitelist to check if the names of the keys in the 
dict being checked (again, allow_dict is implicitly true here) are 
allowed to use uppercase names.

I have some more experimental patches I didn't want to mix in with the 
type checking patches that try to extract pragma checks from this code 
and perform them elsewhere; but that's going to come much, much later.

--js

Re: [PATCH 16/16] qapi/expr.py: Use an expression checker dispatch table

[John Snow]

On 9/24/20 9:18 PM, Cleber Rosa wrote:
> I have to say the style of this line bothers me, but it's just that,
> style. So,

What don't you like?

Re: [PATCH 16/16] qapi/expr.py: Use an expression checker dispatch table

[Helio Loureiro]

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

Hi,

I would replace the word variable "kind" by "category".

./helio

On Fri, Sep 25, 2020, 03:32 John Snow <jsnow@redhat.com> wrote:

> On 9/24/20 9:18 PM, Cleber Rosa wrote:
> > I have to say the style of this line bothers me, but it's just that,
> > style. So,
>
> What don't you like?
>
>
>

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

Re: [PATCH 16/16] qapi/expr.py: Use an expression checker dispatch table

[John Snow]

On 9/25/20 2:03 AM, Helio Loureiro wrote:
> Hi,
> 
> I would replace the word variable "kind" by "category".
> 

Hi, welcome to the list!

For patch reviews, we try to reply in-line, below the original post.

I'm not attached to 'kind', but 'category' is perhaps too broad. 
Category in this particular domain might refer to the difference between 
a "Directive" (include, pragma) and a Definition (enum, struct, union, 
alternate, command, event)

(For more information on the QAPI Schema Language that we are parsing 
and validating here, see docs/devel/qapi-code-gen.txt if you are 
curious. Ultimately it is a JSON-like format that permits multiple 
objects per document and allows comments. We use these structures to 
generate types and command interfaces for our API protocol, QMP.)

Ultimately I am using 'kind' for the 'type of expression', but type is 
an extremely overloaded word when parsing a language in another language!

We also use 'meta' nearby for semantically the same thing, but with 
different typing.

Thanks for looking!
--js

> ./helio
> 
> On Fri, Sep 25, 2020, 03:32 John Snow <jsnow@redhat.com 
> <mailto:jsnow@redhat.com>> wrote:
> 
>     On 9/24/20 9:18 PM, Cleber Rosa wrote:
>      > I have to say the style of this line bothers me, but it's just that,
>      > style. So,
> 
>     What don't you like?
> 
> 

Re: [PATCH 16/16] qapi/expr.py: Use an expression checker dispatch table

[Cleber Rosa]

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

On Thu, Sep 24, 2020 at 09:32:05PM -0400, John Snow wrote:
> On 9/24/20 9:18 PM, Cleber Rosa wrote:
> > I have to say the style of this line bothers me, but it's just that,
> > style. So,
> 
> What don't you like?

It's the sum of the "private" + "global dictionary" + "its item being
called directly".

But don't bother, this is probably the kind of comment that I should
omit, as I don't want you to, say, create a wrapper function around
the dict, partially defeating the purpose of this patch.

- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 06/16] qapi/expr.py: Check type of 'data' member

[Cleber Rosa]

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

On Thu, Sep 24, 2020 at 08:50:27PM -0400, John Snow wrote:
> On 9/24/20 8:31 PM, Cleber Rosa wrote:
> > On Tue, Sep 22, 2020 at 05:13:03PM -0400, John Snow wrote:
> > > Iterating over the members of data isn't going to work if it's not some
> > > form of dict anyway, but for type safety, formalize it.
> > > 
> > > Signed-off-by: John Snow <jsnow@redhat.com>
> > > ---
> > >   scripts/qapi/expr.py | 7 +++++++
> > >   1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> > > index 3f5af5f5e4..633f9b9172 100644
> > > --- a/scripts/qapi/expr.py
> > > +++ b/scripts/qapi/expr.py
> > > @@ -247,6 +247,9 @@ def check_union(expr, info):
> > >               raise QAPISemError(info, "'discriminator' requires 'base'")
> > >           check_name_is_str(discriminator, info, "'discriminator'")
> > > +    if not isinstance(members, dict):
> > > +        raise QAPISemError(info, "'data' must be an object")
> > > +
> > 
> > Don't you mean "must be a dict" ?
> > 
> 
> This error is speaking JSON-ese! json objects become python dicts, so if we
> didn't get a python dict here, we didn't get a json object.

Right!  Thanks for the explanation.

- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 16/16] qapi/expr.py: Use an expression checker dispatch table

[John Snow]

On 9/25/20 12:38 PM, Cleber Rosa wrote:
> On Thu, Sep 24, 2020 at 09:32:05PM -0400, John Snow wrote:
>> On 9/24/20 9:18 PM, Cleber Rosa wrote:
>>> I have to say the style of this line bothers me, but it's just that,
>>> style. So,
>>
>> What don't you like?
> 
> It's the sum of the "private" + "global dictionary" + "its item being
> called directly".
> 
> But don't bother, this is probably the kind of comment that I should
> omit, as I don't want you to, say, create a wrapper function around
> the dict, partially defeating the purpose of this patch.
>

ACK, just wanted to know what the style nit was. Thanks :)

--js

Re: [PATCH 00/16] qapi: static typing conversion, pt2

[John Snow]

On 9/22/20 5:12 PM, John Snow wrote:
> based-on: <20200922210101.4081073-1-jsnow@redhat.com>
>            [PATCH v2 00/38] qapi: static typing conversion, pt1
> 
> Hi, this series adds static type hints to the QAPI module.
> This is part two!
> 
> Part 2: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt2
> Everything: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt6
> 

Thanks for reviews. I will not be re-spinning pt2 until pt1 is fully 
merged, but I have re-based on pt1-v3 and made some minor adjustments to 
accommodate new development in pt1.

You can find that staged here:
https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt2


Here's the review status of pt2:

[01] qapi-expr-py-remove-info       # [SOB] JS [RB] CR,EH
[02] qapi-expr-py-check-for-dict    # [SOB] JS [RB] CR,EH
[03] qapi-expr-py-constrain         # [SOB] JS [RB] CR,EH
[04] qapi-expr-py-add-assertion-for # [SOB] JS [RB] CR,EH
[05] qapi-expr-py-move-string-check # [SOB] JS [RB] CR,EH
[06] qapi-expr-py-check-type-of     # [SOB] JS [RB] EH
[07] qapi-expr-py-add-casts-in-a    # [SOB] JS [RB] CR,EH
[08] qapi-expr-py-add-notational    # [SOB] JS [RB] CR,EH
[09] qapi-expr-py-rewrite-check_if  # [SOB] JS [RB] CR,EH [TB] CR
[10] qapi-expr-py-remove-single     # [SOB] JS [RB] CR,EH
[11] pylint-enable                  # [SOB] JS [TB] CR,EH [RB] CR,EH
[12] qapi-expr-py-add-docstrings    # [SOB] JS [RB] CR
[13] qapi-expr-modify-check_keys-to # [SOB] JS [RB] CR,EH
[14] qapi-expr-use-tuples-instead   # [SOB] JS [RB] CR,EH
[15] qapi-expr-move-related-checks  # [SOB] JS [RB] CR
[16] qapi-expr-use-an-expression    # [SOB] JS [RB] CR,EH

As for the difflog so far:

Patches 2, 3, 7, 8 change import orderings (isort)
Patch 12 sees some docstrings rewritten to pass sphinx.
Patch 15 addresses Eduardo's review comments.

--js

Re: [PATCH 16/16] qapi/expr.py: Use an expression checker dispatch table

[Helio Loureiro]

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

On Fri, Sep 25, 2020, 16:16 John Snow <jsnow@redhat.com> wrote:

> On 9/25/20 2:03 AM, Helio Loureiro wrote:
> > Hi,
> >
> > I would replace the word variable "kind" by "category".
> >
>
> Hi, welcome to the list!
>

Tkz!


> For patch reviews, we try to reply in-line, below the original post.
>

I realized that later.  It has been more than 20 years I don't use this
formating.  But if I intend to join the pack, I need to follow the pack.

>
>
(For more information on the QAPI Schema Language that we are parsing
> and validating here, see docs/devel/qapi-code-gen.txt if you are
> curious. Ultimately it is a JSON-like format that permits multiple
> objects per document and allows comments. We use these structures to
> generate types and command interfaces for our API protocol, QMP.)
>

Based on that I would suggest 'type_ref' instead to match the definitions
over there and since word 'type' itself is reserved.


>
>
./helio

>

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

Re: [PATCH 16/16] qapi/expr.py: Use an expression checker dispatch table

[John Snow]

On 9/26/20 7:31 AM, Helio Loureiro wrote:
> 
> 
> On Fri, Sep 25, 2020, 16:16 John Snow <jsnow@redhat.com 
> <mailto:jsnow@redhat.com>> wrote:
> 
>     On 9/25/20 2:03 AM, Helio Loureiro wrote:
>      > Hi,
>      >
>      > I would replace the word variable "kind" by "category".
>      >
> 
>     Hi, welcome to the list!
> 
> 
> Tkz!
> 
> 
>     For patch reviews, we try to reply in-line, below the original post.
> 
> 
> I realized that later.  It has been more than 20 years I don't use this 
> formating.  But if I intend to join the pack, I need to follow the pack.
> 
> 
> 
>     (For more information on the QAPI Schema Language that we are parsing
>     and validating here, see docs/devel/qapi-code-gen.txt if you are
>     curious. Ultimately it is a JSON-like format that permits multiple
>     objects per document and allows comments. We use these structures to
>     generate types and command interfaces for our API protocol, QMP.)
> 
> 
> Based on that I would suggest 'type_ref' instead to match the 
> definitions over there and since word 'type' itself is reserved.
> 

One of the unsolvable problems in computer science is naming things: 
"TYPE-REF" also has a specific meaning in QAPI, as it is the name of one 
of the BNF grammar tokens we use.

So I might suggest (if "kind" is too ambiguous), that I might use 
"statement_type" or "expression_type" if that helps clarify things.

--js