[Qemu-devel] [PATCH v8 15/18] qapi: Move duplicate member checks to schema check()

[Eric Blake <eblake@redhat.com>]

With the previous commit, we have two different locations for
detecting member name clashes - one at parse time, and another
at QAPISchema*.check() time.  Consolidate some of the checks
into a single place, which is also in line with our TODO to
eventually move all of the parse time semantic checking into
the newer schema code.  The check_member_clash() function is
no longer needed.

Checking variants is tricky: we need to check that the branch
name will not cause a collision (important for C code, but
no bearing on QMP).  Then, if the variant belongs to a union
(flat or simple), we need to check that QMP members of that
type will not collide with non-variant QMP members (but do
not care if they collide with C branch names).  Each call to
variant.check() has a 'seen' that contains all [*] non-variant
C names (which includes all non-variant QMP names), but does
not add to that array (QMP members of one branch do not cause
collisions with other branches).  This means that there is
one form of collision we still miss: when two branch names
collide.  However, that will be dealt with in the next patch.

[*] Exception - the 'seen' array doesn't contain 'base', which
is currently a non-variant C member of structs; but since
structs don't contain variants, it doesn't hurt. Besides, a
later patch will be unboxing structs the way flat unions
have already been unboxed.

The wording of several error messages has changed, but in many
cases feels like an improvement rather than a regression.  The
recent change (commit 7b2a5c2) to avoid an assertion failure
when a flat union branch name collides with its discriminator
name is also handled nicely by this code; but there is more work
needed before we can detect all collisions in simple union branch
names done by the old code.

No change to generated code.

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

---
v8: decide whether to inline members based on union vs. alternate,
not on flat vs. simple, and fix logic to avoid breaking
union-clash-data in the process; add comments; assumes
pull-qapi-2015-10-12 will go in without modifying commit ids
v7: comment improvements, retitle subject
v6: rebase to earlier testsuite improvements, fold in cleanup
of flat-union-clash-type
---
 scripts/qapi.py                               | 70 ++++++++++++---------------
 tests/qapi-schema/flat-union-clash-member.err |  2 +-
 tests/qapi-schema/flat-union-clash-type.err   |  2 +-
 tests/qapi-schema/struct-base-clash-deep.err  |  2 +-
 tests/qapi-schema/struct-base-clash.err       |  2 +-
 5 files changed, 36 insertions(+), 42 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 58c4bb3..144dd4a 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -496,21 +496,6 @@ def check_type(expr_info, source, value, allow_array=False,
                                 'enum'])


-def check_member_clash(expr_info, base_name, data, source=""):
-    base = find_struct(base_name)
-    assert base
-    base_members = base['data']
-    for key in data.keys():
-        if key.startswith('*'):
-            key = key[1:]
-        if key in base_members or "*" + key in base_members:
-            raise QAPIExprError(expr_info,
-                                "Member name '%s'%s clashes with base '%s'"
-                                % (key, source, base_name))
-    if base.get('base'):
-        check_member_clash(expr_info, base['base'], data, source)
-
-
 def check_command(expr, expr_info):
     name = expr['command']

@@ -589,30 +574,18 @@ def check_union(expr, expr_info):
     for (key, value) in members.items():
         check_name(expr_info, "Member of union '%s'" % name, key)

-        # Each value must name a known type; furthermore, in flat unions,
-        # branches must be a struct with no overlapping member names
+        # Each value must name a known type
         check_type(expr_info, "Member '%s' of union '%s'" % (key, name),
                    value, allow_array=not base, allow_metas=allow_metas)
-        if base:
-            branch_struct = find_struct(value)
-            assert branch_struct
-            check_member_clash(expr_info, base, branch_struct['data'],
-                               " of branch '%s'" % key)

         # If the discriminator names an enum type, then all members
-        # of 'data' must also be members of the enum type, which in turn
-        # must not collide with the discriminator name.
+        # of 'data' must also be members of the enum type.
         if enum_define:
             if key not in enum_define['enum_values']:
                 raise QAPIExprError(expr_info,
                                     "Discriminator value '%s' is not found in "
                                     "enum '%s'" %
                                     (key, enum_define["enum_name"]))
-            if discriminator in enum_define['enum_values']:
-                raise QAPIExprError(expr_info,
-                                    "Discriminator name '%s' collides with "
-                                    "enum value in '%s'" %
-                                    (discriminator, enum_define["enum_name"]))

         # Otherwise, check for conflicts in the generated enum
         else:
@@ -687,8 +660,6 @@ def check_struct(expr, expr_info):
                allow_dict=True, allow_optional=True)
     check_type(expr_info, "'base' for struct '%s'" % name, expr.get('base'),
                allow_metas=['struct'])
-    if expr.get('base'):
-        check_member_clash(expr_info, expr['base'], expr['data'])


 def check_keys(expr_elem, meta, required, optional=[]):
@@ -982,7 +953,7 @@ class QAPISchemaObjectType(QAPISchemaType):
         for m in self.local_members:
             m.check(schema, self.info, members, seen)
         if self.variants:
