diff mbox

[v6,05/16] qapi: Test for various name collisions

Message ID 1443497249-15361-6-git-send-email-eblake@redhat.com
State New
Headers show

Commit Message

Eric Blake Sept. 29, 2015, 3:27 a.m. UTC 
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 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.  This patch focuses on
C struct members, and does not consider collisions between commands
and events (affecting C function names), or even on collisions
between generated C type names with user type names (for things
like automatic FOOList struct representing array types).

There are two types of struct collisions we want to catch:
1) Collision that would require the QMP user to specify the
same key more than once inside a single JSON {} object (we
already prevent duplicate keys within a single type, so this
is triggered when merging multiple types into a single JSON
object via base classes or flat unions)
2) Collision between two members of the C struct that is
generated for a given qapi type:
  a) a collision occurs from two qapi names mapping to the
same C name
  b) the collision is with a C name used for another purpose
(we already fixed 'kind' in commit 0f61af3e, and one use of
'base' in commit 1e6c1616 for unions, but we have another
use of 'base' in structs, and still collide with 'data' in
unions, not to mention the tag 'type'/'kind' and the various
tag values).

Oddly enough, while commit 1e6c1616 fixed a type 2b collision
with 'base', it introduced a different collision that was not
previously possible: now that the base members are no longer
boxed behind 'base', they can collide with the enum tag members
(used as member names within the embedded C union).

Ultimately, if we need to have a flat union where an enum
value clashes with a base member name, we could change the
generator to use something like '_tag_value' instead of
'value' for the C name of union members; but until such a
need arises, it will probably be easier to just forbid these
collisions.

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.

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 so that a particular collision no
longer causes failed code generation.

[git rename detection may be a bit confused by the actions of
this commit, since it both creates and renames files that are
equally small and similar in content]

Signed-off-by: Eric Blake <eblake@redhat.com>

