Re: [PATCH v5 13/15] qapi/introspect.py: add introspect.json dummy types

[John Snow <jsnow@redhat.com>]

On 2/8/21 10:43 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> Add some aliases that declare intent for some of the "dictly-typed"
>> objects we pass around in introspect.py.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> ---
>>
>> This patch is optional, it can be dropped if desired. If it's taken,
>> it's probably best to squash it with the prior patch.  It is purely for
>> the sake of demonstrating what the _DObject was meant to convey: a
>> Python Dict that represents some JSON Object. It does not add any type
>> safety in and of itself, but does have some (light) annotational
>> benefit. In this case, it's usually a specific data structure from the
>> QAPI Schema we are referencing, but do not have "compile-time" access to
>> in Python.
>>
>> These are loosely typed and don't bother reproducing the exact structure
>> of the real types. Python 3.6 does not have support for TypedDict
>> structures, so this is as good as we can do without involving a
>> third-party library (e.g. Pydantic), in which we might be able to say:
>>
>>
>> class SchemaMetaType(str, enum.Enum):
>>      BUILTIN = "builtin"
>>      ENUM = "enum"
>>      ARRAY = "array"
>>      OBJECT = "object"
>>      ALTERNATE = "alternate"
>>      COMMAND = "command"
>>      EVENT = "event"
>>
>>
>> class SchemaInfo(pydantic.BaseModel):
>>      name: str
>>      meta-type: SchemaMetaType
>>      features: Optional[List[str]]
>>      data: Union[SchemaInfoBuiltin, SchemaInfoEnum, SchemaInfoArray,
>>                  SchemaInfoObject, SchemaInfoAlternate, SchemaInfoCommand,
>>                  SchemaInfoEvent]
>>
>>
>> However, the cost of reproducing and keeping these structure definitions
>> in-sync between the user-defined portion of the schema and the code
>> generator is likely not worth doing any such thing. However, it does
>> illustrate an interesting dependency the generator has on the
>> user-defined schema itself in terms of types.
>>
>> So, I settled on using some light types that suggest the form of the
>> object instead of enforcing the form.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qapi/introspect.py | 51 +++++++++++++++++++++++---------------
>>   1 file changed, 31 insertions(+), 20 deletions(-)
>>
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index 3afcdda7446..2a39726f40a 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -66,10 +66,15 @@
>>   _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]
>> +# 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.
>> +SchemaInfo = Dict[str, object]
>> +SchemaInfoObject = Dict[str, object]
>> +SchemaInfoObjectVariant = Dict[str, object]
>> +SchemaInfoObjectMember = Dict[str, object]
>> +SchemaInfoCommand = Dict[str, object]
>> +
>>   
>>   _NodeT = TypeVar('_NodeT', bound=_value)
>>   
>> @@ -162,7 +167,7 @@ def __init__(self, prefix: str, unmask: bool):
>>               ' * QAPI/QMP schema introspection', __doc__)
>>           self._unmask = unmask
>>           self._schema: Optional[QAPISchema] = None
>> -        self._trees: List[Annotated[_DObject]] = []
>> +        self._trees: List[Annotated[SchemaInfo]] = []
>>           self._used_types: List[QAPISchemaType] = []
>>           self._name_map: Dict[str, str] = {}
>>           self._genc.add(mcgen('''
>> @@ -234,9 +239,18 @@ def _gen_features(features: List[QAPISchemaFeature]
>>                         ) -> List[Annotated[str]]:
>>           return [Annotated(f.name, f.ifcond) for f in features]
>>   
>> -    def _gen_tree(self, name: str, mtype: str, obj: _DObject,
>> +    def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
>>                     ifcond: List[str],
>>                     features: Optional[List[QAPISchemaFeature]]) -> None:
>> +        """
>> +        Build and append a SchemaInfo object to self._trees.
>> +
>> +        :param name: The entity's name.
>> +        :param mtype: The entity's meta-type.
>> +        :param obj: Additional entity fields, as appropriate for the meta-type.
>> +        :param ifcond: List of conditionals that apply to this entire entity.
>> +        :param features: Optional features field for SchemaInfo.
>> +        """
>>           comment: Optional[str] = None
>>           if mtype not in ('command', 'event', 'builtin', 'array'):
>>               if not self._unmask:
>> @@ -251,8 +265,8 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject,
>>           self._trees.append(Annotated(obj, ifcond, comment))
>>   
>>       def _gen_member(self, member: QAPISchemaObjectTypeMember
>> -                    ) -> Annotated[_DObject]:
>> -        obj: _DObject = {
>> +                    ) -> Annotated[SchemaInfoObjectMember]:
>> +        obj: SchemaInfoObjectMember = {
>>               'name': member.name,
>>               'type': self._use_type(member.type)
>>           }
>> @@ -262,13 +276,9 @@ def _gen_member(self, member: QAPISchemaObjectTypeMember
>>               obj['features'] = self._gen_features(member.features)
>>           return Annotated(obj, member.ifcond)
>>   
>> -    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: QAPISchemaVariant) -> Annotated[_DObject]:
>> -        obj: _DObject = {
>> +    def _gen_variant(self, variant: QAPISchemaVariant
>> +                     ) -> Annotated[SchemaInfoObjectVariant]:
>> +        obj: SchemaInfoObjectVariant = {
>>               'case': variant.name,
>>               'type': self._use_type(variant.type)
>>           }
>> @@ -300,11 +310,12 @@ def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
>>                                  features: List[QAPISchemaFeature],
>>                                  members: List[QAPISchemaObjectTypeMember],
>>                                  variants: Optional[QAPISchemaVariants]) -> None:
>> -        obj: _DObject = {'members': [self._gen_member(m) for m in members]}
>> +        obj: SchemaInfoObject = {
>> +            'members': [self._gen_member(m) for m in members]
>> +        }
>>           if variants:
>> -            obj.update(self._gen_variants(variants.tag_member.name,
>> -                                          variants.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: str, info: Optional[QAPISourceInfo],
>> @@ -329,7 +340,7 @@ def visit_command(self, name: str, info: Optional[QAPISourceInfo],
>>   
>>           arg_type = arg_type or self._schema.the_empty_object_type
>>           ret_type = ret_type or self._schema.the_empty_object_type
>> -        obj: _DObject = {
>> +        obj: SchemaInfoCommand = {
>>               'arg-type': self._use_type(arg_type),
>>               'ret-type': self._use_type(ret_type)
>>           }
> 
> I see what you mean by "some (light) annotational benefit".
> 
> I figure some of is simply due to going from _DObject, a name that tells
> me nothing, to SchemaInfo, which makes me go aha!
> 

Naming is hard! Using the *exact* names seems helpful.

> The remainder is having the subtypes of SchemaInfo, too. albeit without
> actual type checking.  Worthwhile?
> 

For referential purposes it seems useful, at least in so far as 
declaring intent. When I was trying to type this series, I was not aware 
this module was building structures defined in introspect.json until 
quite late into the series.

I kept wondering: "What are the valid keys here? Who consumes this?" -- 
It wasn't obvious.

I'm leaning towards just keeping them all for the documentation benefit, 
though I admit it *could* mislead someone into believing it's type 
checked when it isn't, really. I tried to call that out with the comment 
at the top, which I hope suffices to diffuse that potential 
faith-based-landmine.

> I agree it should be squashed if we want it, or parts of it.
> 

Squash all of it, IMO.

--js