Re: [Qemu-devel] [PATCH v7 11/14] qapi: Move duplicate member checks to schema check()

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> With the previous commit, we have two different locations for
> detecting member name clashes - one at parse time, and another
> at QAPISchema*.check() time.  Consolidate some of the checks
> into a single place, which is also in line with our TODO to
> eventually move all of the parse time semantic checking into
> the newer schema code.  The check_member_clash() function is
> no longer needed.
>
> The wording of several error messages has changed, but in many
> cases feels like an improvement rather than a regression.  The
> recent change to avoid an assertion failure when a flat union
> branch name collides with its discriminator name is also
> handled nicely by this code; but there is more work needed
> before we can detect all collisions in simple union branch
> names done by the old code.
>
> No change to generated code.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v7: comment improvements, retitle subject
> v6: rebase to earlier testsuite improvements, fold in cleanup
> of flat-union-clash-type
> ---
>  scripts/qapi.py                               | 51 ++++++++++-----------------
>  tests/qapi-schema/flat-union-clash-member.err |  2 +-
>  tests/qapi-schema/flat-union-clash-type.err   |  2 +-
>  tests/qapi-schema/struct-base-clash-deep.err  |  2 +-
>  tests/qapi-schema/struct-base-clash.err       |  2 +-
>  5 files changed, 22 insertions(+), 37 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 30f1483..9f98413 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -499,21 +499,6 @@ def check_type(expr_info, source, value, allow_array=False,
>                                  'enum'])
>
>
> -def check_member_clash(expr_info, base_name, data, source=""):
> -    base = find_struct(base_name)
> -    assert base
> -    base_members = base['data']
> -    for key in data.keys():
> -        if key.startswith('*'):
> -            key = key[1:]
> -        if key in base_members or "*" + key in base_members:
> -            raise QAPIExprError(expr_info,
> -                                "Member name '%s'%s clashes with base '%s'"
> -                                % (key, source, base_name))
> -    if base.get('base'):
> -        check_member_clash(expr_info, base['base'], data, source)
> -
> -
>  def check_command(expr, expr_info):
>      name = expr['command']
>
> @@ -592,30 +577,18 @@ def check_union(expr, expr_info):
>      for (key, value) in members.items():
>          check_name(expr_info, "Member of union '%s'" % name, key)
>
> -        # Each value must name a known type; furthermore, in flat unions,
> -        # branches must be a struct with no overlapping member names
> +        # Each value must name a known type
>          check_type(expr_info, "Member '%s' of union '%s'" % (key, name),
>                     value, allow_array=not base, allow_metas=allow_metas)
> -        if base:
> -            branch_struct = find_struct(value)
> -            assert branch_struct
> -            check_member_clash(expr_info, base, branch_struct['data'],
> -                               " of branch '%s'" % key)
>
>          # If the discriminator names an enum type, then all members
> -        # of 'data' must also be members of the enum type, which in turn
> -        # must not collide with the discriminator name.
> +        # of 'data' must also be members of the enum type.
>          if enum_define:
>              if key not in enum_define['enum_values']:
>                  raise QAPIExprError(expr_info,
>                                      "Discriminator value '%s' is not found in "
>                                      "enum '%s'" %
>                                      (key, enum_define["enum_name"]))
> -            if discriminator in enum_define['enum_values']:
> -                raise QAPIExprError(expr_info,
> -                                    "Discriminator name '%s' collides with "
> -                                    "enum value in '%s'" %
> -                                    (discriminator, enum_define["enum_name"]))
>
>          # Otherwise, check for conflicts in the generated enum
>          else:
> @@ -690,8 +663,6 @@ def check_struct(expr, expr_info):
>                 allow_dict=True, allow_optional=True)
>      check_type(expr_info, "'base' for struct '%s'" % name, expr.get('base'),
>                 allow_metas=['struct'])
> -    if expr.get('base'):
> -        check_member_clash(expr_info, expr['base'], expr['data'])
>
>
>  def check_keys(expr_elem, meta, required, optional=[]):
> @@ -1095,16 +1066,30 @@ class QAPISchemaObjectTypeVariants(object):
>          assert isinstance(self.tag_member.type, QAPISchemaEnumType)
>          for v in self.variants:
>              vseen = dict(seen)
> -            v.check(schema, info, self.tag_member.type, vseen)
> +            v.check(schema, info, self.tag_member.type, vseen,
> +                    bool(self.tag_name))
>
>
>  class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>      def __init__(self, name, typ):
>          QAPISchemaObjectTypeMember.__init__(self, name, typ, False)
>
> -    def check(self, schema, info, tag_type, seen):
> +    def check(self, schema, info, tag_type, seen, flat):
>          QAPISchemaObjectTypeMember.check(self, schema, info, [], seen)
>          assert self.name in tag_type.values
> +        if flat:
> +            # For flat unions, check that each QMP member does not
> +            # collide with any non-variant members. No type can
> +            # contain itself as a flat variant
> +            self.type.check(schema)
> +            assert not self.type.variants    # not implemented
> +            for m in self.type.members:
> +                if m.c_name() in seen:
> +                    raise QAPIExprError(info,
> +                                        "Member '%s' of branch '%s' collides "
> +                                        "with %s"
> +                                        % (m.name, self.name,
> +                                           seen[m.c_name()].describe()))

If our data structures were right, we wouldn't need the conditional.

