Eric Blake <ebl...@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 <ebl...@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' } }
