Kevin Wolf <kw...@redhat.com> writes: > Am 31.03.2015 um 22:46 hat Markus Armbruster geschrieben: >> Kevin Wolf <kw...@redhat.com> writes: >> >> > Am 26.03.2015 um 16:04 hat Eric Blake geschrieben: >> >> On 03/26/2015 07:18 AM, Markus Armbruster wrote: >> >> > Eric Blake <ebl...@redhat.com> 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!