Re: [PATCH v4 41/46] qapi/introspect.py: create a typed 'Node' data structure

[John Snow <jsnow@redhat.com>]

On 9/30/20 3:52 PM, Eduardo Habkost wrote:
> On Wed, Sep 30, 2020 at 02:58:04PM -0400, John Snow wrote:
>> On 9/30/20 2:39 PM, Eduardo Habkost wrote:
>>> On Wed, Sep 30, 2020 at 12:31:45AM -0400, John Snow wrote:
>>>> This replaces _make_tree with Node.__init__(), effectively. By creating
>>>> it as a generic container, we can more accurately describe the exact
>>>> nature of this particular Node.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>    scripts/qapi/introspect.py | 77 +++++++++++++++++++-------------------
>>>>    1 file changed, 38 insertions(+), 39 deletions(-)
>>>>
>>>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>>>> index 43b6ba5df1f..86286e755ca 100644
>>>> --- a/scripts/qapi/introspect.py
>>>> +++ b/scripts/qapi/introspect.py
>>>> @@ -12,11 +12,12 @@
>>>>    from typing import (
>>>>        Dict,
>>>> +    Generic,
>>>> +    Iterable,
>>>>        List,
>>>>        Optional,
>>>>        Sequence,
>>>> -    Tuple,
>>>> -    Union,
>>>> +    TypeVar,
>>>>    )
>>>>    from .common import (
>>>> @@ -43,42 +44,42 @@
>>>>    # The correct type for TreeNode is actually:
>>>> -# Union[AnnotatedNode, List[TreeNode], Dict[str, TreeNode], str, bool]
>>>> +# Union[Node[TreeNode], List[TreeNode], Dict[str, TreeNode], str, bool]
>>>>    # but mypy does not support recursive types yet.
>>>>    TreeNode = object
>>>> +_NodeType = TypeVar('_NodeType', bound=TreeNode)
>>>>    _DObject = Dict[str, object]
>>>> -Extra = Dict[str, object]
>>>> -AnnotatedNode = Tuple[TreeNode, Extra]
>>>
>>> Do you have plans to make Node replace TreeNode completely?
>>>
>>> I'd understand this patch as a means to reach that goal, but I'm
>>> not sure the intermediate state of having both Node and TreeNode
>>> types (that can be confused with each other) is desirable.
>>>
>>
>> The problem is that _tree_to_qlit still accepts a broad array of types. The
>> "TreeNode" comment above explains that those types are:
>>
>> Node[TreeNode], List[TreeNode], Dict[str, TreeNode], str, bool
>>
>> Three are containers, two are leaf values.
>> of the containers, the Node container is special in that it houses
>> explicitly one of the four other types (but never itself.)
>>
>> Even if I somehow always enforced Node[T] heading into _tree_to_qlit, I
>> would still need to describe what 'T' is, which is another recursive type
>> that I cannot exactly describe with mypy's current descriptive power:
>>
>> INNER_TYPE = List[Node[INNER_TYPE]], Dict[str, Node[INNER_TYPE]], str, bool
>>
>> And that's not really a huge win, or indeed any different to the existing
>> TreeNode being an "object".
>>
>> So ... no, I felt like I was going to stop here, where we have
>> fundamentally:
>>
>> 1. Undecorated nodes (list, dict, str, bool) ("TreeNode")
>> 2. Decorated nodes (Node[T])                 ("Node")
>>
>> which leads to the question: Why bother swapping Tuple for Node at that
>> point?
>>
>> My answer is simply that having a strong type name allows us to distinguish
>> this from garden-variety Tuples that might sneak in for other reasons in
>> other data types.
> 
> Would:
>    AnnotatedNode = NewType('AnnotatedNode', Tuple[TreeNode, Extra])
> be enough, then?
> 

I don't think so, because the runtime check still checks for tuple. I 
like the consistency and simplicity of a named type.

>>
>> Maybe we want a different nomenclature though, like Node vs AnnotatedNode?
> 
> Yes, I believe having a more explicit name would be better.
> 

I give up on introspect.py; I'm dropping it from my series. I cannot 
possibly justify another single second spent here.

--js