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

[John Snow]

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