Re: [Qemu-devel] [PATCH v10 24/30] qapi: Factor out QAPISchemaObjectType.check_clash()

[Markus Armbruster]

Eric Blake  writes:

> On 11/10/2015 02:15 AM, Markus Armbruster wrote:
>
>>> On the other hand, we've been arguing that check() should populate
>>> everything after construction prior to anything else being run; and not
>>> running Variant.type.check() during Variants.check() of flat unions
>>> feels like we may have a hole (a flat union will have to inline its
>>> types to the overall JSON object, and inlining types requires access to
>>> type.members - but as written, we aren't populating them until
>>> Variants.check_clash()).  I can play with hoisting the type.check() out
>>> of type.check_clash() and instead keep base.check() in type.check(), and
>>> add variant.type.check() in Variants.check() (but only for unions, not
>>> for alternates), if you are interested.
>> 
>> My "qapi: Factor out QAPISchemaObjectTypeMember.check_clash()" adds
>> QAPISchemaObjectTypeMember.check_clash() without changing the common
>> protocol.  The new QAPISchemaObjectTypeMember.check_clash() is merely a
>> helper for QAPISchemaObjectType.check().
>> 
>> The two .check_clash() you add (one in this patch, one in the previous
>> one) are different: both contain calls of QAPISchemaObjectType.check().
>> 
>> I feel the .check() calls are too important to be buried deep like that.
>> I'd stick to prior practice and put the .check() calls right into
>> .check().  Obviously, the .check_clash() methods may only called after
>> .check() then, but that's nothing new.
>> 
>> Fixup for your previous patch:
>> 
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index 4c56935..357127d 100644
>> --- a/scripts/qapi.py
>> +++ b/scripts/qapi.py
>> @@ -1065,7 +1065,6 @@ class QAPISchemaObjectTypeVariants(object):
>>  vseen = dict(seen)
>>  assert isinstance(v.type, QAPISchemaObjectType)
>>  assert not v.type.variants   # not implemented
>> -v.type.check(schema)
>>  for m in v.type.members:
>>  m.check_clash(vseen)
>>  
>> @@ -1077,6 +1076,7 @@ class 
>> QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>>  def check(self, schema, tag_type):
>>  QAPISchemaObjectTypeMember.check(self, schema)
>>  assert self.name in tag_type.values
>> +self.type.check(schema)
>>  
>
> Won't quite work.  You are right that we must call
> self.type.check(schema) for variants used by a union; but calling it for
> ALL variants used by an alternate is wrong, because self.type for at
> least one branch of an alternate will not be an instance of
> QAPISchemaObjectType.  However, I'm currently testing whether it is safe
> to check to just blindly check an object branch of an alternate, if
> present (and that should not lead to cycles, since alternates have no
> base class and since we don't allow one alternate type as a variant of
> another alternate), in which case the fixup for 23/30 is more like:
>
> diff --git i/scripts/qapi.py w/scripts/qapi.py
> index a005c87..25fa642 100644
> --- i/scripts/qapi.py
> +++ w/scripts/qapi.py
> @@ -1065,7 +1065,6 @@ class QAPISchemaObjectTypeVariants(object):
>  vseen = dict(seen)
>  assert isinstance(v.type, QAPISchemaObjectType)
>  assert not v.type.variants   # not implemented
> -v.type.check(schema)
>  for m in v.type.members:
>  m.check_clash(vseen)
>
> @@ -1077,6 +1076,8 @@ class
> QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>  def check(self, schema, tag_type):
>  QAPISchemaObjectTypeMember.check(self, schema)
>  assert self.name in tag_type.values
> +if isinstance(self.type, QAPISchemaObjectType):
> +self.type.check(schema)
>
>  # This function exists to support ugly simple union special cases
>  # TODO get rid of them, and drop the function
> @@ -1098,6 +1099,8 @@ class QAPISchemaAlternateType(QAPISchemaType):
>
>  def check(self, schema):
>  self.variants.tag_member.check(schema)
> +# Not calling self.variants.check_clash(), because there's
> +# nothing to clash with
>  self.variants.check(schema, {})
>
>  def json_type(self):

Makes sense to me.

Re: [Qemu-devel] [PATCH v10 24/30] qapi: Factor out QAPISchemaObjectType.check_clash()

