Re: [PATCH v4 07/14] qapi/introspect.py: Introduce preliminary tree typing

[John Snow <jsnow@redhat.com>]

On 2/3/21 9:30 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> The types will be used in forthcoming patches to add typing. These types
>> describe the layout and structure of the objects passed to
>> _tree_to_qlit, but lack the power to describe annotations until the next
>> commit.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qapi/introspect.py | 30 +++++++++++++++++++++++++++++-
>>   1 file changed, 29 insertions(+), 1 deletion(-)
>>
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index 0aa3b77109f..b82efe16f6e 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -10,7 +10,13 @@
>>   See the COPYING file in the top-level directory.
>>   """
>>   
>> -from typing import Optional
>> +from typing import (
>> +    Any,
>> +    Dict,
>> +    List,
>> +    Optional,
>> +    Union,
>> +)
>>   
>>   from .common import (
>>       c_name,
>> @@ -26,6 +32,28 @@
>>   )
>>   
>>   
>> +# This module constructs a tree 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, TreeValue], List[TreeValue]]
>> +#
>> +# 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[_value]']
> 
> Why is TreeValue commented out?  Oh, because Annotated doesn't exist,
> yet.
> 

In the comment region specifically, the intent was to give a standalone 
grammar of the structure without regard to implementation limitations. 
It is the "real" type of the tree.

i.e., the grammar is complete and accurate, but abstract.

 From the commit message: "These types describe the layout and structure 
of the objects passed to _tree_to_qlit, but lack the power to describe 
annotations until the next commit."

The second occurrence of that type, commented out, could be removed -- 
see below.

> Possibly less confusing:
> 
>     # 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, TreeValue], List[TreeValue]]
>     #
>     # With optional annotations, the type of all values is:
>     # TODO

I'd actually prefer to keep that one in;

>     #
>     # 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]

and augment it here instead, with:

# _TreeValue = TODO - defined in a forthcoming commit.

--js

> 
> or even just
> 
>     # 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, TreeValue], List[TreeValue]]
>     #
>     # 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]
> 
>> +
>> +
>>   def _make_tree(obj, ifcond, comment=None):
>>       extra = {
>>           'if': ifcond,