John Snow <js...@redhat.com> writes:
> On 2/16/21 3:55 AM, Markus Armbruster wrote:
>> John Snow <js...@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 <js...@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.
Automated assistance would be nice, but not having it is no big deal for
me. I don't mind pointing out the occasional long line I spot in
review.
>>> +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.
Let's leave it as is. Rationale: it only becomes a SchemaInfo in
_gen_tree().
>
>>> + 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?)
That would work.
Keeping it in this patch with a suitable hint in the commit message
would also work. Up to you. If you want me to tweak in my tree, tell
me how.
