From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53747) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yd33n-0004gw-Pe for qemu-devel@nongnu.org; Tue, 31 Mar 2015 16:46:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yd33k-0007ql-F6 for qemu-devel@nongnu.org; Tue, 31 Mar 2015 16:46:55 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53829) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yd33k-0007q7-8a for qemu-devel@nongnu.org; Tue, 31 Mar 2015 16:46:52 -0400 From: Markus Armbruster References: <1427227433-5030-1-git-send-email-eblake@redhat.com> <1427227433-5030-7-git-send-email-eblake@redhat.com> <87h9t76gng.fsf@blackfin.pond.sub.org> <55142006.50606@redhat.com> <20150331171320.GD4748@noname.redhat.com> Date: Tue, 31 Mar 2015 22:46:47 +0200 In-Reply-To: <20150331171320.GD4748@noname.redhat.com> (Kevin Wolf's message of "Tue, 31 Mar 2015 19:13:20 +0200") Message-ID: <87ego43new.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v5 06/28] qapi: Add some union tests List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, famz@redhat.com, wenchaoqemu@gmail.com, lcapitulino@redhat.com Kevin Wolf writes: > Am 26.03.2015 um 16:04 hat Eric Blake geschrieben: >> On 03/26/2015 07:18 AM, Markus Armbruster wrote: >> > Eric Blake 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.