WIP: detect ambiguous alternates at compile time

This breaks test code. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
ehabkost · May 4, 2017 · cda70a2 · cda70a2
1 parent 5f8a912
commit cda70a2
Show file tree

Hide file tree

Showing 23 changed files with 82 additions and 30 deletions.
diff --git a/scripts/qapi.py b/scripts/qapi.py
@@ -801,6 +801,8 @@ def check_alternate(expr, info):
         raise QAPISemError(info,
                            "Alternate '%s' should have at least two branches "
                            "in 'data'" % name)
+
+    scalar_members = []
     for (key, value) in members.items():
         check_name(info, "Member of alternate '%s'" % name, key)
 
@@ -816,8 +818,35 @@ def check_alternate(expr, info):
             raise QAPISemError(info, "Alternate '%s' member '%s' can't "
                                "be distinguished from member '%s'"
                                % (name, key, types_seen[qtype]))
+
+        if qtype in ['QTYPE_QINT', 'QTYPE_QFLOAT']:
+            scalar_members.append( (r'^ *-?[0-9]+', key) )
+        elif qtype == 'QTYPE_QBOOL':
+            scalar_members.append( (r'^(true|false|on|off|yes|no)$', key) )
+
         types_seen[qtype] = key
 
+    # Representation of scalar members can't conflict with string values
+    # (enums or 'str' members)
+    if scalar_members and 'QTYPE_QSTRING' in types_seen:
+        strkey = types_seen['QTYPE_QSTRING']
+        strtype = members[strkey]
+
+        if strtype == 'str':
+            raise QAPISemError(info, "Alternate '%s' scalar member '%s' can't "
+                                     "can't be distinguished from string "
+                                     "member '%s'"
+                                     % (name, scalar_members[0][1], strkey))
+        else:
+            assert all_names.get(strtype) == 'enum'
+            for (regexp, scalarkey) in scalar_members:
+                for v in enum_types[strtype].get('data', []):
+                    if re.match(regexp, v):
+                        raise QAPISemError(info, "Alternate '%s' member '%s' "
+                                                 "can't be distinguished from "
+                                                 "enum '%s' member '%s'"
+                                                 % (name, strkey, strtype, v))
+
 
 def check_enum(expr, info):
     name = expr['enum']

diff --git a/tests/Makefile.include b/tests/Makefile.include
@@ -339,6 +339,9 @@ qapi-schema += alternate-base.json
 qapi-schema += alternate-clash.json
 qapi-schema += alternate-conflict-dict.json
 qapi-schema += alternate-conflict-string.json
+qapi-schema += alternate-ambiguous-bool.json
+qapi-schema += alternate-ambiguous-num.json
+qapi-schema += alternate-ambiguous-str-scalar.json
 qapi-schema += alternate-empty.json
 qapi-schema += alternate-nested.json
 qapi-schema += alternate-unknown.json

