Re: [PATCH v4 08/14] qapi/introspect.py: create a typed 'Annotated' data strutcure

[John Snow <jsnow@redhat.com>]

On 2/4/21 10:37 AM, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
>> On Wed, Feb 03, 2021 at 03:47:36PM +0100, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> Presently, we use a tuple to attach a dict containing annotations
>>>> (comments and compile-time conditionals) to a tree node. This is
>>>> undesirable because dicts are difficult to strongly type; promoting it
>>>> to a real class allows us to name the values and types of the
>>>> annotations we are expecting.
>>>>
>>>> In terms of typing, the Annotated<T> type serves as a generic container
>>>> where the annotated node's type is preserved, allowing for greater
>>>> specificity than we'd be able to provide without a generic.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>> [...]
>>>> +class Annotated(Generic[_NodeT]):
>>>> +    """
>>>> +    Annotated generally contains a SchemaInfo-like type (as a dict),
>>>> +    But it also used to wrap comments/ifconds around scalar leaf values,
>>>> +    for the benefit of features and enums.
>>>> +    """
>>>> +    # Remove after 3.7 adds @dataclass:
>>>
>>> Make this
>>>
>>>         # TODO Remove after Python 3.7 ...
>>>
>>> to give us a fighting chance to remember.
>>>
>>>> +    # pylint: disable=too-few-public-methods
>>>> +    def __init__(self, value: _NodeT, ifcond: Iterable[str],
>>>> +                 comment: Optional[str] = None):
>>>
>>> Why not simply value: _value?
>>
>> Example:
>>    x = C(1)
>>    y: C[int]
>>    y = C('x')  # mistake
>>
>> Declaring value as _NodeT does:
>> - Make the inferred type of x be Annotated[int].
>> - Catch the mistake above.
> 
> I smell overengineering.  I may well be wrong.
> 
> Without doubt, there are uses for using the type system for keeping
> SomeGenericType[SomeType] and SomeGenericType[AnotherType] apart.
> 
> But what do we gain by keeping the Annotated[T] for the possible T
> apart?
> 
> _tree_to_qlit() doesn't care: it peels off the wrapper holding ifcond
> and comment, and recurses for the JSON so wrapped.  Regardless of what
> was wrapped, i.e. what kind of T we got.
> 
> Heck, it works just fine even if you wrap your JSON multiple times.  It
> doesn't give a hoot whether that makes sense.  Making sense is the
> caller's business.
> 
> So what does care?
> 
> Or am I simply confused?
> 
> 
> PS: As far as I can tell, _tree_to_qlit() doesn't give a hoot whether a
> dictionary's values are wrapped, either.
> 

You're right that it offers no necessary power to the automated 
checking, I cede as much in my other replies to you.

(1) I hope the explanation was helpful, because the technique will 
return for structures like QAPISchemaMember where it does become more 
obviously useful to disambiguate the wrapped types.

(2) It still has a notational benefit to the human for documentation and 
IDE purposes, e.g. at the end of this series:

```
self._trees: List[Annotated[SchemaInfo]] = []
```

That's nice! It tells us exactly what's in there.

Does it work if I dumb it down to just:

```
self.trees: List[AnnotatedThingy] = []
```

Yes, but it's worse to the human and the IDE both. I consider type hints 
as serving a dual purpose; both provability and declaration of intent.

--js