Subject suggests you're just polishing error messages here. In fact,
you're fixing the generator to detect errors. Suggest something like
qapi: Tighten checking of unions
Eric Blake <ebl...@redhat.com> writes:
> Previous commits demonstrated that the generator had several
> flaws with less-than-perfect unions:
> - make the use of a base without discriminator a hard error,
> since the previous patch removed all remaining uses of it
> - a simple union that listed the same branch twice (or two variant
> names that map to the same C enumerator, including the implicit
> MAX sentinel) ended up generating invalid C code
> - checking 'if discriminator' prior to 'if discriminator == {}'
> leads to dead code in python, and ended up processing anonymous
> unions as if they were simple unions
> - an anonymous union that listed two branches with the same qtype
> ended up generating invalid C code
> - the generator crashed on anonymous union attempts to use an
> array type
> - the generator was silently ignoring a base type for anonymous
> unions
> - the generator allowed unknown types or nested anonymous unions
> as a branch in an anonymous union
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> ---
[...]
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index f6fb930..2390887 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -181,17 +181,8 @@ const int %(name)s_qtypes[QTYPE_MAX] = {
> name=name)
>
> for key in members:
> - qapi_type = members[key]
> - if builtin_types.has_key(qapi_type):
> - qtype = builtin_types[qapi_type]
> - elif find_struct(qapi_type):
> - qtype = "QTYPE_QDICT"
> - elif find_union(qapi_type):
> - qtype = "QTYPE_QDICT"
> - elif find_enum(qapi_type):
> - qtype = "QTYPE_QSTRING"
> - else:
> - assert False, "Invalid anonymous union member"
> + qtype = find_anonymous_member_qtype(members[key])
> + assert qtype, "Invalid anonymous union member"
>
> ret += mcgen('''
> [ %(qtype)s ] = %(abbrev)s_KIND_%(enum)s,
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 3ce8c33..fc7b7f1 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -224,6 +224,23 @@ def find_base_fields(base):
> return None
> return base_struct_define['data']
>
> +# Return the qtype of an anonymous union branch, or None on error.
> +def find_anonymous_member_qtype(qapi_type):
> + if builtin_types.has_key(qapi_type):
> + return builtin_types[qapi_type]
> + elif find_struct(qapi_type):
> + return "QTYPE_QDICT"
> + elif find_enum(qapi_type):
> + return "QTYPE_QSTRING"
> + else:
> + union = find_union(qapi_type)
> + if union:
> + discriminator = union.get('discriminator')
> + if discriminator == {}:
> + return None
> + return "QTYPE_QDICT"
> + return None
> +
> # Return the discriminator enum define if discriminator is specified as an
> # enum type, otherwise return None.
> def discriminator_find_enum_define(expr):
> @@ -258,24 +275,28 @@ def check_union(expr, expr_info):
> base = expr.get('base')
> discriminator = expr.get('discriminator')
> members = expr['data']
> + values = { 'MAX': '(automatic)' }
> + types_seen = {}
>
> - # If the object has a member 'base', its value must name a complex type.
> - if base:
> - base_fields = find_base_fields(base)
> - if not base_fields:
> - raise QAPIExprError(expr_info,
> - "Base '%s' is not a valid type"
> - % base)
> + # Three types of unions, determined by discriminator.
>
> - # If the union object has no member 'discriminator', it's an
> - # ordinary union.
> - if not discriminator:
> + # If the value of member 'discriminator' is {}, it's an
> + # anonymous union, and must not have a base.
> + if discriminator == {}:
> enum_define = None
> + if base:
> + raise QAPIExprError(expr_info,
> + "Anonymous union '%s' must not have a base"
> + % name)
>
> - # Else if the value of member 'discriminator' is {}, it's an
> - # anonymous union.
> - elif discriminator == {}:
> + # Else if the union object has no member 'discriminator', it's an
> + # ordinary union. For now, it must not have a base.
Since you're touching this anyway, you could s/ordinary/simple/.
> + elif not discriminator:
> enum_define = None
> + if base:
> + raise QAPIExprError(expr_info,
> + "Simple union '%s' must not have a base"
> + % name)
>
> # Else, it's a flat union.
> else:
> @@ -284,6 +305,12 @@ def check_union(expr, expr_info):
> raise QAPIExprError(expr_info,
> "Flat union '%s' must have a base field"
> % name)
> + base_fields = find_base_fields(base)
> + if not base_fields:
> + raise QAPIExprError(expr_info,
> + "Base '%s' is not a valid type"
> + % base)
> +
> # The value of member 'discriminator' must name a member of the
> # base type.
> discriminator_type = base_fields.get(discriminator)
> @@ -301,15 +328,42 @@ def check_union(expr, expr_info):
>
> # Check every branch
> for (key, value) in members.items():
> - # If this named member's value names an enum type, then all members
> + # If the discriminator names an enum type, then all members
> # of 'data' must also be members of the enum type.
> - if enum_define and not key in enum_define['enum_values']:
> - raise QAPIExprError(expr_info,
> - "Discriminator value '%s' is not found in "
> - "enum '%s'" %
> - (key, enum_define["enum_name"]))
> - # Todo: add checking for values. Key is checked as above, value can
> be
> - # also checked here, but we need more functions to handle array case.
> + if enum_define:
> + if not key in enum_define['enum_values']:
> + raise QAPIExprError(expr_info,
> + "Discriminator value '%s' is not found
> in "
> + "enum '%s'" %
> + (key, enum_define["enum_name"]))
> +
> + # Otherwise, check for conflicts in the generated enum
> + else:
> + c_key = _generate_enum_string(key)
> + if c_key in values:
> + raise QAPIExprError(expr_info,
> + "Union '%s' member '%s' clashes with
> '%s'"
> + % (name, key, values[c_key]))
> + values[c_key] = key
> +
> + # Ensure anonymous unions have no type conflicts.
> + if discriminator == {}:
> + if isinstance(value, list):
> + raise QAPIExprError(expr_info,
> + "Anonymous union '%s' member '%s' must "
> + "not be array type" % (name, key))
> + qtype = find_anonymous_member_qtype(value)
> + if not qtype:
> + raise QAPIExprError(expr_info,
> + "Anonymous union '%s' member '%s' has "
> + "invalid type '%s'" % (name, key, value))
> + if qtype in types_seen:
> + raise QAPIExprError(expr_info,
> + "Anonymous union '%s' member '%s' has "
> + "same QObject type as member '%s'"
> + % (name, key, types_seen[qtype]))
> + types_seen[qtype] = key
> +
>
> def check_enum(expr, expr_info):
> name = expr['enum']
[...]
> diff --git a/tests/qapi-schema/alternate-conflict-string.err
> b/tests/qapi-schema/alternate-conflict-string.err
> index e69de29..a3b7b6d 100644
> --- a/tests/qapi-schema/alternate-conflict-string.err
> +++ b/tests/qapi-schema/alternate-conflict-string.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/alternate-conflict-string.json:4: Anonymous union
> 'MyUnion' member 'two' has same QObject type as member 'one'
Explains the problem in terms of the implementation. That's okay for a
development tool, but what about "Anonymous union 'MyUnion' member 'two'
can't be distinguished from member 'one'"?
> diff --git a/tests/qapi-schema/alternate-conflict-string.exit
> b/tests/qapi-schema/alternate-conflict-string.exit
> index 573541a..d00491f 100644
> --- a/tests/qapi-schema/alternate-conflict-string.exit
> +++ b/tests/qapi-schema/alternate-conflict-string.exit
> @@ -1 +1 @@
> -0
> +1
> diff --git a/tests/qapi-schema/alternate-conflict-string.json
> b/tests/qapi-schema/alternate-conflict-string.json
> index 5fd1a47..a1b0bea 100644
> --- a/tests/qapi-schema/alternate-conflict-string.json
> +++ b/tests/qapi-schema/alternate-conflict-string.json
> @@ -1,4 +1,4 @@
> -# FIXME: we should reject anonymous unions with multiple string-like branches
> +# we reject anonymous unions with multiple string-like branches
> { 'enum': 'Enum',
> 'data': [ 'hello', 'world' ] }
> { 'union': 'MyUnion',
> diff --git a/tests/qapi-schema/alternate-conflict-string.out
> b/tests/qapi-schema/alternate-conflict-string.out
> index e7b39a2..e69de29 100644
> --- a/tests/qapi-schema/alternate-conflict-string.out
> +++ b/tests/qapi-schema/alternate-conflict-string.out
> @@ -1,5 +0,0 @@
> -[OrderedDict([('enum', 'Enum'), ('data', ['hello', 'world'])]),
> - OrderedDict([('union', 'MyUnion'), ('discriminator', OrderedDict()),
> ('data', OrderedDict([('one', 'str'), ('two', 'Enum')]))])]
> -[{'enum_name': 'Enum', 'enum_values': ['hello', 'world']},
> - {'enum_name': 'MyUnionKind', 'enum_values': None}]
> -[]
[...]
> diff --git a/tests/qapi-schema/union-base-no-discriminator.err
> b/tests/qapi-schema/union-base-no-discriminator.err
> index e69de29..f33392d 100644
> --- a/tests/qapi-schema/union-base-no-discriminator.err
> +++ b/tests/qapi-schema/union-base-no-discriminator.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/union-base-no-discriminator.json:12: Simple union
> 'TestUnion' must not have a base
> diff --git a/tests/qapi-schema/union-base-no-discriminator.exit
> b/tests/qapi-schema/union-base-no-discriminator.exit
> index 573541a..d00491f 100644
> --- a/tests/qapi-schema/union-base-no-discriminator.exit
> +++ b/tests/qapi-schema/union-base-no-discriminator.exit
> @@ -1 +1 @@
> -0
> +1
> diff --git a/tests/qapi-schema/union-base-no-discriminator.json
> b/tests/qapi-schema/union-base-no-discriminator.json
> index b5da546..548a633 100644
> --- a/tests/qapi-schema/union-base-no-discriminator.json
> +++ b/tests/qapi-schema/union-base-no-discriminator.json
> @@ -1,4 +1,5 @@
> -# FIXME: either allow base in non-flat unions, or diagnose missing
> discriminator
> +# for now, we reject a base for non-flat unions
> +# FIXME: might be a useful extension to allow later
This isn't a FIXME, it's an idea :)
> { 'type': 'TestTypeA',
> 'data': { 'string': 'str' } }
>
> diff --git a/tests/qapi-schema/union-base-no-discriminator.out
> b/tests/qapi-schema/union-base-no-discriminator.out
> index 505fd57..e69de29 100644
> --- a/tests/qapi-schema/union-base-no-discriminator.out
> +++ b/tests/qapi-schema/union-base-no-discriminator.out
> @@ -1,8 +0,0 @@
> -[OrderedDict([('type', 'TestTypeA'), ('data', OrderedDict([('string',
> 'str')]))]),
> - OrderedDict([('type', 'TestTypeB'), ('data', OrderedDict([('integer',
> 'int')]))]),
> - OrderedDict([('type', 'Base'), ('data', OrderedDict([('string', 'str')]))]),
> - OrderedDict([('union', 'TestUnion'), ('base', 'Base'), ('data',
> OrderedDict([('value1', 'TestTypeA'), ('value2', 'TestTypeB')]))])]
> -[{'enum_name': 'TestUnionKind', 'enum_values': None}]
> -[OrderedDict([('type', 'TestTypeA'), ('data', OrderedDict([('string',
> 'str')]))]),
> - OrderedDict([('type', 'TestTypeB'), ('data', OrderedDict([('integer',
> 'int')]))]),
> - OrderedDict([('type', 'Base'), ('data', OrderedDict([('string', 'str')]))])]
> diff --git a/tests/qapi-schema/union-max.err b/tests/qapi-schema/union-max.err
> index e69de29..55ce439 100644
> --- a/tests/qapi-schema/union-max.err
> +++ b/tests/qapi-schema/union-max.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/union-max.json:2: Union 'Union' member 'max' clashes with
> '(automatic)'
Cryptic error message, but it'll do.
> diff --git a/tests/qapi-schema/union-max.exit
> b/tests/qapi-schema/union-max.exit
> index 573541a..d00491f 100644
> --- a/tests/qapi-schema/union-max.exit
> +++ b/tests/qapi-schema/union-max.exit
> @@ -1 +1 @@
> -0
> +1
> diff --git a/tests/qapi-schema/union-max.json
> b/tests/qapi-schema/union-max.json
> index 45648c4..d6ad986 100644
> --- a/tests/qapi-schema/union-max.json
> +++ b/tests/qapi-schema/union-max.json
> @@ -1,3 +1,3 @@
> -# FIXME: we should reject 'max' branch in a union, for collision with C enum
> +# we reject 'max' branch in a union, for collision with C enum
> { 'union': 'Union',
> 'data': { 'max': 'int' } }
> diff --git a/tests/qapi-schema/union-max.out b/tests/qapi-schema/union-max.out
> index 2757d36..e69de29 100644
> --- a/tests/qapi-schema/union-max.out
> +++ b/tests/qapi-schema/union-max.out
> @@ -1,3 +0,0 @@
> -[OrderedDict([('union', 'Union'), ('data', OrderedDict([('max', 'int')]))])]
> -[{'enum_name': 'UnionKind', 'enum_values': None}]
> -[]
Feel free to address my comments on top.
Reviewed-by: Markus Armbruster <arm...@redhat.com>
