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

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

[John Snow]

based-on: <20201026194251.11075-1-jsnow@redhat.com>
          [PATCH v2 00/11] qapi: static typing conversion, pt2

Hi, this series adds static type hints to the QAPI module.
This is part three, and it focuses on expr.py.

Part 3: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt3
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)

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/

V2:
 - Rebased on the latest V2
002/16:[0001] [FC] 'qapi/expr.py: Check for dict instead of OrderedDict'
 - Import order differences
007/16:[0006] [FC] 'qapi/expr.py: Add casts in a few select cases'
 - Import order differences
008/16:[0006] [FC] 'qapi/expr.py: add type hint annotations'
 - Import order differents
012/16:[0066] [FC] 'qapi/expr.py: Add docstrings'
 - Various docstring changes for Sphinx
014/16:[0004] [FC] 'qapi/expr.py: Use tuples instead of lists for static data'
 - Change to accommodate new 'coroutine' key
015/16:[0006] [FC] 'qapi/expr.py: move related checks inside check_xxx functions'
 - Fix check order (ehabkost)

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  | 447 +++++++++++++++++++++++++++++++-----------
 scripts/qapi/mypy.ini |   5 -
 scripts/qapi/pylintrc |   1 -
 3 files changed, 334 insertions(+), 119 deletions(-)

-- 
2.26.2

[PATCH v2 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>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Cleber Rosa <crosa@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 2fcaaa2497a8..35695c4c653b 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -104,7 +104,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,
@@ -124,9 +124,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 v2 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>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
---
 scripts/qapi/expr.py | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 35695c4c653b..5694c501fa38 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -14,7 +14,6 @@
 # This work is licensed under the terms of the GNU GPL, version 2.
 # See the COPYING file in the top-level directory.
 
-from collections import OrderedDict
 import re
 
 from .common import c_name
@@ -131,7 +130,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
@@ -162,7 +161,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 v2 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>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Cleber Rosa <crosa@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 5694c501fa38..f7c7f91326ef 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,
@@ -287,9 +295,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 v2 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>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Cleber Rosa <crosa@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 f7c7f91326ef..2c4c341d5243 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -173,7 +173,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 v2 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>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Cleber Rosa <crosa@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 2c4c341d5243..864363881682 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -150,6 +150,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:
@@ -160,10 +164,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 v2 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>
Reviewed-by: Eduardo Habkost <ehabkost@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 864363881682..2654a72e8333 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -254,6 +254,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)
@@ -267,6 +270,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 v2 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>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Cleber Rosa <crosa@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 2654a72e8333..b8720b723377 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 typing import MutableMapping, Optional
+from typing import MutableMapping, Optional, cast
 
 from .common import c_name
 from .error import QAPISemError
@@ -232,7 +232,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)
@@ -240,7 +240,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']
@@ -337,7 +337,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 v2 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>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
---
 scripts/qapi/expr.py  | 71 ++++++++++++++++++++++++++++---------------
 scripts/qapi/mypy.ini |  5 ---
 2 files changed, 46 insertions(+), 30 deletions(-)

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index b8720b723377..b393ccd30e92 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -15,7 +15,14 @@
 # See the COPYING file in the top-level directory.
 
 import re
-from typing import MutableMapping, Optional, cast
+from typing import (
+    Iterable,
+    List,
+    MutableMapping,
+    Optional,
+    Union,
+    cast,
+)
 
 from .common import c_name
 from .error import QAPISemError
@@ -23,9 +30,10 @@
 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,
@@ -35,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('*'):
@@ -62,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)
@@ -91,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(
@@ -109,9 +126,9 @@ def check_flags(expr, info):
                                  "are incompatible")
 
 
-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,
@@ -137,7 +154,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):
@@ -145,8 +162,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
 
@@ -190,7 +210,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):
@@ -207,7 +228,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')
@@ -231,7 +252,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']
 
@@ -239,7 +260,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')
@@ -265,7 +286,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:
@@ -282,7 +303,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)
@@ -293,7 +314,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)
 
