Re: [PATCH 16/37] qapi: establish mypy type-checking baseline

[John Snow <jsnow@redhat.com>]

On 9/21/20 4:05 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 9/18/20 7:55 AM, Markus Armbruster wrote:
>>> Ignorant question: why does this come after PATCH 13 "qapi/common.py:
>>> add notational type hints", but before all the other patches adding type
>>> hints?
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> Fix two very minor issues, and then establish a mypy type-checking
>>>> baseline.
>>>>
>>>> Like pylint, this should be run from the folder above:
>>>>
>>>>    > mypy --config-file=qapi/mypy.ini qapi/
>>> I get:
>>>       $ mypy --config-file qapi/mypy.ini qapi
>>>       qapi/mypy.ini: [mypy]: Strict mode is not supported in configuration files: specify individual flags instead (see 'mypy -h' for the list of flags enabled in strict mode)
>>>       qapi/types.py:29: error: Need type annotation for 'objects_seen' (hint: "objects_seen: Set[<type>] = ...")
>>>       Found 1 error in 1 file (checked 18 source files)
>>> Is this expected?
>>>
>>
>> Nope.
>>
>>> In case it matters:
>>>       $ mypy --version
>>>       mypy 0.761
>>>
>>
>> I am using mypy 0.782.
>>
>> I will investigate to see if there is an *easy* win to allow older
>> versions to work.
>>
>> In the meantime, please consider trying this:
>>
>> pip install --user mypy==0.782
>> ~/.local/bin/mypy --config-file=qapi/mypy.ini qapi/
> 
> I'll consider dragging my feet until upgrading Fedora gives it to me for
> free.
> 
> The less I interact with package managers, the happier I am.
> 

I'll probably be working on making sure CI runs in a virtual environment 
with specifically pinned versions.

We don't need mypy to run or build, so I have been less worried about 
requiring bleeding edge versions for development and regression testing.

>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>    scripts/qapi/doc.py    |  3 +-
>>>>    scripts/qapi/mypy.ini  | 65 ++++++++++++++++++++++++++++++++++++++++++
>>>>    scripts/qapi/schema.py |  3 +-
>>>>    3 files changed, 69 insertions(+), 2 deletions(-)
>>>>    create mode 100644 scripts/qapi/mypy.ini
>>>>
>>>> diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py
>>>> index cbf7076ed9..70f7cdfaa6 100644
>>>> --- a/scripts/qapi/doc.py
>>>> +++ b/scripts/qapi/doc.py
>>>> @@ -5,7 +5,8 @@
>>>>    """This script produces the documentation of a qapi schema in texinfo format"""
>>>>      import re
>>>> -from .gen import QAPIGenDoc, QAPISchemaVisitor
>>>> +from .gen import QAPIGenDoc
>>>> +from .schema import QAPISchemaVisitor
>>> Your mypy doesn't like such lazy imports?  Mine seems not to care.
>>>
>>
>> Yeah, it specifically complained that no such definition existed in
>> that file.
> 
> I sense a certain wobbliness in mypy.  Perhaps to be expected from a
> tool with major version zero.  There's a risk that developers' local
> "make check" and our gating CI differ too much.  We'll see.
> 

It is indeed very cutting edge. It wasn't really viable to use until 
Python 3.6.

