Re: [PATCH v2 32/38] qapi/introspect.py: create a typed 'Node' data structure

[John Snow <jsnow@redhat.com>]

On 9/23/20 2:41 PM, Eduardo Habkost wrote:
> On Tue, Sep 22, 2020 at 05:00:55PM -0400, John Snow wrote:
>> Replacing the un-typed tuple, add a typed Node that we can add typed
>> metadata to.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> This is the most complex patch so far, and it's very hard to
> understand what it does without type annotations.
> 
> Have you consider adding type annotations to the code before
> patch 30/38 (even if using `object` in some parts), so we can
> make this easier to review?
> 
> In case it's useful, below is an attempt to add type annotations
> to the old code.  It can be applied after patch 29/38.  It reuses
> portions of patch 33/38.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   scripts/qapi/introspect.py | 138 ++++++++++++++++++++++++++-----------
>   scripts/qapi/mypy.ini      |   5 --
>   scripts/qapi/schema.py     |   2 +-
>   3 files changed, 100 insertions(+), 45 deletions(-)
> 
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index b036fcf9ce7..4eaebdef58b 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -10,6 +10,17 @@ This work is licensed under the terms of the GNU GPL, version 2.
>   See the COPYING file in the top-level directory.
>   """
>   
> +from typing import (
> +    Dict,
> +    Generic,
> +    List,
> +    NamedTuple,
> +    Optional,
> +    Sequence,
> +    TypeVar,
> +    Tuple
> +)
> +
>   from .common import (
>       c_name,
>       gen_endif,
> @@ -17,25 +28,48 @@ from .common import (
>       mcgen,
>   )
>   from .gen import QAPISchemaMonolithicCVisitor
> -from .schema import (QAPISchemaArrayType, QAPISchemaBuiltinType,
> -                     QAPISchemaType)
> -
> -
> -def _make_tree(obj, ifcond, features, extra=None):
> +from .schema import (
> +    QAPISchema,
> +    QAPISchemaArrayType,
> +    QAPISchemaBuiltinType,
> +    QAPISchemaEntity,
> +    QAPISchemaEnumMember,
> +    QAPISchemaFeature,
> +    QAPISchemaObjectType,
> +    QAPISchemaObjectTypeMember,
> +    QAPISchemaType,
> +    QAPISchemaVariant,
> +    QAPISchemaVariants,
> +)
> +from .source import QAPISourceInfo
> +
> +T = TypeVar('T')
> +# this should actually be: Union[str, list, dict, bool, 'AnnotatedNode']
> +# but mypy doesn't support recursive types
> +TreeNode = object
> +TreeDict = Dict[str, TreeNode]
> +Extra = Dict[str, object]
> +AnnotatedNode = Tuple[T, Extra]
> +
> +def _make_tree(obj: TreeDict, ifcond: List[str],
                        ^^^^^^^^

Technically untrue! obj may be a SchemaInfo-like dict, or a string.
(And I will tell you why mypy didn't catch this!)

> +               features: List[QAPISchemaFeature],
> +               extra: Optional[Extra] = None) -> TreeNode:
>       if extra is None:
>           extra = {}
>       if ifcond:
>           extra['if'] = ifcond
>       if features:
> -        obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]
> +        obj['features'] = ([(f.name, {'if': f.ifcond}) for f in features])
>       if extra:
>           return (obj, extra)
>       return obj
>   
>   
> -def _tree_to_qlit(obj, level=0, suppress_first_indent=False):
> +def _tree_to_qlit(obj: TreeNode,
> +                  level: int = 0,
> +                  suppress_first_indent : bool = False) -> str:
>   
> -    def indent(level):
> +    def indent(level: int) -> str:
>           return level * 4 * ' '
>   
>       if isinstance(obj, tuple):
> @@ -85,21 +119,20 @@ def _tree_to_qlit(obj, level=0, suppress_first_indent=False):
>       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):
>           super().__init__(
>               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[TreeNode] = []
> +        self._used_types: List[QAPISchemaType] = []
> +        self._name_map: Dict[str, str] = {}
>           self._genc.add(mcgen('''
>   #include "qemu/osdep.h"
>   #include "%(prefix)sqapi-introspect.h"
> @@ -107,10 +140,10 @@ class QAPISchemaGenIntrospectVisitor(QAPISchemaMonolithicCVisitor):
>   ''',
>                                prefix=prefix))
>   
> -    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:
>               typ.visit(self)
> @@ -132,18 +165,18 @@ const QLitObject %(c_name)s = %(c_string)s;
>           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:
>           # Map the various integer types to plain int
>           if typ.json_type() == 'int':
>               typ = self._schema.lookup_type('int')
> @@ -162,8 +195,10 @@ const QLitObject %(c_name)s = %(c_string)s;
>               return '[' + self._use_type(typ.element_type) + ']'
>           return self._name(typ.name)
>   
> -    def _gen_tree(self, name, mtype, obj, ifcond, features):
> -        extra = None
> +    def _gen_tree(self, name: str, mtype: str, obj: TreeDict,
> +                  ifcond: List[str],
> +                  features: Optional[List[QAPISchemaFeature]]) -> None:
> +        extra: Extra = None
>           if mtype not in ('command', 'event', 'builtin', 'array'):
>               if not self._unmask:
>                   # Output a comment to make it easy to map masked names
> @@ -174,44 +209,60 @@ const QLitObject %(c_name)s = %(c_string)s;
>           obj['meta-type'] = mtype
>           self._trees.append(_make_tree(obj, ifcond, features, extra))
>   
> -    def _gen_member(self, member):
> +    def _gen_member(self,
> +                    member: QAPISchemaObjectTypeMember) -> TreeNode:
>           obj = {'name': member.name, 'type': self._use_type(member.type)}
>           if member.optional:
>               obj['default'] = None
>           return _make_tree(obj, member.ifcond, member.features)
>   
> -    def _gen_variants(self, tag_name, variants):
> +    def _gen_variants(self, tag_name: str,
> +                      variants: List[QAPISchemaVariant]) -> TreeDict:
>           return {'tag': tag_name,
>                   'variants': [self._gen_variant(v) for v in variants]}
>   
> -    def _gen_variant(self, variant):
> +    def _gen_variant(self, variant: QAPISchemaVariant) -> TreeNode:
>           obj = {'case': variant.name, 'type': self._use_type(variant.type)}
>           return _make_tree(obj, variant.ifcond, None)
>   
> -    def visit_builtin_type(self, name, info, json_type):
> +    def visit_builtin_type(self, name: str,
> +                           info: Optional[QAPISourceInfo],
> +                           json_type: str) -> None:
>           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: QAPISourceInfo,
> +                        ifcond: List[str], features: List[QAPISchemaFeature],
> +                        members: List[QAPISchemaEnumMember],
> +                        prefix: Optional[str]) -> None:
>           self._gen_tree(name, 'enum',
>                          {'values': [_make_tree(m.name, m.ifcond, None)
>                                      for m in members]},

                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Over here, Members is List[QAPISchemaEnumMember], which is true enough, 
but we don't yet have a typing for the 'str' field. mypy fills in with 
the "Any" type, which is *assumed* to fill the type we want.

Nope! It's a string. Once you add the type annotations for schema.py, 
this would fail.

>                          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': element},
>                          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: Sequence[QAPISchemaObjectTypeMember],
> +                               variants: Optional[QAPISchemaVariants]) -> None:
> +        obj: TreeDict = {'members': [self._gen_member(m) for m in members]}
>           if variants:
>               obj.update(self._gen_variants(variants.tag_member.name,
>                                             variants.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: QAPISourceInfo,
> +                             ifcond: List[str],
> +                             features: List[QAPISchemaFeature],
> +                             variants: QAPISchemaVariants) -> None:
> +
>           self._gen_tree(name, 'alternate',
>                          {'members': [
>                              _make_tree({'type': self._use_type(m.type)},
> @@ -219,24 +270,33 @@ const QLitObject %(c_name)s = %(c_string)s;
>                              for m in variants.variants]},
>                          ifcond, features)
>   
> -    def visit_command(self, name, info, ifcond, features,
> -                      arg_type, ret_type, gen, success_response, boxed,
> -                      allow_oob, allow_preconfig):
> +    def visit_command(self, name: str, info: QAPISourceInfo, ifcond: List[str],
> +                      features: List[QAPISchemaFeature],
> +                      arg_type: QAPISchemaObjectType,
> +                      ret_type: Optional[QAPISchemaType], gen: bool,
> +                      success_response: bool, boxed: bool, allow_oob: bool,
> +                      allow_preconfig: bool) -> 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: TreeDict = {
> +            '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: QAPISourceInfo,
> +                    ifcond: List[str], features: List[QAPISchemaFeature],
> +                    arg_type: QAPISchemaObjectType, boxed: bool) -> 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)
>       schema.visit(vis)
>       vis.write(output_dir)
> diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
> index dbfeda748cc..9ce8b56f225 100644
> --- a/scripts/qapi/mypy.ini
> +++ b/scripts/qapi/mypy.ini
> @@ -19,11 +19,6 @@ disallow_untyped_defs = False
>   disallow_incomplete_defs = False
>   check_untyped_defs = False
>   
> -[mypy-qapi.introspect]
> -disallow_untyped_defs = False
> -disallow_incomplete_defs = False
> -check_untyped_defs = False
> -
>   [mypy-qapi.parser]
>   disallow_untyped_defs = False
>   disallow_incomplete_defs = False
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index bb0cd717f1a..3023bab44b6 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -28,7 +28,7 @@ from .parser import QAPISchemaParser
>   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)
>