mbox series

[v2,00/16] qapi: static typing conversion, pt3

Message ID 20201026213637.47087-1-jsnow@redhat.com
Headers show
Series qapi: static typing conversion, pt3 | expand

Message

John Snow Oct. 26, 2020, 9:36 p.m. UTC
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

Comments

John Snow Nov. 4, 2020, 12:41 a.m. UTC | #1
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(-)
>
Marc-André Lureau Nov. 4, 2020, 2:53 p.m. UTC | #2
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
Markus Armbruster Nov. 17, 2020, 4:30 p.m. UTC | #3
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?
Eduardo Habkost Nov. 17, 2020, 6:08 p.m. UTC | #4
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
John Snow Nov. 17, 2020, 7:48 p.m. UTC | #5
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
Markus Armbruster Nov. 18, 2020, 3:36 p.m. UTC | #6
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.