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

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 31.03.2015 um 22:46 hat Markus Armbruster geschrieben:
>> 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.
>
> That seems to be essentially the --verbose version of what I said above,
> except that you also include simple structs. So yes, I think I agree.
>
> Or maybe I'm missing what else you think is different?

Lamport's quip "If you're thinking without writing, you only think
you're thinking" certainly applies to me.  And then I can just as well
send it out, to give you an opportunity to point out holes or
misunderstandings.

For actually coding something, I need to think --verbose.  Computers
have so far stubbornly refused all my attempts to make them DWIM %-}

>> >                             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.
>
> We just need to find a good way to make up names that stay stable and
> don't cause clashes. I'm afraid that unlike block device IDs, we might
> not have additional characters that could select a different namespace
> here?
>
> Not that this would be super hard, but if we need to use the same
> namespace for automatically generated names, we need to properly
> document which other identifiers are automatically used when you
> declare a type (and detect clashes in the generator, of course).

"[PATCH 21] qapi: Require valid names" makes it easy.  I agree that
introspection documentation should explain automatically generated
identifiers.

>> 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.
>
> Who put that milk into my tea?! I never do that! ;-)

Enjoy!