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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> 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.

If you respin, I suggest to add that it's undocumented.

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

As far as I can tell, the generator rejects flat union 'data' members
that aren't discriminator values.  It doesn't require the union to cover
all discriminator members.  Makes sense to me, because the union may not
have additional data for some discriminator values.

Your claim "so that if the enum later adds a member, the union must also
be updated to match" appears to be wrong.

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

Ah, now I get what you mean!  We have a user-defined enum and the simple
union's implicit enum, and no way to connect the two.  In C, we get two
distinct enum types.

Perhaps you can clarify the commit message a bit.  Or maybe just drop
the part that confused me.  Okay since the relevant FIXME doesn't pass
judgement:

# FIXME: either allow base in non-flat unions, or diagnose missing discriminator

> 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.

Too subtle.  The difference should be syntactic, so it's immediately
visible.

>>>                                                       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.

Yes.

>> 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.

Yes, please.

> 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 think you just showed why non-dictionary branches make no sense.

>                              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).

Yes, please.  Follow-up patch okay, if that's easier for you.

>> 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.

Okay, that makes sense.

> 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.

Good.

>>> +++ 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',

Shouldn't this be 'discriminator': 'enum1'?

>>> +  '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'.

Got it.

Since there's nothing currently broken, I wouldn't label the question
"should we allow an anonymous inline base type?" FIXME.

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

R-by stands, but I encourage you to polish the commit message and drop
the FIXME: from the comment in flat-union-bad-base.json.