diff --git a/tests/qapi-schema/alternate-ambiguous-bool.err b/tests/qapi-schema/alternate-ambiguous-bool.err
@@ -0,0 +1 @@
+tests/qapi-schema/alternate-ambiguous-bool.json:4: Alternate 'EnumOrNumber' member 'e' can't be distinguished from enum 'AmbiguousEnum' member 'yes'
diff --git a/tests/qapi-schema/alternate-ambiguous-bool.exit b/tests/qapi-schema/alternate-ambiguous-bool.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/alternate-ambiguous-bool.json b/tests/qapi-schema/alternate-ambiguous-bool.json
@@ -0,0 +1,5 @@
+{ 'enum': 'AmbiguousEnum',
+  'data': ['maybe', 'yes', 'no'] }
+
+{ 'alternate': 'EnumOrNumber',
+  'data': { 'e': 'AmbiguousEnum', 'b': 'bool' } }
diff --git a/tests/qapi-schema/alternate-ambiguous-bool.out b/tests/qapi-schema/alternate-ambiguous-bool.out
diff --git a/tests/qapi-schema/alternate-ambiguous-num.err b/tests/qapi-schema/alternate-ambiguous-num.err
@@ -0,0 +1 @@
+tests/qapi-schema/alternate-ambiguous-num.json:4: Alternate 'EnumOrNumber' member 'e' can't be distinguished from enum 'AmbiguousEnum' member '3'
diff --git a/tests/qapi-schema/alternate-ambiguous-num.exit b/tests/qapi-schema/alternate-ambiguous-num.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/alternate-ambiguous-num.json b/tests/qapi-schema/alternate-ambiguous-num.json
@@ -0,0 +1,5 @@
+{ 'enum': 'AmbiguousEnum',
+  'data': ['one', 'two', '3'] }
+
+{ 'alternate': 'EnumOrNumber',
+  'data': { 'e': 'AmbiguousEnum', 'i': 'int' } }
diff --git a/tests/qapi-schema/alternate-ambiguous-num.out b/tests/qapi-schema/alternate-ambiguous-num.out
diff --git a/tests/qapi-schema/alternate-ambiguous-str-scalar.err b/tests/qapi-schema/alternate-ambiguous-str-scalar.err
@@ -0,0 +1 @@
+tests/qapi-schema/alternate-ambiguous-str-scalar.json:1: Alternate 'StrOrNumber' scalar member 'n' can't can't be distinguished from string member 's'
diff --git a/tests/qapi-schema/alternate-ambiguous-str-scalar.exit b/tests/qapi-schema/alternate-ambiguous-str-scalar.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/alternate-ambiguous-str-scalar.json b/tests/qapi-schema/alternate-ambiguous-str-scalar.json
@@ -0,0 +1,2 @@
+{ 'alternate': 'StrOrNumber',
+  'data': { 's': 'str', 'n': 'number' } }
diff --git a/tests/qapi-schema/alternate-ambiguous-str-scalar.out b/tests/qapi-schema/alternate-ambiguous-str-scalar.out
diff --git a/tests/qapi-schema/alternate-clash.json b/tests/qapi-schema/alternate-clash.json
@@ -5,4 +5,4 @@
 # the implicit Alt1Kind enum, we would still have a collision with the
 # resulting C union trying to have two members named 'a_b'.
 { 'alternate': 'Alt1',
-  'data': { 'a-b': 'str', 'a_b': 'int' } }
+  'data': { 'a-b': 'bool', 'a_b': 'int' } }
diff --git a/tests/qapi-schema/alternate-nested.err b/tests/qapi-schema/alternate-nested.err
@@ -1 +1 @@
-tests/qapi-schema/alternate-nested.json:4: Member 'nested' of alternate 'Alt2' cannot use alternate type 'Alt1'
+tests/qapi-schema/alternate-nested.json:6: Member 'nested' of alternate 'Alt2' cannot use alternate type 'Alt1'
diff --git a/tests/qapi-schema/alternate-nested.json b/tests/qapi-schema/alternate-nested.json
@@ -1,5 +1,7 @@
 # we reject a nested alternate branch
+{ 'enum': 'MyEnum',
+  'data': ['one', 'two', 'three']}
 { 'alternate': 'Alt1',
-  'data': { 'name': 'str', 'value': 'int' } }
+  'data': { 'e': 'MyEnum', 'value': 'int' } }
 { 'alternate': 'Alt2',
   'data': { 'nested': 'Alt1', 'b': 'bool' } }
diff --git a/tests/qapi-schema/args-alternate.json b/tests/qapi-schema/args-alternate.json
@@ -1,3 +1,3 @@
 # we do not allow alternate arguments
-{ 'alternate': 'Alt', 'data': { 'case1': 'int', 'case2': 'str' } }
+{ 'alternate': 'Alt', 'data': { 'case1': 'int', 'case2': 'bool' } }
 { 'command': 'oops', 'data': 'Alt' }
diff --git a/tests/qapi-schema/doc-bad-alternate-member.json b/tests/qapi-schema/doc-bad-alternate-member.json
@@ -6,4 +6,4 @@
 # @bb: b
 ##
 { 'alternate': 'AorB',
-  'data': { 'a': 'str', 'b': 'int' } }
+  'data': { 'a': 'bool', 'b': 'int' } }
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
@@ -93,16 +93,16 @@
 { 'struct': 'WrapAlternate',
   'data': { 'alt': 'UserDefAlternate' } }
 { 'alternate': 'UserDefAlternate',
-  'data': { 'udfu': 'UserDefFlatUnion', 's': 'str', 'i': 'int' } }
+  'data': { 'udfu': 'UserDefFlatUnion', 'e': 'EnumOne', 'i': 'int' } }
 
 { 'struct': 'UserDefC',
   'data': { 'string1': 'str', 'string2': 'str' } }
 
 # for testing use of 'number' within alternates
