mbox series

[v6,00/36] qapi: static typing conversion, pt1

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

Message

John Snow Oct. 9, 2020, 4:15 p.m. UTC
Hi, this series adds static type hints to the QAPI module.
This is part one!

Part 1: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt1
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)

In general, this series tackles the cleanup of one individual QAPI
module at a time. Once it passes pylint or mypy checks, those checks are
enabled for that file.

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.

Notes:

- After patch 07, `isort -c` should pass 100% on this and every
  future commit.

- After patch 08, `flake8 qapi/` should pass 100% on this and every
  future commit.

- After patch 09, `pylint --rcfile=qapi/pylintrc qapi/` should pass 100%
  on this and every future commit.

- After patch 18, `mypy --config-file=qapi/mypy.ini qapi/` should pass
  100% on this and every future commit.

Review Status:

[01] docs-repair-broken-references  #
[02] qapi-modify-docstrings-to-be   #
[03] qapi-gen-separate-arg-parsing  # [RB] CR,EH [TB] CR [SOB] JS
[04] qapi-move-generator-entrypoint # [RB] CR,EH [TB] CR [SOB] JS
[05] qapi-prefer-explicit-relative  # [RB] CR,EH,PM [SOB] JS
[06] qapi-remove-wildcard-includes  # [RB] CR,EH,PM [SOB] JS
[07] qapi-enforce-import-order      # [RB] CR,MA [TB] CR [SOB] JS
[08] qapi-delint-using-flake8       # [RB] CR,EH [SOB] JS
[09] qapi-add-pylintrc              # [RB] CR [TB] CR,EH [SOB] JS
[10] qapi-common-py-remove-python   # [RB] CR,EH [SOB] JS
[11] qapi-common-add-indent-manager # [RB] CR,EH [SOB] JS
[12] qapi-common-py-delint-with     # [RB] CR,EH [SOB] JS
[13] replace-c-by-char              # [RB] CR,EH,PM [SOB] JS
[14] qapi-common-py-check-with      # [RB] CR [TB] CR,EH [SOB] JS
[15] qapi-common-py-add-notational  # [RB] CR,EH [SOB] JS
[16] qapi-common-move-comments-into # [RB] CR,EH [SOB] JS
[17] qapi-split-build_params-into   # [RB] CR,EH [SOB] JS
[18] qapi-establish-mypy-type       # [RB] CR [TB] CR,EH [SOB] JS
[19] qapi-events-py-add-notational  # [RB] CR,EH [SOB] JS
[20] qapi-events-move-comments-into # [RB] CR,EH [SOB] JS
[21] qapi-commands-py-don-t-re-bind # [RB] CR,EH,MA [SOB] JS
[22] qapi-commands-py-add           # [RB] CR,EH [SOB] JS
[23] qapi-commands-py-enable        # [RB] CR,EH [SOB] JS
[24] qapi-source-py-add-notational  # [RB] CR,EH [TB] CR [SOB] JS
[25] qapi-source-py-delint-with     # [RB] CR,EH [TB] CR [SOB] JS
[26] qapi-gen-py-fix-edge-case-of   # [RB] CR,EH,PM [SOB] JS
[27] qapi-gen-py-add-notational     # [RB] CR,EH [SOB] JS
[28] qapi-gen-py-enable-checking    # [RB] CR,EH [TB] CR [SOB] JS
[29] qapi-gen-py-remove-unused      # [RB] CR,EH [SOB] JS
[30] qapi-gen-py-update-write-to-be # [RB] CR,EH,MA [SOB] JS
[31] qapi-gen-py-delint-with-pylint # [RB] CR,EH [SOB] JS
[32] qapi-types-py-add-type-hint    # [RB] CR,EH [SOB] JS
[33] qapi-types-py-remove-one       # [RB] CR,EH [SOB] JS
[34] qapi-visit-py-assert           # [RB] CR,EH [SOB] JS
[35] qapi-visit-py-remove-unused    # [RB] CR,EH,PM [TB] CR [SOB] JS
[36] qapi-visit-py-add-notational   # [RB] CR,EH [TB] CR [SOB] JS

Changelog:

