[Qemu-devel] [PATCH v7 05/14] qapi: Rework collision assertions

[Eric Blake <eblake@redhat.com>]

Now that we have separate namespaces for QMP vs. tag values,
we can simplify how the QAPISchema*.check() methods check for
collisions.  Each QAPISchemaObjectTypeMember check() call is
given a single set of names it must not collide with; this set
is either the QMP names (when this member is used by an
ObjectType) or the case names (when this member is used by an
ObjectTypeVariants).  We no longer need an all_members
parameter, as it can be computed by seen.values().  When used
by a union, QAPISchemaObjectTypeVariant must also perform a
QMP collision check for each member of its corresponding type.

The new ObjectType.check_qmp() is an idempotent subset of
check(), and can be called multiple times over different seen
sets (useful, since the members of one type can be applied
into more than one other location via inheritance or flat
union variants).

The code needs a temporary hack of passing a 'union' flag
through Variants.check(), since we do not inline the branches
of an alternate type into a parent QMP object.  A later patch
will rework how alternates are laid out, by adding a new
subclass, and that will allow us to drop the extra parameter.

There are no changes to generated code, and we can now add a
positive test to qapi-schema-test that proves that alternates
can now use names that would previously trigger assertions
(see commit 7b2a5c2f for an example of the failure that was
still possible for alternates before this commit).

Future patches will add more positive tests, improve error
message quality on actual collisions, and move collision
checks out of ad hoc parse code into the check() methods.

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

---
v7: new patch, although it is a much cleaner implementation of
what was attempted by subset B v8 15/18
https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg03042.html
---
 scripts/qapi.py                         | 55 ++++++++++++++++++++-------------
 tests/qapi-schema/qapi-schema-test.json |  2 ++
 tests/qapi-schema/qapi-schema-test.out  |  5 +++
 3 files changed, 40 insertions(+), 22 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index aab2b46..098ba5d 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -974,28 +974,28 @@ class QAPISchemaObjectType(QAPISchemaType):
         self.variants = variants
         self.members = None

+    # Finish construction, and validate that all members are usable
     def check(self, schema):
         assert self.members is not False        # not running in cycles
         if self.members:
             return
         self.members = False                    # mark as being checked
+        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)
-            members = list(self.base.members)
-        else:
-            members = []
-        seen = {}
-        for m in members:
-            assert c_name(m.name) not in seen
-            seen[m.name] = m
+            self.base.check_qmp(schema, seen)
         for m in self.local_members:
-            m.check(schema, members, seen)
+            m.check(schema, seen)
         if self.variants:
-            self.variants.check(schema, members, seen)
-        self.members = members
+            self.variants.check(schema, seen)
+        self.members = seen.values()
+
+    # Check that this type does not introduce QMP collisions into seen
+    def check_qmp(self, schema, seen):
+        self.check(schema)
+        assert not self.variants       # not implemented
+        for m in self.members:
+            m.check(schema, seen)

     def is_implicit(self):
         # See QAPISchema._make_implicit_object_type()
@@ -1029,11 +1029,13 @@ class QAPISchemaObjectTypeMember(object):
         self.type = None
         self.optional = optional

-    def check(self, schema, all_members, seen):
+    def check(self, schema, seen):
+        # seen is a map of names we must not collide with (either QMP
+        # names, when called by ObjectType, or case names, when called
+        # by Variant). This method is safe to call over multiple 'seen'.
         assert self.name not in seen
         self.type = schema.lookup_type(self._type_name)
         assert self.type
-        all_members.append(self)
         seen[self.name] = self


@@ -1052,24 +1054,33 @@ class QAPISchemaObjectTypeVariants(object):
         self.tag_member = tag_member
         self.variants = variants

-    def check(self, schema, members, seen):
+    # TODO drop union once alternates can be distinguished by tag_member
+    def check(self, schema, seen, union=True):
         if self.tag_name:
             self.tag_member = seen[self.tag_name]
+            assert self.tag_member
         else:
-            self.tag_member.check(schema, members, seen)
+            self.tag_member.check(schema, seen)
         assert isinstance(self.tag_member.type, QAPISchemaEnumType)
+        cases = OrderedDict()
         for v in self.variants:
-            vseen = dict(seen)
-            v.check(schema, self.tag_member.type, vseen)
+            # Reset seen array for each variant, since QMP names from one
+            # branch do not affect another branch, nor add to all_members
+            v.check(schema, self.tag_member.type, dict(seen), cases, union)


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

-    def check(self, schema, tag_type, seen):
-        QAPISchemaObjectTypeMember.check(self, schema, [], seen)
+    # TODO drop union once alternates can be distinguished by tag_type
+    def check(self, schema, tag_type, seen, cases, union):
+        # cases is case names we must not collide with
+        QAPISchemaObjectTypeMember.check(self, schema, cases)
         assert self.name in tag_type.values
+        if union:
+            # seen is QMP names our members must not collide with
+            self.type.check_qmp(schema, seen)

     # This function exists to support ugly simple union special cases
     # TODO get rid of them, and drop the function
@@ -1090,7 +1101,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
         self.variants = variants

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

     def json_type(self):
         return 'value'
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index efa7acc..926bd7e 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -120,6 +120,8 @@
 { 'struct': 'has_a', 'data': { 'MyKind': 'int', 'MyList': ['int'] } }
 { 'union': 'u', 'data': { 'u': 'uint8', 'myKind': 'has_a',
                           'myList': 'has_a' } }
+{ 'alternate': 'AltName', 'data': { 'type': 'int', 'u': 'bool',
+                                    'myKind': 'has_a' } }

 # testing commands
 { 'command': 'user_def_cmd', 'data': {} }
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 34fb317..1c39a2a 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -61,6 +61,11 @@ alternate AltIntNum
     case i: int
     case n: number
 enum AltIntNumKind ['i', 'n']
+alternate AltName
+    case type: int
+    case u: bool
+    case myKind: has_a
+enum AltNameKind ['type', 'u', 'myKind']
 alternate AltNumInt
     case n: number
     case i: int
-- 
2.4.3