Re: [PATCH v3 06/16] qapi/expr.py: Check type of 'data' member

[John Snow]

On 2/24/21 5:39 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
> > Iterating over the members of data isn't going to work if it's not some
> > form of dict anyway, but for the sake of mypy, formalize it.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >   scripts/qapi/expr.py | 7 +++++++
> >   1 file changed, 7 insertions(+)
> >
> > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> > index c97e8ce8a4d..afa6bd07769 100644
> > --- a/scripts/qapi/expr.py
> > +++ b/scripts/qapi/expr.py
> > @@ -254,6 +254,9 @@ def check_union(expr, info):
> >               raise QAPISemError(info, "'discriminator' requires 'base'")
> >           check_name_is_str(discriminator, info, "'discriminator'")
> >   
> > +    if not isinstance(members, dict):
> > +        raise QAPISemError(info, "'data' must be an object")
> > +
> >       for (key, value) in members.items():
> >           source = "'data' member '%s'" % key
> >           check_name_str(key, info, source)
> > @@ -267,6 +270,10 @@ def check_alternate(expr, info):
> >   
> >       if not members:
> >           raise QAPISemError(info, "'data' must not be empty")
> > +
> > +    if not isinstance(members, dict):
> > +        raise QAPISemError(info, "'data' must be an object")
> > +
> >       for (key, value) in members.items():
> >           source = "'data' member '%s'" % key
> >           check_name_str(key, info, source)
>
> All errors require a negative test.
>
> If an error is unreachable, it should be an assertion instead.
>
> If these new errors are reachable, the commit might be a bug fix.

You can, yes:

Traceback (most recent call last):
  File "/home/jsnow/src/qemu/scripts/qapi-gen.py", line 19, in <module>
    sys.exit(main.main())
  File "/home/jsnow/src/qemu/scripts/qapi/main.py", line 89, in main
    generate(args.schema,
  File "/home/jsnow/src/qemu/scripts/qapi/main.py", line 51, in generate
    schema = QAPISchema(schema_file)
  File "/home/jsnow/src/qemu/scripts/qapi/schema.py", line 853, in __init__
    exprs = check_exprs(parser.exprs)
  File "/home/jsnow/src/qemu/scripts/qapi/expr.py", line 337, in 
check_exprs
    check_union(expr, info)
  File "/home/jsnow/src/qemu/scripts/qapi/expr.py", line 248, in 
check_union
    for (key, value) in members.items():
AttributeError: 'list' object has no attribute 'items'


from this beauty:

##
# @Foo:
#
# @id: identifier of the backend
#
# @driver: the backend driver to use
#
# @timer-period: timer period (in microseconds, 0: use lowest possible)
#
# Since: 4.0
##
{ 'union': 'Foo',
  'base': {
    'id':            'str',
    'driver':        'AudiodevDriver',
    '*timer-period': 'uint32' },
  'discriminator': 'driver',
  'data': ['hello', 'world']
}


I'll add some new regression tests for you. Do you want them squished in 
with this commit, or would you like it done kind of independently, at 
the beginning of this series instead?

--js