-{ 'alternate': 'AltStrBool', 'data': { 's': 'str', 'b': 'bool' } }
-{ 'alternate': 'AltStrNum', 'data': { 's': 'str', 'n': 'number' } }
-{ 'alternate': 'AltNumStr', 'data': { 'n': 'number', 's': 'str' } }
-{ 'alternate': 'AltStrInt', 'data': { 's': 'str', 'i': 'int' } }
+{ 'alternate': 'AltEnumBool', 'data': { 'e': 'EnumOne', 'b': 'bool' } }
+{ 'alternate': 'AltEnumNum', 'data': { 'e': 'EnumOne', 'n': 'number' } }
+{ 'alternate': 'AltNumEnum', 'data': { 'n': 'number', 'e': 'EnumOne' } }
+{ 'alternate': 'AltEnumInt', 'data': { 'e': 'EnumOne', 'i': 'int' } }
 { 'alternate': 'AltIntNum', 'data': { 'i': 'int', 'n': 'number' } }
 { 'alternate': 'AltNumInt', 'data': { 'n': 'number', 'i': 'int' } }
 

diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
@@ -2,34 +2,34 @@ alternate AltBoolEnum
     tag type
     case b: bool
     case e: EnumOne
+alternate AltEnumBool
+    tag type
+    case e: EnumOne
+    case b: bool
+alternate AltEnumInt
+    tag type
+    case e: EnumOne
+    case i: int
+alternate AltEnumNum
+    tag type
+    case e: EnumOne
+    case n: number
 alternate AltIntNum
     tag type
     case i: int
     case n: number
-alternate AltNumInt
+alternate AltNumEnum
     tag type
     case n: number
-    case i: int
-alternate AltNumStr
+    case e: EnumOne
+alternate AltNumInt
     tag type
     case n: number
-    case s: str
+    case i: int
 alternate AltOnOffInt
     tag type
     case o: TestOnOffAuto
     case i: int
-alternate AltStrBool
-    tag type
-    case s: str
-    case b: bool
-alternate AltStrInt
-    tag type
-    case s: str
-    case i: int
-alternate AltStrNum
-    tag type
-    case s: str
-    case n: number
 event EVENT_A None
    boxed=False
 event EVENT_B None
@@ -75,7 +75,7 @@ object UserDefA
 alternate UserDefAlternate
     tag type
     case udfu: UserDefFlatUnion
-    case s: str
+    case e: EnumOne
     case i: int
 object UserDefB
     member intb: int optional=False

diff --git a/tests/qapi-schema/returns-alternate.json b/tests/qapi-schema/returns-alternate.json
@@ -1,3 +1,3 @@
 # we reject returns if it is an alternate type
-{ 'alternate': 'Alt', 'data': { 'a': 'int', 'b': 'str' } }
+{ 'alternate': 'Alt', 'data': { 'a': 'int', 'b': 'bool' } }
 { 'command': 'oops', 'returns': 'Alt' }
diff --git a/tests/test-clone-visitor.c b/tests/test-clone-visitor.c
@@ -49,7 +49,7 @@ static void test_clone_alternate(void)
     b_src->u.b = true;
     s_src = g_new0(AltStrBool, 1);
     s_src->type = QTYPE_QSTRING;
-    s_src->u.s = g_strdup("World");
+    s_src->u.e = ENUM_ONE_VALUE2;
 
     b_dst = QAPI_CLONE(AltStrBool, b_src);
     g_assert(b_dst);
@@ -59,7 +59,7 @@ static void test_clone_alternate(void)
     g_assert(s_dst);
     g_assert_cmpint(s_dst->type, ==, s_src->type);
     g_assert_cmpstr(s_dst->u.s, ==, s_src->u.s);
-    g_assert(s_dst->u.s != s_src->u.s);
+    g_assert_cmpint(s_dst->u.s != s_src->u.s);
 
     qapi_free_AltStrBool(b_src);
     qapi_free_AltStrBool(s_src);