[Qemu-devel] [PATCH v11 27/28] qapi: Move duplicate enum value checks to schema check()

[Eric Blake]

Similar to the previous commit, move the detection of a collision
in enum values from parse time to QAPISchemaEnumType.check().
This happens to also detect collisions in union branch names
mapping to the same enum value, even when the names do not
collide case-wise.  So for a decent error message, we have to
determine if the enum is implicit (and if so where the real
collision lies).

Testing this showed that the test union-bad-branch wasn't adding
much: union-clash-branches exposes the error message when branches
directly collide, and union-max exposes the error message when
branches cause an enum collision (union-bad-branch basically
causes an enum collision that would not be a C collision).  This
goes along with our desire to require ALL names to be
case-insensitively unique.

No change to generated code.

Signed-off-by: Eric Blake <address@hidden>

---
v11 (no v10): message of union-clash-branches.err changed: rely on
enum check rather than Variant.check() to catch it
v9: rebase to earlier changes, update commit message, break out
helper _describe() method
v8: rebase to earlier changes; better comments
v7: retitle and improve commit message; earlier subclass patches
avoid problem with detecting 'kind' collision
v6: new patch
---
 scripts/qapi.py                             | 41 ++++++++++++++++-------------
 tests/Makefile                              |  1 -
 tests/qapi-schema/enum-clash-member.err     |  2 +-
 tests/qapi-schema/enum-max-member.err       |  2 +-
 tests/qapi-schema/union-bad-branch.err      |  1 -
 tests/qapi-schema/union-bad-branch.exit     |  1 -
 tests/qapi-schema/union-bad-branch.json     |  8 ------
 tests/qapi-schema/union-bad-branch.out      |  0
 tests/qapi-schema/union-clash-branches.err  |  2 +-
 tests/qapi-schema/union-clash-branches.json |  2 +-
 tests/qapi-schema/union-max.err             |  2 +-
 11 files changed, 28 insertions(+), 34 deletions(-)
 delete mode 100644 tests/qapi-schema/union-bad-branch.err
 delete mode 100644 tests/qapi-schema/union-bad-branch.exit
 delete mode 100644 tests/qapi-schema/union-bad-branch.json
 delete mode 100644 tests/qapi-schema/union-bad-branch.out

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 1d59ce9..08a366e 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -533,7 +533,6 @@ def check_union(expr, expr_info):
     base = expr.get('base')
     discriminator = expr.get('discriminator')
     members = expr['data']
-    values = {'MAX': '(automatic)'}

     # Two types of unions, determined by discriminator.

@@ -593,15 +592,6 @@ def check_union(expr, expr_info):
                                     "enum '%s'" %
                                     (key, enum_define["enum_name"]))

-        # Otherwise, check for conflicts in the generated enum
-        else:
-            c_key = camel_to_upper(key)
-            if c_key in values:
-                raise QAPIExprError(expr_info,
-                                    "Union '%s' member '%s' clashes with '%s'"
-                                    % (name, key, values[c_key]))
-            values[c_key] = key
-

 def check_alternate(expr, expr_info):
     name = expr['alternate']
@@ -630,7 +620,6 @@ def check_enum(expr, expr_info):
     name = expr['enum']
     members = expr.get('data')
     prefix = expr.get('prefix')