>>>>      
>>>>    MSG_FMT = """
>>>> diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
>>>> new file mode 100644
>>>> index 0000000000..a0f2365a53
>>>> --- /dev/null
>>>> +++ b/scripts/qapi/mypy.ini
>>>> @@ -0,0 +1,65 @@
>>>> +[mypy]
>>>> +strict = True
>>>> +strict_optional = False
>>>> +disallow_untyped_calls = False
>>>> +python_version = 3.6
>>>> +
>>>> +[mypy-qapi.commands]
>>>> +disallow_untyped_defs = False
>>>> +disallow_incomplete_defs = False
>>>> +check_untyped_defs = False
>>>> +
>>>> +[mypy-qapi.doc]
>>>> +disallow_subclassing_any = False
>>>> +disallow_untyped_defs = False
>>>> +disallow_incomplete_defs = False
>>>> +
>>>> +[mypy-qapi.error]
>>>> +disallow_untyped_defs = False
>>>> +disallow_incomplete_defs = False
>>>> +check_untyped_defs = False
>>>> +
>>>> +[mypy-qapi.events]
>>>> +disallow_untyped_defs = False
>>>> +disallow_incomplete_defs = False
>>>> +check_untyped_defs = False
>>>> +
>>>> +[mypy-qapi.expr]
>>>> +disallow_untyped_defs = False
>>>> +disallow_incomplete_defs = False
>>>> +check_untyped_defs = False
>>>> +
>>>> +[mypy-qapi.gen]
>>>> +disallow_untyped_defs = False
>>>> +disallow_incomplete_defs = False
>>>> +check_untyped_defs = False
>>>> +
>>>> +[mypy-qapi.introspect]
>>>> +disallow_untyped_defs = False
>>>> +disallow_incomplete_defs = False
>>>> +check_untyped_defs = False
>>>> +
>>>> +[mypy-qapi.parser]
>>>> +disallow_untyped_defs = False
>>>> +disallow_incomplete_defs = False
>>>> +check_untyped_defs = False
>>>> +
>>>> +[mypy-qapi.schema]
>>>> +disallow_untyped_defs = False
>>>> +disallow_incomplete_defs = False
>>>> +check_untyped_defs = False
>>>> +
>>>> +[mypy-qapi.source]
>>>> +disallow_untyped_defs = False
>>>> +disallow_incomplete_defs = False
>>>> +check_untyped_defs = False
>>>> +
>>>> +[mypy-qapi.types]
>>>> +disallow_untyped_defs = False
>>>> +disallow_incomplete_defs = False
>>>> +check_untyped_defs = False
>>>> +
>>>> +[mypy-qapi.visit]
>>>> +disallow_untyped_defs = False
>>>> +disallow_incomplete_defs = False
>>>> +check_untyped_defs = False
>>> Greek to me.  I'll learn in due time.
>>>
>>
>> I am using these options:
>>
>> --strict, which is effectively -Wall.
>>
>> --no-strict-optional, which disables type checking errors on conflict
>>    between Optional[T] and T. Namely, when you initialize a class field
>>   to None and set that variable after initialization, callers must be
>> prepared to see if that field was None or not. We do that effectively
>> nowhere.
>>
>> As Python will surely explode in a noticeable way if we got an
>> unexpected 'None', I am just suppressing these warnings "for now".
> 
> Okay.
> 
> We may want to rethink how we define and initialize these variables.  We
> initialize mostly to keep pylint happy.  We initialize to None precisely
> to make use before the real initialization explode.
> 

The pattern we have is roughly correct; it's just that we rely on that 
implicit explosion, which mypy will warn about if I don't disable this 
option.

It's not worth addressing in our first pass, but if you look at 
schema.py's @property ifcond code, that's the type of thing that would 
"fix" this warning -- basically access shims that add a "real" error 
message when it's None, but convert the return type from Optional[T] to T.

It's later stuff.

>> --allow-untyped-calls silences errors in files that have calls to
>>    functions in files I still have not typed. By the end of the series,
>> this option goes away, because there's nothing untyped left.
>>
>>
>> For each untyped file, we are actually starting with all of the above
>> options and then layering these options on top. Any egregious typing
>> errors present in these "ignored" files will be spotted.
>>
>> To get the bad files to pass, we only need three options:
>>
>> allow untyped defs -- Simply permits us to have functions without
>> annotations.
>>
>> allow incomplete defs -- allows functions that are only partially typed.
>>
>> check untyped defs = False -- Don't try to type check untyped definitions.
> 
> Thanks!
> 
> [...]
>