-            self.variants.check(schema, self.info, members, seen)
+            self.variants.check(schema, self.info, members, seen, True)
         self.members = members

     def is_implicit(self):
@@ -1094,7 +1065,7 @@ class QAPISchemaObjectTypeVariants(object):
         for v in self.variants:
             v.set_owner(name)

-    def check(self, schema, info, members, seen):
+    def check(self, schema, info, members, seen, union):
         if self.tag_name:
             self.tag_member = seen[c_name(self.tag_name)]
             assert self.tag_name == self.tag_member.name
@@ -1102,18 +1073,41 @@ class QAPISchemaObjectTypeVariants(object):
             self.tag_member.check(schema, info, members, seen)
         assert isinstance(self.tag_member.type, QAPISchemaEnumType)
         for v in self.variants:
-            vseen = dict(seen)
-            v.check(schema, info, self.tag_member.type, vseen)
+            v.check(schema, info, self.tag_member.type, dict(seen), union)


 class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
     def __init__(self, name, typ):
         QAPISchemaObjectTypeMember.__init__(self, name, typ, False)

-    def check(self, schema, info, tag_type, seen):
-        QAPISchemaObjectTypeMember.check(self, schema, info, [], seen)
+    # TODO remove union argument once alternate types can be distinguished
+    # solely by their different tag_type
+    def check(self, schema, info, tag_type, seen, union):
+        # Check that the branch name does not collide with non-variant C
+        # members, without modifying caller's copy of seen
+        # TODO Detect collisions between branch names in C - easiest
+        # will be to check instead for collisions in the corresponding
+        # enum type
+        QAPISchemaObjectTypeMember.check(self, schema, info, [], dict(seen))
         assert self.name in tag_type.values

+        # Additionally, for unions (flat or simple), the QMP members of the
+        # (possibly implicit) variant type are flattened into the owner
+        # object, and must not collide with any non-variant members. For
+        # alternates, no flattening occurs.  No type can contain itself
+        # directly as a variant.
+        if union:
+            assert isinstance(self.type, QAPISchemaObjectType)
+            self.type.check(schema)
+            assert not self.type.variants    # not implemented
+            for m in self.type.members:
+                if m.c_name() in seen:
+                    raise QAPIExprError(info,
+                                        "Member '%s' of branch '%s' collides "
+                                        "with %s"
+                                        % (m.name, self.name,
+                                           seen[m.c_name()].describe()))
+
     # This function exists to support ugly simple union special cases
     # TODO get rid of them, and drop the function
     def simple_union_type(self):
@@ -1136,7 +1130,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
         self.variants = variants

     def check(self, schema):
-        self.variants.check(schema, self.info, [], {})
+        self.variants.check(schema, self.info, [], {}, False)

     def json_type(self):
         return 'value'
diff --git a/tests/qapi-schema/flat-union-clash-member.err b/tests/qapi-schema/flat-union-clash-member.err
index 2f0397a..57dd478 100644
--- a/tests/qapi-schema/flat-union-clash-member.err
+++ b/tests/qapi-schema/flat-union-clash-member.err
@@ -1 +1 @@
-tests/qapi-schema/flat-union-clash-member.json:11: Member name 'name' of branch 'value1' clashes with base 'Base'
+tests/qapi-schema/flat-union-clash-member.json:11: Member 'name' of branch 'value1' collides with 'name' (member of Base)
diff --git a/tests/qapi-schema/flat-union-clash-type.err b/tests/qapi-schema/flat-union-clash-type.err
index b44dd40..3f3248b 100644
--- a/tests/qapi-schema/flat-union-clash-type.err
+++ b/tests/qapi-schema/flat-union-clash-type.err
@@ -1 +1 @@
-tests/qapi-schema/flat-union-clash-type.json:11: Discriminator name 'type' collides with enum value in 'TestEnum'
+tests/qapi-schema/flat-union-clash-type.json:11: 'type' (branch of TestUnion) collides with 'type' (member of Base)
diff --git a/tests/qapi-schema/struct-base-clash-deep.err b/tests/qapi-schema/struct-base-clash-deep.err
index f7a25a3..e2d7943 100644
--- a/tests/qapi-schema/struct-base-clash-deep.err
+++ b/tests/qapi-schema/struct-base-clash-deep.err
@@ -1 +1 @@
-tests/qapi-schema/struct-base-clash-deep.json:10: Member name 'name' clashes with base 'Base'
+tests/qapi-schema/struct-base-clash-deep.json:10: 'name' (member of Sub) collides with 'name' (member of Base)
diff --git a/tests/qapi-schema/struct-base-clash.err b/tests/qapi-schema/struct-base-clash.err
index 3a9f66b..c52f33d 100644
--- a/tests/qapi-schema/struct-base-clash.err
+++ b/tests/qapi-schema/struct-base-clash.err
@@ -1 +1 @@
-tests/qapi-schema/struct-base-clash.json:5: Member name 'name' clashes with base 'Base'
+tests/qapi-schema/struct-base-clash.json:5: 'name' (member of Sub) collides with 'name' (member of Base)
-- 
2.4.3