[Qemu-devel] [PATCH v10 28/30] 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 the check_clash() methods.

This addresses a TODO and fixes the previously-broken
args-name-clash test.  The resulting error message demonstrates
the utility of the .describe() method added previously.  No change
to generated code.

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

---
v10 (now in subset C): rebase to latest; update commit message
v9 (now in subset D): rebase to earlier changes, now only one test
affected
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                        | 31 +++++++++++++++++++------------
 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 ------
 5 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index b3af973..3a359cb 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -975,21 +975,24 @@ class QAPISchemaObjectType(QAPISchemaType):
         seen = OrderedDict()
         if self._base_name:
             self.base = schema.lookup_type(self._base_name)
-            self.base.check_clash(schema, seen)
+            self.base.check_clash(schema, self.info, seen)
         for m in self.local_members:
             m.check(schema)
-            m.check_clash(seen)
+            m.check_clash(self.info, seen)
         self.members = seen.values()
         if self.variants:
             self.variants.check(schema, seen)
             assert self.variants.tag_member in self.members
-            self.variants.check_clash(schema, seen)
+            self.variants.check_clash(schema, self.info, seen)

-    def check_clash(self, schema, seen):
+    # Check that the members of this type do not cause duplicate JSON fields,
+    # and update seen to track the members seen so far. Report any errors
+    # on behalf of info, which is not necessarily self.info
+    def check_clash(self, schema, info, seen):
         self.check(schema)
         assert not self.variants       # not implemented
         for m in self.members:
-            m.check_clash(seen)
+            m.check_clash(info, seen)

     def is_implicit(self):
         # See QAPISchema._make_implicit_object_type()
@@ -1035,10 +1038,13 @@ class QAPISchemaObjectTypeMember(object):
         self.type = schema.lookup_type(self._type_name)
         assert self.type

-    def check_clash(self, seen):
-        # TODO change key of seen from QAPI name to C name
-        assert self.name not in seen
-        seen[self.name] = self
+    def check_clash(self, info, seen):
+        name = c_name(self.name)
+        if name in seen:
+            raise QAPIExprError(info,
+                                "%s collides with %s"
+                                % (self.describe(), seen[name].describe()))
+        seen[name] = self

     def _pretty_owner(self):
         # See QAPISchema._make_implicit_object_type() - reverse the
@@ -1081,18 +1087,19 @@ class QAPISchemaObjectTypeVariants(object):

     def check(self, schema, seen):
         if self.tag_name:    # flat union
-            self.tag_member = seen[self.tag_name]
+            self.tag_member = seen[c_name(self.tag_name)]
+            assert self.tag_name == self.tag_member.name
         tag_type = self.tag_member.type
         assert isinstance(tag_type, QAPISchemaEnumType)
         for v in self.variants:
             v.check(schema)
             assert v.name in tag_type.values

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


 class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
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
-- 
2.4.3