Re: [Qemu-devel] [PATCH v7 05/18] qapi: Test for various name collisions

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> Expose some weaknesses in the generator: we don't always forbid
> the generation of structs that contain multiple members that map
> to the same C or QMP name.  This has already been marked FIXME in
> qapi.py in commit d90675f, but having more tests will make sure
> future patches produce desired behavior; and updating existing
> patches to better document things doesn't hurt, either.  Some of
> these collisions are already caught in the old-style parser
> checks, but ultimately we want all collisions to be caught in the
> new-style QAPISchema*.check() methods.
>
> This patch focuses on C struct members, and does not consider
> collisions between commands and events (affecting C function
> names), or even collisions between generated C type names with
> user type names (for things like automatic FOOList struct
> representing array types or FOOKind for an implicit enum).
>
> There are two types of struct collisions we want to catch:
>  1) Collision between two keys in a JSON object. qapi.py prevents
>     that within a single struct (see duplicate-key), but it is

"(see test duplicate-key)" or maybe "(see test duplicate-key.json)".

>     possible to have collisions between a type's members and its
>     base type's members (struct-base-clash, struct-base-clash-deep,
>     flat-union-cycle), and its flat union variant members

(existing tests struct-base-clash, struct-base-clash-deep, new test
flat-union-cycle)

>     (flat-union-clash-member).

(new test flat-union-clash-member)

>  2) Collision between two members of the C struct that is generated
>     for a given QAPI type:
>     a) Multiple QAPI names map to the same C name (args-name-clash)

(new test args-name-clash)

>     b) A QAPI name maps to a C name that is used for another purpose
>        (flat-union-clash-branch, struct-base-clash-base,
>        union-clash-data). We already fixed some such cases in commit

(new tests ...)

>        0f61af3e and 1e6c1616, but more remain.
>     c) Two C names generated for other purposes clash
>        (alternate-clash, union-clash-branches, union-clash-type,
>        flat-union-clash-type)

(updated test alternate-clash, new tests ...)

>
> Ultimately, if we need to have a flat union where an enum value

Suggest "where a tag value", to make it clear it's not just any enum.

> clashes with a base member name, we could change the generator to
> name the union (using 'foo.u.value' rather than 'foo.value') or
> otherwise munge the C name corresponding to enum tag values.  But

Suggest to drop enum here.

> unless such a need arises, it will probably be easier to just
> forbid these collisions.
>
> Some of these negative tests will be deleted later, and positive
> tests added to qapi-schema-test.json in their place, when the
> generator code is reworked to avoid particular code generation
> collisions in class 2).
>
> [Note that viewing this patch with git rename detection enabled
> may see some confusion due to renaming some tests while adding
> others, but where the content is similar enough that git picks
> the wrong pre- and post-patch files to associate]
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v7: rework commit message again, even more comments in tests, yet
> another rename (union-clash-members is now union-clash-branches)
> v6: rework commit message, alter test names and comments to be
> more descriptive, repurpose flat-union-cycle to better match its
> name (and not duplicate what the renamed flat-union-clash-branch
> tests), add new tests (union-clash-type, flat-union-clash-type)
> that detect an assertion failures
[...]
> diff --git a/tests/qapi-schema/alternate-clash.err b/tests/qapi-schema/alternate-clash.err
> index 51bea3e..a475ab6 100644
> --- a/tests/qapi-schema/alternate-clash.err
> +++ b/tests/qapi-schema/alternate-clash.err
> @@ -1 +1 @@
> -tests/qapi-schema/alternate-clash.json:2: Alternate 'Alt1' member 'ONE' clashes with 'one'
> +tests/qapi-schema/alternate-clash.json:7: Alternate 'Alt1' member 'a_b' clashes with 'a-b'
> diff --git a/tests/qapi-schema/alternate-clash.json b/tests/qapi-schema/alternate-clash.json
> index 3947935..6d73bc5 100644
> --- a/tests/qapi-schema/alternate-clash.json
> +++ b/tests/qapi-schema/alternate-clash.json
> @@ -1,3 +1,8 @@
> -# we detect C enum collisions in an alternate
> +# Alternate branch name collision
> +# Reject an alternate that would result in a collision in generated C
> +# names (this would try to generate two enum values 'ALT1_KIND_A_B').
> +# TODO: In the future, if alternates are simplified to not generate
> +# the implicit Alt1Kind enum, we would still have a collision with the
> +# resulting C union trying to have two members named 'a_b'.
>  { 'alternate': 'Alt1',
> -  'data': { 'one': 'str', 'ONE': 'int' } }
> +  'data': { 'a-b': 'str', 'a_b': 'int' } }

