| Message ID | 20180321115211.17937-6-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: add #if pre-processor conditions to generated code | 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(). Drawback: pylint complains. We'll live. > > Suggested-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > Reviewed-by: Markus Armbruster <armbru@redhat.com> Shouldn't this be squashed into the previous patch? > --- > scripts/qapi/common.py | 21 +++++++++++++++++---- > 1 file changed, 17 insertions(+), 4 deletions(-) > > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py > index d8ab3d8f7f..eb07d641ab 100644 > --- a/scripts/qapi/common.py > +++ b/scripts/qapi/common.py > @@ -1026,13 +1026,19 @@ class QAPISchemaEntity(object): > # such place). > self.info = info > self.doc = doc > - self.ifcond = listify_cond(ifcond) > + self._ifcond = ifcond # self.ifcond is set only 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 > @@ -1169,6 +1175,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) > @@ -1201,8 +1208,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): > @@ -1245,6 +1254,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) > @@ -1427,6 +1437,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 > @@ -1470,6 +1481,7 @@ class QAPISchemaCommand(QAPISchemaEntity): > self.allow_oob = allow_oob > > 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 > @@ -1504,6 +1516,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 > @@ -1633,7 +1646,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:1649,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)) > @@ -1679,7 +1692,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) Perhaps 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.
Hi On Tue, Jun 19, 2018 at 11:06 AM, Markus Armbruster <armbru@redhat.com> wrote: > 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(). > > Drawback: pylint complains. We'll live. > >> >> Suggested-by: Markus Armbruster <armbru@redhat.com> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> Reviewed-by: Markus Armbruster <armbru@redhat.com> > > Shouldn't this be squashed into the previous patch? I would rather keep it seperate, as it makes reviewing both a bit easier to me. But feel free to squash on commit. > >> --- >> scripts/qapi/common.py | 21 +++++++++++++++++---- >> 1 file changed, 17 insertions(+), 4 deletions(-) >> >> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py >> index d8ab3d8f7f..eb07d641ab 100644 >> --- a/scripts/qapi/common.py >> +++ b/scripts/qapi/common.py >> @@ -1026,13 +1026,19 @@ class QAPISchemaEntity(object): >> # such place). >> self.info = info >> self.doc = doc >> - self.ifcond = listify_cond(ifcond) >> + self._ifcond = ifcond # self.ifcond is set only 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? There is also implicit object types. > >> >> def is_implicit(self): >> return not self.info >> @@ -1169,6 +1175,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) >> @@ -1201,8 +1208,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): >> @@ -1245,6 +1254,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) >> @@ -1427,6 +1437,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 >> @@ -1470,6 +1481,7 @@ class QAPISchemaCommand(QAPISchemaEntity): >> self.allow_oob = allow_oob >> >> 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 >> @@ -1504,6 +1516,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 >> @@ -1633,7 +1646,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:1649,29: Access to a protected member _ifcond of a client class (protected-access) > > Layering violation. Tolerable, I think. > yeah, alternative would be to add an assert_ifcond() method in type..? I'll add a # pylint: disable=protected-access for now >> self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond, >> None, members, None)) >> @@ -1679,7 +1692,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) > > Perhaps 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. > It doesn't look obvious to me which should receive the same treatement. I'll leave that to you to figure out :)
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > Hi > > On Tue, Jun 19, 2018 at 11:06 AM, Markus Armbruster <armbru@redhat.com> wrote: >> 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(). >> >> Drawback: pylint complains. We'll live. >> >>> >>> Suggested-by: Markus Armbruster <armbru@redhat.com> >>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>> Reviewed-by: Markus Armbruster <armbru@redhat.com> >> >> Shouldn't this be squashed into the previous patch? > > I would rather keep it seperate, as it makes reviewing both a bit > easier to me. But feel free to squash on commit. No need to decide right now. >> >>> --- >>> scripts/qapi/common.py | 21 +++++++++++++++++---- >>> 1 file changed, 17 insertions(+), 4 deletions(-) >>> >>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py >>> index d8ab3d8f7f..eb07d641ab 100644 >>> --- a/scripts/qapi/common.py >>> +++ b/scripts/qapi/common.py >>> @@ -1026,13 +1026,19 @@ class QAPISchemaEntity(object): >>> # such place). >>> self.info = info >>> self.doc = doc >>> - self.ifcond = listify_cond(ifcond) >>> + self._ifcond = ifcond # self.ifcond is set only 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? > > There is also implicit object types. Can you give an example? >>> >>> def is_implicit(self): >>> return not self.info >>> @@ -1169,6 +1175,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) >>> @@ -1201,8 +1208,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): >>> @@ -1245,6 +1254,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) >>> @@ -1427,6 +1437,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 >>> @@ -1470,6 +1481,7 @@ class QAPISchemaCommand(QAPISchemaEntity): >>> self.allow_oob = allow_oob >>> >>> 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 >>> @@ -1504,6 +1516,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 >>> @@ -1633,7 +1646,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:1649,29: Access to a protected member _ifcond of a client class (protected-access) >> >> Layering violation. Tolerable, I think. >> > > yeah, alternative would be to add an assert_ifcond() method in type..? > I'll add a # pylint: disable=protected-access for now Wortwhile only if we make an effort to clean up or suppress the other pylint gripes. I'll look into it. Go ahead and add the directive meanwhile; easily dropped it if we decide we don't want it. >>> self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond, >>> None, members, None)) >>> @@ -1679,7 +1692,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) >> >> Perhaps 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. >> > > It doesn't look obvious to me which should receive the same > treatement. I'll leave that to you to figure out :) Fair enough.
Hi On Wed, Jun 27, 2018 at 7:26 AM, Markus Armbruster <armbru@redhat.com> wrote: >>> 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? >> >> There is also implicit object types. > > Can you give an example? > { 'union': 'Foo', 'data': { 'foo': 'Test' } } will create implicit QAPISchemaObjectType q_obj_Test-wrapper in _make_simple_variant() This happens before check(), so we pass the Test type as ifcond.
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index d8ab3d8f7f..eb07d641ab 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -1026,13 +1026,19 @@ class QAPISchemaEntity(object): # such place). self.info = info self.doc = doc - self.ifcond = listify_cond(ifcond) + self._ifcond = ifcond # self.ifcond is set only 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 @@ -1169,6 +1175,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) @@ -1201,8 +1208,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): @@ -1245,6 +1254,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) @@ -1427,6 +1437,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 @@ -1470,6 +1481,7 @@ class QAPISchemaCommand(QAPISchemaEntity): self.allow_oob = allow_oob 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 @@ -1504,6 +1516,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 @@ -1633,7 +1646,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)) @@ -1679,7 +1692,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)