Re: [PATCH v5 24/36] qapi/source.py: add type hint annotations

From: Markus Armbruster <armbru@redhat.com>
To: John Snow <jsnow@redhat.com>
Cc: Cleber Rosa <crosa@redhat.com>,
	qemu-devel@nongnu.org, Eduardo Habkost <ehabkost@redhat.com>
Subject: Re: [PATCH v5 24/36] qapi/source.py: add type hint annotations
Date: Thu, 08 Oct 2020 10:42:13 +0200	[thread overview]
Message-ID: <87d01t2joa.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <b4f6d2bb-37bb-5510-1fb5-479a3ca55b29@redhat.com> (John Snow's message of "Wed, 7 Oct 2020 12:04:26 -0400")

John Snow <jsnow@redhat.com> writes:

> On 10/7/20 7:55 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> Annotations do not change runtime behavior.
>>> This commit *only* adds annotations.
>>>
>>> A note on typing of __init__: mypy requires init functions with no
>>> parameters to document a return type of None to be considered fully
>>> typed. In the case when there are input parameters, None may be omitted.
>>>
>>> Since __init__ may never return any value, it is preferred to omit the
>>> return annotation whenever possible.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>>> Reviewed-by: Cleber Rosa <crosa@redhat.com>
>>> Tested-by: Cleber Rosa <crosa@redhat.com>
>>> ---
>>>   scripts/qapi/mypy.ini  |  5 -----
>>>   scripts/qapi/source.py | 31 ++++++++++++++++++-------------
>>>   2 files changed, 18 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
>>> index 8ab9ac52cc4..1b8555dfa39 100644
>>> --- a/scripts/qapi/mypy.ini
>>> +++ b/scripts/qapi/mypy.ini
>>> @@ -34,11 +34,6 @@ disallow_untyped_defs = False
>>>   disallow_incomplete_defs = False
>>>   check_untyped_defs = False
>>>   -[mypy-qapi.source]
>>> -disallow_untyped_defs = False
>>> -disallow_incomplete_defs = False
>>> -check_untyped_defs = False
>>> -
>>>   [mypy-qapi.types]
>>>   disallow_untyped_defs = False
>>>   disallow_incomplete_defs = False
>>> diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
>>> index e97b9a8e15e..1cc6a5b82dc 100644
>>> --- a/scripts/qapi/source.py
>>> +++ b/scripts/qapi/source.py
>>> @@ -11,37 +11,42 @@
>>>     import copy
>>>   import sys
>>> +from typing import List, Optional, TypeVar
>>>     
>>>   class QAPISchemaPragma:
>>> -    def __init__(self):
>>> +    def __init__(self) -> None:
>>>           # Are documentation comments required?
>>>           self.doc_required = False
>>>           # Whitelist of commands allowed to return a non-dictionary
>>> -        self.returns_whitelist = []
>>> +        self.returns_whitelist: List[str] = []
>>>           # Whitelist of entities allowed to violate case conventions
>>> -        self.name_case_whitelist = []
>>> +        self.name_case_whitelist: List[str] = []
>>>     
>>>   class QAPISourceInfo:
>>> -    def __init__(self, fname, line, parent):
>>> +    T = TypeVar('T', bound='QAPISourceInfo')
>>> +
>>> +    def __init__(self: T, fname: str, line: int, parent: Optional[T]):
>>
>> More ignorant questions (I'm abusing the review process to learn Python
>> type hints)...
>> 
>> Why do you need to annotate self here, but not elsewhere?
>
> This is admittedly me being a little extra, but I thought it was a
> good way to show a pattern for people who maybe hadn't been exposed to
> it yet.
>
> This is a pattern that allows for subclassing. I am stating that this
> __init__ method takes a parent of the same type as itself, whatever
> that happens to actually be.
>
> T is a TypeVar that we can use for Generics. By declaring the type of
> self as that TypeVar, we bind T to self's type. When we annotate the 
> parent as a T, we are enforcing that the parent we receive is of the
> same type as ourselves.
>
> (Yep, we don't actually subclass QAPISourceInfo and I don't have plans
> to. It felt slightly icky to hard-code the class type name, though. I 
> try to avoid that whenever I can. I'm not sure I would go so far as to
> call it a code smell or an antipattern, but it's something I tend to 
> avoid anyway.)

Say I define class QSISub as a direct subclass of QAPISourceInfo, and
let it inherit __init__().  What's the type of QSISub.__init__()'s
parameter @parent?

According to my reading of your explanation, it's QSISub.  Correct?

If we used QAPISourceInfo instead of T for @parent, then it would be
QAPISourceInfo.  Correct?

Now, perhaps any QAPISourceInfo will do as @parent, perhaps it really
needs to be a QSISub.  We can't know when we write QAPISourceInfo.  But
we don't *have* to get this exactly right for all future subclasses,
because I can always override __init__() when inheritance doesn't give
me the __init__() I want.  Correct?

Say I override __init__(), and have it call super().__init__().  I have
to pass it a QAPISourceInfo @parent.  A QSISub will do (it's a subtype).
Correct?

One more: is bound='QAPISourceInfo' strictly needed?

>> Why do you use T instead of QAPISourceInfo?

[...]