[Markus Armbruster]

Eric Blake  writes:

> On 11/09/2015 07:49 AM, Markus Armbruster wrote:
>> Eric Blake  writes:
>> 
>>> Consolidate two common sequences of clash detection into a
>>> new QAPISchemaObjectType.check_clash() helper method.
>>>
>>> No change to generated code.
>>>
>>> Signed-off-by: Eric Blake 
>>>
>
>>> @@ -980,11 +980,7 @@ class QAPISchemaObjectType(QAPISchemaType):
>>>  seen = OrderedDict()
>>>  if self._base_name:
>>>  self.base = schema.lookup_type(self._base_name)
>>> -assert isinstance(self.base, QAPISchemaObjectType)
>>> -assert not self.base.variants   # not implemented
>>> -self.base.check(schema)
>>> -for m in self.base.members:
>>> -m.check_clash(seen)
>>> +self.base.check_clash(schema, seen)
>>>  for m in self.local_members:
>>>  m.check(schema)
>>>  m.check_clash(seen)
>>> @@ -994,6 +990,12 @@ class QAPISchemaObjectType(QAPISchemaType):
>>>  assert self.variants.tag_member in self.members
>>>  self.variants.check_clash(schema, seen)
>>>
>>> +def check_clash(self, schema, seen):
>>> +self.check(schema)
>> 
>> Do we want to hide this .check() inside .check_clash()?
>> 
>> QAPISchemaObjectTypeMember.check() doesn't.  I think the two better
>> behave the same.
>> 
>>> +assert not self.variants   # not implemented
>>> +for m in self.members:
>>> +m.check_clash(seen)
>
> The self.check(schema) call is necessary to get self.members populated.
>  We cannot iterate over self.members if the type has not had check()
> called; this is true for both callers of type.check_clash()
> (ObjectType.check(), and Variants.check_clash()).

Yes.

We have a common protocol for QAPISchemaFOO objects, namely that certain
instance variables and methods are only valid after .check().

> You are correct that neither Member.check() nor Member.check_clash()
> call a form of type.check() - but that's because at that level, there is
> no need to populate a type.members list.
>
> On the other hand, we've been arguing that check() should populate
> everything after construction prior to anything else being run; and not
> running Variant.type.check() during Variants.check() of flat unions
> feels like we may have a hole (a flat union will have to inline its
> types to the overall JSON object, and inlining types requires access to
> type.members - but as written, we aren't populating them until
> Variants.check_clash()).  I can play with hoisting the type.check() out
> of type.check_clash() and instead keep base.check() in type.check(), and
> add variant.type.check() in Variants.check() (but only for unions, not
> for alternates), if you are interested.

My "qapi: Factor out QAPISchemaObjectTypeMember.check_clash()" added
QAPISchemaObjectTypeMember.check_clash() without changing the common
protocol.  The new QAPISchemaObjectTypeMember.check_clash() is merely a
helper for QAPISchemaObjectType.check().

Your 
Gcc: nnml:mail.redhat.xlst.qemu-devel
From: Markus Armbruster 
--text follows this line--
Eric Blake  writes:

