Re: [Qemu-devel] [PATCH v7 06/18] qapi: Avoid assertion failure on union 'type' collision

[Markus Armbruster]

Eric Blake <address@hidden> 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 <address@hidden>
>
> ---
> 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' } }