002/36:[0001] [FC] 'qapi: modify docstrings to be sphinx-compatible'
003/36:[0030] [FC] 'qapi-gen: Separate arg-parsing from generation'
004/36:[down] 'qapi: move generator entrypoint into package'
008/36:[0008] [FC] 'qapi: delint using flake8'
016/36:[0017] [FC] 'qapi/common.py: Convert comments into docstrings, and elaborate'
017/36:[0004] [FC] 'qapi/common.py: move build_params into gen.py'
022/36:[0011] [FC] 'qapi/commands.py: add type hint annotations'
024/36:[0003] [FC] 'qapi/source.py: add type hint annotations'
033/36:[0022] [FC] 'qapi/types.py: remove one-letter variables'

V6:
 - Rebase
 - 02: Dropped changes not *strictly* required for sphinx building.
 - 03: Split prefix check into helper function,
       Removed default constants,
       Moved error message back to main() function, changed wording
 - 04: Changes from 003; fixed commit message ("package", not "module")
 - 08: Line wrapping changes.
 - 16: Sphinx formatting changes, docstring change.
 - 17: Copyright string changes
 - 22: Reflow and add type for new 'coroutine' parameter
 - 24: Simpler QAPISourceInfo() typing
 - 33: "memb" and "var" instead of "member" and "variant".

V5:
 - Remove DO-NOT-MERGE patches (Now in Part2)
 - Remove introspect.py patches (Now in Part2)
 - 02: Docstring formatting, more commit message (Markus)

V4:
 - Rebase on Peter Maydell's work;
  - 05: Context differences from Peter Maydell's work
  - 06: Remove qapi.doc
  - 07: Remove qapi.doc, remove superfluous "if match"
  - 09: Remove qapi.doc
  - 13: remove qapi.doc
  - 18: remove qapi.doc
  - 22: remove qapi.doc
  - 31: remove QAPIGenDoc changes

 - Minor adjustments from list feedback;
  - 01: More backticks for QAPI json files, now that they are in Sphinx-exe
  - 02: Use :ref: role for the `docker-ref` cross-reference
  - 04: Remove doc.py work changes; add references for start_if/end_if
  - 10: Changes to accommodate isort
  - 11: Added lines_after_imports=2
  - 16: isort whitespace changes
  - 24: Take Markus's docstring phrasing
  - 34: add comment explaining os.open()
  - 37: isort differences

V3:
 - Use isort to enforce import consistency
 - Use sphinx apidoc to check docstring format

V2:
 - Removed Python3.6 patch in favor of Thomas Huth's
 - Addressed (most) feedback from Markus
 - Renamed type hint annotation commits

John Snow (36):
  docs: repair broken references
  qapi: modify docstrings to be sphinx-compatible
  qapi-gen: Separate arg-parsing from generation
  qapi: move generator entrypoint into package
  qapi: Prefer explicit relative imports
  qapi: Remove wildcard includes
  qapi: enforce import order/styling with isort
  qapi: delint using flake8
  qapi: add pylintrc
  qapi/common.py: Remove python compatibility workaround
  qapi/common.py: Add indent manager
  qapi/common.py: delint with pylint
  qapi/common.py: Replace one-letter 'c' variable
  qapi/common.py: check with pylint
  qapi/common.py: add type hint annotations
  qapi/common.py: Convert comments into docstrings, and elaborate
  qapi/common.py: move build_params into gen.py
  qapi: establish mypy type-checking baseline
  qapi/events.py: add type hint annotations
  qapi/events.py: Move comments into docstrings
  qapi/commands.py: Don't re-bind to variable of different type
  qapi/commands.py: add type hint annotations
  qapi/commands.py: enable checking with mypy
  qapi/source.py: add type hint annotations
  qapi/source.py: delint with pylint
  qapi/gen.py: Fix edge-case of _is_user_module
  qapi/gen.py: add type hint annotations
  qapi/gen.py: Enable checking with mypy
  qapi/gen.py: Remove unused parameter
  qapi/gen.py: update write() to be more idiomatic
  qapi/gen.py: delint with pylint
  qapi/types.py: add type hint annotations
  qapi/types.py: remove one-letter variables
  qapi/visit.py: assert tag_member contains a QAPISchemaEnumType
  qapi/visit.py: remove unused parameters from gen_visit_object
  qapi/visit.py: add type hint annotations

 docs/devel/multi-thread-tcg.rst |   2 +-
 docs/devel/testing.rst          |   2 +-
 scripts/qapi-gen.py             |  57 ++--------
 scripts/qapi/.flake8            |   2 +
 scripts/qapi/.isort.cfg         |   7 ++
 scripts/qapi/commands.py        |  90 +++++++++++-----
 scripts/qapi/common.py          | 164 ++++++++++++++++-------------
 scripts/qapi/events.py          |  58 +++++++---
 scripts/qapi/expr.py            |   7 +-
 scripts/qapi/gen.py             | 180 ++++++++++++++++++++------------
 scripts/qapi/introspect.py      |  16 ++-
 scripts/qapi/main.py            |  95 +++++++++++++++++
 scripts/qapi/mypy.ini           |  30 ++++++
 scripts/qapi/parser.py          |   6 +-
 scripts/qapi/pylintrc           |  70 +++++++++++++
 scripts/qapi/schema.py          |  33 +++---
 scripts/qapi/source.py          |  35 ++++---
 scripts/qapi/types.py           | 125 +++++++++++++++-------
 scripts/qapi/visit.py           | 116 ++++++++++++++------
 19 files changed, 759 insertions(+), 336 deletions(-)
 create mode 100644 scripts/qapi/.flake8
 create mode 100644 scripts/qapi/.isort.cfg
 create mode 100644 scripts/qapi/main.py
 create mode 100644 scripts/qapi/mypy.ini
 create mode 100644 scripts/qapi/pylintrc

