Re: [Qemu-devel] [PATCH v5 11/46] qapi: Don't use info as witness of implicit object type

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> A future patch will enable error reporting from the various
> check() methods.  But to report an error on 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.

Ensuring error messages are good even for implicit types could be hard.
But pretty much anything's better than error messages without location
information.

> Rename the info member to _info (so that sub-classes can still
> use it, but external code should not), add an is_implicit()
> method to QAPISchemaObjectType, and adjust the visitor to pass
> another parameter about whether the type is implicit.

I have doubts on the rename.

When you create an stable interface for use in other programs,
religiously hiding instance variables behind accessor methods can pay.
But in a purely internal interface like this one, I don't see the point.

If we run into a case where we want to use a QAPISchemaEntity's info, I
want to write .info and be done with it.  If we rename it to _info now,
we'll rename it back then.

So far, we've used the '_' prefix only for instance variables that are
clearly internal.  Mostly for stuff flowing from __init__() to check().

> Signed-off-by: Eric Blake <address@hidden>
> ---
>  scripts/qapi-types.py          |  4 ++--
>  scripts/qapi-visit.py          |  4 ++--
>  scripts/qapi.py                | 33 +++++++++++++++++++--------------
>  tests/qapi-schema/test-qapi.py |  2 +-
>  4 files changed, 24 insertions(+), 19 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index b292682..aa25e03 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -253,8 +253,8 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>              self.decl += gen_array(name, element_type)
>              self._gen_type_cleanup(name)
>
> -    def visit_object_type(self, name, info, base, members, variants):
> -        if info:
> +    def visit_object_type(self, name, info, base, members, variants, 
> implicit):

This is now right at the PEP8 line length limit, and the number of
parameters is getting unwieldy, too.  Hmm.

