[v2,16/19] qapi: Allow anonymous base for flat union

Rather than requiring all flat unions to explicitly create
a separate base struct, we can allow the qapi schema to specify
the common members via an inline dictionary. This is similar to
how commands can specify an inline anonymous type for its 'data',
and matches the fact that our code base already has several
flat unions that had to create a separate base type that is used
nowhere but in the union.

The patch also has to tweak things to avoid calling base.c_name()
on an implicit type (since implicit types do not have a name);
we already have qapi_FOO_base() which does the trick nicely.

Now that anonymous bases are legal, we need to rework the
flat-union-bad-base negative test (as previously written, it
forms what is now valid QAPI; tweak it to now provide coverage
of a new error message path), and add a positive test in
qapi-schema-test to use an anonymous base.

Note that this patch only allows anonymous bases for flat
unions; simple unions are enough syntactic sugar that we do
not want to burden them further.  Meanwhile, it would be easy
to modify qapi.py to also allow an anonymous base for structs;
however, there is less of a compelling technical reason to
do so, since you can always write the struct to directly
contain any members that the anonymous base would have
mentioned.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v2: rebase onto s/fields/members/ change
v1: rebase and rework to use gen_visit_fields_call(), test new error
Previously posted as part of qapi cleanup subset F:
v6: avoid redundant error check in gen_visit_union(), rebase to
earlier gen_err_check() improvements
---
 scripts/qapi.py                            | 11 +++++++++--
 scripts/qapi-types.py                      | 12 +++++++-----
 scripts/qapi-visit.py                      |  6 +++---
 docs/qapi-code-gen.txt                     | 28 ++++++++++++++--------------
 tests/qapi-schema/flat-union-bad-base.err  |  2 +-
 tests/qapi-schema/flat-union-bad-base.json |  5 ++---
 tests/qapi-schema/qapi-schema-test.json    |  6 +-----
 tests/qapi-schema/qapi-schema-test.out     |  9 ++++-----
 8 files changed, 41 insertions(+), 38 deletions(-)

Eric Blake <eblake@redhat.com> writes:

> Rather than requiring all flat unions to explicitly create
> a separate base struct, we can allow the qapi schema to specify
> the common members via an inline dictionary. This is similar to
> how commands can specify an inline anonymous type for its 'data',

Suggest to end the sentence here, and then...

> and matches the fact that our code base already has several
> flat unions that had to create a separate base type that is used
> nowhere but in the union.

"We already have several struct types that only exist to serve as a
single flat union's base.  The next commits will clean them up."

Replace "them" by "some" if you don't clean them all up.

It's a nice step towards having a variant record type in the schema
language similar to what we have in introspection.

> The patch also has to tweak things to avoid calling base.c_name()
> on an implicit type (since implicit types do not have a name);
> we already have qapi_FOO_base() which does the trick nicely.
>
> Now that anonymous bases are legal, we need to rework the
> flat-union-bad-base negative test (as previously written, it
> forms what is now valid QAPI; tweak it to now provide coverage
> of a new error message path), and add a positive test in
> qapi-schema-test to use an anonymous base.
>
> Note that this patch only allows anonymous bases for flat
> unions; simple unions are enough syntactic sugar that we do
> not want to burden them further.

Indeed.

>                                   Meanwhile, it would be easy
> to modify qapi.py to also allow an anonymous base for structs;
> however, there is less of a compelling technical reason to
> do so, since you can always write the struct to directly
> contain any members that the anonymous base would have
> mentioned.

Suggest something like

      Note that while it would be easy to also allow an anonymous base
      for structs, that would be quite redundant, jsut put the members
      right into the struct instead.

> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v2: rebase onto s/fields/members/ change
> v1: rebase and rework to use gen_visit_fields_call(), test new error
> Previously posted as part of qapi cleanup subset F:
> v6: avoid redundant error check in gen_visit_union(), rebase to
> earlier gen_err_check() improvements
> ---
>  scripts/qapi.py                            | 11 +++++++++--
>  scripts/qapi-types.py                      | 12 +++++++-----
>  scripts/qapi-visit.py                      |  6 +++---
>  docs/qapi-code-gen.txt                     | 28 ++++++++++++++--------------
>  tests/qapi-schema/flat-union-bad-base.err  |  2 +-
>  tests/qapi-schema/flat-union-bad-base.json |  5 ++---
>  tests/qapi-schema/qapi-schema-test.json    |  6 +-----
>  tests/qapi-schema/qapi-schema-test.out     |  9 ++++-----
>  8 files changed, 41 insertions(+), 38 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 38121c5..7c76442 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -329,6 +329,8 @@ class QAPISchemaParser(object):
>
>
>  def find_base_members(base):
> +    if isinstance(base, dict):
> +        return base
>      base_struct_define = find_struct(base)
>      if not base_struct_define:
>          return None
> @@ -562,9 +564,9 @@ def check_union(expr, expr_info):
>
>      # Else, it's a flat union.
>      else:
> -        # The object must have a string member 'base'.
> +        # The object must have a string or dictionary 'base'.
>          check_type(expr_info, "'base' for union '%s'" % name,
> -                   base, allow_metas=['struct'])
> +                   base, allow_dict=True, allow_metas=['struct'])
>          if not base:
>              raise QAPIExprError(expr_info,
>                                  "Flat union '%s' must have a base"

We still have a lot of semantic analysis code to move into class
QAPISchema.

> @@ -1039,6 +1041,8 @@ class QAPISchemaMember(object):
>              owner = owner[5:]
>              if owner.endswith('-arg'):
>                  return '(parameter of %s)' % owner[:-4]
> +            elif owner.endswith('-base'):
> +                return '(base of %s)' % owner[:-5]
>              else:
>                  assert owner.endswith('-wrapper')
>                  # Unreachable and not implemented
> @@ -1323,6 +1327,9 @@ class QAPISchema(object):
>          base = expr.get('base')
>          tag_name = expr.get('discriminator')
>          tag_member = None
> +        if isinstance(base, dict):
> +            base = (self._make_implicit_object_type(
> +                    name, info, 'base', self._make_members(base, info)))
>          if tag_name:
>              variants = [self._make_variant(key, value)
>                          for (key, value) in data.iteritems()]
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 1f090e6..161443e 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -72,13 +72,15 @@ struct %(c_name)s {
>  ''',
>                   c_name=c_name(name))
>
> -    if base:
> -        ret += mcgen('''
> +    if base and not base.is_empty():

I think suppressing the comments for empty base is more closely related
to PATCH 11..13 than to this patch.

> +        if not base.is_implicit():
> +            ret += mcgen('''
>      /* Members inherited from %(c_name)s: */
>  ''',
> -                     c_name=base.c_name())
> +                         c_name=base.c_name())
>          ret += gen_struct_members(base.members)
> -        ret += mcgen('''
> +        if not base.is_implicit():
> +            ret += mcgen('''
>      /* Own members: */
>  ''')
>      ret += gen_struct_members(members)
> @@ -238,7 +240,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>      def visit_object_type(self, name, info, base, members, variants):
>          self._fwdecl += gen_fwd_object_or_array(name)
>          self.decl += gen_object(name, base, members, variants)
> -        if base:
> +        if base and not base.is_implicit():
>              self.decl += gen_upcast(name, base)
>          self._gen_type_cleanup(name)
>
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index a17ecc1..aa244cd 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -34,13 +34,12 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp);
>                   c_name=c_name(name))
>
>
> -def gen_visit_members_call(typ, direct_name, implicit_name=None):
> +def gen_visit_members_call(typ, direct_name, implicit_name):

@implicit_name introduced optional in PATCH 12.  I'd make it mandatory
from the start.

>      ret = ''
>      assert isinstance(typ, QAPISchemaObjectType)
>      if typ.is_empty():
>          pass
>      elif typ.is_implicit():
> -        assert implicit_name

Also fold into PATCH 12 then.

>          assert not typ.variants
>          ret += gen_visit_members(typ.members, prefix=implicit_name)
>      else:
> @@ -63,7 +62,8 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
>                  c_name=c_name(name))
>
>      if base:
> -        ret += gen_visit_members_call(base, '(%s *)obj' % base.c_name())
> +        ret += gen_visit_members_call(base, 'qapi_%s_base(obj)' % c_name(name),

I started at this for several minutes until I could guess what's going
on here.

The old code works fine when the type isn't implicit.

When it is, it fails the assertion in base.c_name(), even though
gen_visit_members_call() is not going to use its value.

You hack around it by passing 'qapi_NAME_base(obj)' instead.

If NAME isn't implicit, the function exists, and does the same as the
expression it replaces.

If NAME is implicit, the function doesn't exist, but
gen_visit_members_call() doesn't care, because it doesn't use the
argument then.

Ugh!  More evidence that we better not munge the two cases together into
one function.

> +                                      'obj->')

Argument needed because base can be implicit now.

>
>      ret += gen_visit_members(members, prefix='obj->')
>
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index 2519c07..1c8f113 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -283,7 +283,7 @@ better than open-coding the field to be type 'str'.
>  === Union types ===
>
>  Usage: { 'union': STRING, 'data': DICT }
> -or:    { 'union': STRING, 'data': DICT, 'base': STRUCT-NAME,
> +or:    { 'union': STRING, 'data': DICT, 'base': STRUCT-NAME-OR-DICT,
>           'discriminator': ENUM-MEMBER-OF-BASE }
>
>  Union types are used to let the user choose between several different
> @@ -319,13 +319,16 @@ an implicit C enum 'NameKind' is created, corresponding to the union
>  the union can be named 'max', as this would collide with the implicit
>  enum.  The value for each branch can be of any type.
>
> -A flat union definition specifies a struct as its base, and
> -avoids nesting on the wire.  All branches of the union must be
> -complex types, and the top-level fields of the union dictionary on
> -the wire will be combination of fields from both the base type and the
> -appropriate branch type (when merging two dictionaries, there must be
> -no keys in common).  The 'discriminator' field must be the name of an
> -enum-typed member of the base struct.
> +A flat union definition avoids nesting on the wire, and specifies a
> +set of common fields that occur in all variants of the union.  The
> +'base' key must specifiy either a type name (the type must be a
> +struct, not a union), or a dictionary representing an anonymous type.
> +All branches of the union must be complex types, and the top-level
> +fields of the union dictionary on the wire will be combination of
> +fields from both the base type and the appropriate branch type (when
> +merging two dictionaries, there must be no keys in common).  The
> +'discriminator' field must be the name of an enum-typed member of the
> +base struct.
>
>  The following example enhances the above simple union example by
>  adding a common field 'readonly', renaming the discriminator to
> @@ -333,10 +336,8 @@ something more applicable, and reducing the number of {} required on
>  the wire:
>
>   { 'enum': 'BlockdevDriver', 'data': [ 'file', 'qcow2' ] }
> - { 'struct': 'BlockdevCommonOptions',
> -   'data': { 'driver': 'BlockdevDriver', 'readonly': 'bool' } }
>   { 'union': 'BlockdevOptions',
> -   'base': 'BlockdevCommonOptions',
> +   'base': { 'driver': 'BlockdevDriver', 'readonly': 'bool' },
>     'discriminator': 'driver',
>     'data': { 'file': 'FileOptions',
>               'qcow2': 'Qcow2Options' } }

Problematic, because it makes the example deviate further from the real
schema.

It deviates before your patch, but that looks accidental:
BlockdevOptions and BlockdevCommonOptions come from commit
5163149..5163149 (merged in commit 405c97c, July 2013, and by the time
BlockdevOptions made it into the schema (merged in commit 33c6cae,
October 2013), its base was named BlockdevOptionsBase.

Can we change the schema's BlockdevOptions to use the anonymous base
feature?

> @@ -354,7 +355,7 @@ code generator can ensure that branches exist for all values of the
>  enum (although the order of the keys need not match the declaration of
>  the enum).  In the resulting generated C data types, a flat union is
>  represented as a struct with the base member fields included directly,
> -and then a union of structures for each branch of the struct.
> +and then a union of pointers to structures for each branch of the struct.

Uh, that became wrong in commit 544a373 already, didn't it?

Is that a bug in PATCH 3 then?

>
>  A simple union can always be re-written as a flat union where the base
>  class has a single member named 'type', and where each branch of the
> @@ -365,10 +366,9 @@ union has a struct with a single member named 'data'.  That is,
>  is identical on the wire to:
>
>   { 'enum': 'Enum', 'data': ['one', 'two'] }
> - { 'struct': 'Base', 'data': { 'type': 'Enum' } }
>   { 'struct': 'Branch1', 'data': { 'data': 'str' } }
>   { 'struct': 'Branch2', 'data': { 'data': 'int' } }
> - { 'union': 'Flat', 'base': 'Base', 'discriminator': 'type',
> + { 'union': 'Flat': 'base': { 'type': 'Enum' }, 'discriminator': 'type',
>     'data': { 'one': 'Branch1', 'two': 'Branch2' } }
>
>
> diff --git a/tests/qapi-schema/flat-union-bad-base.err b/tests/qapi-schema/flat-union-bad-base.err
> index 79b8a71..bee24a2 100644
> --- a/tests/qapi-schema/flat-union-bad-base.err
> +++ b/tests/qapi-schema/flat-union-bad-base.err
> @@ -1 +1 @@
> -tests/qapi-schema/flat-union-bad-base.json:9: 'base' for union 'TestUnion' should be a type name
> +tests/qapi-schema/flat-union-bad-base.json:8: 'string' (member of TestTypeA) collides with 'string' (base of TestUnion)
> diff --git a/tests/qapi-schema/flat-union-bad-base.json b/tests/qapi-schema/flat-union-bad-base.json
> index e2e622b..74dd421 100644
> --- a/tests/qapi-schema/flat-union-bad-base.json
> +++ b/tests/qapi-schema/flat-union-bad-base.json
> @@ -1,5 +1,4 @@
> -# we require the base to be an existing struct
> -# TODO: should we allow an anonymous inline base type?
> +# we allow anonymous base, but enforce no duplicate keys
>  { 'enum': 'TestEnum',
>    'data': [ 'value1', 'value2' ] }
>  { 'struct': 'TestTypeA',
> @@ -7,7 +6,7 @@
>  { 'struct': 'TestTypeB',
>    'data': { 'integer': 'int' } }
>  { 'union': 'TestUnion',
> -  'base': { 'enum1': 'TestEnum', 'kind': 'str' },
> +  'base': { 'enum1': 'TestEnum', 'string': 'str' },
>    'discriminator': 'enum1',
>    'data': { 'value1': 'TestTypeA',
>              'value2': 'TestTypeB' } }
> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
> index 33e8517..a6989d8 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -75,14 +75,10 @@
>    'base': 'UserDefZero',
>    'data': { 'string': 'str', 'enum1': 'EnumOne' } }
>
> -{ 'struct': 'UserDefUnionBase2',
> -  'base': 'UserDefZero',
> -  'data': { 'string': 'str', 'enum1': 'QEnumTwo' } }
> -
>  # this variant of UserDefFlatUnion defaults to a union that uses fields with
>  # allocated types to test corner cases in the cleanup/dealloc visitor
>  { 'union': 'UserDefFlatUnion2',
> -  'base': 'UserDefUnionBase2',
> +  'base': { 'string': 'str', 'enum1': 'QEnumTwo' },

You lost member 'integer' from the base's base.  Harmless (I think), but
visible when you compare generated output.

>    'discriminator': 'enum1',
>    'data': { 'value1' : 'UserDefC', # intentional forward reference
>              'value2' : 'UserDefB' } }
> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
> index 6b97fa5..4ba347e 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -8,6 +8,9 @@ object :obj-EVENT_D-arg
>      member b: str optional=False
>      member c: str optional=True
>      member enum3: EnumOne optional=True
> +object :obj-UserDefFlatUnion2-base
> +    member string: str optional=False
> +    member enum1: QEnumTwo optional=False
>  object :obj-__org.qemu_x-command-arg
>      member a: __org.qemu_x-EnumList optional=False
>      member b: __org.qemu_x-StructList optional=False
> @@ -121,7 +124,7 @@ object UserDefFlatUnion
>      case value2: UserDefB
>      case value3: UserDefB
>  object UserDefFlatUnion2
> -    base UserDefUnionBase2
> +    base :obj-UserDefFlatUnion2-base
>      tag enum1
>      case value1: UserDefC
>      case value2: UserDefB
> @@ -166,10 +169,6 @@ object UserDefUnionBase
>      base UserDefZero
>      member string: str optional=False
>      member enum1: EnumOne optional=False
> -object UserDefUnionBase2
> -    base UserDefZero
> -    member string: str optional=False
> -    member enum1: QEnumTwo optional=False
>  object UserDefZero
>      member integer: int optional=False
>  object WrapAlternate

On 03/03/2016 06:04 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Rather than requiring all flat unions to explicitly create
>> a separate base struct, we can allow the qapi schema to specify
>> the common members via an inline dictionary. This is similar to
>> how commands can specify an inline anonymous type for its 'data',
> 
> Suggest to end the sentence here, and then...
> 
>> and matches the fact that our code base already has several
>> flat unions that had to create a separate base type that is used
>> nowhere but in the union.
> 
> "We already have several struct types that only exist to serve as a
> single flat union's base.  The next commits will clean them up."
> 
> Replace "them" by "some" if you don't clean them all up.
> 
> It's a nice step towards having a variant record type in the schema
> language similar to what we have in introspection.
> 

>> @@ -63,7 +62,8 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
>>                  c_name=c_name(name))
>>
>>      if base:
>> -        ret += gen_visit_members_call(base, '(%s *)obj' % base.c_name())
>> +        ret += gen_visit_members_call(base, 'qapi_%s_base(obj)' % c_name(name),
> 
> I started at this for several minutes until I could guess what's going
> on here.
> 
> The old code works fine when the type isn't implicit.
> 
> When it is, it fails the assertion in base.c_name(), even though
> gen_visit_members_call() is not going to use its value.
> 
> You hack around it by passing 'qapi_NAME_base(obj)' instead.
> 
> If NAME isn't implicit, the function exists, and does the same as the
> expression it replaces.
> 
> If NAME is implicit, the function doesn't exist, but
> gen_visit_members_call() doesn't care, because it doesn't use the
> argument then.
> 
> Ugh!  More evidence that we better not munge the two cases together into
> one function.

Even with my v4 work towards exposing implicit types as a concrete
struct, I'm still not creating qapi_NAME_base(obj) for objects with an
implicit type.  But '(_obj_FOO_base *)FOO' works well for a base with a
concrete implicit base type.


>> @@ -354,7 +355,7 @@ code generator can ensure that branches exist for all values of the
>>  enum (although the order of the keys need not match the declaration of
>>  the enum).  In the resulting generated C data types, a flat union is
>>  represented as a struct with the base member fields included directly,
>> -and then a union of structures for each branch of the struct.
>> +and then a union of pointers to structures for each branch of the struct.
> 
> Uh, that became wrong in commit 544a373 already, didn't it?
> 
> Is that a bug in PATCH 3 then?

Yes, and fixed up accordingly in my v3 respin.  (I think it was some
rebase conflicts that I resolved incorrectly at some point).


>> +++ b/tests/qapi-schema/qapi-schema-test.json
>> @@ -75,14 +75,10 @@
>>    'base': 'UserDefZero',
>>    'data': { 'string': 'str', 'enum1': 'EnumOne' } }
>>
>> -{ 'struct': 'UserDefUnionBase2',
>> -  'base': 'UserDefZero',
>> -  'data': { 'string': 'str', 'enum1': 'QEnumTwo' } }
>> -
>>  # this variant of UserDefFlatUnion defaults to a union that uses fields with
>>  # allocated types to test corner cases in the cleanup/dealloc visitor
>>  { 'union': 'UserDefFlatUnion2',
>> -  'base': 'UserDefUnionBase2',
>> +  'base': { 'string': 'str', 'enum1': 'QEnumTwo' },
> 
> You lost member 'integer' from the base's base.  Harmless (I think), but
> visible when you compare generated output.

Easy enough to keep.