Re: [Qemu-devel] [PATCH v7 14/15] qapi: Allow anonymous branch types in flat union

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Recent commits added support for an anonymous type as the base
> of a flat union; with a bit more work, we can also allow an
> anonymous struct as a branch of a flat union.  This probably
> most useful when a branch adds no additional members beyond the
> common elements of the base (that is, the branch struct is '{}'),
> but can be used for any struct in the same way we allow for an
> anonymous struct for a command.
>
> The generator has to do a bit of special-casing for the fact that
> we do not emit a '_empty' struct nor a 'visit_type__empty_members()'
> corresponding to the special ':empty' type; but when the branch
> is truly empty, there's nothing to do.

Well, it could emit them, if it makes things easier.

> The testsuite gets an update to use the new feature, and to ensure
> that we can still detect invalid collisions of QMP names.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v7: new patch
> ---
>  scripts/qapi.py                          | 21 +++++++++++++++------
>  scripts/qapi-types.py                    | 10 +++++++---
>  scripts/qapi-visit.py                    | 14 ++++++++++----
>  tests/Makefile                           |  1 +
>  tests/qapi-schema/flat-union-inline.err  |  2 +-
>  tests/qapi-schema/flat-union-inline.json |  5 ++---
>  tests/qapi-schema/qapi-schema-test.json  |  5 +++--
>  tests/qapi-schema/qapi-schema-test.out   |  8 ++++++--
>  tests/qapi-schema/union-inline.err       |  1 +
>  tests/qapi-schema/union-inline.exit      |  1 +
>  tests/qapi-schema/union-inline.json      |  4 ++++
>  tests/qapi-schema/union-inline.out       |  0
>  12 files changed, 51 insertions(+), 21 deletions(-)
>  create mode 100644 tests/qapi-schema/union-inline.err
>  create mode 100644 tests/qapi-schema/union-inline.exit
>  create mode 100644 tests/qapi-schema/union-inline.json
>  create mode 100644 tests/qapi-schema/union-inline.out
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 7d568d9..4c531e7 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -607,9 +607,11 @@ def check_union(expr, expr_info):
>      for (key, value) in members.items():
>          check_name(expr_info, "Member of union '%s'" % name, key)
>
> -        # Each value must name a known type
> +        # Each value must name a type; although the type may be anonymous
> +        # for a flat union.
>          check_type(expr_info, "Member '%s' of union '%s'" % (key, name),
> -                   value, allow_array=not base, allow_metas=allow_metas)
> +                   value, allow_array=not base, allow_dict=base is not None,
> +                   allow_optional=True, allow_metas=allow_metas)
>
>          # If the discriminator names an enum type, then all members
>          # of 'data' must also be members of the enum type.
> @@ -1061,6 +1063,9 @@ class QAPISchemaMember(object):
>                  return '(parameter of %s)' % owner[:-4]
>              elif owner.endswith('-base'):
>                  return '(base of %s)' % owner[:-5]
> +            elif owner.endswith('-branch'):
> +                return ('(member of %s branch %s)'
> +                        % tuple(owner[:-7].split(':')))

I think we should point to the spot that puts in the colon, and back.

Do we really need the "of %s" part?

>              else:
>                  assert owner.endswith('-wrapper')
>                  # Unreachable and not implemented
> @@ -1335,7 +1340,11 @@ class QAPISchema(object):
>                                                self._make_members(data, info),
>                                                None))
>
> -    def _make_variant(self, case, typ):
> +    def _make_variant(self, case, typ, info, owner):
> +        if isinstance(typ, dict):
> +            typ = self._make_implicit_object_type(
> +                "%s:%s" % (owner, case), info, 'branch',
> +                self._make_members(typ, info)) or 'q_empty'

This is the spot.

Is this the best spot for creating the type?  Precedent:
_make_simple_variant() creates a wrapper type.  Because of that, I'm
inclined to answer yes.  But let's examine the caller.  They're visible
right below.

