diff mbox

[v12,11/36] qapi: Check for qapi collisions involving variant members

Message ID 1447836791-369-12-git-send-email-eblake@redhat.com
State New
Headers show

Commit Message

Eric Blake Nov. 18, 2015, 8:52 a.m. UTC
Right now, our ad hoc parser ensures that we cannot have a
flat union that introduces any members that would clash with
non-variant members inherited from the union's base type (see
flat-union-clash-member.json).  We want ObjectType.check() to
make the same check, so we can later reduce some of the ad
hoc checks.

We already have a map 'seen' of all non-variant members. We
still need to check for collisions between each variant type's
members and the non-variant ones.

To know the variant type's members, we need to call
variant.type.check().  This also detects when a type contains
itself in a variant, exactly like the existing base.check()
detects when a type contains itself as a base.  (Except that
we currently forbid anything but a struct as the type of a
variant, so we can't actually trigger this type of loop yet.)

Slight complication: an alternate's variant can have arbitrary
type, but only an object type's check() may be called outside
QAPISchema.check(). We could either skip the call for variants
of alternates, or skip it for non-object types.  For now, do
the latter, because it's easier.

Then we call each variant member's check_clash() with the
appropriate 'seen' map.  Since members of different variants
can't clash, we have to clone a fresh seen for each variant.
Wrap this in a new helper method Variants.check_clash().

Note that cloning 'seen' inside Variants.check_clash() resembles
the one we just removed from Variants.check() in 'qapi: Drop
obsolete tag value collision assertions'; the difference here is
that we are now checking for clashes among the qapi members of
the variant type, rather than for a single clash with the variant
tag name itself.

Note that, by construction, collisions can't actually happen for
simple unions: each variant's type is a wrapper with a single
member 'data', which will never collide with the only non-variant
member 'type'.

For alternates, there's nothing for a variant object type's
members to clash with, and therefore no need to call the new
variants.check_clash().

No change to generated code.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v12: retitle; use Markus' improved commit wording
v11: keep type.check() in check(), add comment to alternate
v10: create new Variants.check_clash() rather than piggybacking
on .check()
v9: new patch, split off from v8 7/17
---
 scripts/qapi.py | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Markus Armbruster Nov. 18, 2015, 10:01 a.m. UTC | #1
Eric Blake <eblake@redhat.com> writes:

> Right now, our ad hoc parser ensures that we cannot have a
> flat union that introduces any members that would clash with
> non-variant members inherited from the union's base type (see
> flat-union-clash-member.json).  We want ObjectType.check() to

Recommend not to abbreviate QAPISchemaObjectType.check().

> make the same check, so we can later reduce some of the ad
> hoc checks.
>
> We already have a map 'seen' of all non-variant members. We
> still need to check for collisions between each variant type's
> members and the non-variant ones.
>
> To know the variant type's members, we need to call
> variant.type.check().  This also detects when a type contains
> itself in a variant, exactly like the existing base.check()
> detects when a type contains itself as a base.  (Except that
> we currently forbid anything but a struct as the type of a
> variant, so we can't actually trigger this type of loop yet.)
>
> Slight complication: an alternate's variant can have arbitrary
> type, but only an object type's check() may be called outside
> QAPISchema.check(). We could either skip the call for variants
> of alternates, or skip it for non-object types.  For now, do
> the latter, because it's easier.
>
> Then we call each variant member's check_clash() with the
> appropriate 'seen' map.  Since members of different variants
> can't clash, we have to clone a fresh seen for each variant.
> Wrap this in a new helper method Variants.check_clash().

Likewise: QAPISchemaObjectTypeVariants.check_clash().

> Note that cloning 'seen' inside Variants.check_clash() resembles
> the one we just removed from Variants.check() in 'qapi: Drop

Here, we can say .check_clash() and .check(), and leave the class to
context.

> obsolete tag value collision assertions'; the difference here is
> that we are now checking for clashes among the qapi members of
> the variant type, rather than for a single clash with the variant
> tag name itself.
>
> Note that, by construction, collisions can't actually happen for
> simple unions: each variant's type is a wrapper with a single
> member 'data', which will never collide with the only non-variant
> member 'type'.
>
> For alternates, there's nothing for a variant object type's
> members to clash with, and therefore no need to call the new
> variants.check_clash().
>
> No change to generated code.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>

Only commit message polishing, happy to do that in my tree.
diff mbox

Patch

diff --git a/scripts/qapi.py b/scripts/qapi.py
index c6cb17b..b2d071f 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -992,6 +992,7 @@  class QAPISchemaObjectType(QAPISchemaType):
         if self.variants:
             self.variants.check(schema, seen)
             assert self.variants.tag_member in self.members
+            self.variants.check_clash(schema, seen)

     def is_implicit(self):
         # See QAPISchema._make_implicit_object_type()
@@ -1056,6 +1057,18 @@  class QAPISchemaObjectTypeVariants(object):
         assert isinstance(self.tag_member.type, QAPISchemaEnumType)
         for v in self.variants:
             v.check(schema, self.tag_member.type)
+            if isinstance(v.type, QAPISchemaObjectType):
+                v.type.check(schema)
+
+    def check_clash(self, schema, seen):
+        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
+            for m in v.type.members:
+                m.check_clash(vseen)


 class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
@@ -1086,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):