Re: [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations

[Cleber Rosa]

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.

signature.asc
Description: PGP signature