[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 06/28] qapi: Add some union tests
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v5 06/28] qapi: Add some union tests |
Date: |
Tue, 31 Mar 2015 22:46:47 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
Kevin Wolf <address@hidden> writes:
> Am 26.03.2015 um 16:04 hat Eric Blake geschrieben:
>> On 03/26/2015 07:18 AM, Markus Armbruster wrote:
>> > Eric Blake <address@hidden> writes:
>> >> 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'}}
>
> This would overload 'discriminator' with two different meanings:
>
> * In case [1] it refers to one key in the JSON object which contains the
> name of the union branch to select. That is, it is the _name_ of the
> key used as a discriminator.
>
> * In case [3], the 'type' key is used in the JSON object to select a
> union branch. 'discriminator' describes the _valid values_ of it, i.e.
> the branch names.
>
> We shouldn't mix these meanings. If you need [3], we could call it
> 'discriminator-type' or something like that. If both options are
> consistently used for simple and flat unions, you end up with these
> rules:
>
> * 'discriminator' is the key that is used to select the union branch.
>
> - For flat unions it is required and must refer to an explicitly
> declared key in the base type.
>
> - For simple unions it is optional and defaults to 'type'. The key is
> implicitly created in the union type.
>
> * 'discrimiator-type' describes the valid values of 'discriminator',
> either by referring to an enum type or to 'str'.
>
> - For flat unions, this must match the type of the explicit
> declaration of the discriminator field. It is optional and defauls
> to the only valid value.
>
> - For simple unions it is optional, too, and defaults to 'str'.
>
> - If it is the name of an enum type, that enum type is reused and the
> declared union branches must match the valid values of the enum.
>
> - If it is 'str', a new enum is generated, and all the declared union
> branches are used as valid values.
>
> There's quite some duplication in it where we need to make sure that the
> schema matches in all places, but without an explicit declaration of the
> disriminator field in simple unions, this seems to be the best I can
> come up with.
>
>> 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.
>
> Which could even be ambiguous, couldn't it?
>
>> > 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' } }
>
> Perhaps we should expose all unions for schema introspection in a way
> similar to this (possibly not splitting out Branch1 and Branch2 as
> independent types, though).
My current thinking on this is a bit more radical. I suspect there's a
straightforward object type buried in this mess, struggling to get out:
the good old variant record. It consists of
* a name
* an optional base type name (must be a object type without variants)
* list of own members (empty okay)
Effective members = own members + effective base type members
* optionally variants
- one of the effective members is the discriminator (must be enum)
- for each discriminator value a list of variant members (empty okay)
All existing type/union types are specializations:
* The complex type (struct type) is an object type without variants.
* The simple union type is an object type
- without a base type
- with an implicitly defined own member of an implicitly defined
enumeration type serving as discriminator
- with no other own members
- with a single variant member for each discriminator value
* The flat union type is an object type
- with a base type
- without own members
- with a discriminator
I further suspect lowering these types to the general form will make the
generator simpler, not just the introspection.
> We would just have to give a name to
> implicitly generated enums and base types.
Introspection doesn't care whether we defined something implicitly or
explicitly. Let's make up names to hide that.
I'm trying to get a proof-of-concept introspection patch working this
week. It'll probably be ugly enough to curdle the milk in your tea.
- Re: [Qemu-devel] [PATCH v5 14/28] qapi: Better error messages for bad expressions, (continued)
- [Qemu-devel] [PATCH v5 06/28] qapi: Add some union tests, Eric Blake, 2015/03/24
- Re: [Qemu-devel] [PATCH v5 06/28] qapi: Add some union tests, Markus Armbruster, 2015/03/26
- Re: [Qemu-devel] [PATCH v5 06/28] qapi: Add some union tests, Kevin Wolf, 2015/03/31
- Re: [Qemu-devel] [PATCH v5 06/28] qapi: Add some union tests, Eric Blake, 2015/03/31
- Re: [Qemu-devel] [PATCH v5 06/28] qapi: Add some union tests, Eric Blake, 2015/03/31
- Re: [Qemu-devel] [PATCH v5 06/28] qapi: Add some union tests, Kevin Wolf, 2015/03/31
- Re: [Qemu-devel] [PATCH v5 06/28] qapi: Add some union tests,
Markus Armbruster <=
Re: [Qemu-devel] [PATCH v5 06/28] qapi: Add some union tests, Markus Armbruster, 2015/03/26
[Qemu-devel] [PATCH v5 26/28] qapi: Drop inline nested type in query-version, Eric Blake, 2015/03/24
[Qemu-devel] [PATCH v5 22/28] qapi: Whitelist commands that don't return dictionary, Eric Blake, 2015/03/24