> On 11/09/2015 07:49 AM, Markus Armbruster wrote:
>> Eric Blake  writes:
>> 
>>> Consolidate two common sequences of clash detection into a
>>> new QAPISchemaObjectType.check_clash() helper method.
>>>
>>> No change to generated code.
>>>
>>> Signed-off-by: Eric Blake 
>>>
>
>>> @@ -980,11 +980,7 @@ class QAPISchemaObjectType(QAPISchemaType):
>>>  seen = OrderedDict()
>>>  if self._base_name:
>>>  self.base = schema.lookup_type(self._base_name)
>>> -assert isinstance(self.base, QAPISchemaObjectType)
>>> -assert not self.base.variants   # not implemented
>>> -self.base.check(schema)
>>> -for m in self.base.members:
>>> -m.check_clash(seen)
>>> +self.base.check_clash(schema, seen)
>>>  for m in self.local_members:
>>>  m.check(schema)
>>>  m.check_clash(seen)
>>> @@ -994,6 +990,12 @@ class QAPISchemaObjectType(QAPISchemaType):
>>>  assert self.variants.tag_member in self.members
>>>  self.variants.check_clash(schema, seen)
>>>
>>> +def check_clash(self, schema, seen):
>>> +self.check(schema)
>> 
>> Do we want to hide this .check() inside .check_clash()?
>> 
>> QAPISchemaObjectTypeMember.check() doesn't.  I think the two better
>> behave the same.
>> 
>>> +assert not self.variants   # not implemented
>>> +for m in self.members:
>>> +m.check_clash(seen)
>
> The self.check(schema) call is necessary to get self.members populated.
>  We cannot iterate over self.members if the type has not had check()
> called; this is true for both callers of type.check_clash()
> (ObjectType.check(), and 

Re: [Qemu-devel] [PATCH v10 24/30] qapi: Factor out QAPISchemaObjectType.check_clash()

[Eric Blake]

On 11/10/2015 02:15 AM, Markus Armbruster wrote:

>> On the other hand, we've been arguing that check() should populate
>> everything after construction prior to anything else being run; and not
>> running Variant.type.check() during Variants.check() of flat unions
>> feels like we may have a hole (a flat union will have to inline its
>> types to the overall JSON object, and inlining types requires access to
>> type.members - but as written, we aren't populating them until
>> Variants.check_clash()).  I can play with hoisting the type.check() out
>> of type.check_clash() and instead keep base.check() in type.check(), and
>> add variant.type.check() in Variants.check() (but only for unions, not
>> for alternates), if you are interested.
> 
> My "qapi: Factor out QAPISchemaObjectTypeMember.check_clash()" adds
> QAPISchemaObjectTypeMember.check_clash() without changing the common
> protocol.  The new QAPISchemaObjectTypeMember.check_clash() is merely a
> helper for QAPISchemaObjectType.check().
> 
> The two .check_clash() you add (one in this patch, one in the previous
> one) are different: both contain calls of QAPISchemaObjectType.check().
> 
> I feel the .check() calls are too important to be buried deep like that.
> I'd stick to prior practice and put the .check() calls right into
> .check().  Obviously, the .check_clash() methods may only called after
> .check() then, but that's nothing new.
> 
> Fixup for your previous patch:
> 
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 4c56935..357127d 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1065,7 +1065,6 @@ class QAPISchemaObjectTypeVariants(object):
>  vseen = dict(seen)
>  assert isinstance(v.type, QAPISchemaObjectType)
>  assert not v.type.variants   # not implemented
> -v.type.check(schema)
>  for m in v.type.members:
>  m.check_clash(vseen)
>  
> @@ -1077,6 +1076,7 @@ class 
> QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>  def check(self, schema, tag_type):
>  QAPISchemaObjectTypeMember.check(self, schema)
>  assert self.name in tag_type.values
> +self.type.check(schema)
>  

Won't quite work.  You are right that we must call
self.type.check(schema) for variants used by a union; but calling it for
ALL variants used by an alternate is wrong, because self.type for at
least one branch of an alternate will not be an instance of
QAPISchemaObjectType.  However, I'm currently testing whether it is safe
to check to just blindly check an object branch of an alternate, if
present (and that should not lead to cycles, since alternates have no
base class and since we don't allow one alternate type as a variant of
another alternate), in which case the fixup for 23/30 is more like:

diff --git i/scripts/qapi.py w/scripts/qapi.py
index a005c87..25fa642 100644
--- i/scripts/qapi.py
+++ w/scripts/qapi.py
@@ -1065,7 +1065,6 @@ class QAPISchemaObjectTypeVariants(object):
 vseen = dict(seen)
 assert isinstance(v.type, QAPISchemaObjectType)
 assert not v.type.variants   # not implemented
-v.type.check(schema)
 for m in v.type.members:
 m.check_clash(vseen)

@@ -1077,6 +1076,8 @@ class
QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
 def check(self, schema, tag_type):
 QAPISchemaObjectTypeMember.check(self, schema)
 assert self.name in tag_type.values
+if isinstance(self.type, QAPISchemaObjectType):
+self.type.check(schema)

 # This function exists to support ugly simple union special cases
 # TODO get rid of them, and drop the function
@@ -1098,6 +1099,8 @@ class QAPISchemaAlternateType(QAPISchemaType):

 def check(self, schema):
 self.variants.tag_member.check(schema)
+# Not calling self.variants.check_clash(), because there's
+# nothing to clash with
 self.variants.check(schema, {})

 def json_type(self):




-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v10 24/30] qapi: Factor out QAPISchemaObjectType.check_clash()

