| Message ID | 1443565276-4535-6-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 09/29/2015 04:21 PM, Eric Blake wrote: > Expose some weaknesses in the generator: we don't always forbid > the generation of structs that contain multiple members that map > to the same C or QMP name. This has already been marked FIXME in > qapi.py in commit d90675f, but having more tests will make sure > future patches produce desired behavior; and updating existing > patches to better document things doesn't hurt, either. Some of > these collisions are already caught in the old-style parser > checks, but ultimately we want all collisions to be caught in the > new-style QAPISchema*.check() methods. > > diff --git a/tests/qapi-schema/union-clash-branches.err b/tests/qapi-schema/union-clash-branches.err > new file mode 100644 > index 0000000..005c48d > --- /dev/null > +++ b/tests/qapi-schema/union-clash-branches.err > @@ -0,0 +1 @@ > +tests/qapi-schema/union-clash-branches.json:4: Union 'TestUnion' member 'a_b' clashes with 'a-b' > diff --git a/tests/qapi-schema/union-clash-branches.exit b/tests/qapi-schema/union-clash-branches.exit > new file mode 100644 > index 0000000..d00491f > --- /dev/null > +++ b/tests/qapi-schema/union-clash-branches.exit > @@ -0,0 +1 @@ > +1 > diff --git a/tests/qapi-schema/union-clash-branches.json b/tests/qapi-schema/union-clash-branches.json > new file mode 100644 > index 0000000..31d135f > --- /dev/null > +++ b/tests/qapi-schema/union-clash-branches.json > @@ -0,0 +1,5 @@ > +# Union branch name collision > +# Reject a union that would result in a collision in generated C names (this > +# would try to generate two enum values 'TEST_UNION_KIND_A_B'). > +{ 'union': 'TestUnion', > + 'data': { 'a-b': 'int', 'a_b': 'str' } } Hmm. This test is very similar to the existing union-bad-branch (I guess it's poor name is why I didn't notice it before). But that test only covered 'one' vs. 'ONE' (no clash in the C struct, just in the generated MyUnionKind enum type); while this test also clashes in the C struct. Don't know if it is worth a v8 to clean up the duplication, or if we just save it for a followup patch (namely, where I try to move errors into the QAPISchema.check() methods); I found the issue while playing with v5 15/46.
Eric Blake <eblake@redhat.com> writes: > Expose some weaknesses in the generator: we don't always forbid > the generation of structs that contain multiple members that map > to the same C or QMP name. This has already been marked FIXME in > qapi.py in commit d90675f, but having more tests will make sure > future patches produce desired behavior; and updating existing > patches to better document things doesn't hurt, either. Some of > these collisions are already caught in the old-style parser > checks, but ultimately we want all collisions to be caught in the > new-style QAPISchema*.check() methods. > > This patch focuses on C struct members, and does not consider > collisions between commands and events (affecting C function > names), or even collisions between generated C type names with > user type names (for things like automatic FOOList struct > representing array types or FOOKind for an implicit enum). > > There are two types of struct collisions we want to catch: > 1) Collision between two keys in a JSON object. qapi.py prevents > that within a single struct (see duplicate-key), but it is "(see test duplicate-key)" or maybe "(see test duplicate-key.json)". > possible to have collisions between a type's members and its > base type's members (struct-base-clash, struct-base-clash-deep, > flat-union-cycle), and its flat union variant members (existing tests struct-base-clash, struct-base-clash-deep, new test flat-union-cycle) > (flat-union-clash-member). (new test flat-union-clash-member) > 2) Collision between two members of the C struct that is generated > for a given QAPI type: > a) Multiple QAPI names map to the same C name (args-name-clash) (new test args-name-clash) > b) A QAPI name maps to a C name that is used for another purpose > (flat-union-clash-branch, struct-base-clash-base, > union-clash-data). We already fixed some such cases in commit (new tests ...) > 0f61af3e and 1e6c1616, but more remain. > c) Two C names generated for other purposes clash > (alternate-clash, union-clash-branches, union-clash-type, > flat-union-clash-type) (updated test alternate-clash, new tests ...) > > Ultimately, if we need to have a flat union where an enum value Suggest "where a tag value", to make it clear it's not just any enum. > clashes with a base member name, we could change the generator to > name the union (using 'foo.u.value' rather than 'foo.value') or > otherwise munge the C name corresponding to enum tag values. But Suggest to drop enum here. > unless such a need arises, it will probably be easier to just > forbid these collisions. > > Some of these negative tests will be deleted later, and positive > tests added to qapi-schema-test.json in their place, when the > generator code is reworked to avoid particular code generation > collisions in class 2). > > [Note that viewing this patch with git rename detection enabled > may see some confusion due to renaming some tests while adding > others, but where the content is similar enough that git picks > the wrong pre- and post-patch files to associate] > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v7: rework commit message again, even more comments in tests, yet > another rename (union-clash-members is now union-clash-branches) > v6: rework commit message, alter test names and comments to be > more descriptive, repurpose flat-union-cycle to better match its > name (and not duplicate what the renamed flat-union-clash-branch > tests), add new tests (union-clash-type, flat-union-clash-type) > that detect an assertion failures [...] > diff --git a/tests/qapi-schema/alternate-clash.err b/tests/qapi-schema/alternate-clash.err > index 51bea3e..a475ab6 100644 > --- a/tests/qapi-schema/alternate-clash.err > +++ b/tests/qapi-schema/alternate-clash.err > @@ -1 +1 @@ > -tests/qapi-schema/alternate-clash.json:2: Alternate 'Alt1' member 'ONE' clashes with 'one' > +tests/qapi-schema/alternate-clash.json:7: Alternate 'Alt1' member 'a_b' clashes with 'a-b' > diff --git a/tests/qapi-schema/alternate-clash.json b/tests/qapi-schema/alternate-clash.json > index 3947935..6d73bc5 100644 > --- a/tests/qapi-schema/alternate-clash.json > +++ b/tests/qapi-schema/alternate-clash.json > @@ -1,3 +1,8 @@ > -# we detect C enum collisions in an alternate > +# Alternate branch name collision > +# Reject an alternate that would result in a collision in generated C > +# names (this would try to generate two enum values 'ALT1_KIND_A_B'). > +# TODO: In the future, if alternates are simplified to not generate > +# the implicit Alt1Kind enum, we would still have a collision with the > +# resulting C union trying to have two members named 'a_b'. > { 'alternate': 'Alt1', > - 'data': { 'one': 'str', 'ONE': 'int' } } > + 'data': { 'a-b': 'str', 'a_b': 'int' } } Ah, you're making the test slightly more robust. Works for me. > diff --git a/tests/qapi-schema/flat-union-branch-clash.out b/tests/qapi-schema/args-name-clash.err > similarity index 100% > rename from tests/qapi-schema/flat-union-branch-clash.out > rename to tests/qapi-schema/args-name-clash.err > diff --git a/tests/qapi-schema/args-name-clash.exit b/tests/qapi-schema/args-name-clash.exit > new file mode 100644 > index 0000000..573541a > --- /dev/null > +++ b/tests/qapi-schema/args-name-clash.exit > @@ -0,0 +1 @@ > +0 > diff --git a/tests/qapi-schema/args-name-clash.json b/tests/qapi-schema/args-name-clash.json > new file mode 100644 > index 0000000..9e8f889 > --- /dev/null > +++ b/tests/qapi-schema/args-name-clash.json > @@ -0,0 +1,5 @@ > +# C member name collision > +# FIXME - This parses, but fails to compile, because the C struct is given > +# two 'a_b' members. Either reject this at parse time, or munge the C names > +# to avoid the collision. > +{ 'command': 'oops', 'data': { 'a-b': 'str', 'a_b': 'str' } } > diff --git a/tests/qapi-schema/args-name-clash.out b/tests/qapi-schema/args-name-clash.out > new file mode 100644 > index 0000000..9b2f6e4 > --- /dev/null > +++ b/tests/qapi-schema/args-name-clash.out > @@ -0,0 +1,6 @@ > +object :empty > +object :obj-oops-arg > + member a-b: str optional=False > + member a_b: str optional=False > +command oops :obj-oops-arg -> None > + gen=True success_response=True > diff --git a/tests/qapi-schema/duplicate-key.err b/tests/qapi-schema/duplicate-key.err > index 768b276..6d02f83 100644 > --- a/tests/qapi-schema/duplicate-key.err > +++ b/tests/qapi-schema/duplicate-key.err > @@ -1 +1 @@ > -tests/qapi-schema/duplicate-key.json:2:10: Duplicate key "key" > +tests/qapi-schema/duplicate-key.json:3:10: Duplicate key "key" > diff --git a/tests/qapi-schema/duplicate-key.json b/tests/qapi-schema/duplicate-key.json > index 1b55d88..14ac0e8 100644 > --- a/tests/qapi-schema/duplicate-key.json > +++ b/tests/qapi-schema/duplicate-key.json > @@ -1,2 +1,3 @@ > +# QAPI cannot include the same key more than once in any {} > { 'key': 'value', > 'key': 'value' } > diff --git a/tests/qapi-schema/flat-union-base-union.err b/tests/qapi-schema/flat-union-base-union.err > index ede9859..28725ed 100644 > --- a/tests/qapi-schema/flat-union-base-union.err > +++ b/tests/qapi-schema/flat-union-base-union.err > @@ -1 +1 @@ > -tests/qapi-schema/flat-union-base-union.json:11: Base 'UnionBase' is not a valid struct > +tests/qapi-schema/flat-union-base-union.json:14: Base 'UnionBase' is not a valid struct > diff --git a/tests/qapi-schema/flat-union-base-union.json b/tests/qapi-schema/flat-union-base-union.json > index 6a8ea68..98b4eba 100644 > --- a/tests/qapi-schema/flat-union-base-union.json > +++ b/tests/qapi-schema/flat-union-base-union.json > @@ -1,4 +1,7 @@ > -# we require the base to be a struct > +# For now, we require the base to be a struct without variants > +# TODO: It would be possible to allow a union as a base, as long as all > +# permutations of QMP names exposed by base do not clash with any QMP > +# member names added by local variants. > { 'enum': 'TestEnum', > 'data': [ 'value1', 'value2' ] } > { 'struct': 'TestTypeA', > diff --git a/tests/qapi-schema/flat-union-branch-clash.err b/tests/qapi-schema/flat-union-branch-clash.err > deleted file mode 100644 > index f112766..0000000 > --- a/tests/qapi-schema/flat-union-branch-clash.err > +++ /dev/null > @@ -1 +0,0 @@ > -tests/qapi-schema/flat-union-branch-clash.json:10: Member name 'name' of branch 'value1' clashes with base 'Base' > diff --git a/tests/qapi-schema/flat-union-clash-branch.err b/tests/qapi-schema/flat-union-clash-branch.err > new file mode 100644 > index 0000000..e69de29 > diff --git a/tests/qapi-schema/flat-union-clash-branch.exit b/tests/qapi-schema/flat-union-clash-branch.exit > new file mode 100644 > index 0000000..573541a > --- /dev/null > +++ b/tests/qapi-schema/flat-union-clash-branch.exit > @@ -0,0 +1 @@ > +0 > diff --git a/tests/qapi-schema/flat-union-clash-branch.json b/tests/qapi-schema/flat-union-clash-branch.json > new file mode 100644 > index 0000000..e593336 > --- /dev/null > +++ b/tests/qapi-schema/flat-union-clash-branch.json > @@ -0,0 +1,18 @@ > +# Flat union branch name collision > +# FIXME: this parses, but then fails to compile due to a duplicate 'c_d' > +# (one from the base member, the other from the branch name). We should > +# either reject the collision at parse time, or munge the generated branch > +# name to allow this to compile. > +{ 'enum': 'TestEnum', > + 'data': [ 'base', 'c-d' ] } > +{ 'struct': 'Base', > + 'data': { 'enum1': 'TestEnum', '*c_d': 'str' } } > +{ 'struct': 'Branch1', > + 'data': { 'string': 'str' } } > +{ 'struct': 'Branch2', > + 'data': { 'value': 'int' } } > +{ 'union': 'TestUnion', > + 'base': 'Base', > + 'discriminator': 'enum1', > + 'data': { 'base': 'Branch1', > + 'c-d': 'Branch2' } } > diff --git a/tests/qapi-schema/flat-union-clash-branch.out b/tests/qapi-schema/flat-union-clash-branch.out > new file mode 100644 > index 0000000..8e0da73 > --- /dev/null > +++ b/tests/qapi-schema/flat-union-clash-branch.out > @@ -0,0 +1,14 @@ > +object :empty > +object Base > + member enum1: TestEnum optional=False > + member c_d: str optional=True > +object Branch1 > + member string: str optional=False > +object Branch2 > + member value: int optional=False > +enum TestEnum ['base', 'c-d'] > +object TestUnion > + base Base > + tag enum1 > + case base: Branch1 > + case c-d: Branch2 > diff --git a/tests/qapi-schema/flat-union-clash-member.err b/tests/qapi-schema/flat-union-clash-member.err > new file mode 100644 > index 0000000..152d402 > --- /dev/null > +++ b/tests/qapi-schema/flat-union-clash-member.err > @@ -0,0 +1 @@ > +tests/qapi-schema/flat-union-clash-member.json:10: Member name 'name' of branch 'value1' clashes with base 'Base' > diff --git a/tests/qapi-schema/flat-union-branch-clash.exit b/tests/qapi-schema/flat-union-clash-member.exit > similarity index 100% > rename from tests/qapi-schema/flat-union-branch-clash.exit > rename to tests/qapi-schema/flat-union-clash-member.exit > diff --git a/tests/qapi-schema/flat-union-branch-clash.json b/tests/qapi-schema/flat-union-clash-member.json > similarity index 85% > rename from tests/qapi-schema/flat-union-branch-clash.json > rename to tests/qapi-schema/flat-union-clash-member.json > index 8fb054f..3d7e6c6 100644 > --- a/tests/qapi-schema/flat-union-branch-clash.json > +++ b/tests/qapi-schema/flat-union-clash-member.json > @@ -1,4 +1,4 @@ > -# we check for no duplicate keys between branches and base > +# we check for no duplicate keys between branch members and base Spelling out the clashing members wouldn't hurt. > { 'enum': 'TestEnum', > 'data': [ 'value1', 'value2' ] } > { 'struct': 'Base', > diff --git a/tests/qapi-schema/flat-union-clash-member.out b/tests/qapi-schema/flat-union-clash-member.out > new file mode 100644 > index 0000000..e69de29 > diff --git a/tests/qapi-schema/flat-union-clash-type.err b/tests/qapi-schema/flat-union-clash-type.err > new file mode 100644 > index 0000000..6e64d1d > --- /dev/null > +++ b/tests/qapi-schema/flat-union-clash-type.err > @@ -0,0 +1,16 @@ > +Traceback (most recent call last): > + File "tests/qapi-schema/test-qapi.py", line 55, in <module> > + schema = QAPISchema(sys.argv[1]) > + File "scripts/qapi.py", line 1116, in __init__ > + self.check() > + File "scripts/qapi.py", line 1299, in check > + ent.check(self) > + File "scripts/qapi.py", line 962, in check > + self.variants.check(schema, members, seen) > + File "scripts/qapi.py", line 1024, in check > + v.check(schema, self.tag_member.type, vseen) > + File "scripts/qapi.py", line 1032, in check > + QAPISchemaObjectTypeMember.check(self, schema, [], seen) > + File "scripts/qapi.py", line 994, in check > + assert self.name not in seen > +AssertionError > diff --git a/tests/qapi-schema/flat-union-clash-type.exit b/tests/qapi-schema/flat-union-clash-type.exit > new file mode 100644 > index 0000000..d00491f > --- /dev/null > +++ b/tests/qapi-schema/flat-union-clash-type.exit > @@ -0,0 +1 @@ > +1 > diff --git a/tests/qapi-schema/flat-union-clash-type.json b/tests/qapi-schema/flat-union-clash-type.json > new file mode 100644 > index 0000000..eac51a4 > --- /dev/null > +++ b/tests/qapi-schema/flat-union-clash-type.json > @@ -0,0 +1,15 @@ > +# Flat union branch 'type' > +# FIXME: this triggers an assertion failure. But even with that fixed, we > +# would have a clash in generated C, between the outer tag 'type' and "outer tag"? I guess you mean the member type we inherit from Base. > +# the branch name 'type' within the union. We should either reject this, > +# or munge the generated C to let it compile. > +{ 'enum': 'TestEnum', > + 'data': [ 'type' ] } > +{ 'struct': 'Base', > + 'data': { 'type': 'TestEnum' } } > +{ 'struct': 'Branch1', > + 'data': { 'string': 'str' } } > +{ 'union': 'TestUnion', > + 'base': 'Base', > + 'discriminator': 'type', > + 'data': { 'type': 'Branch1' } } > diff --git a/tests/qapi-schema/flat-union-clash-type.out b/tests/qapi-schema/flat-union-clash-type.out > new file mode 100644 > index 0000000..e69de29 > diff --git a/tests/qapi-schema/flat-union-cycle.err b/tests/qapi-schema/flat-union-cycle.err > new file mode 100644 > index 0000000..9b7978a > --- /dev/null > +++ b/tests/qapi-schema/flat-union-cycle.err > @@ -0,0 +1 @@ > +tests/qapi-schema/flat-union-cycle.json:7: Base 'Union' is not a valid struct > diff --git a/tests/qapi-schema/flat-union-cycle.exit b/tests/qapi-schema/flat-union-cycle.exit > new file mode 100644 > index 0000000..d00491f > --- /dev/null > +++ b/tests/qapi-schema/flat-union-cycle.exit > @@ -0,0 +1 @@ > +1 > diff --git a/tests/qapi-schema/flat-union-cycle.json b/tests/qapi-schema/flat-union-cycle.json > new file mode 100644 > index 0000000..c66c4c9 > --- /dev/null > +++ b/tests/qapi-schema/flat-union-cycle.json > @@ -0,0 +1,8 @@ > +# Ensure that we have a sane error message for attempts at self-inheritance > +# This test currently fails because we don't permit a union base, but > +# even if we relax things to allow that in the future (see > +# flat-union-base-union), self-inheritance should still fail. Do we have a test for the simpler case of a struct inheriting from itself? I believe us merging struct and union types into a single object type is more likely than us implementing union bases. If I'm correct, we won't need this test. > +{ 'enum': 'Enum', 'data': [ 'okay' ] } > +{ 'struct': 'Okay', 'data': { 'int': 'int' } } > +{ 'union': 'Union', 'base': 'Union', 'discriminator': 'switch', > + 'data': { 'okay': 'Okay' } } > diff --git a/tests/qapi-schema/flat-union-cycle.out b/tests/qapi-schema/flat-union-cycle.out > new file mode 100644 > index 0000000..e69de29 > diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json > index 6897a6e..c511338 100644 > --- a/tests/qapi-schema/qapi-schema-test.json > +++ b/tests/qapi-schema/qapi-schema-test.json > @@ -32,11 +32,14 @@ > 'dict1': 'UserDefTwoDict' } } > > # for testing unions > +# Among other things, test that a name collision between branches does > +# not cause any problems (since only one branch can be in use at a time), > +# by intentionally using two branches that both have a C member 'a_b' > { 'struct': 'UserDefA', > - 'data': { 'boolean': 'bool' } } > + 'data': { 'boolean': 'bool', '*a_b': 'int' } } > > { 'struct': 'UserDefB', > - 'data': { 'intb': 'int' } } > + 'data': { 'intb': 'int', '*a-b': 'bool' } } > > { 'union': 'UserDefFlatUnion', > 'base': 'UserDefUnionBase', # intentional forward reference > diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out > index 1f6e858..28a0b3c 100644 > --- a/tests/qapi-schema/qapi-schema-test.out > +++ b/tests/qapi-schema/qapi-schema-test.out > @@ -71,6 +71,7 @@ enum QEnumTwo ['value1', 'value2'] > prefix QENUM_TWO > object UserDefA > member boolean: bool optional=False > + member a_b: int optional=True > alternate UserDefAlternate > case uda: UserDefA > case s: str > @@ -78,6 +79,7 @@ alternate UserDefAlternate > enum UserDefAlternateKind ['uda', 's', 'i'] > object UserDefB > member intb: int optional=False > + member a-b: bool optional=True > object UserDefC > member string1: str optional=False > member string2: str optional=False > diff --git a/tests/qapi-schema/struct-base-clash-base.err b/tests/qapi-schema/struct-base-clash-base.err > new file mode 100644 > index 0000000..e69de29 > diff --git a/tests/qapi-schema/struct-base-clash-base.exit b/tests/qapi-schema/struct-base-clash-base.exit > new file mode 100644 > index 0000000..573541a > --- /dev/null > +++ b/tests/qapi-schema/struct-base-clash-base.exit > @@ -0,0 +1 @@ > +0 > diff --git a/tests/qapi-schema/struct-base-clash-base.json b/tests/qapi-schema/struct-base-clash-base.json > new file mode 100644 > index 0000000..0c84025 > --- /dev/null > +++ b/tests/qapi-schema/struct-base-clash-base.json > @@ -0,0 +1,9 @@ > +# Struct member 'base' > +# FIXME: this parses, but then fails to compile due to a duplicate 'base' > +# (one explicit in QMP, the other used to box the base class members). > +# We should either reject the collision at parse time, or change the > +# generated struct to allow this to compile. > +{ 'struct': 'Base', 'data': {} } > +{ 'struct': 'Sub', > + 'base': 'Base', > + 'data': { 'base': 'str' } } > diff --git a/tests/qapi-schema/struct-base-clash-base.out b/tests/qapi-schema/struct-base-clash-base.out > new file mode 100644 > index 0000000..e69a416 > --- /dev/null > +++ b/tests/qapi-schema/struct-base-clash-base.out > @@ -0,0 +1,5 @@ > +object :empty > +object Base > +object Sub > + base Base > + member base: str optional=False > diff --git a/tests/qapi-schema/struct-base-clash-deep.err b/tests/qapi-schema/struct-base-clash-deep.err > index e3e9f8d..f7a25a3 100644 > --- a/tests/qapi-schema/struct-base-clash-deep.err > +++ b/tests/qapi-schema/struct-base-clash-deep.err > @@ -1 +1 @@ > -tests/qapi-schema/struct-base-clash-deep.json:7: Member name 'name' clashes with base 'Base' > +tests/qapi-schema/struct-base-clash-deep.json:10: Member name 'name' clashes with base 'Base' > diff --git a/tests/qapi-schema/struct-base-clash-deep.json b/tests/qapi-schema/struct-base-clash-deep.json > index 552fe94..fa873ab 100644 > --- a/tests/qapi-schema/struct-base-clash-deep.json > +++ b/tests/qapi-schema/struct-base-clash-deep.json > @@ -1,4 +1,7 @@ > -# we check for no duplicate keys with indirect base > +# Reject attempts to duplicate QMP members > +# Here, 'name' would have to appear twice on the wire, locally and > +# indirectly for the grandparent base; the collision doesn't care that > +# one instance is optional. > { 'struct': 'Base', > 'data': { 'name': 'str' } } > { 'struct': 'Mid', > diff --git a/tests/qapi-schema/struct-base-clash.err b/tests/qapi-schema/struct-base-clash.err > index 3ac37fb..3a9f66b 100644 > --- a/tests/qapi-schema/struct-base-clash.err > +++ b/tests/qapi-schema/struct-base-clash.err > @@ -1 +1 @@ > -tests/qapi-schema/struct-base-clash.json:4: Member name 'name' clashes with base 'Base' > +tests/qapi-schema/struct-base-clash.json:5: Member name 'name' clashes with base 'Base' > diff --git a/tests/qapi-schema/struct-base-clash.json b/tests/qapi-schema/struct-base-clash.json > index f2afc9b..11aec80 100644 > --- a/tests/qapi-schema/struct-base-clash.json > +++ b/tests/qapi-schema/struct-base-clash.json > @@ -1,4 +1,5 @@ > -# we check for no duplicate keys with base > +# Reject attempts to duplicate QMP members > +# Here, 'name' would have to appear twice on the wire, locally and for base. > { 'struct': 'Base', > 'data': { 'name': 'str' } } > { 'struct': 'Sub', > diff --git a/tests/qapi-schema/union-clash-branches.err b/tests/qapi-schema/union-clash-branches.err > new file mode 100644 > index 0000000..005c48d > --- /dev/null > +++ b/tests/qapi-schema/union-clash-branches.err > @@ -0,0 +1 @@ > +tests/qapi-schema/union-clash-branches.json:4: Union 'TestUnion' member 'a_b' clashes with 'a-b' > diff --git a/tests/qapi-schema/union-clash-branches.exit b/tests/qapi-schema/union-clash-branches.exit > new file mode 100644 > index 0000000..d00491f > --- /dev/null > +++ b/tests/qapi-schema/union-clash-branches.exit > @@ -0,0 +1 @@ > +1 > diff --git a/tests/qapi-schema/union-clash-branches.json b/tests/qapi-schema/union-clash-branches.json > new file mode 100644 > index 0000000..31d135f > --- /dev/null > +++ b/tests/qapi-schema/union-clash-branches.json > @@ -0,0 +1,5 @@ > +# Union branch name collision > +# Reject a union that would result in a collision in generated C names (this > +# would try to generate two enum values 'TEST_UNION_KIND_A_B'). > +{ 'union': 'TestUnion', > + 'data': { 'a-b': 'int', 'a_b': 'str' } } > diff --git a/tests/qapi-schema/union-clash-branches.out b/tests/qapi-schema/union-clash-branches.out > new file mode 100644 > index 0000000..e69de29 > diff --git a/tests/qapi-schema/union-clash-data.err b/tests/qapi-schema/union-clash-data.err > new file mode 100644 > index 0000000..e69de29 > diff --git a/tests/qapi-schema/union-clash-data.exit b/tests/qapi-schema/union-clash-data.exit > new file mode 100644 > index 0000000..573541a > --- /dev/null > +++ b/tests/qapi-schema/union-clash-data.exit > @@ -0,0 +1 @@ > +0 > diff --git a/tests/qapi-schema/union-clash-data.json b/tests/qapi-schema/union-clash-data.json > new file mode 100644 > index 0000000..7308e69 > --- /dev/null > +++ b/tests/qapi-schema/union-clash-data.json > @@ -0,0 +1,7 @@ > +# Union branch 'data' > +# FIXME: this parses, but then fails to compile due to a duplicate 'data' > +# (one from the branch name, another as a filler to avoid an empty union). > +# we should either detect the collision at parse time, or change the > +# generated struct to allow this to compile. > +{ 'union': 'TestUnion', > + 'data': { 'data': 'int' } } > diff --git a/tests/qapi-schema/union-clash-data.out b/tests/qapi-schema/union-clash-data.out > new file mode 100644 > index 0000000..6277239 > --- /dev/null > +++ b/tests/qapi-schema/union-clash-data.out > @@ -0,0 +1,6 @@ > +object :empty > +object :obj-int-wrapper > + member data: int optional=False > +object TestUnion > + case data: :obj-int-wrapper > +enum TestUnionKind ['data'] > diff --git a/tests/qapi-schema/union-clash-type.err b/tests/qapi-schema/union-clash-type.err > new file mode 100644 > index 0000000..6e64d1d > --- /dev/null > +++ b/tests/qapi-schema/union-clash-type.err > @@ -0,0 +1,16 @@ > +Traceback (most recent call last): > + File "tests/qapi-schema/test-qapi.py", line 55, in <module> > + schema = QAPISchema(sys.argv[1]) > + File "scripts/qapi.py", line 1116, in __init__ > + self.check() > + File "scripts/qapi.py", line 1299, in check > + ent.check(self) > + File "scripts/qapi.py", line 962, in check > + self.variants.check(schema, members, seen) > + File "scripts/qapi.py", line 1024, in check > + v.check(schema, self.tag_member.type, vseen) > + File "scripts/qapi.py", line 1032, in check > + QAPISchemaObjectTypeMember.check(self, schema, [], seen) > + File "scripts/qapi.py", line 994, in check > + assert self.name not in seen > +AssertionError > diff --git a/tests/qapi-schema/union-clash-type.exit b/tests/qapi-schema/union-clash-type.exit > new file mode 100644 > index 0000000..d00491f > --- /dev/null > +++ b/tests/qapi-schema/union-clash-type.exit > @@ -0,0 +1 @@ > +1 > diff --git a/tests/qapi-schema/union-clash-type.json b/tests/qapi-schema/union-clash-type.json > new file mode 100644 > index 0000000..38330b5 > --- /dev/null > +++ b/tests/qapi-schema/union-clash-type.json > @@ -0,0 +1,9 @@ > +# Union branch 'type' > +# FIXME: this triggers an assertion failure. But even with that fixed, we > +# would have a clash in generated C, between the outer union tag 'kind' and Suggest "the simple union's implicit tag member 'kind' and" > +# the branch name 'kind' within the union. We should either reject this, > +# or munge the generated C to let it compile. > +# TODO: Even when the generated C is switched to use 'type' rather than > +# 'kind', to match the QMP spelling, the collision should still be detected. > +{ 'union': 'TestUnion', > + 'data': { 'kind': 'int', 'type': 'str' } } > diff --git a/tests/qapi-schema/union-clash-type.out b/tests/qapi-schema/union-clash-type.out > new file mode 100644 > index 0000000..e69de29 Found nothing that couldn't be touched up on commit.
Eric Blake <eblake@redhat.com> writes: > On 09/29/2015 04:21 PM, Eric Blake wrote: >> Expose some weaknesses in the generator: we don't always forbid >> the generation of structs that contain multiple members that map >> to the same C or QMP name. This has already been marked FIXME in >> qapi.py in commit d90675f, but having more tests will make sure >> future patches produce desired behavior; and updating existing >> patches to better document things doesn't hurt, either. Some of >> these collisions are already caught in the old-style parser >> checks, but ultimately we want all collisions to be caught in the >> new-style QAPISchema*.check() methods. >> > >> diff --git a/tests/qapi-schema/union-clash-branches.err >> b/tests/qapi-schema/union-clash-branches.err >> new file mode 100644 >> index 0000000..005c48d >> --- /dev/null >> +++ b/tests/qapi-schema/union-clash-branches.err >> @@ -0,0 +1 @@ >> +tests/qapi-schema/union-clash-branches.json:4: Union 'TestUnion' >> member 'a_b' clashes with 'a-b' >> diff --git a/tests/qapi-schema/union-clash-branches.exit >> b/tests/qapi-schema/union-clash-branches.exit >> new file mode 100644 >> index 0000000..d00491f >> --- /dev/null >> +++ b/tests/qapi-schema/union-clash-branches.exit >> @@ -0,0 +1 @@ >> +1 >> diff --git a/tests/qapi-schema/union-clash-branches.json >> b/tests/qapi-schema/union-clash-branches.json >> new file mode 100644 >> index 0000000..31d135f >> --- /dev/null >> +++ b/tests/qapi-schema/union-clash-branches.json >> @@ -0,0 +1,5 @@ >> +# Union branch name collision >> +# Reject a union that would result in a collision in generated C names (this >> +# would try to generate two enum values 'TEST_UNION_KIND_A_B'). >> +{ 'union': 'TestUnion', >> + 'data': { 'a-b': 'int', 'a_b': 'str' } } > > Hmm. This test is very similar to the existing union-bad-branch (I guess > it's poor name is why I didn't notice it before). But that test only > covered 'one' vs. 'ONE' (no clash in the C struct, just in the generated > MyUnionKind enum type); while this test also clashes in the C struct. > Don't know if it is worth a v8 to clean up the duplication, or if we > just save it for a followup patch (namely, where I try to move errors > into the QAPISchema.check() methods); I found the issue while playing > with v5 15/46. Your choice. The rather minor issues I found in this series could all be touched up on commit (assuming consensus on what to do).
On 10/01/2015 05:51 AM, Markus Armbruster wrote: >> +++ b/tests/qapi-schema/alternate-clash.json >> @@ -1,3 +1,8 @@ >> -# we detect C enum collisions in an alternate >> +# Alternate branch name collision >> +# Reject an alternate that would result in a collision in generated C >> +# names (this would try to generate two enum values 'ALT1_KIND_A_B'). >> +# TODO: In the future, if alternates are simplified to not generate >> +# the implicit Alt1Kind enum, we would still have a collision with the >> +# resulting C union trying to have two members named 'a_b'. >> { 'alternate': 'Alt1', >> - 'data': { 'one': 'str', 'ONE': 'int' } } >> + 'data': { 'a-b': 'str', 'a_b': 'int' } } > > Ah, you're making the test slightly more robust. Works for me. I just noticed we lack coverage for: { 'alternate': 'Alt', 'data': { 'type': 'int', 'other': 'str' } } (tries to create a C struct with two members named 'type', even after my v5 patches change to a simpler 'qtype_code type'). Can be done in my later patches that simplify alternates if we don't want a v8 spin of this part of the series. >> +++ b/tests/qapi-schema/flat-union-clash-type.json >> @@ -0,0 +1,15 @@ >> +# Flat union branch 'type' >> +# FIXME: this triggers an assertion failure. But even with that fixed, we >> +# would have a clash in generated C, between the outer tag 'type' and > > "outer tag"? I guess you mean the member type we inherit from Base. Yes. >> +++ b/tests/qapi-schema/flat-union-cycle.json >> @@ -0,0 +1,8 @@ >> +# Ensure that we have a sane error message for attempts at self-inheritance >> +# This test currently fails because we don't permit a union base, but >> +# even if we relax things to allow that in the future (see >> +# flat-union-base-union), self-inheritance should still fail. > > Do we have a test for the simpler case of a struct inheriting from > itself? Not here, but in v5 16/46. That's because it asserts, but while it was easy to fix up in the QAPISchema.check(), I did not find it worth the churn to fix it up in the ad hoc parse code just to rip it back out later, and the QAPISchema.check() code requires several scaffolding patches (so it wasn't as easy as fixing the union 'type' clash asserts). Tracking an assertion failure for any more than one patch at a time is horrible (as any other change to qapi.py changes line numbers that affect the assertion failure). > > I believe us merging struct and union types into a single object type is > more likely than us implementing union bases. If I'm correct, we won't > need this test. Maybe, but even then, we still have to decide if a base object can have variants. > > Found nothing that couldn't be touched up on commit. Your suggestions for wording tweaks are fine; I'm okay if you want to tweak it for your pull request instead of asking me for a v8.
Eric Blake <eblake@redhat.com> writes: > On 10/01/2015 05:51 AM, Markus Armbruster wrote: [...] >>> +++ b/tests/qapi-schema/flat-union-cycle.json >>> @@ -0,0 +1,8 @@ >>> +# Ensure that we have a sane error message for attempts at self-inheritance >>> +# This test currently fails because we don't permit a union base, but >>> +# even if we relax things to allow that in the future (see >>> +# flat-union-base-union), self-inheritance should still fail. >> >> Do we have a test for the simpler case of a struct inheriting from >> itself? > > Not here, but in v5 16/46. That's because it asserts, but while it was > easy to fix up in the QAPISchema.check(), I did not find it worth the > churn to fix it up in the ad hoc parse code just to rip it back out > later, and the QAPISchema.check() code requires several scaffolding > patches (so it wasn't as easy as fixing the union 'type' clash asserts). > Tracking an assertion failure for any more than one patch at a time is > horrible (as any other change to qapi.py changes line numbers that > affect the assertion failure). Well, I'm happy to take a test for inheritance loops, or leave it uncovered for now, but I don't want to take a non-working test of an unimplemented obscure case "union" without a test for the implemented case "struct". I can: A. Drop this test case. B. Replace it with the test case from 16/46. C. Add the test case from 16/46 and keep this one. I very much prefer B. You? >> I believe us merging struct and union types into a single object type is >> more likely than us implementing union bases. If I'm correct, we won't >> need this test. > > Maybe, but even then, we still have to decide if a base object can have > variants. Yes, but even if we want them, we'll have to rewrite this test case from scratch. I'm no friend of present complications to maybe save future work. Even less when it's so unlikely to save any. [...]
On 10/01/2015 09:34 AM, Markus Armbruster wrote: >>> Do we have a test for the simpler case of a struct inheriting from >>> itself? >> >> Not here, but in v5 16/46. That's because it asserts, but while it was >> easy to fix up in the QAPISchema.check(), I did not find it worth the >> churn to fix it up in the ad hoc parse code just to rip it back out >> later, and the QAPISchema.check() code requires several scaffolding >> patches (so it wasn't as easy as fixing the union 'type' clash asserts). >> Tracking an assertion failure for any more than one patch at a time is >> horrible (as any other change to qapi.py changes line numbers that >> affect the assertion failure). > > Well, I'm happy to take a test for inheritance loops, or leave it > uncovered for now, but I don't want to take a non-working test of an > unimplemented obscure case "union" without a test for the implemented > case "struct". > > I can: > > A. Drop this test case. > > B. Replace it with the test case from 16/46. > > C. Add the test case from 16/46 and keep this one. > > I very much prefer B. You? If we go with B, we'd have an assertion failure that does not get fixed by 6/18, and therefore is subject to churn until the fix is present. I'm leaning towards A (calling self-inheritance a name collision is a bit of a stretch in the first place; and leaving it untested until 16/46 goes in doesn't hurt).
Eric Blake <eblake@redhat.com> writes: > On 10/01/2015 09:34 AM, Markus Armbruster wrote: > >>>> Do we have a test for the simpler case of a struct inheriting from >>>> itself? >>> >>> Not here, but in v5 16/46. That's because it asserts, but while it was >>> easy to fix up in the QAPISchema.check(), I did not find it worth the >>> churn to fix it up in the ad hoc parse code just to rip it back out >>> later, and the QAPISchema.check() code requires several scaffolding >>> patches (so it wasn't as easy as fixing the union 'type' clash asserts). >>> Tracking an assertion failure for any more than one patch at a time is >>> horrible (as any other change to qapi.py changes line numbers that >>> affect the assertion failure). >> >> Well, I'm happy to take a test for inheritance loops, or leave it >> uncovered for now, but I don't want to take a non-working test of an >> unimplemented obscure case "union" without a test for the implemented >> case "struct". >> >> I can: >> >> A. Drop this test case. >> >> B. Replace it with the test case from 16/46. >> >> C. Add the test case from 16/46 and keep this one. >> >> I very much prefer B. You? > > If we go with B, we'd have an assertion failure that does not get fixed > by 6/18, and therefore is subject to churn until the fix is present. > > I'm leaning towards A (calling self-inheritance a name collision is a > bit of a stretch in the first place; and leaving it untested until 16/46 > goes in doesn't hurt). Okay, A. it is. I pushed to branch qapi-next at http://repo.or.cz/qemu/armbru.git
On 10/01/2015 11:39 AM, Markus Armbruster wrote: >> I'm leaning towards A (calling self-inheritance a name collision is a >> bit of a stretch in the first place; and leaving it untested until 16/46 >> goes in doesn't hurt). > > Okay, A. it is. I pushed to branch qapi-next at > http://repo.or.cz/qemu/armbru.git Branch looks good to go with one nit: 15/18 commit message (currently e7462ff) has a typo in the text you massaged: The generated code has fewer blank line in qapi-event.c functions, but has no semantic difference. s/line/lines/ I'll rebase my remaining patches on top of your tip, and let you decide how long to wait before sending the pull request. And with your rewrite of 17/18, this part of the series no longer depends on the python 2.6 configure patch.
Eric Blake <eblake@redhat.com> writes: > On 10/01/2015 11:39 AM, Markus Armbruster wrote: > >>> I'm leaning towards A (calling self-inheritance a name collision is a >>> bit of a stretch in the first place; and leaving it untested until 16/46 >>> goes in doesn't hurt). >> >> Okay, A. it is. I pushed to branch qapi-next at >> http://repo.or.cz/qemu/armbru.git > > Branch looks good to go with one nit: 15/18 commit message (currently > e7462ff) has a typo in the text you massaged: > > The generated code has fewer blank line in qapi-event.c functions, > but has no semantic difference. > > s/line/lines/ Fixed and pushed. Thanks! > I'll rebase my remaining patches on top of your tip, and let you decide > how long to wait before sending the pull request. And with your rewrite > of 17/18, this part of the series no longer depends on the python 2.6 > configure patch. I'll wait a few days. We'll need the time to review the next batch of patches anyway.
diff --git a/tests/Makefile b/tests/Makefile index 6fd5885..2fdf88a 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-branches.json +qapi-schema += union-clash-data.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/alternate-clash.err b/tests/qapi-schema/alternate-clash.err index 51bea3e..a475ab6 100644 --- a/tests/qapi-schema/alternate-clash.err +++ b/tests/qapi-schema/alternate-clash.err @@ -1 +1 @@ -tests/qapi-schema/alternate-clash.json:2: Alternate 'Alt1' member 'ONE' clashes with 'one' +tests/qapi-schema/alternate-clash.json:7: Alternate 'Alt1' member 'a_b' clashes with 'a-b' diff --git a/tests/qapi-schema/alternate-clash.json b/tests/qapi-schema/alternate-clash.json index 3947935..6d73bc5 100644 --- a/tests/qapi-schema/alternate-clash.json +++ b/tests/qapi-schema/alternate-clash.json @@ -1,3 +1,8 @@ -# we detect C enum collisions in an alternate +# Alternate branch name collision +# Reject an alternate that would result in a collision in generated C +# names (this would try to generate two enum values 'ALT1_KIND_A_B'). +# TODO: In the future, if alternates are simplified to not generate +# the implicit Alt1Kind enum, we would still have a collision with the +# resulting C union trying to have two members named 'a_b'. { 'alternate': 'Alt1', - 'data': { 'one': 'str', 'ONE': 'int' } } + 'data': { 'a-b': 'str', 'a_b': 'int' } } diff --git a/tests/qapi-schema/flat-union-branch-clash.out b/tests/qapi-schema/args-name-clash.err similarity index 100% rename from tests/qapi-schema/flat-union-branch-clash.out rename to tests/qapi-schema/args-name-clash.err diff --git a/tests/qapi-schema/args-name-clash.exit b/tests/qapi-schema/args-name-clash.exit new file mode 100644 index 0000000..573541a --- /dev/null +++ b/tests/qapi-schema/args-name-clash.exit @@ -0,0 +1 @@ +0 diff --git a/tests/qapi-schema/args-name-clash.json b/tests/qapi-schema/args-name-clash.json new file mode 100644 index 0000000..9e8f889 --- /dev/null +++ b/tests/qapi-schema/args-name-clash.json @@ -0,0 +1,5 @@ +# C member name collision +# FIXME - This parses, but fails to compile, because the C struct is given +# two 'a_b' members. Either reject this at parse time, or munge the C names +# to avoid the collision. +{ 'command': 'oops', 'data': { 'a-b': 'str', 'a_b': 'str' } } diff --git a/tests/qapi-schema/args-name-clash.out b/tests/qapi-schema/args-name-clash.out new file mode 100644 index 0000000..9b2f6e4 --- /dev/null +++ b/tests/qapi-schema/args-name-clash.out @@ -0,0 +1,6 @@ +object :empty +object :obj-oops-arg + member a-b: str optional=False + member a_b: str optional=False +command oops :obj-oops-arg -> None + gen=True success_response=True diff --git a/tests/qapi-schema/duplicate-key.err b/tests/qapi-schema/duplicate-key.err index 768b276..6d02f83 100644 --- a/tests/qapi-schema/duplicate-key.err +++ b/tests/qapi-schema/duplicate-key.err @@ -1 +1 @@ -tests/qapi-schema/duplicate-key.json:2:10: Duplicate key "key" +tests/qapi-schema/duplicate-key.json:3:10: Duplicate key "key" diff --git a/tests/qapi-schema/duplicate-key.json b/tests/qapi-schema/duplicate-key.json index 1b55d88..14ac0e8 100644 --- a/tests/qapi-schema/duplicate-key.json +++ b/tests/qapi-schema/duplicate-key.json @@ -1,2 +1,3 @@ +# QAPI cannot include the same key more than once in any {} { 'key': 'value', 'key': 'value' } diff --git a/tests/qapi-schema/flat-union-base-union.err b/tests/qapi-schema/flat-union-base-union.err index ede9859..28725ed 100644 --- a/tests/qapi-schema/flat-union-base-union.err +++ b/tests/qapi-schema/flat-union-base-union.err @@ -1 +1 @@ -tests/qapi-schema/flat-union-base-union.json:11: Base 'UnionBase' is not a valid struct +tests/qapi-schema/flat-union-base-union.json:14: Base 'UnionBase' is not a valid struct diff --git a/tests/qapi-schema/flat-union-base-union.json b/tests/qapi-schema/flat-union-base-union.json index 6a8ea68..98b4eba 100644 --- a/tests/qapi-schema/flat-union-base-union.json +++ b/tests/qapi-schema/flat-union-base-union.json @@ -1,4 +1,7 @@ -# we require the base to be a struct +# For now, we require the base to be a struct without variants +# TODO: It would be possible to allow a union as a base, as long as all +# permutations of QMP names exposed by base do not clash with any QMP +# member names added by local variants. { 'enum': 'TestEnum', 'data': [ 'value1', 'value2' ] } { 'struct': 'TestTypeA', diff --git a/tests/qapi-schema/flat-union-branch-clash.err b/tests/qapi-schema/flat-union-branch-clash.err deleted file mode 100644 index f112766..0000000 --- a/tests/qapi-schema/flat-union-branch-clash.err +++ /dev/null @@ -1 +0,0 @@ -tests/qapi-schema/flat-union-branch-clash.json:10: Member name 'name' of branch 'value1' clashes with base 'Base' diff --git a/tests/qapi-schema/flat-union-clash-branch.err b/tests/qapi-schema/flat-union-clash-branch.err new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/flat-union-clash-branch.exit b/tests/qapi-schema/flat-union-clash-branch.exit new file mode 100644 index 0000000..573541a --- /dev/null +++ b/tests/qapi-schema/flat-union-clash-branch.exit @@ -0,0 +1 @@ +0 diff --git a/tests/qapi-schema/flat-union-clash-branch.json b/tests/qapi-schema/flat-union-clash-branch.json new file mode 100644 index 0000000..e593336 --- /dev/null +++ b/tests/qapi-schema/flat-union-clash-branch.json @@ -0,0 +1,18 @@ +# Flat union branch name collision +# FIXME: this parses, but then fails to compile due to a duplicate 'c_d' +# (one from the base member, the other from the branch name). We should +# either reject the collision at parse time, or munge the generated branch +# name to allow this to compile. +{ 'enum': 'TestEnum', + 'data': [ 'base', 'c-d' ] } +{ 'struct': 'Base', + 'data': { 'enum1': 'TestEnum', '*c_d': 'str' } } +{ 'struct': 'Branch1', + 'data': { 'string': 'str' } } +{ 'struct': 'Branch2', + 'data': { 'value': 'int' } } +{ 'union': 'TestUnion', + 'base': 'Base', + 'discriminator': 'enum1', + 'data': { 'base': 'Branch1', + 'c-d': 'Branch2' } } diff --git a/tests/qapi-schema/flat-union-clash-branch.out b/tests/qapi-schema/flat-union-clash-branch.out new file mode 100644 index 0000000..8e0da73 --- /dev/null +++ b/tests/qapi-schema/flat-union-clash-branch.out @@ -0,0 +1,14 @@ +object :empty +object Base + member enum1: TestEnum optional=False + member c_d: str optional=True +object Branch1 + member string: str optional=False +object Branch2 + member value: int optional=False +enum TestEnum ['base', 'c-d'] +object TestUnion + base Base + tag enum1 + case base: Branch1 + case c-d: Branch2 diff --git a/tests/qapi-schema/flat-union-clash-member.err b/tests/qapi-schema/flat-union-clash-member.err new file mode 100644 index 0000000..152d402 --- /dev/null +++ b/tests/qapi-schema/flat-union-clash-member.err @@ -0,0 +1 @@ +tests/qapi-schema/flat-union-clash-member.json:10: Member name 'name' of branch 'value1' clashes with base 'Base' diff --git a/tests/qapi-schema/flat-union-branch-clash.exit b/tests/qapi-schema/flat-union-clash-member.exit similarity index 100% rename from tests/qapi-schema/flat-union-branch-clash.exit rename to tests/qapi-schema/flat-union-clash-member.exit diff --git a/tests/qapi-schema/flat-union-branch-clash.json b/tests/qapi-schema/flat-union-clash-member.json similarity index 85% rename from tests/qapi-schema/flat-union-branch-clash.json rename to tests/qapi-schema/flat-union-clash-member.json index 8fb054f..3d7e6c6 100644 --- a/tests/qapi-schema/flat-union-branch-clash.json +++ b/tests/qapi-schema/flat-union-clash-member.json @@ -1,4 +1,4 @@ -# we check for no duplicate keys between branches and base +# we check for no duplicate keys between branch members and base { 'enum': 'TestEnum', 'data': [ 'value1', 'value2' ] } { 'struct': 'Base', diff --git a/tests/qapi-schema/flat-union-clash-member.out b/tests/qapi-schema/flat-union-clash-member.out new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/flat-union-clash-type.err b/tests/qapi-schema/flat-union-clash-type.err new file mode 100644 index 0000000..6e64d1d --- /dev/null +++ b/tests/qapi-schema/flat-union-clash-type.err @@ -0,0 +1,16 @@ +Traceback (most recent call last): + File "tests/qapi-schema/test-qapi.py", line 55, in <module> + schema = QAPISchema(sys.argv[1]) + File "scripts/qapi.py", line 1116, in __init__ + self.check() + File "scripts/qapi.py", line 1299, in check + ent.check(self) + File "scripts/qapi.py", line 962, in check + self.variants.check(schema, members, seen) + File "scripts/qapi.py", line 1024, in check + v.check(schema, self.tag_member.type, vseen) + File "scripts/qapi.py", line 1032, in check + QAPISchemaObjectTypeMember.check(self, schema, [], seen) + File "scripts/qapi.py", line 994, in check + assert self.name not in seen +AssertionError diff --git a/tests/qapi-schema/flat-union-clash-type.exit b/tests/qapi-schema/flat-union-clash-type.exit new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/tests/qapi-schema/flat-union-clash-type.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/flat-union-clash-type.json b/tests/qapi-schema/flat-union-clash-type.json new file mode 100644 index 0000000..eac51a4 --- /dev/null +++ b/tests/qapi-schema/flat-union-clash-type.json @@ -0,0 +1,15 @@ +# Flat union branch 'type' +# FIXME: this triggers an assertion failure. But even with that fixed, we +# would have a clash in generated C, between the outer tag 'type' and +# the branch name 'type' within the union. We should either reject this, +# or munge the generated C to let it compile. +{ 'enum': 'TestEnum', + 'data': [ 'type' ] } +{ 'struct': 'Base', + 'data': { 'type': 'TestEnum' } } +{ 'struct': 'Branch1', + 'data': { 'string': 'str' } } +{ 'union': 'TestUnion', + 'base': 'Base', + 'discriminator': 'type', + 'data': { 'type': 'Branch1' } } diff --git a/tests/qapi-schema/flat-union-clash-type.out b/tests/qapi-schema/flat-union-clash-type.out new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/flat-union-cycle.err b/tests/qapi-schema/flat-union-cycle.err new file mode 100644 index 0000000..9b7978a --- /dev/null +++ b/tests/qapi-schema/flat-union-cycle.err @@ -0,0 +1 @@ +tests/qapi-schema/flat-union-cycle.json:7: Base 'Union' is not a valid struct diff --git a/tests/qapi-schema/flat-union-cycle.exit b/tests/qapi-schema/flat-union-cycle.exit new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/tests/qapi-schema/flat-union-cycle.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/flat-union-cycle.json b/tests/qapi-schema/flat-union-cycle.json new file mode 100644 index 0000000..c66c4c9 --- /dev/null +++ b/tests/qapi-schema/flat-union-cycle.json @@ -0,0 +1,8 @@ +# Ensure that we have a sane error message for attempts at self-inheritance +# This test currently fails because we don't permit a union base, but +# even if we relax things to allow that in the future (see +# flat-union-base-union), self-inheritance should still fail. +{ 'enum': 'Enum', 'data': [ 'okay' ] } +{ 'struct': 'Okay', 'data': { 'int': 'int' } } +{ 'union': 'Union', 'base': 'Union', 'discriminator': 'switch', + 'data': { 'okay': 'Okay' } } diff --git a/tests/qapi-schema/flat-union-cycle.out b/tests/qapi-schema/flat-union-cycle.out new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index 6897a6e..c511338 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -32,11 +32,14 @@ 'dict1': 'UserDefTwoDict' } } # for testing unions +# Among other things, test that a name collision between branches does +# not cause any problems (since only one branch can be in use at a time), +# by intentionally using two branches that both have a C member 'a_b' { 'struct': 'UserDefA', - 'data': { 'boolean': 'bool' } } + 'data': { 'boolean': 'bool', '*a_b': 'int' } } { 'struct': 'UserDefB', - 'data': { 'intb': 'int' } } + 'data': { 'intb': 'int', '*a-b': 'bool' } } { 'union': 'UserDefFlatUnion', 'base': 'UserDefUnionBase', # intentional forward reference diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index 1f6e858..28a0b3c 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -71,6 +71,7 @@ enum QEnumTwo ['value1', 'value2'] prefix QENUM_TWO object UserDefA member boolean: bool optional=False + member a_b: int optional=True alternate UserDefAlternate case uda: UserDefA case s: str @@ -78,6 +79,7 @@ alternate UserDefAlternate enum UserDefAlternateKind ['uda', 's', 'i'] object UserDefB member intb: int optional=False + member a-b: bool optional=True object UserDefC member string1: str optional=False member string2: str optional=False diff --git a/tests/qapi-schema/struct-base-clash-base.err b/tests/qapi-schema/struct-base-clash-base.err new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/struct-base-clash-base.exit b/tests/qapi-schema/struct-base-clash-base.exit new file mode 100644 index 0000000..573541a --- /dev/null +++ b/tests/qapi-schema/struct-base-clash-base.exit @@ -0,0 +1 @@ +0 diff --git a/tests/qapi-schema/struct-base-clash-base.json b/tests/qapi-schema/struct-base-clash-base.json new file mode 100644 index 0000000..0c84025 --- /dev/null +++ b/tests/qapi-schema/struct-base-clash-base.json @@ -0,0 +1,9 @@ +# Struct member 'base' +# FIXME: this parses, but then fails to compile due to a duplicate 'base' +# (one explicit in QMP, the other used to box the base class members). +# We should either reject the collision at parse time, or change the +# generated struct to allow this to compile. +{ 'struct': 'Base', 'data': {} } +{ 'struct': 'Sub', + 'base': 'Base', + 'data': { 'base': 'str' } } diff --git a/tests/qapi-schema/struct-base-clash-base.out b/tests/qapi-schema/struct-base-clash-base.out new file mode 100644 index 0000000..e69a416 --- /dev/null +++ b/tests/qapi-schema/struct-base-clash-base.out @@ -0,0 +1,5 @@ +object :empty +object Base +object Sub + base Base + member base: str optional=False diff --git a/tests/qapi-schema/struct-base-clash-deep.err b/tests/qapi-schema/struct-base-clash-deep.err index e3e9f8d..f7a25a3 100644 --- a/tests/qapi-schema/struct-base-clash-deep.err +++ b/tests/qapi-schema/struct-base-clash-deep.err @@ -1 +1 @@ -tests/qapi-schema/struct-base-clash-deep.json:7: Member name 'name' clashes with base 'Base' +tests/qapi-schema/struct-base-clash-deep.json:10: Member name 'name' clashes with base 'Base' diff --git a/tests/qapi-schema/struct-base-clash-deep.json b/tests/qapi-schema/struct-base-clash-deep.json index 552fe94..fa873ab 100644 --- a/tests/qapi-schema/struct-base-clash-deep.json +++ b/tests/qapi-schema/struct-base-clash-deep.json @@ -1,4 +1,7 @@ -# we check for no duplicate keys with indirect base +# Reject attempts to duplicate QMP members +# Here, 'name' would have to appear twice on the wire, locally and +# indirectly for the grandparent base; the collision doesn't care that +# one instance is optional. { 'struct': 'Base', 'data': { 'name': 'str' } } { 'struct': 'Mid', diff --git a/tests/qapi-schema/struct-base-clash.err b/tests/qapi-schema/struct-base-clash.err index 3ac37fb..3a9f66b 100644 --- a/tests/qapi-schema/struct-base-clash.err +++ b/tests/qapi-schema/struct-base-clash.err @@ -1 +1 @@ -tests/qapi-schema/struct-base-clash.json:4: Member name 'name' clashes with base 'Base' +tests/qapi-schema/struct-base-clash.json:5: Member name 'name' clashes with base 'Base' diff --git a/tests/qapi-schema/struct-base-clash.json b/tests/qapi-schema/struct-base-clash.json index f2afc9b..11aec80 100644 --- a/tests/qapi-schema/struct-base-clash.json +++ b/tests/qapi-schema/struct-base-clash.json @@ -1,4 +1,5 @@ -# we check for no duplicate keys with base +# Reject attempts to duplicate QMP members +# Here, 'name' would have to appear twice on the wire, locally and for base. { 'struct': 'Base', 'data': { 'name': 'str' } } { 'struct': 'Sub', diff --git a/tests/qapi-schema/union-clash-branches.err b/tests/qapi-schema/union-clash-branches.err new file mode 100644 index 0000000..005c48d --- /dev/null +++ b/tests/qapi-schema/union-clash-branches.err @@ -0,0 +1 @@ +tests/qapi-schema/union-clash-branches.json:4: Union 'TestUnion' member 'a_b' clashes with 'a-b' diff --git a/tests/qapi-schema/union-clash-branches.exit b/tests/qapi-schema/union-clash-branches.exit new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/tests/qapi-schema/union-clash-branches.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/union-clash-branches.json b/tests/qapi-schema/union-clash-branches.json new file mode 100644 index 0000000..31d135f --- /dev/null +++ b/tests/qapi-schema/union-clash-branches.json @@ -0,0 +1,5 @@ +# Union branch name collision +# Reject a union that would result in a collision in generated C names (this +# would try to generate two enum values 'TEST_UNION_KIND_A_B'). +{ 'union': 'TestUnion', + 'data': { 'a-b': 'int', 'a_b': 'str' } } diff --git a/tests/qapi-schema/union-clash-branches.out b/tests/qapi-schema/union-clash-branches.out new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/union-clash-data.err b/tests/qapi-schema/union-clash-data.err new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/union-clash-data.exit b/tests/qapi-schema/union-clash-data.exit new file mode 100644 index 0000000..573541a --- /dev/null +++ b/tests/qapi-schema/union-clash-data.exit @@ -0,0 +1 @@ +0 diff --git a/tests/qapi-schema/union-clash-data.json b/tests/qapi-schema/union-clash-data.json new file mode 100644 index 0000000..7308e69 --- /dev/null +++ b/tests/qapi-schema/union-clash-data.json @@ -0,0 +1,7 @@ +# Union branch 'data' +# FIXME: this parses, but then fails to compile due to a duplicate 'data' +# (one from the branch name, another as a filler to avoid an empty union). +# we should either detect the collision at parse time, or change the +# generated struct to allow this to compile. +{ 'union': 'TestUnion', + 'data': { 'data': 'int' } } diff --git a/tests/qapi-schema/union-clash-data.out b/tests/qapi-schema/union-clash-data.out new file mode 100644 index 0000000..6277239 --- /dev/null +++ b/tests/qapi-schema/union-clash-data.out @@ -0,0 +1,6 @@ +object :empty +object :obj-int-wrapper + member data: int optional=False +object TestUnion + case data: :obj-int-wrapper +enum TestUnionKind ['data'] diff --git a/tests/qapi-schema/union-clash-type.err b/tests/qapi-schema/union-clash-type.err new file mode 100644 index 0000000..6e64d1d --- /dev/null +++ b/tests/qapi-schema/union-clash-type.err @@ -0,0 +1,16 @@ +Traceback (most recent call last): + File "tests/qapi-schema/test-qapi.py", line 55, in <module> + schema = QAPISchema(sys.argv[1]) + File "scripts/qapi.py", line 1116, in __init__ + self.check() + File "scripts/qapi.py", line 1299, in check + ent.check(self) + File "scripts/qapi.py", line 962, in check + self.variants.check(schema, members, seen) + File "scripts/qapi.py", line 1024, in check + v.check(schema, self.tag_member.type, vseen) + File "scripts/qapi.py", line 1032, in check + QAPISchemaObjectTypeMember.check(self, schema, [], seen) + File "scripts/qapi.py", line 994, in check + assert self.name not in seen +AssertionError diff --git a/tests/qapi-schema/union-clash-type.exit b/tests/qapi-schema/union-clash-type.exit new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/tests/qapi-schema/union-clash-type.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/union-clash-type.json b/tests/qapi-schema/union-clash-type.json new file mode 100644 index 0000000..38330b5 --- /dev/null +++ b/tests/qapi-schema/union-clash-type.json @@ -0,0 +1,9 @@ +# Union branch 'type' +# FIXME: this triggers an assertion failure. But even with that fixed, we +# would have a clash in generated C, between the outer union tag 'kind' and +# the branch name 'kind' within the union. We should either reject this, +# or munge the generated C to let it compile. +# TODO: Even when the generated C is switched to use 'type' rather than +# 'kind', to match the QMP spelling, the collision should still be detected. +{ 'union': 'TestUnion', + 'data': { 'kind': 'int', 'type': 'str' } }
Expose some weaknesses in the generator: we don't always forbid the generation of structs that contain multiple members that map to the same C or QMP name. This has already been marked FIXME in qapi.py in commit d90675f, but having more tests will make sure future patches produce desired behavior; and updating existing patches to better document things doesn't hurt, either. Some of these collisions are already caught in the old-style parser checks, but ultimately we want all collisions to be caught in the new-style QAPISchema*.check() methods. This patch focuses on C struct members, and does not consider collisions between commands and events (affecting C function names), or even collisions between generated C type names with user type names (for things like automatic FOOList struct representing array types or FOOKind for an implicit enum). There are two types of struct collisions we want to catch: 1) Collision between two keys in a JSON object. qapi.py prevents that within a single struct (see duplicate-key), but it is possible to have collisions between a type's members and its base type's members (struct-base-clash, struct-base-clash-deep, flat-union-cycle), and its flat union variant members (flat-union-clash-member). 2) Collision between two members of the C struct that is generated for a given QAPI type: a) Multiple QAPI names map to the same C name (args-name-clash) b) A QAPI name maps to a C name that is used for another purpose (flat-union-clash-branch, struct-base-clash-base, union-clash-data). We already fixed some such cases in commit 0f61af3e and 1e6c1616, but more remain. c) Two C names generated for other purposes clash (alternate-clash, union-clash-branches, union-clash-type, flat-union-clash-type) 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 name the union (using 'foo.u.value' rather than 'foo.value') or otherwise munge the C name corresponding to enum tag values. But unless such a need arises, it will probably be easier to just forbid these collisions. Some of these negative tests will be deleted later, and positive tests added to qapi-schema-test.json in their place, when the generator code is reworked to avoid particular code generation collisions in class 2). [Note that viewing this patch with git rename detection enabled may see some confusion due to renaming some tests while adding others, but where the content is similar enough that git picks the wrong pre- and post-patch files to associate] Signed-off-by: Eric Blake <eblake@redhat.com> --- v7: rework commit message again, even more comments in tests, yet another rename (union-clash-members is now union-clash-branches) v6: rework commit message, alter test names and comments to be more descriptive, repurpose flat-union-cycle to better match its name (and not duplicate what the renamed flat-union-clash-branch tests), add new tests (union-clash-type, flat-union-clash-type) that detect an assertion failures --- tests/Makefile | 10 +++++++++- tests/qapi-schema/alternate-clash.err | 2 +- tests/qapi-schema/alternate-clash.json | 9 +++++++-- ...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 | 5 +++++ tests/qapi-schema/args-name-clash.out | 6 ++++++ tests/qapi-schema/duplicate-key.err | 2 +- tests/qapi-schema/duplicate-key.json | 1 + tests/qapi-schema/flat-union-base-union.err | 2 +- tests/qapi-schema/flat-union-base-union.json | 5 ++++- 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 | 18 ++++++++++++++++++ tests/qapi-schema/flat-union-clash-branch.out | 14 ++++++++++++++ tests/qapi-schema/flat-union-clash-member.err | 1 + ...-branch-clash.exit => flat-union-clash-member.exit} | 0 ...-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 | 15 +++++++++++++++ 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 | 8 ++++++++ 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 | 9 +++++++++ tests/qapi-schema/struct-base-clash-base.out | 5 +++++ tests/qapi-schema/struct-base-clash-deep.err | 2 +- tests/qapi-schema/struct-base-clash-deep.json | 5 ++++- tests/qapi-schema/struct-base-clash.err | 2 +- tests/qapi-schema/struct-base-clash.json | 3 ++- tests/qapi-schema/union-clash-branches.err | 1 + tests/qapi-schema/union-clash-branches.exit | 1 + tests/qapi-schema/union-clash-branches.json | 5 +++++ tests/qapi-schema/union-clash-branches.out | 0 tests/qapi-schema/union-clash-data.err | 0 tests/qapi-schema/union-clash-data.exit | 1 + tests/qapi-schema/union-clash-data.json | 7 +++++++ tests/qapi-schema/union-clash-data.out | 6 ++++++ tests/qapi-schema/union-clash-type.err | 16 ++++++++++++++++ tests/qapi-schema/union-clash-type.exit | 1 + tests/qapi-schema/union-clash-type.json | 9 +++++++++ tests/qapi-schema/union-clash-type.out | 0 50 files changed, 190 insertions(+), 15 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-branches.err create mode 100644 tests/qapi-schema/union-clash-branches.exit create mode 100644 tests/qapi-schema/union-clash-branches.json create mode 100644 tests/qapi-schema/union-clash-branches.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-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