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

[Eric Blake]

On 03/26/2015 07:18 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> Demonstrate that the qapi generator doesn't deal well with unions
>> that aren't up to par. Later patches will update the expected
>> reseults as the generator is made stricter.
>>
>> Of particular note, we currently allow 'base' without 'discriminator'
>> as a way to create a simple union with a base class.  However, none
>> of the existing QMP or QGA interfaces use it (it is exercised only
>> in the testsuite).
> 
> qapi-code-gen.txt documents 'base' only for flat unions.  We should
> either document its use in simple unions, or outlaw it.

I went with outlaw later in the series, and the rest of my commit
message was an attempt to explain my reasoning in that choice.  But I
can certainly try to improve the wording, if we need a respin.

> 
>>                     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'}}


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.

> 
>>                                                       If both 'base'
>> and 'discriminator' are optional, then converting a simple union
>> with base class to a type-safe simple union with an enum discriminator
>> would not be possible.  So my plan is to get rid of 'base' without
>> 'discriminator' later in the series;
> 
> Aha: you're going to outlaw 'base' in simple unions.  Yes, please.

Okay, you came to my desired conclusion; it's just that my wording in
the middle didn't help.

> 
>>                                      this will be no real loss, as any
>> union that needs additional common fields can always use a flat
>> union.
> 
> The mathematical concept behind unions is the sum type.
> 
> We have three implementations, and we call them simple, flat, anonymous.
> 
> Anonymous unions are implicitly tagged.  They come with the obvious
> restrictions required to make implicit tagging work.

and get renamed to 'alternate' later in the series, so they are not
worth worrying about here.

> 
> The other two are explicitly tagged.  The difference between them is
> just notation.  I like my unions flat, because for me the extra wrapping
> is just notational overhead.
> 
> 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' } }

Probably worth mentioning in my commit message.

Hmm - that makes me wonder - do we support non-dict branches in a flat
union?  The usual use case of a flat union is that all dictionary keys
in the branch's dictionary are treated as keys at the top level of the
resulting overall union; but a non-dictionary branch has no keys to
flatten into the top level.  I may need to tweak a subsequent patch to
ensure that flat union branches can only use dictionaries (while simple
unions can use any type).

> 
> This proves your "can always use a flat union" proposition.  The only
> way they can earn their keep is making the schema easier to read.  I
> haven't thought about that.
> 
> Another historical accident is how we express members common to all
> union cases: base type.  Probably just because complex types already had
> them.  Are you planning to change anything there?

Maybe, per your own review.  More at point [5] below...

> 
> [...]
>> diff --git a/tests/qapi-schema/alternate-nested.err 
>> b/tests/qapi-schema/alternate-nested.err
>> new file mode 100644
>> index 0000000..e69de29
>> diff --git a/tests/qapi-schema/alternate-nested.exit 
>> b/tests/qapi-schema/alternate-nested.exit
>> new file mode 100644
>> index 0000000..573541a
>> --- /dev/null
>> +++ b/tests/qapi-schema/alternate-nested.exit
>> @@ -0,0 +1 @@
>> +0
>> diff --git a/tests/qapi-schema/alternate-nested.json 
>> b/tests/qapi-schema/alternate-nested.json
>> new file mode 100644
>> index 0000000..d5812bf
>> --- /dev/null
>> +++ b/tests/qapi-schema/alternate-nested.json
>> @@ -0,0 +1,7 @@
>> +# FIXME: we should reject a nested anonymous union branch
> 
> Same reason we want to reject nested / anonymous complex types
> elsewhere?  Or is there a deeper reason?

Similar to the reason as we want to reject {'command':'foo',
'data':'alternate-type'} for allowing non-dictionaries - we aren't
currently using it, and it's easier to reject than to worry about making
corner cases of the generator work on something we won't use.

Also, if I have an alternate A that chooses between string and dict,
then want to create an alternate  B that chooses between int and
alternate A, will that even generate correct code?  An alternate
represents multiple QObject types at once, so determining the QObject
type of the next token while parsing the code for union B would have to
worry about BOTH cases of nested union A.

So the FIXME is correct, and later in the series, I nuke this as
unsupported.


>> +++ b/tests/qapi-schema/flat-union-bad-base.json
>> @@ -0,0 +1,13 @@
>> +# we require the base to be an existing complex type
>> +# FIXME: should we allow an anonymous inline base type?
> 
> What do you mean by "anonymous inline base type"?

[5] basically, that the following example could be legal shorthand...

> 
>> +{ 'enum': 'TestEnum',
>> +  'data': [ 'value1', 'value2' ] }
>> +{ 'type': 'TestTypeA',
>> +  'data': { 'string': 'str' } }
>> +{ 'type': 'TestTypeB',
>> +  'data': { 'integer': 'int' } }
>> +{ 'union': 'TestUnion',
>> +  'base': { 'enum1': 'TestEnum', 'kind': 'str' },
>> +  'discriminator': 'TestEnum',
>> +  'data': { 'kind1': 'TestTypeA',
>> +            'kind2': 'TestTypeB' } }

where the { 'enum1':'TestEnum', 'kind':'str' } anonymous type defined at
'base' should be usable instead of having to name the type with a more
verbose:

{ 'type': 'Base', 'data': {'enum1':'TestEnum', 'kind':'str' }}
{ 'union': 'TestUnion', 'base':'Base', ... }

or in other words, that unions behave more like 'command' allowing
'data':{dict} as an anonymous type in place of 'data':'name'.

>> diff --git a/tests/qapi-schema/flat-union-bad-base.out 
>> b/tests/qapi-schema/flat-union-bad-base.out
>> new file mode 100644
>> index 0000000..e69de29
> [...]
> 
> "Doesn't deal well" is an understatement :)
> 
> Since all my questions are about your intentions and rationale:
> 
> Reviewed-by: Markus Armbruster <address@hidden>
> 
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature