* [PATCH v2 01/11] [DO-NOT-MERGE] docs: replace single backtick (`) with double-backtick (``)
2020-10-26 19:42 [PATCH v2 00/11] qapi: static typing conversion, pt2 John Snow
@ 2020-10-26 19:42 ` John Snow
2020-10-26 19:42 ` [PATCH v2 02/11] [DO-NOT-MERGE] docs/sphinx: change default role to "any" John Snow
` (11 subsequent siblings)
12 siblings, 0 replies; 48+ messages in thread
From: John Snow @ 2020-10-26 19:42 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster; +Cc: John Snow, Eduardo Habkost, Cleber Rosa
The single backtick in ReST is the "default role". Currently, Sphinx's
default role is called "content". Sphinx suggests you can use the "Any"
role instead to turn any single-backtick enclosed item into a
cross-reference.
Before we do that, though, we'll need to turn all existing usages of the
"content" role to inline verbatim markup by using double backticks
instead.
Signed-off-by: John Snow <jsnow@redhat.com>
---
docs/devel/build-system.rst | 120 ++++++++++++-------------
docs/devel/migration.rst | 59 ++++++------
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 +-
9 files changed, 164 insertions(+), 161 deletions(-)
diff --git a/docs/devel/build-system.rst b/docs/devel/build-system.rst
index 6fcf8854b70..eb7b7bbd3a3 100644
--- a/docs/devel/build-system.rst
+++ b/docs/devel/build-system.rst
@@ -53,14 +53,14 @@ following tasks:
- Add a Meson build option to meson_options.txt.
- Add support to the command line arg parser to handle any new
- `--enable-XXX`/`--disable-XXX` flags required by the feature.
+ ``--enable-XXX``/``--disable-XXX`` flags required by the feature.
- Add information to the help output message to report on the new
feature flag.
- Add code to perform the actual feature check.
- - Add code to include the feature status in `config-host.h`
+ - Add code to include the feature status in ``config-host.h``
- Add code to print out the feature status in the configure summary
upon completion.
@@ -116,51 +116,51 @@ Helper functions
The configure script provides a variety of helper functions to assist
developers in checking for system features:
-`do_cc $ARGS...`
+``do_cc $ARGS...``
Attempt to run the system C compiler passing it $ARGS...
-`do_cxx $ARGS...`
+``do_cxx $ARGS...``
Attempt to run the system C++ compiler passing it $ARGS...
-`compile_object $CFLAGS`
+``compile_object $CFLAGS``
Attempt to compile a test program with the system C compiler using
$CFLAGS. The test program must have been previously written to a file
- called $TMPC. The replacement in Meson is the compiler object `cc`,
- which has methods such as `cc.compiles()`,
- `cc.check_header()`, `cc.has_function()`.
+ called $TMPC. The replacement in Meson is the compiler object ``cc``,
+ which has methods such as ``cc.compiles()``,
+ ``cc.check_header()``, ``cc.has_function()``.
-`compile_prog $CFLAGS $LDFLAGS`
+``compile_prog $CFLAGS $LDFLAGS``
Attempt to compile a test program with the system C compiler using
$CFLAGS and link it with the system linker using $LDFLAGS. The test
program must have been previously written to a file called $TMPC.
- The replacement in Meson is `cc.find_library()` and `cc.links()`.
+ The replacement in Meson is ``cc.find_library()`` and ``cc.links()``.
-`has $COMMAND`
+``has $COMMAND``
Determine if $COMMAND exists in the current environment, either as a
shell builtin, or executable binary, returning 0 on success. The
- replacement in Meson is `find_program()`.
+ replacement in Meson is ``find_program()``.
-`check_define $NAME`
+``check_define $NAME``
Determine if the macro $NAME is defined by the system C compiler
-`check_include $NAME`
+``check_include $NAME``
Determine if the include $NAME file is available to the system C
- compiler. The replacement in Meson is `cc.has_header()`.
+ compiler. The replacement in Meson is ``cc.has_header()``.
-`write_c_skeleton`
+``write_c_skeleton``
Write a minimal C program main() function to the temporary file
indicated by $TMPC
-`feature_not_found $NAME $REMEDY`
+``feature_not_found $NAME $REMEDY``
Print a message to stderr that the feature $NAME was not available
on the system, suggesting the user try $REMEDY to address the
problem.
-`error_exit $MESSAGE $MORE...`
+``error_exit $MESSAGE $MORE...``
Print $MESSAGE to stderr, followed by $MORE... and then exit from the
configure script with non-zero status
-`query_pkg_config $ARGS...`
+``query_pkg_config $ARGS...``
Run pkg-config passing it $ARGS. If QEMU is doing a static build,
then --static will be automatically added to $ARGS
@@ -193,14 +193,14 @@ compilation as possible. The Meson "sourceset" functionality is used
to list the files and their dependency on various configuration
symbols.
-All executables are built by default, except for some `contrib/`
+All executables are built by default, except for some ``contrib/``
binaries that are known to fail to build on some platforms (for example
32-bit or big-endian platforms). Tests are also built by default,
though that might change in the future.
Various subsystems that are common to both tools and emulators have
-their own sourceset, for example `block_ss` for the block device subsystem,
-`chardev_ss` for the character device subsystem, etc. These sourcesets
+their own sourceset, for example ``block_ss`` for the block device subsystem,
+``chardev_ss`` for the character device subsystem, etc. These sourcesets
are then turned into static libraries as follows::
libchardev = static_library('chardev', chardev_ss.sources(),
@@ -209,8 +209,8 @@ are then turned into static libraries as follows::
chardev = declare_dependency(link_whole: libchardev)
-As of Meson 0.55.1, the special `.fa` suffix should be used for everything
-that is used with `link_whole`, to ensure that the link flags are placed
+As of Meson 0.55.1, the special ``.fa`` suffix should be used for everything
+that is used with ``link_whole``, to ensure that the link flags are placed
correctly in the command line.
Files linked into emulator targets there can be split into two distinct groups
@@ -221,25 +221,25 @@ In the target-independent set lives various general purpose helper code,
such as error handling infrastructure, standard data structures,
platform portability wrapper functions, etc. This code can be compiled
once only and the .o files linked into all output binaries.
-Target-independent code lives in the `common_ss`, `softmmu_ss` and
-`user_ss` sourcesets. `common_ss` is linked into all emulators, `softmmu_ss`
-only in system emulators, `user_ss` only in user-mode emulators.
+Target-independent code lives in the ``common_ss``, ``softmmu_ss`` and
+``user_ss`` sourcesets. ``common_ss`` is linked into all emulators, ``softmmu_ss``
+only in system emulators, ``user_ss`` only in user-mode emulators.
In the target-dependent set lives CPU emulation, device emulation and
much glue code. This sometimes also has to be compiled multiple times,
once for each target being built. Target-dependent files are included
-in the `specific_ss` sourceset.
+in the ``specific_ss`` sourceset.
-All binaries link with a static library `libqemuutil.a`, which is then
-linked to all the binaries. `libqemuutil.a` is built from several
+All binaries link with a static library ``libqemuutil.a``, which is then
+linked to all the binaries. ``libqemuutil.a`` is built from several
sourcesets; most of them however host generated code, and the only two
-of general interest are `util_ss` and `stub_ss`.
+of general interest are ``util_ss`` and ``stub_ss``.
The separation between these two is purely for documentation purposes.
-`util_ss` contains generic utility files. Even though this code is only
+``util_ss`` contains generic utility files. Even though this code is only
linked in some binaries, sometimes it requires hooks only in some of
these and depend on other functions that are not fully implemented by
-all QEMU binaries. `stub_ss` links dummy stubs that will only be linked
+all QEMU binaries. ``stub_ss`` links dummy stubs that will only be linked
into the binary if the real implementation is not present. In a way,
the stubs can be thought of as a portable implementation of the weak
symbols concept.
@@ -247,7 +247,7 @@ symbols concept.
The following files concur in the definition of which files are linked
into each emulator:
-`default-configs/*.mak`
+``default-configs/*.mak``
The files under default-configs/ control what emulated hardware is built
into each QEMU system and userspace emulator targets. They merely contain
a list of config variable definitions like the machines that should be
@@ -257,8 +257,8 @@ into each emulator:
CONFIG_XLNX_ZYNQMP_ARM=y
CONFIG_XLNX_VERSAL=y
-`*/Kconfig`
- These files are processed together with `default-configs/*.mak` and
+``*/Kconfig``
+ These files are processed together with ``default-configs/*.mak`` and
describe the dependencies between various features, subsystems and
device models. They are described in kconfig.rst.
@@ -270,19 +270,19 @@ Support scripts
---------------
Meson has a special convention for invoking Python scripts: if their
-first line is `#! /usr/bin/env python3` and the file is *not* executable,
+first line is ``#! /usr/bin/env python3`` and the file is *not* executable,
find_program() arranges to invoke the script under the same Python
interpreter that was used to invoke Meson. This is the most common
and preferred way to invoke support scripts from Meson build files,
because it automatically uses the value of configure's --python= option.
-In case the script is not written in Python, use a `#! /usr/bin/env ...`
+In case the script is not written in Python, use a ``#! /usr/bin/env ...``
line and make the script executable.
Scripts written in Python, where it is desirable to make the script
executable (for example for test scripts that developers may want to
invoke from the command line, such as tests/qapi-schema/test-qapi.py),
-should be invoked through the `python` variable in meson.build. For
+should be invoked through the ``python`` variable in meson.build. For
example::
test('QAPI schema regression tests', python,
@@ -306,10 +306,10 @@ rules and wraps them so that e.g. submodules are built before QEMU.
The resulting build system is largely non-recursive in nature, in
contrast to common practices seen with automake.
-Tests are also ran by the Makefile with the traditional `make check`
-phony target, while benchmarks are run with `make bench`. Meson test
-suites such as `unit` can be ran with `make check-unit` too. It is also
-possible to run tests defined in meson.build with `meson test`.
+Tests are also ran by the Makefile with the traditional ``make check``
+phony target, while benchmarks are run with ``make bench``. Meson test
+suites such as ``unit`` can be ran with ``make check-unit`` too. It is also
+possible to run tests defined in meson.build with ``meson test``.
Important files for the build system
====================================
@@ -321,28 +321,28 @@ The following key files are statically defined in the source tree, with
the rules needed to build QEMU. Their behaviour is influenced by a
number of dynamically created files listed later.
-`Makefile`
+``Makefile``
The main entry point used when invoking make to build all the components
of QEMU. The default 'all' target will naturally result in the build of
every component. Makefile takes care of recursively building submodules
directly via a non-recursive set of rules.
-`*/meson.build`
+``*/meson.build``
The meson.build file in the root directory is the main entry point for the
Meson build system, and it coordinates the configuration and build of all
executables. Build rules for various subdirectories are included in
other meson.build files spread throughout the QEMU source tree.
-`tests/Makefile.include`
+``tests/Makefile.include``
Rules for external test harnesses. These include the TCG tests,
- `qemu-iotests` and the Avocado-based acceptance tests.
+ ``qemu-iotests`` and the Avocado-based acceptance tests.
-`tests/docker/Makefile.include`
+``tests/docker/Makefile.include``
Rules for Docker tests. Like tests/Makefile, this file is included
directly by the top level Makefile, anything defined in this file will
influence the entire build system.
-`tests/vm/Makefile.include`
+``tests/vm/Makefile.include``
Rules for VM-based tests. Like tests/Makefile, this file is included
directly by the top level Makefile, anything defined in this file will
influence the entire build system.
@@ -358,11 +358,11 @@ Makefile.
Built by configure:
-`config-host.mak`
+``config-host.mak``
When configure has determined the characteristics of the build host it
will write a long list of variables to config-host.mak file. This
provides the various install directories, compiler / linker flags and a
- variety of `CONFIG_*` variables related to optionally enabled features.
+ variety of ``CONFIG_*`` variables related to optionally enabled features.
This is imported by the top level Makefile and meson.build in order to
tailor the build output.
@@ -374,7 +374,7 @@ Built by configure:
build outputs. Variables which are potentially different for each
emulator target are defined by the next file...
-`$TARGET-NAME/config-target.mak`
+``$TARGET-NAME/config-target.mak``
TARGET-NAME is the name of a system or userspace emulator, for example,
x86_64-softmmu denotes the system emulator for the x86_64 architecture.
This file contains the variables which need to vary on a per-target
@@ -385,29 +385,29 @@ Built by configure:
Built by Meson:
-`${TARGET-NAME}-config-devices.mak`
+``${TARGET-NAME}-config-devices.mak``
TARGET-NAME is again the name of a system or userspace emulator. The
config-devices.mak file is automatically generated by make using the
scripts/make_device_config.sh program, feeding it the
default-configs/$TARGET-NAME file as input.
-`config-host.h`, `$TARGET-NAME/config-target.h`, `$TARGET-NAME/config-devices.h`
+``config-host.h``, ``$TARGET-NAME/config-target.h``, ``$TARGET-NAME/config-devices.h``
These files are used by source code to determine what features
are enabled. They are generated from the contents of the corresponding
- `*.h` files using the scripts/create_config program. This extracts
+ ``*.h`` files using the scripts/create_config program. This extracts
relevant variables and formats them as C preprocessor macros.
-`build.ninja`
+``build.ninja``
The build rules.
Built by Makefile:
-`Makefile.ninja`
+``Makefile.ninja``
A Makefile include that bridges to ninja for the actual build. The
Makefile is mostly a list of targets that Meson included in build.ninja.
-`Makefile.mtest`
+``Makefile.mtest``
The Makefile definitions that let "make check" run tests defined in
meson.build. The rules are produced from Meson's JSON description of
tests (obtained with "meson introspect --tests") through the script
@@ -417,9 +417,9 @@ Built by Makefile:
Useful make targets
-------------------
-`help`
+``help``
Print a help message for the most common build targets.
-`print-VAR`
+``print-VAR``
Print the value of the variable VAR. Useful for debugging the build
system.
diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst
index 49112bb27aa..f50e1250359 100644
--- a/docs/devel/migration.rst
+++ b/docs/devel/migration.rst
@@ -53,7 +53,7 @@ savevm/loadvm functionality.
Debugging
=========
-The migration stream can be analyzed thanks to `scripts/analyze_migration.py`.
+The migration stream can be analyzed thanks to ``scripts/analyze_migration.py``.
Example usage:
@@ -74,8 +74,8 @@ Common infrastructure
=====================
The files, sockets or fd's that carry the migration stream are abstracted by
-the ``QEMUFile`` type (see `migration/qemu-file.h`). In most cases this
-is connected to a subtype of ``QIOChannel`` (see `io/`).
+the ``QEMUFile`` type (see ``migration/qemu-file.h``). In most cases this
+is connected to a subtype of ``QIOChannel`` (see ``io/``).
Saving the state of one device
@@ -165,14 +165,14 @@ An example (from hw/input/pckbd.c)
};
We are declaring the state with name "pckbd".
-The `version_id` is 3, and the fields are 4 uint8_t in a KBDState structure.
+The ``version_id`` is 3, and the fields are 4 uint8_t in a KBDState structure.
We registered this with:
.. code:: c
vmstate_register(NULL, 0, &vmstate_kbd, s);
-For devices that are `qdev` based, we can register the device in the class
+For devices that are ``qdev`` based, we can register the device in the class
init function:
.. code:: c
@@ -209,9 +209,9 @@ another to load the state back.
SaveVMHandlers *ops,
void *opaque);
-Two functions in the ``ops`` structure are the `save_state`
-and `load_state` functions. Notice that `load_state` receives a version_id
-parameter to know what state format is receiving. `save_state` doesn't
+Two functions in the ``ops`` structure are the ``save_state``
+and ``load_state`` functions. Notice that ``load_state`` receives a version_id
+parameter to know what state format is receiving. ``save_state`` doesn't
have a version_id parameter because it always uses the latest version.
Note that because the VMState macros still save the data in a raw
@@ -384,26 +384,28 @@ migration of a device, and using them breaks backward-migration
compatibility; in general most changes can be made by adding Subsections
(see above) or _TEST macros (see above) which won't break compatibility.
-Each version is associated with a series of fields saved. The `save_state` always saves
-the state as the newer version. But `load_state` sometimes is able to
-load state from an older version.
+Each version is associated with a series of fields saved. The
+``save_state`` always saves the state as the newer version. But
+``load_state`` sometimes is able to load state from an older version.
You can see that there are several version fields:
-- `version_id`: the maximum version_id supported by VMState for that device.
-- `minimum_version_id`: the minimum version_id that VMState is able to understand
- for that device.
-- `minimum_version_id_old`: For devices that were not able to port to vmstate, we can
- assign a function that knows how to read this old state. This field is
- ignored if there is no `load_state_old` handler.
+- ``version_id``: the maximum version_id supported by VMState for that
+ device.
+- ``minimum_version_id``: the minimum version_id that VMState is able to
+ understand for that device.
+- ``minimum_version_id_old``: For devices that were not able to port to
+ vmstate, we can assign a function that knows how to read this old
+ state. This field is ignored if there is no ``load_state_old``
+ handler.
VMState is able to read versions from minimum_version_id to
version_id. And the function ``load_state_old()`` (if present) is able to
load state from minimum_version_id_old to minimum_version_id. This
function is deprecated and will be removed when no more users are left.
-There are *_V* forms of many ``VMSTATE_`` macros to load fields for version dependent fields,
-e.g.
+There are *_V* forms of many ``VMSTATE_`` macros to load fields for
+version dependent fields, e.g.
.. code:: c
@@ -453,7 +455,7 @@ data and then transferred to the main structure.
If you use memory API functions that update memory layout outside
initialization (i.e., in response to a guest action), this is a strong
-indication that you need to call these functions in a `post_load` callback.
+indication that you need to call these functions in a ``post_load`` callback.
Examples of such memory API functions are:
- memory_region_add_subregion()
@@ -818,17 +820,18 @@ Postcopy now works with hugetlbfs backed memory:
Postcopy with shared memory
---------------------------
-Postcopy migration with shared memory needs explicit support from the other
-processes that share memory and from QEMU. There are restrictions on the type of
-memory that userfault can support shared.
+Postcopy migration with shared memory needs explicit support from the
+other processes that share memory and from QEMU. There are restrictions
+on the type of memory that userfault can support shared.
-The Linux kernel userfault support works on `/dev/shm` memory and on `hugetlbfs`
-(although the kernel doesn't provide an equivalent to `madvise(MADV_DONTNEED)`
-for hugetlbfs which may be a problem in some configurations).
+The Linux kernel userfault support works on ``/dev/shm`` memory and on
+``hugetlbfs`` (although the kernel doesn't provide an equivalent to
+``madvise(MADV_DONTNEED)`` for hugetlbfs which may be a problem in some
+configurations).
The vhost-user code in QEMU supports clients that have Postcopy support,
-and the `vhost-user-bridge` (in `tests/`) and the DPDK package have changes
-to support postcopy.
+and the ``vhost-user-bridge`` (in ``tests/``) and the DPDK package have
+changes to support postcopy.
The client needs to open a userfaultfd and register the areas
of memory that it maps with userfault. The client must then pass the
diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst
index 0568dfa6a49..5057b6e1b2b 100644
--- a/docs/devel/tcg-plugins.rst
+++ b/docs/devel/tcg-plugins.rst
@@ -34,11 +34,11 @@ version they were built against. This can be done simply by::
QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
The core code will refuse to load a plugin that doesn't export a
-`qemu_plugin_version` symbol or if plugin version is outside of QEMU's
+``qemu_plugin_version`` symbol or if plugin version is outside of QEMU's
supported range of API versions.
-Additionally the `qemu_info_t` structure which is passed to the
-`qemu_plugin_install` method of a plugin will detail the minimum and
+Additionally the ``qemu_info_t`` structure which is passed to the
+``qemu_plugin_install`` method of a plugin will detail the minimum and
current API versions supported by QEMU. The API version will be
incremented if new APIs are added. The minimum API version will be
incremented if existing APIs are changed or removed.
@@ -140,12 +140,12 @@ Example Plugins
There are a number of plugins included with QEMU and you are
encouraged to contribute your own plugins plugins upstream. There is a
-`contrib/plugins` directory where they can go.
+``contrib/plugins`` directory where they can go.
- tests/plugins
These are some basic plugins that are used to test and exercise the
-API during the `make check-tcg` target.
+API during the ``make check-tcg`` target.
- contrib/plugins/hotblocks.c
@@ -157,7 +157,7 @@ with linux-user execution as system emulation tends to generate
re-translations as blocks from different programs get swapped in and
out of system memory.
-If your program is single-threaded you can use the `inline` option for
+If your program is single-threaded you can use the ``inline`` option for
slightly faster (but not thread safe) counters.
Example::
@@ -245,7 +245,7 @@ which will lead to a sorted list after the class breakdown::
...
To find the argument shorthand for the class you need to examine the
-source code of the plugin at the moment, specifically the `*opt`
+source code of the plugin at the moment, specifically the ``*opt``
argument in the InsnClassExecCount tables.
- contrib/plugins/lockstep.c
diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 0c3e79d31cd..4ab84926cb2 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -707,7 +707,7 @@ The base test class has also support for tests with more than one
QEMUMachine. The way to get machines is through the ``self.get_vm()``
method which will return a QEMUMachine instance. The ``self.get_vm()``
method accepts arguments that will be passed to the QEMUMachine creation
-and also an optional `name` attribute so you can identify a specific
+and also an optional ``name`` attribute so you can identify a specific
machine and get it more than once through the tests methods. A simple
and hypothetical example follows:
diff --git a/docs/interop/live-block-operations.rst b/docs/interop/live-block-operations.rst
index e13f5a21f8d..5a3f0458275 100644
--- a/docs/interop/live-block-operations.rst
+++ b/docs/interop/live-block-operations.rst
@@ -638,7 +638,7 @@ at this point:
(QEMU) block-job-complete device=job0
In either of the above cases, if you once again run the
-`query-block-jobs` command, there should not be any active block
+``query-block-jobs`` command, there should not be any active block
operation.
Comparing 'commit' and 'mirror': In both then cases, the overlay images
@@ -777,7 +777,7 @@ the content of image [D].
}
(6) [On *destination* QEMU] Finally, resume the guest vCPUs by issuing the
- QMP command `cont`::
+ QMP command ``cont``::
(QEMU) cont
{
diff --git a/docs/system/arm/cpu-features.rst b/docs/system/arm/cpu-features.rst
index 35196a6b759..3622c904798 100644
--- a/docs/system/arm/cpu-features.rst
+++ b/docs/system/arm/cpu-features.rst
@@ -10,22 +10,22 @@ is the Performance Monitoring Unit (PMU). CPU types such as the
Cortex-A15 and the Cortex-A57, which respectively implement Arm
architecture reference manuals ARMv7-A and ARMv8-A, may both optionally
implement PMUs. For example, if a user wants to use a Cortex-A15 without
-a PMU, then the `-cpu` parameter should contain `pmu=off` on the QEMU
-command line, i.e. `-cpu cortex-a15,pmu=off`.
+a PMU, then the ``-cpu`` parameter should contain ``pmu=off`` on the QEMU
+command line, i.e. ``-cpu cortex-a15,pmu=off``.
As not all CPU types support all optional CPU features, then whether or
not a CPU property exists depends on the CPU type. For example, CPUs
that implement the ARMv8-A architecture reference manual may optionally
support the AArch32 CPU feature, which may be enabled by disabling the
-`aarch64` CPU property. A CPU type such as the Cortex-A15, which does
-not implement ARMv8-A, will not have the `aarch64` CPU property.
+``aarch64`` CPU property. A CPU type such as the Cortex-A15, which does
+not implement ARMv8-A, will not have the ``aarch64`` CPU property.
QEMU's support may be limited for some CPU features, only partially
supporting the feature or only supporting the feature under certain
-configurations. For example, the `aarch64` CPU feature, which, when
+configurations. For example, the ``aarch64`` CPU feature, which, when
disabled, enables the optional AArch32 CPU feature, is only supported
when using the KVM accelerator and when running on a host CPU type that
-supports the feature. While `aarch64` currently only works with KVM,
+supports the feature. While ``aarch64`` currently only works with KVM,
it could work with TCG. CPU features that are specific to KVM are
prefixed with "kvm-" and are described in "KVM VCPU Features".
@@ -33,12 +33,12 @@ CPU Feature Probing
===================
Determining which CPU features are available and functional for a given
-CPU type is possible with the `query-cpu-model-expansion` QMP command.
-Below are some examples where `scripts/qmp/qmp-shell` (see the top comment
+CPU type is possible with the ``query-cpu-model-expansion`` QMP command.
+Below are some examples where ``scripts/qmp/qmp-shell`` (see the top comment
block in the script for usage) is used to issue the QMP commands.
-1. Determine which CPU features are available for the `max` CPU type
- (Note, we started QEMU with qemu-system-aarch64, so `max` is
+1. Determine which CPU features are available for the ``max`` CPU type
+ (Note, we started QEMU with qemu-system-aarch64, so ``max`` is
implementing the ARMv8-A reference manual in this case)::
(QEMU) query-cpu-model-expansion type=full model={"name":"max"}
@@ -51,9 +51,9 @@ block in the script for usage) is used to issue the QMP commands.
"sve896": true, "sve1280": true, "sve2048": true
}}}}
-We see that the `max` CPU type has the `pmu`, `aarch64`, `sve`, and many
-`sve<N>` CPU features. We also see that all the CPU features are
-enabled, as they are all `true`. (The `sve<N>` CPU features are all
+We see that the ``max`` CPU type has the ``pmu``, ``aarch64``, ``sve``, and many
+``sve<N>`` CPU features. We also see that all the CPU features are
+enabled, as they are all ``true``. (The ``sve<N>`` CPU features are all
optional SVE vector lengths (see "SVE CPU Properties"). While with TCG
all SVE vector lengths can be supported, when KVM is in use it's more
likely that only a few lengths will be supported, if SVE is supported at
@@ -71,9 +71,9 @@ all.)
"sve896": true, "sve1280": true, "sve2048": true
}}}}
-We see it worked, as `pmu` is now `false`.
+We see it worked, as ``pmu`` is now ``false``.
-(3) Let's try to disable `aarch64`, which enables the AArch32 CPU feature::
+(3) Let's try to disable ``aarch64``, which enables the AArch32 CPU feature::
(QEMU) query-cpu-model-expansion type=full model={"name":"max","props":{"aarch64":false}}
{"error": {
@@ -84,7 +84,7 @@ We see it worked, as `pmu` is now `false`.
It looks like this feature is limited to a configuration we do not
currently have.
-(4) Let's disable `sve` and see what happens to all the optional SVE
+(4) Let's disable ``sve`` and see what happens to all the optional SVE
vector lengths::
(QEMU) query-cpu-model-expansion type=full model={"name":"max","props":{"sve":false}}
@@ -97,14 +97,14 @@ currently have.
"sve896": false, "sve1280": false, "sve2048": false
}}}}
-As expected they are now all `false`.
+As expected they are now all ``false``.
(5) Let's try probing CPU features for the Cortex-A15 CPU type::
(QEMU) query-cpu-model-expansion type=full model={"name":"cortex-a15"}
{"return": {"model": {"name": "cortex-a15", "props": {"pmu": true}}}}
-Only the `pmu` CPU feature is available.
+Only the ``pmu`` CPU feature is available.
A note about CPU feature dependencies
-------------------------------------
@@ -123,29 +123,29 @@ A note about CPU models and KVM
-------------------------------
Named CPU models generally do not work with KVM. There are a few cases
-that do work, e.g. using the named CPU model `cortex-a57` with KVM on a
-seattle host, but mostly if KVM is enabled the `host` CPU type must be
+that do work, e.g. using the named CPU model ``cortex-a57`` with KVM on a
+seattle host, but mostly if KVM is enabled the ``host`` CPU type must be
used. This means the guest is provided all the same CPU features as the
-host CPU type has. And, for this reason, the `host` CPU type should
+host CPU type has. And, for this reason, the ``host`` CPU type should
enable all CPU features that the host has by default. Indeed it's even
a bit strange to allow disabling CPU features that the host has when using
-the `host` CPU type, but in the absence of CPU models it's the best we can
+the ``host`` CPU type, but in the absence of CPU models it's the best we can
do if we want to launch guests without all the host's CPU features enabled.
-Enabling KVM also affects the `query-cpu-model-expansion` QMP command. The
+Enabling KVM also affects the ``query-cpu-model-expansion`` QMP command. The
affect is not only limited to specific features, as pointed out in example
(3) of "CPU Feature Probing", but also to which CPU types may be expanded.
-When KVM is enabled, only the `max`, `host`, and current CPU type may be
+When KVM is enabled, only the ``max``, ``host``, and current CPU type may be
expanded. This restriction is necessary as it's not possible to know all
CPU types that may work with KVM, but it does impose a small risk of users
experiencing unexpected errors. For example on a seattle, as mentioned
-above, the `cortex-a57` CPU type is also valid when KVM is enabled.
-Therefore a user could use the `host` CPU type for the current type, but
-then attempt to query `cortex-a57`, however that query will fail with our
+above, the ``cortex-a57`` CPU type is also valid when KVM is enabled.
+Therefore a user could use the ``host`` CPU type for the current type, but
+then attempt to query ``cortex-a57``, however that query will fail with our
restrictions. This shouldn't be an issue though as management layers and
-users have been preferring the `host` CPU type for use with KVM for quite
+users have been preferring the ``host`` CPU type for use with KVM for quite
some time. Additionally, if the KVM-enabled QEMU instance running on a
-seattle host is using the `cortex-a57` CPU type, then querying `cortex-a57`
+seattle host is using the ``cortex-a57`` CPU type, then querying ``cortex-a57``
will work.
Using CPU Features
@@ -158,12 +158,12 @@ QEMU command line with that CPU type::
$ qemu-system-aarch64 -M virt -cpu max,pmu=off,sve=on,sve128=on,sve256=on
The example above disables the PMU and enables the first two SVE vector
-lengths for the `max` CPU type. Note, the `sve=on` isn't actually
-necessary, because, as we observed above with our probe of the `max` CPU
-type, `sve` is already on by default. Also, based on our probe of
+lengths for the ``max`` CPU type. Note, the ``sve=on`` isn't actually
+necessary, because, as we observed above with our probe of the ``max`` CPU
+type, ``sve`` is already on by default. Also, based on our probe of
defaults, it would seem we need to disable many SVE vector lengths, rather
than only enabling the two we want. This isn't the case, because, as
-disabling many SVE vector lengths would be quite verbose, the `sve<N>` CPU
+disabling many SVE vector lengths would be quite verbose, the ``sve<N>`` CPU
properties have special semantics (see "SVE CPU Property Parsing
Semantics").
@@ -214,49 +214,49 @@ the list of KVM VCPU features and their descriptions.
SVE CPU Properties
==================
-There are two types of SVE CPU properties: `sve` and `sve<N>`. The first
-is used to enable or disable the entire SVE feature, just as the `pmu`
+There are two types of SVE CPU properties: ``sve`` and ``sve<N>``. The first
+is used to enable or disable the entire SVE feature, just as the ``pmu``
CPU property completely enables or disables the PMU. The second type
-is used to enable or disable specific vector lengths, where `N` is the
-number of bits of the length. The `sve<N>` CPU properties have special
+is used to enable or disable specific vector lengths, where ``N`` is the
+number of bits of the length. The ``sve<N>`` CPU properties have special
dependencies and constraints, see "SVE CPU Property Dependencies and
Constraints" below. Additionally, as we want all supported vector lengths
to be enabled by default, then, in order to avoid overly verbose command
-lines (command lines full of `sve<N>=off`, for all `N` not wanted), we
+lines (command lines full of ``sve<N>=off``, for all ``N`` not wanted), we
provide the parsing semantics listed in "SVE CPU Property Parsing
Semantics".
SVE CPU Property Dependencies and Constraints
---------------------------------------------
- 1) At least one vector length must be enabled when `sve` is enabled.
+ 1) At least one vector length must be enabled when ``sve`` is enabled.
- 2) If a vector length `N` is enabled, then, when KVM is enabled, all
+ 2) If a vector length ``N`` is enabled, then, when KVM is enabled, all
smaller, host supported vector lengths must also be enabled. If
KVM is not enabled, then only all the smaller, power-of-two vector
lengths must be enabled. E.g. with KVM if the host supports all
- vector lengths up to 512-bits (128, 256, 384, 512), then if `sve512`
+ vector lengths up to 512-bits (128, 256, 384, 512), then if ``sve512``
is enabled, the 128-bit vector length, 256-bit vector length, and
384-bit vector length must also be enabled. Without KVM, the 384-bit
vector length would not be required.
3) If KVM is enabled then only vector lengths that the host CPU type
support may be enabled. If SVE is not supported by the host, then
- no `sve*` properties may be enabled.
+ no ``sve*`` properties may be enabled.
SVE CPU Property Parsing Semantics
----------------------------------
- 1) If SVE is disabled (`sve=off`), then which SVE vector lengths
+ 1) If SVE is disabled (``sve=off``), then which SVE vector lengths
are enabled or disabled is irrelevant to the guest, as the entire
SVE feature is disabled and that disables all vector lengths for
- the guest. However QEMU will still track any `sve<N>` CPU
- properties provided by the user. If later an `sve=on` is provided,
- then the guest will get only the enabled lengths. If no `sve=on`
+ the guest. However QEMU will still track any ``sve<N>`` CPU
+ properties provided by the user. If later an ``sve=on`` is provided,
+ then the guest will get only the enabled lengths. If no ``sve=on``
is provided and there are explicitly enabled vector lengths, then
an error is generated.
- 2) If SVE is enabled (`sve=on`), but no `sve<N>` CPU properties are
+ 2) If SVE is enabled (``sve=on``), but no ``sve<N>`` CPU properties are
provided, then all supported vector lengths are enabled, which when
KVM is not in use means including the non-power-of-two lengths, and,
when KVM is in use, it means all vector lengths supported by the host
@@ -272,7 +272,7 @@ SVE CPU Property Parsing Semantics
constraint (2) of "SVE CPU Property Dependencies and Constraints").
5) When KVM is enabled, if the host does not support SVE, then an error
- is generated when attempting to enable any `sve*` properties (see
+ is generated when attempting to enable any ``sve*`` properties (see
constraint (3) of "SVE CPU Property Dependencies and Constraints").
6) When KVM is enabled, if the host does support SVE, then an error is
@@ -280,8 +280,8 @@ SVE CPU Property Parsing Semantics
by the host (see constraint (3) of "SVE CPU Property Dependencies and
Constraints").
- 7) If one or more `sve<N>` CPU properties are set `off`, but no `sve<N>`,
- CPU properties are set `on`, then the specified vector lengths are
+ 7) If one or more ``sve<N>`` CPU properties are set ``off``, but no ``sve<N>``,
+ CPU properties are set ``on``, then the specified vector lengths are
disabled but the default for any unspecified lengths remains enabled.
When KVM is not enabled, disabling a power-of-two vector length also
disables all vector lengths larger than the power-of-two length.
@@ -289,15 +289,15 @@ SVE CPU Property Parsing Semantics
disables all larger vector lengths (see constraint (2) of "SVE CPU
Property Dependencies and Constraints").
- 8) If one or more `sve<N>` CPU properties are set to `on`, then they
+ 8) If one or more ``sve<N>`` CPU properties are set to ``on``, then they
are enabled and all unspecified lengths default to disabled, except
for the required lengths per constraint (2) of "SVE CPU Property
Dependencies and Constraints", which will even be auto-enabled if
they were not explicitly enabled.
- 9) If SVE was disabled (`sve=off`), allowing all vector lengths to be
+ 9) If SVE was disabled (``sve=off``), allowing all vector lengths to be
explicitly disabled (i.e. avoiding the error specified in (3) of
- "SVE CPU Property Parsing Semantics"), then if later an `sve=on` is
+ "SVE CPU Property Parsing Semantics"), then if later an ``sve=on`` is
provided an error will be generated. To avoid this error, one must
enable at least one vector length prior to enabling SVE.
@@ -308,12 +308,12 @@ SVE CPU Property Examples
$ qemu-system-aarch64 -M virt -cpu max,sve=off
- 2) Implicitly enable all vector lengths for the `max` CPU type::
+ 2) Implicitly enable all vector lengths for the ``max`` CPU type::
$ qemu-system-aarch64 -M virt -cpu max
3) When KVM is enabled, implicitly enable all host CPU supported vector
- lengths with the `host` CPU type::
+ lengths with the ``host`` CPU type::
$ qemu-system-aarch64 -M virt,accel=kvm -cpu host
diff --git a/docs/system/arm/nuvoton.rst b/docs/system/arm/nuvoton.rst
index e3e1a3a3a73..b0a35b54bd4 100644
--- a/docs/system/arm/nuvoton.rst
+++ b/docs/system/arm/nuvoton.rst
@@ -77,7 +77,7 @@ Boot options
------------
The Nuvoton machines can boot from an OpenBMC firmware image, or directly into
-a kernel using the ``-kernel`` option. OpenBMC images for `quanta-gsj` and
+a kernel using the ``-kernel`` option. OpenBMC images for ``quanta-gsj`` and
possibly others can be downloaded from the OpenPOWER jenkins :
https://openpower.xyz/
diff --git a/docs/system/s390x/protvirt.rst b/docs/system/s390x/protvirt.rst
index 712974ad87b..d208c12a962 100644
--- a/docs/system/s390x/protvirt.rst
+++ b/docs/system/s390x/protvirt.rst
@@ -14,11 +14,11 @@ Prerequisites
To run PVMs, a machine with the Protected Virtualization feature, as
indicated by the Ultravisor Call facility (stfle bit 158), is
required. The Ultravisor needs to be initialized at boot by setting
-`prot_virt=1` on the host's kernel command line.
+``prot_virt=1`` on the host's kernel command line.
Running PVMs requires using the KVM hypervisor.
-If those requirements are met, the capability `KVM_CAP_S390_PROTECTED`
+If those requirements are met, the capability ``KVM_CAP_S390_PROTECTED``
will indicate that KVM can support PVMs on that LPAR.
@@ -26,8 +26,8 @@ QEMU Settings
-------------
To indicate to the VM that it can transition into protected mode, the
-`Unpack facility` (stfle bit 161 represented by the feature
-`unpack`/`S390_FEAT_UNPACK`) needs to be part of the cpu model of
+``Unpack facility`` (stfle bit 161 represented by the feature
+``unpack``/``S390_FEAT_UNPACK``) needs to be part of the cpu model of
the VM.
All I/O devices need to use the IOMMU.
@@ -56,5 +56,5 @@ from the disk boot. This memory layout includes the encrypted
components (kernel, initrd, cmdline), the stage3a loader and
metadata. In case this boot method is used, the command line
options -initrd and -cmdline are ineffective. The preparation of a PVM
-image is done via the `genprotimg` tool from the s390-tools
+image is done via the ``genprotimg`` tool from the s390-tools
collection.
diff --git a/qapi/block-core.json b/qapi/block-core.json
index e00fc27b5ea..4ee81d226a9 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -498,11 +498,11 @@
# @status: current status of the dirty bitmap (since 2.4)
#
# @recording: true if the bitmap is recording new writes from the guest.
-# Replaces `active` and `disabled` statuses. (since 4.0)
+# Replaces ``active`` and ``disabled`` statuses. (since 4.0)
#
# @busy: true if the bitmap is in-use by some operation (NBD or jobs)
# and cannot be modified via QMP or used by another operation.
-# Replaces `locked` and `frozen` statuses. (since 4.0)
+# Replaces ``locked`` and ``frozen`` statuses. (since 4.0)
#
# @persistent: true if the bitmap was stored on disk, is scheduled to be stored
# on disk, or both. (since 4.0)
--
2.26.2
^ permalink raw reply related [flat|nested] 48+ messages in thread* [PATCH v2 02/11] [DO-NOT-MERGE] docs/sphinx: change default role to "any"
2020-10-26 19:42 [PATCH v2 00/11] qapi: static typing conversion, pt2 John Snow
2020-10-26 19:42 ` [PATCH v2 01/11] [DO-NOT-MERGE] docs: replace single backtick (`) with double-backtick (``) John Snow
@ 2020-10-26 19:42 ` John Snow
2020-10-26 19:42 ` [PATCH v2 03/11] [DO-NOT-MERGE] docs: enable sphinx-autodoc for scripts/qapi John Snow
` (10 subsequent siblings)
12 siblings, 0 replies; 48+ messages in thread
From: John Snow @ 2020-10-26 19:42 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster; +Cc: John Snow, Eduardo Habkost, Cleber Rosa
This interprets single-backtick syntax in all of our Sphinx docs as a
cross-reference to *something*, including Python symbols.
Signed-off-by: John Snow <jsnow@redhat.com>
---
docs/conf.py | 3 +++
1 file changed, 3 insertions(+)
diff --git a/docs/conf.py b/docs/conf.py
index e584f683938..bbc71ebdc36 100644
--- a/docs/conf.py
+++ b/docs/conf.py
@@ -84,6 +84,9 @@
# The master toctree document.
master_doc = 'index'
+# Interpret `this` to be a cross-reference to "anything".
+default_role = 'any'
+
# General information about the project.
project = u'QEMU'
copyright = u'2020, The QEMU Project Developers'
--
2.26.2
^ permalink raw reply related [flat|nested] 48+ messages in thread* [PATCH v2 03/11] [DO-NOT-MERGE] docs: enable sphinx-autodoc for scripts/qapi
2020-10-26 19:42 [PATCH v2 00/11] qapi: static typing conversion, pt2 John Snow
2020-10-26 19:42 ` [PATCH v2 01/11] [DO-NOT-MERGE] docs: replace single backtick (`) with double-backtick (``) John Snow
2020-10-26 19:42 ` [PATCH v2 02/11] [DO-NOT-MERGE] docs/sphinx: change default role to "any" John Snow
@ 2020-10-26 19:42 ` John Snow
2020-10-26 19:42 ` [PATCH v2 04/11] qapi/introspect.py: add assertions and casts John Snow
` (9 subsequent siblings)
12 siblings, 0 replies; 48+ messages in thread
From: John Snow @ 2020-10-26 19:42 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster; +Cc: John Snow, Eduardo Habkost, Cleber Rosa
This is just POC to prove that the docstrings, where they are written,
are correct to some minimum standard. It is included here for
reviewing/testing convenience.
Signed-off-by: John Snow <jsnow@redhat.com>
---
docs/conf.py | 3 ++-
docs/devel/index.rst | 1 +
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 +++++++
17 files changed, 128 insertions(+), 1 deletion(-)
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
diff --git a/docs/conf.py b/docs/conf.py
index bbc71ebdc36..9cbaecfb669 100644
--- a/docs/conf.py
+++ b/docs/conf.py
@@ -70,7 +70,8 @@
# Add any Sphinx extension module names here, as strings. They can be
# extensions coming with Sphinx (named 'sphinx.ext.*') or your custom
# ones.
-extensions = ['kerneldoc', 'qmp_lexer', 'hxtool', 'depfile', 'qapidoc']
+extensions = ['kerneldoc', 'qmp_lexer', 'hxtool',
+ 'depfile', 'qapidoc', 'sphinx.ext.autodoc']
# Add any paths that contain templates here, relative to this directory.
templates_path = ['_templates']
diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index 77baae5c771..7c5c94997e8 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -34,3 +34,4 @@ Contents:
clocks
qom
block-coroutine-wrapper
+ python/index
diff --git a/docs/devel/python/index.rst b/docs/devel/python/index.rst
new file mode 100644
index 00000000000..31c470154b3
--- /dev/null
+++ b/docs/devel/python/index.rst
@@ -0,0 +1,7 @@
+qapi
+====
+
+.. toctree::
+ :maxdepth: 4
+
+ qapi
diff --git a/docs/devel/python/qapi.commands.rst b/docs/devel/python/qapi.commands.rst
new file mode 100644
index 00000000000..018f7b08a9c
--- /dev/null
+++ b/docs/devel/python/qapi.commands.rst
@@ -0,0 +1,7 @@
+qapi.commands module
+====================
+
+.. automodule:: qapi.commands
+ :members:
+ :undoc-members:
+ :show-inheritance:
diff --git a/docs/devel/python/qapi.common.rst b/docs/devel/python/qapi.common.rst
new file mode 100644
index 00000000000..128a90d74be
--- /dev/null
+++ b/docs/devel/python/qapi.common.rst
@@ -0,0 +1,7 @@
+qapi.common module
+==================
+
+.. automodule:: qapi.common
+ :members:
+ :undoc-members:
+ :show-inheritance:
diff --git a/docs/devel/python/qapi.error.rst b/docs/devel/python/qapi.error.rst
new file mode 100644
index 00000000000..980e32b63de
--- /dev/null
+++ b/docs/devel/python/qapi.error.rst
@@ -0,0 +1,7 @@
+qapi.error module
+=================
+
+.. automodule:: qapi.error
+ :members:
+ :undoc-members:
+ :show-inheritance:
diff --git a/docs/devel/python/qapi.events.rst b/docs/devel/python/qapi.events.rst
new file mode 100644
index 00000000000..1fce85b044e
--- /dev/null
+++ b/docs/devel/python/qapi.events.rst
@@ -0,0 +1,7 @@
+qapi.events module
+==================
+
+.. automodule:: qapi.events
+ :members:
+ :undoc-members:
+ :show-inheritance:
diff --git a/docs/devel/python/qapi.expr.rst b/docs/devel/python/qapi.expr.rst
new file mode 100644
index 00000000000..0660270629c
--- /dev/null
+++ b/docs/devel/python/qapi.expr.rst
@@ -0,0 +1,7 @@
+qapi.expr module
+================
+
+.. automodule:: qapi.expr
+ :members:
+ :undoc-members:
+ :show-inheritance:
diff --git a/docs/devel/python/qapi.gen.rst b/docs/devel/python/qapi.gen.rst
new file mode 100644
index 00000000000..7b495fd4bf2
--- /dev/null
+++ b/docs/devel/python/qapi.gen.rst
@@ -0,0 +1,7 @@
+qapi.gen module
+===============
+
+.. automodule:: qapi.gen
+ :members:
+ :undoc-members:
+ :show-inheritance:
diff --git a/docs/devel/python/qapi.introspect.rst b/docs/devel/python/qapi.introspect.rst
new file mode 100644
index 00000000000..f65ebfccd1b
--- /dev/null
+++ b/docs/devel/python/qapi.introspect.rst
@@ -0,0 +1,7 @@
+qapi.introspect module
+======================
+
+.. automodule:: qapi.introspect
+ :members:
+ :undoc-members:
+ :show-inheritance:
diff --git a/docs/devel/python/qapi.main.rst b/docs/devel/python/qapi.main.rst
new file mode 100644
index 00000000000..1255fcda633
--- /dev/null
+++ b/docs/devel/python/qapi.main.rst
@@ -0,0 +1,7 @@
+qapi.main module
+================
+
+.. automodule:: qapi.main
+ :members:
+ :undoc-members:
+ :show-inheritance:
diff --git a/docs/devel/python/qapi.parser.rst b/docs/devel/python/qapi.parser.rst
new file mode 100644
index 00000000000..1a8f7b347eb
--- /dev/null
+++ b/docs/devel/python/qapi.parser.rst
@@ -0,0 +1,8 @@
+qapi.parser module
+==================
+
+.. automodule:: qapi.parser
+ :members:
+ :undoc-members:
+ :show-inheritance:
+ :private-members:
diff --git a/docs/devel/python/qapi.rst b/docs/devel/python/qapi.rst
new file mode 100644
index 00000000000..c762019aad3
--- /dev/null
+++ b/docs/devel/python/qapi.rst
@@ -0,0 +1,26 @@
+qapi package
+============
+
+.. automodule:: qapi
+ :members:
+ :undoc-members:
+ :show-inheritance:
+
+Submodules
+----------
+
+.. toctree::
+
+ qapi.commands
+ qapi.common
+ qapi.error
+ qapi.events
+ qapi.expr
+ qapi.gen
+ qapi.introspect
+ qapi.main
+ qapi.parser
+ qapi.schema
+ qapi.source
+ qapi.types
+ qapi.visit
diff --git a/docs/devel/python/qapi.schema.rst b/docs/devel/python/qapi.schema.rst
new file mode 100644
index 00000000000..a08f75ed720
--- /dev/null
+++ b/docs/devel/python/qapi.schema.rst
@@ -0,0 +1,7 @@
+qapi.schema module
+==================
+
+.. automodule:: qapi.schema
+ :members:
+ :undoc-members:
+ :show-inheritance:
diff --git a/docs/devel/python/qapi.source.rst b/docs/devel/python/qapi.source.rst
new file mode 100644
index 00000000000..e61e9f60212
--- /dev/null
+++ b/docs/devel/python/qapi.source.rst
@@ -0,0 +1,7 @@
+qapi.source module
+==================
+
+.. automodule:: qapi.source
+ :members:
+ :undoc-members:
+ :show-inheritance:
diff --git a/docs/devel/python/qapi.types.rst b/docs/devel/python/qapi.types.rst
new file mode 100644
index 00000000000..6eea827557d
--- /dev/null
+++ b/docs/devel/python/qapi.types.rst
@@ -0,0 +1,7 @@
+qapi.types module
+=================
+
+.. automodule:: qapi.types
+ :members:
+ :undoc-members:
+ :show-inheritance:
diff --git a/docs/devel/python/qapi.visit.rst b/docs/devel/python/qapi.visit.rst
new file mode 100644
index 00000000000..84307cbc236
--- /dev/null
+++ b/docs/devel/python/qapi.visit.rst
@@ -0,0 +1,7 @@
+qapi.visit module
+=================
+
+.. automodule:: qapi.visit
+ :members:
+ :undoc-members:
+ :show-inheritance:
--
2.26.2
^ permalink raw reply related [flat|nested] 48+ messages in thread* [PATCH v2 04/11] qapi/introspect.py: add assertions and casts
2020-10-26 19:42 [PATCH v2 00/11] qapi: static typing conversion, pt2 John Snow
` (2 preceding siblings ...)
2020-10-26 19:42 ` [PATCH v2 03/11] [DO-NOT-MERGE] docs: enable sphinx-autodoc for scripts/qapi John Snow
@ 2020-10-26 19:42 ` John Snow
2020-11-06 18:59 ` Cleber Rosa
2020-10-26 19:42 ` [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations John Snow
` (8 subsequent siblings)
12 siblings, 1 reply; 48+ messages in thread
From: John Snow @ 2020-10-26 19:42 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster; +Cc: John Snow, Eduardo Habkost, Cleber Rosa
This is necessary to keep mypy passing in the next patch when we add
preliminary type hints. It will be removed shortly.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index fafec94e022..63f721ebfb6 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -10,6 +10,8 @@
See the COPYING file in the top-level directory.
"""
+from typing import Optional, Sequence, cast
+
from .common import (
c_name,
gen_endif,
@@ -30,6 +32,7 @@ def _make_tree(obj, ifcond, features, extra=None):
if ifcond:
extra['if'] = ifcond
if features:
+ assert isinstance(obj, dict)
obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]
if extra:
return (obj, extra)
@@ -43,7 +46,7 @@ def indent(level):
if isinstance(obj, tuple):
ifobj, extra = obj
- ifcond = extra.get('if')
+ ifcond = cast(Optional[Sequence[str]], extra.get('if'))
comment = extra.get('comment')
ret = ''
if comment:
--
2.26.2
^ permalink raw reply related [flat|nested] 48+ messages in thread* [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations
2020-10-26 19:42 [PATCH v2 00/11] qapi: static typing conversion, pt2 John Snow
` (3 preceding siblings ...)
2020-10-26 19:42 ` [PATCH v2 04/11] qapi/introspect.py: add assertions and casts John Snow
@ 2020-10-26 19:42 ` John Snow
2020-11-07 2:12 ` Cleber Rosa
2020-11-13 16:48 ` Markus Armbruster
2020-10-26 19:42 ` [PATCH v2 06/11] qapi/introspect.py: add _gen_features helper John Snow
` (7 subsequent siblings)
12 siblings, 2 replies; 48+ messages in thread
From: John Snow @ 2020-10-26 19:42 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster; +Cc: John Snow, Eduardo Habkost, Cleber Rosa
The typing of _make_tree and friends is a bit involved, but it can be
done with some stubbed out types and a bit of elbow grease. The
forthcoming patches attempt to make some simplifications, but having the
type hints in advance may aid in review of subsequent patches.
Some notes on the abstract types used at this point, and what they
represent:
- TreeValue represents any object in the type tree. _make_tree is an
optional call -- not every node in the final type tree will have been
passed to _make_tree, so this type encompasses not only what is passed
to _make_tree (dicts, strings) or returned from it (dicts, strings, a
2-tuple), but any recursive value for any of the dicts passed to
_make_tree -- which includes lists, strings, integers, null constants,
and so on.
- _DObject is a type alias I use to mean "A JSON-style object,
represented as a Python dict." There is no "JSON" type in Python, they
are converted natively to recursively nested dicts and lists, with
leaf values of str, int, float, None, True/False and so on. This type
structure is not possible to accurately portray in mypy yet, so a
placeholder is used.
In this case, _DObject is being used to refer to SchemaInfo-like
structures as defined in qapi/introspect.json, OR any sub-object
values they may reference. We don't have strong typing available for
those, so a generic alternative is used.
- Extra refers explicitly to the dict containing "extra" information
about a node in the tree. mypy does not offer per-key typing for dicts
in Python 3.6, so this is the best we can do here.
- Annotated refers to (one of) the return types of _make_tree:
It represents a 2-tuple of (TreeValue, Extra).
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 157 ++++++++++++++++++++++++++++---------
scripts/qapi/mypy.ini | 5 --
scripts/qapi/schema.py | 2 +-
3 files changed, 121 insertions(+), 43 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 63f721ebfb6..803288a64e7 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -10,7 +10,16 @@
See the COPYING file in the top-level directory.
"""
-from typing import Optional, Sequence, cast
+from typing import (
+ Any,
+ Dict,
+ List,
+ Optional,
+ Sequence,
+ Tuple,
+ Union,
+ cast,
+)
from .common import (
c_name,
@@ -20,13 +29,56 @@
)
from .gen import QAPISchemaMonolithicCVisitor
from .schema import (
+ QAPISchema,
QAPISchemaArrayType,
QAPISchemaBuiltinType,
+ QAPISchemaEntity,
+ QAPISchemaEnumMember,
+ QAPISchemaFeature,
+ QAPISchemaObjectType,
+ QAPISchemaObjectTypeMember,
QAPISchemaType,
+ QAPISchemaVariant,
+ QAPISchemaVariants,
)
+from .source import QAPISourceInfo
-def _make_tree(obj, ifcond, features, extra=None):
+# This module constructs a tree-like data structure that is used to
+# generate the introspection information for QEMU. It behaves similarly
+# to a JSON value.
+#
+# A complexity over JSON is that our values may or may not be annotated.
+#
+# Un-annotated values may be:
+# Scalar: str, bool, None.
+# Non-scalar: List, Dict
+# _Value = Union[str, bool, None, Dict[str, Value], List[Value]]
+#
+# With optional annotations, the type of all values is:
+# TreeValue = Union[_Value, Annotated[_Value]]
+#
+# Sadly, mypy does not support recursive types, so we must approximate this.
+_stub = Any
+_scalar = Union[str, bool, None]
+_nonscalar = Union[Dict[str, _stub], List[_stub]]
+_value = Union[_scalar, _nonscalar]
+TreeValue = Union[_value, 'Annotated']
+
+# This is just an alias for an object in the structure described above:
+_DObject = Dict[str, object]
+
+# Represents the annotations themselves:
+Annotations = Dict[str, object]
+
+# Represents an annotated node (of some kind).
+Annotated = Tuple[_value, Annotations]
+
+
+def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
+ features: List[QAPISchemaFeature],
+ extra: Optional[Annotations] = None
+ ) -> TreeValue:
if extra is None:
extra = {}
if ifcond:
@@ -39,9 +91,11 @@ def _make_tree(obj, ifcond, features, extra=None):
return obj
-def _tree_to_qlit(obj, level=0, suppress_first_indent=False):
+def _tree_to_qlit(obj: TreeValue,
+ level: int = 0,
+ suppress_first_indent: bool = False) -> str:
- def indent(level):
+ def indent(level: int) -> str:
return level * 4 * ' '
if isinstance(obj, tuple):
@@ -91,21 +145,20 @@ def indent(level):
return ret
-def to_c_string(string):
+def to_c_string(string: str) -> str:
return '"' + string.replace('\\', r'\\').replace('"', r'\"') + '"'
class QAPISchemaGenIntrospectVisitor(QAPISchemaMonolithicCVisitor):
-
- def __init__(self, prefix, unmask):
+ def __init__(self, prefix: str, unmask: bool):
super().__init__(
prefix, 'qapi-introspect',
' * QAPI/QMP schema introspection', __doc__)
self._unmask = unmask
- self._schema = None
- self._trees = []
- self._used_types = []
- self._name_map = {}
+ self._schema: Optional[QAPISchema] = None
+ self._trees: List[TreeValue] = []
+ self._used_types: List[QAPISchemaType] = []
+ self._name_map: Dict[str, str] = {}
self._genc.add(mcgen('''
#include "qemu/osdep.h"
#include "%(prefix)sqapi-introspect.h"
@@ -113,10 +166,10 @@ def __init__(self, prefix, unmask):
''',
prefix=prefix))
- def visit_begin(self, schema):
+ def visit_begin(self, schema: QAPISchema) -> None:
self._schema = schema
- def visit_end(self):
+ def visit_end(self) -> None:
# visit the types that are actually used
for typ in self._used_types:
typ.visit(self)
@@ -138,18 +191,18 @@ def visit_end(self):
self._used_types = []
self._name_map = {}
- def visit_needed(self, entity):
+ def visit_needed(self, entity: QAPISchemaEntity) -> bool:
# Ignore types on first pass; visit_end() will pick up used types
return not isinstance(entity, QAPISchemaType)
- def _name(self, name):
+ def _name(self, name: str) -> str:
if self._unmask:
return name
if name not in self._name_map:
self._name_map[name] = '%d' % len(self._name_map)
return self._name_map[name]
- def _use_type(self, typ):
+ def _use_type(self, typ: QAPISchemaType) -> str:
# Map the various integer types to plain int
if typ.json_type() == 'int':
typ = self._schema.lookup_type('int')
@@ -168,8 +221,10 @@ def _use_type(self, typ):
return '[' + self._use_type(typ.element_type) + ']'
return self._name(typ.name)
- def _gen_tree(self, name, mtype, obj, ifcond, features):
- extra = None
+ def _gen_tree(self, name: str, mtype: str, obj: _DObject,
+ ifcond: List[str],
+ features: Optional[List[QAPISchemaFeature]]) -> None:
+ extra: Optional[Annotations] = None
if mtype not in ('command', 'event', 'builtin', 'array'):
if not self._unmask:
# Output a comment to make it easy to map masked names
@@ -180,44 +235,64 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
obj['meta-type'] = mtype
self._trees.append(_make_tree(obj, ifcond, features, extra))
- def _gen_member(self, member):
- obj = {'name': member.name, 'type': self._use_type(member.type)}
+ def _gen_member(self,
+ member: QAPISchemaObjectTypeMember) -> TreeValue:
+ obj: _DObject = {
+ 'name': member.name,
+ 'type': self._use_type(member.type)
+ }
if member.optional:
obj['default'] = None
return _make_tree(obj, member.ifcond, member.features)
- def _gen_variants(self, tag_name, variants):
+ def _gen_variants(self, tag_name: str,
+ variants: List[QAPISchemaVariant]) -> _DObject:
return {'tag': tag_name,
'variants': [self._gen_variant(v) for v in variants]}
- def _gen_variant(self, variant):
- obj = {'case': variant.name, 'type': self._use_type(variant.type)}
+ def _gen_variant(self, variant: QAPISchemaVariant) -> TreeValue:
+ obj: _DObject = {
+ 'case': variant.name,
+ 'type': self._use_type(variant.type)
+ }
return _make_tree(obj, variant.ifcond, None)
- def visit_builtin_type(self, name, info, json_type):
+ def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
+ json_type: str) -> None:
self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
- def visit_enum_type(self, name, info, ifcond, features, members, prefix):
+ def visit_enum_type(self, name: str, info: QAPISourceInfo,
+ ifcond: List[str], features: List[QAPISchemaFeature],
+ members: List[QAPISchemaEnumMember],
+ prefix: Optional[str]) -> None:
self._gen_tree(name, 'enum',
{'values': [_make_tree(m.name, m.ifcond, None)
for m in members]},
ifcond, features)
- def visit_array_type(self, name, info, ifcond, element_type):
+ def visit_array_type(self, name: str, info: Optional[QAPISourceInfo],
+ ifcond: List[str],
+ element_type: QAPISchemaType) -> None:
element = self._use_type(element_type)
self._gen_tree('[' + element + ']', 'array', {'element-type': element},
ifcond, None)
- def visit_object_type_flat(self, name, info, ifcond, features,
- members, variants):
- obj = {'members': [self._gen_member(m) for m in members]}
+ def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
+ ifcond: List[str],
+ features: List[QAPISchemaFeature],
+ members: Sequence[QAPISchemaObjectTypeMember],
+ variants: Optional[QAPISchemaVariants]) -> None:
+ obj: _DObject = {'members': [self._gen_member(m) for m in members]}
if variants:
obj.update(self._gen_variants(variants.tag_member.name,
variants.variants))
self._gen_tree(name, 'object', obj, ifcond, features)
- def visit_alternate_type(self, name, info, ifcond, features, variants):
+ def visit_alternate_type(self, name: str, info: QAPISourceInfo,
+ ifcond: List[str],
+ features: List[QAPISchemaFeature],
+ variants: QAPISchemaVariants) -> None:
self._gen_tree(name, 'alternate',
{'members': [
_make_tree({'type': self._use_type(m.type)},
@@ -225,24 +300,32 @@ def visit_alternate_type(self, name, info, ifcond, features, variants):
for m in variants.variants]},
ifcond, features)
- def visit_command(self, name, info, ifcond, features,
- arg_type, ret_type, gen, success_response, boxed,
- allow_oob, allow_preconfig, coroutine):
+ def visit_command(self, name: str, info: QAPISourceInfo, ifcond: List[str],
+ features: List[QAPISchemaFeature],
+ arg_type: QAPISchemaObjectType,
+ ret_type: Optional[QAPISchemaType], gen: bool,
+ success_response: bool, boxed: bool, allow_oob: bool,
+ allow_preconfig: bool, coroutine: bool) -> None:
arg_type = arg_type or self._schema.the_empty_object_type
ret_type = ret_type or self._schema.the_empty_object_type
- obj = {'arg-type': self._use_type(arg_type),
- 'ret-type': self._use_type(ret_type)}
+ obj: _DObject = {
+ 'arg-type': self._use_type(arg_type),
+ 'ret-type': self._use_type(ret_type)
+ }
if allow_oob:
obj['allow-oob'] = allow_oob
self._gen_tree(name, 'command', obj, ifcond, features)
- def visit_event(self, name, info, ifcond, features, arg_type, boxed):
+ def visit_event(self, name: str, info: QAPISourceInfo,
+ ifcond: List[str], features: List[QAPISchemaFeature],
+ arg_type: QAPISchemaObjectType, boxed: bool) -> None:
arg_type = arg_type or self._schema.the_empty_object_type
self._gen_tree(name, 'event', {'arg-type': self._use_type(arg_type)},
ifcond, features)
-def gen_introspect(schema, output_dir, prefix, opt_unmask):
+def gen_introspect(schema: QAPISchema, output_dir: str, prefix: str,
+ opt_unmask: bool) -> None:
vis = QAPISchemaGenIntrospectVisitor(prefix, opt_unmask)
schema.visit(vis)
vis.write(output_dir)
diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
index 74fc6c82153..c0f2a58306d 100644
--- a/scripts/qapi/mypy.ini
+++ b/scripts/qapi/mypy.ini
@@ -14,11 +14,6 @@ disallow_untyped_defs = False
disallow_incomplete_defs = False
check_untyped_defs = False
-[mypy-qapi.introspect]
-disallow_untyped_defs = False
-disallow_incomplete_defs = False
-check_untyped_defs = False
-
[mypy-qapi.parser]
disallow_untyped_defs = False
disallow_incomplete_defs = False
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 720449feee4..e91b77fadc3 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -28,7 +28,7 @@
class QAPISchemaEntity:
meta: Optional[str] = None
- def __init__(self, name, info, doc, ifcond=None, features=None):
+ def __init__(self, name: str, info, doc, ifcond=None, features=None):
assert name is None or isinstance(name, str)
for f in features or []:
assert isinstance(f, QAPISchemaFeature)
--
2.26.2
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations
2020-10-26 19:42 ` [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations John Snow
@ 2020-11-07 2:12 ` Cleber Rosa
2020-12-07 21:29 ` John Snow
2020-11-13 16:48 ` Markus Armbruster
1 sibling, 1 reply; 48+ messages in thread
From: Cleber Rosa @ 2020-11-07 2:12 UTC (permalink / raw)
To: John Snow; +Cc: qemu-devel, Eduardo Habkost, Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 4363 bytes --]
On Mon, Oct 26, 2020 at 03:42:45PM -0400, John Snow wrote:
> The typing of _make_tree and friends is a bit involved, but it can be
> done with some stubbed out types and a bit of elbow grease. The
> forthcoming patches attempt to make some simplifications, but having the
> type hints in advance may aid in review of subsequent patches.
>
>
> Some notes on the abstract types used at this point, and what they
> represent:
>
> - TreeValue represents any object in the type tree. _make_tree is an
> optional call -- not every node in the final type tree will have been
> passed to _make_tree, so this type encompasses not only what is passed
> to _make_tree (dicts, strings) or returned from it (dicts, strings, a
> 2-tuple), but any recursive value for any of the dicts passed to
> _make_tree -- which includes lists, strings, integers, null constants,
> and so on.
>
> - _DObject is a type alias I use to mean "A JSON-style object,
> represented as a Python dict." There is no "JSON" type in Python, they
> are converted natively to recursively nested dicts and lists, with
> leaf values of str, int, float, None, True/False and so on. This type
> structure is not possible to accurately portray in mypy yet, so a
> placeholder is used.
>
> In this case, _DObject is being used to refer to SchemaInfo-like
> structures as defined in qapi/introspect.json, OR any sub-object
> values they may reference. We don't have strong typing available for
> those, so a generic alternative is used.
>
> - Extra refers explicitly to the dict containing "extra" information
> about a node in the tree. mypy does not offer per-key typing for dicts
> in Python 3.6, so this is the best we can do here.
>
> - Annotated refers to (one of) the return types of _make_tree:
> It represents a 2-tuple of (TreeValue, Extra).
>
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/introspect.py | 157 ++++++++++++++++++++++++++++---------
> scripts/qapi/mypy.ini | 5 --
> scripts/qapi/schema.py | 2 +-
> 3 files changed, 121 insertions(+), 43 deletions(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 63f721ebfb6..803288a64e7 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -10,7 +10,16 @@
> See the COPYING file in the top-level directory.
> """
>
> -from typing import Optional, Sequence, cast
> +from typing import (
> + Any,
> + Dict,
> + List,
> + Optional,
> + Sequence,
> + Tuple,
> + Union,
> + cast,
> +)
>
> from .common import (
> c_name,
> @@ -20,13 +29,56 @@
> )
> from .gen import QAPISchemaMonolithicCVisitor
> from .schema import (
> + QAPISchema,
> QAPISchemaArrayType,
> QAPISchemaBuiltinType,
> + QAPISchemaEntity,
> + QAPISchemaEnumMember,
> + QAPISchemaFeature,
> + QAPISchemaObjectType,
> + QAPISchemaObjectTypeMember,
> QAPISchemaType,
> + QAPISchemaVariant,
> + QAPISchemaVariants,
> )
> +from .source import QAPISourceInfo
>
>
> -def _make_tree(obj, ifcond, features, extra=None):
> +# This module constructs a tree-like data structure that is used to
> +# generate the introspection information for QEMU. It behaves similarly
> +# to a JSON value.
> +#
> +# A complexity over JSON is that our values may or may not be annotated.
> +#
> +# Un-annotated values may be:
> +# Scalar: str, bool, None.
> +# Non-scalar: List, Dict
> +# _Value = Union[str, bool, None, Dict[str, Value], List[Value]]
Here (and in a few other places) you mention `_Value`, but then define it as
`_value` (lowercase).
> +#
> +# With optional annotations, the type of all values is:
> +# TreeValue = Union[_Value, Annotated[_Value]]
> +#
> +# Sadly, mypy does not support recursive types, so we must approximate this.
> +_stub = Any
> +_scalar = Union[str, bool, None]
> +_nonscalar = Union[Dict[str, _stub], List[_stub]]
Why not use `Any` here instead of declaring/using `_stub`?
> +_value = Union[_scalar, _nonscalar]
> +TreeValue = Union[_value, 'Annotated']
> +
Maybe declare `Annotations` first, then `Annotated`, then *use*
`Annotated` proper here?
- Cleber.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations
2020-11-07 2:12 ` Cleber Rosa
@ 2020-12-07 21:29 ` John Snow
0 siblings, 0 replies; 48+ messages in thread
From: John Snow @ 2020-12-07 21:29 UTC (permalink / raw)
To: Cleber Rosa; +Cc: qemu-devel, Eduardo Habkost, Markus Armbruster
On 11/6/20 9:12 PM, Cleber Rosa wrote:
> On Mon, Oct 26, 2020 at 03:42:45PM -0400, John Snow wrote:
>> The typing of _make_tree and friends is a bit involved, but it can be
>> done with some stubbed out types and a bit of elbow grease. The
>> forthcoming patches attempt to make some simplifications, but having the
>> type hints in advance may aid in review of subsequent patches.
>>
>>
>> Some notes on the abstract types used at this point, and what they
>> represent:
>>
>> - TreeValue represents any object in the type tree. _make_tree is an
>> optional call -- not every node in the final type tree will have been
>> passed to _make_tree, so this type encompasses not only what is passed
>> to _make_tree (dicts, strings) or returned from it (dicts, strings, a
>> 2-tuple), but any recursive value for any of the dicts passed to
>> _make_tree -- which includes lists, strings, integers, null constants,
>> and so on.
>>
>> - _DObject is a type alias I use to mean "A JSON-style object,
>> represented as a Python dict." There is no "JSON" type in Python, they
>> are converted natively to recursively nested dicts and lists, with
>> leaf values of str, int, float, None, True/False and so on. This type
>> structure is not possible to accurately portray in mypy yet, so a
>> placeholder is used.
>>
>> In this case, _DObject is being used to refer to SchemaInfo-like
>> structures as defined in qapi/introspect.json, OR any sub-object
>> values they may reference. We don't have strong typing available for
>> those, so a generic alternative is used.
>>
>> - Extra refers explicitly to the dict containing "extra" information
>> about a node in the tree. mypy does not offer per-key typing for dicts
>> in Python 3.6, so this is the best we can do here.
>>
>> - Annotated refers to (one of) the return types of _make_tree:
>> It represents a 2-tuple of (TreeValue, Extra).
>>
>>
>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qapi/introspect.py | 157 ++++++++++++++++++++++++++++---------
>> scripts/qapi/mypy.ini | 5 --
>> scripts/qapi/schema.py | 2 +-
>> 3 files changed, 121 insertions(+), 43 deletions(-)
>>
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index 63f721ebfb6..803288a64e7 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -10,7 +10,16 @@
>> See the COPYING file in the top-level directory.
>> """
>>
>> -from typing import Optional, Sequence, cast
>> +from typing import (
>> + Any,
>> + Dict,
>> + List,
>> + Optional,
>> + Sequence,
>> + Tuple,
>> + Union,
>> + cast,
>> +)
>>
>> from .common import (
>> c_name,
>> @@ -20,13 +29,56 @@
>> )
>> from .gen import QAPISchemaMonolithicCVisitor
>> from .schema import (
>> + QAPISchema,
>> QAPISchemaArrayType,
>> QAPISchemaBuiltinType,
>> + QAPISchemaEntity,
>> + QAPISchemaEnumMember,
>> + QAPISchemaFeature,
>> + QAPISchemaObjectType,
>> + QAPISchemaObjectTypeMember,
>> QAPISchemaType,
>> + QAPISchemaVariant,
>> + QAPISchemaVariants,
>> )
>> +from .source import QAPISourceInfo
>>
>>
>> -def _make_tree(obj, ifcond, features, extra=None):
>> +# This module constructs a tree-like data structure that is used to
>> +# generate the introspection information for QEMU. It behaves similarly
>> +# to a JSON value.
>> +#
>> +# A complexity over JSON is that our values may or may not be annotated.
>> +#
>> +# Un-annotated values may be:
>> +# Scalar: str, bool, None.
>> +# Non-scalar: List, Dict
>> +# _Value = Union[str, bool, None, Dict[str, Value], List[Value]]
>
> Here (and in a few other places) you mention `_Value`, but then define it as
> `_value` (lowercase).
>
This was maybe too subtle: I didn't intend for it to be the "same" as
_value; this is the "idealized version". And then I went and re-used the
same exact name of "TreeValue", which muddied the waters.
Let's just err back on the side of synchronicity and say:
# _value = Union[str, bool, None, Dict[str, TreeValue], List[TreeValue]]
# TreeValue = Union[_value, Annotated[_value]]
>> +#
>> +# With optional annotations, the type of all values is:
>> +# TreeValue = Union[_Value, Annotated[_Value]]
>> +#
>> +# Sadly, mypy does not support recursive types, so we must approximate this.
>> +_stub = Any
>> +_scalar = Union[str, bool, None]
>> +_nonscalar = Union[Dict[str, _stub], List[_stub]]
>
> Why not use `Any` here instead of declaring/using `_stub`?
>
This was my way of calling visual attention to the exact places in the
type tree we are breaking recursion with a stubbed type.
I wanted to communicate that we didn't WANT the any type, but we were
forced to accept a "stub" type. (Which we implement with the Any type.)
>> +_value = Union[_scalar, _nonscalar]
>> +TreeValue = Union[_value, 'Annotated']
>> +
>
> Maybe declare `Annotations` first, then `Annotated`, then *use*
> `Annotated` proper here?
>
As long as that doesn't lose clarity and synchronicity with the little
comment blurb, or cause problems later in the patch. I will see if there
was a reason I couldn't.
Otherwise, it's not really too important to shuffle around things to
avoid strings; it doesn't change anything for mypy or for the runtime.
> - Cleber.
>
Thanks!
--js
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations
2020-10-26 19:42 ` [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations John Snow
2020-11-07 2:12 ` Cleber Rosa
@ 2020-11-13 16:48 ` Markus Armbruster
2020-12-07 23:48 ` John Snow
1 sibling, 1 reply; 48+ messages in thread
From: Markus Armbruster @ 2020-11-13 16:48 UTC (permalink / raw)
To: John Snow; +Cc: Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> The typing of _make_tree and friends is a bit involved, but it can be
> done with some stubbed out types and a bit of elbow grease. The
> forthcoming patches attempt to make some simplifications, but having the
> type hints in advance may aid in review of subsequent patches.
>
>
> Some notes on the abstract types used at this point, and what they
> represent:
>
> - TreeValue represents any object in the type tree. _make_tree is an
> optional call -- not every node in the final type tree will have been
> passed to _make_tree, so this type encompasses not only what is passed
> to _make_tree (dicts, strings) or returned from it (dicts, strings, a
> 2-tuple), but any recursive value for any of the dicts passed to
> _make_tree -- which includes lists, strings, integers, null constants,
> and so on.
>
> - _DObject is a type alias I use to mean "A JSON-style object,
> represented as a Python dict." There is no "JSON" type in Python, they
> are converted natively to recursively nested dicts and lists, with
> leaf values of str, int, float, None, True/False and so on. This type
> structure is not possible to accurately portray in mypy yet, so a
> placeholder is used.
>
> In this case, _DObject is being used to refer to SchemaInfo-like
> structures as defined in qapi/introspect.json, OR any sub-object
> values they may reference. We don't have strong typing available for
> those, so a generic alternative is used.
>
> - Extra refers explicitly to the dict containing "extra" information
> about a node in the tree. mypy does not offer per-key typing for dicts
> in Python 3.6, so this is the best we can do here.
>
> - Annotated refers to (one of) the return types of _make_tree:
> It represents a 2-tuple of (TreeValue, Extra).
>
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/introspect.py | 157 ++++++++++++++++++++++++++++---------
> scripts/qapi/mypy.ini | 5 --
> scripts/qapi/schema.py | 2 +-
> 3 files changed, 121 insertions(+), 43 deletions(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 63f721ebfb6..803288a64e7 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -10,7 +10,16 @@
> See the COPYING file in the top-level directory.
> """
>
> -from typing import Optional, Sequence, cast
> +from typing import (
> + Any,
> + Dict,
> + List,
> + Optional,
> + Sequence,
> + Tuple,
> + Union,
> + cast,
> +)
>
> from .common import (
> c_name,
> @@ -20,13 +29,56 @@
> )
> from .gen import QAPISchemaMonolithicCVisitor
> from .schema import (
> + QAPISchema,
> QAPISchemaArrayType,
> QAPISchemaBuiltinType,
> + QAPISchemaEntity,
> + QAPISchemaEnumMember,
> + QAPISchemaFeature,
> + QAPISchemaObjectType,
> + QAPISchemaObjectTypeMember,
> QAPISchemaType,
> + QAPISchemaVariant,
> + QAPISchemaVariants,
> )
> +from .source import QAPISourceInfo
>
>
> -def _make_tree(obj, ifcond, features, extra=None):
> +# This module constructs a tree-like data structure that is used to
"Tree-like" suggests it's not a tree, it just looks like one if you
squint. Drop "-like"?
> +# generate the introspection information for QEMU. It behaves similarly
> +# to a JSON value.
> +#
> +# A complexity over JSON is that our values may or may not be annotated.
It's the obvious abstract syntax tree for JSON, hacked up^W^Wextended to
support certain annotations.
Let me add a bit of context and history.
The module's job is generating qapi-introspect.[ch] for a QAPISchema.
The purpose of qapi-introspect.[ch] is providing the information
query-qmp-schema needs, i.e. (a suitable C representation of) a JSON
value conforming to [SchemaInfo]. Details of this C representation are
not interesting right now.
We first go from QAPISchema to a suitable Python representation of
[SchemaInfo], then from there to the C source code, neatly separating
concerns.
Stupidest solution Python representation that could possibly work: the
obvious abstract syntax tree for JSON (that's also how Python's json
module works).
Parts corresponding to QAPISchema parts guarded by 'if' conditionals
need to be guarded by #if conditionals.
We want to prefix parts corresponding to certain QAPISchema parts with a
comment.
These two requirements came later, and were hacked into the existing
stupidest solution: any tree node can be a tuple (json, extra), where
json is the "stupidest" node, and extra is a dict of annotations. In
other words, to annotate an unannotated node N with dict D, replace N by
(N, D).
Possible annotations:
'comment': str
'if': Sequence[str]
They say there are just three answers a Marine may give to an officer's
questions: "Yes, sir!", "No, sir!", "No excuse, sir!". Let me put that
to use here:
Is this an elegant design? No, sir!
Is the code easy to read? No excuse, sir!
Was it cheap to make? Yes, sir!
> +#
> +# Un-annotated values may be:
> +# Scalar: str, bool, None.
> +# Non-scalar: List, Dict
> +# _Value = Union[str, bool, None, Dict[str, Value], List[Value]]
> +#
> +# With optional annotations, the type of all values is:
> +# TreeValue = Union[_Value, Annotated[_Value]]
> +#
> +# Sadly, mypy does not support recursive types, so we must approximate this.
> +_stub = Any
> +_scalar = Union[str, bool, None]
> +_nonscalar = Union[Dict[str, _stub], List[_stub]]
> +_value = Union[_scalar, _nonscalar]
> +TreeValue = Union[_value, 'Annotated']
Are there naming conventions for this kind of variables? I'm asking
because you capitalize some, but not all, and I can't see a pattern.
Ignorant question: only 'Annotated' has quotes; why?
There is "_Value", "Value" and "_value". Suggest to add "value"
somewhere, for completeness ;-P
I find the names _value and TreeValue a bit unfortunate: the difference
between the two isn't Tree. I'll come back to this below.
> +
> +# This is just an alias for an object in the structure described above:
> +_DObject = Dict[str, object]
I'm confused. Which structure, and why do we want to alias it?
> +
> +# Represents the annotations themselves:
> +Annotations = Dict[str, object]
Losely typed. I have no idea whether that's bad :)
> +
> +# Represents an annotated node (of some kind).
> +Annotated = Tuple[_value, Annotations]
So, _value seems to represent a JSON value, Annotated an annotated JSON
value, and TreeValue their union, i.e. a possibly annotated JSON value.
Naming is hard... BareJsonValue, AnnotatedJsonValue, JsonValue?
> +
> +
> +def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
I'd expect obj: _value, i.e. "unannotated value".
> + features: List[QAPISchemaFeature],
> + extra: Optional[Annotations] = None
> + ) -> TreeValue:
> if extra is None:
> extra = {}
> if ifcond:
> @@ -39,9 +91,11 @@ def _make_tree(obj, ifcond, features, extra=None):
> return obj
>
>
> -def _tree_to_qlit(obj, level=0, suppress_first_indent=False):
> +def _tree_to_qlit(obj: TreeValue,
> + level: int = 0,
> + suppress_first_indent: bool = False) -> str:
>
> - def indent(level):
> + def indent(level: int) -> str:
> return level * 4 * ' '
>
> if isinstance(obj, tuple):
> @@ -91,21 +145,20 @@ def indent(level):
> return ret
>
>
> -def to_c_string(string):
> +def to_c_string(string: str) -> str:
> return '"' + string.replace('\\', r'\\').replace('"', r'\"') + '"'
>
>
> class QAPISchemaGenIntrospectVisitor(QAPISchemaMonolithicCVisitor):
> -
Intentional?
> - def __init__(self, prefix, unmask):
> + def __init__(self, prefix: str, unmask: bool):
> super().__init__(
> prefix, 'qapi-introspect',
> ' * QAPI/QMP schema introspection', __doc__)
> self._unmask = unmask
> - self._schema = None
> - self._trees = []
> - self._used_types = []
> - self._name_map = {}
> + self._schema: Optional[QAPISchema] = None
> + self._trees: List[TreeValue] = []
> + self._used_types: List[QAPISchemaType] = []
> + self._name_map: Dict[str, str] = {}
> self._genc.add(mcgen('''
> #include "qemu/osdep.h"
> #include "%(prefix)sqapi-introspect.h"
> @@ -113,10 +166,10 @@ def __init__(self, prefix, unmask):
> ''',
> prefix=prefix))
>
> - def visit_begin(self, schema):
> + def visit_begin(self, schema: QAPISchema) -> None:
> self._schema = schema
>
> - def visit_end(self):
> + def visit_end(self) -> None:
> # visit the types that are actually used
> for typ in self._used_types:
> typ.visit(self)
> @@ -138,18 +191,18 @@ def visit_end(self):
> self._used_types = []
> self._name_map = {}
>
> - def visit_needed(self, entity):
> + def visit_needed(self, entity: QAPISchemaEntity) -> bool:
> # Ignore types on first pass; visit_end() will pick up used types
> return not isinstance(entity, QAPISchemaType)
>
> - def _name(self, name):
> + def _name(self, name: str) -> str:
> if self._unmask:
> return name
> if name not in self._name_map:
> self._name_map[name] = '%d' % len(self._name_map)
> return self._name_map[name]
>
> - def _use_type(self, typ):
> + def _use_type(self, typ: QAPISchemaType) -> str:
> # Map the various integer types to plain int
> if typ.json_type() == 'int':
> typ = self._schema.lookup_type('int')
> @@ -168,8 +221,10 @@ def _use_type(self, typ):
> return '[' + self._use_type(typ.element_type) + ']'
> return self._name(typ.name)
>
> - def _gen_tree(self, name, mtype, obj, ifcond, features):
> - extra = None
> + def _gen_tree(self, name: str, mtype: str, obj: _DObject,
> + ifcond: List[str],
> + features: Optional[List[QAPISchemaFeature]]) -> None:
_gen_tree() builds a complete tree (i.e. one SchemaInfo), and adds it to
._trees.
The SchemaInfo's common parts are name, meta-type and features.
_gen_tree() takes them as arguments @name, @mtype, @features.
It takes SchemaInfo's variant parts as a dict @obj.
It completes @obj into an unannotated tree node by the common parts into
@obj.
It also takes a QAPI conditional argument @ifcond.
Now let me review the type annotations:
* name: str matches SchemaInfo, good.
* mtype: str approximates SchemaInfo's enum SchemaMetaType (it's not
Python Enum because those were off limits when this code was written).
* obj: _DObject ... I'd expect "unannotated JSON value".
* ifcond: List[str] should work, but gen_if(), gen_endif() use
Sequence[str]. Suggest to pick one (please explain why), and stick to
it.
More instances of ifcond: List[str] elsewhere; I'm not flagging them.
* features: Optional[List[QAPISchemaFeature]] is correct. "No features"
has two representations: None and []. I guess we could eliminate
None, trading a tiny bit of efficiency for simpler typing. Not a
demand.
> + extra: Optional[Annotations] = None
"No annotations" is represented as None here, not {}. I guess we could
use {} for simpler typing. Not a demand.
> if mtype not in ('command', 'event', 'builtin', 'array'):
> if not self._unmask:
> # Output a comment to make it easy to map masked names
> @@ -180,44 +235,64 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
> obj['meta-type'] = mtype
> self._trees.append(_make_tree(obj, ifcond, features, extra))
>
> - def _gen_member(self, member):
> - obj = {'name': member.name, 'type': self._use_type(member.type)}
> + def _gen_member(self,
> + member: QAPISchemaObjectTypeMember) -> TreeValue:
> + obj: _DObject = {
I'd expect "unannotated value". More of the same below.
> + 'name': member.name,
> + 'type': self._use_type(member.type)
> + }
> if member.optional:
> obj['default'] = None
> return _make_tree(obj, member.ifcond, member.features)
>
> - def _gen_variants(self, tag_name, variants):
> + def _gen_variants(self, tag_name: str,
> + variants: List[QAPISchemaVariant]) -> _DObject:
> return {'tag': tag_name,
> 'variants': [self._gen_variant(v) for v in variants]}
>
> - def _gen_variant(self, variant):
> - obj = {'case': variant.name, 'type': self._use_type(variant.type)}
> + def _gen_variant(self, variant: QAPISchemaVariant) -> TreeValue:
> + obj: _DObject = {
> + 'case': variant.name,
> + 'type': self._use_type(variant.type)
> + }
> return _make_tree(obj, variant.ifcond, None)
>
> - def visit_builtin_type(self, name, info, json_type):
> + def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
A built-in's type info is always None. Perhaps we should drop the
parameter.
> + json_type: str) -> None:
> self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
>
> - def visit_enum_type(self, name, info, ifcond, features, members, prefix):
> + def visit_enum_type(self, name: str, info: QAPISourceInfo,
> + ifcond: List[str], features: List[QAPISchemaFeature],
> + members: List[QAPISchemaEnumMember],
> + prefix: Optional[str]) -> None:
> self._gen_tree(name, 'enum',
> {'values': [_make_tree(m.name, m.ifcond, None)
> for m in members]},
> ifcond, features)
>
> - def visit_array_type(self, name, info, ifcond, element_type):
> + def visit_array_type(self, name: str, info: Optional[QAPISourceInfo],
Here, @info is indeed optional: it's None when @element_type is a
built-in type.
> + ifcond: List[str],
> + element_type: QAPISchemaType) -> None:
> element = self._use_type(element_type)
> self._gen_tree('[' + element + ']', 'array', {'element-type': element},
> ifcond, None)
>
> - def visit_object_type_flat(self, name, info, ifcond, features,
> - members, variants):
> - obj = {'members': [self._gen_member(m) for m in members]}
> + def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
And here it is optional due to the internal object type 'q_empty'.
> + ifcond: List[str],
> + features: List[QAPISchemaFeature],
> + members: Sequence[QAPISchemaObjectTypeMember],
> + variants: Optional[QAPISchemaVariants]) -> None:
We represent "no variants" as None, not as []. I guess we could
eliminate use [], trading a tiny bit of efficiency for simpler typing.
Not a demand.
> + obj: _DObject = {'members': [self._gen_member(m) for m in members]}
> if variants:
> obj.update(self._gen_variants(variants.tag_member.name,
> variants.variants))
>
> self._gen_tree(name, 'object', obj, ifcond, features)
>
> - def visit_alternate_type(self, name, info, ifcond, features, variants):
> + def visit_alternate_type(self, name: str, info: QAPISourceInfo,
> + ifcond: List[str],
> + features: List[QAPISchemaFeature],
> + variants: QAPISchemaVariants) -> None:
> self._gen_tree(name, 'alternate',
> {'members': [
> _make_tree({'type': self._use_type(m.type)},
> @@ -225,24 +300,32 @@ def visit_alternate_type(self, name, info, ifcond, features, variants):
> for m in variants.variants]},
> ifcond, features)
>
> - def visit_command(self, name, info, ifcond, features,
> - arg_type, ret_type, gen, success_response, boxed,
> - allow_oob, allow_preconfig, coroutine):
> + def visit_command(self, name: str, info: QAPISourceInfo, ifcond: List[str],
> + features: List[QAPISchemaFeature],
> + arg_type: QAPISchemaObjectType,
> + ret_type: Optional[QAPISchemaType], gen: bool,
Are you sure arg_type can't be None?
> + success_response: bool, boxed: bool, allow_oob: bool,
> + allow_preconfig: bool, coroutine: bool) -> None:
> arg_type = arg_type or self._schema.the_empty_object_type
> ret_type = ret_type or self._schema.the_empty_object_type
> - obj = {'arg-type': self._use_type(arg_type),
> - 'ret-type': self._use_type(ret_type)}
> + obj: _DObject = {
> + 'arg-type': self._use_type(arg_type),
> + 'ret-type': self._use_type(ret_type)
> + }
> if allow_oob:
> obj['allow-oob'] = allow_oob
> self._gen_tree(name, 'command', obj, ifcond, features)
>
> - def visit_event(self, name, info, ifcond, features, arg_type, boxed):
> + def visit_event(self, name: str, info: QAPISourceInfo,
> + ifcond: List[str], features: List[QAPISchemaFeature],
> + arg_type: QAPISchemaObjectType, boxed: bool) -> None:
> arg_type = arg_type or self._schema.the_empty_object_type
> self._gen_tree(name, 'event', {'arg-type': self._use_type(arg_type)},
> ifcond, features)
>
>
> -def gen_introspect(schema, output_dir, prefix, opt_unmask):
> +def gen_introspect(schema: QAPISchema, output_dir: str, prefix: str,
> + opt_unmask: bool) -> None:
> vis = QAPISchemaGenIntrospectVisitor(prefix, opt_unmask)
> schema.visit(vis)
> vis.write(output_dir)
> diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
> index 74fc6c82153..c0f2a58306d 100644
> --- a/scripts/qapi/mypy.ini
> +++ b/scripts/qapi/mypy.ini
> @@ -14,11 +14,6 @@ disallow_untyped_defs = False
> disallow_incomplete_defs = False
> check_untyped_defs = False
>
> -[mypy-qapi.introspect]
> -disallow_untyped_defs = False
> -disallow_incomplete_defs = False
> -check_untyped_defs = False
> -
> [mypy-qapi.parser]
> disallow_untyped_defs = False
> disallow_incomplete_defs = False
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 720449feee4..e91b77fadc3 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -28,7 +28,7 @@
> class QAPISchemaEntity:
> meta: Optional[str] = None
>
> - def __init__(self, name, info, doc, ifcond=None, features=None):
> + def __init__(self, name: str, info, doc, ifcond=None, features=None):
> assert name is None or isinstance(name, str)
> for f in features or []:
> assert isinstance(f, QAPISchemaFeature)
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations
2020-11-13 16:48 ` Markus Armbruster
@ 2020-12-07 23:48 ` John Snow
2020-12-16 7:51 ` Markus Armbruster
0 siblings, 1 reply; 48+ messages in thread
From: John Snow @ 2020-12-07 23:48 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Cleber Rosa, qemu-devel, Eduardo Habkost
On 11/13/20 11:48 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> The typing of _make_tree and friends is a bit involved, but it can be
>> done with some stubbed out types and a bit of elbow grease. The
>> forthcoming patches attempt to make some simplifications, but having the
>> type hints in advance may aid in review of subsequent patches.
>>
>>
>> Some notes on the abstract types used at this point, and what they
>> represent:
>>
>> - TreeValue represents any object in the type tree. _make_tree is an
>> optional call -- not every node in the final type tree will have been
>> passed to _make_tree, so this type encompasses not only what is passed
>> to _make_tree (dicts, strings) or returned from it (dicts, strings, a
>> 2-tuple), but any recursive value for any of the dicts passed to
>> _make_tree -- which includes lists, strings, integers, null constants,
>> and so on.
>>
>> - _DObject is a type alias I use to mean "A JSON-style object,
>> represented as a Python dict." There is no "JSON" type in Python, they
>> are converted natively to recursively nested dicts and lists, with
>> leaf values of str, int, float, None, True/False and so on. This type
>> structure is not possible to accurately portray in mypy yet, so a
>> placeholder is used.
>>
>> In this case, _DObject is being used to refer to SchemaInfo-like
>> structures as defined in qapi/introspect.json, OR any sub-object
>> values they may reference. We don't have strong typing available for
>> those, so a generic alternative is used.
>>
>> - Extra refers explicitly to the dict containing "extra" information
>> about a node in the tree. mypy does not offer per-key typing for dicts
>> in Python 3.6, so this is the best we can do here.
>>
>> - Annotated refers to (one of) the return types of _make_tree:
>> It represents a 2-tuple of (TreeValue, Extra).
>>
>>
>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qapi/introspect.py | 157 ++++++++++++++++++++++++++++---------
>> scripts/qapi/mypy.ini | 5 --
>> scripts/qapi/schema.py | 2 +-
>> 3 files changed, 121 insertions(+), 43 deletions(-)
>>
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index 63f721ebfb6..803288a64e7 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -10,7 +10,16 @@
>> See the COPYING file in the top-level directory.
>> """
>>
>> -from typing import Optional, Sequence, cast
>> +from typing import (
>> + Any,
>> + Dict,
>> + List,
>> + Optional,
>> + Sequence,
>> + Tuple,
>> + Union,
>> + cast,
>> +)
>>
>> from .common import (
>> c_name,
>> @@ -20,13 +29,56 @@
>> )
>> from .gen import QAPISchemaMonolithicCVisitor
>> from .schema import (
>> + QAPISchema,
>> QAPISchemaArrayType,
>> QAPISchemaBuiltinType,
>> + QAPISchemaEntity,
>> + QAPISchemaEnumMember,
>> + QAPISchemaFeature,
>> + QAPISchemaObjectType,
>> + QAPISchemaObjectTypeMember,
>> QAPISchemaType,
>> + QAPISchemaVariant,
>> + QAPISchemaVariants,
>> )
>> +from .source import QAPISourceInfo
>>
>>
>> -def _make_tree(obj, ifcond, features, extra=None):
>> +# This module constructs a tree-like data structure that is used to
>
> "Tree-like" suggests it's not a tree, it just looks like one if you
> squint. Drop "-like"?
>
Sure. I think I am grammatically predisposed to assume "binary tree" or
at least some kind of monomorphic tree when I see "tree", hence the
hedging and weasel-words.
No problem just to drop it.
>> +# generate the introspection information for QEMU. It behaves similarly
>> +# to a JSON value.
>> +#
>> +# A complexity over JSON is that our values may or may not be annotated.
>
> It's the obvious abstract syntax tree for JSON, hacked up^W^Wextended to
> support certain annotations.
>
Yes.
> Let me add a bit of context and history.
>
> The module's job is generating qapi-introspect.[ch] for a QAPISchema.
>
> The purpose of qapi-introspect.[ch] is providing the information
> query-qmp-schema needs, i.e. (a suitable C representation of) a JSON
> value conforming to [SchemaInfo]. Details of this C representation are
> not interesting right now.
>
> We first go from QAPISchema to a suitable Python representation of
> [SchemaInfo], then from there to the C source code, neatly separating
> concerns.
>
> Stupidest solution Python representation that could possibly work: the
> obvious abstract syntax tree for JSON (that's also how Python's json
> module works).
>
> Parts corresponding to QAPISchema parts guarded by 'if' conditionals
> need to be guarded by #if conditionals.
>
> We want to prefix parts corresponding to certain QAPISchema parts with a
> comment.
>
> These two requirements came later, and were hacked into the existing
> stupidest solution: any tree node can be a tuple (json, extra), where
> json is the "stupidest" node, and extra is a dict of annotations. In
> other words, to annotate an unannotated node N with dict D, replace N by
> (N, D).
>
> Possible annotations:
>
> 'comment': str
> 'if': Sequence[str]
>
> They say there are just three answers a Marine may give to an officer's
> questions: "Yes, sir!", "No, sir!", "No excuse, sir!". Let me put that
> to use here:
>
> Is this an elegant design? No, sir!
>
> Is the code easy to read? No excuse, sir!
>
> Was it cheap to make? Yes, sir!
>
Yes, I believe I am on the same page so far. It was difficult to read
and difficult to annotate, but once I got to the bottom, it's easy to
see why it happened this way in retrospect. It's the simplest thing that
could work.
And it does work!
So far, this comment isn't really a correction on what I wrote, but if
you believe my comment that explains the types could be clearer, feel
free to suggest.
>> +#
>> +# Un-annotated values may be:
>> +# Scalar: str, bool, None.
>> +# Non-scalar: List, Dict
>> +# _Value = Union[str, bool, None, Dict[str, Value], List[Value]]
>> +#
>> +# With optional annotations, the type of all values is:
>> +# TreeValue = Union[_Value, Annotated[_Value]]
>> +#
>> +# Sadly, mypy does not support recursive types, so we must approximate this.
>> +_stub = Any
>> +_scalar = Union[str, bool, None]
>> +_nonscalar = Union[Dict[str, _stub], List[_stub]]
>> +_value = Union[_scalar, _nonscalar]
>> +TreeValue = Union[_value, 'Annotated']
>
> Are there naming conventions for this kind of variables? I'm asking
> because you capitalize some, but not all, and I can't see a pattern.
>
Types generally get CamelCase names; though for interior aliases I used
underscore + lowercase. In this case, I was trying to illustrate that
these intermediate types were only interesting or useful in service of
the Capitalized Thing, TreeValue.
> Ignorant question: only 'Annotated' has quotes; why?
>
It hasn't been defined yet, so it's a forward reference. Cleber
suggested I hoist its definition up to avoid this. I forget if I had
some strong reason for doing it this way, admittedly.
A lot of this code gets changed shortly in the following patches, so
honestly I was likely just not really putting in a lot of effort to make
code that gets deleted soon pretty.
(Why did the patches go in that order then? During early review, Eduardo
wanted to see the type hints go in first to help review for the cleanup
be easier ...!)
> There is "_Value", "Value" and "_value". Suggest to add "value"
> somewhere, for completeness ;-P
>
Yeah, my mistake. See my response to Cleber here...
> I find the names _value and TreeValue a bit unfortunate: the difference
> between the two isn't Tree. I'll come back to this below.
>
Feel free to suggest something better; I am at a point with this
particular module where I'd be happy to have someone more opinionated
than me telling me what they want.
>> +
>> +# This is just an alias for an object in the structure described above:
>> +_DObject = Dict[str, object]
>
> I'm confused. Which structure, and why do we want to alias it?
>
Yep, I don't have a good name for this either. I mean this to be a
generic type that describes the natural python representation for a JSON
object.
i.e. Dict[str, THING].
All of the various bits and pieces here that are generating SchemaInfo
subtype representations (like _gen_member, _gen_variant, etc.) are using
this as a generic "Well, it's some kind of dict/object."
(Aside: this is an interesting part of code, because it is not type safe
with respect to the QAPI definitions that define SchemaInfo's descendant
types. There's no more explicit type for me to use here to describe
those objects.)
>> +
>> +# Represents the annotations themselves:
>> +Annotations = Dict[str, object]
>
> Losely typed. I have no idea whether that's bad :)
>
Kind of: unlike Any, using 'object' means that if we treat the
dictionary values in a manner that we could not treat *all* values, mypy
will raise an error.
So it is a very broad type, but it's "safe".
>> +
>> +# Represents an annotated node (of some kind).
>> +Annotated = Tuple[_value, Annotations]
>
> So, _value seems to represent a JSON value, Annotated an annotated JSON
> value, and TreeValue their union, i.e. a possibly annotated JSON value.
>
> Naming is hard... BareJsonValue, AnnotatedJsonValue, JsonValue?
>
I was afraid of using "JsonValue" to avoid implicating it as a literal
JSON value -- since it isn't, exactly. It's the Python native
approximation of a subset of JSON values.
>> +
>> +
>> +def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
>
> I'd expect obj: _value, i.e. "unannotated value".
>
Sure, and I believe that would work -- this type is "tighter".
_make_tree only ever sees Dict[str, object] and str, actually.
Later in the series, _make_tree goes away and this weird sub-type also
goes away in favor of the more generic type.
>> + features: List[QAPISchemaFeature],
>> + extra: Optional[Annotations] = None
>> + ) -> TreeValue:
>> if extra is None:
>> extra = {}
>> if ifcond:
>> @@ -39,9 +91,11 @@ def _make_tree(obj, ifcond, features, extra=None):
>> return obj
>>
>>
>> -def _tree_to_qlit(obj, level=0, suppress_first_indent=False):
>> +def _tree_to_qlit(obj: TreeValue,
>> + level: int = 0,
>> + suppress_first_indent: bool = False) -> str:
>>
>> - def indent(level):
>> + def indent(level: int) -> str:
>> return level * 4 * ' '
>>
>> if isinstance(obj, tuple):
>> @@ -91,21 +145,20 @@ def indent(level):
>> return ret
>>
>>
>> -def to_c_string(string):
>> +def to_c_string(string: str) -> str:
>> return '"' + string.replace('\\', r'\\').replace('"', r'\"') + '"'
>>
>>
>> class QAPISchemaGenIntrospectVisitor(QAPISchemaMonolithicCVisitor):
>> -
>
> Intentional?
>
I'm sure I thought it looked nice at the time. It's not important.
>> - def __init__(self, prefix, unmask):
>> + def __init__(self, prefix: str, unmask: bool):
>> super().__init__(
>> prefix, 'qapi-introspect',
>> ' * QAPI/QMP schema introspection', __doc__)
>> self._unmask = unmask
>> - self._schema = None
>> - self._trees = []
>> - self._used_types = []
>> - self._name_map = {}
>> + self._schema: Optional[QAPISchema] = None
>> + self._trees: List[TreeValue] = []
>> + self._used_types: List[QAPISchemaType] = []
>> + self._name_map: Dict[str, str] = {}
>> self._genc.add(mcgen('''
>> #include "qemu/osdep.h"
>> #include "%(prefix)sqapi-introspect.h"
>> @@ -113,10 +166,10 @@ def __init__(self, prefix, unmask):
>> ''',
>> prefix=prefix))
>>
>> - def visit_begin(self, schema):
>> + def visit_begin(self, schema: QAPISchema) -> None:
>> self._schema = schema
>>
>> - def visit_end(self):
>> + def visit_end(self) -> None:
>> # visit the types that are actually used
>> for typ in self._used_types:
>> typ.visit(self)
>> @@ -138,18 +191,18 @@ def visit_end(self):
>> self._used_types = []
>> self._name_map = {}
>>
>> - def visit_needed(self, entity):
>> + def visit_needed(self, entity: QAPISchemaEntity) -> bool:
>> # Ignore types on first pass; visit_end() will pick up used types
>> return not isinstance(entity, QAPISchemaType)
>>
>> - def _name(self, name):
>> + def _name(self, name: str) -> str:
>> if self._unmask:
>> return name
>> if name not in self._name_map:
>> self._name_map[name] = '%d' % len(self._name_map)
>> return self._name_map[name]
>>
>> - def _use_type(self, typ):
>> + def _use_type(self, typ: QAPISchemaType) -> str:
>> # Map the various integer types to plain int
>> if typ.json_type() == 'int':
>> typ = self._schema.lookup_type('int')
>> @@ -168,8 +221,10 @@ def _use_type(self, typ):
>> return '[' + self._use_type(typ.element_type) + ']'
>> return self._name(typ.name)
>>
>> - def _gen_tree(self, name, mtype, obj, ifcond, features):
>> - extra = None
>> + def _gen_tree(self, name: str, mtype: str, obj: _DObject,
>> + ifcond: List[str],
>> + features: Optional[List[QAPISchemaFeature]]) -> None:
>
> _gen_tree() builds a complete tree (i.e. one SchemaInfo), and adds it to
> ._trees.
>
> The SchemaInfo's common parts are name, meta-type and features.
> _gen_tree() takes them as arguments @name, @mtype, @features.
>
> It takes SchemaInfo's variant parts as a dict @obj.
>
> It completes @obj into an unannotated tree node by the common parts into
> @obj.
>
> It also takes a QAPI conditional argument @ifcond.
>
> Now let me review the type annotations:
>
> * name: str matches SchemaInfo, good.
>
> * mtype: str approximates SchemaInfo's enum SchemaMetaType (it's not
> Python Enum because those were off limits when this code was written).
>
It's also a little cumbersome, perhaps, to duplicate information from
the QAPI schema directly into the QAPI generator.
Didn't feel like getting clever enough to "fix" this.
> * obj: _DObject ... I'd expect "unannotated JSON value".
>
It's NOT any value though -- like you said: "_gen_tree() builds a
complete tree (i.e. one SchemaInfo)" -- we only accept objects here,
which is correct.
> * ifcond: List[str] should work, but gen_if(), gen_endif() use
> Sequence[str]. Suggest to pick one (please explain why), and stick to
> it.
>
> More instances of ifcond: List[str] elsewhere; I'm not flagging them.
>
Yes, I should probably be using Sequence, if I can. generally:
- Input types should use the most generic type they can cope with
(Iterable, Sequence, Collection) based on what properties they actually
need in the incoming type.
(By the end of this series, Iterable[str] should actually be sufficient,
but I'll have to see where it makes sense to slacken the input types in
this series. It's been through the washer quite a few times I'm afraid.)
- Output types should be as explicit as possible.
I was not always perfectly good about generalizing the input types; List
is correct, but not necessarily maximally correct.
> * features: Optional[List[QAPISchemaFeature]] is correct. "No features"
> has two representations: None and []. I guess we could eliminate
> None, trading a tiny bit of efficiency for simpler typing. Not a
> demand.
>
>> + extra: Optional[Annotations] = None
>
> "No annotations" is represented as None here, not {}. I guess we could
> use {} for simpler typing. Not a demand.
>
This goes away later, kinda. It becomes:
comment: Optional[str] = None
and that comment is eventually passed to an Annotated node constructor
that takes the comment specifically.
>> if mtype not in ('command', 'event', 'builtin', 'array'):
>> if not self._unmask:
>> # Output a comment to make it easy to map masked names
>> @@ -180,44 +235,64 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
>> obj['meta-type'] = mtype
>> self._trees.append(_make_tree(obj, ifcond, features, extra))
>>
>> - def _gen_member(self, member):
>> - obj = {'name': member.name, 'type': self._use_type(member.type)}
>> + def _gen_member(self,
>> + member: QAPISchemaObjectTypeMember) -> TreeValue:
>> + obj: _DObject = {
>
> I'd expect "unannotated value". More of the same below.
>
See my remark to your expectation for what _gen_tree should accept. It
deals with "objects" and "objects" have the property that more keys can
be assigned to them, and this is a fundamental feature of _gen_tree.
>> + 'name': member.name,
>> + 'type': self._use_type(member.type)
>> + }
>> if member.optional:
>> obj['default'] = None
>> return _make_tree(obj, member.ifcond, member.features)
>>
>> - def _gen_variants(self, tag_name, variants):
>> + def _gen_variants(self, tag_name: str,
>> + variants: List[QAPISchemaVariant]) -> _DObject:
>> return {'tag': tag_name,
>> 'variants': [self._gen_variant(v) for v in variants]}
>>
>> - def _gen_variant(self, variant):
>> - obj = {'case': variant.name, 'type': self._use_type(variant.type)}
>> + def _gen_variant(self, variant: QAPISchemaVariant) -> TreeValue:
>> + obj: _DObject = {
>> + 'case': variant.name,
>> + 'type': self._use_type(variant.type)
>> + }
>> return _make_tree(obj, variant.ifcond, None)
>>
>> - def visit_builtin_type(self, name, info, json_type):
>> + def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
>
> A built-in's type info is always None. Perhaps we should drop the
> parameter.
>
QAPISchemaBuiltinType -> QAPISchemaType -> QAPISchemaEntity
This class ultimately takes an info parameter in the constructor which
is Optional, which means that the type for the info field is
Optional[QAPISourceInfo].
If you want to remove the parameter here, that works.
>> + json_type: str) -> None:
>> self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
>>
>> - def visit_enum_type(self, name, info, ifcond, features, members, prefix):
>> + def visit_enum_type(self, name: str, info: QAPISourceInfo,
>> + ifcond: List[str], features: List[QAPISchemaFeature],
>> + members: List[QAPISchemaEnumMember],
>> + prefix: Optional[str]) -> None:
>> self._gen_tree(name, 'enum',
>> {'values': [_make_tree(m.name, m.ifcond, None)
>> for m in members]},
>> ifcond, features)
>>
>> - def visit_array_type(self, name, info, ifcond, element_type):
>> + def visit_array_type(self, name: str, info: Optional[QAPISourceInfo],
>
> Here, @info is indeed optional: it's None when @element_type is a
> built-in type.
>
>> + ifcond: List[str],
>> + element_type: QAPISchemaType) -> None:
>> element = self._use_type(element_type)
>> self._gen_tree('[' + element + ']', 'array', {'element-type': element},
>> ifcond, None)
>>
>> - def visit_object_type_flat(self, name, info, ifcond, features,
>> - members, variants):
>> - obj = {'members': [self._gen_member(m) for m in members]}
>> + def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
>
> And here it is optional due to the internal object type 'q_empty'.
>
>> + ifcond: List[str],
>> + features: List[QAPISchemaFeature],
>> + members: Sequence[QAPISchemaObjectTypeMember],
>> + variants: Optional[QAPISchemaVariants]) -> None:
>
> We represent "no variants" as None, not as []. I guess we could
> eliminate use [], trading a tiny bit of efficiency for simpler typing.
> Not a demand.
>
For a later series:
I recommend turning QAPISchemaVariants into an extension of
Sequence[QAPISchemaVariant], and then always creating an empty
collection for the purpose of simplifying the type signature.
I'd recommend the following magicks:
__bool__ -- for writing "if variants: ..."
__iter__ -- for writing "for variant in variants: ..."
Then we can just always say "variants: QAPISchemaVariants" and go about
our lives.
(Maybe that won't work, QAPISchemaVariants has a lot of other parameters
it takes that maybe don't apply to empty collections. Something to come
back to, I think.)
>> + obj: _DObject = {'members': [self._gen_member(m) for m in members]}
>> if variants:
>> obj.update(self._gen_variants(variants.tag_member.name,
>> variants.variants))
>>
>> self._gen_tree(name, 'object', obj, ifcond, features)
>>
>> - def visit_alternate_type(self, name, info, ifcond, features, variants):
>> + def visit_alternate_type(self, name: str, info: QAPISourceInfo,
>> + ifcond: List[str],
>> + features: List[QAPISchemaFeature],
>> + variants: QAPISchemaVariants) -> None:
>> self._gen_tree(name, 'alternate',
>> {'members': [
>> _make_tree({'type': self._use_type(m.type)},
>> @@ -225,24 +300,32 @@ def visit_alternate_type(self, name, info, ifcond, features, variants):
>> for m in variants.variants]},
>> ifcond, features)
>>
>> - def visit_command(self, name, info, ifcond, features,
>> - arg_type, ret_type, gen, success_response, boxed,
>> - allow_oob, allow_preconfig, coroutine):
>> + def visit_command(self, name: str, info: QAPISourceInfo, ifcond: List[str],
>> + features: List[QAPISchemaFeature],
>> + arg_type: QAPISchemaObjectType,
>> + ret_type: Optional[QAPISchemaType], gen: bool,
>
> Are you sure arg_type can't be None?
>
I am not.
Strict optional checking is still disabled at this point in the series,
there are problems like this that it would uncover if I turned it on,
but many other errors are spurious. I could change approach and begin
enabling it right away to make these discrepancies go away -- at the
expense of maybe more "noise" with various assertions and other fluff.
Turning on strict optional at the beginning of part 2 shows 96 errors.
Adding a quick dumb type kludge for genc/genh, there's 18 errors prior
to series 2. I can make a "part 1.5" and try to whittle that down to
zero if you're willing to review it. Another quick edit to commands.py,
I'm down to 15 errors all in gen.py.
It might be a lot of frustrating fluff, but it will make Optional[T] vs
T auditing a bit easier (if not more cumbersome on the way.)
In order to turn it on, though, I do have to go all the way to the end
of the six parts, and then backport the fixes to the earlier parts. You
won't necessarily notice a difference until schema.py itself is fully typed.
--js
>> + success_response: bool, boxed: bool, allow_oob: bool,
>> + allow_preconfig: bool, coroutine: bool) -> None:
>> arg_type = arg_type or self._schema.the_empty_object_type
>> ret_type = ret_type or self._schema.the_empty_object_type
>> - obj = {'arg-type': self._use_type(arg_type),
>> - 'ret-type': self._use_type(ret_type)}
>> + obj: _DObject = {
>> + 'arg-type': self._use_type(arg_type),
>> + 'ret-type': self._use_type(ret_type)
>> + }
>> if allow_oob:
>> obj['allow-oob'] = allow_oob
>> self._gen_tree(name, 'command', obj, ifcond, features)
>>
>> - def visit_event(self, name, info, ifcond, features, arg_type, boxed):
>> + def visit_event(self, name: str, info: QAPISourceInfo,
>> + ifcond: List[str], features: List[QAPISchemaFeature],
>> + arg_type: QAPISchemaObjectType, boxed: bool) -> None:
>> arg_type = arg_type or self._schema.the_empty_object_type
>> self._gen_tree(name, 'event', {'arg-type': self._use_type(arg_type)},
>> ifcond, features)
>>
>>
>> -def gen_introspect(schema, output_dir, prefix, opt_unmask):
>> +def gen_introspect(schema: QAPISchema, output_dir: str, prefix: str,
>> + opt_unmask: bool) -> None:
>> vis = QAPISchemaGenIntrospectVisitor(prefix, opt_unmask)
>> schema.visit(vis)
>> vis.write(output_dir)
>> diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
>> index 74fc6c82153..c0f2a58306d 100644
>> --- a/scripts/qapi/mypy.ini
>> +++ b/scripts/qapi/mypy.ini
>> @@ -14,11 +14,6 @@ disallow_untyped_defs = False
>> disallow_incomplete_defs = False
>> check_untyped_defs = False
>>
>> -[mypy-qapi.introspect]
>> -disallow_untyped_defs = False
>> -disallow_incomplete_defs = False
>> -check_untyped_defs = False
>> -
>> [mypy-qapi.parser]
>> disallow_untyped_defs = False
>> disallow_incomplete_defs = False
>> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>> index 720449feee4..e91b77fadc3 100644
>> --- a/scripts/qapi/schema.py
>> +++ b/scripts/qapi/schema.py
>> @@ -28,7 +28,7 @@
>> class QAPISchemaEntity:
>> meta: Optional[str] = None
>>
>> - def __init__(self, name, info, doc, ifcond=None, features=None):
>> + def __init__(self, name: str, info, doc, ifcond=None, features=None):
>> assert name is None or isinstance(name, str)
>> for f in features or []:
>> assert isinstance(f, QAPISchemaFeature)
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations
2020-12-07 23:48 ` John Snow
@ 2020-12-16 7:51 ` Markus Armbruster
2020-12-16 17:49 ` Eduardo Habkost
2020-12-17 1:35 ` John Snow
0 siblings, 2 replies; 48+ messages in thread
From: Markus Armbruster @ 2020-12-16 7:51 UTC (permalink / raw)
To: John Snow; +Cc: qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> On 11/13/20 11:48 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> The typing of _make_tree and friends is a bit involved, but it can be
>>> done with some stubbed out types and a bit of elbow grease. The
>>> forthcoming patches attempt to make some simplifications, but having the
>>> type hints in advance may aid in review of subsequent patches.
>>>
>>>
>>> Some notes on the abstract types used at this point, and what they
>>> represent:
>>>
>>> - TreeValue represents any object in the type tree. _make_tree is an
>>> optional call -- not every node in the final type tree will have been
>>> passed to _make_tree, so this type encompasses not only what is passed
>>> to _make_tree (dicts, strings) or returned from it (dicts, strings, a
>>> 2-tuple), but any recursive value for any of the dicts passed to
>>> _make_tree -- which includes lists, strings, integers, null constants,
>>> and so on.
>>>
>>> - _DObject is a type alias I use to mean "A JSON-style object,
>>> represented as a Python dict." There is no "JSON" type in Python, they
>>> are converted natively to recursively nested dicts and lists, with
>>> leaf values of str, int, float, None, True/False and so on. This type
>>> structure is not possible to accurately portray in mypy yet, so a
>>> placeholder is used.
>>>
>>> In this case, _DObject is being used to refer to SchemaInfo-like
>>> structures as defined in qapi/introspect.json, OR any sub-object
>>> values they may reference. We don't have strong typing available for
>>> those, so a generic alternative is used.
>>>
>>> - Extra refers explicitly to the dict containing "extra" information
>>> about a node in the tree. mypy does not offer per-key typing for dicts
>>> in Python 3.6, so this is the best we can do here.
>>>
>>> - Annotated refers to (one of) the return types of _make_tree:
>>> It represents a 2-tuple of (TreeValue, Extra).
>>>
>>>
>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>> scripts/qapi/introspect.py | 157 ++++++++++++++++++++++++++++---------
>>> scripts/qapi/mypy.ini | 5 --
>>> scripts/qapi/schema.py | 2 +-
>>> 3 files changed, 121 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>>> index 63f721ebfb6..803288a64e7 100644
>>> --- a/scripts/qapi/introspect.py
>>> +++ b/scripts/qapi/introspect.py
>>> @@ -10,7 +10,16 @@
>>> See the COPYING file in the top-level directory.
>>> """
>>>
>>> -from typing import Optional, Sequence, cast
>>> +from typing import (
>>> + Any,
>>> + Dict,
>>> + List,
>>> + Optional,
>>> + Sequence,
>>> + Tuple,
>>> + Union,
>>> + cast,
>>> +)
>>>
>>> from .common import (
>>> c_name,
>>> @@ -20,13 +29,56 @@
>>> )
>>> from .gen import QAPISchemaMonolithicCVisitor
>>> from .schema import (
>>> + QAPISchema,
>>> QAPISchemaArrayType,
>>> QAPISchemaBuiltinType,
>>> + QAPISchemaEntity,
>>> + QAPISchemaEnumMember,
>>> + QAPISchemaFeature,
>>> + QAPISchemaObjectType,
>>> + QAPISchemaObjectTypeMember,
>>> QAPISchemaType,
>>> + QAPISchemaVariant,
>>> + QAPISchemaVariants,
>>> )
>>> +from .source import QAPISourceInfo
>>>
>>>
>>> -def _make_tree(obj, ifcond, features, extra=None):
>>> +# This module constructs a tree-like data structure that is used to
>>
>> "Tree-like" suggests it's not a tree, it just looks like one if you
>> squint. Drop "-like"?
>>
>
> Sure. I think I am grammatically predisposed to assume "binary tree" or
> at least some kind of monomorphic tree when I see "tree", hence the
> hedging and weasel-words.
>
> No problem just to drop it.
>
>>> +# generate the introspection information for QEMU. It behaves similarly
>>> +# to a JSON value.
>>> +#
>>> +# A complexity over JSON is that our values may or may not be annotated.
>>
>> It's the obvious abstract syntax tree for JSON, hacked up^W^Wextended to
>> support certain annotations.
>>
>
> Yes.
>
>> Let me add a bit of context and history.
>>
>> The module's job is generating qapi-introspect.[ch] for a QAPISchema.
>>
>> The purpose of qapi-introspect.[ch] is providing the information
>> query-qmp-schema needs, i.e. (a suitable C representation of) a JSON
>> value conforming to [SchemaInfo]. Details of this C representation are
>> not interesting right now.
>>
>> We first go from QAPISchema to a suitable Python representation of
>> [SchemaInfo], then from there to the C source code, neatly separating
>> concerns.
>>
>> Stupidest solution Python representation that could possibly work: the
>> obvious abstract syntax tree for JSON (that's also how Python's json
>> module works).
>>
>> Parts corresponding to QAPISchema parts guarded by 'if' conditionals
>> need to be guarded by #if conditionals.
>>
>> We want to prefix parts corresponding to certain QAPISchema parts with a
>> comment.
>>
>> These two requirements came later, and were hacked into the existing
>> stupidest solution: any tree node can be a tuple (json, extra), where
>> json is the "stupidest" node, and extra is a dict of annotations. In
>> other words, to annotate an unannotated node N with dict D, replace N by
>> (N, D).
>>
>> Possible annotations:
>>
>> 'comment': str
>> 'if': Sequence[str]
>>
>> They say there are just three answers a Marine may give to an officer's
>> questions: "Yes, sir!", "No, sir!", "No excuse, sir!". Let me put that
>> to use here:
>>
>> Is this an elegant design? No, sir!
>>
>> Is the code easy to read? No excuse, sir!
>>
>> Was it cheap to make? Yes, sir!
>>
>
> Yes, I believe I am on the same page so far. It was difficult to read
> and difficult to annotate, but once I got to the bottom, it's easy to
> see why it happened this way in retrospect. It's the simplest thing that
> could work.
>
> And it does work!
>
> So far, this comment isn't really a correction on what I wrote, but if
> you believe my comment that explains the types could be clearer, feel
> free to suggest.
Yes, it's not correction, it's elaboration.
You guys clearly struggled with the tree data structure. Documentation
would have helped[*]. Since you're going to replace it (PATCH 09),
adding it now makes little sense.
*My* struggle is with the type annotations.
The initial state is messy to type, in part due to mypy's surprising
inability to deal with recursive types, in part because the tree data
structure is messier than it could be.
Much of the series is cleanup that benefits the typing. Makes sense.
What makes review hard for me: you add (fairly complicated) typing
first, then evolve it along with the cleanups. I have to first grok the
complicated typing (a struggle), then for each cleanup grok the type
changes in addition to the code changes.
I believe adding the typing before the cleanups is a mistake.
I share the desire to have type annotations that help with understanding
the code. I understand the desire to have them sooner rather than
later. I just think they're a costly distraction at this stage for this
code. Once you understand the data structure, the cleanups are fairly
straightforward.
Is it too late to delay the introduction of type hints until after the
data structure cleanups?
If yes, I have to spend more time replying below.
>>> +#
>>> +# Un-annotated values may be:
>>> +# Scalar: str, bool, None.
>>> +# Non-scalar: List, Dict
>>> +# _Value = Union[str, bool, None, Dict[str, Value], List[Value]]
>>> +#
>>> +# With optional annotations, the type of all values is:
>>> +# TreeValue = Union[_Value, Annotated[_Value]]
>>> +#
>>> +# Sadly, mypy does not support recursive types, so we must approximate this.
>>> +_stub = Any
>>> +_scalar = Union[str, bool, None]
>>> +_nonscalar = Union[Dict[str, _stub], List[_stub]]
>>> +_value = Union[_scalar, _nonscalar]
>>> +TreeValue = Union[_value, 'Annotated']
>>
>> Are there naming conventions for this kind of variables? I'm asking
>> because you capitalize some, but not all, and I can't see a pattern.
>>
>
> Types generally get CamelCase names; though for interior aliases I used
> underscore + lowercase. In this case, I was trying to illustrate that
> these intermediate types were only interesting or useful in service of
> the Capitalized Thing, TreeValue.
>
>> Ignorant question: only 'Annotated' has quotes; why?
>>
>
> It hasn't been defined yet, so it's a forward reference. Cleber
> suggested I hoist its definition up to avoid this. I forget if I had
> some strong reason for doing it this way, admittedly.
>
> A lot of this code gets changed shortly in the following patches, so
> honestly I was likely just not really putting in a lot of effort to make
> code that gets deleted soon pretty.
>
> (Why did the patches go in that order then? During early review, Eduardo
> wanted to see the type hints go in first to help review for the cleanup
> be easier ...!)
Understandable, but it didn't play out well, I'm afraid.
>> There is "_Value", "Value" and "_value". Suggest to add "value"
>> somewhere, for completeness ;-P
>>
>
> Yeah, my mistake. See my response to Cleber here...
>
>> I find the names _value and TreeValue a bit unfortunate: the difference
>> between the two isn't Tree. I'll come back to this below.
>>
>
> Feel free to suggest something better; I am at a point with this
> particular module where I'd be happy to have someone more opinionated
> than me telling me what they want.
>
>>> +
>>> +# This is just an alias for an object in the structure described above:
>>> +_DObject = Dict[str, object]
>>
>> I'm confused. Which structure, and why do we want to alias it?
>>
>
> Yep, I don't have a good name for this either. I mean this to be a
> generic type that describes the natural python representation for a JSON
> object.
>
> i.e. Dict[str, THING].
>
> All of the various bits and pieces here that are generating SchemaInfo
> subtype representations (like _gen_member, _gen_variant, etc.) are using
> this as a generic "Well, it's some kind of dict/object."
>
> (Aside: this is an interesting part of code, because it is not type safe
> with respect to the QAPI definitions that define SchemaInfo's descendant
> types. There's no more explicit type for me to use here to describe
> those objects.)
>
>>> +
>>> +# Represents the annotations themselves:
>>> +Annotations = Dict[str, object]
>>
>> Losely typed. I have no idea whether that's bad :)
>>
>
> Kind of: unlike Any, using 'object' means that if we treat the
> dictionary values in a manner that we could not treat *all* values, mypy
> will raise an error.
>
> So it is a very broad type, but it's "safe".
>
>>> +
>>> +# Represents an annotated node (of some kind).
>>> +Annotated = Tuple[_value, Annotations]
>>
>> So, _value seems to represent a JSON value, Annotated an annotated JSON
>> value, and TreeValue their union, i.e. a possibly annotated JSON value.
>>
>> Naming is hard... BareJsonValue, AnnotatedJsonValue, JsonValue?
>>
>
> I was afraid of using "JsonValue" to avoid implicating it as a literal
> JSON value -- since it isn't, exactly. It's the Python native
> approximation of a subset of JSON values.
>
>>> +
>>> +
>>> +def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
>>
>> I'd expect obj: _value, i.e. "unannotated value".
>>
>
> Sure, and I believe that would work -- this type is "tighter".
> _make_tree only ever sees Dict[str, object] and str, actually.
>
> Later in the series, _make_tree goes away and this weird sub-type also
> goes away in favor of the more generic type.
>
>>> + features: List[QAPISchemaFeature],
>>> + extra: Optional[Annotations] = None
>>> + ) -> TreeValue:
>>> if extra is None:
>>> extra = {}
>>> if ifcond:
>>> @@ -39,9 +91,11 @@ def _make_tree(obj, ifcond, features, extra=None):
>>> return obj
>>>
>>>
>>> -def _tree_to_qlit(obj, level=0, suppress_first_indent=False):
>>> +def _tree_to_qlit(obj: TreeValue,
>>> + level: int = 0,
>>> + suppress_first_indent: bool = False) -> str:
>>>
>>> - def indent(level):
>>> + def indent(level: int) -> str:
>>> return level * 4 * ' '
>>>
>>> if isinstance(obj, tuple):
>>> @@ -91,21 +145,20 @@ def indent(level):
>>> return ret
>>>
>>>
>>> -def to_c_string(string):
>>> +def to_c_string(string: str) -> str:
>>> return '"' + string.replace('\\', r'\\').replace('"', r'\"') + '"'
>>>
>>>
>>> class QAPISchemaGenIntrospectVisitor(QAPISchemaMonolithicCVisitor):
>>> -
>>
>> Intentional?
>>
>
> I'm sure I thought it looked nice at the time. It's not important.
>
>>> - def __init__(self, prefix, unmask):
>>> + def __init__(self, prefix: str, unmask: bool):
>>> super().__init__(
>>> prefix, 'qapi-introspect',
>>> ' * QAPI/QMP schema introspection', __doc__)
>>> self._unmask = unmask
>>> - self._schema = None
>>> - self._trees = []
>>> - self._used_types = []
>>> - self._name_map = {}
>>> + self._schema: Optional[QAPISchema] = None
>>> + self._trees: List[TreeValue] = []
>>> + self._used_types: List[QAPISchemaType] = []
>>> + self._name_map: Dict[str, str] = {}
>>> self._genc.add(mcgen('''
>>> #include "qemu/osdep.h"
>>> #include "%(prefix)sqapi-introspect.h"
>>> @@ -113,10 +166,10 @@ def __init__(self, prefix, unmask):
>>> ''',
>>> prefix=prefix))
>>>
>>> - def visit_begin(self, schema):
>>> + def visit_begin(self, schema: QAPISchema) -> None:
>>> self._schema = schema
>>>
>>> - def visit_end(self):
>>> + def visit_end(self) -> None:
>>> # visit the types that are actually used
>>> for typ in self._used_types:
>>> typ.visit(self)
>>> @@ -138,18 +191,18 @@ def visit_end(self):
>>> self._used_types = []
>>> self._name_map = {}
>>>
>>> - def visit_needed(self, entity):
>>> + def visit_needed(self, entity: QAPISchemaEntity) -> bool:
>>> # Ignore types on first pass; visit_end() will pick up used types
>>> return not isinstance(entity, QAPISchemaType)
>>>
>>> - def _name(self, name):
>>> + def _name(self, name: str) -> str:
>>> if self._unmask:
>>> return name
>>> if name not in self._name_map:
>>> self._name_map[name] = '%d' % len(self._name_map)
>>> return self._name_map[name]
>>>
>>> - def _use_type(self, typ):
>>> + def _use_type(self, typ: QAPISchemaType) -> str:
>>> # Map the various integer types to plain int
>>> if typ.json_type() == 'int':
>>> typ = self._schema.lookup_type('int')
>>> @@ -168,8 +221,10 @@ def _use_type(self, typ):
>>> return '[' + self._use_type(typ.element_type) + ']'
>>> return self._name(typ.name)
>>>
>>> - def _gen_tree(self, name, mtype, obj, ifcond, features):
>>> - extra = None
>>> + def _gen_tree(self, name: str, mtype: str, obj: _DObject,
>>> + ifcond: List[str],
>>> + features: Optional[List[QAPISchemaFeature]]) -> None:
>>
>> _gen_tree() builds a complete tree (i.e. one SchemaInfo), and adds it to
>> ._trees.
>>
>> The SchemaInfo's common parts are name, meta-type and features.
>> _gen_tree() takes them as arguments @name, @mtype, @features.
>>
>> It takes SchemaInfo's variant parts as a dict @obj.
>>
>> It completes @obj into an unannotated tree node by the common parts into
>> @obj.
>>
>> It also takes a QAPI conditional argument @ifcond.
>>
>> Now let me review the type annotations:
>>
>> * name: str matches SchemaInfo, good.
>>
>> * mtype: str approximates SchemaInfo's enum SchemaMetaType (it's not
>> Python Enum because those were off limits when this code was written).
>>
>
> It's also a little cumbersome, perhaps, to duplicate information from
> the QAPI schema directly into the QAPI generator.
qapi/introspect.json is strongly coupled with
scripts/qapi/introspect.py. Not proud of it, but I don't have better
ideas.
> Didn't feel like getting clever enough to "fix" this.
>
>> * obj: _DObject ... I'd expect "unannotated JSON value".
>>
>
> It's NOT any value though -- like you said: "_gen_tree() builds a
> complete tree (i.e. one SchemaInfo)" -- we only accept objects here,
> which is correct.
>
>> * ifcond: List[str] should work, but gen_if(), gen_endif() use
>> Sequence[str]. Suggest to pick one (please explain why), and stick to
>> it.
>>
>> More instances of ifcond: List[str] elsewhere; I'm not flagging them.
>>
>
> Yes, I should probably be using Sequence, if I can. generally:
>
> - Input types should use the most generic type they can cope with
> (Iterable, Sequence, Collection) based on what properties they actually
> need in the incoming type.
>
> (By the end of this series, Iterable[str] should actually be sufficient,
> but I'll have to see where it makes sense to slacken the input types in
> this series. It's been through the washer quite a few times I'm afraid.)
>
> - Output types should be as explicit as possible.
Got it.
> I was not always perfectly good about generalizing the input types; List
> is correct, but not necessarily maximally correct.
>
>> * features: Optional[List[QAPISchemaFeature]] is correct. "No features"
>> has two representations: None and []. I guess we could eliminate
>> None, trading a tiny bit of efficiency for simpler typing. Not a
>> demand.
>>
>>> + extra: Optional[Annotations] = None
>>
>> "No annotations" is represented as None here, not {}. I guess we could
>> use {} for simpler typing. Not a demand.
>>
>
> This goes away later, kinda. It becomes:
>
> comment: Optional[str] = None
>
> and that comment is eventually passed to an Annotated node constructor
> that takes the comment specifically.
>
>>> if mtype not in ('command', 'event', 'builtin', 'array'):
>>> if not self._unmask:
>>> # Output a comment to make it easy to map masked names
>>> @@ -180,44 +235,64 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
>>> obj['meta-type'] = mtype
>>> self._trees.append(_make_tree(obj, ifcond, features, extra))
>>>
>>> - def _gen_member(self, member):
>>> - obj = {'name': member.name, 'type': self._use_type(member.type)}
>>> + def _gen_member(self,
>>> + member: QAPISchemaObjectTypeMember) -> TreeValue:
>>> + obj: _DObject = {
>>
>> I'd expect "unannotated value". More of the same below.
>>
>
> See my remark to your expectation for what _gen_tree should accept. It
> deals with "objects" and "objects" have the property that more keys can
> be assigned to them, and this is a fundamental feature of _gen_tree.
>
>>> + 'name': member.name,
>>> + 'type': self._use_type(member.type)
>>> + }
>>> if member.optional:
>>> obj['default'] = None
>>> return _make_tree(obj, member.ifcond, member.features)
>>>
>>> - def _gen_variants(self, tag_name, variants):
>>> + def _gen_variants(self, tag_name: str,
>>> + variants: List[QAPISchemaVariant]) -> _DObject:
>>> return {'tag': tag_name,
>>> 'variants': [self._gen_variant(v) for v in variants]}
>>>
>>> - def _gen_variant(self, variant):
>>> - obj = {'case': variant.name, 'type': self._use_type(variant.type)}
>>> + def _gen_variant(self, variant: QAPISchemaVariant) -> TreeValue:
>>> + obj: _DObject = {
>>> + 'case': variant.name,
>>> + 'type': self._use_type(variant.type)
>>> + }
>>> return _make_tree(obj, variant.ifcond, None)
>>>
>>> - def visit_builtin_type(self, name, info, json_type):
>>> + def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
>>
>> A built-in's type info is always None. Perhaps we should drop the
>> parameter.
>>
>
> QAPISchemaBuiltinType -> QAPISchemaType -> QAPISchemaEntity
>
> This class ultimately takes an info parameter in the constructor which
> is Optional, which means that the type for the info field is
> Optional[QAPISourceInfo].
I understand you've since worked on tightening Optional all over, so
this might be moot.
> If you want to remove the parameter here, that works.
>
>>> + json_type: str) -> None:
>>> self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
>>>
>>> - def visit_enum_type(self, name, info, ifcond, features, members, prefix):
>>> + def visit_enum_type(self, name: str, info: QAPISourceInfo,
>>> + ifcond: List[str], features: List[QAPISchemaFeature],
>>> + members: List[QAPISchemaEnumMember],
>>> + prefix: Optional[str]) -> None:
>>> self._gen_tree(name, 'enum',
>>> {'values': [_make_tree(m.name, m.ifcond, None)
>>> for m in members]},
>>> ifcond, features)
>>>
>>> - def visit_array_type(self, name, info, ifcond, element_type):
>>> + def visit_array_type(self, name: str, info: Optional[QAPISourceInfo],
>>
>> Here, @info is indeed optional: it's None when @element_type is a
>> built-in type.
>>
>>> + ifcond: List[str],
>>> + element_type: QAPISchemaType) -> None:
>>> element = self._use_type(element_type)
>>> self._gen_tree('[' + element + ']', 'array', {'element-type': element},
>>> ifcond, None)
>>>
>>> - def visit_object_type_flat(self, name, info, ifcond, features,
>>> - members, variants):
>>> - obj = {'members': [self._gen_member(m) for m in members]}
>>> + def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
>>
>> And here it is optional due to the internal object type 'q_empty'.
>>
>>> + ifcond: List[str],
>>> + features: List[QAPISchemaFeature],
>>> + members: Sequence[QAPISchemaObjectTypeMember],
>>> + variants: Optional[QAPISchemaVariants]) -> None:
>>
>> We represent "no variants" as None, not as []. I guess we could
>> eliminate use [], trading a tiny bit of efficiency for simpler typing.
>> Not a demand.
>>
>
> For a later series:
>
> I recommend turning QAPISchemaVariants into an extension of
> Sequence[QAPISchemaVariant],
I admittedly wasn't deep enough into Python at the time to even think of
this.
> and then always creating an empty
> collection for the purpose of simplifying the type signature.
>
> I'd recommend the following magicks:
>
> __bool__ -- for writing "if variants: ..."
> __iter__ -- for writing "for variant in variants: ..."
>
> Then we can just always say "variants: QAPISchemaVariants" and go about
> our lives.
>
> (Maybe that won't work, QAPISchemaVariants has a lot of other parameters
> it takes that maybe don't apply to empty collections. Something to come
> back to, I think.)
One way to find out whether it's an improvement. Not now.
[...]
[*] Dearth of documentation making the code hard to understand is a
common theme in scripts/qapi/. Sorry about that.
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations
2020-12-16 7:51 ` Markus Armbruster
@ 2020-12-16 17:49 ` Eduardo Habkost
2020-12-17 6:51 ` Markus Armbruster
2020-12-17 1:35 ` John Snow
1 sibling, 1 reply; 48+ messages in thread
From: Eduardo Habkost @ 2020-12-16 17:49 UTC (permalink / raw)
To: Markus Armbruster; +Cc: John Snow, qemu-devel, Cleber Rosa
On Wed, Dec 16, 2020 at 08:51:10AM +0100, Markus Armbruster wrote:
[...]
> You guys clearly struggled with the tree data structure. Documentation
> would have helped[*]. Since you're going to replace it (PATCH 09),
> adding it now makes little sense.
>
> *My* struggle is with the type annotations.
>
> The initial state is messy to type, in part due to mypy's surprising
> inability to deal with recursive types, in part because the tree data
> structure is messier than it could be.
>
> Much of the series is cleanup that benefits the typing. Makes sense.
>
> What makes review hard for me: you add (fairly complicated) typing
> first, then evolve it along with the cleanups. I have to first grok the
> complicated typing (a struggle), then for each cleanup grok the type
> changes in addition to the code changes.
>
> I believe adding the typing before the cleanups is a mistake.
Possibly my fault, as I remember asking John to do that (in
earlier versions of these patches, type annotations were added
after cleanup).
>
> I share the desire to have type annotations that help with understanding
> the code. I understand the desire to have them sooner rather than
> later. I just think they're a costly distraction at this stage for this
> code. Once you understand the data structure, the cleanups are fairly
> straightforward.
>
I expected the type annotations to be a simple and helpful tool
for understanding the data structure before refactoring. In the
case of introspect.py, I was completely wrong about "simple", and
I'm not entirely sure about "helpful".
I wasn't expecting them to be an obstacle for patch review,
though. If the type annotations look good at the end of the
series, do we care about the intermediate state? Bisectability
isn't an issue because type annotations are ignored by the Python
interpreter.
--
Eduardo
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations
2020-12-16 17:49 ` Eduardo Habkost
@ 2020-12-17 6:51 ` Markus Armbruster
0 siblings, 0 replies; 48+ messages in thread
From: Markus Armbruster @ 2020-12-17 6:51 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: John Snow, qemu-devel, Cleber Rosa
Eduardo Habkost <ehabkost@redhat.com> writes:
> On Wed, Dec 16, 2020 at 08:51:10AM +0100, Markus Armbruster wrote:
> [...]
>> You guys clearly struggled with the tree data structure. Documentation
>> would have helped[*]. Since you're going to replace it (PATCH 09),
>> adding it now makes little sense.
>>
>> *My* struggle is with the type annotations.
>>
>> The initial state is messy to type, in part due to mypy's surprising
>> inability to deal with recursive types, in part because the tree data
>> structure is messier than it could be.
>>
>> Much of the series is cleanup that benefits the typing. Makes sense.
>>
>> What makes review hard for me: you add (fairly complicated) typing
>> first, then evolve it along with the cleanups. I have to first grok the
>> complicated typing (a struggle), then for each cleanup grok the type
>> changes in addition to the code changes.
>>
>> I believe adding the typing before the cleanups is a mistake.
>
> Possibly my fault, as I remember asking John to do that (in
> earlier versions of these patches, type annotations were added
> after cleanup).
>
>>
>> I share the desire to have type annotations that help with understanding
>> the code. I understand the desire to have them sooner rather than
>> later. I just think they're a costly distraction at this stage for this
>> code. Once you understand the data structure, the cleanups are fairly
>> straightforward.
>>
>
> I expected the type annotations to be a simple and helpful tool
> for understanding the data structure before refactoring. In the
> case of introspect.py, I was completely wrong about "simple", and
> I'm not entirely sure about "helpful".
Quite excusable. We lack the mypy experience to predict such outcomes.
> I wasn't expecting them to be an obstacle for patch review,
> though. If the type annotations look good at the end of the
> series, do we care about the intermediate state? Bisectability
> isn't an issue because type annotations are ignored by the Python
> interpreter.
I don't worry about bisectability. The issue is reviewability.
Here's the best case for me reviewing a single patch. First, the commit
message convinces me this makes sense. Then I read the patch mostly in
order. It does what the commit message made me expect, I think I
understand how it does it, and it doesn't touch anything I know to be
subtle.
Here's the best case for me reviewing a patch series: every patch in
order is a best case review.
As soon as review deviates from this best case, I slow down. A lot. If
there is something I didn't expect, maybe I'm misunderstanding the
patch's purpose. If I feel confused about how the patch achieves its
purpose, I better figure it out. If something subtle is being touched,
I better recall its subtleties and carefully check the patch. Slow and
exhausting work.
This way of review can be overly careful. But even deciding "this isn't
important, let it go" is slow, unless I do it wholesale. All we get
then is "looks good at a glance". But that's maybe an Acked-by,
certainly not a Reviewed-by.
Me finding the patch where the type hints start to be "serious" is slow.
Me mentally separating changes to type hints from other changes in
patches before that point is slow. Me examining the type hints at that
point (which need not be entirely visible in the patch) is slow.
If the annotations in the intermediate state don't have to be good, do
they have to be there? If John can take them out, review will be easier
and faster.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations
2020-12-16 7:51 ` Markus Armbruster
2020-12-16 17:49 ` Eduardo Habkost
@ 2020-12-17 1:35 ` John Snow
2020-12-17 7:00 ` Markus Armbruster
1 sibling, 1 reply; 48+ messages in thread
From: John Snow @ 2020-12-17 1:35 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Cleber Rosa, qemu-devel, Eduardo Habkost
On 12/16/20 2:51 AM, Markus Armbruster wrote:
>
> Is it too late to delay the introduction of type hints until after the
> data structure cleanups?
>
> If yes, I have to spend more time replying below.
>
No, I re-ordered the series again to delay typing until the end, or
close to it.
Though I abandoned plans to slacken List[...] inputs to Iterable[...] or
Sequence[...] like I had mentioned; I think it could still be done, but
it's fine to just use List[...] for the inputs for now. We can worry
about optimizing type flexibility later, I think.
Let's just get the dog hunting at all first.
--js
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations
2020-12-17 1:35 ` John Snow
@ 2020-12-17 7:00 ` Markus Armbruster
0 siblings, 0 replies; 48+ messages in thread
From: Markus Armbruster @ 2020-12-17 7:00 UTC (permalink / raw)
To: John Snow; +Cc: qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> On 12/16/20 2:51 AM, Markus Armbruster wrote:
>> Is it too late to delay the introduction of type hints until after
>> the
>> data structure cleanups?
>> If yes, I have to spend more time replying below.
>>
>
> No, I re-ordered the series again to delay typing until the end, or
> close to it.
Thanks!
> Though I abandoned plans to slacken List[...] inputs to Iterable[...]
> or Sequence[...] like I had mentioned; I think it could still be done,
> but it's fine to just use List[...] for the inputs for now. We can
> worry about optimizing type flexibility later, I think.
Shouldn't be a problem now I know what to expect.
> Let's just get the dog hunting at all first.
Yes.
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v2 06/11] qapi/introspect.py: add _gen_features helper
2020-10-26 19:42 [PATCH v2 00/11] qapi: static typing conversion, pt2 John Snow
` (4 preceding siblings ...)
2020-10-26 19:42 ` [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations John Snow
@ 2020-10-26 19:42 ` John Snow
2020-11-07 4:23 ` Cleber Rosa
2020-11-16 8:47 ` Markus Armbruster
2020-10-26 19:42 ` [PATCH v2 07/11] qapi/introspect.py: Unify return type of _make_tree() John Snow
` (6 subsequent siblings)
12 siblings, 2 replies; 48+ messages in thread
From: John Snow @ 2020-10-26 19:42 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster; +Cc: John Snow, Eduardo Habkost, Cleber Rosa
_make_tree might receive a dict or some other type. Adding features
information should arguably be performed by the caller at such a time
when we know the type of the object and don't have to re-interrogate it.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 803288a64e7..16282f2634b 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -76,16 +76,12 @@
def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
- features: List[QAPISchemaFeature],
extra: Optional[Annotations] = None
) -> TreeValue:
if extra is None:
extra = {}
if ifcond:
extra['if'] = ifcond
- if features:
- assert isinstance(obj, dict)
- obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]
if extra:
return (obj, extra)
return obj
@@ -221,6 +217,11 @@ def _use_type(self, typ: QAPISchemaType) -> str:
return '[' + self._use_type(typ.element_type) + ']'
return self._name(typ.name)
+ @classmethod
+ def _gen_features(cls,
+ features: List[QAPISchemaFeature]) -> List[TreeValue]:
+ return [_make_tree(f.name, f.ifcond) for f in features]
+
def _gen_tree(self, name: str, mtype: str, obj: _DObject,
ifcond: List[str],
features: Optional[List[QAPISchemaFeature]]) -> None:
@@ -233,7 +234,9 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject,
name = self._name(name)
obj['name'] = name
obj['meta-type'] = mtype
- self._trees.append(_make_tree(obj, ifcond, features, extra))
+ if features:
+ obj['features'] = self._gen_features(features)
+ self._trees.append(_make_tree(obj, ifcond, extra))
def _gen_member(self,
member: QAPISchemaObjectTypeMember) -> TreeValue:
@@ -243,7 +246,9 @@ def _gen_member(self,
}
if member.optional:
obj['default'] = None
- return _make_tree(obj, member.ifcond, member.features)
+ if member.features:
+ obj['features'] = self._gen_features(member.features)
+ return _make_tree(obj, member.ifcond)
def _gen_variants(self, tag_name: str,
variants: List[QAPISchemaVariant]) -> _DObject:
@@ -255,7 +260,7 @@ def _gen_variant(self, variant: QAPISchemaVariant) -> TreeValue:
'case': variant.name,
'type': self._use_type(variant.type)
}
- return _make_tree(obj, variant.ifcond, None)
+ return _make_tree(obj, variant.ifcond)
def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
json_type: str) -> None:
--
2.26.2
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH v2 06/11] qapi/introspect.py: add _gen_features helper
2020-10-26 19:42 ` [PATCH v2 06/11] qapi/introspect.py: add _gen_features helper John Snow
@ 2020-11-07 4:23 ` Cleber Rosa
2020-11-16 8:47 ` Markus Armbruster
1 sibling, 0 replies; 48+ messages in thread
From: Cleber Rosa @ 2020-11-07 4:23 UTC (permalink / raw)
To: John Snow; +Cc: qemu-devel, Eduardo Habkost, Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 385 bytes --]
On Mon, Oct 26, 2020 at 03:42:46PM -0400, John Snow wrote:
> _make_tree might receive a dict or some other type. Adding features
> information should arguably be performed by the caller at such a time
> when we know the type of the object and don't have to re-interrogate it.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
Reviewed-by: Cleber Rosa <crosa@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v2 06/11] qapi/introspect.py: add _gen_features helper
2020-10-26 19:42 ` [PATCH v2 06/11] qapi/introspect.py: add _gen_features helper John Snow
2020-11-07 4:23 ` Cleber Rosa
@ 2020-11-16 8:47 ` Markus Armbruster
2020-12-07 23:57 ` John Snow
1 sibling, 1 reply; 48+ messages in thread
From: Markus Armbruster @ 2020-11-16 8:47 UTC (permalink / raw)
To: John Snow; +Cc: Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> _make_tree might receive a dict or some other type.
Are you talking about @obj?
> Adding features
> information should arguably be performed by the caller at such a time
> when we know the type of the object and don't have to re-interrogate it.
Fair enough. There are just two such callers anyway.
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/introspect.py | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 803288a64e7..16282f2634b 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -76,16 +76,12 @@
>
>
> def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
> - features: List[QAPISchemaFeature],
> extra: Optional[Annotations] = None
> ) -> TreeValue:
> if extra is None:
> extra = {}
> if ifcond:
> extra['if'] = ifcond
> - if features:
> - assert isinstance(obj, dict)
> - obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]
> if extra:
> return (obj, extra)
> return obj
> @@ -221,6 +217,11 @@ def _use_type(self, typ: QAPISchemaType) -> str:
> return '[' + self._use_type(typ.element_type) + ']'
> return self._name(typ.name)
>
> + @classmethod
> + def _gen_features(cls,
> + features: List[QAPISchemaFeature]) -> List[TreeValue]:
> + return [_make_tree(f.name, f.ifcond) for f in features]
> +
Ignorant question: when to use @classmethod, and when to use
@staticmethod?
> def _gen_tree(self, name: str, mtype: str, obj: _DObject,
> ifcond: List[str],
> features: Optional[List[QAPISchemaFeature]]) -> None:
> @@ -233,7 +234,9 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject,
> name = self._name(name)
> obj['name'] = name
> obj['meta-type'] = mtype
> - self._trees.append(_make_tree(obj, ifcond, features, extra))
> + if features:
> + obj['features'] = self._gen_features(features)
> + self._trees.append(_make_tree(obj, ifcond, extra))
>
> def _gen_member(self,
> member: QAPISchemaObjectTypeMember) -> TreeValue:
No change when not features. Else, you change
obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]
to
obj['features'] = [_make_tree(f.name, f.ifcond) for f in features]
where
_make_tree(f.name, f.ifcond)
= (f.name, {'if': f.ifcond}) if f.ifcond
= f.name else
Works, and feels less lazy. However, the commit message did not prepare
me for this. If you split this off into its own patch, you can describe
it properly.
> @@ -243,7 +246,9 @@ def _gen_member(self,
> }
> if member.optional:
> obj['default'] = None
> - return _make_tree(obj, member.ifcond, member.features)
> + if member.features:
> + obj['features'] = self._gen_features(member.features)
> + return _make_tree(obj, member.ifcond)
>
> def _gen_variants(self, tag_name: str,
> variants: List[QAPISchemaVariant]) -> _DObject:
> @@ -255,7 +260,7 @@ def _gen_variant(self, variant: QAPISchemaVariant) -> TreeValue:
> 'case': variant.name,
> 'type': self._use_type(variant.type)
> }
> - return _make_tree(obj, variant.ifcond, None)
> + return _make_tree(obj, variant.ifcond)
>
> def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
> json_type: str) -> None:
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 06/11] qapi/introspect.py: add _gen_features helper
2020-11-16 8:47 ` Markus Armbruster
@ 2020-12-07 23:57 ` John Snow
2020-12-15 16:55 ` Markus Armbruster
0 siblings, 1 reply; 48+ messages in thread
From: John Snow @ 2020-12-07 23:57 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Cleber Rosa, qemu-devel, Eduardo Habkost
On 11/16/20 3:47 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> _make_tree might receive a dict or some other type.
>
> Are you talking about @obj?
>
Yes. It *usually* takes a dict. sometimes it doesn't.
>> Adding features
>> information should arguably be performed by the caller at such a time
>> when we know the type of the object and don't have to re-interrogate it.
>
> Fair enough. There are just two such callers anyway.
>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qapi/introspect.py | 19 ++++++++++++-------
>> 1 file changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index 803288a64e7..16282f2634b 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -76,16 +76,12 @@
>>
>>
>> def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
>> - features: List[QAPISchemaFeature],
>> extra: Optional[Annotations] = None
>> ) -> TreeValue:
>> if extra is None:
>> extra = {}
>> if ifcond:
>> extra['if'] = ifcond
>> - if features:
>> - assert isinstance(obj, dict)
>> - obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]
>> if extra:
>> return (obj, extra)
>> return obj
>> @@ -221,6 +217,11 @@ def _use_type(self, typ: QAPISchemaType) -> str:
>> return '[' + self._use_type(typ.element_type) + ']'
>> return self._name(typ.name)
>>
>> + @classmethod
>> + def _gen_features(cls,
>> + features: List[QAPISchemaFeature]) -> List[TreeValue]:
>> + return [_make_tree(f.name, f.ifcond) for f in features]
>> +
>
> Ignorant question: when to use @classmethod, and when to use
> @staticmethod?
>
Matter of taste. My preference is to just always use @classmethod,
because they can be extended or referenced by subclasses.
@staticmethod does not take a class argument, @classmethod does. Static
methods therefore cannot address any other classmethods, but a
classmethod can.
I just always reach for classmethod by default.
>> def _gen_tree(self, name: str, mtype: str, obj: _DObject,
>> ifcond: List[str],
>> features: Optional[List[QAPISchemaFeature]]) -> None:
>> @@ -233,7 +234,9 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject,
>> name = self._name(name)
>> obj['name'] = name
>> obj['meta-type'] = mtype
>> - self._trees.append(_make_tree(obj, ifcond, features, extra))
>> + if features:
>> + obj['features'] = self._gen_features(features)
>> + self._trees.append(_make_tree(obj, ifcond, extra))
>>
>> def _gen_member(self,
>> member: QAPISchemaObjectTypeMember) -> TreeValue:
>
> No change when not features. Else, you change
>
> obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]
>
> to
>
> obj['features'] = [_make_tree(f.name, f.ifcond) for f in features]
>
Yep. I am consolidating the logic for (node, annotation) in so doing.
> where
>
> _make_tree(f.name, f.ifcond)
> = (f.name, {'if': f.ifcond}) if f.ifcond
> = f.name else
>
> Works, and feels less lazy. However, the commit message did not prepare
> me for this. If you split this off into its own patch, you can describe
> it properly.
>
OK.
>> @@ -243,7 +246,9 @@ def _gen_member(self,
>> }
>> if member.optional:
>> obj['default'] = None
>> - return _make_tree(obj, member.ifcond, member.features)
>> + if member.features:
>> + obj['features'] = self._gen_features(member.features)
>> + return _make_tree(obj, member.ifcond)
>>
>> def _gen_variants(self, tag_name: str,
>> variants: List[QAPISchemaVariant]) -> _DObject:
>> @@ -255,7 +260,7 @@ def _gen_variant(self, variant: QAPISchemaVariant) -> TreeValue:
>> 'case': variant.name,
>> 'type': self._use_type(variant.type)
>> }
>> - return _make_tree(obj, variant.ifcond, None)
>> + return _make_tree(obj, variant.ifcond)
>>
>> def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
>> json_type: str) -> None:
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 06/11] qapi/introspect.py: add _gen_features helper
2020-12-07 23:57 ` John Snow
@ 2020-12-15 16:55 ` Markus Armbruster
2020-12-15 18:49 ` John Snow
0 siblings, 1 reply; 48+ messages in thread
From: Markus Armbruster @ 2020-12-15 16:55 UTC (permalink / raw)
To: John Snow; +Cc: qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> On 11/16/20 3:47 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> _make_tree might receive a dict or some other type.
>>
>> Are you talking about @obj?
>>
>
> Yes.
Recommend to be explict: _make_tree()'s first argument can be ...
> It *usually* takes a dict. sometimes it doesn't.
Yes. It takes an abstract syntax tree: dict for JSON object, list for
JSON array, str for JSON string, bool for JSON true and false, NoneType
for JSON none. JSON int isn't implemented, because it doesn't occur in
SchemaInfo.
>>> Adding features
>>> information should arguably be performed by the caller at such a time
>>> when we know the type of the object and don't have to re-interrogate it.
>>
>> Fair enough. There are just two such callers anyway.
>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>> scripts/qapi/introspect.py | 19 ++++++++++++-------
>>> 1 file changed, 12 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>>> index 803288a64e7..16282f2634b 100644
>>> --- a/scripts/qapi/introspect.py
>>> +++ b/scripts/qapi/introspect.py
>>> @@ -76,16 +76,12 @@
>>>
>>>
>>> def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
>>> - features: List[QAPISchemaFeature],
>>> extra: Optional[Annotations] = None
>>> ) -> TreeValue:
>>> if extra is None:
>>> extra = {}
>>> if ifcond:
>>> extra['if'] = ifcond
>>> - if features:
>>> - assert isinstance(obj, dict)
>>> - obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]
>>> if extra:
>>> return (obj, extra)
>>> return obj
>>> @@ -221,6 +217,11 @@ def _use_type(self, typ: QAPISchemaType) -> str:
>>> return '[' + self._use_type(typ.element_type) + ']'
>>> return self._name(typ.name)
>>>
>>> + @classmethod
>>> + def _gen_features(cls,
>>> + features: List[QAPISchemaFeature]) -> List[TreeValue]:
>>> + return [_make_tree(f.name, f.ifcond) for f in features]
>>> +
>>
>> Ignorant question: when to use @classmethod, and when to use
>> @staticmethod?
>
> Matter of taste. My preference is to just always use @classmethod,
> because they can be extended or referenced by subclasses.
Non-issue here, sub-classes are vanishingly unlikely.
> @staticmethod does not take a class argument, @classmethod does. Static
> methods therefore cannot address any other classmethods, but a
> classmethod can.
>
> I just always reach for classmethod by default.
Unused cls parameters are slightly annoying, though.
I've been using @staticmethod whenever it suffices. Makes "this is a
function, i.e. it can't mess with the class or instances" immediately
obvious.
[...]
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 06/11] qapi/introspect.py: add _gen_features helper
2020-12-15 16:55 ` Markus Armbruster
@ 2020-12-15 18:49 ` John Snow
0 siblings, 0 replies; 48+ messages in thread
From: John Snow @ 2020-12-15 18:49 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Eduardo Habkost, Cleber Rosa
On 12/15/20 11:55 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> On 11/16/20 3:47 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> _make_tree might receive a dict or some other type.
>>>
>>> Are you talking about @obj?
>>>
>>
>> Yes.
>
> Recommend to be explict: _make_tree()'s first argument can be ...
>
>> It *usually* takes a dict. sometimes it doesn't.
>
> Yes. It takes an abstract syntax tree: dict for JSON object, list for
> JSON array, str for JSON string, bool for JSON true and false, NoneType
> for JSON none. JSON int isn't implemented, because it doesn't occur in
> SchemaInfo.
>
>>>> Adding features
>>>> information should arguably be performed by the caller at such a time
>>>> when we know the type of the object and don't have to re-interrogate it.
>>>
>>> Fair enough. There are just two such callers anyway.
>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>> scripts/qapi/introspect.py | 19 ++++++++++++-------
>>>> 1 file changed, 12 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>>>> index 803288a64e7..16282f2634b 100644
>>>> --- a/scripts/qapi/introspect.py
>>>> +++ b/scripts/qapi/introspect.py
>>>> @@ -76,16 +76,12 @@
>>>>
>>>>
>>>> def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
>>>> - features: List[QAPISchemaFeature],
>>>> extra: Optional[Annotations] = None
>>>> ) -> TreeValue:
>>>> if extra is None:
>>>> extra = {}
>>>> if ifcond:
>>>> extra['if'] = ifcond
>>>> - if features:
>>>> - assert isinstance(obj, dict)
>>>> - obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]
>>>> if extra:
>>>> return (obj, extra)
>>>> return obj
>>>> @@ -221,6 +217,11 @@ def _use_type(self, typ: QAPISchemaType) -> str:
>>>> return '[' + self._use_type(typ.element_type) + ']'
>>>> return self._name(typ.name)
>>>>
>>>> + @classmethod
>>>> + def _gen_features(cls,
>>>> + features: List[QAPISchemaFeature]) -> List[TreeValue]:
>>>> + return [_make_tree(f.name, f.ifcond) for f in features]
>>>> +
>>>
>>> Ignorant question: when to use @classmethod, and when to use
>>> @staticmethod?
>>
>> Matter of taste. My preference is to just always use @classmethod,
>> because they can be extended or referenced by subclasses.
>
> Non-issue here, sub-classes are vanishingly unlikely.
>
True. I did admit it was just simply my default. I can adjust it
case-by-case for circumstances when ...
>> @staticmethod does not take a class argument, @classmethod does. Static
>> methods therefore cannot address any other classmethods, but a
>> classmethod can.
>>
>> I just always reach for classmethod by default.
>
> Unused cls parameters are slightly annoying, though.
>
> I've been using @staticmethod whenever it suffices. Makes "this is a
> function, i.e. it can't mess with the class or instances" immediately
> obvious.
>
... you feel it provides additional clarity to do so.
--js
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v2 07/11] qapi/introspect.py: Unify return type of _make_tree()
2020-10-26 19:42 [PATCH v2 00/11] qapi: static typing conversion, pt2 John Snow
` (5 preceding siblings ...)
2020-10-26 19:42 ` [PATCH v2 06/11] qapi/introspect.py: add _gen_features helper John Snow
@ 2020-10-26 19:42 ` John Snow
2020-11-07 5:08 ` Cleber Rosa
2020-11-16 9:46 ` Markus Armbruster
2020-10-26 19:42 ` [PATCH v2 08/11] qapi/introspect.py: replace 'extra' dict with 'comment' argument John Snow
` (5 subsequent siblings)
12 siblings, 2 replies; 48+ messages in thread
From: John Snow @ 2020-10-26 19:42 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster; +Cc: John Snow, Eduardo Habkost, Cleber Rosa
Returning two different types conditionally can be complicated to
type. Let's always return a tuple for consistency. Prohibit the use of
annotations with dict-values in this circumstance. It can be implemented
later if and when the need for it arises.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 16282f2634b..ef469b6c06e 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -77,14 +77,12 @@
def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
extra: Optional[Annotations] = None
- ) -> TreeValue:
+ ) -> Annotated:
if extra is None:
extra = {}
if ifcond:
extra['if'] = ifcond
- if extra:
- return (obj, extra)
- return obj
+ return (obj, extra)
def _tree_to_qlit(obj: TreeValue,
@@ -98,12 +96,16 @@ def indent(level: int) -> str:
ifobj, extra = obj
ifcond = cast(Optional[Sequence[str]], extra.get('if'))
comment = extra.get('comment')
+
+ msg = "Comments and Conditionals not implemented for dict values"
+ assert not (suppress_first_indent and (ifcond or comment)), msg
+
ret = ''
if comment:
ret += indent(level) + '/* %s */\n' % comment
if ifcond:
ret += gen_if(ifcond)
- ret += _tree_to_qlit(ifobj, level)
+ ret += _tree_to_qlit(ifobj, level, suppress_first_indent)
if ifcond:
ret += '\n' + gen_endif(ifcond)
return ret
@@ -152,7 +154,7 @@ def __init__(self, prefix: str, unmask: bool):
' * QAPI/QMP schema introspection', __doc__)
self._unmask = unmask
self._schema: Optional[QAPISchema] = None
- self._trees: List[TreeValue] = []
+ self._trees: List[Annotated] = []
self._used_types: List[QAPISchemaType] = []
self._name_map: Dict[str, str] = {}
self._genc.add(mcgen('''
@@ -219,7 +221,8 @@ def _use_type(self, typ: QAPISchemaType) -> str:
@classmethod
def _gen_features(cls,
- features: List[QAPISchemaFeature]) -> List[TreeValue]:
+ features: List[QAPISchemaFeature]
+ ) -> List[Annotated]:
return [_make_tree(f.name, f.ifcond) for f in features]
def _gen_tree(self, name: str, mtype: str, obj: _DObject,
@@ -239,7 +242,7 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject,
self._trees.append(_make_tree(obj, ifcond, extra))
def _gen_member(self,
- member: QAPISchemaObjectTypeMember) -> TreeValue:
+ member: QAPISchemaObjectTypeMember) -> Annotated:
obj: _DObject = {
'name': member.name,
'type': self._use_type(member.type)
@@ -255,7 +258,7 @@ def _gen_variants(self, tag_name: str,
return {'tag': tag_name,
'variants': [self._gen_variant(v) for v in variants]}
- def _gen_variant(self, variant: QAPISchemaVariant) -> TreeValue:
+ def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated:
obj: _DObject = {
'case': variant.name,
'type': self._use_type(variant.type)
--
2.26.2
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH v2 07/11] qapi/introspect.py: Unify return type of _make_tree()
2020-10-26 19:42 ` [PATCH v2 07/11] qapi/introspect.py: Unify return type of _make_tree() John Snow
@ 2020-11-07 5:08 ` Cleber Rosa
2020-12-15 0:22 ` John Snow
2020-11-16 9:46 ` Markus Armbruster
1 sibling, 1 reply; 48+ messages in thread
From: Cleber Rosa @ 2020-11-07 5:08 UTC (permalink / raw)
To: John Snow; +Cc: qemu-devel, Eduardo Habkost, Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 408 bytes --]
On Mon, Oct 26, 2020 at 03:42:47PM -0400, John Snow wrote:
> Returning two different types conditionally can be complicated to
> type. Let's always return a tuple for consistency.
This seems like a standalone change.
> Prohibit the use of
> annotations with dict-values in this circumstance. It can be implemented
> later if and when the need for it arises.
And this seems like another change.
- Cleber.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v2 07/11] qapi/introspect.py: Unify return type of _make_tree()
2020-10-26 19:42 ` [PATCH v2 07/11] qapi/introspect.py: Unify return type of _make_tree() John Snow
2020-11-07 5:08 ` Cleber Rosa
@ 2020-11-16 9:46 ` Markus Armbruster
2020-12-08 0:06 ` John Snow
1 sibling, 1 reply; 48+ messages in thread
From: Markus Armbruster @ 2020-11-16 9:46 UTC (permalink / raw)
To: John Snow; +Cc: Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> Returning two different types conditionally can be complicated to
> type. Let's always return a tuple for consistency. Prohibit the use of
> annotations with dict-values in this circumstance. It can be implemented
> later if and when the need for it arises.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/introspect.py | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 16282f2634b..ef469b6c06e 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -77,14 +77,12 @@
>
> def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
> extra: Optional[Annotations] = None
> - ) -> TreeValue:
> + ) -> Annotated:
> if extra is None:
> extra = {}
> if ifcond:
> extra['if'] = ifcond
> - if extra:
> - return (obj, extra)
> - return obj
> + return (obj, extra)
Less efficient, but that's okay.
>
>
> def _tree_to_qlit(obj: TreeValue,
> @@ -98,12 +96,16 @@ def indent(level: int) -> str:
> ifobj, extra = obj
> ifcond = cast(Optional[Sequence[str]], extra.get('if'))
> comment = extra.get('comment')
> +
> + msg = "Comments and Conditionals not implemented for dict values"
> + assert not (suppress_first_indent and (ifcond or comment)), msg
What exactly does this assertion guard?
> +
> ret = ''
> if comment:
> ret += indent(level) + '/* %s */\n' % comment
> if ifcond:
> ret += gen_if(ifcond)
> - ret += _tree_to_qlit(ifobj, level)
> + ret += _tree_to_qlit(ifobj, level, suppress_first_indent)
Why do you need to pass on @suppress_first_indent now?
> if ifcond:
> ret += '\n' + gen_endif(ifcond)
> return ret
> @@ -152,7 +154,7 @@ def __init__(self, prefix: str, unmask: bool):
> ' * QAPI/QMP schema introspection', __doc__)
> self._unmask = unmask
> self._schema: Optional[QAPISchema] = None
> - self._trees: List[TreeValue] = []
> + self._trees: List[Annotated] = []
> self._used_types: List[QAPISchemaType] = []
> self._name_map: Dict[str, str] = {}
> self._genc.add(mcgen('''
> @@ -219,7 +221,8 @@ def _use_type(self, typ: QAPISchemaType) -> str:
>
> @classmethod
> def _gen_features(cls,
> - features: List[QAPISchemaFeature]) -> List[TreeValue]:
> + features: List[QAPISchemaFeature]
> + ) -> List[Annotated]:
> return [_make_tree(f.name, f.ifcond) for f in features]
>
> def _gen_tree(self, name: str, mtype: str, obj: _DObject,
> @@ -239,7 +242,7 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject,
> self._trees.append(_make_tree(obj, ifcond, extra))
>
> def _gen_member(self,
> - member: QAPISchemaObjectTypeMember) -> TreeValue:
> + member: QAPISchemaObjectTypeMember) -> Annotated:
> obj: _DObject = {
> 'name': member.name,
> 'type': self._use_type(member.type)
> @@ -255,7 +258,7 @@ def _gen_variants(self, tag_name: str,
> return {'tag': tag_name,
> 'variants': [self._gen_variant(v) for v in variants]}
>
> - def _gen_variant(self, variant: QAPISchemaVariant) -> TreeValue:
> + def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated:
> obj: _DObject = {
> 'case': variant.name,
> 'type': self._use_type(variant.type)
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 07/11] qapi/introspect.py: Unify return type of _make_tree()
2020-11-16 9:46 ` Markus Armbruster
@ 2020-12-08 0:06 ` John Snow
2020-12-16 6:35 ` Markus Armbruster
0 siblings, 1 reply; 48+ messages in thread
From: John Snow @ 2020-12-08 0:06 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Cleber Rosa, qemu-devel, Eduardo Habkost
On 11/16/20 4:46 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Returning two different types conditionally can be complicated to
>> type. Let's always return a tuple for consistency. Prohibit the use of
>> annotations with dict-values in this circumstance. It can be implemented
>> later if and when the need for it arises.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qapi/introspect.py | 21 ++++++++++++---------
>> 1 file changed, 12 insertions(+), 9 deletions(-)
>>
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index 16282f2634b..ef469b6c06e 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -77,14 +77,12 @@
>>
>> def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
>> extra: Optional[Annotations] = None
>> - ) -> TreeValue:
>> + ) -> Annotated:
>> if extra is None:
>> extra = {}
>> if ifcond:
>> extra['if'] = ifcond
>> - if extra:
>> - return (obj, extra)
>> - return obj
>> + return (obj, extra)
>
> Less efficient, but that's okay.
>
I have bad news about Python :)
>>
>>
>> def _tree_to_qlit(obj: TreeValue,
>> @@ -98,12 +96,16 @@ def indent(level: int) -> str:
>> ifobj, extra = obj
>> ifcond = cast(Optional[Sequence[str]], extra.get('if'))
>> comment = extra.get('comment')
>> +
>> + msg = "Comments and Conditionals not implemented for dict values"
>> + assert not (suppress_first_indent and (ifcond or comment)), msg
>
> What exactly does this assertion guard?
>
Something that Eduardo noticed in his review. It's ugly, and I explained
it poorly.
We don't support annotations on dict *values*, basically. When this
function is called with suppress_first_indent, we know that we are being
called to process a dict *value* and not a dict *key*.
What do you do with comments or conditionals on just one half of a key:
value pair?
Well, break.
>> +
>> ret = ''
>> if comment:
>> ret += indent(level) + '/* %s */\n' % comment
>> if ifcond:
>> ret += gen_if(ifcond)
>> - ret += _tree_to_qlit(ifobj, level)
>> + ret += _tree_to_qlit(ifobj, level, suppress_first_indent)
>
> Why do you need to pass on @suppress_first_indent now?
>
We either never should or we always should have. This is just in the
case that "suppress first indent" is used on an annotated node. Which,
err, for the annotations we actually support right now (comment, ifcond)
-- we will reject in this case.
But it felt precarious...
>> if ifcond:
>> ret += '\n' + gen_endif(ifcond)
>> return ret
>> @@ -152,7 +154,7 @@ def __init__(self, prefix: str, unmask: bool):
>> ' * QAPI/QMP schema introspection', __doc__)
>> self._unmask = unmask
>> self._schema: Optional[QAPISchema] = None
>> - self._trees: List[TreeValue] = []
>> + self._trees: List[Annotated] = []
>> self._used_types: List[QAPISchemaType] = []
>> self._name_map: Dict[str, str] = {}
>> self._genc.add(mcgen('''
>> @@ -219,7 +221,8 @@ def _use_type(self, typ: QAPISchemaType) -> str:
>>
>> @classmethod
>> def _gen_features(cls,
>> - features: List[QAPISchemaFeature]) -> List[TreeValue]:
>> + features: List[QAPISchemaFeature]
>> + ) -> List[Annotated]:
>> return [_make_tree(f.name, f.ifcond) for f in features]
>>
>> def _gen_tree(self, name: str, mtype: str, obj: _DObject,
>> @@ -239,7 +242,7 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject,
>> self._trees.append(_make_tree(obj, ifcond, extra))
>>
>> def _gen_member(self,
>> - member: QAPISchemaObjectTypeMember) -> TreeValue:
>> + member: QAPISchemaObjectTypeMember) -> Annotated:
>> obj: _DObject = {
>> 'name': member.name,
>> 'type': self._use_type(member.type)
>> @@ -255,7 +258,7 @@ def _gen_variants(self, tag_name: str,
>> return {'tag': tag_name,
>> 'variants': [self._gen_variant(v) for v in variants]}
>>
>> - def _gen_variant(self, variant: QAPISchemaVariant) -> TreeValue:
>> + def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated:
>> obj: _DObject = {
>> 'case': variant.name,
>> 'type': self._use_type(variant.type)
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 07/11] qapi/introspect.py: Unify return type of _make_tree()
2020-12-08 0:06 ` John Snow
@ 2020-12-16 6:35 ` Markus Armbruster
0 siblings, 0 replies; 48+ messages in thread
From: Markus Armbruster @ 2020-12-16 6:35 UTC (permalink / raw)
To: John Snow; +Cc: qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> On 11/16/20 4:46 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> Returning two different types conditionally can be complicated to
>>> type. Let's always return a tuple for consistency. Prohibit the use of
>>> annotations with dict-values in this circumstance. It can be implemented
>>> later if and when the need for it arises.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>> scripts/qapi/introspect.py | 21 ++++++++++++---------
>>> 1 file changed, 12 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>>> index 16282f2634b..ef469b6c06e 100644
>>> --- a/scripts/qapi/introspect.py
>>> +++ b/scripts/qapi/introspect.py
>>> @@ -77,14 +77,12 @@
>>>
>>> def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
>>> extra: Optional[Annotations] = None
>>> - ) -> TreeValue:
>>> + ) -> Annotated:
>>> if extra is None:
>>> extra = {}
>>> if ifcond:
>>> extra['if'] = ifcond
>>> - if extra:
>>> - return (obj, extra)
>>> - return obj
>>> + return (obj, extra)
>>
>> Less efficient, but that's okay.
>>
>
> I have bad news about Python :)
Well played, sir!
>>>
>>>
>>> def _tree_to_qlit(obj: TreeValue,
>>> @@ -98,12 +96,16 @@ def indent(level: int) -> str:
>>> ifobj, extra = obj
>>> ifcond = cast(Optional[Sequence[str]], extra.get('if'))
>>> comment = extra.get('comment')
>>> +
>>> + msg = "Comments and Conditionals not implemented for dict values"
>>> + assert not (suppress_first_indent and (ifcond or comment)), msg
>>
>> What exactly does this assertion guard?
>>
>
> Something that Eduardo noticed in his review. It's ugly, and I explained
> it poorly.
>
> We don't support annotations on dict *values*, basically. When this
> function is called with suppress_first_indent, we know that we are being
> called to process a dict *value* and not a dict *key*.
>
> What do you do with comments or conditionals on just one half of a key:
> value pair?
>
> Well, break.
Your @msg is a bit misleading then: it suggests comments and
conditionals are not implemented for dict values, but could be. Not
true for conditionals.
Minimally invasive patch correction: drop @msg.
But the actual impossibility to guard against is even simpler, I
believe:
if isinstance(obj, tuple):
assert not suppress_first_indent
But^2, I figure this can be made truly impossible: factor out the part
of _tree_to_qlit() that deals with non-tuple @obj into its own function
(the conditional at its end), then use that for the dict values.
>>> +
>>> ret = ''
>>> if comment:
>>> ret += indent(level) + '/* %s */\n' % comment
>>> if ifcond:
>>> ret += gen_if(ifcond)
>>> - ret += _tree_to_qlit(ifobj, level)
>>> + ret += _tree_to_qlit(ifobj, level, suppress_first_indent)
>>
>> Why do you need to pass on @suppress_first_indent now?
>>
>
> We either never should or we always should have. This is just in the
> case that "suppress first indent" is used on an annotated node. Which,
> err, for the annotations we actually support right now (comment, ifcond)
> -- we will reject in this case.
>
> But it felt precarious...
I suspect the factoring I suggested above will make this less
precarious, too.
>>> if ifcond:
>>> ret += '\n' + gen_endif(ifcond)
>>> return ret
>>> @@ -152,7 +154,7 @@ def __init__(self, prefix: str, unmask: bool):
>>> ' * QAPI/QMP schema introspection', __doc__)
>>> self._unmask = unmask
>>> self._schema: Optional[QAPISchema] = None
>>> - self._trees: List[TreeValue] = []
>>> + self._trees: List[Annotated] = []
>>> self._used_types: List[QAPISchemaType] = []
>>> self._name_map: Dict[str, str] = {}
>>> self._genc.add(mcgen('''
>>> @@ -219,7 +221,8 @@ def _use_type(self, typ: QAPISchemaType) -> str:
>>>
>>> @classmethod
>>> def _gen_features(cls,
>>> - features: List[QAPISchemaFeature]) -> List[TreeValue]:
>>> + features: List[QAPISchemaFeature]
>>> + ) -> List[Annotated]:
>>> return [_make_tree(f.name, f.ifcond) for f in features]
>>>
>>> def _gen_tree(self, name: str, mtype: str, obj: _DObject,
>>> @@ -239,7 +242,7 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject,
>>> self._trees.append(_make_tree(obj, ifcond, extra))
>>>
>>> def _gen_member(self,
>>> - member: QAPISchemaObjectTypeMember) -> TreeValue:
>>> + member: QAPISchemaObjectTypeMember) -> Annotated:
>>> obj: _DObject = {
>>> 'name': member.name,
>>> 'type': self._use_type(member.type)
>>> @@ -255,7 +258,7 @@ def _gen_variants(self, tag_name: str,
>>> return {'tag': tag_name,
>>> 'variants': [self._gen_variant(v) for v in variants]}
>>>
>>> - def _gen_variant(self, variant: QAPISchemaVariant) -> TreeValue:
>>> + def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated:
>>> obj: _DObject = {
>>> 'case': variant.name,
>>> 'type': self._use_type(variant.type)
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v2 08/11] qapi/introspect.py: replace 'extra' dict with 'comment' argument
2020-10-26 19:42 [PATCH v2 00/11] qapi: static typing conversion, pt2 John Snow
` (6 preceding siblings ...)
2020-10-26 19:42 ` [PATCH v2 07/11] qapi/introspect.py: Unify return type of _make_tree() John Snow
@ 2020-10-26 19:42 ` John Snow
2020-11-07 5:10 ` Cleber Rosa
2020-11-16 9:55 ` Markus Armbruster
2020-10-26 19:42 ` [PATCH v2 09/11] qapi/introspect.py: create a typed 'Annotated' data strutcure John Snow
` (4 subsequent siblings)
12 siblings, 2 replies; 48+ messages in thread
From: John Snow @ 2020-10-26 19:42 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster; +Cc: John Snow, Eduardo Habkost, Cleber Rosa
This is only used to pass in a dictionary with a comment already set, so
skip the runaround and just accept the comment.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index ef469b6c06e..a0978cb3adb 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -76,12 +76,11 @@
def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
- extra: Optional[Annotations] = None
- ) -> Annotated:
- if extra is None:
- extra = {}
- if ifcond:
- extra['if'] = ifcond
+ comment: Optional[str] = None) -> Annotated:
+ extra: Annotations = {
+ 'if': ifcond,
+ 'comment': comment,
+ }
return (obj, extra)
@@ -228,18 +227,18 @@ def _gen_features(cls,
def _gen_tree(self, name: str, mtype: str, obj: _DObject,
ifcond: List[str],
features: Optional[List[QAPISchemaFeature]]) -> None:
- extra: Optional[Annotations] = None
+ comment: Optional[str] = None
if mtype not in ('command', 'event', 'builtin', 'array'):
if not self._unmask:
# Output a comment to make it easy to map masked names
# back to the source when reading the generated output.
- extra = {'comment': '"%s" = %s' % (self._name(name), name)}
+ comment = f'"{self._name(name)}" = {name}'
name = self._name(name)
obj['name'] = name
obj['meta-type'] = mtype
if features:
obj['features'] = self._gen_features(features)
- self._trees.append(_make_tree(obj, ifcond, extra))
+ self._trees.append(_make_tree(obj, ifcond, comment))
def _gen_member(self,
member: QAPISchemaObjectTypeMember) -> Annotated:
--
2.26.2
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH v2 08/11] qapi/introspect.py: replace 'extra' dict with 'comment' argument
2020-10-26 19:42 ` [PATCH v2 08/11] qapi/introspect.py: replace 'extra' dict with 'comment' argument John Snow
@ 2020-11-07 5:10 ` Cleber Rosa
2020-11-16 9:55 ` Markus Armbruster
1 sibling, 0 replies; 48+ messages in thread
From: Cleber Rosa @ 2020-11-07 5:10 UTC (permalink / raw)
To: John Snow; +Cc: qemu-devel, Eduardo Habkost, Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 292 bytes --]
On Mon, Oct 26, 2020 at 03:42:48PM -0400, John Snow wrote:
> This is only used to pass in a dictionary with a comment already set, so
> skip the runaround and just accept the comment.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
Reviewed-by: Cleber Rosa <crosa@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v2 08/11] qapi/introspect.py: replace 'extra' dict with 'comment' argument
2020-10-26 19:42 ` [PATCH v2 08/11] qapi/introspect.py: replace 'extra' dict with 'comment' argument John Snow
2020-11-07 5:10 ` Cleber Rosa
@ 2020-11-16 9:55 ` Markus Armbruster
2020-12-08 0:12 ` John Snow
1 sibling, 1 reply; 48+ messages in thread
From: Markus Armbruster @ 2020-11-16 9:55 UTC (permalink / raw)
To: John Snow; +Cc: Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> This is only used to pass in a dictionary with a comment already set, so
> skip the runaround and just accept the comment.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/introspect.py | 17 ++++++++---------
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index ef469b6c06e..a0978cb3adb 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -76,12 +76,11 @@
>
>
> def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
> - extra: Optional[Annotations] = None
> - ) -> Annotated:
> - if extra is None:
> - extra = {}
> - if ifcond:
> - extra['if'] = ifcond
> + comment: Optional[str] = None) -> Annotated:
> + extra: Annotations = {
> + 'if': ifcond,
> + 'comment': comment,
> + }
Works because _tree_to_qlit() treats 'if': None, 'comment': None exactly
like absent 'if', 'comment'. Mentioning this in the commit message
wouldn't hurt.
We create even more dicts now. Okay.
> return (obj, extra)
>
>
> @@ -228,18 +227,18 @@ def _gen_features(cls,
> def _gen_tree(self, name: str, mtype: str, obj: _DObject,
> ifcond: List[str],
> features: Optional[List[QAPISchemaFeature]]) -> None:
> - extra: Optional[Annotations] = None
> + comment: Optional[str] = None
> if mtype not in ('command', 'event', 'builtin', 'array'):
> if not self._unmask:
> # Output a comment to make it easy to map masked names
> # back to the source when reading the generated output.
> - extra = {'comment': '"%s" = %s' % (self._name(name), name)}
> + comment = f'"{self._name(name)}" = {name}'
Drive-by modernization, fine with me. Aside: many more opportunities; a
systematic hunt is called for. Not now.
> name = self._name(name)
> obj['name'] = name
> obj['meta-type'] = mtype
> if features:
> obj['features'] = self._gen_features(features)
> - self._trees.append(_make_tree(obj, ifcond, extra))
> + self._trees.append(_make_tree(obj, ifcond, comment))
>
> def _gen_member(self,
> member: QAPISchemaObjectTypeMember) -> Annotated:
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 08/11] qapi/introspect.py: replace 'extra' dict with 'comment' argument
2020-11-16 9:55 ` Markus Armbruster
@ 2020-12-08 0:12 ` John Snow
0 siblings, 0 replies; 48+ messages in thread
From: John Snow @ 2020-12-08 0:12 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Cleber Rosa, qemu-devel, Eduardo Habkost
On 11/16/20 4:55 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> This is only used to pass in a dictionary with a comment already set, so
>> skip the runaround and just accept the comment.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qapi/introspect.py | 17 ++++++++---------
>> 1 file changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index ef469b6c06e..a0978cb3adb 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -76,12 +76,11 @@
>>
>>
>> def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
>> - extra: Optional[Annotations] = None
>> - ) -> Annotated:
>> - if extra is None:
>> - extra = {}
>> - if ifcond:
>> - extra['if'] = ifcond
>> + comment: Optional[str] = None) -> Annotated:
>> + extra: Annotations = {
>> + 'if': ifcond,
>> + 'comment': comment,
>> + }
>
> Works because _tree_to_qlit() treats 'if': None, 'comment': None exactly
> like absent 'if', 'comment'. Mentioning this in the commit message
> wouldn't hurt.
>
OK.
> We create even more dicts now. Okay.
>
It's just shuffling around where this dict is made, I don't think we
create more in total.
>> return (obj, extra)
>>
>>
>> @@ -228,18 +227,18 @@ def _gen_features(cls,
>> def _gen_tree(self, name: str, mtype: str, obj: _DObject,
>> ifcond: List[str],
>> features: Optional[List[QAPISchemaFeature]]) -> None:
>> - extra: Optional[Annotations] = None
>> + comment: Optional[str] = None
>> if mtype not in ('command', 'event', 'builtin', 'array'):
>> if not self._unmask:
>> # Output a comment to make it easy to map masked names
>> # back to the source when reading the generated output.
>> - extra = {'comment': '"%s" = %s' % (self._name(name), name)}
>> + comment = f'"{self._name(name)}" = {name}'
>
> Drive-by modernization, fine with me. Aside: many more opportunities; a
> systematic hunt is called for. Not now.
>
It happened because of line-length limits, admittedly.
>> name = self._name(name)
>> obj['name'] = name
>> obj['meta-type'] = mtype
>> if features:
>> obj['features'] = self._gen_features(features)
>> - self._trees.append(_make_tree(obj, ifcond, extra))
>> + self._trees.append(_make_tree(obj, ifcond, comment))
>>
>> def _gen_member(self,
>> member: QAPISchemaObjectTypeMember) -> Annotated:
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v2 09/11] qapi/introspect.py: create a typed 'Annotated' data strutcure
2020-10-26 19:42 [PATCH v2 00/11] qapi: static typing conversion, pt2 John Snow
` (7 preceding siblings ...)
2020-10-26 19:42 ` [PATCH v2 08/11] qapi/introspect.py: replace 'extra' dict with 'comment' argument John Snow
@ 2020-10-26 19:42 ` John Snow
2020-11-07 5:45 ` Cleber Rosa
2020-11-16 10:12 ` Markus Armbruster
2020-10-26 19:42 ` [PATCH v2 10/11] qapi/introspect.py: improve readability of _tree_to_qlit John Snow
` (3 subsequent siblings)
12 siblings, 2 replies; 48+ messages in thread
From: John Snow @ 2020-10-26 19:42 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster; +Cc: John Snow, Eduardo Habkost, Cleber Rosa
This replaces _make_tree with Annotated(). By creating it as a generic
container, we can more accurately describe the exact nature of this
particular value. i.e., each Annotated object is actually an
Annotated<T>, describing its contained value.
This adds stricter typing to Annotated nodes and extra annotated
information. It also replaces a check of "isinstance tuple" with the
much more explicit "isinstance Annotated" which is guaranteed not to
break if a tuple is accidentally introduced into the type tree. (Perhaps
as a result of a bad conversion from a list.)
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 97 +++++++++++++++++++-------------------
1 file changed, 48 insertions(+), 49 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index a0978cb3adb..a261e402d69 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -13,12 +13,13 @@
from typing import (
Any,
Dict,
+ Generic,
+ Iterable,
List,
Optional,
Sequence,
- Tuple,
+ TypeVar,
Union,
- cast,
)
from .common import (
@@ -63,50 +64,48 @@
_scalar = Union[str, bool, None]
_nonscalar = Union[Dict[str, _stub], List[_stub]]
_value = Union[_scalar, _nonscalar]
-TreeValue = Union[_value, 'Annotated']
+TreeValue = Union[_value, 'Annotated[_value]']
# This is just an alias for an object in the structure described above:
_DObject = Dict[str, object]
-# Represents the annotations themselves:
-Annotations = Dict[str, object]
-# Represents an annotated node (of some kind).
-Annotated = Tuple[_value, Annotations]
+_AnnoType = TypeVar('_AnnoType', bound=TreeValue)
-def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
- comment: Optional[str] = None) -> Annotated:
- extra: Annotations = {
- 'if': ifcond,
- 'comment': comment,
- }
- return (obj, extra)
+class Annotated(Generic[_AnnoType]):
+ """
+ Annotated generally contains a SchemaInfo-like type (as a dict),
+ But it also used to wrap comments/ifconds around scalar leaf values,
+ for the benefit of features and enums.
+ """
+ # Remove after 3.7 adds @dataclass:
+ # pylint: disable=too-few-public-methods
+ def __init__(self, value: _AnnoType, ifcond: Iterable[str],
+ comment: Optional[str] = None):
+ self.value = value
+ self.comment: Optional[str] = comment
+ self.ifcond: Sequence[str] = tuple(ifcond)
-def _tree_to_qlit(obj: TreeValue,
- level: int = 0,
+def _tree_to_qlit(obj: TreeValue, level: int = 0,
suppress_first_indent: bool = False) -> str:
def indent(level: int) -> str:
return level * 4 * ' '
- if isinstance(obj, tuple):
- ifobj, extra = obj
- ifcond = cast(Optional[Sequence[str]], extra.get('if'))
- comment = extra.get('comment')
-
+ if isinstance(obj, Annotated):
msg = "Comments and Conditionals not implemented for dict values"
- assert not (suppress_first_indent and (ifcond or comment)), msg
+ assert not (suppress_first_indent and (obj.comment or obj.ifcond)), msg
ret = ''
- if comment:
- ret += indent(level) + '/* %s */\n' % comment
- if ifcond:
- ret += gen_if(ifcond)
- ret += _tree_to_qlit(ifobj, level, suppress_first_indent)
- if ifcond:
- ret += '\n' + gen_endif(ifcond)
+ if obj.comment:
+ ret += indent(level) + '/* %s */\n' % obj.comment
+ if obj.ifcond:
+ ret += gen_if(obj.ifcond)
+ ret += _tree_to_qlit(obj.value, level, suppress_first_indent)
+ if obj.ifcond:
+ ret += '\n' + gen_endif(obj.ifcond)
return ret
ret = ''
@@ -153,7 +152,7 @@ def __init__(self, prefix: str, unmask: bool):
' * QAPI/QMP schema introspection', __doc__)
self._unmask = unmask
self._schema: Optional[QAPISchema] = None
- self._trees: List[Annotated] = []
+ self._trees: List[Annotated[_DObject]] = []
self._used_types: List[QAPISchemaType] = []
self._name_map: Dict[str, str] = {}
self._genc.add(mcgen('''
@@ -219,10 +218,9 @@ def _use_type(self, typ: QAPISchemaType) -> str:
return self._name(typ.name)
@classmethod
- def _gen_features(cls,
- features: List[QAPISchemaFeature]
- ) -> List[Annotated]:
- return [_make_tree(f.name, f.ifcond) for f in features]
+ def _gen_features(
+ cls, features: List[QAPISchemaFeature]) -> List[Annotated[str]]:
+ return [Annotated(f.name, f.ifcond) for f in features]
def _gen_tree(self, name: str, mtype: str, obj: _DObject,
ifcond: List[str],
@@ -238,10 +236,10 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject,
obj['meta-type'] = mtype
if features:
obj['features'] = self._gen_features(features)
- self._trees.append(_make_tree(obj, ifcond, comment))
+ self._trees.append(Annotated(obj, ifcond, comment))
def _gen_member(self,
- member: QAPISchemaObjectTypeMember) -> Annotated:
+ member: QAPISchemaObjectTypeMember) -> Annotated[_DObject]:
obj: _DObject = {
'name': member.name,
'type': self._use_type(member.type)
@@ -250,19 +248,19 @@ def _gen_member(self,
obj['default'] = None
if member.features:
obj['features'] = self._gen_features(member.features)
- return _make_tree(obj, member.ifcond)
+ return Annotated(obj, member.ifcond)
def _gen_variants(self, tag_name: str,
variants: List[QAPISchemaVariant]) -> _DObject:
return {'tag': tag_name,
'variants': [self._gen_variant(v) for v in variants]}
- def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated:
+ def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated[_DObject]:
obj: _DObject = {
'case': variant.name,
'type': self._use_type(variant.type)
}
- return _make_tree(obj, variant.ifcond)
+ return Annotated(obj, variant.ifcond)
def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
json_type: str) -> None:
@@ -272,10 +270,11 @@ def visit_enum_type(self, name: str, info: QAPISourceInfo,
ifcond: List[str], features: List[QAPISchemaFeature],
members: List[QAPISchemaEnumMember],
prefix: Optional[str]) -> None:
- self._gen_tree(name, 'enum',
- {'values': [_make_tree(m.name, m.ifcond, None)
- for m in members]},
- ifcond, features)
+ self._gen_tree(
+ name, 'enum',
+ {'values': [Annotated(m.name, m.ifcond) for m in members]},
+ ifcond, features
+ )
def visit_array_type(self, name: str, info: Optional[QAPISourceInfo],
ifcond: List[str],
@@ -300,12 +299,12 @@ def visit_alternate_type(self, name: str, info: QAPISourceInfo,
ifcond: List[str],
features: List[QAPISchemaFeature],
variants: QAPISchemaVariants) -> None:
- self._gen_tree(name, 'alternate',
- {'members': [
- _make_tree({'type': self._use_type(m.type)},
- m.ifcond, None)
- for m in variants.variants]},
- ifcond, features)
+ self._gen_tree(
+ name, 'alternate',
+ {'members': [Annotated({'type': self._use_type(m.type)}, m.ifcond)
+ for m in variants.variants]},
+ ifcond, features
+ )
def visit_command(self, name: str, info: QAPISourceInfo, ifcond: List[str],
features: List[QAPISchemaFeature],
--
2.26.2
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH v2 09/11] qapi/introspect.py: create a typed 'Annotated' data strutcure
2020-10-26 19:42 ` [PATCH v2 09/11] qapi/introspect.py: create a typed 'Annotated' data strutcure John Snow
@ 2020-11-07 5:45 ` Cleber Rosa
2020-11-16 10:12 ` Markus Armbruster
1 sibling, 0 replies; 48+ messages in thread
From: Cleber Rosa @ 2020-11-07 5:45 UTC (permalink / raw)
To: John Snow; +Cc: qemu-devel, Eduardo Habkost, Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 2059 bytes --]
On Mon, Oct 26, 2020 at 03:42:49PM -0400, John Snow wrote:
> This replaces _make_tree with Annotated(). By creating it as a generic
> container, we can more accurately describe the exact nature of this
> particular value. i.e., each Annotated object is actually an
> Annotated<T>, describing its contained value.
>
> This adds stricter typing to Annotated nodes and extra annotated
> information. It also replaces a check of "isinstance tuple" with the
> much more explicit "isinstance Annotated" which is guaranteed not to
> break if a tuple is accidentally introduced into the type tree. (Perhaps
> as a result of a bad conversion from a list.)
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/introspect.py | 97 +++++++++++++++++++-------------------
> 1 file changed, 48 insertions(+), 49 deletions(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index a0978cb3adb..a261e402d69 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -13,12 +13,13 @@
> from typing import (
> Any,
> Dict,
> + Generic,
> + Iterable,
> List,
> Optional,
> Sequence,
> - Tuple,
> + TypeVar,
> Union,
> - cast,
> )
>
> from .common import (
> @@ -63,50 +64,48 @@
> _scalar = Union[str, bool, None]
> _nonscalar = Union[Dict[str, _stub], List[_stub]]
> _value = Union[_scalar, _nonscalar]
> -TreeValue = Union[_value, 'Annotated']
> +TreeValue = Union[_value, 'Annotated[_value]']
>
> # This is just an alias for an object in the structure described above:
> _DObject = Dict[str, object]
>
> -# Represents the annotations themselves:
> -Annotations = Dict[str, object]
>
> -# Represents an annotated node (of some kind).
> -Annotated = Tuple[_value, Annotations]
> +_AnnoType = TypeVar('_AnnoType', bound=TreeValue)
Here it becomes much harder to keep the suggestions I made on patch
5 because of forward and backward references.
Reviewed-by: Cleber Rosa <crosa@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 09/11] qapi/introspect.py: create a typed 'Annotated' data strutcure
2020-10-26 19:42 ` [PATCH v2 09/11] qapi/introspect.py: create a typed 'Annotated' data strutcure John Snow
2020-11-07 5:45 ` Cleber Rosa
@ 2020-11-16 10:12 ` Markus Armbruster
2020-12-08 0:21 ` John Snow
1 sibling, 1 reply; 48+ messages in thread
From: Markus Armbruster @ 2020-11-16 10:12 UTC (permalink / raw)
To: John Snow; +Cc: Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> This replaces _make_tree with Annotated(). By creating it as a generic
> container, we can more accurately describe the exact nature of this
> particular value. i.e., each Annotated object is actually an
> Annotated<T>, describing its contained value.
>
> This adds stricter typing to Annotated nodes and extra annotated
> information.
Inhowfar?
> It also replaces a check of "isinstance tuple" with the
> much more explicit "isinstance Annotated" which is guaranteed not to
> break if a tuple is accidentally introduced into the type tree. (Perhaps
> as a result of a bad conversion from a list.)
Sure this is worth writing home about? Such accidents seem quite
unlikely.
For me, the commit's benefit is making the structure of the annotated
tree node more explicit (your first paragraph, I guess). It's a bit of
a pattern in developing Python code: we start with a Tuple because it's
terse and easy, then things get more complex, terse becomes too terse,
and we're replacing the Tuple with a class.
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/introspect.py | 97 +++++++++++++++++++-------------------
> 1 file changed, 48 insertions(+), 49 deletions(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index a0978cb3adb..a261e402d69 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -13,12 +13,13 @@
> from typing import (
> Any,
> Dict,
> + Generic,
> + Iterable,
> List,
> Optional,
> Sequence,
> - Tuple,
> + TypeVar,
> Union,
> - cast,
> )
>
> from .common import (
> @@ -63,50 +64,48 @@
> _scalar = Union[str, bool, None]
> _nonscalar = Union[Dict[str, _stub], List[_stub]]
> _value = Union[_scalar, _nonscalar]
> -TreeValue = Union[_value, 'Annotated']
> +TreeValue = Union[_value, 'Annotated[_value]']
>
> # This is just an alias for an object in the structure described above:
> _DObject = Dict[str, object]
>
> -# Represents the annotations themselves:
> -Annotations = Dict[str, object]
>
> -# Represents an annotated node (of some kind).
> -Annotated = Tuple[_value, Annotations]
> +_AnnoType = TypeVar('_AnnoType', bound=TreeValue)
>
>
> -def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
> - comment: Optional[str] = None) -> Annotated:
> - extra: Annotations = {
> - 'if': ifcond,
> - 'comment': comment,
> - }
> - return (obj, extra)
> +class Annotated(Generic[_AnnoType]):
> + """
> + Annotated generally contains a SchemaInfo-like type (as a dict),
> + But it also used to wrap comments/ifconds around scalar leaf values,
> + for the benefit of features and enums.
> + """
> + # Remove after 3.7 adds @dataclass:
> + # pylint: disable=too-few-public-methods
> + def __init__(self, value: _AnnoType, ifcond: Iterable[str],
> + comment: Optional[str] = None):
> + self.value = value
> + self.comment: Optional[str] = comment
> + self.ifcond: Sequence[str] = tuple(ifcond)
>
>
> -def _tree_to_qlit(obj: TreeValue,
> - level: int = 0,
> +def _tree_to_qlit(obj: TreeValue, level: int = 0,
> suppress_first_indent: bool = False) -> str:
>
> def indent(level: int) -> str:
> return level * 4 * ' '
>
> - if isinstance(obj, tuple):
> - ifobj, extra = obj
> - ifcond = cast(Optional[Sequence[str]], extra.get('if'))
> - comment = extra.get('comment')
> -
> + if isinstance(obj, Annotated):
> msg = "Comments and Conditionals not implemented for dict values"
> - assert not (suppress_first_indent and (ifcond or comment)), msg
> + assert not (suppress_first_indent and (obj.comment or obj.ifcond)), msg
>
> ret = ''
> - if comment:
> - ret += indent(level) + '/* %s */\n' % comment
> - if ifcond:
> - ret += gen_if(ifcond)
> - ret += _tree_to_qlit(ifobj, level, suppress_first_indent)
> - if ifcond:
> - ret += '\n' + gen_endif(ifcond)
> + if obj.comment:
> + ret += indent(level) + '/* %s */\n' % obj.comment
> + if obj.ifcond:
> + ret += gen_if(obj.ifcond)
> + ret += _tree_to_qlit(obj.value, level, suppress_first_indent)
> + if obj.ifcond:
> + ret += '\n' + gen_endif(obj.ifcond)
> return ret
>
> ret = ''
> @@ -153,7 +152,7 @@ def __init__(self, prefix: str, unmask: bool):
> ' * QAPI/QMP schema introspection', __doc__)
> self._unmask = unmask
> self._schema: Optional[QAPISchema] = None
> - self._trees: List[Annotated] = []
> + self._trees: List[Annotated[_DObject]] = []
> self._used_types: List[QAPISchemaType] = []
> self._name_map: Dict[str, str] = {}
> self._genc.add(mcgen('''
> @@ -219,10 +218,9 @@ def _use_type(self, typ: QAPISchemaType) -> str:
> return self._name(typ.name)
>
> @classmethod
> - def _gen_features(cls,
> - features: List[QAPISchemaFeature]
> - ) -> List[Annotated]:
> - return [_make_tree(f.name, f.ifcond) for f in features]
> + def _gen_features(
> + cls, features: List[QAPISchemaFeature]) -> List[Annotated[str]]:
Indent this way from the start for lesser churn.
> + return [Annotated(f.name, f.ifcond) for f in features]
>
> def _gen_tree(self, name: str, mtype: str, obj: _DObject,
> ifcond: List[str],
> @@ -238,10 +236,10 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject,
> obj['meta-type'] = mtype
> if features:
> obj['features'] = self._gen_features(features)
> - self._trees.append(_make_tree(obj, ifcond, comment))
> + self._trees.append(Annotated(obj, ifcond, comment))
>
> def _gen_member(self,
> - member: QAPISchemaObjectTypeMember) -> Annotated:
> + member: QAPISchemaObjectTypeMember) -> Annotated[_DObject]:
Long line. Ty hanging indent.
> obj: _DObject = {
> 'name': member.name,
> 'type': self._use_type(member.type)
> @@ -250,19 +248,19 @@ def _gen_member(self,
> obj['default'] = None
> if member.features:
> obj['features'] = self._gen_features(member.features)
> - return _make_tree(obj, member.ifcond)
> + return Annotated(obj, member.ifcond)
>
> def _gen_variants(self, tag_name: str,
> variants: List[QAPISchemaVariant]) -> _DObject:
> return {'tag': tag_name,
> 'variants': [self._gen_variant(v) for v in variants]}
>
> - def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated:
> + def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated[_DObject]:
> obj: _DObject = {
> 'case': variant.name,
> 'type': self._use_type(variant.type)
> }
> - return _make_tree(obj, variant.ifcond)
> + return Annotated(obj, variant.ifcond)
>
> def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
> json_type: str) -> None:
> @@ -272,10 +270,11 @@ def visit_enum_type(self, name: str, info: QAPISourceInfo,
> ifcond: List[str], features: List[QAPISchemaFeature],
> members: List[QAPISchemaEnumMember],
> prefix: Optional[str]) -> None:
> - self._gen_tree(name, 'enum',
> - {'values': [_make_tree(m.name, m.ifcond, None)
> - for m in members]},
> - ifcond, features)
> + self._gen_tree(
> + name, 'enum',
> + {'values': [Annotated(m.name, m.ifcond) for m in members]},
> + ifcond, features
> + )
>
> def visit_array_type(self, name: str, info: Optional[QAPISourceInfo],
> ifcond: List[str],
> @@ -300,12 +299,12 @@ def visit_alternate_type(self, name: str, info: QAPISourceInfo,
> ifcond: List[str],
> features: List[QAPISchemaFeature],
> variants: QAPISchemaVariants) -> None:
> - self._gen_tree(name, 'alternate',
> - {'members': [
> - _make_tree({'type': self._use_type(m.type)},
> - m.ifcond, None)
> - for m in variants.variants]},
> - ifcond, features)
> + self._gen_tree(
> + name, 'alternate',
> + {'members': [Annotated({'type': self._use_type(m.type)}, m.ifcond)
Long line. Try breaking the line before m.ifcond, or before Annotated.
> + for m in variants.variants]},
> + ifcond, features
> + )
>
> def visit_command(self, name: str, info: QAPISourceInfo, ifcond: List[str],
> features: List[QAPISchemaFeature],
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 09/11] qapi/introspect.py: create a typed 'Annotated' data strutcure
2020-11-16 10:12 ` Markus Armbruster
@ 2020-12-08 0:21 ` John Snow
2020-12-16 7:08 ` Markus Armbruster
0 siblings, 1 reply; 48+ messages in thread
From: John Snow @ 2020-12-08 0:21 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Cleber Rosa, qemu-devel, Eduardo Habkost
On 11/16/20 5:12 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> This replaces _make_tree with Annotated(). By creating it as a generic
>> container, we can more accurately describe the exact nature of this
>> particular value. i.e., each Annotated object is actually an
>> Annotated<T>, describing its contained value.
>>
>> This adds stricter typing to Annotated nodes and extra annotated
>> information.
>
> Inhowfar?
>
The Generic[T] trick lets us express the type of the annotated node
itself, which is more specific than Tuple[_something, ...etc...] and
this type can be preserved when we peel the annotations off.
It's not super crucial, but like you say, the big benefit is the field
names and strict types for the special-purpose structure.
>> It also replaces a check of "isinstance tuple" with the
>> much more explicit "isinstance Annotated" which is guaranteed not to
>> break if a tuple is accidentally introduced into the type tree. (Perhaps
>> as a result of a bad conversion from a list.)
>
> Sure this is worth writing home about? Such accidents seem quite
> unlikely.
>
We all have our phobias. I find "isinstance(x,
extremely_common_stdlib_type)" to be extremely fragile and likely to
frustrate.
Maybe what's unlikely is anyone editing this code ever again. You've
mentioned wanting to look into changing how the schema information is
stored in QEMU before, so a lot of this might not matter for too much
longer, who knows.
> For me, the commit's benefit is making the structure of the annotated
> tree node more explicit (your first paragraph, I guess). It's a bit of
> a pattern in developing Python code: we start with a Tuple because it's
> terse and easy, then things get more complex, terse becomes too terse,
> and we're replacing the Tuple with a class.
>
Yep.
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qapi/introspect.py | 97 +++++++++++++++++++-------------------
>> 1 file changed, 48 insertions(+), 49 deletions(-)
>>
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index a0978cb3adb..a261e402d69 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -13,12 +13,13 @@
>> from typing import (
>> Any,
>> Dict,
>> + Generic,
>> + Iterable,
>> List,
>> Optional,
>> Sequence,
>> - Tuple,
>> + TypeVar,
>> Union,
>> - cast,
>> )
>>
>> from .common import (
>> @@ -63,50 +64,48 @@
>> _scalar = Union[str, bool, None]
>> _nonscalar = Union[Dict[str, _stub], List[_stub]]
>> _value = Union[_scalar, _nonscalar]
>> -TreeValue = Union[_value, 'Annotated']
>> +TreeValue = Union[_value, 'Annotated[_value]']
>>
>> # This is just an alias for an object in the structure described above:
>> _DObject = Dict[str, object]
>>
>> -# Represents the annotations themselves:
>> -Annotations = Dict[str, object]
>>
>> -# Represents an annotated node (of some kind).
>> -Annotated = Tuple[_value, Annotations]
>> +_AnnoType = TypeVar('_AnnoType', bound=TreeValue)
>>
>>
>> -def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
>> - comment: Optional[str] = None) -> Annotated:
>> - extra: Annotations = {
>> - 'if': ifcond,
>> - 'comment': comment,
>> - }
>> - return (obj, extra)
>> +class Annotated(Generic[_AnnoType]):
>> + """
>> + Annotated generally contains a SchemaInfo-like type (as a dict),
>> + But it also used to wrap comments/ifconds around scalar leaf values,
>> + for the benefit of features and enums.
>> + """
>> + # Remove after 3.7 adds @dataclass:
>> + # pylint: disable=too-few-public-methods
>> + def __init__(self, value: _AnnoType, ifcond: Iterable[str],
>> + comment: Optional[str] = None):
>> + self.value = value
>> + self.comment: Optional[str] = comment
>> + self.ifcond: Sequence[str] = tuple(ifcond)
>>
>>
>> -def _tree_to_qlit(obj: TreeValue,
>> - level: int = 0,
>> +def _tree_to_qlit(obj: TreeValue, level: int = 0,
>> suppress_first_indent: bool = False) -> str:
>>
>> def indent(level: int) -> str:
>> return level * 4 * ' '
>>
>> - if isinstance(obj, tuple):
>> - ifobj, extra = obj
>> - ifcond = cast(Optional[Sequence[str]], extra.get('if'))
>> - comment = extra.get('comment')
>> -
>> + if isinstance(obj, Annotated):
>> msg = "Comments and Conditionals not implemented for dict values"
>> - assert not (suppress_first_indent and (ifcond or comment)), msg
>> + assert not (suppress_first_indent and (obj.comment or obj.ifcond)), msg
>>
>> ret = ''
>> - if comment:
>> - ret += indent(level) + '/* %s */\n' % comment
>> - if ifcond:
>> - ret += gen_if(ifcond)
>> - ret += _tree_to_qlit(ifobj, level, suppress_first_indent)
>> - if ifcond:
>> - ret += '\n' + gen_endif(ifcond)
>> + if obj.comment:
>> + ret += indent(level) + '/* %s */\n' % obj.comment
>> + if obj.ifcond:
>> + ret += gen_if(obj.ifcond)
>> + ret += _tree_to_qlit(obj.value, level, suppress_first_indent)
>> + if obj.ifcond:
>> + ret += '\n' + gen_endif(obj.ifcond)
>> return ret
>>
>> ret = ''
>> @@ -153,7 +152,7 @@ def __init__(self, prefix: str, unmask: bool):
>> ' * QAPI/QMP schema introspection', __doc__)
>> self._unmask = unmask
>> self._schema: Optional[QAPISchema] = None
>> - self._trees: List[Annotated] = []
>> + self._trees: List[Annotated[_DObject]] = []
>> self._used_types: List[QAPISchemaType] = []
>> self._name_map: Dict[str, str] = {}
>> self._genc.add(mcgen('''
>> @@ -219,10 +218,9 @@ def _use_type(self, typ: QAPISchemaType) -> str:
>> return self._name(typ.name)
>>
>> @classmethod
>> - def _gen_features(cls,
>> - features: List[QAPISchemaFeature]
>> - ) -> List[Annotated]:
>> - return [_make_tree(f.name, f.ifcond) for f in features]
>> + def _gen_features(
>> + cls, features: List[QAPISchemaFeature]) -> List[Annotated[str]]:
>
> Indent this way from the start for lesser churn.
>
OK
>> + return [Annotated(f.name, f.ifcond) for f in features]
>>
>> def _gen_tree(self, name: str, mtype: str, obj: _DObject,
>> ifcond: List[str],
>> @@ -238,10 +236,10 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject,
>> obj['meta-type'] = mtype
>> if features:
>> obj['features'] = self._gen_features(features)
>> - self._trees.append(_make_tree(obj, ifcond, comment))
>> + self._trees.append(Annotated(obj, ifcond, comment))
>>
>> def _gen_member(self,
>> - member: QAPISchemaObjectTypeMember) -> Annotated:
>> + member: QAPISchemaObjectTypeMember) -> Annotated[_DObject]:
>
> Long line. Ty hanging indent.
>
OK. Admittedly, I hate hanging the return argument, I think it looks
bad. Worst part of python types. :(
>> obj: _DObject = {
>> 'name': member.name,
>> 'type': self._use_type(member.type)
>> @@ -250,19 +248,19 @@ def _gen_member(self,
>> obj['default'] = None
>> if member.features:
>> obj['features'] = self._gen_features(member.features)
>> - return _make_tree(obj, member.ifcond)
>> + return Annotated(obj, member.ifcond)
>>
>> def _gen_variants(self, tag_name: str,
>> variants: List[QAPISchemaVariant]) -> _DObject:
>> return {'tag': tag_name,
>> 'variants': [self._gen_variant(v) for v in variants]}
>>
>> - def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated:
>> + def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated[_DObject]:
>> obj: _DObject = {
>> 'case': variant.name,
>> 'type': self._use_type(variant.type)
>> }
>> - return _make_tree(obj, variant.ifcond)
>> + return Annotated(obj, variant.ifcond)
>>
>> def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
>> json_type: str) -> None:
>> @@ -272,10 +270,11 @@ def visit_enum_type(self, name: str, info: QAPISourceInfo,
>> ifcond: List[str], features: List[QAPISchemaFeature],
>> members: List[QAPISchemaEnumMember],
>> prefix: Optional[str]) -> None:
>> - self._gen_tree(name, 'enum',
>> - {'values': [_make_tree(m.name, m.ifcond, None)
>> - for m in members]},
>> - ifcond, features)
>> + self._gen_tree(
>> + name, 'enum',
>> + {'values': [Annotated(m.name, m.ifcond) for m in members]},
>> + ifcond, features
>> + )
>>
>> def visit_array_type(self, name: str, info: Optional[QAPISourceInfo],
>> ifcond: List[str],
>> @@ -300,12 +299,12 @@ def visit_alternate_type(self, name: str, info: QAPISourceInfo,
>> ifcond: List[str],
>> features: List[QAPISchemaFeature],
>> variants: QAPISchemaVariants) -> None:
>> - self._gen_tree(name, 'alternate',
>> - {'members': [
>> - _make_tree({'type': self._use_type(m.type)},
>> - m.ifcond, None)
>> - for m in variants.variants]},
>> - ifcond, features)
>> + self._gen_tree(
>> + name, 'alternate',
>> + {'members': [Annotated({'type': self._use_type(m.type)}, m.ifcond)
>
> Long line. Try breaking the line before m.ifcond, or before Annotated.
>
OK.
>> + for m in variants.variants]},
>> + ifcond, features
>> + )
>>
>> def visit_command(self, name: str, info: QAPISourceInfo, ifcond: List[str],
>> features: List[QAPISchemaFeature],
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 09/11] qapi/introspect.py: create a typed 'Annotated' data strutcure
2020-12-08 0:21 ` John Snow
@ 2020-12-16 7:08 ` Markus Armbruster
2020-12-17 1:30 ` John Snow
0 siblings, 1 reply; 48+ messages in thread
From: Markus Armbruster @ 2020-12-16 7:08 UTC (permalink / raw)
To: John Snow; +Cc: qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> On 11/16/20 5:12 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> This replaces _make_tree with Annotated(). By creating it as a generic
>>> container, we can more accurately describe the exact nature of this
>>> particular value. i.e., each Annotated object is actually an
>>> Annotated<T>, describing its contained value.
>>>
>>> This adds stricter typing to Annotated nodes and extra annotated
>>> information.
>>
>> Inhowfar?
>>
>
> The Generic[T] trick lets us express the type of the annotated node
> itself, which is more specific than Tuple[_something, ...etc...] and
> this type can be preserved when we peel the annotations off.
>
> It's not super crucial, but like you say, the big benefit is the field
> names and strict types for the special-purpose structure.
I'd lead with a brief description of the data structure you're
replacing, how we got there, and why it's ugly. You can steal from my
review of PATCH 5. Then explain its replacement, briefly. And only
then talk about types.
By the time you get to types, I'm nodding along "yes, please", and will
be predisposed to accept your typing arguments at face value.
If you start with typing arguments, they have to negotiate the "yes,
please" bar all by themselves. Harder, because Python typing stuff you
have to explain for dummies.
>>> It also replaces a check of "isinstance tuple" with the
>>> much more explicit "isinstance Annotated" which is guaranteed not to
>>> break if a tuple is accidentally introduced into the type tree. (Perhaps
>>> as a result of a bad conversion from a list.)
>>
>> Sure this is worth writing home about? Such accidents seem quite
>> unlikely.
>>
>
> We all have our phobias. I find "isinstance(x,
> extremely_common_stdlib_type)" to be extremely fragile and likely to
> frustrate.
You're applying programming-in-the-large reasoning to a
programming-in-the-small case.
Say you're writing a piece of code you expect to be used in contexts you
prudently refuse to predict. The code deals with a bunch of basic
Python types. Reserving another basic Python type for internal use may
well be unwise then, because it can make your code break confusingly
when this other type appears in input. Which it shouldn't, but making
your reusable code harder to misuse, and misuses easier to diagnose are
laudable goals.
This is not such a piece of code. All the users it will ever have are
in the same file of 200-something LOC.
Your commit message makes the case for your patch. Sometimes, dropping
weak arguments strengthens a case. I believe dropping the "It also
replaces" argument would strengthen your case.
> Maybe what's unlikely is anyone editing this code ever again. You've
> mentioned wanting to look into changing how the schema information is
> stored in QEMU before, so a lot of this might not matter for too much
> longer, who knows.
Yes, I expect generating the SchemaInfoList directly would beat
generating QLitObject, then converting QLitObject -> QObject ->
SchemaInfoList. Whether it's worth the effort is unclear.
>> For me, the commit's benefit is making the structure of the annotated
>> tree node more explicit (your first paragraph, I guess). It's a bit of
>> a pattern in developing Python code: we start with a Tuple because it's
>> terse and easy, then things get more complex, terse becomes too terse,
>> and we're replacing the Tuple with a class.
>>
>
> Yep.
>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v2 09/11] qapi/introspect.py: create a typed 'Annotated' data strutcure
2020-12-16 7:08 ` Markus Armbruster
@ 2020-12-17 1:30 ` John Snow
0 siblings, 0 replies; 48+ messages in thread
From: John Snow @ 2020-12-17 1:30 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Eduardo Habkost, Cleber Rosa
On 12/16/20 2:08 AM, Markus Armbruster wrote:
>> We all have our phobias. I find "isinstance(x,
>> extremely_common_stdlib_type)" to be extremely fragile and likely to
>> frustrate.
> You're applying programming-in-the-large reasoning to a
> programming-in-the-small case.
>
"Surely, they won't use my proof of concept code in production!"
Ah, alas, ...
> Say you're writing a piece of code you expect to be used in contexts you
> prudently refuse to predict. The code deals with a bunch of basic
> Python types. Reserving another basic Python type for internal use may
> well be unwise then, because it can make your code break confusingly
> when this other type appears in input. Which it shouldn't, but making
> your reusable code harder to misuse, and misuses easier to diagnose are
> laudable goals.
>
> This is not such a piece of code. All the users it will ever have are
> in the same file of 200-something LOC.
>
I'm just saying that this type of code has bitten me in the ass before.
You're right that it's not likely to bite someone explicitly here, but
that's indeed why it came in the "Also, ..." section.
I've reworked the commit message a bit by now, but I suspect you'll
still want to take the red marker to it a bit.
--js
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v2 10/11] qapi/introspect.py: improve readability of _tree_to_qlit
2020-10-26 19:42 [PATCH v2 00/11] qapi: static typing conversion, pt2 John Snow
` (8 preceding siblings ...)
2020-10-26 19:42 ` [PATCH v2 09/11] qapi/introspect.py: create a typed 'Annotated' data strutcure John Snow
@ 2020-10-26 19:42 ` John Snow
2020-11-07 5:54 ` Cleber Rosa
2020-11-16 10:17 ` Markus Armbruster
2020-10-26 19:42 ` [PATCH v2 11/11] qapi/introspect.py: Add docstring to _tree_to_qlit John Snow
` (2 subsequent siblings)
12 siblings, 2 replies; 48+ messages in thread
From: John Snow @ 2020-10-26 19:42 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster; +Cc: John Snow, Eduardo Habkost, Cleber Rosa
Subjective, but I find getting rid of the comprehensions helps. Also,
divide the sections into scalar and non-scalar sections, and remove
old-style string formatting.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 37 +++++++++++++++++++++----------------
1 file changed, 21 insertions(+), 16 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index a261e402d69..d4f28485ba5 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -100,7 +100,7 @@ def indent(level: int) -> str:
ret = ''
if obj.comment:
- ret += indent(level) + '/* %s */\n' % obj.comment
+ ret += indent(level) + f"/* {obj.comment} */\n"
if obj.ifcond:
ret += gen_if(obj.ifcond)
ret += _tree_to_qlit(obj.value, level, suppress_first_indent)
@@ -111,31 +111,36 @@ def indent(level: int) -> str:
ret = ''
if not suppress_first_indent:
ret += indent(level)
+
+ # Scalars:
if obj is None:
ret += 'QLIT_QNULL'
elif isinstance(obj, str):
- ret += 'QLIT_QSTR(' + to_c_string(obj) + ')'
+ ret += f"QLIT_QSTR({to_c_string(obj)})"
+ elif isinstance(obj, bool):
+ ret += "QLIT_QBOOL({:s})".format(str(obj).lower())
+
+ # Non-scalars:
elif isinstance(obj, list):
- elts = [_tree_to_qlit(elt, level + 1).strip('\n')
- for elt in obj]
- elts.append(indent(level + 1) + "{}")
ret += 'QLIT_QLIST(((QLitObject[]) {\n'
- ret += '\n'.join(elts) + '\n'
+ for value in obj:
+ ret += _tree_to_qlit(value, level + 1).strip('\n') + '\n'
+ ret += indent(level + 1) + '{}\n'
ret += indent(level) + '}))'
elif isinstance(obj, dict):
- elts = []
- for key, value in sorted(obj.items()):
- elts.append(indent(level + 1) + '{ %s, %s }' %
- (to_c_string(key),
- _tree_to_qlit(value, level + 1, True)))
- elts.append(indent(level + 1) + '{}')
ret += 'QLIT_QDICT(((QLitDictEntry[]) {\n'
- ret += ',\n'.join(elts) + '\n'
+ for key, value in sorted(obj.items()):
+ ret += indent(level + 1) + "{{ {:s}, {:s} }},\n".format(
+ to_c_string(key),
+ _tree_to_qlit(value, level + 1, suppress_first_indent=True)
+ )
+ ret += indent(level + 1) + '{}\n'
ret += indent(level) + '}))'
- elif isinstance(obj, bool):
- ret += 'QLIT_QBOOL(%s)' % ('true' if obj else 'false')
else:
- assert False # not implemented
+ raise NotImplementedError(
+ f"type '{type(obj).__name__}' not implemented"
+ )
+
if level > 0:
ret += ','
return ret
--
2.26.2
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH v2 10/11] qapi/introspect.py: improve readability of _tree_to_qlit
2020-10-26 19:42 ` [PATCH v2 10/11] qapi/introspect.py: improve readability of _tree_to_qlit John Snow
@ 2020-11-07 5:54 ` Cleber Rosa
2020-11-16 10:17 ` Markus Armbruster
1 sibling, 0 replies; 48+ messages in thread
From: Cleber Rosa @ 2020-11-07 5:54 UTC (permalink / raw)
To: John Snow; +Cc: qemu-devel, Eduardo Habkost, Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 3195 bytes --]
On Mon, Oct 26, 2020 at 03:42:50PM -0400, John Snow wrote:
> Subjective, but I find getting rid of the comprehensions helps. Also,
> divide the sections into scalar and non-scalar sections, and remove
> old-style string formatting.
>
It's certainly a matter of picking your favorite poison... but for the
most part I think this is an improvement...
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/introspect.py | 37 +++++++++++++++++++++----------------
> 1 file changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index a261e402d69..d4f28485ba5 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -100,7 +100,7 @@ def indent(level: int) -> str:
>
> ret = ''
> if obj.comment:
> - ret += indent(level) + '/* %s */\n' % obj.comment
> + ret += indent(level) + f"/* {obj.comment} */\n"
> if obj.ifcond:
> ret += gen_if(obj.ifcond)
> ret += _tree_to_qlit(obj.value, level, suppress_first_indent)
> @@ -111,31 +111,36 @@ def indent(level: int) -> str:
> ret = ''
> if not suppress_first_indent:
> ret += indent(level)
> +
> + # Scalars:
> if obj is None:
> ret += 'QLIT_QNULL'
> elif isinstance(obj, str):
> - ret += 'QLIT_QSTR(' + to_c_string(obj) + ')'
> + ret += f"QLIT_QSTR({to_c_string(obj)})"
> + elif isinstance(obj, bool):
> + ret += "QLIT_QBOOL({:s})".format(str(obj).lower())
> +
> + # Non-scalars:
> elif isinstance(obj, list):
> - elts = [_tree_to_qlit(elt, level + 1).strip('\n')
> - for elt in obj]
> - elts.append(indent(level + 1) + "{}")
> ret += 'QLIT_QLIST(((QLitObject[]) {\n'
> - ret += '\n'.join(elts) + '\n'
> + for value in obj:
> + ret += _tree_to_qlit(value, level + 1).strip('\n') + '\n'
> + ret += indent(level + 1) + '{}\n'
> ret += indent(level) + '}))'
> elif isinstance(obj, dict):
> - elts = []
> - for key, value in sorted(obj.items()):
> - elts.append(indent(level + 1) + '{ %s, %s }' %
> - (to_c_string(key),
> - _tree_to_qlit(value, level + 1, True)))
> - elts.append(indent(level + 1) + '{}')
> ret += 'QLIT_QDICT(((QLitDictEntry[]) {\n'
> - ret += ',\n'.join(elts) + '\n'
> + for key, value in sorted(obj.items()):
> + ret += indent(level + 1) + "{{ {:s}, {:s} }},\n".format(
> + to_c_string(key),
> + _tree_to_qlit(value, level + 1, suppress_first_indent=True)
> + )
> + ret += indent(level + 1) + '{}\n'
> ret += indent(level) + '}))'
> - elif isinstance(obj, bool):
> - ret += 'QLIT_QBOOL(%s)' % ('true' if obj else 'false')
> else:
> - assert False # not implemented
> + raise NotImplementedError(
This is an improvement, but doesn't fall into the *readability* bucket.
IMO it's worth splitting it out.
- Cleber.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 10/11] qapi/introspect.py: improve readability of _tree_to_qlit
2020-10-26 19:42 ` [PATCH v2 10/11] qapi/introspect.py: improve readability of _tree_to_qlit John Snow
2020-11-07 5:54 ` Cleber Rosa
@ 2020-11-16 10:17 ` Markus Armbruster
2020-12-15 15:25 ` John Snow
1 sibling, 1 reply; 48+ messages in thread
From: Markus Armbruster @ 2020-11-16 10:17 UTC (permalink / raw)
To: John Snow; +Cc: Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> Subjective, but I find getting rid of the comprehensions helps. Also,
> divide the sections into scalar and non-scalar sections, and remove
> old-style string formatting.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/introspect.py | 37 +++++++++++++++++++++----------------
> 1 file changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index a261e402d69..d4f28485ba5 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -100,7 +100,7 @@ def indent(level: int) -> str:
>
> ret = ''
> if obj.comment:
> - ret += indent(level) + '/* %s */\n' % obj.comment
> + ret += indent(level) + f"/* {obj.comment} */\n"
> if obj.ifcond:
> ret += gen_if(obj.ifcond)
> ret += _tree_to_qlit(obj.value, level, suppress_first_indent)
> @@ -111,31 +111,36 @@ def indent(level: int) -> str:
> ret = ''
> if not suppress_first_indent:
> ret += indent(level)
> +
> + # Scalars:
> if obj is None:
> ret += 'QLIT_QNULL'
> elif isinstance(obj, str):
> - ret += 'QLIT_QSTR(' + to_c_string(obj) + ')'
> + ret += f"QLIT_QSTR({to_c_string(obj)})"
> + elif isinstance(obj, bool):
> + ret += "QLIT_QBOOL({:s})".format(str(obj).lower())
Changed from
ret += 'QLIT_QBOOL(%s)' % ('true' if obj else 'false')
Doesn't look like an improvement to me.
> +
> + # Non-scalars:
> elif isinstance(obj, list):
> - elts = [_tree_to_qlit(elt, level + 1).strip('\n')
> - for elt in obj]
> - elts.append(indent(level + 1) + "{}")
> ret += 'QLIT_QLIST(((QLitObject[]) {\n'
> - ret += '\n'.join(elts) + '\n'
> + for value in obj:
> + ret += _tree_to_qlit(value, level + 1).strip('\n') + '\n'
> + ret += indent(level + 1) + '{}\n'
> ret += indent(level) + '}))'
> elif isinstance(obj, dict):
> - elts = []
> - for key, value in sorted(obj.items()):
> - elts.append(indent(level + 1) + '{ %s, %s }' %
> - (to_c_string(key),
> - _tree_to_qlit(value, level + 1, True)))
> - elts.append(indent(level + 1) + '{}')
> ret += 'QLIT_QDICT(((QLitDictEntry[]) {\n'
> - ret += ',\n'.join(elts) + '\n'
> + for key, value in sorted(obj.items()):
> + ret += indent(level + 1) + "{{ {:s}, {:s} }},\n".format(
> + to_c_string(key),
> + _tree_to_qlit(value, level + 1, suppress_first_indent=True)
> + )
> + ret += indent(level + 1) + '{}\n'
> ret += indent(level) + '}))'
> - elif isinstance(obj, bool):
> - ret += 'QLIT_QBOOL(%s)' % ('true' if obj else 'false')
> else:
> - assert False # not implemented
> + raise NotImplementedError(
> + f"type '{type(obj).__name__}' not implemented"
> + )
Not covered by the commit message's mandate.
Why bother?
> +
> if level > 0:
> ret += ','
> return ret
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 10/11] qapi/introspect.py: improve readability of _tree_to_qlit
2020-11-16 10:17 ` Markus Armbruster
@ 2020-12-15 15:25 ` John Snow
0 siblings, 0 replies; 48+ messages in thread
From: John Snow @ 2020-12-15 15:25 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Cleber Rosa, qemu-devel, Eduardo Habkost
On 11/16/20 5:17 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Subjective, but I find getting rid of the comprehensions helps. Also,
>> divide the sections into scalar and non-scalar sections, and remove
>> old-style string formatting.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qapi/introspect.py | 37 +++++++++++++++++++++----------------
>> 1 file changed, 21 insertions(+), 16 deletions(-)
>>
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index a261e402d69..d4f28485ba5 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -100,7 +100,7 @@ def indent(level: int) -> str:
>>
>> ret = ''
>> if obj.comment:
>> - ret += indent(level) + '/* %s */\n' % obj.comment
>> + ret += indent(level) + f"/* {obj.comment} */\n"
>> if obj.ifcond:
>> ret += gen_if(obj.ifcond)
>> ret += _tree_to_qlit(obj.value, level, suppress_first_indent)
>> @@ -111,31 +111,36 @@ def indent(level: int) -> str:
>> ret = ''
>> if not suppress_first_indent:
>> ret += indent(level)
>> +
>> + # Scalars:
>> if obj is None:
>> ret += 'QLIT_QNULL'
>> elif isinstance(obj, str):
>> - ret += 'QLIT_QSTR(' + to_c_string(obj) + ')'
>> + ret += f"QLIT_QSTR({to_c_string(obj)})"
>> + elif isinstance(obj, bool):
>> + ret += "QLIT_QBOOL({:s})".format(str(obj).lower())
>
> Changed from
>
> ret += 'QLIT_QBOOL(%s)' % ('true' if obj else 'false')
>
> Doesn't look like an improvement to me.
>
Momentary lapse of judgment and/or just got lost in a sea of 150
patches, please pick your preferred excuse.
(I've made this one use an f-string, too.)
>> +
>> + # Non-scalars:
>> elif isinstance(obj, list):
>> - elts = [_tree_to_qlit(elt, level + 1).strip('\n')
>> - for elt in obj]
>> - elts.append(indent(level + 1) + "{}")
>> ret += 'QLIT_QLIST(((QLitObject[]) {\n'
>> - ret += '\n'.join(elts) + '\n'
>> + for value in obj:
>> + ret += _tree_to_qlit(value, level + 1).strip('\n') + '\n'
>> + ret += indent(level + 1) + '{}\n'
>> ret += indent(level) + '}))'
>> elif isinstance(obj, dict):
>> - elts = []
>> - for key, value in sorted(obj.items()):
>> - elts.append(indent(level + 1) + '{ %s, %s }' %
>> - (to_c_string(key),
>> - _tree_to_qlit(value, level + 1, True)))
>> - elts.append(indent(level + 1) + '{}')
>> ret += 'QLIT_QDICT(((QLitDictEntry[]) {\n'
>> - ret += ',\n'.join(elts) + '\n'
>> + for key, value in sorted(obj.items()):
>> + ret += indent(level + 1) + "{{ {:s}, {:s} }},\n".format(
>> + to_c_string(key),
>> + _tree_to_qlit(value, level + 1, suppress_first_indent=True)
>> + )
>> + ret += indent(level + 1) + '{}\n'
>> ret += indent(level) + '}))'
>> - elif isinstance(obj, bool):
>> - ret += 'QLIT_QBOOL(%s)' % ('true' if obj else 'false')
>> else:
>> - assert False # not implemented
>> + raise NotImplementedError(
>> + f"type '{type(obj).__name__}' not implemented"
>> + )
>
> Not covered by the commit message's mandate.
>
> Why bother?
>
Somewhat as a debugging mechanism while I was toying with various
refactors. I like my error messages to be informative, I guess.
>> +
>> if level > 0:
>> ret += ','
>> return ret
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v2 11/11] qapi/introspect.py: Add docstring to _tree_to_qlit
2020-10-26 19:42 [PATCH v2 00/11] qapi: static typing conversion, pt2 John Snow
` (9 preceding siblings ...)
2020-10-26 19:42 ` [PATCH v2 10/11] qapi/introspect.py: improve readability of _tree_to_qlit John Snow
@ 2020-10-26 19:42 ` John Snow
2020-11-07 5:57 ` Cleber Rosa
2020-11-02 15:40 ` [PATCH v2 00/11] qapi: static typing conversion, pt2 John Snow
2020-11-16 13:17 ` introspect.py output representation (was: [PATCH v2 00/11] qapi: static typing conversion, pt2) Markus Armbruster
12 siblings, 1 reply; 48+ messages in thread
From: John Snow @ 2020-10-26 19:42 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster; +Cc: John Snow, Eduardo Habkost, Cleber Rosa
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/introspect.py | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index d4f28485ba5..8db9650adbc 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -1,10 +1,11 @@
"""
QAPI introspection generator
-Copyright (C) 2015-2018 Red Hat, Inc.
+Copyright (C) 2015-2020 Red Hat, Inc.
Authors:
Markus Armbruster <armbru@redhat.com>
+ John Snow <jsnow@redhat.com>
This work is licensed under the terms of the GNU GPL, version 2.
See the COPYING file in the top-level directory.
@@ -90,6 +91,13 @@ def __init__(self, value: _AnnoType, ifcond: Iterable[str],
def _tree_to_qlit(obj: TreeValue, level: int = 0,
suppress_first_indent: bool = False) -> str:
+ """
+ Convert the type tree into a QLIT C string, recursively.
+
+ :param obj: The value to convert.
+ :param level: The indentation level for this particular value.
+ :param suppress_first_indent: True for dict value children.
+ """
def indent(level: int) -> str:
return level * 4 * ' '
--
2.26.2
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH v2 00/11] qapi: static typing conversion, pt2
2020-10-26 19:42 [PATCH v2 00/11] qapi: static typing conversion, pt2 John Snow
` (10 preceding siblings ...)
2020-10-26 19:42 ` [PATCH v2 11/11] qapi/introspect.py: Add docstring to _tree_to_qlit John Snow
@ 2020-11-02 15:40 ` John Snow
2020-11-04 9:51 ` Marc-André Lureau
2020-11-16 13:17 ` introspect.py output representation (was: [PATCH v2 00/11] qapi: static typing conversion, pt2) Markus Armbruster
12 siblings, 1 reply; 48+ messages in thread
From: John Snow @ 2020-11-02 15:40 UTC (permalink / raw)
To: qemu-devel, Markus Armbruster; +Cc: Eduardo Habkost, Cleber Rosa
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
>
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 00/11] qapi: static typing conversion, pt2
2020-11-02 15:40 ` [PATCH v2 00/11] qapi: static typing conversion, pt2 John Snow
@ 2020-11-04 9:51 ` Marc-André Lureau
2020-12-15 15:52 ` John Snow
0 siblings, 1 reply; 48+ messages in thread
From: Marc-André Lureau @ 2020-11-04 9:51 UTC (permalink / raw)
To: John Snow; +Cc: Cleber Rosa, QEMU, Eduardo Habkost, Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 5803 bytes --]
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
> >
>
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 7715 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v2 00/11] qapi: static typing conversion, pt2
2020-11-04 9:51 ` Marc-André Lureau
@ 2020-12-15 15:52 ` John Snow
0 siblings, 0 replies; 48+ messages in thread
From: John Snow @ 2020-12-15 15:52 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Cleber Rosa, QEMU, Eduardo Habkost, Markus Armbruster
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
^ permalink raw reply [flat|nested] 48+ messages in thread
* introspect.py output representation (was: [PATCH v2 00/11] qapi: static typing conversion, pt2)
2020-10-26 19:42 [PATCH v2 00/11] qapi: static typing conversion, pt2 John Snow
` (11 preceding siblings ...)
2020-11-02 15:40 ` [PATCH v2 00/11] qapi: static typing conversion, pt2 John Snow
@ 2020-11-16 13:17 ` Markus Armbruster
12 siblings, 0 replies; 48+ messages in thread
From: Markus Armbruster @ 2020-11-16 13:17 UTC (permalink / raw)
To: John Snow; +Cc: Cleber Rosa, qemu-devel, Eduardo Habkost
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\""
"}"
^ permalink raw reply [flat|nested] 48+ messages in thread