[Qemu-devel] [PATCH v8 14/18] qapi: Detect collisions in C member names

[Eric Blake <eblake@redhat.com>]

Detect attempts to declare two object members that would result
in the same C member name, by keying the 'seen' dictionary off
of the C name rather than the qapi name.  It also requires passing
info through some of the check() methods.

This fixes two previously-broken tests, and the resulting error
messages demonstrate the utility of the .describe() method added
previously.  No change to generated code.

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

---
v8: rebase to earlier changes
v7: split out error reporting prep and member.c_name() addition
v6: rebase to earlier testsuite and info improvements
---
 scripts/qapi.py                                | 33 ++++++++++++++++----------
 tests/qapi-schema/args-name-clash.err          |  1 +
 tests/qapi-schema/args-name-clash.exit         |  2 +-
 tests/qapi-schema/args-name-clash.json         |  6 ++---
 tests/qapi-schema/args-name-clash.out          |  6 -----
 tests/qapi-schema/flat-union-clash-branch.err  |  1 +
 tests/qapi-schema/flat-union-clash-branch.exit |  2 +-
 tests/qapi-schema/flat-union-clash-branch.json |  9 +++----
 tests/qapi-schema/flat-union-clash-branch.out  | 14 -----------
 9 files changed, 32 insertions(+), 42 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 8b29e11..58c4bb3 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -978,11 +978,11 @@ class QAPISchemaObjectType(QAPISchemaType):
         seen = {}
         for m in members:
             assert m.c_name() not in seen
-            seen[m.name] = m
+            seen[m.c_name()] = m
         for m in self.local_members:
-            m.check(schema, members, seen)
+            m.check(schema, self.info, members, seen)
         if self.variants:
-            self.variants.check(schema, members, seen)
+            self.variants.check(schema, self.info, members, seen)
         self.members = members

     def is_implicit(self):
@@ -1022,13 +1022,19 @@ class QAPISchemaObjectTypeMember(object):
         assert not self.owner
         self.owner = name

-    def check(self, schema, all_members, seen):
+    def check(self, schema, info, all_members, seen):
         assert self.owner
-        assert self.name not in seen
         self.type = schema.lookup_type(self._type_name)
         assert self.type
+        # Check that there is no collision in generated C names (which
+        # also ensures no collisions in QMP names)
+        if self.c_name() in seen:
+            raise QAPIExprError(info,
+                                "%s collides with %s"
+                                % (self.describe(),
+                                   seen[self.c_name()].describe()))
         all_members.append(self)
-        seen[self.name] = self
+        seen[self.c_name()] = self

     def c_name(self):
         return c_name(self.name)
@@ -1088,23 +1094,24 @@ class QAPISchemaObjectTypeVariants(object):
         for v in self.variants:
             v.set_owner(name)

-    def check(self, schema, members, seen):
+    def check(self, schema, info, members, seen):
         if self.tag_name:
-            self.tag_member = seen[self.tag_name]
+            self.tag_member = seen[c_name(self.tag_name)]
+            assert self.tag_name == self.tag_member.name
         else:
-            self.tag_member.check(schema, members, seen)
+            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, self.tag_member.type, vseen)
+            v.check(schema, info, self.tag_member.type, vseen)


 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)
+    def check(self, schema, info, tag_type, seen):
+        QAPISchemaObjectTypeMember.check(self, schema, info, [], seen)
         assert self.name in tag_type.values

     # This function exists to support ugly simple union special cases
@@ -1129,7 +1136,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
         self.variants = variants

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

     def json_type(self):
         return 'value'