[Markus Armbruster]

Eric Blake  writes:

> Consolidate two common sequences of clash detection into a
> new QAPISchemaObjectType.check_clash() helper method.
>
> No change to generated code.
>
> Signed-off-by: Eric Blake 
>
> ---
> v10: rebase on new Variants.check_clash()
> v9: new patch, split off from v8 7/17
> ---
>  scripts/qapi.py | 19 ---
>  1 file changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 4c56935..6d8c4c7 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -980,11 +980,7 @@ class QAPISchemaObjectType(QAPISchemaType):
>  seen = OrderedDict()
>  if self._base_name:
>  self.base = schema.lookup_type(self._base_name)
> -assert isinstance(self.base, QAPISchemaObjectType)
> -assert not self.base.variants   # not implemented
> -self.base.check(schema)
> -for m in self.base.members:
> -m.check_clash(seen)
> +self.base.check_clash(schema, seen)
>  for m in self.local_members:
>  m.check(schema)
>  m.check_clash(seen)
> @@ -994,6 +990,12 @@ class QAPISchemaObjectType(QAPISchemaType):
>  assert self.variants.tag_member in self.members
>  self.variants.check_clash(schema, seen)
>
> +def check_clash(self, schema, seen):
> +self.check(schema)

Do we want to hide this .check() inside .check_clash()?

QAPISchemaObjectTypeMember.check() doesn't.  I think the two better
behave the same.

> +assert not self.variants   # not implemented
> +for m in self.members:
> +m.check_clash(seen)
> +
>  def is_implicit(self):
>  # See QAPISchema._make_implicit_object_type()
>  return self.name[0] == ':'
> @@ -1062,12 +1064,7 @@ class QAPISchemaObjectTypeVariants(object):
>  for v in self.variants:
>  # Reset seen map for each variant, since qapi names from one
>  # branch do not affect another branch
> -vseen = dict(seen)
> -assert isinstance(v.type, QAPISchemaObjectType)
> -assert not v.type.variants   # not implemented
> -v.type.check(schema)
> -for m in v.type.members:
> -m.check_clash(vseen)
> +v.type.check_clash(schema, dict(seen))
>
>
>  class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):

Re: [Qemu-devel] [PATCH v10 24/30] qapi: Factor out QAPISchemaObjectType.check_clash()

[Markus Armbruster]

Eric Blake  writes:

> Consolidate two common sequences of clash detection into a
> new QAPISchemaObjectType.check_clash() helper method.
>
> No change to generated code.
>
> Signed-off-by: Eric Blake 
>
> ---
> v10: rebase on new Variants.check_clash()
> v9: new patch, split off from v8 7/17
> ---
>  scripts/qapi.py | 19 ---
>  1 file changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 4c56935..6d8c4c7 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -980,11 +980,7 @@ class QAPISchemaObjectType(QAPISchemaType):
>  seen = OrderedDict()
>  if self._base_name:
>  self.base = schema.lookup_type(self._base_name)
> -assert isinstance(self.base, QAPISchemaObjectType)

This assertion is lost.

> -assert not self.base.variants   # not implemented
> -self.base.check(schema)
> -for m in self.base.members:
> -m.check_clash(seen)
> +self.base.check_clash(schema, seen)
>  for m in self.local_members:
>  m.check(schema)
>  m.check_clash(seen)
> @@ -994,6 +990,12 @@ class QAPISchemaObjectType(QAPISchemaType):
>  assert self.variants.tag_member in self.members
>  self.variants.check_clash(schema, seen)
>
> +def check_clash(self, schema, seen):
> +self.check(schema)
> +assert not self.variants   # not implemented
> +for m in self.members:
> +m.check_clash(seen)
> +
>  def is_implicit(self):
>  # See QAPISchema._make_implicit_object_type()
>  return self.name[0] == ':'
> @@ -1062,12 +1064,7 @@ class QAPISchemaObjectTypeVariants(object):
>  for v in self.variants:
>  # Reset seen map for each variant, since qapi names from one
>  # branch do not affect another branch
> -vseen = dict(seen)
> -assert isinstance(v.type, QAPISchemaObjectType)

This assertion is lost.

