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

[John Snow]

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!
>
> [...]