Marc-André Lureau <marcandre.lur...@gmail.com> writes:
> Hi
>
> On Tue, Jun 19, 2018 at 11:06 AM, Markus Armbruster <arm...@redhat.com> wrote:
>> 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().
>>
>> Drawback: pylint complains. We'll live.
>>
>>>
>>> Suggested-by: Markus Armbruster <arm...@redhat.com>
>>> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
>>> Reviewed-by: Markus Armbruster <arm...@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.