> -assert not v.type.variants   # not implemented
> -v.type.check(schema)
> -for m in v.type.members:
> -m.check_clash(vseen)
> +v.type.check_clash(schema, dict(seen))
>
>
>  class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):

Re: [Qemu-devel] [PATCH v10 24/30] qapi: Factor out QAPISchemaObjectType.check_clash()

[Eric Blake]

On 11/09/2015 12:11 PM, Markus Armbruster wrote:
> Eric Blake  writes:
> 
>> On 11/09/2015 06:00 AM, Markus Armbruster wrote:
>>> Eric Blake  writes:
>>>
 Consolidate two common sequences of clash detection into a
 new QAPISchemaObjectType.check_clash() helper method.

 No change to generated code.

 Signed-off-by: Eric Blake 

 @@ -980,11 +980,7 @@ class QAPISchemaObjectType(QAPISchemaType):
  seen = OrderedDict()
  if self._base_name:
  self.base = schema.lookup_type(self._base_name)
 -assert isinstance(self.base, QAPISchemaObjectType)
>>>
>>> This assertion is lost.
>>>

>>
>> Directly lost, but indirectly still present.  The new code is calling
>> QAPISchemaObjectType.check_clash(), which won't exist unless self.base
>> is a QAPISchemaObjectType.
> 
> or a QAPISchemaObjectTypeMember, or a QAPISchemaObjectVariants, or
> whatever else acquires the method in the future.
> 

> 
> Maybe, but the isinstance(self.base, QAPISchemaObjectType) will come
> right back anyway when we move the "'base' for FOO cannot use BAR type"
> check from the old semantic analysis into the check methods.  Until
> then, it makes sense at least as a place holder.

Good point, I'll add it back in.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v10 24/30] qapi: Factor out QAPISchemaObjectType.check_clash()

[Eric Blake]

On 11/09/2015 07:49 AM, Markus Armbruster wrote:
> Eric Blake  writes:
> 
>> Consolidate two common sequences of clash detection into a
>> new QAPISchemaObjectType.check_clash() helper method.
>>
>> No change to generated code.
>>
>> Signed-off-by: Eric Blake 
>>

>> @@ -980,11 +980,7 @@ class QAPISchemaObjectType(QAPISchemaType):
>>  seen = OrderedDict()
>>  if self._base_name:
>>  self.base = schema.lookup_type(self._base_name)
>> -assert isinstance(self.base, QAPISchemaObjectType)
>> -assert not self.base.variants   # not implemented
>> -self.base.check(schema)
>> -for m in self.base.members:
>> -m.check_clash(seen)
>> +self.base.check_clash(schema, seen)
>>  for m in self.local_members:
>>  m.check(schema)
>>  m.check_clash(seen)
>> @@ -994,6 +990,12 @@ class QAPISchemaObjectType(QAPISchemaType):
>>  assert self.variants.tag_member in self.members
>>  self.variants.check_clash(schema, seen)
>>
>> +def check_clash(self, schema, seen):
>> +self.check(schema)
> 
> Do we want to hide this .check() inside .check_clash()?
> 
> QAPISchemaObjectTypeMember.check() doesn't.  I think the two better
> behave the same.
> 
>> +assert not self.variants   # not implemented
>> +for m in self.members:
>> +m.check_clash(seen)

The self.check(schema) call is necessary to get self.members populated.
 We cannot iterate over self.members if the type has not had check()
called; this is true for both callers of type.check_clash()
(ObjectType.check(), and Variants.check_clash()).

You are correct that neither Member.check() nor Member.check_clash()
call a form of type.check() - but that's because at that level, there is
no need to populate a type.members list.

On the other hand, we've been arguing that check() should populate
everything after construction prior to anything else being run; and not
running Variant.type.check() during Variants.check() of flat unions
feels like we may have a hole (a flat union will have to inline its
types to the overall JSON object, and inlining types requires access to
type.members - but as written, we aren't populating them until
Variants.check_clash()).  I can play with hoisting the type.check() out
of type.check_clash() and instead keep base.check() in type.check(), and
add variant.type.check() in Variants.check() (but only for unions, not
for alternates), if you are interested.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v10 24/30] qapi: Factor out QAPISchemaObjectType.check_clash()

[Eric Blake]

