qapi: Forbid base without discriminator in unions

None of the existing QMP or QGA interfaces uses a union with a
base type but no discriminator; it is easier to avoid this in the
generator to save room for other future extensions more likely to
be useful.  An earlier commit added a union-base-no-discriminator
test to ensure that we eventually give a decent error message;
likewise, removing UserDefUnion outright is okay, because we moved
all the tests we wish to keep into the tests of the simple union
UserDefNativeListUnion in the previous commit.  Now is the time to
actually forbid simple union with base, and remove the last
vestiges from the testsuite.
Signed-off-by: NEric Blake <eblake@redhat.com>
Reviewed-by: NMarkus Armbruster <armbru@redhat.com>
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>

scripts/qapi-types.py

@@ -242,10 +242,9 @@ struct %(name)s
 ''')
 
     if base:
-        base_fields = find_struct(base)['data']
-        if discriminator:
-            base_fields = base_fields.copy()
-            del base_fields[discriminator]
+        assert discriminator
+        base_fields = find_struct(base)['data'].copy()
+        del base_fields[discriminator]
         ret += generate_struct_fields(base_fields)
     else:
         assert not discriminator

scripts/qapi-visit.py

@@ -310,16 +310,15 @@ def generate_visit_union(expr):
         ret = ""
         disc_type = enum_define['enum_name']
     else:
-        # There will always be a discriminator in the C switch code, by default it
-        # is an enum type generated silently as "'%sKind' % (name)"
+        # There will always be a discriminator in the C switch code, by default
+        # it is an enum type generated silently as "'%sKind' % (name)"
         ret = generate_visit_enum('%sKind' % name, members.keys())
         disc_type = '%sKind' % (name)
 
     if base:
-        base_fields = find_struct(base)['data']
-        if discriminator:
-            base_fields = base_fields.copy()
-            del base_fields[discriminator]
+        assert discriminator
+        base_fields = find_struct(base)['data'].copy()
+        del base_fields[discriminator]
         ret += generate_visit_struct_fields(name, "", "", base_fields)
 
     if discriminator:

scripts/qapi.py

@@ -259,22 +259,22 @@ def check_union(expr, expr_info):
     discriminator = expr.get('discriminator')
     members = expr['data']
 
-    # If the object has a member 'base', its value must name a complex type.
-    if base:
+    # If the object has a member 'base', its value must name a complex type,
+    # and there must be a discriminator.
+    if base is not None:
+        if discriminator is None:
+            raise QAPIExprError(expr_info,
+                                "Union '%s' requires a discriminator to go "
+                                "along with base" %name)
         base_fields = find_base_fields(base)
         if not base_fields:
             raise QAPIExprError(expr_info,
                                 "Base '%s' is not a valid type"
                                 % base)
 
-    # If the union object has no member 'discriminator', it's an
-    # ordinary union.
-    if not discriminator:
-        enum_define = None
-
-    # Else if the value of member 'discriminator' is {}, it's an
-    # anonymous union.
-    elif discriminator == {}:
+    # If the union object has no member 'discriminator', it's a
+    # simple union. If 'discriminator' is {}, it is an anonymous union.
+    if not discriminator or discriminator == {}:
         enum_define = None
 
     # Else, it's a flat union.

tests/qapi-schema/qapi-schema-test.json

@@ -36,10 +36,6 @@
 { 'type': 'UserDefC',
   'data': { 'string1': 'str', 'string2': 'str' } }
 
-{ 'union': 'UserDefUnion',
-  'base': 'UserDefZero',
-  'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
-
 { 'type': 'UserDefUnionBase',
   'data': { 'string': 'str', 'enum1': 'EnumOne' } }
 

tests/qapi-schema/qapi-schema-test.out

@@ -7,7 +7,6 @@
  OrderedDict([('type', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
  OrderedDict([('type', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]),
  OrderedDict([('type', 'UserDefC'), ('data', OrderedDict([('string1', 'str'), ('string2', 'str')]))]),
- OrderedDict([('union', 'UserDefUnion'), ('base', 'UserDefZero'), ('data', OrderedDict([('a', 'UserDefA'), ('b', 'UserDefB')]))]),
  OrderedDict([('type', 'UserDefUnionBase'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
  OrderedDict([('union', 'UserDefFlatUnion'), ('base', 'UserDefUnionBase'), ('discriminator', 'enum1'), ('data', OrderedDict([('value1', 'UserDefA'), ('value2', 'UserDefB'), ('value3', 'UserDefB')]))]),
  OrderedDict([('union', 'UserDefFlatUnion2'), ('base', 'UserDefUnionBase'), ('discriminator', 'enum1'), ('data', OrderedDict([('value1', 'UserDefC'), ('value2', 'UserDefB'), ('value3', 'UserDefA')]))]),
@@ -24,7 +23,6 @@
  OrderedDict([('event', 'EVENT_C'), ('data', OrderedDict([('*a', 'int'), ('*b', 'UserDefOne'), ('c', 'str')]))]),
  OrderedDict([('event', 'EVENT_D'), ('data', OrderedDict([('a', 'EventStructOne'), ('b', 'str'), ('*c', 'str'), ('*enum3', 'EnumOne')]))])]
 [{'enum_name': 'EnumOne', 'enum_values': ['value1', 'value2', 'value3']},
- {'enum_name': 'UserDefUnionKind', 'enum_values': None},
  {'enum_name': 'UserDefAnonUnionKind', 'enum_values': None},
  {'enum_name': 'UserDefNativeListUnionKind', 'enum_values': None}]
 [OrderedDict([('type', 'NestedEnumsOne'), ('data', OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3', 'EnumOne'), ('*enum4', 'EnumOne')]))]),

tests/qapi-schema/union-base-no-discriminator.err

+tests/qapi-schema/union-base-no-discriminator.json:11: Union 'TestUnion' requires a discriminator to go along with base

tests/qapi-schema/union-base-no-discriminator.exit

-0
+1

tests/qapi-schema/union-base-no-discriminator.json

-# FIXME: we should reject simple unions with a base
+# we reject simple unions with a base (or flat unions without discriminator)
 { 'type': 'TestTypeA',
   'data': { 'string': 'str' } }
 

tests/qapi-schema/union-base-no-discriminator.out

-[OrderedDict([('type', 'TestTypeA'), ('data', OrderedDict([('string', 'str')]))]),
- OrderedDict([('type', 'TestTypeB'), ('data', OrderedDict([('integer', 'int')]))]),
- OrderedDict([('type', 'Base'), ('data', OrderedDict([('string', 'str')]))]),
- OrderedDict([('union', 'TestUnion'), ('base', 'Base'), ('data', OrderedDict([('value1', 'TestTypeA'), ('value2', 'TestTypeB')]))])]
-[{'enum_name': 'TestUnionKind', 'enum_values': None}]
-[OrderedDict([('type', 'TestTypeA'), ('data', OrderedDict([('string', 'str')]))]),
- OrderedDict([('type', 'TestTypeB'), ('data', OrderedDict([('integer', 'int')]))]),
- OrderedDict([('type', 'Base'), ('data', OrderedDict([('string', 'str')]))])]

tests/test-qmp-input-visitor.c

@@ -293,23 +293,6 @@ static void test_visitor_in_list(TestInputVisitorData *data,
     qapi_free_UserDefOneList(head);
 }
 
-static void test_visitor_in_union(TestInputVisitorData *data,
-                                  const void *unused)
-{
-    Visitor *v;
-    Error *err = NULL;
-    UserDefUnion *tmp;
-
-    v = visitor_input_test_init(data, "{ 'type': 'b', 'integer': 41, 'data' : { 'integer': 42 } }");
-
-    visit_type_UserDefUnion(v, &tmp, NULL, &err);
-    g_assert(err == NULL);
-    g_assert_cmpint(tmp->kind, ==, USER_DEF_UNION_KIND_B);
-    g_assert_cmpint(tmp->integer, ==, 41);
-    g_assert_cmpint(tmp->b->integer, ==, 42);
-    qapi_free_UserDefUnion(tmp);
-}
-
 static void test_visitor_in_union_flat(TestInputVisitorData *data,
                                        const void *unused)
 {
@@ -679,8 +662,6 @@ int main(int argc, char **argv)
                            &in_visitor_data, test_visitor_in_struct_nested);
     input_visitor_test_add("/visitor/input/list",
                            &in_visitor_data, test_visitor_in_list);
-    input_visitor_test_add("/visitor/input/union",
-                            &in_visitor_data, test_visitor_in_union);
     input_visitor_test_add("/visitor/input/union-flat",
                            &in_visitor_data, test_visitor_in_union_flat);
     input_visitor_test_add("/visitor/input/union-anon",

tests/test-qmp-output-visitor.c

@@ -422,40 +422,6 @@ static void test_visitor_out_list_qapi_free(TestOutputVisitorData *data,
     qapi_free_UserDefNestedList(head);
 }
 
-static void test_visitor_out_union(TestOutputVisitorData *data,
-                                   const void *unused)
-{
-    QObject *arg, *qvalue;
-    QDict *qdict, *value;
-
-    Error *err = NULL;
-
-    UserDefUnion *tmp = g_malloc0(sizeof(UserDefUnion));
-    tmp->kind = USER_DEF_UNION_KIND_A;
-    tmp->integer = 41;
-    tmp->a = g_malloc0(sizeof(UserDefA));
-    tmp->a->boolean = true;
-
-    visit_type_UserDefUnion(data->ov, &tmp, NULL, &err);
-    g_assert(err == NULL);
-    arg = qmp_output_get_qobject(data->qov);
-
-    g_assert(qobject_type(arg) == QTYPE_QDICT);
-    qdict = qobject_to_qdict(arg);
-
-    g_assert_cmpstr(qdict_get_str(qdict, "type"), ==, "a");
-    g_assert_cmpint(qdict_get_int(qdict, "integer"), ==, 41);
-
-    qvalue = qdict_get(qdict, "data");
-    g_assert(data != NULL);
-    g_assert(qobject_type(qvalue) == QTYPE_QDICT);
-    value = qobject_to_qdict(qvalue);
-    g_assert_cmpint(qdict_get_bool(value, "boolean"), ==, true);
-
-    qapi_free_UserDefUnion(tmp);
-    QDECREF(qdict);
-}
-
 static void test_visitor_out_union_flat(TestOutputVisitorData *data,
                                         const void *unused)
 {
@@ -862,8 +828,6 @@ int main(int argc, char **argv)
                             &out_visitor_data, test_visitor_out_list);
     output_visitor_test_add("/visitor/output/list-qapi-free",
                             &out_visitor_data, test_visitor_out_list_qapi_free);
-    output_visitor_test_add("/visitor/output/union",
-                            &out_visitor_data, test_visitor_out_union);
     output_visitor_test_add("/visitor/output/union-flat",
                             &out_visitor_data, test_visitor_out_union_flat);
     output_visitor_test_add("/visitor/output/union-anon",