Re: [Qemu-devel] [PATCH v7 10/14] 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 some of the check() methods.
>
> This fixes two previously-broken tests, and the resulting error
> messages demonstrate the utility of the .describe() method added
> previously.  No change to generated code.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v7: split out error reporting prep and member.c_name() addition
> v6: rebase to earlier testsuite and info improvements
> ---
>  scripts/qapi.py                                | 33 
> ++++++++++++++++----------
>  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 -----
>  tests/qapi-schema/flat-union-clash-branch.err  |  1 +
>  tests/qapi-schema/flat-union-clash-branch.exit |  2 +-
>  tests/qapi-schema/flat-union-clash-branch.json |  9 +++----
>  tests/qapi-schema/flat-union-clash-branch.out  | 14 -----------
>  9 files changed, 32 insertions(+), 42 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 11ffc49..30f1483 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -993,11 +993,11 @@ class QAPISchemaObjectType(QAPISchemaType):
>          seen = {}
>          for m in members:
>              assert m.c_name() not in seen
> -            seen[m.name] = m
> +            seen[m.c_name()] = m
>          for m in self.local_members:
> -            m.check(schema, members, seen)
> +            m.check(schema, self.info, members, seen)
>          if self.variants:
> -            self.variants.check(schema, members, seen)
> +            self.variants.check(schema, self.info, members, seen)
>          self.members = members
>
>      def _is_implicit(self):
> @@ -1033,13 +1033,19 @@ class QAPISchemaObjectTypeMember(object):
>          self.optional = optional
>          self.owner = None   # will be filled by owner's init
>
> -    def check(self, schema, all_members, seen):
> +    def check(self, schema, info, all_members, seen):
>          assert self.owner
> -        assert self.name not in seen
>          self.type = schema.lookup_type(self._type_name)
>          assert self.type
> +        # Check that there is no collision in generated C names (which
> +        # also ensures no collisions in QMP names)
> +        if self.c_name() in seen:
> +            raise QAPIExprError(info,
> +                                "%s collides with %s"
> +                                % (self.describe(),
> +                                   seen[self.c_name()].describe()))
>          all_members.append(self)
> -        seen[self.name] = self
> +        seen[self.c_name()] = self
>
>      def c_name(self):
>          return c_name(self.name)
> @@ -1080,23 +1086,24 @@ class QAPISchemaObjectTypeVariants(object):
>          self.tag_member = tag_member
>          self.variants = variants
>
> -    def check(self, schema, members, seen):
> +    def check(self, schema, info, members, seen):
>          if self.tag_name:
> -            self.tag_member = seen[self.tag_name]
> +            self.tag_member = seen[c_name(self.tag_name)]
> +            assert self.tag_name == self.tag_member.name
>          else:
> -            self.tag_member.check(schema, members, seen)
> +            self.tag_member.check(schema, info, members, seen)
>          assert isinstance(self.tag_member.type, QAPISchemaEnumType)
>          for v in self.variants:
>              vseen = dict(seen)
> -            v.check(schema, self.tag_member.type, vseen)
> +            v.check(schema, info, self.tag_member.type, vseen)
>
>
>  class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>      def __init__(self, name, typ):
>          QAPISchemaObjectTypeMember.__init__(self, name, typ, False)
>
> -    def check(self, schema, tag_type, seen):
> -        QAPISchemaObjectTypeMember.check(self, schema, [], seen)
> +    def check(self, schema, info, tag_type, seen):
> +        QAPISchemaObjectTypeMember.check(self, schema, info, [], seen)
>          assert self.name in tag_type.values
>
>      # This function exists to support ugly simple union special cases
> @@ -1124,7 +1131,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
>          self.variants = variants
>
>      def check(self, schema):
> -        self.variants.check(schema, [], {})
> +        self.variants.check(schema, self.info, [], {})
>
>      def json_type(self):
>          return 'value'
> diff --git a/tests/qapi-schema/args-name-clash.err 
> b/tests/qapi-schema/args-name-clash.err
> index e69de29..66f609c 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' (member of oops arguments) 
> collides with 'a-b' (member of oops arguments)

"(argument of oops)" would be better, but "(member of oops arguments)"
will do.

> 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.
>  { '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 0e986b6..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 arguments
> -    member a-b: str optional=False
> -    member a_b: str optional=False
> -command oops :obj-oops arguments -> None
> -   gen=True success_response=True
> diff --git a/tests/qapi-schema/flat-union-clash-branch.err 
> b/tests/qapi-schema/flat-union-clash-branch.err
> index e69de29..0190d79 100644
> --- a/tests/qapi-schema/flat-union-clash-branch.err
> +++ b/tests/qapi-schema/flat-union-clash-branch.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/flat-union-clash-branch.json:15: 'c-d' (branch of 
> TestUnion) collides with 'c_d' (member of Base)
> diff --git a/tests/qapi-schema/flat-union-clash-branch.exit 
> b/tests/qapi-schema/flat-union-clash-branch.exit
> index 573541a..d00491f 100644
> --- a/tests/qapi-schema/flat-union-clash-branch.exit
> +++ b/tests/qapi-schema/flat-union-clash-branch.exit
> @@ -1 +1 @@
> -0
> +1
> diff --git a/tests/qapi-schema/flat-union-clash-branch.json 
> b/tests/qapi-schema/flat-union-clash-branch.json
> index e593336..a6c302f 100644
> --- a/tests/qapi-schema/flat-union-clash-branch.json
> +++ b/tests/qapi-schema/flat-union-clash-branch.json
> @@ -1,8 +1,9 @@
>  # Flat union branch name collision
> -# FIXME: this parses, but then fails to compile due to a duplicate 'c_d'
> -# (one from the base member, the other from the branch name).  We should
> -# either reject the collision at parse time, or munge the generated branch
> -# name to allow this to compile.
> +# Reject attempts to use a branch name that would clash with a non-variant
> +# member, when mapped to C names (here, we would have two 'c_d' members,
> +# one from the base member, the other from the branch name).
> +# TODO: We could munge the generated branch name to avoid the collision and
> +# allow this to compile.
>  { 'enum': 'TestEnum',
>    'data': [ 'base', 'c-d' ] }
>  { 'struct': 'Base',
> diff --git a/tests/qapi-schema/flat-union-clash-branch.out 
> b/tests/qapi-schema/flat-union-clash-branch.out
> index 8e0da73..e69de29 100644
> --- a/tests/qapi-schema/flat-union-clash-branch.out
> +++ b/tests/qapi-schema/flat-union-clash-branch.out
> @@ -1,14 +0,0 @@
> -object :empty
> -object Base
> -    member enum1: TestEnum optional=False
> -    member c_d: str optional=True
> -object Branch1
> -    member string: str optional=False
> -object Branch2
> -    member value: int optional=False
> -enum TestEnum ['base', 'c-d']
> -object TestUnion
> -    base Base
> -    tag enum1
> -    case base: Branch1
> -    case c-d: Branch2