---
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
---
 tests/Makefile                                           | 10 +++++++++-
 .../{flat-union-branch-clash.out => args-name-clash.err} |  0
 tests/qapi-schema/args-name-clash.exit                   |  1 +
 tests/qapi-schema/args-name-clash.json                   |  2 ++
 tests/qapi-schema/args-name-clash.out                    |  6 ++++++
 tests/qapi-schema/flat-union-branch-clash.err            |  1 -
 tests/qapi-schema/flat-union-clash-branch.err            |  0
 tests/qapi-schema/flat-union-clash-branch.exit           |  1 +
 tests/qapi-schema/flat-union-clash-branch.json           | 15 +++++++++++++++
 tests/qapi-schema/flat-union-clash-branch.out            | 14 ++++++++++++++
 tests/qapi-schema/flat-union-clash-member.err            |  1 +
 ...on-branch-clash.exit => flat-union-clash-member.exit} |  0
 ...on-branch-clash.json => flat-union-clash-member.json} |  2 +-
 tests/qapi-schema/flat-union-clash-member.out            |  0
 tests/qapi-schema/flat-union-clash-type.err              | 16 ++++++++++++++++
 tests/qapi-schema/flat-union-clash-type.exit             |  1 +
 tests/qapi-schema/flat-union-clash-type.json             | 11 +++++++++++
 tests/qapi-schema/flat-union-clash-type.out              |  0
 tests/qapi-schema/flat-union-cycle.err                   |  1 +
 tests/qapi-schema/flat-union-cycle.exit                  |  1 +
 tests/qapi-schema/flat-union-cycle.json                  |  7 +++++++
 tests/qapi-schema/flat-union-cycle.out                   |  0
 tests/qapi-schema/qapi-schema-test.json                  |  7 +++++--
 tests/qapi-schema/qapi-schema-test.out                   |  2 ++
 tests/qapi-schema/struct-base-clash-base.err             |  0
 tests/qapi-schema/struct-base-clash-base.exit            |  1 +
 tests/qapi-schema/struct-base-clash-base.json            |  6 ++++++
 tests/qapi-schema/struct-base-clash-base.out             |  5 +++++
 tests/qapi-schema/union-clash-data.err                   |  0
 tests/qapi-schema/union-clash-data.exit                  |  1 +
 tests/qapi-schema/union-clash-data.json                  |  4 ++++
 tests/qapi-schema/union-clash-data.out                   |  6 ++++++
 tests/qapi-schema/union-clash-members.err                |  1 +
 tests/qapi-schema/union-clash-members.exit               |  1 +
 tests/qapi-schema/union-clash-members.json               |  3 +++
 tests/qapi-schema/union-clash-members.out                |  0
 tests/qapi-schema/union-clash-type.err                   | 16 ++++++++++++++++
 tests/qapi-schema/union-clash-type.exit                  |  1 +
 tests/qapi-schema/union-clash-type.json                  |  3 +++
 tests/qapi-schema/union-clash-type.out                   |  0
 40 files changed, 142 insertions(+), 5 deletions(-)
 rename tests/qapi-schema/{flat-union-branch-clash.out => args-name-clash.err} (100%)
 create mode 100644 tests/qapi-schema/args-name-clash.exit
 create mode 100644 tests/qapi-schema/args-name-clash.json
 create mode 100644 tests/qapi-schema/args-name-clash.out
 delete mode 100644 tests/qapi-schema/flat-union-branch-clash.err
 create mode 100644 tests/qapi-schema/flat-union-clash-branch.err
 create mode 100644 tests/qapi-schema/flat-union-clash-branch.exit
 create mode 100644 tests/qapi-schema/flat-union-clash-branch.json
 create mode 100644 tests/qapi-schema/flat-union-clash-branch.out
 create mode 100644 tests/qapi-schema/flat-union-clash-member.err
 rename tests/qapi-schema/{flat-union-branch-clash.exit => flat-union-clash-member.exit} (100%)
 rename tests/qapi-schema/{flat-union-branch-clash.json => flat-union-clash-member.json} (85%)
 create mode 100644 tests/qapi-schema/flat-union-clash-member.out
 create mode 100644 tests/qapi-schema/flat-union-clash-type.err
 create mode 100644 tests/qapi-schema/flat-union-clash-type.exit
 create mode 100644 tests/qapi-schema/flat-union-clash-type.json
 create mode 100644 tests/qapi-schema/flat-union-clash-type.out
 create mode 100644 tests/qapi-schema/flat-union-cycle.err
 create mode 100644 tests/qapi-schema/flat-union-cycle.exit
 create mode 100644 tests/qapi-schema/flat-union-cycle.json
 create mode 100644 tests/qapi-schema/flat-union-cycle.out
 create mode 100644 tests/qapi-schema/struct-base-clash-base.err
 create mode 100644 tests/qapi-schema/struct-base-clash-base.exit
 create mode 100644 tests/qapi-schema/struct-base-clash-base.json
 create mode 100644 tests/qapi-schema/struct-base-clash-base.out
 create mode 100644 tests/qapi-schema/union-clash-data.err
 create mode 100644 tests/qapi-schema/union-clash-data.exit
 create mode 100644 tests/qapi-schema/union-clash-data.json
 create mode 100644 tests/qapi-schema/union-clash-data.out
 create mode 100644 tests/qapi-schema/union-clash-members.err
 create mode 100644 tests/qapi-schema/union-clash-members.exit
 create mode 100644 tests/qapi-schema/union-clash-members.json
 create mode 100644 tests/qapi-schema/union-clash-members.out
 create mode 100644 tests/qapi-schema/union-clash-type.err
 create mode 100644 tests/qapi-schema/union-clash-type.exit
 create mode 100644 tests/qapi-schema/union-clash-type.json
 create mode 100644 tests/qapi-schema/union-clash-type.out

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

Comments

Markus Armbruster Sept. 29, 2015, 12:33 p.m. UTC | #1 
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 name.

C or QMP name, perhaps?

