| Message ID | 1443565276-4535-7-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > The previous commit added two tests that triggered an assertion > failure. It's fairly straightforward to avoid the failure by > just outright forbidding the collision between a union's tag > values and its discriminator name (including the implicit name > 'kind' supplied for simple unions [*]). Ultimately, we'd like > to move the collision detection into QAPISchema*.check(), but > for now it is easier just to enhance the existing checks. > > [*] Of course, down the road, we have plans to rename the simple > union tag name to 'type' to match the QMP wire name, but the > idea of the collision will still be present even then. > > Technically, we could avoid the collision by naming the C union > members representing each enum value as '_case_value' rather > than 'value'; but until we have an actual qapi client (and not > just our testsuite) that has a legitimate reason to match a > case label to the name of a QMP key and needs the name munging > to satisfy the compiler, it's easier to just reject the qapi > as invalid. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v7: rebase to testsuite changes in previous patch > v6: new patch > --- > scripts/qapi.py | 10 ++++++++-- > tests/qapi-schema/flat-union-clash-type.err | 17 +---------------- > tests/qapi-schema/flat-union-clash-type.json | 7 +++---- > tests/qapi-schema/union-clash-type.err | 17 +---------------- > tests/qapi-schema/union-clash-type.json | 7 +++---- > 5 files changed, 16 insertions(+), 42 deletions(-) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 4b5d574..8d2681b 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -544,7 +544,7 @@ def check_union(expr, expr_info): > base = expr.get('base') > discriminator = expr.get('discriminator') > members = expr['data'] > - values = {'MAX': '(automatic)'} > + values = {'MAX': '(automatic)', 'KIND': '(automatic)'} > > # Two types of unions, determined by discriminator. > > @@ -603,13 +603,19 @@ def check_union(expr, expr_info): > " of branch '%s'" % key) > > # If the discriminator names an enum type, then all members > - # of 'data' must also be members of the enum type. > + # of 'data' must also be members of the enum type, which in turn > + # must not collide with the discriminator name. > if enum_define: > if key not in enum_define['enum_values']: > raise QAPIExprError(expr_info, > "Discriminator value '%s' is not found in " > "enum '%s'" % > (key, enum_define["enum_name"])) > + if discriminator in enum_define['enum_values']: > + raise QAPIExprError(expr_info, > + "Discriminator name '%s' collides with " > + "enum value in '%s'" % > + (discriminator, enum_define["enum_name"])) > > # Otherwise, check for conflicts in the generated enum > else: > diff --git a/tests/qapi-schema/flat-union-clash-type.err b/tests/qapi-schema/flat-union-clash-type.err > index 6e64d1d..b44dd40 100644 > --- a/tests/qapi-schema/flat-union-clash-type.err > +++ b/tests/qapi-schema/flat-union-clash-type.err > @@ -1,16 +1 @@ > -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 > +tests/qapi-schema/flat-union-clash-type.json:11: Discriminator name 'type' collides with enum value in 'TestEnum' > diff --git a/tests/qapi-schema/flat-union-clash-type.json b/tests/qapi-schema/flat-union-clash-type.json > index eac51a4..8f710f0 100644 > --- a/tests/qapi-schema/flat-union-clash-type.json > +++ b/tests/qapi-schema/flat-union-clash-type.json > @@ -1,8 +1,7 @@ > # 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. > +# Reject this, because we would have a clash in generated C, between the > +# outer tag 'type' and the branch name 'type' within the union. > +# TODO: We could munge the generated C branch name to let it compile. > { 'enum': 'TestEnum', > 'data': [ 'type' ] } > { 'struct': 'Base', > diff --git a/tests/qapi-schema/union-clash-type.err b/tests/qapi-schema/union-clash-type.err > index 6e64d1d..b1de417 100644 > --- a/tests/qapi-schema/union-clash-type.err > +++ b/tests/qapi-schema/union-clash-type.err > @@ -1,16 +1 @@ > -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 > +tests/qapi-schema/union-clash-type.json:7: Union 'TestUnion' member 'kind' clashes with '(automatic)' > diff --git a/tests/qapi-schema/union-clash-type.json b/tests/qapi-schema/union-clash-type.json > index 38330b5..453fd6d 100644 > --- a/tests/qapi-schema/union-clash-type.json > +++ b/tests/qapi-schema/union-clash-type.json > @@ -1,9 +1,8 @@ > # 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. > +# Reject this, because we would have a clash in generated C, between the > +# outer union tag 'kind' and the branch name 'kind' within the union. Suggest "the simple union's implicit tag member 'kind' and" > # 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. > +# Or, we could munge the branch name to allow compilation. > { 'union': 'TestUnion', > 'data': { 'kind': 'int', 'type': 'str' } }
diff --git a/scripts/qapi.py b/scripts/qapi.py index 4b5d574..8d2681b 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -544,7 +544,7 @@ def check_union(expr, expr_info): base = expr.get('base') discriminator = expr.get('discriminator') members = expr['data'] - values = {'MAX': '(automatic)'} + values = {'MAX': '(automatic)', 'KIND': '(automatic)'} # Two types of unions, determined by discriminator. @@ -603,13 +603,19 @@ def check_union(expr, expr_info): " of branch '%s'" % key) # If the discriminator names an enum type, then all members - # of 'data' must also be members of the enum type. + # of 'data' must also be members of the enum type, which in turn + # must not collide with the discriminator name. if enum_define: if key not in enum_define['enum_values']: raise QAPIExprError(expr_info, "Discriminator value '%s' is not found in " "enum '%s'" % (key, enum_define["enum_name"])) + if discriminator in enum_define['enum_values']: + raise QAPIExprError(expr_info, + "Discriminator name '%s' collides with " + "enum value in '%s'" % + (discriminator, enum_define["enum_name"])) # Otherwise, check for conflicts in the generated enum else: diff --git a/tests/qapi-schema/flat-union-clash-type.err b/tests/qapi-schema/flat-union-clash-type.err index 6e64d1d..b44dd40 100644 --- a/tests/qapi-schema/flat-union-clash-type.err +++ b/tests/qapi-schema/flat-union-clash-type.err @@ -1,16 +1 @@ -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 +tests/qapi-schema/flat-union-clash-type.json:11: Discriminator name 'type' collides with enum value in 'TestEnum' diff --git a/tests/qapi-schema/flat-union-clash-type.json b/tests/qapi-schema/flat-union-clash-type.json index eac51a4..8f710f0 100644 --- a/tests/qapi-schema/flat-union-clash-type.json +++ b/tests/qapi-schema/flat-union-clash-type.json @@ -1,8 +1,7 @@ # 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. +# Reject this, because we would have a clash in generated C, between the +# outer tag 'type' and the branch name 'type' within the union. +# TODO: We could munge the generated C branch name to let it compile. { 'enum': 'TestEnum', 'data': [ 'type' ] } { 'struct': 'Base', diff --git a/tests/qapi-schema/union-clash-type.err b/tests/qapi-schema/union-clash-type.err index 6e64d1d..b1de417 100644 --- a/tests/qapi-schema/union-clash-type.err +++ b/tests/qapi-schema/union-clash-type.err @@ -1,16 +1 @@ -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 +tests/qapi-schema/union-clash-type.json:7: Union 'TestUnion' member 'kind' clashes with '(automatic)' diff --git a/tests/qapi-schema/union-clash-type.json b/tests/qapi-schema/union-clash-type.json index 38330b5..453fd6d 100644 --- a/tests/qapi-schema/union-clash-type.json +++ b/tests/qapi-schema/union-clash-type.json @@ -1,9 +1,8 @@ # 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. +# Reject this, because we would have a clash in generated C, between the +# outer union tag 'kind' and the branch name 'kind' within the union. # 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. +# Or, we could munge the branch name to allow compilation. { 'union': 'TestUnion', 'data': { 'kind': 'int', 'type': 'str' } }
The previous commit added two tests that triggered an assertion failure. It's fairly straightforward to avoid the failure by just outright forbidding the collision between a union's tag values and its discriminator name (including the implicit name 'kind' supplied for simple unions [*]). Ultimately, we'd like to move the collision detection into QAPISchema*.check(), but for now it is easier just to enhance the existing checks. [*] Of course, down the road, we have plans to rename the simple union tag name to 'type' to match the QMP wire name, but the idea of the collision will still be present even then. Technically, we could avoid the collision by naming the C union members representing each enum value as '_case_value' rather than 'value'; but until we have an actual qapi client (and not just our testsuite) that has a legitimate reason to match a case label to the name of a QMP key and needs the name munging to satisfy the compiler, it's easier to just reject the qapi as invalid. Signed-off-by: Eric Blake <eblake@redhat.com> --- v7: rebase to testsuite changes in previous patch v6: new patch --- scripts/qapi.py | 10 ++++++++-- tests/qapi-schema/flat-union-clash-type.err | 17 +---------------- tests/qapi-schema/flat-union-clash-type.json | 7 +++---- tests/qapi-schema/union-clash-type.err | 17 +---------------- tests/qapi-schema/union-clash-type.json | 7 +++---- 5 files changed, 16 insertions(+), 42 deletions(-)