From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40404) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yb9Ky-0007S3-3t for qemu-devel@nongnu.org; Thu, 26 Mar 2015 11:04:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yb9Kt-0004g5-Kw for qemu-devel@nongnu.org; Thu, 26 Mar 2015 11:04:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:54191) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yb9Kt-0004fV-D4 for qemu-devel@nongnu.org; Thu, 26 Mar 2015 11:04:43 -0400 Message-ID: <55142006.50606@redhat.com> Date: Thu, 26 Mar 2015 09:04:38 -0600 From: Eric Blake MIME-Version: 1.0 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> In-Reply-To: <87h9t76gng.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="CVkXMAR1uJLP8Va7gs69TBkfEB11arUbB" 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: Markus Armbruster Cc: kwolf@redhat.com, lcapitulino@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, wenchaoqemu@gmail.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --CVkXMAR1uJLP8Va7gs69TBkfEB11arUbB Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 03/26/2015 07:18 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> Demonstrate that the qapi generator doesn't deal well with unions >> that aren't up to par. Later patches will update the expected >> reseults as the generator is made stricter. >> >> Of particular note, we currently allow 'base' without 'discriminator' >> as a way to create a simple union with a base class. However, none >> of the existing QMP or QGA interfaces use it (it is exercised only >> in the testsuite). >=20 > qapi-code-gen.txt documents 'base' only for flat unions. We should > either document its use in simple unions, or outlaw it. I went with outlaw later in the series, and the rest of my commit message was an attempt to explain my reasoning in that choice. But I can certainly try to improve the wording, if we need a respin. >=20 >> 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). >=20 > 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'}} 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. >=20 >> If both 'base' >> and 'discriminator' are optional, then converting a simple union >> with base class to a type-safe simple union with an enum discriminator= >> would not be possible. So my plan is to get rid of 'base' without >> 'discriminator' later in the series; >=20 > Aha: you're going to outlaw 'base' in simple unions. Yes, please. Okay, you came to my desired conclusion; it's just that my wording in the middle didn't help. >=20 >> this will be no real loss, as any= >> union that needs additional common fields can always use a flat >> union. >=20 > The mathematical concept behind unions is the sum type. >=20 > We have three implementations, and we call them simple, flat, anonymous= =2E >=20 > Anonymous unions are implicitly tagged. They come with the obvious > restrictions required to make implicit tagging work. and get renamed to 'alternate' later in the series, so they are not worth worrying about here. >=20 > The other two are explicitly tagged. The difference between them is > just notation. I like my unions flat, because for me the extra wrappin= g > is just notational overhead. >=20 > 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' } } Probably worth mentioning in my commit message. Hmm - that makes me wonder - do we support non-dict branches in a flat union? The usual use case of a flat union is that all dictionary keys in the branch's dictionary are treated as keys at the top level of the resulting overall union; but a non-dictionary branch has no keys to flatten into the top level. I may need to tweak a subsequent patch to ensure that flat union branches can only use dictionaries (while simple unions can use any type). >=20 > This proves your "can always use a flat union" proposition. The only > way they can earn their keep is making the schema easier to read. I > haven't thought about that. >=20 > Another historical accident is how we express members common to all > union cases: base type. Probably just because complex types already ha= d > them. Are you planning to change anything there? Maybe, per your own review. More at point [5] below... >=20 > [...] >> diff --git a/tests/qapi-schema/alternate-nested.err b/tests/qapi-schem= a/alternate-nested.err >> new file mode 100644 >> index 0000000..e69de29 >> diff --git a/tests/qapi-schema/alternate-nested.exit b/tests/qapi-sche= ma/alternate-nested.exit >> new file mode 100644 >> index 0000000..573541a >> --- /dev/null >> +++ b/tests/qapi-schema/alternate-nested.exit >> @@ -0,0 +1 @@ >> +0 >> diff --git a/tests/qapi-schema/alternate-nested.json b/tests/qapi-sche= ma/alternate-nested.json >> new file mode 100644 >> index 0000000..d5812bf >> --- /dev/null >> +++ b/tests/qapi-schema/alternate-nested.json >> @@ -0,0 +1,7 @@ >> +# FIXME: we should reject a nested anonymous union branch >=20 > Same reason we want to reject nested / anonymous complex types > elsewhere? Or is there a deeper reason? Similar to the reason as we want to reject {'command':'foo', 'data':'alternate-type'} for allowing non-dictionaries - we aren't currently using it, and it's easier to reject than to worry about making corner cases of the generator work on something we won't use. Also, if I have an alternate A that chooses between string and dict, then want to create an alternate B that chooses between int and alternate A, will that even generate correct code? An alternate represents multiple QObject types at once, so determining the QObject type of the next token while parsing the code for union B would have to worry about BOTH cases of nested union A. So the FIXME is correct, and later in the series, I nuke this as unsupported. >> +++ b/tests/qapi-schema/flat-union-bad-base.json >> @@ -0,0 +1,13 @@ >> +# we require the base to be an existing complex type >> +# FIXME: should we allow an anonymous inline base type? >=20 > What do you mean by "anonymous inline base type"? [5] basically, that the following example could be legal shorthand... >=20 >> +{ 'enum': 'TestEnum', >> + 'data': [ 'value1', 'value2' ] } >> +{ 'type': 'TestTypeA', >> + 'data': { 'string': 'str' } } >> +{ 'type': 'TestTypeB', >> + 'data': { 'integer': 'int' } } >> +{ 'union': 'TestUnion', >> + 'base': { 'enum1': 'TestEnum', 'kind': 'str' }, >> + 'discriminator': 'TestEnum', >> + 'data': { 'kind1': 'TestTypeA', >> + 'kind2': 'TestTypeB' } } where the { 'enum1':'TestEnum', 'kind':'str' } anonymous type defined at 'base' should be usable instead of having to name the type with a more verbose: { 'type': 'Base', 'data': {'enum1':'TestEnum', 'kind':'str' }} { 'union': 'TestUnion', 'base':'Base', ... } or in other words, that unions behave more like 'command' allowing 'data':{dict} as an anonymous type in place of 'data':'name'. >> diff --git a/tests/qapi-schema/flat-union-bad-base.out b/tests/qapi-sc= hema/flat-union-bad-base.out >> new file mode 100644 >> index 0000000..e69de29 > [...] >=20 > "Doesn't deal well" is an understatement :) >=20 > Since all my questions are about your intentions and rationale: >=20 > Reviewed-by: Markus Armbruster >=20 >=20 --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --CVkXMAR1uJLP8Va7gs69TBkfEB11arUbB Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJVFCAGAAoJEKeha0olJ0Nqo3QH/27fnfMRSftD8dHuXDhNliVP P6bggkawBLHtP+GNDcpBrQr3keu+QJ65E+Sifqa+ekk4uws7pOxQF7+s6+25tQUQ qc4XgeRzx/tPF2TnYmBJybzDDUm8p7fUpi2UWukHQCsUwwETOVm600EOvllT59MA P3xRSKMd3kZCN2ViFkYz0oD6z9mibMZt592sRLWvFBasdJEomRu0joHPdZsDyq9U SQ1eVcywN0uBP8o9NCOjvYtZ/35kL49rvHhhQexvQz6pT+vpXyHTTzYvlaywBLI+ 9bBRSRz7gCORwb5hxifBF69Apol4RTDdTH+/2qAhUFZAhUEbqzkHtu/f7oS9Xic= =7ywG -----END PGP SIGNATURE----- --CVkXMAR1uJLP8Va7gs69TBkfEB11arUbB--