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

[Markus Armbruster <armbru@redhat.com>]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Thu, Feb 04, 2021 at 04:37:45PM +0100, 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.
>
> To me it's just regular and idiomatic use of Generic.

Bear in mind that I'm (ab)using these reviews to learn Python's way of
static typing.  My ignorant questions may evolve into mere grumblings as
I learn.  Or into requests for change.

Grumbling: the notational overhead is regrettable.

>> 
>> 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?
>
> I understand this as (valid) criticism of the use of Generic.
> If we don't want to make Generic[T1] and Generic[T2] be
> different types, there's no point in using Generic at all.

True.

>> _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?
>
> Those are valid questions.  Maybe using Generic will be useful
> in the future, but maybe we don't need it right now.
>
> Personally, I don't care either way.  I just wish this small
> choice don't became another obstacle for doing useful work.

I agree this one's minor.

The general problem is not.

Some invariants can be elegantly expressed as static types.  Easier to
read than assertions and comments, and statically checkable.  Lovely, me
want.

There are also cases we can express, but the notational overhead
compromises readability.  We effectively trade readability for static
checking.  I believe this is a bad trade for the QAPI generator.

"Compromises readability" is highly subjective.  Lots of gray area.

My point is: please don't aim for maximally tight types.  Try to
optimize comprehension instead.  Be pragmatic.

There are plenty of languages "where you have to sit with a teacup of
types balanced on your knee and make polite conversation with a strict
old aunt of a compiler."[*]  Let's not press Python into that role :)

>> PS: As far as I can tell, _tree_to_qlit() doesn't give a hoot whether a
>> dictionary's values are wrapped, either.


[*] Paul Graham