@@ -302,7 +323,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 c0f2a58306de..df9b05e4ab36 100644
--- a/scripts/qapi/mypy.ini
+++ b/scripts/qapi/mypy.ini
@@ -9,11 +9,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 v2 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>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
Tested-by: Cleber Rosa <crosa@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 b393ccd30e92..4d4ee3daa002 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -143,15 +143,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 v2 10/16] qapi/expr.py: Remove single-letter variable

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Cleber Rosa <crosa@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 4d4ee3daa002..99c5c2ff99b0 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -218,14 +218,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 v2 11/16] qapi/expr.py: enable pylint checks

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
Tested-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>
---
 scripts/qapi/pylintrc | 1 -
 1 file changed, 1 deletion(-)

diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
index b9e077a1642d..fb0386d529ac 100644
--- a/scripts/qapi/pylintrc
+++ b/scripts/qapi/pylintrc
@@ -3,7 +3,6 @@
 # Add files or directories matching the regex patterns to the ignore list.
 # The regex matches against base names, not paths.
 ignore-patterns=error.py,
-                expr.py,
                 parser.py,
                 schema.py,
 
-- 
2.26.2

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

[John Snow]

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

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 99c5c2ff99b0..8ad82ca98864 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 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,
@@ -32,7 +49,7 @@
 
 # Arbitrary form for a JSON-like object.
 _JSObject = MutableMapping[str, object]
-# Expressions in their raw form are (just) JSON-like objects.
+#: Expressions in their unvalidated form are JSON-like objects.
 Expression = _JSObject
 
 
@@ -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,25 @@ 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 +113,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 +135,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 +166,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(
@@ -127,6 +190,19 @@ 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]``.
+
+    :param expr: A ``dict``; the ``if`` field, if present, will be validated.
+    :param info: QAPI source file information.
+
+    :forms:
+      :sugared: ``Union[str, List[str]]``
+      :canonical: ``List[str]``
+    """
 
     def check_if_str(ifcond: object) -> None:
         if not isinstance(ifcond, str):
@@ -155,6 +231,17 @@ 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}``.
+
+    :forms:
+      :sugared: ``Dict[str, Union[str, TypeRef]]``
+      :canonical: ``Dict[str, TypeRef]``
+    """
     if isinstance(members, dict):
         for key, arg in members.items():
             if isinstance(arg, dict):
@@ -167,6 +254,21 @@ 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.
+                        When given a string, check if that name is allowed to
+                        have keys that use uppercase letters, and modify
+                        validation accordingly.
+    """
     if value is None:
         return
 
@@ -212,6 +314,16 @@ 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}``.
+
+    :forms:
+      :sugared: ``List[Union[str, Feature]]``
+      :canonical: ``List[Feature]``
+    """
     if features is None:
         return
     if not isinstance(features, list):
@@ -229,6 +341,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')
@@ -253,6 +371,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']
 
@@ -261,6 +385,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')
@@ -287,6 +417,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:
@@ -304,6 +440,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)
@@ -315,6 +457,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]`` (see: `check_if`)
+      :features: ``Optional[Features]`` (see: `check_features`)
+    """
     args = expr.get('data')
     boxed = expr.get('boxed', False)
 
@@ -324,6 +476,15 @@ 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.
+    :return: The same list of expressions (now modified).
+    """
     for expr_elem in exprs:
         # Expression
         assert isinstance(expr_elem['expr'], dict)
-- 
2.26.2

[PATCH v2 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>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Cleber Rosa <crosa@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 8ad82ca98864..883aa1781599 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -133,8 +133,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]
 
@@ -155,7 +155,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 v2 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>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Cleber Rosa <crosa@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 883aa1781599..9253560cd9ce 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -172,11 +172,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', 'coroutine']:
+    for key in ('boxed', 'allow-oob', 'allow-preconfig', 'coroutine'):
         if key in expr and expr[key] is not True:
             raise QAPISemError(
                 info, "flag '%s' may only use true value" % key)
-- 
2.26.2

