| Message ID | 20201026213637.47087-1-jsnow@redhat.com |
|---|---|
| Headers | show |
| Series | qapi: static typing conversion, pt3 | expand |
On 10/26/20 5:36 PM, John Snow wrote: > based-on: <20201026194251.11075-1-jsnow@redhat.com> > [PATCH v2 00/11] qapi: static typing conversion, pt2 Ping, This series can be reviewed independently of pt2, so I encourage you to try if you have the time. > > Hi, this series adds static type hints to the QAPI module. > This is part three, and it focuses on expr.py. > > Part 3: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt3 > Everything: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt6 > > - Requires Python 3.6+ > - Requires mypy 0.770 or newer (for type analysis only) > - Requires pylint 2.6.0 or newer (for lint checking only) > > Type hints are added in patches that add *only* type hints and change no > other behavior. Any necessary changes to behavior to accommodate typing > are split out into their own tiny patches. > > Every commit should pass with: > - flake8 qapi/ > - pylint --rcfile=qapi/pylintrc qapi/ > - mypy --config-file=qapi/mypy.ini qapi/ > > V2: > - Rebased on the latest V2 > 002/16:[0001] [FC] 'qapi/expr.py: Check for dict instead of OrderedDict' > - Import order differences > 007/16:[0006] [FC] 'qapi/expr.py: Add casts in a few select cases' > - Import order differences > 008/16:[0006] [FC] 'qapi/expr.py: add type hint annotations' > - Import order differents > 012/16:[0066] [FC] 'qapi/expr.py: Add docstrings' > - Various docstring changes for Sphinx > 014/16:[0004] [FC] 'qapi/expr.py: Use tuples instead of lists for static data' > - Change to accommodate new 'coroutine' key > 015/16:[0006] [FC] 'qapi/expr.py: move related checks inside check_xxx functions' > - Fix check order (ehabkost) > > John Snow (16): > qapi/expr.py: Remove 'info' argument from nested check_if_str > qapi/expr.py: Check for dict instead of OrderedDict > qapi/expr.py: constrain incoming expression types > qapi/expr.py: Add assertion for union type 'check_dict' > qapi/expr.py: move string check upwards in check_type > qapi/expr.py: Check type of 'data' member > qapi/expr.py: Add casts in a few select cases > qapi/expr.py: add type hint annotations > qapi/expr.py: rewrite check_if > qapi/expr.py: Remove single-letter variable > qapi/expr.py: enable pylint checks > qapi/expr.py: Add docstrings > qapi/expr.py: Modify check_keys to accept any Iterable > qapi/expr.py: Use tuples instead of lists for static data > qapi/expr.py: move related checks inside check_xxx functions > qapi/expr.py: Use an expression checker dispatch table > > scripts/qapi/expr.py | 447 +++++++++++++++++++++++++++++++----------- > scripts/qapi/mypy.ini | 5 - > scripts/qapi/pylintrc | 1 - > 3 files changed, 334 insertions(+), 119 deletions(-) >
Hi On Wed, Nov 4, 2020 at 5:16 AM John Snow <jsnow@redhat.com> wrote: > On 10/26/20 5:36 PM, John Snow wrote: > > based-on: <20201026194251.11075-1-jsnow@redhat.com> > > [PATCH v2 00/11] qapi: static typing conversion, pt2 > > Ping, > > This series can be reviewed independently of pt2, so I encourage you to > try if you have the time. > > > > > Hi, this series adds static type hints to the QAPI module. > > This is part three, and it focuses on expr.py. > > > > Part 3: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt3 > > Everything: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt6 > > > > - Requires Python 3.6+ > > - Requires mypy 0.770 or newer (for type analysis only) > > - Requires pylint 2.6.0 or newer (for lint checking only) > > > > Type hints are added in patches that add *only* type hints and change no > > other behavior. Any necessary changes to behavior to accommodate typing > > are split out into their own tiny patches. > > > > Every commit should pass with: > > - flake8 qapi/ > > - pylint --rcfile=qapi/pylintrc qapi/ > > - mypy --config-file=qapi/mypy.ini qapi/ > > > > V2: > > - Rebased on the latest V2 > > 002/16:[0001] [FC] 'qapi/expr.py: Check for dict instead of OrderedDict' > > - Import order differences > > 007/16:[0006] [FC] 'qapi/expr.py: Add casts in a few select cases' > > - Import order differences > > 008/16:[0006] [FC] 'qapi/expr.py: add type hint annotations' > > - Import order differents > > 012/16:[0066] [FC] 'qapi/expr.py: Add docstrings' > > - Various docstring changes for Sphinx > > 014/16:[0004] [FC] 'qapi/expr.py: Use tuples instead of lists for static > data' > > - Change to accommodate new 'coroutine' key > > 015/16:[0006] [FC] 'qapi/expr.py: move related checks inside check_xxx > functions' > > - Fix check order (ehabkost) > > > > John Snow (16): > > qapi/expr.py: Remove 'info' argument from nested check_if_str > > qapi/expr.py: Check for dict instead of OrderedDict > > qapi/expr.py: constrain incoming expression types > > qapi/expr.py: Add assertion for union type 'check_dict' > > qapi/expr.py: move string check upwards in check_type > > qapi/expr.py: Check type of 'data' member > > qapi/expr.py: Add casts in a few select cases > > qapi/expr.py: add type hint annotations > > qapi/expr.py: rewrite check_if > > qapi/expr.py: Remove single-letter variable > > qapi/expr.py: enable pylint checks > > qapi/expr.py: Add docstrings > > qapi/expr.py: Modify check_keys to accept any Iterable > > qapi/expr.py: Use tuples instead of lists for static data > > qapi/expr.py: move related checks inside check_xxx functions > > qapi/expr.py: Use an expression checker dispatch table > > > > scripts/qapi/expr.py | 447 +++++++++++++++++++++++++++++++----------- > > scripts/qapi/mypy.ini | 5 - > > scripts/qapi/pylintrc | 1 - > > 3 files changed, 334 insertions(+), 119 deletions(-) > > > > > Looks all good to me. And you have already reviewed-by on all patches. Given that it's hardening the current code, I would suggest to merge it during the freeze. Unless Markus can maintain a qapi-next branch where we can base our work on? thanks
John Snow <jsnow@redhat.com> writes: > OrderedDict is a subtype of dict, so we can check for a more general form. > > Signed-off-by: John Snow <jsnow@redhat.com> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > Reviewed-by: Cleber Rosa <crosa@redhat.com> > --- > scripts/qapi/expr.py | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > index 35695c4c653b..5694c501fa38 100644 > --- a/scripts/qapi/expr.py > +++ b/scripts/qapi/expr.py > @@ -14,7 +14,6 @@ > # This work is licensed under the terms of the GNU GPL, version 2. > # See the COPYING file in the top-level directory. > > -from collections import OrderedDict > import re > > from .common import c_name > @@ -131,7 +130,7 @@ def check_if_str(ifcond): > > > def normalize_members(members): > - if isinstance(members, OrderedDict): > + if isinstance(members, dict): > for key, arg in members.items(): > if isinstance(arg, dict): > continue > @@ -162,7 +161,7 @@ def check_type(value, info, source, > if not allow_dict: > raise QAPISemError(info, "%s should be a type name" % source) > > - if not isinstance(value, OrderedDict): > + if not isinstance(value, dict): > raise QAPISemError(info, > "%s should be an object or type name" % source) Plain dict remembers insertion order since Python 3.6, but it wasn't formally promised until 3.7. Can we simply ditch OrderedDict entirely?
On Tue, Nov 17, 2020 at 05:30:04PM +0100, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > > > OrderedDict is a subtype of dict, so we can check for a more general form. > > > > Signed-off-by: John Snow <jsnow@redhat.com> > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > Reviewed-by: Cleber Rosa <crosa@redhat.com> > > --- > > scripts/qapi/expr.py | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > > index 35695c4c653b..5694c501fa38 100644 > > --- a/scripts/qapi/expr.py > > +++ b/scripts/qapi/expr.py > > @@ -14,7 +14,6 @@ > > # This work is licensed under the terms of the GNU GPL, version 2. > > # See the COPYING file in the top-level directory. > > > > -from collections import OrderedDict > > import re > > > > from .common import c_name > > @@ -131,7 +130,7 @@ def check_if_str(ifcond): > > > > > > def normalize_members(members): > > - if isinstance(members, OrderedDict): > > + if isinstance(members, dict): > > for key, arg in members.items(): > > if isinstance(arg, dict): > > continue > > @@ -162,7 +161,7 @@ def check_type(value, info, source, > > if not allow_dict: > > raise QAPISemError(info, "%s should be a type name" % source) > > > > - if not isinstance(value, OrderedDict): > > + if not isinstance(value, dict): > > raise QAPISemError(info, > > "%s should be an object or type name" % source) > > Plain dict remembers insertion order since Python 3.6, but it wasn't > formally promised until 3.7. > > Can we simply ditch OrderedDict entirely? In theory, our build requirement is "Python >= 3.6", not "CPython >= 3.6". In practice, I don't expect anybody to ever use any other Python implementation except CPython to build QEMU. I think we can get rid of OrderedDict if you really want to. -- Eduardo
On 11/17/20 1:08 PM, Eduardo Habkost wrote: > On Tue, Nov 17, 2020 at 05:30:04PM +0100, Markus Armbruster wrote: >> John Snow <jsnow@redhat.com> writes: >> >>> OrderedDict is a subtype of dict, so we can check for a more general form. >>> >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> >>> Reviewed-by: Cleber Rosa <crosa@redhat.com> >>> --- >>> scripts/qapi/expr.py | 5 ++--- >>> 1 file changed, 2 insertions(+), 3 deletions(-) >>> >>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py >>> index 35695c4c653b..5694c501fa38 100644 >>> --- a/scripts/qapi/expr.py >>> +++ b/scripts/qapi/expr.py >>> @@ -14,7 +14,6 @@ >>> # This work is licensed under the terms of the GNU GPL, version 2. >>> # See the COPYING file in the top-level directory. >>> >>> -from collections import OrderedDict >>> import re >>> >>> from .common import c_name >>> @@ -131,7 +130,7 @@ def check_if_str(ifcond): >>> >>> >>> def normalize_members(members): >>> - if isinstance(members, OrderedDict): >>> + if isinstance(members, dict): >>> for key, arg in members.items(): >>> if isinstance(arg, dict): >>> continue >>> @@ -162,7 +161,7 @@ def check_type(value, info, source, >>> if not allow_dict: >>> raise QAPISemError(info, "%s should be a type name" % source) >>> >>> - if not isinstance(value, OrderedDict): >>> + if not isinstance(value, dict): >>> raise QAPISemError(info, >>> "%s should be an object or type name" % source) >> >> Plain dict remembers insertion order since Python 3.6, but it wasn't >> formally promised until 3.7. >> >> Can we simply ditch OrderedDict entirely? > > In theory, our build requirement is "Python >= 3.6", not > "CPython >= 3.6". In practice, I don't expect anybody to ever > use any other Python implementation except CPython to build QEMU. > > I think we can get rid of OrderedDict if you really want to. > No harm in keeping it right now either, it doesn't make typing harder. The OrderedDict is created in the parser, so we can cover ditching OrderedDict when we get to that module if desired. --js
John Snow <jsnow@redhat.com> writes: > Iterating over the members of data isn't going to work if it's not some > form of dict anyway, but for type safety, formalize it. > > Signed-off-by: John Snow <jsnow@redhat.com> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > --- > scripts/qapi/expr.py | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > index 864363881682..2654a72e8333 100644 > --- a/scripts/qapi/expr.py > +++ b/scripts/qapi/expr.py > @@ -254,6 +254,9 @@ def check_union(expr, info): > raise QAPISemError(info, "'discriminator' requires 'base'") > check_name_is_str(discriminator, info, "'discriminator'") > > + if not isinstance(members, dict): > + raise QAPISemError(info, "'data' must be an object") > + > for (key, value) in members.items(): > source = "'data' member '%s'" % key > check_name_str(key, info, source) > @@ -267,6 +270,10 @@ def check_alternate(expr, info): > > if not members: > raise QAPISemError(info, "'data' must not be empty") > + > + if not isinstance(members, dict): > + raise QAPISemError(info, "'data' must be an object") > + > for (key, value) in members.items(): > source = "'data' member '%s'" % key > check_name_str(key, info, source) No raise without a test case covering it. Add the test case *first*. Just in case it demonstrates a frontend bug. I believe these two are such bugs.
based-on: <20201026194251.11075-1-jsnow@redhat.com> [PATCH v2 00/11] qapi: static typing conversion, pt2 Hi, this series adds static type hints to the QAPI module. This is part three, and it focuses on expr.py. Part 3: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt3 Everything: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt6 - Requires Python 3.6+ - Requires mypy 0.770 or newer (for type analysis only) - Requires pylint 2.6.0 or newer (for lint checking only) Type hints are added in patches that add *only* type hints and change no other behavior. Any necessary changes to behavior to accommodate typing are split out into their own tiny patches. Every commit should pass with: - flake8 qapi/ - pylint --rcfile=qapi/pylintrc qapi/ - mypy --config-file=qapi/mypy.ini qapi/ V2: - Rebased on the latest V2 002/16:[0001] [FC] 'qapi/expr.py: Check for dict instead of OrderedDict' - Import order differences 007/16:[0006] [FC] 'qapi/expr.py: Add casts in a few select cases' - Import order differences 008/16:[0006] [FC] 'qapi/expr.py: add type hint annotations' - Import order differents 012/16:[0066] [FC] 'qapi/expr.py: Add docstrings' - Various docstring changes for Sphinx 014/16:[0004] [FC] 'qapi/expr.py: Use tuples instead of lists for static data' - Change to accommodate new 'coroutine' key 015/16:[0006] [FC] 'qapi/expr.py: move related checks inside check_xxx functions' - Fix check order (ehabkost) John Snow (16): qapi/expr.py: Remove 'info' argument from nested check_if_str qapi/expr.py: Check for dict instead of OrderedDict qapi/expr.py: constrain incoming expression types qapi/expr.py: Add assertion for union type 'check_dict' qapi/expr.py: move string check upwards in check_type qapi/expr.py: Check type of 'data' member qapi/expr.py: Add casts in a few select cases qapi/expr.py: add type hint annotations qapi/expr.py: rewrite check_if qapi/expr.py: Remove single-letter variable qapi/expr.py: enable pylint checks qapi/expr.py: Add docstrings qapi/expr.py: Modify check_keys to accept any Iterable qapi/expr.py: Use tuples instead of lists for static data qapi/expr.py: move related checks inside check_xxx functions qapi/expr.py: Use an expression checker dispatch table scripts/qapi/expr.py | 447 +++++++++++++++++++++++++++++++----------- scripts/qapi/mypy.ini | 5 - scripts/qapi/pylintrc | 1 - 3 files changed, 334 insertions(+), 119 deletions(-) -- 2.26.2