Re: [Qemu-devel] [PATCH v5 06/28] qapi: Add some union tests

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Demonstrate that the qapi generator doesn't deal well with unions
> that aren't up to par. Later patches will update the expected
> reseults as the generator is made stricter.
>
> Of particular note, we currently allow 'base' without 'discriminator'
> as a way to create a simple union with a base class.  However, none
> of the existing QMP or QGA interfaces use it (it is exercised only
> in the testsuite).

qapi-code-gen.txt documents 'base' only for flat unions.  We should
either document its use in simple unions, or outlaw it.

>                     Meanwhile, it would be nice to allow
> 'discriminator':'EnumType' without 'base' for creating a simple union
> that is type-safe rather than open-coded to a generated enum (right
> now, we are only type-safe when using a flat union, but that uses
> a different syntax of 'discriminator':'member-name' which requires
> a base class containing a 'member-name' enum field).

I'm afraid I don't get you.  Can you give examples?

>                                                       If both 'base'
> and 'discriminator' are optional, then converting a simple union
> with base class to a type-safe simple union with an enum discriminator
> would not be possible.  So my plan is to get rid of 'base' without
> 'discriminator' later in the series;

Aha: you're going to outlaw 'base' in simple unions.  Yes, please.

>                                      this will be no real loss, as any
> union that needs additional common fields can always use a flat
> union.

The mathematical concept behind unions is the sum type.

We have three implementations, and we call them simple, flat, anonymous.

Anonymous unions are implicitly tagged.  They come with the obvious
restrictions required to make implicit tagging work.

The other two are explicitly tagged.  The difference between them is
just notation.  I like my unions flat, because for me the extra wrapping
is just notational overhead.

In particular, I can define simple unions in terms of flat ones by
restricting all union cases to a single member named 'data'.  They're
not implemented that way, but that's a historical accident.  Simple
unions are a redundant concept.

This proves your "can always use a flat union" proposition.  The only
way they can earn their keep is making the schema easier to read.  I
haven't thought about that.

Another historical accident is how we express members common to all
union cases: base type.  Probably just because complex types already had
them.  Are you planning to change anything there?

[...]
> diff --git a/tests/qapi-schema/alternate-nested.err 
> b/tests/qapi-schema/alternate-nested.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/alternate-nested.exit 
> b/tests/qapi-schema/alternate-nested.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/alternate-nested.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/alternate-nested.json 
> b/tests/qapi-schema/alternate-nested.json
> new file mode 100644
> index 0000000..d5812bf
> --- /dev/null
> +++ b/tests/qapi-schema/alternate-nested.json
> @@ -0,0 +1,7 @@
> +# FIXME: we should reject a nested anonymous union branch

Same reason we want to reject nested / anonymous complex types
elsewhere?  Or is there a deeper reason?

> +{ 'union': 'Union1',
> +  'discriminator': {},
> +  'data': { 'name': 'str', 'value': 'int' } }
> +{ 'union': 'Union2',
> +  'discriminator': {},
> +  'data': { 'nested': 'Union1' } }
> diff --git a/tests/qapi-schema/alternate-nested.out 
> b/tests/qapi-schema/alternate-nested.out
> new file mode 100644
> index 0000000..0137c1f
> --- /dev/null
> +++ b/tests/qapi-schema/alternate-nested.out
> @@ -0,0 +1,5 @@
> +[OrderedDict([('union', 'Union1'), ('discriminator', OrderedDict()), 
> ('data', OrderedDict([('name', 'str'), ('value', 'int')]))]),
> + OrderedDict([('union', 'Union2'), ('discriminator', OrderedDict()), 
> ('data', OrderedDict([('nested', 'Union1')]))])]
> +[{'enum_name': 'Union1Kind', 'enum_values': None},
> + {'enum_name': 'Union2Kind', 'enum_values': None}]
> +[]
[...]
> diff --git a/tests/qapi-schema/flat-union-bad-base.err 
> b/tests/qapi-schema/flat-union-bad-base.err
> new file mode 100644
> index 0000000..5962ff4
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-bad-base.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/flat-union-bad-base.json:9: Base 'OrderedDict([('enum1', 
> 'TestEnum'), ('kind', 'str')])' is not a valid type
> diff --git a/tests/qapi-schema/flat-union-bad-base.exit 
> b/tests/qapi-schema/flat-union-bad-base.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-bad-base.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/flat-union-bad-base.json 
> b/tests/qapi-schema/flat-union-bad-base.json
> new file mode 100644
> index 0000000..d69168f
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-bad-base.json
> @@ -0,0 +1,13 @@
> +# we require the base to be an existing complex type
> +# FIXME: should we allow an anonymous inline base type?

What do you mean by "anonymous inline base type"?

> +{ 'enum': 'TestEnum',
> +  'data': [ 'value1', 'value2' ] }
> +{ 'type': 'TestTypeA',
> +  'data': { 'string': 'str' } }
> +{ 'type': 'TestTypeB',
> +  'data': { 'integer': 'int' } }
> +{ 'union': 'TestUnion',
> +  'base': { 'enum1': 'TestEnum', 'kind': 'str' },
> +  'discriminator': 'TestEnum',
> +  'data': { 'kind1': 'TestTypeA',
> +            'kind2': 'TestTypeB' } }
> diff --git a/tests/qapi-schema/flat-union-bad-base.out 
> b/tests/qapi-schema/flat-union-bad-base.out
> new file mode 100644
> index 0000000..e69de29
[...]

"Doesn't deal well" is an understatement :)

Since all my questions are about your intentions and rationale:

Reviewed-by: Markus Armbruster <address@hidden>