[PATCH v2 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>
Reviewed-by: Cleber Rosa <crosa@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 9253560cd9ce..3b62e801a42c 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -347,6 +347,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')
@@ -377,6 +381,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']
 
@@ -391,6 +400,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')
@@ -423,6 +439,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:
@@ -446,6 +467,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', 'coroutine'))
+    normalize_members(expr.get('data'))
+
     args = expr.get('data')
     rets = expr.get('returns')
     boxed = expr.get('boxed', False)
@@ -467,6 +495,11 @@ def check_event(expr: Expression, info: QAPISourceInfo) -> None:
       :if: ``Optional[Ifcond]`` (see: `check_if`)
       :features: ``Optional[Features]`` (see: `check_features`)
     """
+    check_keys(expr, info, 'event',
+               required=['event'],
+               optional=('data', 'boxed', 'if', 'features'))
+    normalize_members(expr.get('data'))
+
     args = expr.get('data')
     boxed = expr.get('boxed', False)
 
@@ -534,38 +567,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', 'coroutine'])
-            normalize_members(expr.get('data'))
             check_command(expr, info)
         elif meta == 'event':
-            check_keys(expr, info, meta,
-                       ['event'], ['data', 'boxed', 'if', 'features'])
-            normalize_members(expr.get('data'))
             check_event(expr, info)
         else:
             assert False, 'unexpected meta type'
-- 
2.26.2

[PATCH v2 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>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Cleber Rosa <crosa@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 3b62e801a42c..3b0a589652c9 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,
@@ -508,6 +511,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]
@@ -534,28 +557,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:
@@ -566,22 +581,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 v2 00/16] qapi: static typing conversion, pt3

[John Snow]

On 10/26/20 5:36 PM, John Snow wrote:
> based-on: <20201026194251.11075-1-jsnow@redhat.com>
>            [PATCH v2 00/11] qapi: static typing conversion, pt2

Ping,

This series can be reviewed independently of pt2, so I encourage you to 
try if you have the time.

> 
> Hi, this series adds static type hints to the QAPI module.
> This is part three, and it focuses on expr.py.
> 
> Part 3: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt3
> 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)
> 
> 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/
> 
> V2:
>   - Rebased on the latest V2
> 002/16:[0001] [FC] 'qapi/expr.py: Check for dict instead of OrderedDict'
>   - Import order differences
> 007/16:[0006] [FC] 'qapi/expr.py: Add casts in a few select cases'
>   - Import order differences
> 008/16:[0006] [FC] 'qapi/expr.py: add type hint annotations'
>   - Import order differents
> 012/16:[0066] [FC] 'qapi/expr.py: Add docstrings'
>   - Various docstring changes for Sphinx
> 014/16:[0004] [FC] 'qapi/expr.py: Use tuples instead of lists for static data'
>   - Change to accommodate new 'coroutine' key
> 015/16:[0006] [FC] 'qapi/expr.py: move related checks inside check_xxx functions'
>   - Fix check order (ehabkost)
> 
> 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  | 447 +++++++++++++++++++++++++++++++-----------
>   scripts/qapi/mypy.ini |   5 -
>   scripts/qapi/pylintrc |   1 -
>   3 files changed, 334 insertions(+), 119 deletions(-)
> 

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

[Marc-André Lureau]

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

Hi

On Wed, Nov 4, 2020 at 5:16 AM John Snow <jsnow@redhat.com> wrote:

> On 10/26/20 5:36 PM, John Snow wrote:
> > based-on: <20201026194251.11075-1-jsnow@redhat.com>
> >            [PATCH v2 00/11] qapi: static typing conversion, pt2
>
> Ping,
>
> This series can be reviewed independently of pt2, so I encourage you to
> try if you have the time.
>
> >
> > Hi, this series adds static type hints to the QAPI module.
> > This is part three, and it focuses on expr.py.
> >
> > Part 3: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt3
> > 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)
> >
> > 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/
> >
> > V2:
> >   - Rebased on the latest V2
> > 002/16:[0001] [FC] 'qapi/expr.py: Check for dict instead of OrderedDict'
> >   - Import order differences
> > 007/16:[0006] [FC] 'qapi/expr.py: Add casts in a few select cases'
> >   - Import order differences
> > 008/16:[0006] [FC] 'qapi/expr.py: add type hint annotations'
> >   - Import order differents
> > 012/16:[0066] [FC] 'qapi/expr.py: Add docstrings'
> >   - Various docstring changes for Sphinx
> > 014/16:[0004] [FC] 'qapi/expr.py: Use tuples instead of lists for static
> data'
> >   - Change to accommodate new 'coroutine' key
> > 015/16:[0006] [FC] 'qapi/expr.py: move related checks inside check_xxx
> functions'
> >   - Fix check order (ehabkost)
> >
> > 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  | 447 +++++++++++++++++++++++++++++++-----------
> >   scripts/qapi/mypy.ini |   5 -
> >   scripts/qapi/pylintrc |   1 -
> >   3 files changed, 334 insertions(+), 119 deletions(-)
> >
>
>
>
Looks all good to me. And you have already reviewed-by on all patches.
Given that it's hardening the current code, I would suggest to merge it
during the freeze. Unless Markus can maintain a qapi-next branch where we
can base our work on?

thanks

-- 
Marc-André Lureau

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

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> 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>
> Reviewed-by: Cleber Rosa <crosa@redhat.com>
> ---
>  scripts/qapi/expr.py | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 35695c4c653b..5694c501fa38 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -14,7 +14,6 @@
>  # This work is licensed under the terms of the GNU GPL, version 2.
>  # See the COPYING file in the top-level directory.
>  
> -from collections import OrderedDict
>  import re
>  
>  from .common import c_name
> @@ -131,7 +130,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
> @@ -162,7 +161,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)

Plain dict remembers insertion order since Python 3.6, but it wasn't
formally promised until 3.7.  

Can we simply ditch OrderedDict entirely?

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

[Eduardo Habkost]

On Tue, Nov 17, 2020 at 05:30:04PM +0100, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
> > 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>
> > Reviewed-by: Cleber Rosa <crosa@redhat.com>
> > ---
> >  scripts/qapi/expr.py | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> > index 35695c4c653b..5694c501fa38 100644
> > --- a/scripts/qapi/expr.py
> > +++ b/scripts/qapi/expr.py
> > @@ -14,7 +14,6 @@
> >  # This work is licensed under the terms of the GNU GPL, version 2.
> >  # See the COPYING file in the top-level directory.
> >  
> > -from collections import OrderedDict
> >  import re
> >  
> >  from .common import c_name
> > @@ -131,7 +130,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
> > @@ -162,7 +161,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)
> 
> Plain dict remembers insertion order since Python 3.6, but it wasn't
> formally promised until 3.7.  
> 
> Can we simply ditch OrderedDict entirely?

In theory, our build requirement is "Python >= 3.6", not
"CPython >= 3.6".  In practice, I don't expect anybody to ever
use any other Python implementation except CPython to build QEMU.

I think we can get rid of OrderedDict if you really want to.

-- 
Eduardo

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

[John Snow]

On 11/17/20 1:08 PM, Eduardo Habkost wrote:
> On Tue, Nov 17, 2020 at 05:30:04PM +0100, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> 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>
>>> Reviewed-by: Cleber Rosa <crosa@redhat.com>
>>> ---
>>>   scripts/qapi/expr.py | 5 ++---
>>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>>> index 35695c4c653b..5694c501fa38 100644
>>> --- a/scripts/qapi/expr.py
>>> +++ b/scripts/qapi/expr.py
>>> @@ -14,7 +14,6 @@
>>>   # This work is licensed under the terms of the GNU GPL, version 2.
>>>   # See the COPYING file in the top-level directory.
>>>   
>>> -from collections import OrderedDict
>>>   import re
>>>   
>>>   from .common import c_name
>>> @@ -131,7 +130,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
>>> @@ -162,7 +161,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)
>>
>> Plain dict remembers insertion order since Python 3.6, but it wasn't
>> formally promised until 3.7.
>>
>> Can we simply ditch OrderedDict entirely?
> 
> In theory, our build requirement is "Python >= 3.6", not
> "CPython >= 3.6".  In practice, I don't expect anybody to ever
> use any other Python implementation except CPython to build QEMU.
> 
> I think we can get rid of OrderedDict if you really want to.
> 

No harm in keeping it right now either, it doesn't make typing harder. 
The OrderedDict is created in the parser, so we can cover ditching 
OrderedDict when we get to that module if desired.

--js

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

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

I'm not sure I understand what you mean by "dict masquerading as
object".

>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> Reviewed-by: Cleber Rosa <crosa@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 5694c501fa38..f7c7f91326ef 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.

PEP 8: "For flowing long blocks of text with fewer structural
restrictions (docstrings or comments), the line length should be limited
to 72 characters."

> +Expression = MutableMapping[str, object]
>  
>  
>  # Names must be letters, numbers, -, and _.  They must start with letter,
> @@ -287,9 +295,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')

dict is arguably a rather awkward choice here.  I figure a named tuple
would be neater.  A class feels like overkill.  Observation, not demand.

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

I hope further typing work will reduce the isinstance() assertions
again.

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

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

Spoilsport.

Peeking at the patch... aha, it's about check_type()'s parameter
@allow_dict.

@allow_dict tells us whether an anonymous type is allowed, and also
whether its member names may violate the naming rules.

* a str: allow anonymous type, waive member naming rules if @allow_dict
  is in .name_case_whitelist.

  Used for checking struct's 'data' and union's 'base'.

* True: allow anonymous type, enforce member naming rules

  Used for checking 'data' of commands and events.  Waiving the naming
  rules is simply not implemented there.

* False (default): do not allow anonymous type

Perhaps the "is in .name_case_whitelist" check should be lifted into the
two callers that pass a str.  We could then turn the parameter into an
enum.  Meh.  Perhaps a separate @permit_upper parameter, only valid with
allow_dict=True.  Meh again.

Splitting check_type() into multiple functions feels more promising.
Not now.

> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> Reviewed-by: Cleber Rosa <crosa@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 f7c7f91326ef..2c4c341d5243 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -173,7 +173,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

Slightly more compact:

       permit_upper = (isinstance(allow_dict, str)
                       and allow_dict in info.pragma.name_case_whitelist)

Matter of taste.

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

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> 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>
> ---
>  scripts/qapi/expr.py | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 864363881682..2654a72e8333 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -254,6 +254,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)
> @@ -267,6 +270,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)

No raise without a test case covering it.

Add the test case *first*.  Just in case it demonstrates a frontend bug.
I believe these two are such bugs.

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> 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>
> Reviewed-by: Cleber Rosa <crosa@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 2654a72e8333..b8720b723377 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 typing import MutableMapping, Optional
> +from typing import MutableMapping, Optional, cast
>  
>  from .common import c_name
>  from .error import QAPISemError
> @@ -232,7 +232,7 @@ def check_enum(expr, info):
>  
>  
>  def check_struct(expr, info):
> -    name = expr['struct']
> +    name = cast(str, expr['struct'])  # Asserted in check_exprs

Suggest # check_exprs() ensures this

>      members = expr['data']
>  
>      check_type(members, info, "'data'", allow_dict=name)
> @@ -240,7 +240,7 @@ def check_struct(expr, info):
>  
>  
>  def check_union(expr, info):
> -    name = expr['union']
> +    name = cast(str, expr['union'])  # Asserted in check_exprs

Likewise.

>      base = expr.get('base')
>      discriminator = expr.get('discriminator')
>      members = expr['data']
> @@ -337,7 +337,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)

Uh, why do we need to cast then?

It's not actually "asserted".  check_name_is_str() ensures str by
raising an error if not str.

>          info.set_defn(meta, name)
>          check_defn_name_str(name, info, meta)