Re: [Qemu-devel] [PATCH v8 07/17] qapi: Rework collision assertions

[Markus Armbruster]

Eric Blake <address@hidden> 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.