>                      This has already been marked FIXME in
> qapi.py in commit d90675f, but having more tests will make sure
> future patches produce desired behavior.  This patch focuses on
> C struct members, and does not consider collisions between commands
> and events (affecting C function names), or even on collisions
> between generated C type names with user type names (for things
> like automatic FOOList struct representing array types).
>
> There are two types of struct collisions we want to catch:
> 1) Collision that would require the QMP user to specify the
> same key more than once inside a single JSON {} object (we
> already prevent duplicate keys within a single type, so this
> is triggered when merging multiple types into a single JSON
> object via base classes or flat unions)

I understand what you want to say here, but find it a bit awkward.
Perhaps:

    1) Collision between two keys in a JSON object.  qapi.py should prevent
    that, but currently fails to do so for collisions between a type's
    members, its base type's members, and its flat union variant members.

Could add pointers to the tests covering this type of collision.

> 2) Collision between two members of the C struct that is
> generated for a given qapi type:

We generally spell it QAPI.

>   a) a collision occurs from two qapi names mapping to the
> same C name
>   b) the collision is with a C name used for another purpose
> (we already fixed 'kind' in commit 0f61af3e, and one use of
> 'base' in commit 1e6c1616 for unions, but we have another
> use of 'base' in structs, and still collide with 'data' in
> unions, not to mention the tag 'type'/'kind' and the various
> tag values).

I'm not sure this makes sense to readers who aren't already familiar
with our name clashing issues.

Perhaps:

    2) Collision between members of the C struct that is generated for a
    given qapi type.  Two cases:

      a) Multiple QAPI names map to the same C name.
      b) A QAPI name maps to a C name that is used for another purpose.
         We already fixed such cases in commit 0f61af3e and commit 1e6c1616,
         but more cases are left.

Avoids getting bogged down in details prematurely.

Could add pointers to the tests covering each type of collision.

> Oddly enough, while commit 1e6c1616 fixed a type 2b collision
> with 'base',

What collided with 'base' before 1e6c1616 and not after?

>              it introduced a different collision that was not
> previously possible: now that the base members are no longer
> boxed behind 'base', they can collide with the enum tag members
> (used as member names within the embedded C union).
>
> Ultimately, if we need to have a flat union where an enum
> value clashes with a base member name, we could change the

Suggest "an enum tag member", because this is confusing enough without
using multiple names for the same thing :)

> generator to use something like '_tag_value' instead of
> 'value' for the C name of union members;

Or wrap the base in a struct, or give the union member a name.

>                                          but until such a
> need arises, it will probably be easier to just forbid these
> collisions.

Again, I'm not sure this makes sense to readers who aren't already
familiar with our name clashing issues.

Do you fix any of this later in your series?  If yes, you could punt
detailed bug descriptions to the patches that fixes them.

> 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.
>
> 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 so that a particular collision no
> longer causes failed code generation.
>
> [git rename detection may be a bit confused by the actions of
> this commit, since it both creates and renames files that are
> equally small and similar in content]

Why square brackets?

> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> 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
> ---
>  tests/Makefile                                           | 10 +++++++++-
>  .../{flat-union-branch-clash.out => args-name-clash.err} |  0
>  tests/qapi-schema/args-name-clash.exit                   |  1 +
>  tests/qapi-schema/args-name-clash.json                   |  2 ++
>  tests/qapi-schema/args-name-clash.out                    |  6 ++++++
>  tests/qapi-schema/flat-union-branch-clash.err            |  1 -
>  tests/qapi-schema/flat-union-clash-branch.err            |  0
>  tests/qapi-schema/flat-union-clash-branch.exit           |  1 +
>  tests/qapi-schema/flat-union-clash-branch.json           | 15 +++++++++++++++
>  tests/qapi-schema/flat-union-clash-branch.out            | 14 ++++++++++++++
>  tests/qapi-schema/flat-union-clash-member.err            |  1 +
>  ...on-branch-clash.exit => flat-union-clash-member.exit} |  0
>  ...on-branch-clash.json => flat-union-clash-member.json} |  2 +-
>  tests/qapi-schema/flat-union-clash-member.out            |  0
>  tests/qapi-schema/flat-union-clash-type.err              | 16 ++++++++++++++++
>  tests/qapi-schema/flat-union-clash-type.exit             |  1 +
>  tests/qapi-schema/flat-union-clash-type.json             | 11 +++++++++++
>  tests/qapi-schema/flat-union-clash-type.out              |  0
>  tests/qapi-schema/flat-union-cycle.err                   |  1 +
>  tests/qapi-schema/flat-union-cycle.exit                  |  1 +
>  tests/qapi-schema/flat-union-cycle.json                  |  7 +++++++
>  tests/qapi-schema/flat-union-cycle.out                   |  0
>  tests/qapi-schema/qapi-schema-test.json                  |  7 +++++--
>  tests/qapi-schema/qapi-schema-test.out                   |  2 ++
>  tests/qapi-schema/struct-base-clash-base.err             |  0
>  tests/qapi-schema/struct-base-clash-base.exit            |  1 +
>  tests/qapi-schema/struct-base-clash-base.json            |  6 ++++++
>  tests/qapi-schema/struct-base-clash-base.out             |  5 +++++
>  tests/qapi-schema/union-clash-data.err                   |  0
>  tests/qapi-schema/union-clash-data.exit                  |  1 +
>  tests/qapi-schema/union-clash-data.json                  |  4 ++++
>  tests/qapi-schema/union-clash-data.out                   |  6 ++++++
>  tests/qapi-schema/union-clash-members.err                |  1 +
>  tests/qapi-schema/union-clash-members.exit               |  1 +
>  tests/qapi-schema/union-clash-members.json               |  3 +++
>  tests/qapi-schema/union-clash-members.out                |  0
>  tests/qapi-schema/union-clash-type.err                   | 16 ++++++++++++++++
>  tests/qapi-schema/union-clash-type.exit                  |  1 +
>  tests/qapi-schema/union-clash-type.json                  |  3 +++
>  tests/qapi-schema/union-clash-type.out                   |  0
>  40 files changed, 142 insertions(+), 5 deletions(-)
>  rename tests/qapi-schema/{flat-union-branch-clash.out => args-name-clash.err} (100%)
>  create mode 100644 tests/qapi-schema/args-name-clash.exit
>  create mode 100644 tests/qapi-schema/args-name-clash.json
>  create mode 100644 tests/qapi-schema/args-name-clash.out
>  delete mode 100644 tests/qapi-schema/flat-union-branch-clash.err
>  create mode 100644 tests/qapi-schema/flat-union-clash-branch.err
>  create mode 100644 tests/qapi-schema/flat-union-clash-branch.exit
>  create mode 100644 tests/qapi-schema/flat-union-clash-branch.json
>  create mode 100644 tests/qapi-schema/flat-union-clash-branch.out
>  create mode 100644 tests/qapi-schema/flat-union-clash-member.err
>  rename tests/qapi-schema/{flat-union-branch-clash.exit => flat-union-clash-member.exit} (100%)
>  rename tests/qapi-schema/{flat-union-branch-clash.json => flat-union-clash-member.json} (85%)
>  create mode 100644 tests/qapi-schema/flat-union-clash-member.out
>  create mode 100644 tests/qapi-schema/flat-union-clash-type.err
>  create mode 100644 tests/qapi-schema/flat-union-clash-type.exit
>  create mode 100644 tests/qapi-schema/flat-union-clash-type.json
>  create mode 100644 tests/qapi-schema/flat-union-clash-type.out
>  create mode 100644 tests/qapi-schema/flat-union-cycle.err
>  create mode 100644 tests/qapi-schema/flat-union-cycle.exit
>  create mode 100644 tests/qapi-schema/flat-union-cycle.json
>  create mode 100644 tests/qapi-schema/flat-union-cycle.out
>  create mode 100644 tests/qapi-schema/struct-base-clash-base.err
>  create mode 100644 tests/qapi-schema/struct-base-clash-base.exit
>  create mode 100644 tests/qapi-schema/struct-base-clash-base.json
>  create mode 100644 tests/qapi-schema/struct-base-clash-base.out
>  create mode 100644 tests/qapi-schema/union-clash-data.err
>  create mode 100644 tests/qapi-schema/union-clash-data.exit
>  create mode 100644 tests/qapi-schema/union-clash-data.json
>  create mode 100644 tests/qapi-schema/union-clash-data.out
>  create mode 100644 tests/qapi-schema/union-clash-members.err
>  create mode 100644 tests/qapi-schema/union-clash-members.exit
>  create mode 100644 tests/qapi-schema/union-clash-members.json
>  create mode 100644 tests/qapi-schema/union-clash-members.out
>  create mode 100644 tests/qapi-schema/union-clash-type.err
>  create mode 100644 tests/qapi-schema/union-clash-type.exit
>  create mode 100644 tests/qapi-schema/union-clash-type.json
>  create mode 100644 tests/qapi-schema/union-clash-type.out
>
> diff --git a/tests/Makefile b/tests/Makefile
> index 6fd5885..d43b8e9 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -238,6 +238,7 @@ qapi-schema += args-invalid.json
>  qapi-schema += args-member-array-bad.json
>  qapi-schema += args-member-array.json
>  qapi-schema += args-member-unknown.json
> +qapi-schema += args-name-clash.json
>  qapi-schema += args-union.json
>  qapi-schema += args-unknown.json
>  qapi-schema += bad-base.json
> @@ -273,7 +274,10 @@ qapi-schema += flat-union-bad-base.json
>  qapi-schema += flat-union-bad-discriminator.json
>  qapi-schema += flat-union-base-any.json
>  qapi-schema += flat-union-base-union.json
> -qapi-schema += flat-union-branch-clash.json
> +qapi-schema += flat-union-clash-branch.json
> +qapi-schema += flat-union-clash-member.json
> +qapi-schema += flat-union-clash-type.json
> +qapi-schema += flat-union-cycle.json
>  qapi-schema += flat-union-inline.json
>  qapi-schema += flat-union-int-branch.json
>  qapi-schema += flat-union-invalid-branch-key.json
> @@ -315,6 +319,7 @@ qapi-schema += returns-dict.json
>  qapi-schema += returns-int.json
>  qapi-schema += returns-unknown.json
>  qapi-schema += returns-whitelist.json
> +qapi-schema += struct-base-clash-base.json
>  qapi-schema += struct-base-clash-deep.json
>  qapi-schema += struct-base-clash.json
>  qapi-schema += struct-data-invalid.json
> @@ -328,6 +333,9 @@ qapi-schema += unclosed-string.json
>  qapi-schema += unicode-str.json
>  qapi-schema += union-bad-branch.json
>  qapi-schema += union-base-no-discriminator.json
> +qapi-schema += union-clash-data.json
> +qapi-schema += union-clash-members.json
> +qapi-schema += union-clash-type.json
>  qapi-schema += union-invalid-base.json
>  qapi-schema += union-max.json
>  qapi-schema += union-optional-branch.json
> 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..19bf792
> --- /dev/null
> +++ b/tests/qapi-schema/args-name-clash.json
> @@ -0,0 +1,2 @@
> +# FIXME - we should reject data with members that clash when mapped to C names
> +{ 'command': 'oops', 'data': { 'a-b': 'str', 'a_b': 'str' } }

