Second thoughts... Marc-André Lureau <marcandre.lur...@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 <arm...@redhat.com> > Signed-off-by: Marc-André Lureau <marcandre.lur...@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)