mbox series

[v2,00/11] qapi: static typing conversion, pt2

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

Message

John Snow Oct. 26, 2020, 7:42 p.m. UTC
Hi, this series adds static type hints to the QAPI module.
This is part two, and covers introspect.py.

Part 2: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt2
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:
 - Dropped all R-B from previous series; enough has changed.
 - pt2 is now introspect.py, expr.py is pushed to pt3.
 - Reworked again to have less confusing (?) type names
 - Added an assertion to prevent future accidental breakage

John Snow (11):
  [DO-NOT-MERGE] docs: replace single backtick (`) with double-backtick
    (``)
  [DO-NOT-MERGE] docs/sphinx: change default role to "any"
  [DO-NOT-MERGE] docs: enable sphinx-autodoc for scripts/qapi
  qapi/introspect.py: add assertions and casts
  qapi/introspect.py: add preliminary type hint annotations
  qapi/introspect.py: add _gen_features helper
  qapi/introspect.py: Unify return type of _make_tree()
  qapi/introspect.py: replace 'extra' dict with 'comment' argument
  qapi/introspect.py: create a typed 'Annotated' data strutcure
  qapi/introspect.py: improve readability of _tree_to_qlit
  qapi/introspect.py: Add docstring to _tree_to_qlit

 docs/conf.py                           |   6 +-
 docs/devel/build-system.rst            | 120 +++++------
 docs/devel/index.rst                   |   1 +
 docs/devel/migration.rst               |  59 +++---
 docs/devel/python/index.rst            |   7 +
 docs/devel/python/qapi.commands.rst    |   7 +
 docs/devel/python/qapi.common.rst      |   7 +
 docs/devel/python/qapi.error.rst       |   7 +
 docs/devel/python/qapi.events.rst      |   7 +
 docs/devel/python/qapi.expr.rst        |   7 +
 docs/devel/python/qapi.gen.rst         |   7 +
 docs/devel/python/qapi.introspect.rst  |   7 +
 docs/devel/python/qapi.main.rst        |   7 +
 docs/devel/python/qapi.parser.rst      |   8 +
 docs/devel/python/qapi.rst             |  26 +++
 docs/devel/python/qapi.schema.rst      |   7 +
 docs/devel/python/qapi.source.rst      |   7 +
 docs/devel/python/qapi.types.rst       |   7 +
 docs/devel/python/qapi.visit.rst       |   7 +
 docs/devel/tcg-plugins.rst             |  14 +-
 docs/devel/testing.rst                 |   2 +-
 docs/interop/live-block-operations.rst |   4 +-
 docs/system/arm/cpu-features.rst       | 110 +++++-----
 docs/system/arm/nuvoton.rst            |   2 +-
 docs/system/s390x/protvirt.rst         |  10 +-
 qapi/block-core.json                   |   4 +-
 scripts/qapi/introspect.py             | 277 +++++++++++++++++--------
 scripts/qapi/mypy.ini                  |   5 -
 scripts/qapi/schema.py                 |   2 +-
 29 files changed, 487 insertions(+), 254 deletions(-)
 create mode 100644 docs/devel/python/index.rst
 create mode 100644 docs/devel/python/qapi.commands.rst
 create mode 100644 docs/devel/python/qapi.common.rst
 create mode 100644 docs/devel/python/qapi.error.rst
 create mode 100644 docs/devel/python/qapi.events.rst
 create mode 100644 docs/devel/python/qapi.expr.rst
 create mode 100644 docs/devel/python/qapi.gen.rst
 create mode 100644 docs/devel/python/qapi.introspect.rst
 create mode 100644 docs/devel/python/qapi.main.rst
 create mode 100644 docs/devel/python/qapi.parser.rst
 create mode 100644 docs/devel/python/qapi.rst
 create mode 100644 docs/devel/python/qapi.schema.rst
 create mode 100644 docs/devel/python/qapi.source.rst
 create mode 100644 docs/devel/python/qapi.types.rst
 create mode 100644 docs/devel/python/qapi.visit.rst

Comments

John Snow Nov. 2, 2020, 3:40 p.m. UTC | #1
On 10/26/20 3:42 PM, John Snow wrote:
> Hi, this series adds static type hints to the QAPI module.
> This is part two, and covers introspect.py.
> 
> Part 2: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt2
> 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:
>   - Dropped all R-B from previous series; enough has changed.
>   - pt2 is now introspect.py, expr.py is pushed to pt3.
>   - Reworked again to have less confusing (?) type names
>   - Added an assertion to prevent future accidental breakage
> 

Ping!

Patches 1-3: Can be skipped; just enables sphinx to check the docstring 
syntax. Don't worry about these too much, they're just here for you to 
test with.

Patch 4 adds some small changes, to support:
Patch 5 adds the type hints.
Patches 6-11 try to improve the readability of the types and the code.

This was a challenging file to clean up, so I am sure there's lots of 
easy, low-hanging fruit in the review/feedback for me to improve.

> John Snow (11):
>    [DO-NOT-MERGE] docs: replace single backtick (`) with double-backtick
>      (``)
>    [DO-NOT-MERGE] docs/sphinx: change default role to "any"
>    [DO-NOT-MERGE] docs: enable sphinx-autodoc for scripts/qapi
>    qapi/introspect.py: add assertions and casts
>    qapi/introspect.py: add preliminary type hint annotations
>    qapi/introspect.py: add _gen_features helper
>    qapi/introspect.py: Unify return type of _make_tree()
>    qapi/introspect.py: replace 'extra' dict with 'comment' argument
>    qapi/introspect.py: create a typed 'Annotated' data strutcure
>    qapi/introspect.py: improve readability of _tree_to_qlit
>    qapi/introspect.py: Add docstring to _tree_to_qlit
> 
>   docs/conf.py                           |   6 +-
>   docs/devel/build-system.rst            | 120 +++++------
>   docs/devel/index.rst                   |   1 +
>   docs/devel/migration.rst               |  59 +++---
>   docs/devel/python/index.rst            |   7 +
>   docs/devel/python/qapi.commands.rst    |   7 +
>   docs/devel/python/qapi.common.rst      |   7 +
>   docs/devel/python/qapi.error.rst       |   7 +
>   docs/devel/python/qapi.events.rst      |   7 +
>   docs/devel/python/qapi.expr.rst        |   7 +
>   docs/devel/python/qapi.gen.rst         |   7 +
>   docs/devel/python/qapi.introspect.rst  |   7 +
>   docs/devel/python/qapi.main.rst        |   7 +
>   docs/devel/python/qapi.parser.rst      |   8 +
>   docs/devel/python/qapi.rst             |  26 +++
>   docs/devel/python/qapi.schema.rst      |   7 +
>   docs/devel/python/qapi.source.rst      |   7 +
>   docs/devel/python/qapi.types.rst       |   7 +
>   docs/devel/python/qapi.visit.rst       |   7 +
>   docs/devel/tcg-plugins.rst             |  14 +-
>   docs/devel/testing.rst                 |   2 +-
>   docs/interop/live-block-operations.rst |   4 +-
>   docs/system/arm/cpu-features.rst       | 110 +++++-----
>   docs/system/arm/nuvoton.rst            |   2 +-
>   docs/system/s390x/protvirt.rst         |  10 +-
>   qapi/block-core.json                   |   4 +-
>   scripts/qapi/introspect.py             | 277 +++++++++++++++++--------
>   scripts/qapi/mypy.ini                  |   5 -
>   scripts/qapi/schema.py                 |   2 +-
>   29 files changed, 487 insertions(+), 254 deletions(-)
>   create mode 100644 docs/devel/python/index.rst
>   create mode 100644 docs/devel/python/qapi.commands.rst
>   create mode 100644 docs/devel/python/qapi.common.rst
>   create mode 100644 docs/devel/python/qapi.error.rst
>   create mode 100644 docs/devel/python/qapi.events.rst
>   create mode 100644 docs/devel/python/qapi.expr.rst
>   create mode 100644 docs/devel/python/qapi.gen.rst
>   create mode 100644 docs/devel/python/qapi.introspect.rst
>   create mode 100644 docs/devel/python/qapi.main.rst
>   create mode 100644 docs/devel/python/qapi.parser.rst
>   create mode 100644 docs/devel/python/qapi.rst
>   create mode 100644 docs/devel/python/qapi.schema.rst
>   create mode 100644 docs/devel/python/qapi.source.rst
>   create mode 100644 docs/devel/python/qapi.types.rst
>   create mode 100644 docs/devel/python/qapi.visit.rst
>
Marc-André Lureau Nov. 4, 2020, 9:51 a.m. UTC | #2
Hi

