Cc'ing Kevin as fair punishment for is previous work on QAPI code
generation in general, and union types in particular.
Eric Blake <ebl...@redhat.com> 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.
