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

[John Snow <jsnow@redhat.com>]

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