On Mon, Nov 2, 2020 at 7:41 PM John Snow <jsnow@redhat.com> wrote:

> On 10/26/20 3:42 PM, John Snow wrote:
> > Hi, this series adds static type hints to the QAPI module.
> > This is part two, and covers introspect.py.
> >
> > Part 2: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt2
> > 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:
> >   - Dropped all R-B from previous series; enough has changed.
> >   - pt2 is now introspect.py, expr.py is pushed to pt3.
> >   - Reworked again to have less confusing (?) type names
> >   - Added an assertion to prevent future accidental breakage
> >
>
> Ping!
>
> Patches 1-3: Can be skipped; just enables sphinx to check the docstring
> syntax. Don't worry about these too much, they're just here for you to
> test with.
>

They are interesting, but the rebase version fails. And the error produced
is not exactly friendly:
Exception occurred:
  File "/usr/lib/python3.9/site-packages/sphinx/domains/c.py", line 3751,
in resolve_any_xref
    return [('c:' + self.role_for_objtype(objtype), retnode)]
TypeError: can only concatenate str (not "NoneType") to str

Could you rebase and split off in a separate series?

Patch 4 adds some small changes, to support:
> Patch 5 adds the type hints.
> Patches 6-11 try to improve the readability of the types and the code.
>
> This was a challenging file to clean up, so I am sure there's lots of
> easy, low-hanging fruit in the review/feedback for me to improve.
>

