Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
qapi: Test for various name collisions
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 test duplicate-key), but it is possible to have collisions between a type's members and its base type's members (existing tests struct-base-clash, struct-base-clash-deep), and its flat union variant members (renamed 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 (new test args-name-clash) b) A QAPI name maps to a C name that is used for another purpose (new tests flat-union-clash-branch, struct-base-clash-base, union-clash-data). We already fixed some such cases in commit 0f61af3 and 1e6c161, but more remain. c) Two C names generated for other purposes clash (updated test alternate-clash, new test union-clash-branches, union-clash-type, flat-union-clash-type) Ultimately, if we need to have a flat union where a tag 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 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> Message-Id: <1443565276-4535-6-git-send-email-eblake@redhat.com> [Improve commit message and comments a bit, drop an unrelated test] Signed-off-by: Markus Armbruster <armbru@redhat.com>
- Loading branch information
Showing
46 changed files
with
182 additions
and
15 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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' |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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' } } |
File renamed without changes.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
0 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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' } } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
tests/qapi-schema/duplicate-key.json:2:10: Duplicate key "key" | ||
tests/qapi-schema/duplicate-key.json:3:10: Duplicate key "key" |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,3 @@ | ||
# QAPI cannot include the same key more than once in any {} | ||
{ 'key': 'value', | ||
'key': 'value' } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
Empty file.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
0 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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' } } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
tests/qapi-schema/flat-union-clash-member.json:11: Member name 'name' of branch 'value1' clashes with base 'Base' |
File renamed without changes.
3 changes: 2 additions & 1 deletion
3
.../qapi-schema/flat-union-branch-clash.json → .../qapi-schema/flat-union-clash-member.json
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Empty file.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
1 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
# 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 member 'type' | ||
# inherited from 'Base' 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' } } |
Empty file.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Empty file.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
0 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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' } } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
object :empty | ||
object Base | ||
object Sub | ||
base Base | ||
member base: str optional=False |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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' |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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' |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
tests/qapi-schema/union-clash-branches.json:4: Union 'TestUnion' member 'a_b' clashes with 'a-b' |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
1 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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' } } |
Empty file.
Empty file.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
0 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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' } } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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'] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
1 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
# Union branch 'type' | ||
# FIXME: this triggers an assertion failure. But even with that fixed, | ||
# we would have a clash in generated C, between 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' } } |
Empty file.