[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v5 12/15] qapi/introspect.py: add type hint annotations

From: John Snow
Subject: Re: [PATCH v5 12/15] qapi/introspect.py: add type hint annotations
Date: Mon, 8 Feb 2021 15:45:21 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0

On 2/8/21 10:34 AM, Markus Armbruster wrote:
John Snow <jsnow@redhat.com> writes:

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


See the next patch for an optional amendment that helps to clarify what
_DObject is meant to be.

Signed-off-by: John Snow <jsnow@redhat.com>
  scripts/qapi/introspect.py | 117 ++++++++++++++++++++++++++-----------
  scripts/qapi/mypy.ini      |   5 --
  scripts/qapi/schema.py     |   2 +-
  3 files changed, 84 insertions(+), 40 deletions(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index cf0e4e05c5c..3afcdda7446 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -30,10 +30,19 @@
  from .gen import QAPISchemaMonolithicCVisitor
  from .schema import (
+    QAPISchema,
+    QAPISchemaEntity,
+    QAPISchemaEnumMember,
+    QAPISchemaFeature,
+    QAPISchemaObjectType,
+    QAPISchemaObjectTypeMember,
+    QAPISchemaVariant,
+    QAPISchemaVariants,
+from .source import QAPISourceInfo
# This module constructs a tree data structure that is used to
@@ -57,6 +66,10 @@
    _stub = Any
    _scalar = Union[str, bool, None]
    _nonscalar = Union[Dict[str, _stub], List[_stub]]
  _value = Union[_scalar, _nonscalar]
  TreeValue = Union[_value, 'Annotated[_value]']
+# This is an alias for an arbitrary JSON object, represented here as a Dict.
+# It is stricter on the value type than the recursive definition above.
+# It is used to represent SchemaInfo-related structures exclusively.
+_DObject = Dict[str, object]

Please work in an abridged version of your helpful reply to my ignorant
questions in review of v4.

Maybe not needed after we squash the named aliases patch in?.

My comments below are based on the following understanding:

_value has a Dict[str, Any] branch.

_DObject is Dict[str, object].

Both types are for the tree's dict nodes.  Both under-constrain the
dict's values in the sense that anything can go into the dict.  The
difference is static type checking on use of dict values: none with the
former, overly strict with the latter (to actually do something
interesting with a value, you usually have to narrow its static type to
a more specific one than object).

So, having _DObject in addition to the _value branch buys us a little
more static type checking.  I don't understand what type checking
exactly, and therefore can't judge whether it's worth the price in
complexity.  If you can enlighten me, I'm all ears.

It enables type checking on any value that a _DObject (SchemaInfo and its union arm types) may have.

foo: SchemaInfo = {}  # Dict[str, object]

says that the values here must be an object. Everything is an object, so this doesn't constrain much in the write direction, but it constrains a lot in the read direction.

the type of this expression:


is object, not "Any". Which means if we do this:

x = foo.get('key') + 5

that is a type error for mypy. If we pass this value on to any function that is looking for or expects a specific type, it will also be an error. If we treat this value in any way beyond the capabilities of Python's most basic, abstract type, it will be an error.

When you use 'Any', it actually *disables* type checking at that boundary instead. If you pass an "Any" on to an interface that expects "int", mypy will just assume you're right about that.

When I started this series I wasn't as clear on the distinction myself, but during review and re-iteration I've shifted to using 'object' absolutely whenever I can if I need to describe an abstract value.


- I prefer "object" to "Any", when possible
- We have to use "Any" for the recursive stubs
- I use _DObject (and later SchemaInfo et al) to represent specific types, not abstract recursive types, so they get the stricter 'object'.

_NodeT = TypeVar('_NodeT', bound=_value) @@ -76,9 +89,11 @@ def __init__(self, value: _NodeT, ifcond: Iterable[str],
          self.ifcond: Tuple[str, ...] = tuple(ifcond)
-def _tree_to_qlit(obj, level=0, dict_value=False):
+def _tree_to_qlit(obj: TreeValue,
+                  level: int = 0,
+                  dict_value: bool = False) -> str:
- def indent(level):
+    def indent(level: int) -> str:
          return level * 4 * ' '
if isinstance(obj, Annotated):
@@ -135,21 +150,21 @@ def indent(level):
      return ret
-def to_c_string(string):
+def to_c_string(string: str) -> str:
      return '"' + string.replace('\\', r'\\').replace('"', r'\"') + '"'
class QAPISchemaGenIntrospectVisitor(QAPISchemaMonolithicCVisitor): - def __init__(self, prefix, unmask):
+    def __init__(self, prefix: str, unmask: bool):
              prefix, 'qapi-introspect',
              ' * QAPI/QMP schema introspection', __doc__)
          self._unmask = unmask
-        self._schema = None
-        self._trees = []
-        self._used_types = []
-        self._name_map = {}
+        self._schema: Optional[QAPISchema] = None
+        self._trees: List[Annotated[_DObject]] = []
+        self._used_types: List[QAPISchemaType] = []
+        self._name_map: Dict[str, str] = {}
  #include "qemu/osdep.h"
  #include "%(prefix)sqapi-introspect.h"
@@ -157,10 +172,10 @@ def __init__(self, prefix, unmask):
- def visit_begin(self, schema):
+    def visit_begin(self, schema: QAPISchema) -> None:
          self._schema = schema
- def visit_end(self):
+    def visit_end(self) -> None:
          # visit the types that are actually used
          for typ in self._used_types:
@@ -182,18 +197,18 @@ def visit_end(self):
          self._used_types = []
          self._name_map = {}
- def visit_needed(self, entity):
+    def visit_needed(self, entity: QAPISchemaEntity) -> bool:
          # Ignore types on first pass; visit_end() will pick up used types
          return not isinstance(entity, QAPISchemaType)
- def _name(self, name):
+    def _name(self, name: str) -> str:
          if self._unmask:
              return name
          if name not in self._name_map:
              self._name_map[name] = '%d' % len(self._name_map)
          return self._name_map[name]
- def _use_type(self, typ):
+    def _use_type(self, typ: QAPISchemaType) -> str:
          assert self._schema is not None
# Map the various integer types to plain int
@@ -215,10 +230,13 @@ def _use_type(self, typ):
          return self._name(typ.name)
-    def _gen_features(features):
+    def _gen_features(features: List[QAPISchemaFeature]
+                      ) -> List[Annotated[str]]:
          return [Annotated(f.name, f.ifcond) for f in features]
- def _gen_tree(self, name, mtype, obj, ifcond, features):
+    def _gen_tree(self, name: str, mtype: str, obj: _DObject,
+                  ifcond: List[str],
+                  features: Optional[List[QAPISchemaFeature]]) -> None:

Recycling my review of v2...

* @name corresponds to SchemaInfo member name, which is indeed str.

* @mtype corresponds to member meta-type, which is enum SchemaMetaType.
   It's not a Python Enum, because those were off limits when this code
   was written.  We type what we have, and we have str.  Okay.

I can *make* it an enum if you'd like. I'll throw it on the end of the series as an optional patch that can be kept or dropped at your discretion.

* @obj will be turned by _gen_tree() into the tree for a SchemaInfo.
   _DObject fits the bill.

* ifcond: List[str] should work, but gen_if(), gen_endif() use
   Sequence[str].  When I pointed this out for v2, you replied "should
   probable be using Sequence".

   More instances of ifcond: List[str] elsewhere; I'm not flagging them.

Oh, yes.

List winds up being correct, but is unnecessarily restrictive. It doesn't wind up mattering, but we can use Sequence[str].

I'll add a patch to fix the types-so-far at the beginning of the series, and then squash the other annotation changes into this patch.

* features: Optional[List[QAPISchemaFeature]] is correct.  "No features"
   has two representations: None and [].  I guess we could eliminate
   None, trading a tiny bit of efficiency for simpler typing.  Not a

This is easy enough to make happen. I didn't notice.

I'll include it as an extra patch at the end of the series.

          comment: Optional[str] = None

"No annotations" is represented as None here, not {}.  I guess we could
use {} for simpler typing.  When I pointed this out for v2, you replied
it'll go away later.  Good enough for me.

Something might have gotten garbled between then and now. I consider "Annotations" to be both the if conditionals and comments; this field is just the comment. Optional[str] is the right type for it here:

self._trees.append(Annotated(obj, ifcond, comment))

          if mtype not in ('command', 'event', 'builtin', 'array'):
              if not self._unmask:
@@ -232,47 +250,67 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
              obj['features'] = self._gen_features(features)
          self._trees.append(Annotated(obj, ifcond, comment))
- def _gen_member(self, member):
-        obj = {'name': member.name, 'type': self._use_type(member.type)}
+    def _gen_member(self, member: QAPISchemaObjectTypeMember
+                    ) -> Annotated[_DObject]:
+        obj: _DObject = {
+            'name': member.name,
+            'type': self._use_type(member.type)
+        }
          if member.optional:
              obj['default'] = None
          if member.features:
              obj['features'] = self._gen_features(member.features)
          return Annotated(obj, member.ifcond)
- def _gen_variants(self, tag_name, variants):
+    def _gen_variants(self, tag_name: str,
+                      variants: List[QAPISchemaVariant]) -> _DObject:
          return {'tag': tag_name,
                  'variants': [self._gen_variant(v) for v in variants]}
- def _gen_variant(self, variant):
-        obj = {'case': variant.name, 'type': self._use_type(variant.type)}
+    def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated[_DObject]:
+        obj: _DObject = {
+            'case': variant.name,
+            'type': self._use_type(variant.type)
+        }
          return Annotated(obj, variant.ifcond)
- def visit_builtin_type(self, name, info, json_type):
+    def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
+                           json_type: str) -> None:

A built-in type's info is always None.  Perhaps we should drop the

We could.

For the moment, info cannot be "None" here because mypy thinks it *might* be something.

...but we could just remove it for clarity. (As long as we don't later re-add that built-in source info object style idea and then we'd want to put it back, but ...)


          self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
- def visit_enum_type(self, name, info, ifcond, features, members, prefix):
+    def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
+                        ifcond: List[str], features: List[QAPISchemaFeature],
+                        members: List[QAPISchemaEnumMember],
+                        prefix: Optional[str]) -> None:
              name, 'enum',
              {'values': [Annotated(m.name, m.ifcond) for m in members]},
              ifcond, features
