Eric Blake <ebl...@redhat.com> writes: > On 11/02/2015 08:37 AM, Markus Armbruster wrote: > >> >> Not checked: variant's members don't collide with non-variant members. >> I think this check got lost when we simplified >> QAPISchemaObjectTypeVariants to hold a single member. > > Yep, I found the culprit: in your v2 proposal for QAPISchema, you had: > > +class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): > + def __init__(self, name, typ, flat): > + QAPISchemaObjectTypeMember.__init__(self, name, typ, False) > + assert isinstance(flat, bool) > + self.flat = flat > + def check(self, schema, tag_type, seen): > + QAPISchemaObjectTypeMember.check(self, schema, [], seen) > + assert self.name in tag_type.values > + if self.flat: > + self.type.check(schema) > + assert isinstance(self.type, QAPISchemaObjectType) > > https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg00394.html > > but the 'if self.flat' clause was lost in v3: > > https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg00450.html
Quoting v3's change log: - Lower simple variants to flat ones as described on qapi-code-gen.txt. Replace QAPISchemaObjectType's .flat by .simple_union_type(), for use by pre-existing code-generation warts. > I am in fact reinstating it here, but for v9, will do it in a separate > patch rather than blended in with the rest of the changes. Any "is this union flat or simple" check signals a flaw. It's either a pointless difference in generated code (these should all be marked TODO by now), or something's wrong with the desugaring of simple to flat unions. In this case, it looks like a collision check was lost when I switched from "simple unions are its own separate type" to "simple unions are sugar for flat unions". Reinstating it makes sense, except for the "if self.flat" part. If the check's correct for flat unions, it must be correct for desugared simple unions, or else we screwed up the desugaring. For a genuine flat union case 'tag-value': 'FlatVariant' self.type is the object type 'FlatVariant'. Its members are the case's variant members, and they can clash with the flat union's non-variant members. For a desugared simple union case 'tag-value': 'SimpleVariant' self.type is an object type with a single member 'data' of type 'SimpleVariant'. Its member 'data' is the case's only variant member, and it can clash with the simple union's non-variant members. In theory only, because the only non-variant member is the implicit tag, and it's called 'type'. Therefore, the if self.flat is superfluous. Good, because otherwise our desugaring must be flawed. > [wow - we've been hammering at this since July?] First RFC was in April, but we started hammering in earnest only in summer.