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

[Markus Armbruster]

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!

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

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