Re: [PATCH v4 05/19] qapi/expr.py: constrain incoming expression types

[John Snow <jsnow@redhat.com>]

On 3/26/21 1:40 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 3/25/21 10:04 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> 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 | 26 +++++++++++++++++++++++---
>>>>    1 file changed, 23 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>>>> index b4bbcd54c0..b75c85c160 100644
>>>> --- a/scripts/qapi/expr.py
>>>> +++ b/scripts/qapi/expr.py
>>>> @@ -15,9 +15,18 @@
>>>>    # See the COPYING file in the top-level directory.
>>>>    
>>>>    import re
>>>> +from typing import Dict, Optional
>>>>    
>>>>    from .common import c_name
>>>>    from .error import QAPISemError
>>>> +from .parser import QAPIDoc
>>>> +from .source import QAPISourceInfo
>>>> +
>>>> +
>>>> +# Deserialized JSON objects as returned by the parser;
>>>> +# The values of this mapping are not necessary to exhaustively type
>>>
>>> Not necessary and also not practical with current mypy.  Correct?
>>
>> Neither necessary nor practical. Typing as 'object' guarantees that
>> these values will never be used in a manner not supported by all python
>> objects. Mypy does not complain, so we know that we don't misuse the type.
>>
>> This is helpful for proving the validity of the expr.py validator
>> itself: we know that we are not forgetting to perform type narrowing and
>> using the value contained therein inappropriately.
>>
>> Adding a more exhaustive typing here is impractical (for reasons we
>> learned during introspect.py), but also provides no benefit to the
>> static analysis here anyway.
>>
>> (None of the functions written here *assume* the shape of the structure,
>> so there are no functions that benefit from having a more laboriously
>> specified type.)
>>
>> If the comment needs more work, suggest away -- I tried to follow our
>> last discussion here as best as I was able.
> 
> "Needs more work" sounds like "inadequate", which isn't the case.
> 
> The comment focuses on what we need from mypy here.  We may or may not
> want to hint at the other aspect: what mypy can provide.
> 
>>>> +# here, because the purpose of this module is to interrogate that type.
>>>> +_JSONObject = Dict[str, object]
> [...]
> 
> If we want to, maybe:
> 
>       # Deserialized JSON objects as returned by the parser.
>       # The values of this mapping are not necessary to exhaustively type
>       # here (and also not practical as long as mypy lacks recursive
>       # types), because the purpose of this module is to interrogate that
>       # type.
> 
> Thoughts?
> 

If it occurs to you to want the extra explanation, I won't say no to it. 
I will fold it in.

--js