Re: [Qemu-devel] [PATCH v5 08/28] qapi: Better error messages for bad unions

[Markus Armbruster <armbru@redhat.com>]

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 <eblake@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 <eblake@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 <armbru@redhat.com>