Re: [Qemu-devel] [PATCH v5 03/46] qapi: Test for C member name collisions

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Expose some weaknesses in the generator: we don't always forbid
> the generation of structs that contain multiple members that map

Slightly misleading.  args-name-clash is a clash between command
arguments.  These are a struct internally, but we don't currently
generate an actual struct for it, only an argument list.

> to the same C name.  This has already been marked FIXME in
> qapi.py, but having more tests will make sure future patches
> produce desired behavior.

Point to commit d90675f?

> Some of these tests will be deleted later, and a positive test
> added to qapi-schema-test.json in its place, when the code is

"in their place"?

> reworked so that the collision no longer occurs.
>
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  tests/Makefile                                  |  6 ++++++
>  tests/qapi-schema/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.json  |  2 +-
>  tests/qapi-schema/flat-union-branch-clash2.err  |  0
>  tests/qapi-schema/flat-union-branch-clash2.exit |  1 +
>  tests/qapi-schema/flat-union-branch-clash2.json | 14 ++++++++++++++
>  tests/qapi-schema/flat-union-branch-clash2.out  | 14 ++++++++++++++
>  tests/qapi-schema/flat-union-cycle.err          |  1 +
>  tests/qapi-schema/flat-union-cycle.exit         |  1 +
>  tests/qapi-schema/flat-union-cycle.json         |  6 ++++++
>  tests/qapi-schema/flat-union-cycle.out          |  0
>  tests/qapi-schema/qapi-schema-test.json         |  5 +++--
>  tests/qapi-schema/qapi-schema-test.out          |  2 ++
>  tests/qapi-schema/struct-base-clash2.err        |  0
>  tests/qapi-schema/struct-base-clash2.exit       |  1 +
>  tests/qapi-schema/struct-base-clash2.json       |  5 +++++
>  tests/qapi-schema/struct-base-clash2.out        |  5 +++++
>  tests/qapi-schema/union-clash.err               |  1 +
>  tests/qapi-schema/union-clash.exit              |  1 +
>  tests/qapi-schema/union-clash.json              |  3 +++
>  tests/qapi-schema/union-clash.out               |  0
>  tests/qapi-schema/union-clash2.err              |  0
>  tests/qapi-schema/union-clash2.exit             |  1 +
>  tests/qapi-schema/union-clash2.json             |  3 +++
>  tests/qapi-schema/union-clash2.out              |  6 ++++++
>  28 files changed, 84 insertions(+), 3 deletions(-)
>  create mode 100644 tests/qapi-schema/args-name-clash.err
>  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
>  create mode 100644 tests/qapi-schema/flat-union-branch-clash2.err
>  create mode 100644 tests/qapi-schema/flat-union-branch-clash2.exit
>  create mode 100644 tests/qapi-schema/flat-union-branch-clash2.json
>  create mode 100644 tests/qapi-schema/flat-union-branch-clash2.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-clash2.err
>  create mode 100644 tests/qapi-schema/struct-base-clash2.exit
>  create mode 100644 tests/qapi-schema/struct-base-clash2.json
>  create mode 100644 tests/qapi-schema/struct-base-clash2.out
>  create mode 100644 tests/qapi-schema/union-clash.err
>  create mode 100644 tests/qapi-schema/union-clash.exit
>  create mode 100644 tests/qapi-schema/union-clash.json
>  create mode 100644 tests/qapi-schema/union-clash.out
>  create mode 100644 tests/qapi-schema/union-clash2.err
>  create mode 100644 tests/qapi-schema/union-clash2.exit
>  create mode 100644 tests/qapi-schema/union-clash2.json
>  create mode 100644 tests/qapi-schema/union-clash2.out
>
> diff --git a/tests/Makefile b/tests/Makefile
> index 6fd5885..97434f6 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
> @@ -274,6 +275,8 @@ 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-branch-clash2.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
> @@ -317,6 +320,7 @@ qapi-schema += returns-unknown.json
>  qapi-schema += returns-whitelist.json
>  qapi-schema += struct-base-clash-deep.json
>  qapi-schema += struct-base-clash.json
> +qapi-schema += struct-base-clash2.json
>  qapi-schema += struct-data-invalid.json
>  qapi-schema += struct-member-invalid.json
>  qapi-schema += trailing-comma-list.json
> @@ -328,6 +332,8 @@ 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.json
> +qapi-schema += union-clash2.json
>  qapi-schema += union-invalid-base.json
>  qapi-schema += union-max.json
>  qapi-schema += union-optional-branch.json
> diff --git a/tests/qapi-schema/args-name-clash.err 
> b/tests/qapi-schema/args-name-clash.err
> new file mode 100644
> index 0000000..e69de29
> 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.json 
> b/tests/qapi-schema/flat-union-branch-clash.json
> index 8fb054f..3d7e6c6 100644
> --- a/tests/qapi-schema/flat-union-branch-clash.json
> +++ b/tests/qapi-schema/flat-union-branch-clash.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',

