Re: [Qemu-devel] [PATCH v10 23/30] qapi: Check for qapi collisions of flat union branches

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 11/09/2015 10:16 PM, Eric Blake wrote:
>
>>> We have tests covering attempts to do the former
>>> (struct-cycle-direct.json, struct-cycle-indirect.json).  As far as I can
>>> see, we don't have tests covering the latter.  Do we catch it?
>> 
>> Yes, at least by virtue of the ad hoc tests: attempting to reuse a base
>> type of the flat union as a variant member will cause the qapi members
>> of the base type to appear more than once in the JSON object (that is,
>> the checks that reject flat-union-clash-member.json would also reject
>> this scenario). To test:
>> 
>> diff --git i/tests/qapi-schema/qapi-schema-test.json
>> w/tests/qapi-schema/qapi-schema-test.json
>> index 44638da..16b2ffb 100644
>> --- i/tests/qapi-schema/qapi-schema-test.json
>> +++ w/tests/qapi-schema/qapi-schema-test.json
>> @@ -67,7 +67,7 @@
>>    'discriminator': 'enum1',
>>    'data': { 'value1' : 'UserDefA',
>>              'value2' : 'UserDefB',
>> -            'value3' : 'UserDefB' } }
>> +            'value3' : 'UserDefUnionBase' } }
>
> Another test I just tried is creating a flat union with:
>
> empty -> base -> union
>
> then use empty as one of the union branches.  In that case, there is no
> conflict (although base is included twice, neither inclusion adds
> members to the JSON object; and the inclusion is not circular so things
> compile just fine).  So probably not worth adding a test for it.

Member name clashes occur because:

1. Two separately defined members happen to have a clashing name.

   Whether the members are local or inherited, variant or non-variant
   doesn't matter.

2. The same type gets spliced in twice, and conflict.

   By "spliced", I mean the members get included.  A bit like unboxed,
   but also "unwrapped".

   Base types and variant types get spliced in.  Variant types don't
   conflict with each other.  Empty type can't conflict (outlawing it
   anyway would be pointless).  Therefore, the only conflicting splice
   is a non-empty type occuring both as (base type of the) base type and
   as (base type of a) variant type.

   We detect this kind of clash exactly like 1.  That's fine, because
   the resulting error messages are plainly good enough.

3. Type contains itself.

   The type gets spliced into itself, or has itself as unboxed member.

   We don't unbox object members so far.  That leaves the spliced ones:
   base types and variants.  Thus, a type contains itself if it's its
   own base or one of its own variants, or a direct or indirect base of
   either.

   We detect this kind of clash separately, when we guard against
   infinite check() recursion.  Reporting it right there will be
   easiest.

We want to cover all this with tests.  I guess we're lacking only 3.