Message ID | 20180111213250.16511-8-marcandre.lureau@redhat.com |
---|---|
State | New |
Headers | show |
Series | Hi, | expand |
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> Shouldn't this be squashed into the previous patch? > --- > 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() Suggest either "only after .check" or "in .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) > > 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) > 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 > 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) I figure other attributes that become valid only at .check() time should receive the same treatment. Not necessarily in this series, not necessarily by you. A TODO comment wouldn't hurt, though. I have only comment improvements, so: Reviewed-by: 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)
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) 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) 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 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)
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(-)