Re: [Qemu-devel] [PATCH v4 07/51] qapi: leave the ifcond attribute undefined until check()

[Markus Armbruster <armbru@redhat.com>]

Second thoughts...

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> We commonly initialize attributes to None in .init(), then set their
> real value in .check().  Accessing the attribute before .check()
> yields None.  If we're lucky, the code that accesses the attribute
> prematurely chokes on None.
>
> It won't for .ifcond, because None is a legitimate value.
>
> Leave the ifcond attribute undefined until check().
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  scripts/qapi.py | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 8f54dead8d..4d2c214f19 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1011,13 +1011,19 @@ class QAPISchemaEntity(object):
>          # such place).
>          self.info = info
>          self.doc = doc
> -        self.ifcond = listify_cond(ifcond)
> +        self._ifcond = ifcond  # self.ifcond is set after check()
>  
>      def c_name(self):
>          return c_name(self.name)
>  
>      def check(self, schema):
> -        pass
> +        if isinstance(self._ifcond, QAPISchemaType):
> +            # inherit the condition from a type
> +            typ = self._ifcond
> +            typ.check(schema)
> +            self.ifcond = typ.ifcond
> +        else:
> +            self.ifcond = listify_cond(self._ifcond)

Whenever we add a .check(), we need to prove QAPISchema.check()'s
recursion still terminates, and terminates the right way.

Argument before this patch: we recurse only into types contained in
types, e.g. an object type's base type, and we detect and report cycles
as "Object %s contains itself", in QAPISchemaObjectType.check().

The .check() added here recurses into a type.  If this creates a cycle,
it'll be caught and reported as "contains itself".  We still need to
show that the error message remains accurate.

We .check() here to inherit .ifcond from a type.  As far as I can tell,
we use this inheritance feature only to inherit an array's condition
from its element type.  That's safe, because an array does contain its
elements.

This is hardly a rigorous proof.  Just enough to make me believe your
code works.

However, I suspect adding the inheritance feature at the entity level
complicates the correctness argument without real need.  Can we restrict
it to array elements?  Have QAPISchemaArrayType.check() resolve
type-valued ._ifcond, and all the others choke on it?

>  
>      def is_implicit(self):
>          return not self.info
> @@ -1138,6 +1144,7 @@ class QAPISchemaEnumType(QAPISchemaType):
>          self.prefix = prefix
>  
>      def check(self, schema):
> +        QAPISchemaType.check(self, schema)
>          seen = {}
>          for v in self.values:
>              v.check_clash(self.info, seen)
> @@ -1170,8 +1177,10 @@ class QAPISchemaArrayType(QAPISchemaType):
>          self.element_type = None
>  
>      def check(self, schema):
> +        QAPISchemaType.check(self, schema)
>          self.element_type = schema.lookup_type(self._element_type_name)
>          assert self.element_type
> +        self.element_type.check(schema)

This .check is locally obvious: an array contains its elements.

>          self.ifcond = self.element_type.ifcond
>  
>      def is_implicit(self):
> @@ -1214,6 +1223,7 @@ class QAPISchemaObjectType(QAPISchemaType):
>          self.members = None
>  
>      def check(self, schema):
> +        QAPISchemaType.check(self, schema)
>          if self.members is False:               # check for cycles
>              raise QAPISemError(self.info,
>                                 "Object %s contains itself" % self.name)
> @@ -1396,6 +1406,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
>          self.variants = variants
>  
>      def check(self, schema):
> +        QAPISchemaType.check(self, schema)
>          self.variants.tag_member.check(schema)
>          # Not calling self.variants.check_clash(), because there's nothing
>          # to clash with
> @@ -1438,6 +1449,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
>          self.boxed = boxed
>  
>      def check(self, schema):
> +        QAPISchemaEntity.check(self, schema)
>          if self._arg_type_name:
>              self.arg_type = schema.lookup_type(self._arg_type_name)
>              assert (isinstance(self.arg_type, QAPISchemaObjectType) or
> @@ -1471,6 +1483,7 @@ class QAPISchemaEvent(QAPISchemaEntity):
>          self.boxed = boxed
>  
>      def check(self, schema):
> +        QAPISchemaEntity.check(self, schema)
>          if self._arg_type_name:
>              self.arg_type = schema.lookup_type(self._arg_type_name)
>              assert (isinstance(self.arg_type, QAPISchemaObjectType) or
> @@ -1589,7 +1602,7 @@ class QAPISchema(object):
>              # But it's not tight: the disjunction need not imply it.  We
>              # may end up compiling useless wrapper types.
>              # TODO kill simple unions or implement the disjunction
> -            assert ifcond == typ.ifcond
> +            assert ifcond == typ._ifcond

pylint complains

    W:1605,29: Access to a protected member _ifcond of a client class (protected-access)

Layering violation.  Tolerable, I think.

>          else:
>              self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond,
>                                                    None, members, None))
> @@ -1635,7 +1648,7 @@ class QAPISchema(object):
>              assert len(typ) == 1
>              typ = self._make_array_type(typ[0], info)
>          typ = self._make_implicit_object_type(
> -            typ, info, None, self.lookup_type(typ).ifcond,
> +            typ, info, None, self.lookup_type(typ),
>              'wrapper', [self._make_member('data', typ, info)])
>          return QAPISchemaObjectTypeVariant(case, typ)