>          return QAPISchemaObjectTypeVariant(case, typ)
>
>      def _make_simple_variant(self, case, typ, info):
> @@ -1356,7 +1365,7 @@ class QAPISchema(object):
>              base = (self._make_implicit_object_type(
>                      name, info, 'base', self._make_members(base, info)))
>          if tag_name:
> -            variants = [self._make_variant(key, value)
> +            variants = [self._make_variant(key, value, info, name)
>                          for (key, value) in data.iteritems()]
>              members = []
>          else:

This is the caller we need to extend to cope with an anonymous type.
The anonymous type arrives in form of @value being a dict rather than a
type name.  Possible, because the change to check_union() now permits
dict in addition to string.

> @@ -1375,7 +1384,7 @@ class QAPISchema(object):
>      def _def_alternate_type(self, expr, info):
>          name = expr['alternate']
>          data = expr['data']
> -        variants = [self._make_variant(key, value)
> +        variants = [self._make_variant(key, value, info, name)
>                      for (key, value) in data.iteritems()]
>          tag_member = QAPISchemaObjectTypeMember('type', 'QType', False)
>          self._def_entity(

Here, dict still can't happen, but passing more argument's isn't exactly
a burden.

> @@ -1485,7 +1494,7 @@ def c_enum_const(type_name, const_name, prefix=None):
>          type_name = prefix
>      return camel_to_upper(type_name) + '_' + c_name(const_name, 
> False).upper()
>
> -c_name_trans = string.maketrans('.-', '__')
> +c_name_trans = string.maketrans('.-:', '___')

Because you use the colon as separator.  Hmm.

>
>
>  # Map @name to a valid C identifier.
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 5a9e2da..f1edab2 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -61,7 +61,8 @@ def gen_object(name, base, members, variants):
   def gen_object(name, base, members, variants):
       if name in objects_seen:
           return ''
       objects_seen.add(name)

>      ret = ''
>      if variants:
>          for v in variants.variants:
> -            if isinstance(v.type, QAPISchemaObjectType):
> +            if (isinstance(v.type, QAPISchemaObjectType)
> +                    and not (v.type.is_implicit() and v.type.is_empty())):
>                  ret += gen_object(v.type.name, v.type.base,
>                                    v.type.local_members, v.type.variants)
>

This is the recursion that ensures an object type's variant member types
are emitted before the object type.

We can't simply .type == schema.the_empty_object_type like
qapi-introspect.py does, because we don't have schema handy here.  Hmm.

Do we really need this change?  Note that gen_object() does nothing for
name in objects_seen, and we do this in visit_begin():

        # gen_object() is recursive, ensure it doesn't visit the empty type
        objects_seen.add(schema.the_empty_object_type.name)

> @@ -123,11 +124,14 @@ def gen_variants(variants):
>                  c_name=c_name(variants.tag_member.name))
>
>      for var in variants.variants:

Here, we emit the C union member for a variant:

> +        typ = var.type.c_unboxed_type()
> +        if (isinstance(var.type, QAPISchemaObjectType) and
> +                var.type.is_empty() and var.type.is_implicit()):
> +            typ = 'char'
>          ret += mcgen('''
>          %(c_type)s %(c_name)s;
>  ''',
> -                     c_type=var.type.c_unboxed_type(),
> -                     c_name=c_name(var.name))
> +                     c_type=typ, c_name=c_name(var.name))

Your change replaces the C type when var.type is the empty type.
Without that, we'd get 'q_empty', I guess.

Do we need the union member?  Hmm, we may want to avoid empty unions, to
avoid pedantic warnings.

Should we make the_empty_object_type.c_unboxed_type() return a suitable
C type instead of 'q_empty'?

>
>      ret += mcgen('''
>      } u;
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 07ae6d1..46f8b39 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -79,13 +79,19 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s 
> *obj, Error **errp)
>          for var in variants.variants:
>              ret += mcgen('''
>      case %(case)s:

Here, we emit the visit of the "active" C union member:

> -        visit_type_%(c_type)s_members(v, &obj->u.%(c_name)s, &err);
> -        break;
>  ''',
>                           case=c_enum_const(variants.tag_member.type.name,
>                                             var.name,
> -                                           variants.tag_member.type.prefix),
> -                         c_type=var.type.c_name(), c_name=c_name(var.name))
> +                                           variants.tag_member.type.prefix))
> +            if (not isinstance(var.type, QAPISchemaObjectType) or
> +                    not var.type.is_empty()):
> +                ret += mcgen('''
> +        visit_type_%(c_type)s_members(v, &obj->u.%(c_name)s, &err);
> +''',
> +                             c_type=var.type.c_name(), 
> c_name=c_name(var.name))
> +            ret += mcgen('''
> +        break;
> +''')

Your change suppresses the visit_type_FOO_members() call when var.type
is the empty type.  Without that, we'd get visit_type_q_empty_members(),
which doesn't exist.

Why not make it exist?

>
>          ret += mcgen('''
>      default:
> diff --git a/tests/Makefile b/tests/Makefile
> index 5cd6177..d7d9597 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -379,6 +379,7 @@ qapi-schema += union-base-no-discriminator.json
>  qapi-schema += union-branch-case.json
>  qapi-schema += union-clash-branches.json
>  qapi-schema += union-empty.json
> +qapi-schema += union-inline.json
>  qapi-schema += union-invalid-base.json
>  qapi-schema += union-optional-branch.json
>  qapi-schema += union-unknown.json
> diff --git a/tests/qapi-schema/flat-union-inline.err 
> b/tests/qapi-schema/flat-union-inline.err
> index 2333358..efcafec 100644
> --- a/tests/qapi-schema/flat-union-inline.err
> +++ b/tests/qapi-schema/flat-union-inline.err
> @@ -1 +1 @@
> -tests/qapi-schema/flat-union-inline.json:7: Member 'value1' of union 
> 'TestUnion' should be a type name
> +tests/qapi-schema/flat-union-inline.json:6: 'kind' (member of TestUnion 
> branch value2) collides with 'kind' (member of Base)
> diff --git a/tests/qapi-schema/flat-union-inline.json 
> b/tests/qapi-schema/flat-union-inline.json
> index 62c7cda..a049ec8 100644
> --- a/tests/qapi-schema/flat-union-inline.json
> +++ b/tests/qapi-schema/flat-union-inline.json
> @@ -1,5 +1,4 @@
> -# we require branches to be a struct name
> -# TODO: should we allow anonymous inline branch types?
> +# we allow anonymous union branches, but they must not have clashing names
>  { 'enum': 'TestEnum',
>    'data': [ 'value1', 'value2' ] }
>  { 'struct': 'Base',
> @@ -8,4 +7,4 @@
>    'base': 'Base',
>    'discriminator': 'enum1',
>    'data': { 'value1': { 'string': 'str' },
> -            'value2': { 'integer': 'int' } } }
> +            'value2': { 'kind': 'int' } } }
> diff --git a/tests/qapi-schema/qapi-schema-test.json 
> b/tests/qapi-schema/qapi-schema-test.json
> index df91f3d..5128b49 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -23,7 +23,7 @@
>  # for testing override of default naming heuristic
>  { 'enum': 'QEnumTwo',
>    'prefix': 'QENUM_TWO',
> -  'data': [ 'value1', 'value2' ] }
> +  'data': [ 'value1', 'value2', 'value3' ] }
>
>  # for testing nested structs
>  { 'struct': 'UserDefOne',
> @@ -81,7 +81,8 @@
>    'base': { '*integer': 'int', 'string': 'str', 'enum1': 'QEnumTwo' },
>    'discriminator': 'enum1',
>    'data': { 'value1' : 'UserDefC', # intentional forward reference
> -            'value2' : 'UserDefB' } }

