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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> 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']
> }

Definitely a bug fix.

The commit message should say so.  It's not just for mypy's sake.

> 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?

I prefer the latter, because then bug fix patch nicely illustrates
what's being fixed.  Preference, not demand.