Message ID | 20201026194251.11075-1-jsnow@redhat.com |
---|---|
Headers | show |
Series | qapi: static typing conversion, pt2 | expand |
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 >
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 > > > > >
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\"" "}"
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