[PATCH 04/16] qapi/expr.py: Add assertion for union type 'check_dict'

John Snow posted 16 patches 5 years, 4 months ago
There is a newer version of this series
[PATCH 04/16] qapi/expr.py: Add assertion for union type 'check_dict'
Posted by John Snow 5 years, 4 months ago
mypy isn't fond of allowing you to check for bool membership in a
collection of str elements. Guard this lookup for precisely when we were
given a name.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/expr.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index f6b55a87c1..67892502e9 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -166,7 +166,9 @@ def check_type(value, info, source,
         raise QAPISemError(info,
                            "%s should be an object or type name" % source)
 
-    permit_upper = allow_dict in info.pragma.name_case_whitelist
+    permit_upper = False
+    if isinstance(allow_dict, str):
+        permit_upper = allow_dict in info.pragma.name_case_whitelist
 
     # value is a dictionary, check that each member is okay
     for (key, arg) in value.items():
-- 
2.26.2


Re: [PATCH 04/16] qapi/expr.py: Add assertion for union type 'check_dict'
Posted by Eduardo Habkost 5 years, 4 months ago
On Tue, Sep 22, 2020 at 05:13:01PM -0400, John Snow wrote:
> mypy isn't fond of allowing you to check for bool membership in a
> collection of str elements. Guard this lookup for precisely when we were
> given a name.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/expr.py | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index f6b55a87c1..67892502e9 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -166,7 +166,9 @@ def check_type(value, info, source,
>          raise QAPISemError(info,
>                             "%s should be an object or type name" % source)
>  
> -    permit_upper = allow_dict in info.pragma.name_case_whitelist
> +    permit_upper = False
> +    if isinstance(allow_dict, str):
> +        permit_upper = allow_dict in info.pragma.name_case_whitelist

Well, this keeps existing behavior, so:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

But: what exactly is the meaning of allow_dict=False,
allow_dict=True, and allow_dict being a string?


>  
>      # value is a dictionary, check that each member is okay
>      for (key, arg) in value.items():
> -- 
> 2.26.2
> 

-- 
Eduardo


Re: [PATCH 04/16] qapi/expr.py: Add assertion for union type 'check_dict'
Posted by John Snow 5 years, 4 months ago
On 9/23/20 3:53 PM, Eduardo Habkost wrote:
> On Tue, Sep 22, 2020 at 05:13:01PM -0400, John Snow wrote:
>> mypy isn't fond of allowing you to check for bool membership in a
>> collection of str elements. Guard this lookup for precisely when we were
>> given a name.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qapi/expr.py | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index f6b55a87c1..67892502e9 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -166,7 +166,9 @@ def check_type(value, info, source,
>>           raise QAPISemError(info,
>>                              "%s should be an object or type name" % source)
>>   
>> -    permit_upper = allow_dict in info.pragma.name_case_whitelist
>> +    permit_upper = False
>> +    if isinstance(allow_dict, str):
>> +        permit_upper = allow_dict in info.pragma.name_case_whitelist
> 
> Well, this keeps existing behavior, so:
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> But: what exactly is the meaning of allow_dict=False,
> allow_dict=True, and allow_dict being a string?
> 
> 

allow_dict = True -- allows the type to be an object describing the type.

allow_dict: str -- allows the type to be an object (like True), but also 
passes a name in for the purposes of validating the name with the pragma 
whitelist(!)

>>   
>>       # value is a dictionary, check that each member is okay
>>       for (key, arg) in value.items():
>> -- 
>> 2.26.2
>>
> 


Re: [PATCH 04/16] qapi/expr.py: Add assertion for union type 'check_dict'
Posted by Eduardo Habkost 5 years, 4 months ago
On Thu, Sep 24, 2020 at 08:47:31PM -0400, John Snow wrote:
> On 9/23/20 3:53 PM, Eduardo Habkost wrote:
> > On Tue, Sep 22, 2020 at 05:13:01PM -0400, John Snow wrote:
> > > mypy isn't fond of allowing you to check for bool membership in a
> > > collection of str elements. Guard this lookup for precisely when we were
> > > given a name.
> > > 
> > > Signed-off-by: John Snow <jsnow@redhat.com>
> > > ---
> > >   scripts/qapi/expr.py | 4 +++-
> > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> > > index f6b55a87c1..67892502e9 100644
> > > --- a/scripts/qapi/expr.py
> > > +++ b/scripts/qapi/expr.py
> > > @@ -166,7 +166,9 @@ def check_type(value, info, source,
> > >           raise QAPISemError(info,
> > >                              "%s should be an object or type name" % source)
> > > -    permit_upper = allow_dict in info.pragma.name_case_whitelist
> > > +    permit_upper = False
> > > +    if isinstance(allow_dict, str):
> > > +        permit_upper = allow_dict in info.pragma.name_case_whitelist
> > 
> > Well, this keeps existing behavior, so:
> > 
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > 
> > But: what exactly is the meaning of allow_dict=False,
> > allow_dict=True, and allow_dict being a string?
> > 
> > 
> 
> allow_dict = True -- allows the type to be an object describing the type.
> 
> allow_dict: str -- allows the type to be an object (like True), but also
> passes a name in for the purposes of validating the name with the pragma
> whitelist(!)

What.

-- 
Eduardo


Re: [PATCH 04/16] qapi/expr.py: Add assertion for union type 'check_dict'
Posted by John Snow 5 years, 4 months ago
On 9/24/20 9:08 PM, Eduardo Habkost wrote:
> On Thu, Sep 24, 2020 at 08:47:31PM -0400, John Snow wrote:
>> On 9/23/20 3:53 PM, Eduardo Habkost wrote:
>>> On Tue, Sep 22, 2020 at 05:13:01PM -0400, John Snow wrote:
>>>> mypy isn't fond of allowing you to check for bool membership in a
>>>> collection of str elements. Guard this lookup for precisely when we were
>>>> given a name.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>    scripts/qapi/expr.py | 4 +++-
>>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>>>> index f6b55a87c1..67892502e9 100644
>>>> --- a/scripts/qapi/expr.py
>>>> +++ b/scripts/qapi/expr.py
>>>> @@ -166,7 +166,9 @@ def check_type(value, info, source,
>>>>            raise QAPISemError(info,
>>>>                               "%s should be an object or type name" % source)
>>>> -    permit_upper = allow_dict in info.pragma.name_case_whitelist
>>>> +    permit_upper = False
>>>> +    if isinstance(allow_dict, str):
>>>> +        permit_upper = allow_dict in info.pragma.name_case_whitelist
>>>
>>> Well, this keeps existing behavior, so:
>>>
>>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>>>
>>> But: what exactly is the meaning of allow_dict=False,
>>> allow_dict=True, and allow_dict being a string?
>>>
>>>
>>
>> allow_dict = True -- allows the type to be an object describing the type.
>>
>> allow_dict: str -- allows the type to be an object (like True), but also
>> passes a name in for the purposes of validating the name with the pragma
>> whitelist(!)
> 
> What.

(lol)

> 

What's going on here is that when you pass in a name, bool(allow_dict) 
is True -- so we will allow the object being checked here to be a dict.

Also, when you pass in a name, that name is looked up in 
info.pragma.name_case_whitelist to check if the names of the keys in the 
dict being checked (again, allow_dict is implicitly true here) are 
allowed to use uppercase names.

I have some more experimental patches I didn't want to mix in with the 
type checking patches that try to extract pragma checks from this code 
and perform them elsewhere; but that's going to come much, much later.

--js


Re: [PATCH 04/16] qapi/expr.py: Add assertion for union type 'check_dict'
Posted by Cleber Rosa 5 years, 4 months ago
On Tue, Sep 22, 2020 at 05:13:01PM -0400, John Snow wrote:
> mypy isn't fond of allowing you to check for bool membership in a
> collection of str elements. Guard this lookup for precisely when we were
> given a name.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Cleber Rosa <crosa@redhat.com>