Eric Blake <ebl...@redhat.com> 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 <ebl...@redhat.com>
>
> ---
> 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