On 11/09/2015 06:00 AM, Markus Armbruster wrote:
> Eric Blake  writes:
> 
>> Consolidate two common sequences of clash detection into a
>> new QAPISchemaObjectType.check_clash() helper method.
>>
>> No change to generated code.
>>
>> Signed-off-by: Eric Blake 
>>
>> ---

>> @@ -980,11 +980,7 @@ class QAPISchemaObjectType(QAPISchemaType):
>>  seen = OrderedDict()
>>  if self._base_name:
>>  self.base = schema.lookup_type(self._base_name)
>> -assert isinstance(self.base, QAPISchemaObjectType)
> 
> This assertion is lost.
> 
>> -assert not self.base.variants   # not implemented
>> -self.base.check(schema)
>> -for m in self.base.members:
>> -m.check_clash(seen)
>> +self.base.check_clash(schema, seen)

Directly lost, but indirectly still present.  The new code is calling
QAPISchemaObjectType.check_clash(), which won't exist unless self.base
is a QAPISchemaObjectType.  Folding the assert into the refactored
function makes no sense (the condition isinstance(self,
QAPISchemaObjectType) would always be true), and leaving the assert
prior to calling self.base.check_clash() adds no real protection against
programming bugs.

>> @@ -1062,12 +1064,7 @@ class QAPISchemaObjectTypeVariants(object):
>>  for v in self.variants:
>>  # Reset seen map for each variant, since qapi names from one
>>  # branch do not affect another branch
>> -vseen = dict(seen)
>> -assert isinstance(v.type, QAPISchemaObjectType)
> 
> This assertion is lost.
> 
>> -assert not v.type.variants   # not implemented
>> -v.type.check(schema)
>> -for m in v.type.members:
>> -m.check_clash(vseen)
>> +v.type.check_clash(schema, dict(seen))

Same explanation.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v10 24/30] qapi: Factor out QAPISchemaObjectType.check_clash()

[Markus Armbruster]

Eric Blake  writes:

> On 11/09/2015 06:00 AM, Markus Armbruster wrote:
>> Eric Blake  writes:
>> 
>>> Consolidate two common sequences of clash detection into a
>>> new QAPISchemaObjectType.check_clash() helper method.
>>>
>>> No change to generated code.
>>>
>>> Signed-off-by: Eric Blake 
>>>
>>> ---
>
>>> @@ -980,11 +980,7 @@ class QAPISchemaObjectType(QAPISchemaType):
>>>  seen = OrderedDict()
>>>  if self._base_name:
>>>  self.base = schema.lookup_type(self._base_name)
>>> -assert isinstance(self.base, QAPISchemaObjectType)
>> 
>> This assertion is lost.
>> 
>>> -assert not self.base.variants   # not implemented
>>> -self.base.check(schema)
>>> -for m in self.base.members:
>>> -m.check_clash(seen)
>>> +self.base.check_clash(schema, seen)
>
> Directly lost, but indirectly still present.  The new code is calling
> QAPISchemaObjectType.check_clash(), which won't exist unless self.base
> is a QAPISchemaObjectType.

or a QAPISchemaObjectTypeMember, or a QAPISchemaObjectVariants, or
whatever else acquires the method in the future.

> Folding the assert into the refactored
> function makes no sense (the condition isinstance(self,
> QAPISchemaObjectType) would always be true),

Correct.

>  and leaving the assert
> prior to calling self.base.check_clash() adds no real protection against
> programming bugs.

Maybe, but the isinstance(self.base, QAPISchemaObjectType) will come
right back anyway when we move the "'base' for FOO cannot use BAR type"
check from the old semantic analysis into the check methods.  Until
then, it makes sense at least as a place holder.

>>> @@ -1062,12 +1064,7 @@ class QAPISchemaObjectTypeVariants(object):
>>>  for v in self.variants:
>>>  # Reset seen map for each variant, since qapi names from one
>>>  # branch do not affect another branch
>>> -vseen = dict(seen)
>>> -assert isinstance(v.type, QAPISchemaObjectType)
>> 
>> This assertion is lost.
>> 
>>> -assert not v.type.variants   # not implemented
>>> -v.type.check(schema)
>>> -for m in v.type.members:
>>> -m.check_clash(vseen)
>>> +v.type.check_clash(schema, dict(seen))
>
> Same explanation.