Re: [Qemu-devel] [PATCH v5 04/46] qapi: Add tests for empty unions

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> The documentation claims that alternates are useful for
> allowing two types, although nothing enforces this.  Meanwhile,
> it is silent on whether empty unions are allowed.  In practice,
> the generated code will compile, in part because we have a
> 'void *data' branch; but attempting to visit such a type will
> cause an abort().  Add some tests to expose the problems, and

There's nothing really wrong with degenerate alternates or unions, but I
don't want to spend time on making them work.  Outlawing them will do.
The commit message could perhaps be rephrased a bit to better convey
that.

What about empty structs and enums?

> adjust existing tests that should be failing for other reasons.

I had to read this a few times until I understood "should be failing for
other reasons" means something like "are meant to test something else,
but could fail for the wrong reason if we reject degenerate alternates /
unions".

> Signed-off-by: Eric Blake <address@hidden>
> ---
>  tests/Makefile                           | 3 +++
>  tests/qapi-schema/alternate-empty.err    | 0
>  tests/qapi-schema/alternate-empty.exit   | 1 +
>  tests/qapi-schema/alternate-empty.json   | 2 ++
>  tests/qapi-schema/alternate-empty.out    | 4 ++++
>  tests/qapi-schema/alternate-nested.json  | 2 +-
>  tests/qapi-schema/alternate-unknown.json | 2 +-
>  tests/qapi-schema/flat-union-empty.err   | 0
>  tests/qapi-schema/flat-union-empty.exit  | 1 +
>  tests/qapi-schema/flat-union-empty.json  | 4 ++++
>  tests/qapi-schema/flat-union-empty.out   | 7 +++++++
>  tests/qapi-schema/union-empty.err        | 0
>  tests/qapi-schema/union-empty.exit       | 1 +
>  tests/qapi-schema/union-empty.json       | 2 ++
>  tests/qapi-schema/union-empty.out        | 3 +++
>  15 files changed, 30 insertions(+), 2 deletions(-)
>  create mode 100644 tests/qapi-schema/alternate-empty.err
>  create mode 100644 tests/qapi-schema/alternate-empty.exit
>  create mode 100644 tests/qapi-schema/alternate-empty.json
>  create mode 100644 tests/qapi-schema/alternate-empty.out
>  create mode 100644 tests/qapi-schema/flat-union-empty.err
>  create mode 100644 tests/qapi-schema/flat-union-empty.exit
>  create mode 100644 tests/qapi-schema/flat-union-empty.json
>  create mode 100644 tests/qapi-schema/flat-union-empty.out
>  create mode 100644 tests/qapi-schema/union-empty.err
>  create mode 100644 tests/qapi-schema/union-empty.exit
>  create mode 100644 tests/qapi-schema/union-empty.json
>  create mode 100644 tests/qapi-schema/union-empty.out
>
> diff --git a/tests/Makefile b/tests/Makefile
> index 97434f6..df16c9c 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -226,6 +226,7 @@ qapi-schema += alternate-base.json
>  qapi-schema += alternate-clash.json
>  qapi-schema += alternate-conflict-dict.json
>  qapi-schema += alternate-conflict-string.json
> +qapi-schema += alternate-empty.json
>  qapi-schema += alternate-good.json
>  qapi-schema += alternate-nested.json
>  qapi-schema += alternate-unknown.json
> @@ -277,6 +278,7 @@ qapi-schema += flat-union-base-union.json
>  qapi-schema += flat-union-branch-clash.json
>  qapi-schema += flat-union-branch-clash2.json
>  qapi-schema += flat-union-cycle.json
> +qapi-schema += flat-union-empty.json
>  qapi-schema += flat-union-inline.json
>  qapi-schema += flat-union-int-branch.json
>  qapi-schema += flat-union-invalid-branch-key.json
> @@ -334,6 +336,7 @@ qapi-schema += union-bad-branch.json
>  qapi-schema += union-base-no-discriminator.json
>  qapi-schema += union-clash.json
>  qapi-schema += union-clash2.json
> +qapi-schema += union-empty.json
>  qapi-schema += union-invalid-base.json
>  qapi-schema += union-max.json
>  qapi-schema += union-optional-branch.json
> diff --git a/tests/qapi-schema/alternate-empty.err 
> b/tests/qapi-schema/alternate-empty.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/alternate-empty.exit 
> b/tests/qapi-schema/alternate-empty.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/alternate-empty.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/alternate-empty.json 
> b/tests/qapi-schema/alternate-empty.json
> new file mode 100644
> index 0000000..db3820f
> --- /dev/null
> +++ b/tests/qapi-schema/alternate-empty.json
> @@ -0,0 +1,2 @@
> +# FIXME - alternates should list at least two types to be useful
> +{ 'alternate': 'Alt', 'data': { 'i': 'int' } }
> diff --git a/tests/qapi-schema/alternate-empty.out 
> b/tests/qapi-schema/alternate-empty.out
> new file mode 100644
> index 0000000..0f153b6
> --- /dev/null
> +++ b/tests/qapi-schema/alternate-empty.out
> @@ -0,0 +1,4 @@
> +object :empty
> +alternate Alt
> +    case i: int
> +enum AltKind ['i']
> diff --git a/tests/qapi-schema/alternate-nested.json 
> b/tests/qapi-schema/alternate-nested.json
> index c4233b9..8e22186 100644
> --- a/tests/qapi-schema/alternate-nested.json
> +++ b/tests/qapi-schema/alternate-nested.json
> @@ -2,4 +2,4 @@
>  { 'alternate': 'Alt1',
>    'data': { 'name': 'str', 'value': 'int' } }
>  { 'alternate': 'Alt2',
> -  'data': { 'nested': 'Alt1' } }
> +  'data': { 'nested': 'Alt1', 'b': 'bool' } }
> diff --git a/tests/qapi-schema/alternate-unknown.json 
> b/tests/qapi-schema/alternate-unknown.json
> index ad5c103..08c80dc 100644
> --- a/tests/qapi-schema/alternate-unknown.json
> +++ b/tests/qapi-schema/alternate-unknown.json
> @@ -1,3 +1,3 @@
>  # we reject an alternate with unknown type in branch
>  { 'alternate': 'Alt',
> -  'data': { 'unknown': 'MissingType' } }
> +  'data': { 'unknown': 'MissingType', 'i': 'int' } }
> diff --git a/tests/qapi-schema/flat-union-empty.err 
> b/tests/qapi-schema/flat-union-empty.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/flat-union-empty.exit 
> b/tests/qapi-schema/flat-union-empty.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-empty.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/flat-union-empty.json 
> b/tests/qapi-schema/flat-union-empty.json
> new file mode 100644
> index 0000000..67dd297
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-empty.json
> @@ -0,0 +1,4 @@
> +# FIXME - flat unions should not be empty
> +{ 'enum': 'Empty', 'data': [ ] }
> +{ 'struct': 'Base', 'data': { 'type': 'Empty' } }
> +{ 'union': 'Union', 'base': 'Base', 'discriminator': 'type', 'data': { } }

