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>
>
> ---
> v6: new patch
> ---
> scripts/qapi.py | 10 ++++++++--
> tests/qapi-schema/flat-union-clash-type.err | 17 +----------------
> tests/qapi-schema/union-clash-type.err | 17 +----------------
> 3 files changed, 10 insertions(+), 34 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 7e8a99d..ec450a6 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..879beb8 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:8: Discriminator name 'type'
> collides with enum value in 'TestEnum'
> diff --git a/tests/qapi-schema/union-clash-type.err
> b/tests/qapi-schema/union-clash-type.err
> index 6e64d1d..773900b 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:2: Union 'TestUnion' member 'kind'
> clashes with '(automatic)'
Preexisting: this error message sucks. But improving it isn't this
patch's job.
