Re: [Qemu-devel] [PATCH v9 19/27] 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 QAPISchema*.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 passed
> in to QAPISchemaObjectTypeVariants.check(), which we clone for
> each particular variant (since the members of one variant do
> not clash with another).  So all that is additionally needed
> is to actually check the each member of the variant type do
> not add any collisions.
>
> 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).
>
> 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).
>
> But for alternates, we do NOT want to check the type members
> for adding collisions (an alternate has no parent JSON object
> that is merging in member names, the way a flat union does), so
> we branch on whether seen is empty to distinguish whether we
> are checking a union or an alternate.
>
> No change to generated code.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v9: new patch, split off from v8 7/17
> ---
>  scripts/qapi.py | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index a20abda..3054628 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1057,8 +1057,9 @@ class QAPISchemaObjectTypeVariants(object):
>              assert self.tag_member in seen.itervalues()
>          assert isinstance(self.tag_member.type, QAPISchemaEnumType)
>          for v in self.variants:
> -            vseen = dict(seen)
> -            v.check(schema, self.tag_member.type, vseen)
> +            # Reset seen array for each variant, since qapi names from one
> +            # branch do not affect another branch
> +            v.check(schema, self.tag_member.type, dict(seen))
>
>
>  class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
> @@ -1068,6 +1069,14 @@ class 
> QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>      def check(self, schema, tag_type, seen):
>          QAPISchemaObjectTypeMember.check(self, schema)
>          assert self.name in tag_type.values
> +        if seen:
> +            # This variant is used within a union; ensure each qapi member
> +            # field does not collide with the union's non-variant members.
> +            assert isinstance(self.type, QAPISchemaObjectType)
> +            assert not self.type.variants       # not implemented
> +            self.type.check(schema)
> +            for m in self.type.members:
> +                m.check_clash(seen)
>
>      # This function exists to support ugly simple union special cases
>      # TODO get rid of them, and drop the function

Two call chains:

* QAPISchemaObjectType.check() via QAPISchemaObjectTypeVariants.check()

  In this case, the new conditional executes.

* QAPISchemaAlternateType.check() via same

  In this case, it doesn't.

Why can't we simply add the new code to QAPISchemaObjectType.check()?
The rest of the clash checking is already there...