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