You're deleting the equally intentional case of a backward reference :)

> +            'value2' : { },
> +            'value3' : { 'boolean': 'bool', '*number': 'number' } } }
>
>  { 'struct': 'WrapAlternate',
>    'data': { 'alt': 'UserDefAlternate' } }
> diff --git a/tests/qapi-schema/qapi-schema-test.out 
> b/tests/qapi-schema/qapi-schema-test.out
> index 8a00c6b..7fac2da 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -50,7 +50,7 @@ object NestedEnumsOne
>      member enum2: EnumOne optional=True
>      member enum3: EnumOne optional=False
>      member enum4: EnumOne optional=True
> -enum QEnumTwo ['value1', 'value2']
> +enum QEnumTwo ['value1', 'value2', 'value3']
>      prefix QENUM_TWO
>  enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 
> 'qbool']
>      prefix QTYPE
> @@ -82,7 +82,8 @@ object UserDefFlatUnion2
>      base q_obj_UserDefFlatUnion2-base
>      tag enum1
>      case value1: UserDefC
> -    case value2: UserDefB
> +    case value2: q_empty
> +    case value3: q_obj_UserDefFlatUnion2:value3-branch
>  object UserDefNativeListUnion
>      member type: UserDefNativeListUnionKind optional=False
>      tag type
> @@ -179,6 +180,9 @@ object q_obj_UserDefFlatUnion2-base
>      member integer: int optional=True
>      member string: str optional=False
>      member enum1: QEnumTwo optional=False
> +object q_obj_UserDefFlatUnion2:value3-branch
> +    member boolean: bool optional=False
> +    member number: number optional=True
>  object q_obj___org.qemu_x-command-arg
>      member a: __org.qemu_x-EnumList optional=False
>      member b: __org.qemu_x-StructList optional=False
> diff --git a/tests/qapi-schema/union-inline.err 
> b/tests/qapi-schema/union-inline.err
> new file mode 100644
> index 0000000..6c5389a
> --- /dev/null
> +++ b/tests/qapi-schema/union-inline.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/union-inline.json:2: Member 'value1' of union 'TestUnion' 
> should be a type name
> diff --git a/tests/qapi-schema/union-inline.exit 
> b/tests/qapi-schema/union-inline.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/union-inline.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/union-inline.json 
> b/tests/qapi-schema/union-inline.json
> new file mode 100644
> index 0000000..b8c5df6
> --- /dev/null
> +++ b/tests/qapi-schema/union-inline.json
> @@ -0,0 +1,4 @@
> +# simple unions cannot have anonymous branches (only flat unions can)
> +{ 'union': 'TestUnion',
> +  'data': { 'value1': { 'string': 'str' },
> +            'value2': { 'kind': 'int' } } }
> diff --git a/tests/qapi-schema/union-inline.out 
> b/tests/qapi-schema/union-inline.out
> new file mode 100644
> index 0000000..e69de29