Here, the collision is fairly obvious.  We could spell it out anyway:

# Multiple members mapping to the same C identifier: both 'a-b' and
# 'a_b' map to a_b'
# FIXME they clash in generated C, should reject them cleanly instead.

Unfortunately, the test doesn't actually make the clash visible.  Same
for all the other tests that produce clashes in C or QMP without
actually upsetting the generator.  Oh well, good enough.

> 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/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..cda3af5
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-clash-branch.json
> @@ -0,0 +1,15 @@
> +# FIXME: this parses, but then fails to compile due to a duplicate 'c_d'
> +# we should either detect the collision or allow this to compile

Here, the collision is less obvious, mostly because it gets lost in the
scaffolding.  Let's spell it out:

# Base members and tag enum members mapping to the same C identifier:
# both 'c-d' and 'c_d' map to c_d'
# FIXME they clash in generated C, should reject them cleanly instead.

> +{ '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..87e5fd8
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-clash-type.json
> @@ -0,0 +1,11 @@
> +# FIXME: detect this collision, rather than triggering an assertion failure

Here, the collision is unobvious.  Could you spell it out, please?

> +{ '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..d4f2a09
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-cycle.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/flat-union-cycle.json:6: 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..7414277
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-cycle.json
> @@ -0,0 +1,7 @@
> +# 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, this should still fail)

Humor me: start sentences with a capital letter and end them with
punctuation.  Headlines don't end with punctuation, but still start with
a capital letter.

> +{ 'enum': 'Enum', 'data': [ 'okay' ] }
> +{ 'struct': 'Okay', 'data': { 'int': 'int' } }
> +{ 'union': 'Union', 'base': 'Union', 'discriminator': 'switch',
> +  'data': { 'okay': 'Okay' } }

Currently tests the exact same thing as flat-union-base-union.json,
doesn't it?

> 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..fb25128
> --- /dev/null
> +++ b/tests/qapi-schema/struct-base-clash-base.json
> @@ -0,0 +1,6 @@
> +# FIXME: this parses, but then fails to compile due to a duplicate 'base'
> +# we should either detect the collision or allow this to compile

Suggest:

# Struct type member 'base'
# FIXME clashes in generated C, should either reject 'base' cleanly or
# make it work.

> +{ '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/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..322f61b
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-data.json
> @@ -0,0 +1,4 @@
> +# FIXME: this parses, but then fails to compile due to a duplicate 'data'
> +# we should either detect the collision or allow this to compile

Suggest:

# Union tag member 'data'
# FIXME clashes in generated C, should either reject 'data' cleanly or
# make it work.

> +{ '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-members.err b/tests/qapi-schema/union-clash-members.err
> new file mode 100644
> index 0000000..b77aeb8
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-members.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/union-clash-members.json:2: Union 'TestUnion' member 'a_b' clashes with 'a-b'
> diff --git a/tests/qapi-schema/union-clash-members.exit b/tests/qapi-schema/union-clash-members.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-members.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/union-clash-members.json b/tests/qapi-schema/union-clash-members.json
> new file mode 100644
> index 0000000..0393ed8
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-members.json
> @@ -0,0 +1,3 @@
> +# we reject unions where branch names clash when mapped to C
> +{ 'union': 'TestUnion',
> +  'data': { 'a-b': 'int', 'a_b': 'str' } }
> diff --git a/tests/qapi-schema/union-clash-members.out b/tests/qapi-schema/union-clash-members.out
> new file mode 100644
> index 0000000..e69de29
> 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..2294f4f
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-type.json
> @@ -0,0 +1,3 @@
> +# FIXME: detect this collision, rather than triggering an assertion failure

Could you spell out the collision?

> +{ '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
Eric Blake Sept. 29, 2015, 2:11 p.m. UTC | #2 
On 09/29/2015 06:33 AM, Markus Armbruster wrote:
> 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 name.
> 
> C or QMP name, perhaps?

Lots of good advice for an improved commit message and test comments;
looks like I'll do a v7 respin to address them.


> 
>> Oddly enough, while commit 1e6c1616 fixed a type 2b collision
>> with 'base',
> 
> What collided with 'base' before 1e6c1616 and not after?

Uggh. I wrote that comment late at night, and it shows.  I was mixing up
struct bases (which are boxed behind a 'FOO *base' member) and flat
unions; but flat union base classes have always been expressed on a
per-member basis.  I'll either drop this paragraph entirely, or come up
with a real example (about the only one that might be left is when we
dropped the mis-use of 'kind', we were then introducing the possibility
of collision with the 'discriminator':'name' - and that's one of the two
tests added in this commit where we assert).


> 
>>              it introduced a different collision that was not
>> previously possible: now that the base members are no longer
>> boxed behind 'base', they can collide with the enum tag members
>> (used as member names within the embedded C union).
>>
>> Ultimately, if we need to have a flat union where an enum
>> value clashes with a base member name, we could change the
> 
> Suggest "an enum tag member", because this is confusing enough without
> using multiple names for the same thing :)
> 
>> generator to use something like '_tag_value' instead of
>> 'value' for the C name of union members;
> 
> Or wrap the base in a struct, or give the union member a name.

Giving the union member a name might be the least amount of typing, but
would still touch quite a bit of code if we have to do it.

> 
>>                                          but until such a
>> need arises, it will probably be easier to just forbid these
>> collisions.
> 
> Again, I'm not sure this makes sense to readers who aren't already
> familiar with our name clashing issues.
> 
> Do you fix any of this later in your series?  If yes, you could punt
> detailed bug descriptions to the patches that fixes them.

Yes, I fix a number of the issues later on (although only the two
assertion failures get fixed in subset A; the rest of the fixups are in
the tail end of v5 which will become subset B).

> 
>> 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.
>>
>> 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 so that a particular collision no
>> longer causes failed code generation.
>>
>> [git rename detection may be a bit confused by the actions of
>> this commit, since it both creates and renames files that are
>> equally small and similar in content]
> 
> Why square brackets?
> 

To delineate that this paragraph is is unrelated to the contents of the
patch, but instead guides a reader to how to interpret the git diff of
the patch - if rename detection is enabled, git botches the rename
detection, doing things like:

>> ---
>>  tests/Makefile                                           | 10 +++++++++-
>>  .../{flat-union-branch-clash.out => args-name-clash.err} |  0

claiming this file is a rename (in reality, I renamed
flat-union-branch-clash.out to flat-union-clash-branch.out; and .out
files never really rename to .err files other than the fact that both
happened to be empty).

We could drop it if desired, but I think it makes sense to keep the
comment in qemu.git (because 'git diff' will botch the rename detection
no matter how much in the future you are inspecting this patch).


>> +++ b/tests/qapi-schema/args-name-clash.json
>> @@ -0,0 +1,2 @@
>> +# FIXME - we should reject data with members that clash when mapped to C names
>> +{ 'command': 'oops', 'data': { 'a-b': 'str', 'a_b': 'str' } }
> 
> Here, the collision is fairly obvious.  We could spell it out anyway:
> 
> # Multiple members mapping to the same C identifier: both 'a-b' and
> # 'a_b' map to a_b'
> # FIXME they clash in generated C, should reject them cleanly instead.
> 
> Unfortunately, the test doesn't actually make the clash visible.  Same
> for all the other tests that produce clashes in C or QMP without
> actually upsetting the generator.  Oh well, good enough.

And for every FIXME I add here, a later patch in the v5 series resolves
it (either by making it a parse error, or by removing the generated
collision).
diff mbox

Patch

diff --git a/tests/Makefile b/tests/Makefile
index 6fd5885..d43b8e9 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -238,6 +238,7 @@  qapi-schema += args-invalid.json
 qapi-schema += args-member-array-bad.json
 qapi-schema += args-member-array.json
 qapi-schema += args-member-unknown.json
+qapi-schema += args-name-clash.json
 qapi-schema += args-union.json
 qapi-schema += args-unknown.json
 qapi-schema += bad-base.json
@@ -273,7 +274,10 @@  qapi-schema += flat-union-bad-base.json
 qapi-schema += flat-union-bad-discriminator.json
 qapi-schema += flat-union-base-any.json
 qapi-schema += flat-union-base-union.json
-qapi-schema += flat-union-branch-clash.json
+qapi-schema += flat-union-clash-branch.json
+qapi-schema += flat-union-clash-member.json
+qapi-schema += flat-union-clash-type.json
+qapi-schema += flat-union-cycle.json
 qapi-schema += flat-union-inline.json
 qapi-schema += flat-union-int-branch.json
 qapi-schema += flat-union-invalid-branch-key.json
@@ -315,6 +319,7 @@  qapi-schema += returns-dict.json
 qapi-schema += returns-int.json
 qapi-schema += returns-unknown.json
 qapi-schema += returns-whitelist.json
+qapi-schema += struct-base-clash-base.json
 qapi-schema += struct-base-clash-deep.json
 qapi-schema += struct-base-clash.json
 qapi-schema += struct-data-invalid.json
@@ -328,6 +333,9 @@  qapi-schema += unclosed-string.json
 qapi-schema += unicode-str.json
 qapi-schema += union-bad-branch.json
 qapi-schema += union-base-no-discriminator.json
+qapi-schema += union-clash-data.json
+qapi-schema += union-clash-members.json
+qapi-schema += union-clash-type.json
 qapi-schema += union-invalid-base.json
 qapi-schema += union-max.json
 qapi-schema += union-optional-branch.json
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..19bf792
--- /dev/null
+++ b/tests/qapi-schema/args-name-clash.json
@@ -0,0 +1,2 @@ 
+# FIXME - we should reject data with members that clash when mapped to C names
+{ '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/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..cda3af5
--- /dev/null
+++ b/tests/qapi-schema/flat-union-clash-branch.json
@@ -0,0 +1,15 @@ 
+# FIXME: this parses, but then fails to compile due to a duplicate 'c_d'
+# we should either detect the collision or 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
 { '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..87e5fd8
--- /dev/null
+++ b/tests/qapi-schema/flat-union-clash-type.json
@@ -0,0 +1,11 @@ 
+# FIXME: detect this collision, rather than triggering an assertion failure
+{ '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..d4f2a09
--- /dev/null
+++ b/tests/qapi-schema/flat-union-cycle.err
@@ -0,0 +1 @@ 
+tests/qapi-schema/flat-union-cycle.json:6: 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..7414277
--- /dev/null
+++ b/tests/qapi-schema/flat-union-cycle.json
@@ -0,0 +1,7 @@ 
+# 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, this should still fail)
+{ '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..fb25128
--- /dev/null
+++ b/tests/qapi-schema/struct-base-clash-base.json
@@ -0,0 +1,6 @@ 
+# FIXME: this parses, but then fails to compile due to a duplicate 'base'
+# we should either detect the collision or 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/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..322f61b
--- /dev/null
+++ b/tests/qapi-schema/union-clash-data.json
@@ -0,0 +1,4 @@ 
+# FIXME: this parses, but then fails to compile due to a duplicate 'data'
+# we should either detect the collision or 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-members.err b/tests/qapi-schema/union-clash-members.err
new file mode 100644
index 0000000..b77aeb8
--- /dev/null
+++ b/tests/qapi-schema/union-clash-members.err
@@ -0,0 +1 @@ 
+tests/qapi-schema/union-clash-members.json:2: Union 'TestUnion' member 'a_b' clashes with 'a-b'
diff --git a/tests/qapi-schema/union-clash-members.exit b/tests/qapi-schema/union-clash-members.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/union-clash-members.exit
@@ -0,0 +1 @@ 
+1
diff --git a/tests/qapi-schema/union-clash-members.json b/tests/qapi-schema/union-clash-members.json
new file mode 100644
index 0000000..0393ed8
--- /dev/null
+++ b/tests/qapi-schema/union-clash-members.json
@@ -0,0 +1,3 @@ 
+# we reject unions where branch names clash when mapped to C
+{ 'union': 'TestUnion',
+  'data': { 'a-b': 'int', 'a_b': 'str' } }
diff --git a/tests/qapi-schema/union-clash-members.out b/tests/qapi-schema/union-clash-members.out
new file mode 100644
index 0000000..e69de29
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..2294f4f
--- /dev/null
+++ b/tests/qapi-schema/union-clash-type.json
@@ -0,0 +1,3 @@ 
+# FIXME: detect this collision, rather than triggering an assertion failure
+{ 'union': 'TestUnion',
+  'data': { 'kind': 'int', 'type': 'str' } }