[v4,01/10] qapi: Assert in places where variants are not handled

Commit Message

Eric Blake March 5, 2016, 4:16 p.m. UTC

We are getting closer to the point where we could use one union
as the base or variant type within another union type (as long
as there are no collisions between any possible combination of
member names allowed across all discriminator choices).  But
until we get to that point, it is worth asserting that variants
are not present in places where we are not prepared to handle
them: base types must still be plain structs, and anywhere we
explode a struct into a parameter list (events and command
marshalling), we don't support variants in that explosion.

Signed-off-by: Eric Blake <[e-mail]>

---
v4: new patch, split out from .is_empty() patch
---
 scripts/qapi.py          | 1 +
 scripts/qapi-commands.py | 3 ++-
 scripts/qapi-event.py    | 1 +
 3 files changed, 4 insertions(+), 1 deletion(-)

Comments

Markus Armbruster March 8, 2016, 10:12 a.m. UTC | #1

Eric Blake <[e-mail]> writes:

> We are getting closer to the point where we could use one union
> as the base or variant type within another union type (as long
> as there are no collisions between any possible combination of
> member names allowed across all discriminator choices).  But
> until we get to that point, it is worth asserting that variants
> are not present in places where we are not prepared to handle
> them: base types must still be plain structs, and anywhere we
> explode a struct into a parameter list (events and command
> marshalling), we don't support variants in that explosion.
>
> Signed-off-by: Eric Blake <[e-mail]>
>
> ---
> v4: new patch, split out from .is_empty() patch
> ---
>  scripts/qapi.py          | 1 +
>  scripts/qapi-commands.py | 3 ++-
>  scripts/qapi-event.py    | 1 +
>  3 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 6b2aa6e..dc26ef9 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -960,6 +960,7 @@ class QAPISchemaObjectType(QAPISchemaType):
>              assert isinstance(self.base, QAPISchemaObjectType)
>              self.base.check(schema)
>              self.base.check_clash(schema, self.info, seen)
> +            assert not self.base.variants

I'd move this two lines up, so it's next to the isinstance.

Assertions in .check() are place-holders for semantic checks that
haven't been moved from the old semantic analysis to the classes.
Whenever we add one, we should double-check the old semantic analysis
catches whatever we assert.  For object types, that's check_struct() and
check_union().  Both check_type() the base with allow_metas=['struct']),
so we're good.

Inconsistency: you add the check for base, but not for variants.

On closer look, adding it for either is actually redundant, because
se.f.base.check_clash() already asserts it, with a nice "not
implemented" comment.

If we think asserting twice is useful for base, then it's useful for
variants, too.  But I think asserting once suffices.

While reviewing use of .check_clash(), I got quite confused by
QAPISchemaAlternateType().check():

    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, {})
        # Alternate branch names have no relation to the tag enum values;
        # so we have to check for potential name collisions ourselves.
        seen = {}
        for v in self.variants.variants:
            v.check_clash(self.info, seen)

Commit b807a1e added the "Not calling self.variants.check_clash()"
comment.

Commit 0426d53 added the loop with its comment.  Which on first glance
looks like the loop in self.variants.check_clash()!  But it's really
different, namely:

        for v in self.variants.variants:
            # Reset seen map for each variant, since qapi names from one
            # branch do not affect another branch
            assert isinstance(v.type, QAPISchemaObjectType)
            v.type.check_clash(schema, info, dict(seen))

i.e. it calls QAPISchemaObjectType.check_clash() to check for clashes
between each variant's members and the common members in seen, whereas
the other loop calls QAPISchemaObjectTypeVariant.check_clash(), which is
really QAPISchemaMember.check_clash(), to check for clashes between
variant names.

Nice little OO maze we've built there %-}

>          for m in self.local_members:
>              m.check(schema)
>              m.check_clash(self.info, seen)
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index f44e01f..edcbd10 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -2,7 +2,7 @@
>  # QAPI command marshaller generator
>  #
>  # Copyright IBM, Corp. 2011
> -# Copyright (C) 2014-2015 Red Hat, Inc.
> +# Copyright (C) 2014-2016 Red Hat, Inc.
>  #
>  # Authors:
>  #  Anthony Liguori <[e-mail]>
> @@ -30,6 +30,7 @@ def gen_call(name, arg_type, ret_type):
>
>      argstr = ''
>      if arg_type:
> +        assert not arg_type.variants
>          for memb in arg_type.members:
>              if memb.optional:
>                  argstr += 'has_%s, ' % c_name(memb.name)

Assertions in generators need to be protected by semantic checks in
.check().  The .check() can either do it themselves, or assert and
delegate to the old semantic analysis.  Whenever we add an assertion to
a generator, we should double-check it's protected properly.

gen_call() is used only by QAPISchemaGenCommandVisitor.visit_command()
via gen_marshal(), so QAPISchemaCommand.check() is responsible for
protecting.  It has a "not implemented" assertion, i.e. it delegates to
check_command().  That one check_type()s the argument with
allow_metas=['struct'], so we're good.

> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index fb579dd..c03cb78 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -59,6 +59,7 @@ def gen_event_send(name, arg_type):
>                   name=name)
>
>      if arg_type and arg_type.members:
> +        assert not arg_type.variants
>          ret += mcgen('''
>      qov = qmp_output_visitor_new();
>      v = qmp_output_get_visitor(qov);

gen_event_send() is used only by
QAPISchemaGenEventVisitor.visit_event(), so QAPISchemaEvent.check() is
responsible for protecting.  It has a "not implemented" assertion,
i.e. it delegates to check_event().  That one check_type()s the argument
with allow_metas=['struct'], so we're good.

Eric Blake March 8, 2016, 3:49 p.m. UTC | #2

On 03/08/2016 03:12 AM, Markus Armbruster wrote:
> Eric Blake <[e-mail]> writes:
> 
>> We are getting closer to the point where we could use one union
>> as the base or variant type within another union type (as long
>> as there are no collisions between any possible combination of
>> member names allowed across all discriminator choices).  But
>> until we get to that point, it is worth asserting that variants
>> are not present in places where we are not prepared to handle
>> them: base types must still be plain structs, and anywhere we
>> explode a struct into a parameter list (events and command
>> marshalling), we don't support variants in that explosion.
>>
>> Signed-off-by: Eric Blake <[e-mail]>
>>

>> +++ b/scripts/qapi.py
>> @@ -960,6 +960,7 @@ class QAPISchemaObjectType(QAPISchemaType):
>>              assert isinstance(self.base, QAPISchemaObjectType)
>>              self.base.check(schema)
>>              self.base.check_clash(schema, self.info, seen)
>> +            assert not self.base.variants
> 
> I'd move this two lines up, so it's next to the isinstance.
> 
> Assertions in .check() are place-holders for semantic checks that
> haven't been moved from the old semantic analysis to the classes.
> Whenever we add one, we should double-check the old semantic analysis
> catches whatever we assert.  For object types, that's check_struct() and
> check_union().  Both check_type() the base with allow_metas=['struct']),
> so we're good.
> 
> Inconsistency: you add the check for base, but not for variants.
> 
> On closer look, adding it for either is actually redundant, because
> se.f.base.check_clash() already asserts it, with a nice "not
> implemented" comment.
> 
> If we think asserting twice is useful for base, then it's useful for
> variants, too.  But I think asserting once suffices.

So basically, we can drop this hunk, right?

Markus Armbruster March 8, 2016, 5:46 p.m. UTC | #3

Eric Blake <[e-mail]> writes:

> On 03/08/2016 03:12 AM, Markus Armbruster wrote:
>> Eric Blake <[e-mail]> writes:
>> 
>>> We are getting closer to the point where we could use one union
>>> as the base or variant type within another union type (as long
>>> as there are no collisions between any possible combination of
>>> member names allowed across all discriminator choices).  But
>>> until we get to that point, it is worth asserting that variants
>>> are not present in places where we are not prepared to handle
>>> them: base types must still be plain structs, and anywhere we
>>> explode a struct into a parameter list (events and command
>>> marshalling), we don't support variants in that explosion.
>>>
>>> Signed-off-by: Eric Blake <[e-mail]>
>>>
>
>>> +++ b/scripts/qapi.py
>>> @@ -960,6 +960,7 @@ class QAPISchemaObjectType(QAPISchemaType):
>>>              assert isinstance(self.base, QAPISchemaObjectType)
>>>              self.base.check(schema)
>>>              self.base.check_clash(schema, self.info, seen)
>>> +            assert not self.base.variants
>> 
>> I'd move this two lines up, so it's next to the isinstance.
>> 
>> Assertions in .check() are place-holders for semantic checks that
>> haven't been moved from the old semantic analysis to the classes.
>> Whenever we add one, we should double-check the old semantic analysis
>> catches whatever we assert.  For object types, that's check_struct() and
>> check_union().  Both check_type() the base with allow_metas=['struct']),
>> so we're good.
>> 
>> Inconsistency: you add the check for base, but not for variants.
>> 
>> On closer look, adding it for either is actually redundant, because
>> se.f.base.check_clash() already asserts it, with a nice "not
>> implemented" comment.
>> 
>> If we think asserting twice is useful for base, then it's useful for
>> variants, too.  But I think asserting once suffices.
>
> So basically, we can drop this hunk, right?

Yes.

Patch

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 6b2aa6e..dc26ef9 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -960,6 +960,7 @@  class QAPISchemaObjectType(QAPISchemaType):
             assert isinstance(self.base, QAPISchemaObjectType)
             self.base.check(schema)
             self.base.check_clash(schema, self.info, seen)
+            assert not self.base.variants
         for m in self.local_members:
             m.check(schema)
             m.check_clash(self.info, seen)
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index f44e01f..edcbd10 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -2,7 +2,7 @@ 
 # QAPI command marshaller generator
 #
 # Copyright IBM, Corp. 2011
-# Copyright (C) 2014-2015 Red Hat, Inc.
+# Copyright (C) 2014-2016 Red Hat, Inc.
 #
 # Authors:
 #  Anthony Liguori <[e-mail]>
@@ -30,6 +30,7 @@  def gen_call(name, arg_type, ret_type):

     argstr = ''
     if arg_type:
+        assert not arg_type.variants
         for memb in arg_type.members:
             if memb.optional:
                 argstr += 'has_%s, ' % c_name(memb.name)
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index fb579dd..c03cb78 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -59,6 +59,7 @@  def gen_event_send(name, arg_type):
                  name=name)

     if arg_type and arg_type.members:
+        assert not arg_type.variants
         ret += mcgen('''
     qov = qmp_output_visitor_new();
     v = qmp_output_get_visitor(qov);