Re: [Qemu-devel] [PATCH v10 23/30] qapi: Check for qapi collisions of flat union branches

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Right now, our ad hoc parser ensures that we cannot have a
> flat union that introduces any qapi member names that would
> conflict with the non-variant qapi members already present
> from the union's base type (see flat-union-clash-member.json).
> We want QAPISchemaObjectType.check() to make the same check,
> so we can later reduce some of the ad hoc checks.
>
> We already ensure that all branches of a flat union are qapi
> structs with no variants, at which point those members appear
> in the same JSON object as all non-variant members.  And we
> already have a map 'seen' of all non-variant members.  All
> we need is a new QAPISchemaObjectTypeVariants.check_clash(),
> which clones the seen map then checks for clashes with each
> member of the variant's qapi type.
>
> Note that the clone of seen inside Variants.check_clash()
> resembles the one we just removed from Variants.check(); the
> difference here is that we are now checking for clashes
> among the qapi members of the variant type, rather than for
> a single clash with the variant tag name itself.
>
> In general, a type used as a branch of a flat union cannot
> also be the base type of the flat union, so even though we are
> adding a call to variant.type.check() in order to populate
> variant.type.members, this is merely a case of gaining
> topological sorting of how types are visited (and type.check()
> is already set up to allow multiple calls due to base types).

Yes, a type cannot contain itself, neither as base nor as variant.

We have tests covering attempts to do the former
(struct-cycle-direct.json, struct-cycle-indirect.json).  As far as I can
see, we don't have tests covering the latter.  Do we catch it?

> For simple unions, the same code happens to work by design,
> because of our synthesized wrapper classes (however, the
> wrapper has a single member 'data' which will never collide
> with the one non-variant member 'type', so it doesn't really
> matter).
>
> There is no impact to alternates, which intentionally do not
> need to call variants.check_clash() (there, at most one of
> the variant's branches will be an ObjectType, and even if one
> exists, we are not inlining the qapi members of that object
> into a parent object, the way we do for unions).
>
> No change to generated code.
>
> Signed-off-by: Eric Blake <address@hidden>

Patch looks good.