Ah, you're making the test slightly more robust.  Works for me.

> diff --git a/tests/qapi-schema/flat-union-branch-clash.out b/tests/qapi-schema/args-name-clash.err
> similarity index 100%
> rename from tests/qapi-schema/flat-union-branch-clash.out
> rename to tests/qapi-schema/args-name-clash.err
> diff --git a/tests/qapi-schema/args-name-clash.exit b/tests/qapi-schema/args-name-clash.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/args-name-clash.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/args-name-clash.json b/tests/qapi-schema/args-name-clash.json
> new file mode 100644
> index 0000000..9e8f889
> --- /dev/null
> +++ b/tests/qapi-schema/args-name-clash.json
> @@ -0,0 +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.
> +{ '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
> new file mode 100644
> index 0000000..9b2f6e4
> --- /dev/null
> +++ b/tests/qapi-schema/args-name-clash.out
> @@ -0,0 +1,6 @@
> +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
> diff --git a/tests/qapi-schema/duplicate-key.err b/tests/qapi-schema/duplicate-key.err
> index 768b276..6d02f83 100644
> --- a/tests/qapi-schema/duplicate-key.err
> +++ b/tests/qapi-schema/duplicate-key.err
> @@ -1 +1 @@
> -tests/qapi-schema/duplicate-key.json:2:10: Duplicate key "key"
> +tests/qapi-schema/duplicate-key.json:3:10: Duplicate key "key"
> diff --git a/tests/qapi-schema/duplicate-key.json b/tests/qapi-schema/duplicate-key.json
> index 1b55d88..14ac0e8 100644
> --- a/tests/qapi-schema/duplicate-key.json
> +++ b/tests/qapi-schema/duplicate-key.json
> @@ -1,2 +1,3 @@
> +# QAPI cannot include the same key more than once in any {}
>  { 'key': 'value',
>    'key': 'value' }
> diff --git a/tests/qapi-schema/flat-union-base-union.err b/tests/qapi-schema/flat-union-base-union.err
> index ede9859..28725ed 100644
> --- a/tests/qapi-schema/flat-union-base-union.err
> +++ b/tests/qapi-schema/flat-union-base-union.err
> @@ -1 +1 @@
> -tests/qapi-schema/flat-union-base-union.json:11: Base 'UnionBase' is not a valid struct
> +tests/qapi-schema/flat-union-base-union.json:14: Base 'UnionBase' is not a valid struct
> diff --git a/tests/qapi-schema/flat-union-base-union.json b/tests/qapi-schema/flat-union-base-union.json
> index 6a8ea68..98b4eba 100644
> --- a/tests/qapi-schema/flat-union-base-union.json
> +++ b/tests/qapi-schema/flat-union-base-union.json
> @@ -1,4 +1,7 @@
> -# we require the base to be a struct
> +# For now, we require the base to be a struct without variants
> +# TODO: It would be possible to allow a union as a base, as long as all
> +# permutations of QMP names exposed by base do not clash with any QMP
> +# member names added by local variants.
>  { 'enum': 'TestEnum',
>    'data': [ 'value1', 'value2' ] }
>  { 'struct': 'TestTypeA',
> diff --git a/tests/qapi-schema/flat-union-branch-clash.err b/tests/qapi-schema/flat-union-branch-clash.err
> deleted file mode 100644
> index f112766..0000000
> --- a/tests/qapi-schema/flat-union-branch-clash.err
> +++ /dev/null
> @@ -1 +0,0 @@
> -tests/qapi-schema/flat-union-branch-clash.json:10: Member name 'name' of branch 'value1' clashes with base 'Base'
> diff --git a/tests/qapi-schema/flat-union-clash-branch.err b/tests/qapi-schema/flat-union-clash-branch.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/flat-union-clash-branch.exit b/tests/qapi-schema/flat-union-clash-branch.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-clash-branch.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/flat-union-clash-branch.json b/tests/qapi-schema/flat-union-clash-branch.json
> new file mode 100644
> index 0000000..e593336
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-clash-branch.json
> @@ -0,0 +1,18 @@
> +# 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.
> +{ 'enum': 'TestEnum',
> +  'data': [ 'base', 'c-d' ] }
> +{ 'struct': 'Base',
> +  'data': { 'enum1': 'TestEnum', '*c_d': 'str' } }
> +{ 'struct': 'Branch1',
> +  'data': { 'string': 'str' } }
> +{ 'struct': 'Branch2',
> +  'data': { 'value': 'int' } }
> +{ 'union': 'TestUnion',
> +  'base': 'Base',
> +  'discriminator': 'enum1',
> +  'data': { 'base': 'Branch1',
> +            'c-d': 'Branch2' } }
> diff --git a/tests/qapi-schema/flat-union-clash-branch.out b/tests/qapi-schema/flat-union-clash-branch.out
> new file mode 100644
> index 0000000..8e0da73
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-clash-branch.out
> @@ -0,0 +1,14 @@
> +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
> diff --git a/tests/qapi-schema/flat-union-clash-member.err b/tests/qapi-schema/flat-union-clash-member.err
> new file mode 100644
> index 0000000..152d402
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-clash-member.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/flat-union-clash-member.json:10: Member name 'name' of branch 'value1' clashes with base 'Base'
> diff --git a/tests/qapi-schema/flat-union-branch-clash.exit b/tests/qapi-schema/flat-union-clash-member.exit
> similarity index 100%
> rename from tests/qapi-schema/flat-union-branch-clash.exit
> rename to tests/qapi-schema/flat-union-clash-member.exit
> diff --git a/tests/qapi-schema/flat-union-branch-clash.json b/tests/qapi-schema/flat-union-clash-member.json
> similarity index 85%
> rename from tests/qapi-schema/flat-union-branch-clash.json
> rename to tests/qapi-schema/flat-union-clash-member.json
> index 8fb054f..3d7e6c6 100644
> --- a/tests/qapi-schema/flat-union-branch-clash.json
> +++ b/tests/qapi-schema/flat-union-clash-member.json
> @@ -1,4 +1,4 @@
> -# we check for no duplicate keys between branches and base
> +# we check for no duplicate keys between branch members and base

Spelling out the clashing members wouldn't hurt.

>  { 'enum': 'TestEnum',
>    'data': [ 'value1', 'value2' ] }
>  { 'struct': 'Base',
> diff --git a/tests/qapi-schema/flat-union-clash-member.out b/tests/qapi-schema/flat-union-clash-member.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/flat-union-clash-type.err b/tests/qapi-schema/flat-union-clash-type.err
> new file mode 100644
> index 0000000..6e64d1d
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-clash-type.err
> @@ -0,0 +1,16 @@
> +Traceback (most recent call last):
> +  File "tests/qapi-schema/test-qapi.py", line 55, in <module>
> +    schema = QAPISchema(sys.argv[1])
> +  File "scripts/qapi.py", line 1116, in __init__
> +    self.check()
> +  File "scripts/qapi.py", line 1299, in check
> +    ent.check(self)
> +  File "scripts/qapi.py", line 962, in check
> +    self.variants.check(schema, members, seen)
> +  File "scripts/qapi.py", line 1024, in check
> +    v.check(schema, self.tag_member.type, vseen)
> +  File "scripts/qapi.py", line 1032, in check
> +    QAPISchemaObjectTypeMember.check(self, schema, [], seen)
> +  File "scripts/qapi.py", line 994, in check
> +    assert self.name not in seen
> +AssertionError
> diff --git a/tests/qapi-schema/flat-union-clash-type.exit b/tests/qapi-schema/flat-union-clash-type.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-clash-type.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/flat-union-clash-type.json b/tests/qapi-schema/flat-union-clash-type.json
> new file mode 100644
> index 0000000..eac51a4
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-clash-type.json
> @@ -0,0 +1,15 @@
> +# Flat union branch 'type'
> +# FIXME: this triggers an assertion failure. But even with that fixed, we
> +# would have a clash in generated C, between the outer tag 'type' and

"outer tag"?  I guess you mean the member type we inherit from Base.

> +# the branch name 'type' within the union. We should either reject this,
> +# or munge the generated C to let it compile.
> +{ 'enum': 'TestEnum',
> +  'data': [ 'type' ] }
> +{ 'struct': 'Base',
> +  'data': { 'type': 'TestEnum' } }
> +{ 'struct': 'Branch1',
> +  'data': { 'string': 'str' } }
> +{ 'union': 'TestUnion',
> +  'base': 'Base',
> +  'discriminator': 'type',
> +  'data': { 'type': 'Branch1' } }
> diff --git a/tests/qapi-schema/flat-union-clash-type.out b/tests/qapi-schema/flat-union-clash-type.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/flat-union-cycle.err b/tests/qapi-schema/flat-union-cycle.err
> new file mode 100644
> index 0000000..9b7978a
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-cycle.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/flat-union-cycle.json:7: Base 'Union' is not a valid struct
> diff --git a/tests/qapi-schema/flat-union-cycle.exit b/tests/qapi-schema/flat-union-cycle.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-cycle.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/flat-union-cycle.json b/tests/qapi-schema/flat-union-cycle.json
> new file mode 100644
> index 0000000..c66c4c9
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-cycle.json
> @@ -0,0 +1,8 @@
> +# Ensure that we have a sane error message for attempts at self-inheritance
> +# This test currently fails because we don't permit a union base, but
> +# even if we relax things to allow that in the future (see
> +# flat-union-base-union), self-inheritance should still fail.

Do we have a test for the simpler case of a struct inheriting from
itself?

I believe us merging struct and union types into a single object type is
more likely than us implementing union bases.  If I'm correct, we won't
need this test.

> +{ 'enum': 'Enum', 'data': [ 'okay' ] }
> +{ 'struct': 'Okay', 'data': { 'int': 'int' } }
> +{ 'union': 'Union', 'base': 'Union', 'discriminator': 'switch',
> +  'data': { 'okay': 'Okay' } }
> diff --git a/tests/qapi-schema/flat-union-cycle.out b/tests/qapi-schema/flat-union-cycle.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
> index 6897a6e..c511338 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -32,11 +32,14 @@
>              'dict1': 'UserDefTwoDict' } }
>
>  # for testing unions
> +# Among other things, test that a name collision between branches does
> +# not cause any problems (since only one branch can be in use at a time),
> +# by intentionally using two branches that both have a C member 'a_b'
>  { 'struct': 'UserDefA',
> -  'data': { 'boolean': 'bool' } }
> +  'data': { 'boolean': 'bool', '*a_b': 'int' } }
>
>  { 'struct': 'UserDefB',
> -  'data': { 'intb': 'int' } }
> +  'data': { 'intb': 'int', '*a-b': 'bool' } }
>
>  { 'union': 'UserDefFlatUnion',
>    'base': 'UserDefUnionBase',   # intentional forward reference
> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
> index 1f6e858..28a0b3c 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -71,6 +71,7 @@ enum QEnumTwo ['value1', 'value2']
>      prefix QENUM_TWO
>  object UserDefA
>      member boolean: bool optional=False
> +    member a_b: int optional=True
>  alternate UserDefAlternate
>      case uda: UserDefA
>      case s: str
> @@ -78,6 +79,7 @@ alternate UserDefAlternate
>  enum UserDefAlternateKind ['uda', 's', 'i']
>  object UserDefB
>      member intb: int optional=False
> +    member a-b: bool optional=True
>  object UserDefC
>      member string1: str optional=False
>      member string2: str optional=False
> diff --git a/tests/qapi-schema/struct-base-clash-base.err b/tests/qapi-schema/struct-base-clash-base.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/struct-base-clash-base.exit b/tests/qapi-schema/struct-base-clash-base.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/struct-base-clash-base.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/struct-base-clash-base.json b/tests/qapi-schema/struct-base-clash-base.json
> new file mode 100644
> index 0000000..0c84025
> --- /dev/null
> +++ b/tests/qapi-schema/struct-base-clash-base.json
> @@ -0,0 +1,9 @@
> +# Struct member 'base'
> +# FIXME: this parses, but then fails to compile due to a duplicate 'base'
> +# (one explicit in QMP, the other used to box the base class members).
> +# We should either reject the collision at parse time, or change the
> +# generated struct to allow this to compile.
> +{ 'struct': 'Base', 'data': {} }
> +{ 'struct': 'Sub',
> +  'base': 'Base',
> +  'data': { 'base': 'str' } }
> diff --git a/tests/qapi-schema/struct-base-clash-base.out b/tests/qapi-schema/struct-base-clash-base.out
> new file mode 100644
> index 0000000..e69a416
> --- /dev/null
> +++ b/tests/qapi-schema/struct-base-clash-base.out
> @@ -0,0 +1,5 @@
> +object :empty
> +object Base
> +object Sub
> +    base Base
> +    member base: str optional=False
> diff --git a/tests/qapi-schema/struct-base-clash-deep.err b/tests/qapi-schema/struct-base-clash-deep.err
> index e3e9f8d..f7a25a3 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:7: Member name 'name' clashes with base 'Base'
> +tests/qapi-schema/struct-base-clash-deep.json:10: Member name 'name' clashes with base 'Base'
> diff --git a/tests/qapi-schema/struct-base-clash-deep.json b/tests/qapi-schema/struct-base-clash-deep.json
> index 552fe94..fa873ab 100644
> --- a/tests/qapi-schema/struct-base-clash-deep.json
> +++ b/tests/qapi-schema/struct-base-clash-deep.json
> @@ -1,4 +1,7 @@
> -# we check for no duplicate keys with indirect base
> +# Reject attempts to duplicate QMP members
> +# Here, 'name' would have to appear twice on the wire, locally and
> +# indirectly for the grandparent base; the collision doesn't care that
> +# one instance is optional.
>  { 'struct': 'Base',
>    'data': { 'name': 'str' } }
>  { 'struct': 'Mid',
> diff --git a/tests/qapi-schema/struct-base-clash.err b/tests/qapi-schema/struct-base-clash.err
> index 3ac37fb..3a9f66b 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:4: Member name 'name' clashes with base 'Base'
> +tests/qapi-schema/struct-base-clash.json:5: Member name 'name' clashes with base 'Base'
> diff --git a/tests/qapi-schema/struct-base-clash.json b/tests/qapi-schema/struct-base-clash.json
> index f2afc9b..11aec80 100644
> --- a/tests/qapi-schema/struct-base-clash.json
> +++ b/tests/qapi-schema/struct-base-clash.json
> @@ -1,4 +1,5 @@
> -# we check for no duplicate keys with base
> +# Reject attempts to duplicate QMP members
> +# Here, 'name' would have to appear twice on the wire, locally and for base.
>  { 'struct': 'Base',
>    'data': { 'name': 'str' } }
>  { 'struct': 'Sub',
> diff --git a/tests/qapi-schema/union-clash-branches.err b/tests/qapi-schema/union-clash-branches.err
> new file mode 100644
> index 0000000..005c48d
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-branches.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/union-clash-branches.json:4: Union 'TestUnion' member 'a_b' clashes with 'a-b'
> diff --git a/tests/qapi-schema/union-clash-branches.exit b/tests/qapi-schema/union-clash-branches.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-branches.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/union-clash-branches.json b/tests/qapi-schema/union-clash-branches.json
> new file mode 100644
> index 0000000..31d135f
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-branches.json
> @@ -0,0 +1,5 @@
> +# Union branch name collision
> +# Reject a union that would result in a collision in generated C names (this
> +# would try to generate two enum values 'TEST_UNION_KIND_A_B').
> +{ 'union': 'TestUnion',
> +  'data': { 'a-b': 'int', 'a_b': 'str' } }
> diff --git a/tests/qapi-schema/union-clash-branches.out b/tests/qapi-schema/union-clash-branches.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/union-clash-data.err b/tests/qapi-schema/union-clash-data.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/union-clash-data.exit b/tests/qapi-schema/union-clash-data.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-data.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/union-clash-data.json b/tests/qapi-schema/union-clash-data.json
> new file mode 100644
> index 0000000..7308e69
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-data.json
> @@ -0,0 +1,7 @@
> +# Union branch 'data'
> +# FIXME: this parses, but then fails to compile due to a duplicate 'data'
> +# (one from the branch name, another as a filler to avoid an empty union).
> +# we should either detect the collision at parse time, or change the
> +# generated struct to allow this to compile.
> +{ 'union': 'TestUnion',
> +  'data': { 'data': 'int' } }
> diff --git a/tests/qapi-schema/union-clash-data.out b/tests/qapi-schema/union-clash-data.out
> new file mode 100644
> index 0000000..6277239
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-data.out
> @@ -0,0 +1,6 @@
> +object :empty
> +object :obj-int-wrapper
> +    member data: int optional=False
> +object TestUnion
> +    case data: :obj-int-wrapper
> +enum TestUnionKind ['data']
> diff --git a/tests/qapi-schema/union-clash-type.err b/tests/qapi-schema/union-clash-type.err
> new file mode 100644
> index 0000000..6e64d1d
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-type.err
> @@ -0,0 +1,16 @@
> +Traceback (most recent call last):
> +  File "tests/qapi-schema/test-qapi.py", line 55, in <module>
> +    schema = QAPISchema(sys.argv[1])
> +  File "scripts/qapi.py", line 1116, in __init__
> +    self.check()
> +  File "scripts/qapi.py", line 1299, in check
> +    ent.check(self)
> +  File "scripts/qapi.py", line 962, in check
> +    self.variants.check(schema, members, seen)
> +  File "scripts/qapi.py", line 1024, in check
> +    v.check(schema, self.tag_member.type, vseen)
> +  File "scripts/qapi.py", line 1032, in check
> +    QAPISchemaObjectTypeMember.check(self, schema, [], seen)
> +  File "scripts/qapi.py", line 994, in check
> +    assert self.name not in seen
> +AssertionError
> diff --git a/tests/qapi-schema/union-clash-type.exit b/tests/qapi-schema/union-clash-type.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-type.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/union-clash-type.json b/tests/qapi-schema/union-clash-type.json
> new file mode 100644
> index 0000000..38330b5
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-type.json
> @@ -0,0 +1,9 @@
> +# Union branch 'type'
> +# FIXME: this triggers an assertion failure. But even with that fixed, we
> +# would have a clash in generated C, between the outer union tag 'kind' and

Suggest "the simple union's implicit tag member 'kind' and"

> +# the branch name 'kind' within the union. We should either reject this,
> +# or munge the generated C to let it compile.
> +# TODO: Even when the generated C is switched to use 'type' rather than
> +# 'kind', to match the QMP spelling, the collision should still be detected.
> +{ 'union': 'TestUnion',
> +  'data': { 'kind': 'int', 'type': 'str' } }
> diff --git a/tests/qapi-schema/union-clash-type.out b/tests/qapi-schema/union-clash-type.out
> new file mode 100644
> index 0000000..e69de29

Found nothing that couldn't be touched up on commit.