> +        if not implicit:
>              self._fwdecl += gen_fwd_object_or_array(name)
>              if variants:
>                  assert not members      # not implemented
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 1f287ba..62a47fa 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -348,8 +348,8 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
>              self.decl += decl
>              self.defn += defn
>
> -    def visit_object_type(self, name, info, base, members, variants):
> -        if info:
> +    def visit_object_type(self, name, info, base, members, variants, 
> implicit):
> +        if not implicit:
>              self.decl += gen_visit_decl(name)
>              if variants:
>                  assert not members      # not implemented
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 7275daa..1dc7641 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -786,7 +786,7 @@ class QAPISchemaEntity(object):
>      def __init__(self, name, info):
>          assert isinstance(name, str)
>          self.name = name
> -        self.info = info
> +        self._info = info
>
>      def c_name(self):
>          return c_name(self.name)
> @@ -814,7 +814,7 @@ class QAPISchemaVisitor(object):
>      def visit_array_type(self, name, info, element_type):
>          pass
>
> -    def visit_object_type(self, name, info, base, members, variants):
> +    def visit_object_type(self, name, info, base, members, variants, 
> implicit):
>          pass
>
>      def visit_object_type_flat(self, name, info, members, variants):
> @@ -877,7 +877,7 @@ class QAPISchemaBuiltinType(QAPISchemaType):
>          return self._json_type_name
>
>      def visit(self, visitor):
> -        visitor.visit_builtin_type(self.name, self.info, self.json_type())
> +        visitor.visit_builtin_type(self.name, self._info, self.json_type())
>
>
>  class QAPISchemaEnumType(QAPISchemaType):
> @@ -903,7 +903,7 @@ class QAPISchemaEnumType(QAPISchemaType):
>          return 'string'
>
>      def visit(self, visitor):
> -        visitor.visit_enum_type(self.name, self.info,
> +        visitor.visit_enum_type(self.name, self._info,
>                                  self.values, self.prefix)
>
>
> @@ -922,7 +922,7 @@ class QAPISchemaArrayType(QAPISchemaType):
>          return 'array'
>
>      def visit(self, visitor):
> -        visitor.visit_array_type(self.name, self.info, self.element_type)
> +        visitor.visit_array_type(self.name, self._info, self.element_type)
>
>
>  class QAPISchemaObjectType(QAPISchemaType):
> @@ -961,21 +961,25 @@ class QAPISchemaObjectType(QAPISchemaType):
>              self.variants.check(schema, members, seen)
>          self.members = members
>
> +    def is_implicit(self):
> +        return self.name[0] == ':'
> +

The predicate could be defined on any QAPISchemaType, or even any
QAPISchemaEntity, but right now we only ever want to test it for
objects.  Okay.

>      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):
>          return 'object'
>
>      def visit(self, visitor):
> -        visitor.visit_object_type(self.name, self.info,
> -                                  self.base, self.local_members, 
> self.variants)
> -        visitor.visit_object_type_flat(self.name, self.info,
> +        visitor.visit_object_type(self.name, self._info,
> +                                  self.base, self.local_members, 
> self.variants,
> +                                  self.is_implicit())
> +        visitor.visit_object_type_flat(self.name, self._info,
>                                         self.members, self.variants)
>
>
> @@ -1034,7 +1038,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 isinstance(self.type, QAPISchemaObjectType) and \
> +           self.type.is_implicit():
>              assert len(self.type.members) == 1
>              assert not self.type.variants
>              return self.type.members[0].type
> @@ -1055,7 +1060,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
>          return 'value'
>
>      def visit(self, visitor):
> -        visitor.visit_alternate_type(self.name, self.info, self.variants)
> +        visitor.visit_alternate_type(self.name, self._info, self.variants)
>
>
>  class QAPISchemaCommand(QAPISchemaEntity):
> @@ -1080,7 +1085,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
>              assert isinstance(self.ret_type, QAPISchemaType)
>
>      def visit(self, visitor):
> -        visitor.visit_command(self.name, self.info,
> +        visitor.visit_command(self.name, self._info,
>                                self.arg_type, self.ret_type,
>                                self.gen, self.success_response)
>
> @@ -1099,7 +1104,7 @@ class QAPISchemaEvent(QAPISchemaEntity):
>              assert not self.arg_type.variants   # not implemented
>
>      def visit(self, visitor):
> -        visitor.visit_event(self.name, self.info, self.arg_type)
> +        visitor.visit_event(self.name, self._info, self.arg_type)
>
>
>  class QAPISchema(object):
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index 649677e..f2cce64 100644
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -22,7 +22,7 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
>          if prefix:
>              print '    prefix %s' % prefix
>
> -    def visit_object_type(self, name, info, base, members, variants):
> +    def visit_object_type(self, name, info, base, members, variants, 
> implicit):
>          print 'object %s' % name
>          if base:
>              print '    base %s' % base.name

Three of our visitors implement visit_object_type():

* test-qapi.py doesn't care about implicit (implicitness is obvious
  enough from the name here).

* qapi-types.py and qapi-visit.py ignore implicit object types.  Hmm.

  qapi-introspect.py has a similar need: it wants to ignore *all* types.
  Two ways to ignore entities seem one too many.  Preexisting, but your
  patch makes it stand out a bit more.

  Could we reuse the existing mechanism somehow (and keep method
  visit_object_type() simple)?

  To reuse it without changes, we'd have to make implicit object types a
  separate class, so that QAPISchema.visit()'s isinstance() test can be
  put to work.

  Another option is generalizing QAPISchema's filter.  How?

  A third option is to abandon QAPISchema's filter, and make
  qapi-introspect.py filter in the visitor methods, just like we filter
  implicit objects.

Patch could be split into

A. Encapsulate the "is implicit" predicate in a method, i.e. replace
   not o.info by o.is_implicit().

B. Clean up how we filter out implicit objects.  May better go before A,
   not sure.

C. Rename .info to ._info.  Not sure we even want this part.