This clashing business is awfully confusing as soon as unions come into
play.  When I'm confused, I need to think in writing.

The basic case is clash between local, non-variant members.  Needs test
coverage.  args-name-clash.json provides it, because internally the
arguments are just another object type.

With a base, the members inherited from base get added to the mix.  We
need to test a clash betwen local, non-variant member and a member
inherited from base.

With unions, things get complicated, because we have multiple kinds of
clashes.  Best explained with an example.  Let's use UserDefFlatUnion
from qapi-schema-test.json.

    { 'union': 'UserDefFlatUnion',
      'base': 'UserDefUnionBase',   # intentional forward reference
      'discriminator': 'enum1',
      'data': { 'value1' : 'UserDefA',
                'value2' : 'UserDefB',
                'value3' : 'UserDefB' } }

    { 'struct': 'UserDefUnionBase',
      'base': 'UserDefZero',
      'data': { 'string': 'str', 'enum1': 'EnumOne' } }

Generated C looks like this:

    struct UserDefFlatUnion {
        /* Members inherited from UserDefUnionBase: */
        int64_t integer;
        char *string;
        EnumOne enum1;
        /* Own members: */
        // if the schema language supported adding non-variant local
        // members, they'd go right here
        union { /* union tag is @enum1 */
            void *data;
            UserDefA *value1;
            UserDefB *value2;
            UserDefB *value3;
        };
    };

Thus, what can clash in C is the tag values value1, value2, value3 with
the non-variant members integer, string, enum1.

On the wire, the union members are unboxed, i.e. we get just

    "boolean": false

instead of

    "value1": { "boolean": false }

Thus what can clash on the wire is the variant members with the
non-variant members: boolean with integer, string, enum1 when enum1 is
value1, and so forth.

This is the clash flat-union-branch-clash.json tests.  Its error message
is "Member name 'name' of branch 'value1' clashes with base 'Base'".
Suboptimal, it should say "with member 'name' of base 'Base'".

> diff --git a/tests/qapi-schema/flat-union-branch-clash2.err 
> b/tests/qapi-schema/flat-union-branch-clash2.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/flat-union-branch-clash2.exit 
> b/tests/qapi-schema/flat-union-branch-clash2.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-branch-clash2.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/flat-union-branch-clash2.json 
> b/tests/qapi-schema/flat-union-branch-clash2.json
> new file mode 100644
> index 0000000..b3eefb3
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-branch-clash2.json
> @@ -0,0 +1,14 @@
> +# FIXME: we should check for no duplicate C names between branches and base
> +{ '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' } }

This tests the other kind of clash: tag value 'c-d' clashes with
non-variant member name 'c_d'.

Please add a comment explaining what clash should be reported here.

