Re: [Qemu-devel] [PATCH v6 08/12] qapi: Defer 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 defer 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

Which patch was that again?

> 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>
>
> ---
> v6: rebase to earlier testsuite improvements, fold in cleanup
> of flat-union-clash-type
> ---
>  scripts/qapi.py                               | 46 +++++++++------------------
>  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, 19 insertions(+), 35 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 1acf02b..e58c5a8 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,15 +577,9 @@ 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
> @@ -611,11 +590,6 @@ def check_union(expr, expr_info):
           # 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.

Should this comment be updated for the code removal below?

           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 +664,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=[]):
> @@ -1046,16 +1018,28 @@ 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,
> +                    self.tag_name is not None)
>
>
>  class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>      def __init__(self, name, typ, owner):
>          QAPISchemaObjectTypeMember.__init__(self, name, typ, False, owner)
>
> -    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:

Any way to avoid the conditional?

I've tried hard to make simple unions mere sugar for flat ones.  There
are a few special cases left where we need to distinguish the two, but
they're all marked TODO.

Can we have a brief comment explaing what we're checking here?  We
generally don't have such comments in check() methods so far.  Sort of
okay as long as they merely assert, but you're now starting to move
semantic analysis into them, which raises the bar.

> +            self.type.check(schema)

Uh, careful.

It's always okay for a check() method to call the check() of a child in
the abstract syntax tree, e.g. QAPISchemaObjectType.check() calling
m.check() for its members.  A tree has no cycles.

Calling it on anything else requires a non-trivial correctness argument.
Example: QAPISchemaObjectType.check() calls self.base.check().  Okay,
because no type may be a base of itself, and therefore a cycle would be
an error.  The code takes care to detect cycles.

I think the correctness argument here would be "no type contain itself
as variant member, and therefore a cycle would be an error."  Do we
detect cycles?

> +            assert isinstance(self.type.members, list)

Implied by QAPISchemaObjectType.check()'s postcondition.  Feels
redundant here, in particular since you iterate over it next anyway.

> +            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"

Hanging indentation would avoid the split string.

> +                                        % (m.name, self.name,
> +                                           seen[m.c_name()].describe()))
>
>      def describe(self):
>          return "'%s' (branch of %s)" % (self.name, self._owner)
> 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)