Re: [PATCH v6 14/19] qapi/introspect.py: add type hint annotations

[John Snow <jsnow@redhat.com>]

On 2/16/21 3:55 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> NB: The type aliases (SchemaInfo et al) declare intent for some of the
>> "dictly-typed" objects we pass around in introspect.py. They do not
>> enforce the shape of those objects, and cannot, until Python 3.7 or
>> later. (And even then, it may not be "worth it".)
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qapi/introspect.py | 124 +++++++++++++++++++++++++++----------
>>   scripts/qapi/mypy.ini      |   5 --
>>   scripts/qapi/schema.py     |   2 +-
>>   3 files changed, 92 insertions(+), 39 deletions(-)
>>
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index b0fcc4443c1..45284af1330 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -17,6 +17,7 @@
>>       Iterable,
>>       List,
>>       Optional,
>> +    Sequence,
>>       Tuple,
>>       TypeVar,
>>       Union,
>> @@ -30,10 +31,19 @@
>>   )
>>   from .gen import QAPISchemaMonolithicCVisitor
>>   from .schema import (
>> +    QAPISchema,
>>       QAPISchemaArrayType,
>>       QAPISchemaBuiltinType,
>> +    QAPISchemaEntity,
>> +    QAPISchemaEnumMember,
>> +    QAPISchemaFeature,
>> +    QAPISchemaObjectType,
>> +    QAPISchemaObjectTypeMember,
>>       QAPISchemaType,
>> +    QAPISchemaVariant,
>> +    QAPISchemaVariants,
>>   )
>> +from .source import QAPISourceInfo
>>   
>>   
>>   # This module constructs a tree data structure that is used to
>> @@ -58,6 +68,15 @@
>>   _Value = Union[_Scalar, _NonScalar]
>>   JSONValue = Union[_Value, 'Annotated[_Value]']
>>   
>> +# These types are based on structures defined in QEMU's schema, so we lack
>> +# precise types for them here. Python 3.6 does not offer TypedDict constructs,
>> +# so they are broadly typed here as simple Python Dicts.
> 
> PEP 8: "For flowing long blocks of text with fewer structural
> restrictions (docstrings or comments), the line length should be limited
> to 72 characters."
> 

I'm very likely going to keep violating this until some tool enforces it 
on me. I'm also very unlikely to enforce it for anyone else.

You can reflow it as you see fit, but I'll likely need better long-term 
assistance for remembering that 72/80 column DANGER ZONE.

>> +SchemaInfo = Dict[str, object]
>> +SchemaInfoObject = Dict[str, object]
>> +SchemaInfoObjectVariant = Dict[str, object]
>> +SchemaInfoObjectMember = Dict[str, object]
>> +SchemaInfoCommand = Dict[str, object]
>> +
>>   
>>   _ValueT = TypeVar('_ValueT', bound=_Value)
>>   
>> @@ -77,9 +96,11 @@ def __init__(self, value: _ValueT, ifcond: Iterable[str],
>>           self.ifcond: Tuple[str, ...] = tuple(ifcond)
>>   
>>   
>> -def _tree_to_qlit(obj, level=0, dict_value=False):
>> +def _tree_to_qlit(obj: JSONValue,
>> +                  level: int = 0,
>> +                  dict_value: bool = False) -> str:
>>   
>> -    def indent(level):
>> +    def indent(level: int) -> str:
>>           return level * 4 * ' '
>>   
>>       if isinstance(obj, Annotated):
>> @@ -136,21 +157,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):
>>           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[Annotated[SchemaInfo]] = []
>> +        self._used_types: List[QAPISchemaType] = []
>> +        self._name_map: Dict[str, str] = {}
>>           self._genc.add(mcgen('''
>>   #include "qemu/osdep.h"
>>   #include "%(prefix)sqapi-introspect.h"
>> @@ -158,10 +179,10 @@ def __init__(self, prefix, unmask):
>>   ''',
>>                                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)
>> @@ -183,18 +204,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
>> @@ -216,10 +237,13 @@ def _use_type(self, typ):
>>           return self._name(typ.name)
>>   
>>       @staticmethod
>> -    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: Dict[str, object],
> 
> Schould this be obj: SchemaInfo?
> 

Yes-ish. It's kind of like the dictly-typed object is being promoted to 
a SchemaInfo. In a sense, it isn't one yet (It's missing necessary 
keys), but we upgrade it into one in this very function.

I talk about TypedDict a lot and how we don't have it yet; one 
interesting feature of TypedDict is that it doesn't allow you to 
incrementally build the object -- it requires all of the necessary keys 
be present right away.

If we were to have that kind of model in our heads, then this wouldn't 
be a SchemaInfo coming in.

So I'll admit here: I don't know. It depends on your perspective, 
honestly. It might be the sort of thing that a docstring comment would 
be best at addressing, since we're already in the margins for what mypy 
can reasonably enforce statically.

>> +                  ifcond: Sequence[str],
>> +                  features: Optional[List[QAPISchemaFeature]]) -> None:
>>           comment: Optional[str] = None
>>           if mtype not in ('command', 'event', 'builtin', 'array'):
>>               if not self._unmask:
>> @@ -233,42 +257,65 @@ 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[SchemaInfoObjectMember]:
>> +        obj: SchemaInfoObjectMember = {
>> +            '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_variant(self, variant):
>> -        obj = {'case': variant.name, 'type': self._use_type(variant.type)}
>> +    def _gen_variant(self, variant: QAPISchemaVariant
>> +                     ) -> Annotated[SchemaInfoObjectVariant]:
>> +        obj: SchemaInfoObjectVariant = {
>> +            '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:
>>           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: Sequence[str],
>> +                        features: List[QAPISchemaFeature],
>> +                        members: List[QAPISchemaEnumMember],
>> +                        prefix: Optional[str]) -> None:
>>           self._gen_tree(
>>               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: Sequence[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: Sequence[str],
>> +                               features: List[QAPISchemaFeature],
>> +                               members: List[QAPISchemaObjectTypeMember],
>> +                               variants: Optional[QAPISchemaVariants]) -> None:
>> +        obj: SchemaInfoObject = {
>> +            'members': [self._gen_member(m) for m in members]
>> +        }
>>           if variants:
>>               obj['tag'] = variants.tag_member.name
>>               obj['variants'] = [self._gen_variant(v) for v in 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: Optional[QAPISourceInfo],
>> +                             ifcond: Sequence[str],
>> +                             features: List[QAPISchemaFeature],
>> +                             variants: QAPISchemaVariants) -> None:
>>           self._gen_tree(
>>               name, 'alternate',
>>               {'members': [Annotated({'type': self._use_type(m.type)},
>> @@ -277,27 +324,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: Sequence[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: SchemaInfoCommand = {
>> +            '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: Sequence[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)
>>       schema.visit(vis)
>>       vis.write(output_dir)
>> 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
>>   
>> -[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 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)
> 
> How is this hunk related to typing introspect.py
> 

I forget!

qapi/introspect.py:262: error: Returning Any from function declared to 
return "str"
Found 1 error in 1 file (checked 14 source files)

Oh, for this reason:

         if isinstance(typ, QAPISchemaBuiltinType):
             return typ.name

_use_type has a return type that is dependent upon the type of 
"typ.name", which required typing the QAPISchemaEntity initializer.


(Do you want this in its own preceding patch?)