- def visit_array_type(self, name, info, ifcond, element_type):
+    def visit_array_type(self, name: str, info: Optional[QAPISourceInfo],
+                         ifcond: List[str],
+                         element_type: QAPISchemaType) -> None:
          element = self._use_type(element_type)
          self._gen_tree('[' + element + ']', 'array', {'element-type': 
                         ifcond, None)
- def visit_object_type_flat(self, name, info, ifcond, features,
-                               members, variants):
-        obj = {'members': [self._gen_member(m) for m in members]}
+    def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
+                               ifcond: List[str],
+                               features: List[QAPISchemaFeature],
+                               members: List[QAPISchemaObjectTypeMember],
+                               variants: Optional[QAPISchemaVariants]) -> None:

We represent "no variants" as None, not as [].  I guess we could
eliminate use [], trading a tiny bit of efficiency for simpler typing.
When I pointed this out for v2, you replied we should turn
QAPISchemaVariants into an extension of Sequence[QAPISchemaVariant] in a
later series.  Okay.

Yeah, that's a longer-term fix I think. Not for doing right now -- but you're right, it stands out as suspect.

I think ideally we'd have a Variants object that behaves like a collection where the empty collection is false-ish and we'd always create such a collection.

(Though, that might create problems for tag_name vs tag_member where we might now have circumstances where we have neither, maybe that makes typing harder instead of easier. I'll need to experiment ... I have some questions about how to achieve better typing for our collections class that clash with the delayed-type-evaluation mechanisms we have. Ongoing research.)

+        obj: _DObject = {'members': [self._gen_member(m) for m in members]}
          if variants:
self._gen_tree(name, 'object', obj, ifcond, features) - def visit_alternate_type(self, name, info, ifcond, features, variants):
+    def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
+                             ifcond: List[str],
+                             features: List[QAPISchemaFeature],
+                             variants: QAPISchemaVariants) -> None:
              name, 'alternate',
              {'members': [Annotated({'type': self._use_type(m.type)}, m.ifcond)
@@ -280,27 +318,38 @@ def visit_alternate_type(self, name, info, ifcond, 
features, variants):
              ifcond, features
- def visit_command(self, name, info, ifcond, features,
-                      arg_type, ret_type, gen, success_response, boxed,
-                      allow_oob, allow_preconfig, coroutine):
+    def visit_command(self, name: str, info: Optional[QAPISourceInfo],
+                      ifcond: List[str],
+                      features: List[QAPISchemaFeature],
+                      arg_type: Optional[QAPISchemaObjectType],
+                      ret_type: Optional[QAPISchemaType], gen: bool,
+                      success_response: bool, boxed: bool, allow_oob: bool,
+                      allow_preconfig: bool, coroutine: bool) -> None:
          assert self._schema is not None
arg_type = arg_type or self._schema.the_empty_object_type
          ret_type = ret_type or self._schema.the_empty_object_type
-        obj = {'arg-type': self._use_type(arg_type),
-               'ret-type': self._use_type(ret_type)}
+        obj: _DObject = {
+            'arg-type': self._use_type(arg_type),
+            'ret-type': self._use_type(ret_type)
+        }
          if allow_oob:
              obj['allow-oob'] = allow_oob
          self._gen_tree(name, 'command', obj, ifcond, features)
- def visit_event(self, name, info, ifcond, features, arg_type, boxed):
+    def visit_event(self, name: str, info: Optional[QAPISourceInfo],
+                    ifcond: List[str], features: List[QAPISchemaFeature],
+                    arg_type: Optional[QAPISchemaObjectType],
+                    boxed: bool) -> None:
          assert self._schema is not None
          arg_type = arg_type or self._schema.the_empty_object_type
          self._gen_tree(name, 'event', {'arg-type': self._use_type(arg_type)},
                         ifcond, features)
-def gen_introspect(schema, output_dir, prefix, opt_unmask):
+def gen_introspect(schema: QAPISchema, output_dir: str, prefix: str,
+                   opt_unmask: bool) -> None:
      vis = QAPISchemaGenIntrospectVisitor(prefix, opt_unmask)
diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
index 04bd5db5278..0a000d58b37 100644
--- a/scripts/qapi/mypy.ini
+++ b/scripts/qapi/mypy.ini
@@ -13,11 +13,6 @@ disallow_untyped_defs = False
  disallow_incomplete_defs = False
  check_untyped_defs = False
-disallow_untyped_defs = False
-disallow_incomplete_defs = False
-check_untyped_defs = False
  disallow_untyped_defs = False
  disallow_incomplete_defs = False
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 353e8020a27..ff16578f6de 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -28,7 +28,7 @@
  class QAPISchemaEntity:
      meta: Optional[str] = None
- def __init__(self, name, info, doc, ifcond=None, features=None):
+    def __init__(self, name: str, info, doc, ifcond=None, features=None):
          assert name is None or isinstance(name, str)
          for f in features or []:
              assert isinstance(f, QAPISchemaFeature)

reply via email to

[Prev in Thread] Current Thread [Next in Thread]