Re: [Qemu-devel] [PATCH v10 28/30] qapi: Detect collisions in C member names

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Detect attempts to declare two object members that would result
> in the same C member name, by keying the 'seen' dictionary off
> of the C name rather than the qapi name.  It also requires passing
> info through the check_clash() methods.
>
> This addresses a TODO and fixes the previously-broken
> args-name-clash test.  The resulting error message demonstrates
> the utility of the .describe() method added previously.  No change
> to generated code.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v10 (now in subset C): rebase to latest; update commit message
> v9 (now in subset D): rebase to earlier changes, now only one test
> affected
> v8: rebase to earlier changes
> v7: split out error reporting prep and member.c_name() addition
> v6: rebase to earlier testsuite and info improvements
> ---
>  scripts/qapi.py                        | 31 +++++++++++++++++++------------
>  tests/qapi-schema/args-name-clash.err  |  1 +
>  tests/qapi-schema/args-name-clash.exit |  2 +-
>  tests/qapi-schema/args-name-clash.json |  6 +++---
>  tests/qapi-schema/args-name-clash.out  |  6 ------
>  5 files changed, 24 insertions(+), 22 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index b3af973..3a359cb 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -975,21 +975,24 @@ class QAPISchemaObjectType(QAPISchemaType):
>          seen = OrderedDict()
>          if self._base_name:
>              self.base = schema.lookup_type(self._base_name)
> -            self.base.check_clash(schema, seen)
> +            self.base.check_clash(schema, self.info, seen)

Note that if this one reports an error for self.info, something went
wrong.  We first run self.base.check(), which we survive only when
self.base doesn't have clashing members.  Would be easier to see if
self.base.check() wasn't hiding in self.base.check_clash().

>          for m in self.local_members:
>              m.check(schema)
> -            m.check_clash(seen)
> +            m.check_clash(self.info, seen)
>          self.members = seen.values()
>          if self.variants:
>              self.variants.check(schema, seen)
>              assert self.variants.tag_member in self.members
> -            self.variants.check_clash(schema, seen)
> +            self.variants.check_clash(schema, self.info, seen)
>
> -    def check_clash(self, schema, seen):
> +    # Check that the members of this type do not cause duplicate JSON fields,
> +    # and update seen to track the members seen so far. Report any errors
> +    # on behalf of info, which is not necessarily self.info

Do we actually need info != self.info?  If yes, test case?  If no,
should the comment be adjusted?

> +    def check_clash(self, schema, info, seen):
>          self.check(schema)
>          assert not self.variants       # not implemented
>          for m in self.members:
> -            m.check_clash(seen)
> +            m.check_clash(info, seen)
>
>      def is_implicit(self):
>          # See QAPISchema._make_implicit_object_type()
> @@ -1035,10 +1038,13 @@ class QAPISchemaObjectTypeMember(object):
>          self.type = schema.lookup_type(self._type_name)
>          assert self.type
>
> -    def check_clash(self, seen):
> -        # TODO change key of seen from QAPI name to C name
> -        assert self.name not in seen
> -        seen[self.name] = self
> +    def check_clash(self, info, seen):
> +        name = c_name(self.name)

I'd call it cname.  Matter of taste, of course.

> +        if name in seen:
> +            raise QAPIExprError(info,
> +                                "%s collides with %s"
> +                                % (self.describe(), seen[name].describe()))
> +        seen[name] = self
>
>      def _pretty_owner(self):
>          # See QAPISchema._make_implicit_object_type() - reverse the
> @@ -1081,18 +1087,19 @@ class QAPISchemaObjectTypeVariants(object):
>
>      def check(self, schema, seen):
>          if self.tag_name:    # flat union
> -            self.tag_member = seen[self.tag_name]
> +            self.tag_member = seen[c_name(self.tag_name)]
> +            assert self.tag_name == self.tag_member.name
>          tag_type = self.tag_member.type
>          assert isinstance(tag_type, QAPISchemaEnumType)
>          for v in self.variants:
>              v.check(schema)
>              assert v.name in tag_type.values
>
> -    def check_clash(self, schema, seen):
> +    def check_clash(self, schema, info, seen):
>          for v in self.variants:
>              # Reset seen map for each variant, since qapi names from one
>              # branch do not affect another branch
> -            v.type.check_clash(schema, dict(seen))
> +            v.type.check_clash(schema, info, dict(seen))

Since we can't inherit variant members, info is always the owning object
type's info.

>
>
>  class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
> diff --git a/tests/qapi-schema/args-name-clash.err 
> b/tests/qapi-schema/args-name-clash.err
> index e69de29..2735217 100644
> --- a/tests/qapi-schema/args-name-clash.err
> +++ b/tests/qapi-schema/args-name-clash.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/args-name-clash.json:5: 'a_b' (argument of oops) collides 
> with 'a-b' (argument of oops)
> diff --git a/tests/qapi-schema/args-name-clash.exit 
> b/tests/qapi-schema/args-name-clash.exit
> index 573541a..d00491f 100644
> --- a/tests/qapi-schema/args-name-clash.exit
> +++ b/tests/qapi-schema/args-name-clash.exit
> @@ -1 +1 @@
> -0
> +1
> diff --git a/tests/qapi-schema/args-name-clash.json 
> b/tests/qapi-schema/args-name-clash.json
> index 9e8f889..3fe4ea5 100644
> --- a/tests/qapi-schema/args-name-clash.json
> +++ b/tests/qapi-schema/args-name-clash.json
> @@ -1,5 +1,5 @@
>  # C member name collision
> -# FIXME - This parses, but fails to compile, because the C struct is given
> -# two 'a_b' members.  Either reject this at parse time, or munge the C names
> -# to avoid the collision.
> +# Reject members that clash when mapped to C names (we would have two 'a_b'
> +# members). It would also be possible to munge the C names to avoid the
> +# collision, but unlikely to be worth the effort.

I'd drop the second sentence.

>  { 'command': 'oops', 'data': { 'a-b': 'str', 'a_b': 'str' } }
> diff --git a/tests/qapi-schema/args-name-clash.out 
> b/tests/qapi-schema/args-name-clash.out
> index 9b2f6e4..e69de29 100644
> --- a/tests/qapi-schema/args-name-clash.out
> +++ b/tests/qapi-schema/args-name-clash.out
> @@ -1,6 +0,0 @@
> -object :empty
> -object :obj-oops-arg
> -    member a-b: str optional=False
> -    member a_b: str optional=False
> -command oops :obj-oops-arg -> None
> -   gen=True success_response=True