[Qemu-devel] [PATCH v6 07/12] 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.  Doing this was made
easier by adding a member.c_name() helper function.

As this is the first error raised within the QAPISchema*.check()
methods, we also have to pass 'info' through the call stack, and
fix the overall 'try' to display errors detected during the
check() phase.

This fixes a couple of 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>

---
v6: rebase to earlier testsuite and info improvements
---
 scripts/qapi.py                                | 46 ++++++++++++++++----------
 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, 40 insertions(+), 47 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 880de94..1acf02b 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -103,6 +103,7 @@ class QAPISchemaError(Exception):
 class QAPIExprError(Exception):
     def __init__(self, expr_info, msg):
         Exception.__init__(self)
+        assert expr_info
         self.info = expr_info
         self.msg = msg

@@ -964,11 +965,12 @@ class QAPISchemaObjectType(QAPISchemaType):
             members = []
         seen = {}
         for m in members:
-            seen[m.name] = m
+            assert m.c_name() not in seen
+            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):
@@ -1004,12 +1006,19 @@ class QAPISchemaObjectTypeMember(object):
         self.optional = optional
         self._owner = owner

-    def check(self, schema, all_members, seen):
-        assert self.name not in seen
+    def check(self, schema, info, all_members, seen):
+        if self.c_name() in seen:
+            raise QAPIExprError(info,
+                                "%s collides with %s"
+                                % (self.describe(),
+                                   seen[self.c_name()].describe()))
         self.type = schema.lookup_type(self._type_name)
         assert self.type
         all_members.append(self)
-        seen[self.name] = self
+        seen[self.c_name()] = self
+
+    def c_name(self):
+        return c_name(self.name)

     def describe(self):
         return "'%s' (member of %s)" % (self.name, self._owner)
@@ -1028,23 +1037,24 @@ class QAPISchemaObjectTypeVariants(object):
         self.tag_member = tag_member
         self.variants = variants

-    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, owner):
         QAPISchemaObjectTypeMember.__init__(self, name, typ, False, owner)

-    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

     def describe(self):
@@ -1069,7 +1079,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'
@@ -1128,14 +1138,14 @@ class QAPISchema(object):
     def __init__(self, fname):
         try:
             self.exprs = check_exprs(QAPISchemaParser(open(fname, "r")).exprs)
+            self._entity_dict = {}
+            self._def_predefineds()
+            QAPISchema.predefined_initialized = True
+            self._def_exprs()
+            self.check()
         except (QAPISchemaError, QAPIExprError), err:
             print >>sys.stderr, err
             exit(1)
-        self._entity_dict = {}
-        self._def_predefineds()
-        QAPISchema.predefined_initialized = True
-        self._def_exprs()
-        self.check()

     def _def_entity(self, ent):
         assert ent.name not in self._entity_dict
diff --git a/tests/qapi-schema/args-name-clash.err b/tests/qapi-schema/args-name-clash.err
index e69de29..66f609c 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' (member of oops arguments) collides with 'a-b' (member of oops arguments)
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