diff --git a/tests/qapi-schema/args-name-clash.err b/tests/qapi-schema/args-name-clash.err
index e69de29..2735217 100644
--- a/tests/qapi-schema/args-name-clash.err
+++ b/tests/qapi-schema/args-name-clash.err
@@ -0,0 +1 @@
+tests/qapi-schema/args-name-clash.json:5: 'a_b' (argument of oops) collides with 'a-b' (argument of oops)
diff --git a/tests/qapi-schema/args-name-clash.exit b/tests/qapi-schema/args-name-clash.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/args-name-clash.exit
+++ b/tests/qapi-schema/args-name-clash.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/args-name-clash.json b/tests/qapi-schema/args-name-clash.json
index 9e8f889..3fe4ea5 100644
--- a/tests/qapi-schema/args-name-clash.json
+++ b/tests/qapi-schema/args-name-clash.json
@@ -1,5 +1,5 @@
 # C member name collision
-# FIXME - This parses, but fails to compile, because the C struct is given
-# two 'a_b' members.  Either reject this at parse time, or munge the C names
-# to avoid the collision.
+# Reject members that clash when mapped to C names (we would have two 'a_b'
+# members). It would also be possible to munge the C names to avoid the
+# collision, but unlikely to be worth the effort.
 { 'command': 'oops', 'data': { 'a-b': 'str', 'a_b': 'str' } }
diff --git a/tests/qapi-schema/args-name-clash.out b/tests/qapi-schema/args-name-clash.out
index 9b2f6e4..e69de29 100644
--- a/tests/qapi-schema/args-name-clash.out
+++ b/tests/qapi-schema/args-name-clash.out
@@ -1,6 +0,0 @@
-object :empty
-object :obj-oops-arg
-    member a-b: str optional=False
-    member a_b: str optional=False
-command oops :obj-oops-arg -> None
-   gen=True success_response=True
diff --git a/tests/qapi-schema/flat-union-clash-branch.err b/tests/qapi-schema/flat-union-clash-branch.err
index e69de29..0190d79 100644
--- a/tests/qapi-schema/flat-union-clash-branch.err
+++ b/tests/qapi-schema/flat-union-clash-branch.err
@@ -0,0 +1 @@
+tests/qapi-schema/flat-union-clash-branch.json:15: 'c-d' (branch of TestUnion) collides with 'c_d' (member of Base)
diff --git a/tests/qapi-schema/flat-union-clash-branch.exit b/tests/qapi-schema/flat-union-clash-branch.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/flat-union-clash-branch.exit
+++ b/tests/qapi-schema/flat-union-clash-branch.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/flat-union-clash-branch.json b/tests/qapi-schema/flat-union-clash-branch.json
index e593336..a6c302f 100644
--- a/tests/qapi-schema/flat-union-clash-branch.json
+++ b/tests/qapi-schema/flat-union-clash-branch.json
@@ -1,8 +1,9 @@
 # Flat union branch name collision
-# FIXME: this parses, but then fails to compile due to a duplicate 'c_d'
-# (one from the base member, the other from the branch name).  We should
-# either reject the collision at parse time, or munge the generated branch
-# name to allow this to compile.
+# Reject attempts to use a branch name that would clash with a non-variant
+# member, when mapped to C names (here, we would have two 'c_d' members,
+# one from the base member, the other from the branch name).
+# TODO: We could munge the generated branch name to avoid the collision and
+# allow this to compile.
 { 'enum': 'TestEnum',
   'data': [ 'base', 'c-d' ] }
 { 'struct': 'Base',
diff --git a/tests/qapi-schema/flat-union-clash-branch.out b/tests/qapi-schema/flat-union-clash-branch.out
index 8e0da73..e69de29 100644
--- a/tests/qapi-schema/flat-union-clash-branch.out
+++ b/tests/qapi-schema/flat-union-clash-branch.out
@@ -1,14 +0,0 @@
-object :empty
-object Base
-    member enum1: TestEnum optional=False
-    member c_d: str optional=True
-object Branch1
-    member string: str optional=False
-object Branch2
-    member value: int optional=False
-enum TestEnum ['base', 'c-d']
-object TestUnion
-    base Base
-    tag enum1
-    case base: Branch1
-    case c-d: Branch2
-- 
2.4.3