Three cases:

1. Flat union

   self is a flat union object type's variant case.  self.name is a tag
   value, and self.type is an object type providing the members for this
   tag value.  It has no variants.

   Example: UserDefFlatUnion's first case, self.name is 'value1',
   self.type is type UserDefA.

   We need to check that the case's variant members (i.e. self.type's
   members) don't clash with non-variant members, because in QMP, they
   become members of the same JSON object.

2. Simple union

   self is a simple union object type's variant case.  self.name is a
   tag value, and self.type is an object type providing the members for
   this tag value.  It has no variants.  Exactly the same as 1.  The
   fact that the tag member, its type and all the variant types are
   implicitly defined is immaterial.

   Example: __org.qemu_x-Union1's first case, self.name is
   '__org.qemu_x-branch', self.type is the implicitly defined type
   :obj-str-wrapper.  It's only member is named 'data'.

   The case's variant members (i.e. self.type's members) don't clash
   with non-variant members by construction.  Since simple unions can't
   have a base, the only non-variant member is the implicitly defined
   tag.  Its name is 'type'.  Each case has exactly one variant member
   named 'data'.

   Therefore, there should be no need to special-case simple unions
   here: the check() for flat unions should work fine, it just shouldn't
   find clashes.

3. Alternate

   self is an alternate type's case.  self.name names the case.  It is
   not a tag value in QMP, because alternates are implicitly tagged in
   QMP.  It is a tag value in C.  self.type is a type.  It needn't be an
   object type.

   Example: UserDefAlternate's first case, self.name is 'uda', self.type
   is type UserDefA.

   The case's members (if any) can't clash with anything.  However, the
   check() for flat unions doesn't quite work.

   First, it assumes self.type is an object type without variants.
   That's not true for alternate cases.

   Second, it puts the implicit tag member into seen, even though it's
   not actually on the wire.

So far the theory, now practice: if I hack the code to test "not
alternate" (i.e. case 1 and 2) instead of "flat union" (just case 1),
"make check" passes except for union-clash-data.json.  There, we now
report a clash we didn't report before:

    union-clash-data.json:6: Member 'data' of branch 'data' collides with 'data' (branch of TestUnion)

The FIXME in union-clash-data.json actually asks for an error, because
'data' clashes with our stupid filler to avoid empty unions.  But that's
not what this error message reports!  I think what happens is this:
QAPISchemaObjectTypeVariant.check() adds the case name self.name (here:
'data') to seen.  When we then check the case's 'data': 'int' member, it
finds the with the case name, and reports a clash.  I can't see why it
should.

This clashing business is awfully confusing, because we have to consider
(flat unions, simple unions, alternates) times (QMP, C).

It would be simpler if we could make C clash only when QMP clashes.

I've been trying to make simple unions a genuine special case of flat
unions in part to get shrink this matrix.

We might want to separate out alternates.  Dunno.

>      # This function exists to support ugly simple union special cases
>      # TODO get rid of them, and drop the function
> diff --git a/tests/qapi-schema/flat-union-clash-member.err b/tests/qapi-schema/flat-union-clash-member.err
> index 2f0397a..57dd478 100644
> --- a/tests/qapi-schema/flat-union-clash-member.err
> +++ b/tests/qapi-schema/flat-union-clash-member.err
> @@ -1 +1 @@
> -tests/qapi-schema/flat-union-clash-member.json:11: Member name 'name' of branch 'value1' clashes with base 'Base'
> +tests/qapi-schema/flat-union-clash-member.json:11: Member 'name' of branch 'value1' collides with 'name' (member of Base)
> diff --git a/tests/qapi-schema/flat-union-clash-type.err b/tests/qapi-schema/flat-union-clash-type.err
> index b44dd40..3f3248b 100644
> --- a/tests/qapi-schema/flat-union-clash-type.err
> +++ b/tests/qapi-schema/flat-union-clash-type.err
> @@ -1 +1 @@
> -tests/qapi-schema/flat-union-clash-type.json:11: Discriminator name 'type' collides with enum value in 'TestEnum'
> +tests/qapi-schema/flat-union-clash-type.json:11: 'type' (branch of TestUnion) collides with 'type' (member of Base)
> diff --git a/tests/qapi-schema/struct-base-clash-deep.err b/tests/qapi-schema/struct-base-clash-deep.err
> index f7a25a3..e2d7943 100644
> --- a/tests/qapi-schema/struct-base-clash-deep.err
> +++ b/tests/qapi-schema/struct-base-clash-deep.err
> @@ -1 +1 @@
> -tests/qapi-schema/struct-base-clash-deep.json:10: Member name 'name' clashes with base 'Base'
> +tests/qapi-schema/struct-base-clash-deep.json:10: 'name' (member of Sub) collides with 'name' (member of Base)
> diff --git a/tests/qapi-schema/struct-base-clash.err b/tests/qapi-schema/struct-base-clash.err
> index 3a9f66b..c52f33d 100644
> --- a/tests/qapi-schema/struct-base-clash.err
> +++ b/tests/qapi-schema/struct-base-clash.err
> @@ -1 +1 @@
> -tests/qapi-schema/struct-base-clash.json:5: Member name 'name' clashes with base 'Base'
> +tests/qapi-schema/struct-base-clash.json:5: 'name' (member of Sub) collides with 'name' (member of Base)