Re: [Qemu-devel] [PATCH v11 11/28] qapi: Check for qapi collisions of flat union branches

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> 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.  Still
> missing, until now, was a call to variant.type.check() during
> Variants.check() (to populate variant.type.members), and a
> new Variants.check_clash() which clones the seen map for each
> variant before checking that variant's members for clashes.
>
> Note that cloning '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 non-empy type used as a branch of a flat union

non-empty

> 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.

When the variant's type or one of its base types equals its owning type
or one of its base types, then each of this type's members clashes with
itself.  Your clash detection code will flag that.

Except when "this type" is the owning type.  Then the owning type
contains itself, which the QAPISchemaObjectType.check()'s cycle detector
flags before we get around to checking for member clashes.

Your patch actually does two things:

1. Extend the "object type cannot contain itself" check to cover its
   variant types in addition to base type.  This is the two-liner change
   to QAPISchemaObjectTypeVariants.check().

2. Extend the "object type cannot contain members with clashing names"
   to cover variant members.

They're related: you need the former's side effect to compute .members
to be able to do the latter.

Doing it in two separate patches *might* make explaining it in the
commit message easier.  See below.

> 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).

The number of object variants we support is immaterial, because variants
can't collide with each other.

I'd simplify to something like

    For alternates, there's nothing for a variant object type's members
    to clash with, and therefore no need to call variants.check_clash().

>                                                   However,
> since variants can have non-object branches, it did mean that
> the addition of v.type.check() in Variants.check() had to be
> conditional on visiting an object type.

"did mean" and "had to be"?  It still means, and it still has to be...

It has to be conditional because only object types' check() is prepared
for getting called multiple times.  All the other types' check() really
want to be called exactly once from QAPISchema.check().

The conditional can only be false for alternates.  When it's true for
alternates, the .check() isn't really needed as of now, but doesn't
hurt.

> No change to generated code.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v11: keep type.check() in check(), add comment to alternate
> v10: create new Variants.check_clash() rather than piggybacking
> on .check()
> v9: new patch, split off from v8 7/17

Patch looks good.

Let me have a stab at the commit message of the *unsplit* patch, use as
you see fit:

qapi: Check for collisions involving variant members

Right now, our ad hoc parser ensures that we cannot have a flat union
that introduces any members that would clash with non-variant members
inherited 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 have a map 'seen' of all non-variant members.  We still need
to check for collisions between each variant type's members and the
non-variant ones.

To know the variant type's members, we need to call
variant.type.check().  This also detects when a type contains itself in
a variant, exactly like the existing base.check() detects when a type
contains itself as a base.

Slight complication: an alternate's variant can have arbitrary type, but
only an object type's check() may be called outside QAPISchema.check().
We could either skip the call for variants of alternates, or skip it for
non-object types.  Do the latter, because it's easier.

Then we can call each variant member's check_clash() with the
appropriate 'seen' map.  Since members of different variants can't
clash, We have to clone a fresh seen for each variant.  Wrap this in the
new helper method QAPISchemaObjectTypeVariants.check_clash().

Note that cloning '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.

Note that by construction collisions can't actually happen for simple
unions: each variant's type is a wrapper with a single member 'data',
which will never collide with the only non-variant member 'type'.

For alternates, there's nothing for a variant object type's members to
clash with, and therefore no need to call variants.check_clash().

No change to generated code.