-- 
2.26.2

Comments

Markus Armbruster Oct. 9, 2020, 5:26 p.m. UTC | #1
John Snow <jsnow@redhat.com> writes:

> The edge case is that if the name is '', this expression returns a
> string instead of a bool, which violates our declared type.
>
> In practice, module names are not allowed to be the empty string, but
> this constraint is not modeled for the type system.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Cleber Rosa <crosa@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  scripts/qapi/gen.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index fff0c0acb6d..2c305c4f82c 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -241,7 +241,7 @@ def __init__(self, prefix, what, user_blurb, builtin_blurb, pydoc):
>  
>      @staticmethod
>      def _is_user_module(name):
> -        return name and not name.startswith('./')
> +        return bool(name and not name.startswith('./'))

           return not (name is None or name.startswith('./')

Looks slightly clearer to me.

>  
>      @staticmethod
>      def _is_builtin_module(name):
Eduardo Habkost Oct. 9, 2020, 5:39 p.m. UTC | #2
On Fri, Oct 09, 2020 at 07:26:02PM +0200, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:

> 

> > The edge case is that if the name is '', this expression returns a

> > string instead of a bool, which violates our declared type.

> >

> > In practice, module names are not allowed to be the empty string, but

> > this constraint is not modeled for the type system.

> >

> > Signed-off-by: John Snow <jsnow@redhat.com>

> > Reviewed-by: Cleber Rosa <crosa@redhat.com>

> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> > ---

> >  scripts/qapi/gen.py | 2 +-

> >  1 file changed, 1 insertion(+), 1 deletion(-)

> >

> > diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py

> > index fff0c0acb6d..2c305c4f82c 100644

> > --- a/scripts/qapi/gen.py

> > +++ b/scripts/qapi/gen.py

> > @@ -241,7 +241,7 @@ def __init__(self, prefix, what, user_blurb, builtin_blurb, pydoc):

> >  

> >      @staticmethod

> >      def _is_user_module(name):

> > -        return name and not name.startswith('./')

> > +        return bool(name and not name.startswith('./'))

> 

>            return not (name is None or name.startswith('./')

> 

> Looks slightly clearer to me.


That would have exactly the same behavior as the
  name is not None and name.startswith('./')
expression we had in v1.

-- 
Eduardo
Markus Armbruster Oct. 10, 2020, 6:57 a.m. UTC | #3
Eduardo Habkost <ehabkost@redhat.com> writes:

> On Fri, Oct 09, 2020 at 07:26:02PM +0200, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>> > The edge case is that if the name is '', this expression returns a
>> > string instead of a bool, which violates our declared type.
>> > In practice, module names are not allowed to be the empty string, but
>> > this constraint is not modeled for the type system.
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>> > Reviewed-by: Cleber Rosa <crosa@redhat.com>
>> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Actually:

1. We also map None to None, which is also not bool.

2. There is no declared type to violate until the next patch.

3. The subject's claim 'Fix edge-case' is misleading: this is a cleanup,
   not a bug fix.

Easy enough to fix:

    qapi/gen: Make _is_user_module() return bool

    _is_user_module() returns thruth values.  The next commit wants it to
    return bool.  Make it so.

>> > ---
>> >  scripts/qapi/gen.py | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>> > index fff0c0acb6d..2c305c4f82c 100644
>> > --- a/scripts/qapi/gen.py
>> > +++ b/scripts/qapi/gen.py
>> > @@ -241,7 +241,7 @@ def __init__(self, prefix, what, user_blurb, builtin_blurb, pydoc):
>> >  
>> >      @staticmethod
>> >      def _is_user_module(name):
>> > -        return name and not name.startswith('./')
>> > +        return bool(name and not name.startswith('./'))
>> 
>>            return not (name is None or name.startswith('./')
>> 
>> Looks slightly clearer to me.
>
> That would have exactly the same behavior as the
>   name is not None and name.startswith('./')
> expression we had in v1.

True.

Let's review what this function should do, and what it does.

The function should return whether @name is a user module name.
Returning truthy and falsy is fine; the callers expect no more.

system module names are either pathnames starting with './' (see
_add_system_module(), or None.

user module names are pathnames not starting with './' (see
_module_name()).

Before the patch:

    if @name is falsy, i.e. None or '', return @name
    else return name.startswith('./')

Thus, the function maps

    None              -> None   (1)
    ''                -> ''     (2)
    './' + any string -> False  (3)
    any other string  -> True   (4)

This is correct.  Case (2) can't actually happen ('' is not a pathname).

John's version of the patch normalizes case (1) and (2) to

    None              -> False  (1)
    ''                -> False  (2)

so the next patch can declare the function returns bool.  Safe, because
it doesn't change "thruthiness".

My version of the patch 

    if @name is None, return False,
    else return not name.startswith('./')

Now the function maps

    None              -> False  (1)
    ''                -> True   (2)
    './' + any string -> False  (3)
    any other string  -> True   (4)

The only difference to John's patch is case (2).  That case is
impossible, so no difference in actual behavior.  Nevertheless, mapping
'' to True is unclean: it claims '' is a user module name, which it
isn't.

This would be clean:

    @staticmethod
    def _is_system_module(name):
        return name is None or name.startswith('./')

Adjusting callers would be straightforward.

Not worth it right now.  Taking John's patch with the rewritten commit
message.

Eduardo, thanks for making me think!
Markus Armbruster Oct. 10, 2020, 9:43 a.m. UTC | #4
John Snow <jsnow@redhat.com> writes:

> Hi, this series adds static type hints to the QAPI module.
> This is part one!
>
> Part 1: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt1
> 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)
>
> In general, this series tackles the cleanup of one individual QAPI
> module at a time. Once it passes pylint or mypy checks, those checks are
> enabled for that file.
>
> 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.
>
> Notes:
>
> - After patch 07, `isort -c` should pass 100% on this and every
>   future commit.
>
> - After patch 08, `flake8 qapi/` should pass 100% on this and every
>   future commit.
>
> - After patch 09, `pylint --rcfile=qapi/pylintrc qapi/` should pass 100%
>   on this and every future commit.
>
> - After patch 18, `mypy --config-file=qapi/mypy.ini qapi/` should pass
>   100% on this and every future commit.

Series
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Queued, thanks!
John Snow Oct. 12, 2020, 2:19 p.m. UTC | #5
On 10/10/20 5:43 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> Hi, this series adds static type hints to the QAPI module.
>> This is part one!
>>
>> Part 1: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt1
>> 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)
>>
>> In general, this series tackles the cleanup of one individual QAPI
>> module at a time. Once it passes pylint or mypy checks, those checks are
>> enabled for that file.
>>
>> 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.
>>
>> Notes:
>>
>> - After patch 07, `isort -c` should pass 100% on this and every
>>    future commit.
>>
>> - After patch 08, `flake8 qapi/` should pass 100% on this and every
>>    future commit.
>>
>> - After patch 09, `pylint --rcfile=qapi/pylintrc qapi/` should pass 100%
>>    on this and every future commit.
>>
>> - After patch 18, `mypy --config-file=qapi/mypy.ini qapi/` should pass
>>    100% on this and every future commit.
> 
> Series
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 
> Queued, thanks!
> 

Thanks for putting up with me!

Only five left to go! Enjoy your PTO.

--js