Nothing obvious to me.

Python typing is fairly new to me, and I don't know the best practices. I
would just take what you did and improve later, if needed.

A wish item before we proceed with more python code cleanups is
documentation and/or automated tests.

Could you add a new make check-python and perhaps document what the new
code-style expectations?


> > John Snow (11):
> >    [DO-NOT-MERGE] docs: replace single backtick (`) with double-backtick
> >      (``)
> >    [DO-NOT-MERGE] docs/sphinx: change default role to "any"
> >    [DO-NOT-MERGE] docs: enable sphinx-autodoc for scripts/qapi
> >    qapi/introspect.py: add assertions and casts
> >    qapi/introspect.py: add preliminary type hint annotations
> >    qapi/introspect.py: add _gen_features helper
> >    qapi/introspect.py: Unify return type of _make_tree()
> >    qapi/introspect.py: replace 'extra' dict with 'comment' argument
> >    qapi/introspect.py: create a typed 'Annotated' data strutcure
> >    qapi/introspect.py: improve readability of _tree_to_qlit
> >    qapi/introspect.py: Add docstring to _tree_to_qlit
> >
> >   docs/conf.py                           |   6 +-
> >   docs/devel/build-system.rst            | 120 +++++------
> >   docs/devel/index.rst                   |   1 +
> >   docs/devel/migration.rst               |  59 +++---
> >   docs/devel/python/index.rst            |   7 +
> >   docs/devel/python/qapi.commands.rst    |   7 +
> >   docs/devel/python/qapi.common.rst      |   7 +
> >   docs/devel/python/qapi.error.rst       |   7 +
> >   docs/devel/python/qapi.events.rst      |   7 +
> >   docs/devel/python/qapi.expr.rst        |   7 +
> >   docs/devel/python/qapi.gen.rst         |   7 +
> >   docs/devel/python/qapi.introspect.rst  |   7 +
> >   docs/devel/python/qapi.main.rst        |   7 +
> >   docs/devel/python/qapi.parser.rst      |   8 +
> >   docs/devel/python/qapi.rst             |  26 +++
> >   docs/devel/python/qapi.schema.rst      |   7 +
> >   docs/devel/python/qapi.source.rst      |   7 +
> >   docs/devel/python/qapi.types.rst       |   7 +
> >   docs/devel/python/qapi.visit.rst       |   7 +
> >   docs/devel/tcg-plugins.rst             |  14 +-
> >   docs/devel/testing.rst                 |   2 +-
> >   docs/interop/live-block-operations.rst |   4 +-
> >   docs/system/arm/cpu-features.rst       | 110 +++++-----
> >   docs/system/arm/nuvoton.rst            |   2 +-
> >   docs/system/s390x/protvirt.rst         |  10 +-
> >   qapi/block-core.json                   |   4 +-
> >   scripts/qapi/introspect.py             | 277 +++++++++++++++++--------
> >   scripts/qapi/mypy.ini                  |   5 -
> >   scripts/qapi/schema.py                 |   2 +-
> >   29 files changed, 487 insertions(+), 254 deletions(-)
> >   create mode 100644 docs/devel/python/index.rst
> >   create mode 100644 docs/devel/python/qapi.commands.rst
> >   create mode 100644 docs/devel/python/qapi.common.rst
> >   create mode 100644 docs/devel/python/qapi.error.rst
> >   create mode 100644 docs/devel/python/qapi.events.rst
> >   create mode 100644 docs/devel/python/qapi.expr.rst
> >   create mode 100644 docs/devel/python/qapi.gen.rst
> >   create mode 100644 docs/devel/python/qapi.introspect.rst
> >   create mode 100644 docs/devel/python/qapi.main.rst
> >   create mode 100644 docs/devel/python/qapi.parser.rst
> >   create mode 100644 docs/devel/python/qapi.rst
> >   create mode 100644 docs/devel/python/qapi.schema.rst
> >   create mode 100644 docs/devel/python/qapi.source.rst
> >   create mode 100644 docs/devel/python/qapi.types.rst
> >   create mode 100644 docs/devel/python/qapi.visit.rst
> >
>
>
>
Markus Armbruster Nov. 16, 2020, 1:17 p.m. UTC | #3
Warning: losely related brain dump ahead.

introspect.py's purpose is providing query-qmp-schema with the data it
needs to built its response, which is a JSON object conforming to
['SchemaInfo'].

Stupidest solution that could possibly work: have this module generate a
C string holding the (JSON text) response.

Since a QMP command handler returns a QAPI object, not the response
string, this becomes:

     schema
        |
        |                                       qapi-gen.py
        v
    C string        --------------------------------------------------
        |
        |           qobject_from_json()
        v
     QObject                                    qmp_query_qmp_schema()
        |
        |           QObject input visitor
        v
  SchemaInfoList    --------------------------------------------------
        |
        |           QObject output visitor      generated wrapper
        v
     QObject        --------------------------------------------------
        |
        |           qobject_to_json()           QMP core
        v
    C string

Meh.  So many pointless conversions.

Shortcut: 'gen' false lets us cut out two:

     schema
        |
        |                                       qapi-gen.py
        v
    C string        --------------------------------------------------
        |
        |           qobject_from_json()         qmp_query_qmp_schema()
        v
     QObject        --------------------------------------------------
        |
        |           qobject_to_json()           QMP core
        v
    C string

Less work for handwritten qmp_query_qmp_schema(); it's now a one-liner.
This is the initial version (commit 39a1815816).

Commit 7d0f982bfb replaced the generated C string by a QLitObject:

     schema
        |
        |                                       qapi-gen.py
        v
     QLitObj        --------------------------------------------------
        |
        |           qobject_from_qlit()         qmp_query_qmp_schema()
        v
     QObject        --------------------------------------------------
        |
        |           qobject_to_json()           QMP core
        v
    C string

The commit message claims the QLitObj is "easier to deal with".  I doubt
it.  The conversion to QObject is a one-liner before and after.  Neither
form is hard to generate (example appended).

QLitObj takes more memory: ~360KiB, mostly .data (unnecessarily?),
wheras the C string is ~150KiB .text.  Of course, both are dwarved many
times over by QObject: 12.4MiB.  Gross.

However, to actually work with the introspection data in C, we'd want it
as SchemaInfoList.  I have a few uses for introspection data in mind,
and I definitely won't code them taking QObject input.

SchemaInfoList is one visitor away from QObject, so no big deal.

But what if we generated SchemaInfoList directly?  We'd have:

     schema
        |
        |                                       qapi-gen.py
        v
  SchemaInfoList    --------------------------------------------------
        |
        |           QObject output visitor      generated wrapper
        v
     QObject        --------------------------------------------------
        |
        |           qobject_to_json()           QMP core
        v
    C string

At ~480KiB, SchemaInfoList is less compact than QLitObj (let alone the C
string).  It should go entirely into .text, though.

I don't think generating SchemaInfoList would be particularly hard.
Just work.  Bigger fish to fry, I guess.  But getting the idea out can't
hurt.



Example: BlockdevOptionsFile

    { 'struct': 'BlockdevOptionsFile',
      'data': { 'filename': 'str',
                '*pr-manager': 'str',
                '*locking': 'OnOffAuto',
                '*aio': 'BlockdevAioOptions',
                '*drop-cache': {'type': 'bool',
                                'if': 'defined(CONFIG_LINUX)'},
                '*x-check-cache-dropped': 'bool' },
      'features': [ { 'name': 'dynamic-auto-read-only',
                      'if': 'defined(CONFIG_POSIX)' } ] }

Generated QLitObj:

        /* "267" = BlockdevOptionsFile */
        QLIT_QDICT(((QLitDictEntry[]) {
            { "features", QLIT_QLIST(((QLitObject[]) {
    #if defined(CONFIG_POSIX)
                QLIT_QSTR("dynamic-auto-read-only"),
    #endif /* defined(CONFIG_POSIX) */
                {}
            })), },
            { "members", QLIT_QLIST(((QLitObject[]) {
                QLIT_QDICT(((QLitDictEntry[]) {
                    { "name", QLIT_QSTR("filename"), },
                    { "type", QLIT_QSTR("str"), },
                    {}
                })),
                QLIT_QDICT(((QLitDictEntry[]) {
                    { "default", QLIT_QNULL, },
                    { "name", QLIT_QSTR("pr-manager"), },
                    { "type", QLIT_QSTR("str"), },
                    {}
                })),
                QLIT_QDICT(((QLitDictEntry[]) {
                    { "default", QLIT_QNULL, },
                    { "name", QLIT_QSTR("locking"), },
                    { "type", QLIT_QSTR("412"), },
                    {}
                })),
                QLIT_QDICT(((QLitDictEntry[]) {
                    { "default", QLIT_QNULL, },
                    { "name", QLIT_QSTR("aio"), },
                    { "type", QLIT_QSTR("413"), },
                    {}
                })),
    #if defined(CONFIG_LINUX)
                QLIT_QDICT(((QLitDictEntry[]) {
                    { "default", QLIT_QNULL, },
                    { "name", QLIT_QSTR("drop-cache"), },
                    { "type", QLIT_QSTR("bool"), },
                    {}
                })),
    #endif /* defined(CONFIG_LINUX) */
                QLIT_QDICT(((QLitDictEntry[]) {
                    { "default", QLIT_QNULL, },
                    { "name", QLIT_QSTR("x-check-cache-dropped"), },
                    { "type", QLIT_QSTR("bool"), },
                    {}
                })),
                {}
            })), },
            { "meta-type", QLIT_QSTR("object"), },
            { "name", QLIT_QSTR("267"), },
            {}
        })),

Generated C string would look like this:

        "{\"features\": ["
    #if defined(CONFIG_POSIX)
        "\"dynamic-auto-read-only\""
    #endif /* defined(CONFIG_POSIX) */
        "], "
        "\"members\": ["
        "{\"name\": \"filename\", \"type\": \"str\"}, "
        "{\"name\": \"pr-manager\", \"default\": null, \"type\": \"str\"}, "
        "{\"name\": \"locking\", \"default\": null, \"type\": \"412\"}, "
        "{\"name\": \"aio\", \"default\": null, \"type\": \"413\"}, "
    #if defined(CONFIG_LINUX)
        "{\"name\": \"drop-cache\", \"default\": null, \"type\": \"bool\"}, "
    #endif /* defined(CONFIG_LINUX) */
        "{\"name\": \"x-check-cache-dropped\", \"default\": null, \"type\": \"bool\"}"
        "], "
        "\"meta-type\": \"object\", "
        "\"name\": \"267\""
        "}"
John Snow Dec. 15, 2020, 3:52 p.m. UTC | #4
On 11/4/20 4:51 AM, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Nov 2, 2020 at 7:41 PM John Snow <jsnow@redhat.com 
> <mailto:jsnow@redhat.com>> wrote:
> 
>     On 10/26/20 3:42 PM, John Snow wrote:
>      > Hi, this series adds static type hints to the QAPI module.
>      > This is part two, and covers introspect.py.
>      >
>      > Part 2:
>     https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt2
>     <https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt2>
>      > Everything:
>     https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt6
>     <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:
>      >   - Dropped all R-B from previous series; enough has changed.
>      >   - pt2 is now introspect.py, expr.py is pushed to pt3.
>      >   - Reworked again to have less confusing (?) type names
>      >   - Added an assertion to prevent future accidental breakage
>      >
> 
>     Ping!
> 
>     Patches 1-3: Can be skipped; just enables sphinx to check the docstring
>     syntax. Don't worry about these too much, they're just here for you to
>     test with.
> 
> 
> They are interesting, but the rebase version fails. And the error 
> produced is not exactly friendly:
> Exception occurred:
>    File "/usr/lib/python3.9/site-packages/sphinx/domains/c.py", line 
> 3751, in resolve_any_xref
>      return [('c:' + self.role_for_objtype(objtype), retnode)]
> TypeError: can only concatenate str (not "NoneType") to str
> 
> Could you rebase and split off in a separate series?
> 

Done, new versions of these patches will omit these.

>     Patch 4 adds some small changes, to support:
>     Patch 5 adds the type hints.
>     Patches 6-11 try to improve the readability of the types and the code.
> 
>     This was a challenging file to clean up, so I am sure there's lots of
>     easy, low-hanging fruit in the review/feedback for me to improve.
> 
> 
> Nothing obvious to me.
> 
> Python typing is fairly new to me, and I don't know the best practices. 
> I would just take what you did and improve later, if needed.
> 

Neither do I, but I'm learning as I go.

> A wish item before we proceed with more python code cleanups is 
> documentation and/or automated tests.
> 

OK. It's on my list to write a python style guide document, I can detail 
our typing strategies, documentation strategies, etc there.

> Could you add a new make check-python and perhaps document what the new 
> code-style expectations?
> 

Yes, I have one for python/qemu that I tried to get merged prior to 5.2 
but it didn't make it in time because there were some concerns over 
exactly how the test ran and where it provisioned its packages from.

> 
>      > John Snow (11):
>      >    [DO-NOT-MERGE] docs: replace single backtick (`) with
>     double-backtick
>      >      (``)
>      >    [DO-NOT-MERGE] docs/sphinx: change default role to "any"
>      >    [DO-NOT-MERGE] docs: enable sphinx-autodoc for scripts/qapi
>      >    qapi/introspect.py: add assertions and casts
>      >    qapi/introspect.py: add preliminary type hint annotations
>      >    qapi/introspect.py: add _gen_features helper
>      >    qapi/introspect.py: Unify return type of _make_tree()
>      >    qapi/introspect.py: replace 'extra' dict with 'comment' argument
>      >    qapi/introspect.py: create a typed 'Annotated' data strutcure
>      >    qapi/introspect.py: improve readability of _tree_to_qlit
>      >    qapi/introspect.py: Add docstring to _tree_to_qlit
>      >
>      >   docs/conf.py                           |   6 +-
>      >   docs/devel/build-system.rst            | 120 +++++------
>      >   docs/devel/index.rst                   |   1 +
>      >   docs/devel/migration.rst               |  59 +++---
>      >   docs/devel/python/index.rst            |   7 +
>      >   docs/devel/python/qapi.commands.rst    |   7 +
>      >   docs/devel/python/qapi.common.rst      |   7 +
>      >   docs/devel/python/qapi.error.rst       |   7 +
>      >   docs/devel/python/qapi.events.rst      |   7 +
>      >   docs/devel/python/qapi.expr.rst        |   7 +
>      >   docs/devel/python/qapi.gen.rst         |   7 +
>      >   docs/devel/python/qapi.introspect.rst  |   7 +
>      >   docs/devel/python/qapi.main.rst        |   7 +
>      >   docs/devel/python/qapi.parser.rst      |   8 +
>      >   docs/devel/python/qapi.rst             |  26 +++
>      >   docs/devel/python/qapi.schema.rst      |   7 +
>      >   docs/devel/python/qapi.source.rst      |   7 +
>      >   docs/devel/python/qapi.types.rst       |   7 +
>      >   docs/devel/python/qapi.visit.rst       |   7 +
>      >   docs/devel/tcg-plugins.rst             |  14 +-
>      >   docs/devel/testing.rst                 |   2 +-
>      >   docs/interop/live-block-operations.rst |   4 +-
>      >   docs/system/arm/cpu-features.rst       | 110 +++++-----
>      >   docs/system/arm/nuvoton.rst            |   2 +-
>      >   docs/system/s390x/protvirt.rst         |  10 +-
>      >   qapi/block-core.json                   |   4 +-
>      >   scripts/qapi/introspect.py             | 277
>     +++++++++++++++++--------
>      >   scripts/qapi/mypy.ini                  |   5 -
>      >   scripts/qapi/schema.py                 |   2 +-
>      >   29 files changed, 487 insertions(+), 254 deletions(-)
>      >   create mode 100644 docs/devel/python/index.rst
>      >   create mode 100644 docs/devel/python/qapi.commands.rst
>      >   create mode 100644 docs/devel/python/qapi.common.rst
>      >   create mode 100644 docs/devel/python/qapi.error.rst
>      >   create mode 100644 docs/devel/python/qapi.events.rst
>      >   create mode 100644 docs/devel/python/qapi.expr.rst
>      >   create mode 100644 docs/devel/python/qapi.gen.rst
>      >   create mode 100644 docs/devel/python/qapi.introspect.rst
>      >   create mode 100644 docs/devel/python/qapi.main.rst
>      >   create mode 100644 docs/devel/python/qapi.parser.rst
>      >   create mode 100644 docs/devel/python/qapi.rst
>      >   create mode 100644 docs/devel/python/qapi.schema.rst
>      >   create mode 100644 docs/devel/python/qapi.source.rst
>      >   create mode 100644 docs/devel/python/qapi.types.rst
>      >   create mode 100644 docs/devel/python/qapi.visit.rst
>      >
> 
> 
> 
> 
> -- 
> Marc-André Lureau