Re: [Qemu-devel] [PATCH v8 07/18] qapi: Don't use info as witness of implicit object type

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> A future patch will enable error reporting from the various
> QAPISchema*.check() methods.  But to report an error related
> to an implicit type, we'll need to associate a location with
> the type (the same location as the top-level entity that is
> causing the creation of the implicit type), and once we do
> that, keying off of whether foo.info exists is no longer a
> viable way to determine if foo is an implicit type.
>
> Instead, add an is_implicit() method to QAPISchemaEntity, with
> overrides for ObjectType and EnumType, and use that function
> where needed.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v8: separate isinstance back to verbose form, add comments
> v7: declare at the Entity level, with an optional argument for
> filtering by sub-class
> v6: split 11/46 into pieces; don't rename _info yet; rework atop
> nicer filtering mechanism, including no need to change visitor
> signature
> ---
>  scripts/qapi-types.py |  3 ++-
>  scripts/qapi-visit.py |  3 ++-
>  scripts/qapi.py       | 22 ++++++++++++++++++----
>  3 files changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 2a29c6e..4fe618e 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -235,7 +235,8 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>
>      def visit_needed(self, entity):
>          # Visit everything except implicit objects
> -        return not isinstance(entity, QAPISchemaObjectType) or entity.info
> +        return not (entity.is_implicit() and
> +                    isinstance(entity, QAPISchemaObjectType))
>
>      def _gen_type_cleanup(self, name):
>          self.decl += gen_type_cleanup_decl(name)
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 9fc79a7..2a9fab8 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -335,7 +335,8 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
>
>      def visit_needed(self, entity):
>          # Visit everything except implicit objects
> -        return not isinstance(entity, QAPISchemaObjectType) or entity.info
> +        return not (entity.is_implicit() and
> +                    isinstance(entity, QAPISchemaObjectType))
>
>      def visit_enum_type(self, name, info, values, prefix):
>          self.decl += gen_visit_decl(name, scalar=True)
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 68f97a1..e263ecf 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -798,6 +798,9 @@ class QAPISchemaEntity(object):
>      def check(self, schema):
>          pass
>
> +    def is_implicit(self):
> +        return not self.info
> +
>      def visit(self, visitor):
>          pass
>
> @@ -900,6 +903,10 @@ class QAPISchemaEnumType(QAPISchemaType):
>      def check(self, schema):
>          assert len(set(self.values)) == len(self.values)
>
> +    def is_implicit(self):
> +        # See QAPISchema._make_implicit_enum_type()
> +        return self.name[-4:] == 'Kind'
> +
>      def c_type(self, is_param=False):
>          return c_name(self.name)
>

I believe this method...

> @@ -970,12 +977,16 @@ class QAPISchemaObjectType(QAPISchemaType):
>              self.variants.check(schema, members, seen)
>          self.members = members
>
> +    def is_implicit(self):
> +        # See QAPISchema._make_implicit_object_type()
> +        return self.name[0] == ':'
> +

... as well as this one are redundant at this stage.  They become
necessary only when you start passing info != None to the constructor in
PATCH 12.  If I'm right, then moving the two to PATCH 12 makes sense.

>      def c_name(self):
> -        assert self.info
> +        assert not self.is_implicit()
>          return QAPISchemaType.c_name(self)
>
>      def c_type(self, is_param=False):
> -        assert self.info
> +        assert not self.is_implicit()
>          return QAPISchemaType.c_type(self)
>
>      def json_type(self):
> @@ -1043,7 +1054,8 @@ class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>      # This function exists to support ugly simple union special cases
>      # TODO get rid of them, and drop the function
>      def simple_union_type(self):
> -        if isinstance(self.type, QAPISchemaObjectType) and not self.type.info:
> +        if (self.type.is_implicit() and
> +                isinstance(self.type, QAPISchemaObjectType)):
>              assert len(self.type.members) == 1
>              assert not self.type.variants
>              return self.type.members[0].type
> @@ -1162,11 +1174,13 @@ class QAPISchema(object):
>          self._def_entity(self.the_empty_object_type)
>
>      def _make_implicit_enum_type(self, name, values):
> -        name = name + 'Kind'
> +        name = name + 'Kind'   # Use namespace reserved by add_name()

This is the comment I suggested...

>          self._def_entity(QAPISchemaEnumType(name, None, values, None))
>          return name
>
>      def _make_array_type(self, element_type):
> +        # TODO fooList namespace is not reserved; user can create collisions,
> +        # or abuse our type system with ['fooList'] for 2D array

... and this is its buddy you added on your own initiative.  Thanks!

Did you actually try the abuse?  If not, I'd say "or maybe abuse", out
of caution.

>          name = element_type + 'List'
>          if not self.lookup_type(name):
>              self._def_entity(QAPISchemaArrayType(name, None, element_type))