| Message ID | 20200915224027.2529813-17-jsnow@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | qapi: static typing conversion, pt1 | expand |
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? In case it matters: $ mypy --version mypy 0.761 > 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. > > > 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. > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > index b4921b46c9..bb0cd717f1 100644 > --- a/scripts/qapi/schema.py > +++ b/scripts/qapi/schema.py > @@ -17,6 +17,7 @@ > import os > import re > from collections import OrderedDict > +from typing import Optional > > from .common import c_name, POINTER_SUFFIX > from .error import QAPIError, QAPISemError > @@ -25,7 +26,7 @@ > > > class QAPISchemaEntity: > - meta = None > + meta: Optional[str] = None > > def __init__(self, name, info, doc, ifcond=None, features=None): > assert name is None or isinstance(name, str)
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/ >> 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. >> >> >> 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". --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. >> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py >> index b4921b46c9..bb0cd717f1 100644 >> --- a/scripts/qapi/schema.py >> +++ b/scripts/qapi/schema.py >> @@ -17,6 +17,7 @@ >> import os >> import re >> from collections import OrderedDict >> +from typing import Optional >> >> from .common import c_name, POINTER_SUFFIX >> from .error import QAPIError, QAPISemError >> @@ -25,7 +26,7 @@ >> >> >> class QAPISchemaEntity: >> - meta = None >> + meta: Optional[str] = None >> >> def __init__(self, name, info, doc, ifcond=None, features=None): >> assert name is None or isinstance(name, str)
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? > > In case it matters: > > $ mypy --version > mypy 0.761 > (Warning; FiSH and stgit ahead) cd ~/src/qemu/scripts pipenv --python 3.6 pipenv shell pip install pylint flake8 ### Testing mypy 0.770 pip install mypy==0.770 stg goto qapi-establish-mypy-type while true; and flake8 qapi/; and pylint --rcfile=qapi/pylintrc qapi/; and mypy --config-file=qapi/mypy.ini qapi/; and stg push; end pip uninstall mypy ### 0.782 - OK 0.770 - OK 0.760 - FAIL, Fixable* 0.750 - OK* 0.740 - OK* 0.730 - OK* 0.720 - OK* 0.710 - OK** (Does not recognize 'no_implicit_reexport' option) 0.700 - OK*** (Not compatible with bleeding edge pylint/flake8) 0.670 - OK*** 0.660 - OK*** 0.650 - OK*** 0.641 - OK*** 0.630 - Fails again. 0.760 doesn't support strict in the config file (It needs component options), and it wants a few extra annotations where its inference power is weaker. Well, easy enough to fix up. 0.630 fails again and insists that __init__ should have a return type annotation of None. Modern mypy is smart enough to know that's what that type is supposed to be. Arbitrarily, this is my cutoff for what seems reasonable to even want to support. I still find the lack of "strict=true" in the config file irritating and might ask to target 0.770 or newer. There should be no reason we can't install that in a venv for CI to chew on. Humbly ask I take the lazy way out and document that we support mypy >= 0.770. --js
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. >>> 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. >>> >>> 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. > --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! [...]
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? >> In case it matters: >> $ mypy --version >> mypy 0.761 >> > > (Warning; FiSH and stgit ahead) > > cd ~/src/qemu/scripts > pipenv --python 3.6 > pipenv shell > pip install pylint flake8 > > ### Testing mypy 0.770 > > pip install mypy==0.770 > stg goto qapi-establish-mypy-type > > while true; and flake8 qapi/; and pylint --rcfile=qapi/pylintrc qapi/; > and mypy --config-file=qapi/mypy.ini qapi/; and stg push; end > > pip uninstall mypy > > ### > > > > 0.782 - OK > 0.770 - OK > 0.760 - FAIL, Fixable* > 0.750 - OK* > 0.740 - OK* > 0.730 - OK* > 0.720 - OK* > 0.710 - OK** (Does not recognize 'no_implicit_reexport' option) > 0.700 - OK*** (Not compatible with bleeding edge pylint/flake8) > 0.670 - OK*** > 0.660 - OK*** > 0.650 - OK*** > 0.641 - OK*** > 0.630 - Fails again. > > > > 0.760 doesn't support strict in the config file (It needs component > options), and it wants a few extra annotations where its inference > power is weaker. Well, easy enough to fix up. > > 0.630 fails again and insists that __init__ should have a return type > annotation of None. Modern mypy is smart enough to know that's what > that type is supposed to be. Arbitrarily, this is my cutoff for what > seems reasonable to even want to support. > > I still find the lack of "strict=true" in the config file irritating > and might ask to target 0.770 or newer. There should be no reason we > can't install that in a venv for CI to chew on. > > Humbly ask I take the lazy way out and document that we support mypy > >= 0.770. Our "supported build platforms" policy puts hard limits on the minimum versions for tools the build requires. Mypy is not such a tool. I hope we get to the point where we can have "make check" run it, but skipping the test when we don't have a sufficiently modern mypy feels okay to me, as long as our gating CI still guards the master branch.
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! > > [...] >
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? >>> In case it matters: >>> $ mypy --version >>> mypy 0.761 >>> >> >> (Warning; FiSH and stgit ahead) >> >> cd ~/src/qemu/scripts >> pipenv --python 3.6 >> pipenv shell >> pip install pylint flake8 >> >> ### Testing mypy 0.770 >> >> pip install mypy==0.770 >> stg goto qapi-establish-mypy-type >> >> while true; and flake8 qapi/; and pylint --rcfile=qapi/pylintrc qapi/; >> and mypy --config-file=qapi/mypy.ini qapi/; and stg push; end >> >> pip uninstall mypy >> >> ### >> >> >> >> 0.782 - OK >> 0.770 - OK >> 0.760 - FAIL, Fixable* >> 0.750 - OK* >> 0.740 - OK* >> 0.730 - OK* >> 0.720 - OK* >> 0.710 - OK** (Does not recognize 'no_implicit_reexport' option) >> 0.700 - OK*** (Not compatible with bleeding edge pylint/flake8) >> 0.670 - OK*** >> 0.660 - OK*** >> 0.650 - OK*** >> 0.641 - OK*** >> 0.630 - Fails again. >> >> >> >> 0.760 doesn't support strict in the config file (It needs component >> options), and it wants a few extra annotations where its inference >> power is weaker. Well, easy enough to fix up. >> >> 0.630 fails again and insists that __init__ should have a return type >> annotation of None. Modern mypy is smart enough to know that's what >> that type is supposed to be. Arbitrarily, this is my cutoff for what >> seems reasonable to even want to support. >> >> I still find the lack of "strict=true" in the config file irritating >> and might ask to target 0.770 or newer. There should be no reason we >> can't install that in a venv for CI to chew on. >> >> Humbly ask I take the lazy way out and document that we support mypy >>> = 0.770. > > Our "supported build platforms" policy puts hard limits on the minimum > versions for tools the build requires. > > Mypy is not such a tool. I hope we get to the point where we can have > "make check" run it, but skipping the test when we don't have a > sufficiently modern mypy feels okay to me, as long as our gating CI > still guards the master branch. > Yes, that's been my view. These tools *are* new and they *do* change often. Chasing a wide compatibility window with them is prohibitively difficult. Luckily, they're not needed for running the packages at all. If I do add linting to 'make check', it is going to be through a virtual environment that deploys *specific versions* of these tools, and it will happen transparently. The VM tests already do this. --js
On Mon, Sep 21, 2020 at 10:05:24AM +0200, Markus Armbruster wrote: [...] > 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. This possibility shouldn't exist. If you don't have the required version in your system, the validation script must know how to create a virtualenv and install the right version inside it. -- Eduardo
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 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 diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index b4921b46c9..bb0cd717f1 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -17,6 +17,7 @@ import os import re from collections import OrderedDict +from typing import Optional from .common import c_name, POINTER_SUFFIX from .error import QAPIError, QAPISemError @@ -25,7 +26,7 @@ class QAPISchemaEntity: - meta = None + meta: Optional[str] = None def __init__(self, name, info, doc, ifcond=None, features=None): assert name is None or isinstance(name, str)