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

[Markus Armbruster]

Markus Armbruster <address@hidden> writes:

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

Yes.  QAPISchemaObjectTypeVariants.check_clash() checks for each
variant's members clashing with other members in the same name space.
For alternates, there are no such other members.

That said, should we add a comment to QAPISchemaAlternateType.check()?
Perhaps:

        # Not calling self.variant.check_clash(), because there's
        # nothing to clash with.

>> No change to generated code.
>>
>> Signed-off-by: Eric Blake <address@hidden>
>
> Patch looks good.