-    values = {'MAX': '(automatic)'}

     if not isinstance(members, list):
         raise QAPIExprError(expr_info,
@@ -641,12 +630,6 @@ def check_enum(expr, expr_info):
     for member in members:
         check_name(expr_info, "Member of enum '%s'" % name, member,
                    enum_member=True)
-        key = camel_to_upper(member)
-        if key in values:
-            raise QAPIExprError(expr_info,
-                                "Enum '%s' member '%s' clashes with '%s'"
-                                % (name, member, values[key]))
-        values[key] = member


 def check_struct(expr, expr_info):
@@ -873,8 +856,30 @@ class QAPISchemaEnumType(QAPISchemaType):
         self.values = values
         self.prefix = prefix

+    def _describe(self, schema):
+        # If the enum is implicit, report the error on behalf of
+        # the union or alternate that triggered the enum
+        if self.is_implicit():
+            owner = schema.lookup_type(self.name[:-4])
+            assert owner
+            if isinstance(owner, QAPISchemaAlternateType):
+                return "Alternate '%s' branch" % owner.name
+            else:
+                return "Union '%s' branch" % owner.name
+        else:
+            return "Enum '%s' value" % self.name
+
     def check(self, schema):
-        assert len(set(self.values)) == len(self.values)
+        # Check for collisions on the generated C enum values
+        seen = {c_enum_const(self.name, 'MAX'): '(automatic MAX)'}
+        for value in self.values:
+            c_value = c_enum_const(self.name, value)
+            if c_value in seen:
+                raise QAPIExprError(self.info,
+                                    "%s '%s' clashes with '%s'"
+                                    % (self._describe(schema), value,
+                                       seen[c_value]))
+            seen[c_value] = value

     def is_implicit(self):
         # See QAPISchema._make_implicit_enum_type()
diff --git a/tests/Makefile b/tests/Makefile
index d1c6817..cdff7a4 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -336,7 +336,6 @@ qapi-schema += unclosed-list.json
 qapi-schema += unclosed-object.json
 qapi-schema += unclosed-string.json
 qapi-schema += unicode-str.json
-qapi-schema += union-bad-branch.json
 qapi-schema += union-base-no-discriminator.json
 qapi-schema += union-clash-branches.json
 qapi-schema += union-clash-data.json
diff --git a/tests/qapi-schema/enum-clash-member.err 
b/tests/qapi-schema/enum-clash-member.err
index 48bd136..84030c5 100644
--- a/tests/qapi-schema/enum-clash-member.err
+++ b/tests/qapi-schema/enum-clash-member.err
@@ -1 +1 @@
-tests/qapi-schema/enum-clash-member.json:2: Enum 'MyEnum' member 'ONE' clashes 
with 'one'
+tests/qapi-schema/enum-clash-member.json:2: Enum 'MyEnum' value 'ONE' clashes 
with 'one'
diff --git a/tests/qapi-schema/enum-max-member.err 
b/tests/qapi-schema/enum-max-member.err
index f77837f..6b9ef9b 100644
--- a/tests/qapi-schema/enum-max-member.err
+++ b/tests/qapi-schema/enum-max-member.err
@@ -1 +1 @@
-tests/qapi-schema/enum-max-member.json:3: Enum 'MyEnum' member 'max' clashes 
with '(automatic)'
+tests/qapi-schema/enum-max-member.json:3: Enum 'MyEnum' value 'max' clashes 
with '(automatic MAX)'
diff --git a/tests/qapi-schema/union-bad-branch.err 
b/tests/qapi-schema/union-bad-branch.err
deleted file mode 100644
index 8822735..0000000
--- a/tests/qapi-schema/union-bad-branch.err
+++ /dev/null
@@ -1 +0,0 @@
-tests/qapi-schema/union-bad-branch.json:6: Union 'MyUnion' member 'ONE' 
clashes with 'one'
diff --git a/tests/qapi-schema/union-bad-branch.exit 
b/tests/qapi-schema/union-bad-branch.exit
deleted file mode 100644
index d00491f..0000000
--- a/tests/qapi-schema/union-bad-branch.exit
+++ /dev/null
@@ -1 +0,0 @@
-1
diff --git a/tests/qapi-schema/union-bad-branch.json 
b/tests/qapi-schema/union-bad-branch.json
deleted file mode 100644
index 913aa38..0000000
--- a/tests/qapi-schema/union-bad-branch.json
+++ /dev/null
@@ -1,8 +0,0 @@
-# we reject normal unions where branches would collide in C
-{ 'struct': 'One',
-  'data': { 'string': 'str' } }
-{ 'struct': 'Two',
-  'data': { 'number': 'int' } }
-{ 'union': 'MyUnion',
-  'data': { 'one': 'One',
-            'ONE': 'Two' } }
diff --git a/tests/qapi-schema/union-bad-branch.out 
b/tests/qapi-schema/union-bad-branch.out
deleted file mode 100644
index e69de29..0000000
diff --git a/tests/qapi-schema/union-clash-branches.err 
b/tests/qapi-schema/union-clash-branches.err
index 005c48d..d8f1265 100644
--- a/tests/qapi-schema/union-clash-branches.err
+++ b/tests/qapi-schema/union-clash-branches.err
@@ -1 +1 @@
-tests/qapi-schema/union-clash-branches.json:4: Union 'TestUnion' member 'a_b' 
clashes with 'a-b'
+tests/qapi-schema/union-clash-branches.json:4: Union 'TestUnion' branch 'a_b' 
clashes with 'a-b'
diff --git a/tests/qapi-schema/union-clash-branches.json 
b/tests/qapi-schema/union-clash-branches.json
index 31d135f..3bece8c 100644
--- a/tests/qapi-schema/union-clash-branches.json
+++ b/tests/qapi-schema/union-clash-branches.json
@@ -1,5 +1,5 @@
 # Union branch name collision
 # Reject a union that would result in a collision in generated C names (this
-# would try to generate two enum values 'TEST_UNION_KIND_A_B').
+# would try to generate two members 'a_b').
 { 'union': 'TestUnion',
   'data': { 'a-b': 'int', 'a_b': 'str' } }
diff --git a/tests/qapi-schema/union-max.err b/tests/qapi-schema/union-max.err
index 55ce439..b93beae 100644
--- a/tests/qapi-schema/union-max.err
+++ b/tests/qapi-schema/union-max.err
@@ -1 +1 @@
-tests/qapi-schema/union-max.json:2: Union 'Union' member 'max' clashes with 
'(automatic)'
+tests/qapi-schema/union-max.json:2: Union 'Union' branch 'max' clashes with 
'(automatic MAX)'
-- 
2.4.3