Re: [Qemu-devel] [PATCH v5 06/28] qapi: Add some union tests

[Markus Armbruster <armbru@redhat.com>]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 26.03.2015 um 16:04 hat Eric Blake geschrieben:
>> On 03/26/2015 07:18 AM, Markus Armbruster wrote:
>> > Eric Blake <eblake@redhat.com> writes:
>> >>                     Meanwhile, it would be nice to allow
>> >> 'discriminator':'EnumType' without 'base' for creating a simple union
>> >> that is type-safe rather than open-coded to a generated enum (right
>> >> now, we are only type-safe when using a flat union, but that uses
>> >> a different syntax of 'discriminator':'member-name' which requires
>> >> a base class containing a 'member-name' enum field).
>> > 
>> > I'm afraid I don't get you.  Can you give examples?
>> 
>> Using this common code with the appropriate union for each example:
>> { 'command':'foo', 'data':UNION }
>> 
>> Right now, we have flat unions which are required to be type-safe (all
>> branches MUST map back to the enum type of the discriminator, enforced
>> by the generator, so that if the enum later adds a member, the union
>> must also be updated to match):
>> 
>> [1]
>> { 'union':'Safe', 'base':'Base', 'discriminator':'switch',
>>   'data':{ 'one':'str', 'two':'int' }}
>> 
>> {"execute":"foo", "arguments":{'switch':'one', 'data':'hello'}}
>> 
>> and simple unions which cannot be typesafe (the branches of the union
>> are open-coded - even if they correlate to an existing enum, there is
>> nothing enforcing that extensions to the enum be reflected into the union):
>> 
>> [2]
>> { 'union':'SimpleButOpenCoded',
>>   'data':{ 'one': 'str', 'two':'int' }}
>> 
>> {"execute":"foo", "arguments":{'type':'one', 'data':'hello'}}
>> 
>> I'm hoping to add as a followup series a variant of simple unions that
>> is type-safe:
>> 
>> [3]
>> { 'union':'SimpleAndSafe', 'discriminator':'MyEnum',
>>   'data':{ 'one':'str', 'two':'int' }}
>> 
>> {"execute":"foo", "arguments":{'type':'one', 'data':'hello'}}
>
> This would overload 'discriminator' with two different meanings:
>
> * In case [1] it refers to one key in the JSON object which contains the
>   name of the union branch to select. That is, it is the _name_ of the
>   key used as a discriminator.
>
> * In case [3], the 'type' key is used in the JSON object to select a
>   union branch. 'discriminator' describes the _valid values_ of it, i.e.
>   the branch names.
>
> We shouldn't mix these meanings. If you need [3], we could call it
> 'discriminator-type' or something like that. If both options are
> consistently used for simple and flat unions, you end up with these
> rules:
>
> * 'discriminator' is the key that is used to select the union branch.
>
>   - For flat unions it is required and must refer to an explicitly
>     declared key in the base type.
>
>   - For simple unions it is optional and defaults to 'type'. The key is
>     implicitly created in the union type.
>
> * 'discrimiator-type' describes the valid values of 'discriminator',
>   either by referring to an enum type or to 'str'.
>
>   - For flat unions, this must match the type of the explicit
>     declaration of the discriminator field. It is optional and defauls
>     to the only valid value.
>
>   - For simple unions it is optional, too, and defaults to 'str'.
>
>   - If it is the name of an enum type, that enum type is reused and the
>     declared union branches must match the valid values of the enum.
>
>   - If it is 'str', a new enum is generated, and all the declared union
>     branches are used as valid values.
>
> There's quite some duplication in it where we need to make sure that the
> schema matches in all places, but without an explicit declaration of the
> disriminator field in simple unions, this seems to be the best I can
> come up with.
>
>> But the existing, unused-except-in-testsuite, notion of a simple union
>> with a base class looks like:
>> 
>> [4]
>> { 'type':'Shared', 'data':{'common':'int'}}
>> { 'union':'SimpleWithBase', 'base':'Shared',
>>   'data':{ 'one':'str', 'two':'int' }}
>> 
>> {"execute":"foo", "arguments":{'common':1, 'type':'one', 'data':'hello'}}
>> 
>> If we were to allow the addition of 'discriminator':'EnumType' to a
>> simple union [3], but then add that discriminator to an existing case of
>> a simple union with base [4], it would look like:
>> 
>> { 'type':'Shared', 'data':{'common':'int'}}
>> { 'union':'SimpleWithBaseAndDiscriminator', 'base':'Shared',
>>   'discriminator':'MyEnum',
>>   'data':{ 'one':'str', 'two':'int' }}
>> 
>> Yuck.  That is indistinguishable from flat unions [1], except by whether
>> discriminator names an enum type or a member of the base class.
>
> Which could even be ambiguous, couldn't it?
>
>> > In particular, I can define simple unions in terms of flat ones by
>> > restricting all union cases to a single member named 'data'.  They're
>> > not implemented that way, but that's a historical accident.  Simple
>> > unions are a redundant concept.
>> 
>> Cool.  Or more concretely,
>> 
>> { 'union': 'Simple', 'data': { 'one': 'str', 'two': 'int' } }
>> 
>> is identical on the wire to:
>> 
>> { 'enum': 'MyEnum', 'data': ['one', 'two'] }
>> { 'type': 'Base', 'data': { 'type': 'MyEnum' } }
>> { 'type': 'Branch1', 'data': { 'data': 'str' } }
>> { 'type': 'Branch2', 'data': { 'data': 'int' } }
>> { 'union': 'Flat', 'base': 'Base', 'discriminator': 'type',
>>   'data': { 'one': 'Branch1', 'two': 'Branch2' } }
>
> Perhaps we should expose all unions for schema introspection in a way
> similar to this (possibly not splitting out Branch1 and Branch2 as
> independent types, though).

My current thinking on this is a bit more radical.  I suspect there's a
straightforward object type buried in this mess, struggling to get out:
the good old variant record.  It consists of

* a name

* an optional base type name (must be a object type without variants)

* list of own members (empty okay)

  Effective members = own members + effective base type members

* optionally variants

  - one of the effective members is the discriminator (must be enum)

  - for each discriminator value a list of variant members (empty okay)

All existing type/union types are specializations:

* The complex type (struct type) is an object type without variants.

* The simple union type is an object type

  - without a base type

  - with an implicitly defined own member of an implicitly defined
    enumeration type serving as discriminator

  - with no other own members

  - with a single variant member for each discriminator value

* The flat union type is an object type

  - with a base type

  - without own members

  - with a discriminator

I further suspect lowering these types to the general form will make the
generator simpler, not just the introspection.

>                             We would just have to give a name to
> implicitly generated enums and base types.

Introspection doesn't care whether we defined something implicitly or
explicitly.  Let's make up names to hide that.

I'm trying to get a proof-of-concept introspection patch working this
week.  It'll probably be ugly enough to curdle the milk in your tea.