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

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(+)

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.

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

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.