Re: [Qemu-devel] RFC: 'alternate' qapi top-level expression [was: [PATCH v4 12/19] qapi: Add some type check tests]

[Markus Armbruster]

Cc'ing Kevin as fair punishment for is previous work on QAPI code
generation in general, and union types in particular.

Eric Blake <address@hidden> writes:

> [revisiting this series, finally!]
>
> On 09/25/2014 10:19 AM, Markus Armbruster wrote:
>
>>> Return of an anon union isn't used yet, but _might_ make sense (as the
>>> only feasible way of changing existing commands that return an array or
>>> primitive extensible to instead return a dict) - 
>> 
>> Good point.
>> 
>>>                                                  except that back-compat
>>> demands that we can't return a dict in place of a primitive unless the
>>> arguments of the command are also enhanced (that is, older callers are
>>> not expecting a dict, so we can't return a dict unless the caller
>>> witnesses they are new enough by explicitly asking for a dict return).
>> 
>> I think we can keep things simple for now and reject anonymous unions.
>> We can always relax the check when we run into a use.
>
> In trying to code up what it would take to easily reject anonymous
> unions from a return type, I'm realizing that it would be smarter to
> quit mixing anonymous unions in with other unions.
>
> Refresher course: on the wire, both a regular union:
>
> QAPI:
>  { 'type': 'Type1', 'data': { 'value': 'int' } }
>  { 'union': 'Foo', 'data': { 'a': 'Type1', 'b': 'Type2' } }
>  { 'command': 'bar', 'data': 'Foo' }
> Wire:
>  { "execute": "bar", "arguments": { "type": "a",
>    "data": { "value": 1 } } }
>
> and a flat union:
>
> QAPI:
>  { 'type': 'Type1', 'data': { 'value': 'int' } }
>  { 'enum': 'Enum', 'data': [ 'a', 'b' ] }
>  { 'type': 'Base', { 'switch': 'Enum' } }
>  { 'union': 'Foo', 'base': 'Base', 'discriminator': 'switch',
>    'data': { 'a': 'Type1', 'b': 'Type2' } }
>  { 'command': 'bar', 'data': 'Foo' }
> Wire:
>  { "execute": "bar", "arguments": { "switch": "a",
>    "value": 1 } }
>
> happen to guarantee a top-level dictionary (plain unions always have a
> two-element dictionary, flat unions are required to have a base type
> which is itself a dictionary).

Yes.

>                                 But an anonymous union is explicitly
> allowing a multi-type variable, where the determination of which branch
> of the union is made by the type of the variable.  Furthermore, we do
> not allow two branches to have the same type,

Even more severe: the JSON types have to be different!  Thus, at most
one can be a complex or union type, and at most one can be a string or
enum type.

>                                               so at least one branch
> must be a non-dictionary;

Correct.

>                           but as _all_ QMP commands currently take a
> dictionary for the "arguments" key, we do not want to allow:
>
> QAPI:
>  { 'type': 'Type1', 'data': { 'value': 'int' } }
>  { 'union': 'Foo', 'discriminator': {},
>    'data': { 'a': 'Type1', 'b': 'int' } }
>  { 'command': 'bar', 'data': 'Foo' }
> Wire:
>  { "execute": "bar", "arguments": 1 }

Yes, let's stay out of the generalization tar pits and stick to named
parameters, i.e. JSON object arguments.

> Tracking all three qapi expressions as union types is making the
> generator code a bit verbose, especially since the code generation for
> all three is so distinct.
>
>
> Proposal: I am proposing that we convert to an alternate syntax for what
> we now term as anonymous unions.  It will not have any impact to the
> wire representation of QMP, only to the qapi code generators.  The
> proposal is simple: instead of using "'union':'Name',
> 'discriminator':{}", we instead use "'alternate': 'Foo'" when declaring
> a type as an anonymous union (which, for obvious reasons, I would then
> update the documentation to call an "alternate" instead of an "anonymous
> union").

This separates tagged and untagged unions more clearly: reserve 'union'
for the two kinds of tagged unions, switch the untagged union to
'alternate'.

I don't mind.  Kevin, any objections?

> I'll go ahead and propose the patches (I've already done the bulk of the
> conversion work, to prove that not many files were affected).

I'll gladly review.