Re: [PATCH 03/16] qapi/expr.py: constrain incoming expression types

[John Snow <jsnow@redhat.com>]

On 9/23/20 3:42 PM, Eduardo Habkost wrote:
> On Tue, Sep 22, 2020 at 05:13:00PM -0400, John Snow wrote:
>> mypy does not know the types of values stored in Dicts that masquerade
>> as objects. Help the type checker out by constraining the type.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qapi/expr.py | 25 ++++++++++++++++++++++---
>>   1 file changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index 1872a8a3cc..f6b55a87c1 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -15,9 +15,17 @@
>>   # See the COPYING file in the top-level directory.
>>   
>>   import re
>> +from typing import MutableMapping, Optional
>>   
>>   from .common import c_name
>>   from .error import QAPISemError
>> +from .parser import QAPIDoc
>> +from .source import QAPISourceInfo
>> +
>> +
>> +# Expressions in their raw form are JSON-like structures with arbitrary forms.
>> +# Minimally, their top-level form must be a mapping of strings to values.
>> +Expression = MutableMapping[str, object]
>>   
>>   
>>   # Names must be letters, numbers, -, and _.  They must start with letter,
>> @@ -280,9 +288,20 @@ def check_event(expr, info):
>>   
>>   def check_exprs(exprs):
>>       for expr_elem in exprs:
>> -        expr = expr_elem['expr']
>> -        info = expr_elem['info']
>> -        doc = expr_elem.get('doc')
>> +        # Expression
>> +        assert isinstance(expr_elem['expr'], dict)
>> +        expr: Expression = expr_elem['expr']
>> +        for key in expr.keys():
>> +            assert isinstance(key, str)
> 
> mypy doesn't seem to require the key type asserts, on my testing.
> 

Strictly no. This code is removed somewhere in part 5 when I introduce a 
typed structure to carry this data from the Parser to the Expression 
checker.

(Sometimes, these asserts were for my own sake.)

>> +
>> +        # QAPISourceInfo
>> +        assert isinstance(expr_elem['info'], QAPISourceInfo)
>> +        info: QAPISourceInfo = expr_elem['info']
>> +
>> +        # Optional[QAPIDoc]
>> +        tmp = expr_elem.get('doc')
>> +        assert tmp is None or isinstance(tmp, QAPIDoc)
>> +        doc: Optional[QAPIDoc] = tmp
> 
> Do you need a separate variable here?  This seems to work too:
> 
>          doc = expr_elem.get('doc')
>          assert doc is None or isinstance(doc, QAPIDoc)
> 
> after the assert, mypy will infer the type of doc to be
> Optional[QAPIDoc].
> 

In full honesty, I don't recall... but this code does get replaced by 
the end of this marathon.

> None of this should block a useful 120-patch cleanup series, so:
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 

Thanks!