Here you can see how empty flat unions are connected to empty enums.

> diff --git a/tests/qapi-schema/flat-union-empty.out 
> b/tests/qapi-schema/flat-union-empty.out
> new file mode 100644
> index 0000000..0e0665a
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-empty.out
> @@ -0,0 +1,7 @@
> +object :empty
> +object Base
> +    member type: Empty optional=False
> +enum Empty []
> +object Union
> +    base Base
> +    tag type
> diff --git a/tests/qapi-schema/union-empty.err 
> b/tests/qapi-schema/union-empty.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/union-empty.exit 
> b/tests/qapi-schema/union-empty.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/union-empty.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/union-empty.json 
> b/tests/qapi-schema/union-empty.json
> new file mode 100644
> index 0000000..1785007
> --- /dev/null
> +++ b/tests/qapi-schema/union-empty.json
> @@ -0,0 +1,2 @@
> +# FIXME - unions should not be empty
> +{ 'union': 'Union', 'data': { } }
> diff --git a/tests/qapi-schema/union-empty.out 
> b/tests/qapi-schema/union-empty.out
> new file mode 100644
> index 0000000..8b5a7bf
> --- /dev/null
> +++ b/tests/qapi-schema/union-empty.out
> @@ -0,0 +1,3 @@
> +object :empty
> +object Union
> +enum UnionKind []