> diff --git a/tests/qapi-schema/flat-union-branch-clash2.out 
> b/tests/qapi-schema/flat-union-branch-clash2.out
> new file mode 100644
> index 0000000..8e0da73
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-branch-clash2.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-cycle.err 
> b/tests/qapi-schema/flat-union-cycle.err
> new file mode 100644
> index 0000000..152c6f0
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-cycle.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/flat-union-cycle.json:5: Member name 'switch' of branch 
> 'loop' clashes with base 'Base'
> 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..8f6cd93
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-cycle.json
> @@ -0,0 +1,6 @@
> +# we reject a loop in flat unions, due to member collision
> +{ 'enum': 'Enum', 'data': [ 'okay', 'loop' ] }
> +{ 'struct': 'Base', 'data': { 'switch': 'Enum' } }
> +{ 'struct': 'Okay', 'data': { 'int': 'int' } }
> +{ 'union': 'Union', 'base': 'Base', 'discriminator': 'switch',
> +  'data': { 'okay': 'Okay', 'loop': 'Base' } }

This isn't a loop, it's a fork: we get the members of Base via its use
as base, and again via its use as type of a variant case.

What does it add over flat-union-branch-clash.json?

> 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..c904db4 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -32,11 +32,12 @@
>              'dict1': 'UserDefTwoDict' } }
>
>  # for testing unions
> +# name collisions between branches should not clash
>  { '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

This tests that different variants may have clashing names.  Okay.

I'm afraid the comment is a bit too terse.  Not sure I'd make the
connection from it to member a_b and to UserDefB's a-b a fortnight from
now.

> 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-clash2.err 
> b/tests/qapi-schema/struct-base-clash2.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/struct-base-clash2.exit 
> b/tests/qapi-schema/struct-base-clash2.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/struct-base-clash2.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/struct-base-clash2.json 
> b/tests/qapi-schema/struct-base-clash2.json
> new file mode 100644
> index 0000000..56166e0
> --- /dev/null
> +++ b/tests/qapi-schema/struct-base-clash2.json
> @@ -0,0 +1,5 @@
> +# FIXME - a base class collides with a member named base
> +{ 'struct': 'Base', 'data': {} }
> +{ 'struct': 'Sub',
> +  'base': 'Base',
> +  'data': { 'base': 'str' } }

What's this about?  Hmm, I think it's about the way we do a struct
type's base.  For a union type, we add the base's members, as shown
above.  For a struct type, we add the base *boxed*, like this:

    struct Sub {
        // The base type
        Base *base;
        // Own members
        char *base;
    };

Therefore, a struct type with a base can't have a member named base.
But that's simply daft.  As soon as we change it to match union types,
this test case goes away.  If we change it soon, do we still need this
test?  Will it be done later in this series?

> diff --git a/tests/qapi-schema/struct-base-clash2.out 
> b/tests/qapi-schema/struct-base-clash2.out
> new file mode 100644
> index 0000000..e69a416
> --- /dev/null
> +++ b/tests/qapi-schema/struct-base-clash2.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.err 
> b/tests/qapi-schema/union-clash.err
> new file mode 100644
> index 0000000..64637ed
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/union-clash.json:2: Union 'TestUnion' member 'a_b' clashes 
> with 'a-b'
> diff --git a/tests/qapi-schema/union-clash.exit 
> b/tests/qapi-schema/union-clash.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/union-clash.json 
> b/tests/qapi-schema/union-clash.json
> new file mode 100644
> index 0000000..0393ed8
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash.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' } }

Is it possible for branch names to clash in C when the enumeration (be
it implicit or explicit) passes clash checks?

> diff --git a/tests/qapi-schema/union-clash.out 
> b/tests/qapi-schema/union-clash.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/union-clash2.err 
> b/tests/qapi-schema/union-clash2.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/union-clash2.exit 
> b/tests/qapi-schema/union-clash2.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash2.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/union-clash2.json 
> b/tests/qapi-schema/union-clash2.json
> new file mode 100644
> index 0000000..b2d45fb
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash2.json
> @@ -0,0 +1,3 @@
> +# FIXME - a union branch named 'data' collides with generated C code
> +{ 'union': 'TestUnion',
> +  'data': { 'data': 'int' } }

This tests another stupid clash: we put a member named data in our
generated unions.  As soon as we stop doing that, this test will go
away.  If we stop soon, do we still need this test?  Will we stop later
in this series?

> diff --git a/tests/qapi-schema/union-clash2.out 
> b/tests/qapi-schema/union-clash2.out
> new file mode 100644
> index 0000000..6277239
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash2.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']