[Qemu-devel] [PATCH v5 14/46] 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.  As this is the first
error raised within the QapiSchema check() methods, we also have
to pass 'info' around through the call stack, and fix the overall
'try' to check for errors detected during the check() phase.

The resulting error messages demonstrate the utility of the
.describe() method added previously.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 scripts/qapi.py                                 | 44 +++++++++++++++----------
 tests/qapi-schema/args-name-clash.err           |  1 +
 tests/qapi-schema/args-name-clash.exit          |  2 +-
 tests/qapi-schema/args-name-clash.json          |  2 +-
 tests/qapi-schema/args-name-clash.out           |  6 ----
 tests/qapi-schema/flat-union-branch-clash2.err  |  1 +
 tests/qapi-schema/flat-union-branch-clash2.exit |  2 +-
 tests/qapi-schema/flat-union-branch-clash2.json |  2 +-
 tests/qapi-schema/flat-union-branch-clash2.out  | 14 --------
 9 files changed, 33 insertions(+), 41 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 6bc13f1..0a0ac90 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

@@ -956,11 +957,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):
@@ -997,12 +999,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)
@@ -1024,23 +1033,24 @@ class QAPISchemaObjectTypeVariants(object):
                                                          '(implicit)')
         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):
@@ -1065,7 +1075,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'
@@ -1122,13 +1132,13 @@ class QAPISchema(object):
     def __init__(self, fname):
         try:
             self.exprs = check_exprs(QAPISchemaParser(open(fname, "r")).exprs)
+            self._entity_dict = {}
+            self._def_predefineds()
+            self._def_exprs()
+            self.check()
         except (QAPISchemaError, QAPIExprError), err:
             print >>sys.stderr, err
             exit(1)
-        self._entity_dict = {}
-        self._def_predefineds()
-        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..743afdb 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:2: '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 19bf792..602db6a 100644
--- a/tests/qapi-schema/args-name-clash.json
+++ b/tests/qapi-schema/args-name-clash.json
@@ -1,2 +1,2 @@
-# FIXME - we should reject data with members that clash when mapped to C names
+# we reject data with members that clash when mapped to C names
 { '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-branch-clash2.err b/tests/qapi-schema/flat-union-branch-clash2.err
index e69de29..99eacd2 100644
--- a/tests/qapi-schema/flat-union-branch-clash2.err
+++ b/tests/qapi-schema/flat-union-branch-clash2.err
@@ -0,0 +1 @@
+tests/qapi-schema/flat-union-branch-clash2.json:10: 'c-d' (branch of TestUnion)' collides with 'c_d' (member of Base)
diff --git a/tests/qapi-schema/flat-union-branch-clash2.exit b/tests/qapi-schema/flat-union-branch-clash2.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/flat-union-branch-clash2.exit
+++ b/tests/qapi-schema/flat-union-branch-clash2.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/flat-union-branch-clash2.json b/tests/qapi-schema/flat-union-branch-clash2.json
index b3eefb3..b0dd85e 100644
--- a/tests/qapi-schema/flat-union-branch-clash2.json
+++ b/tests/qapi-schema/flat-union-branch-clash2.json
@@ -1,4 +1,4 @@
-# FIXME: we should check for no duplicate C names between branches and base
+# we check for no duplicate C names between branches and base
 { 'enum': 'TestEnum',
   'data': [ 'base', 'c-d' ] }
 { 'struct': 'Base',
diff --git a/tests/qapi-schema/flat-union-branch-clash2.out b/tests/qapi-schema/flat-union-branch-clash2.out
index 8e0da73..e69de29 100644
--- a/tests/qapi-schema/flat-union-branch-clash2.out
+++ b/tests/qapi-schema/flat-union-branch-clash2.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