Re: [PATCH v2 22/38] qapi/source.py: add type hint annotations

[John Snow <jsnow@redhat.com>]

On 9/25/20 1:05 PM, Cleber Rosa wrote:
> On Wed, Sep 23, 2020 at 07:55:50PM -0400, John Snow wrote:
>> On 9/23/20 6:36 PM, Cleber Rosa wrote:
>>> On Tue, Sep 22, 2020 at 05:00:45PM -0400, John Snow wrote:
>>>> Annotations do not change runtime behavior.
>>>> This commit *only* adds annotations.
>>>>
>>>> Signed-off-by: John Snow <jsnow@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 9da1dccef4..43c8bd1973 100644
>>>> --- a/scripts/qapi/mypy.ini
>>>> +++ b/scripts/qapi/mypy.ini
>>>> @@ -39,11 +39,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
>>>> -
>>>
>>> This is what I meant in my comment in the previous patch.  It looks
>>> like a mix of commit grannurality styles.  Not a blocker though.
>>>
>>
>> Yep. Just how the chips fell. Some files were just very quick to cleanup and
>> I didn't have to refactor them much when I split things out, so the
>> enablements got rolled in.
>>
>> I will, once reviews are in (and there is a commitment to merge), try to
>> squash things where it seems appropriate.
>>
>>>>    [mypy-qapi.types]
>>>>    disallow_untyped_defs = False
>>>>    disallow_incomplete_defs = False
>>>> diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
>>>> index e97b9a8e15..1cc6a5b82d 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:
>>>
>>> I don't follow the reason for typing this...
>>>
>>>>            # 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]):
>>>
>>> And not this... to tune my review approach, should I assume that this
>>> series intends to add complete type hints or not?
>>>
>>
>> This is a mypy quirk you've discovered that I've simply forgotten about.
>>
>> When __init__ has *no* arguments, you need to annotate its return to explain
>> to mypy that you have fully typed that method. It's a sentinel that says
>> "Please type check this class!"
>>
> 
> Ouch.  Is this a permanent quirk or a known bug that will eventually
> be addressed?

Permanent, it is a feature.

mypy intentionally supports gradual typing as a paradigm: it allows you 
to intermix "typed" and "untyped" functions.

```
def __init__(self):
     pass
```

Happens to pass as both untyped and fully typed. In order to distinguish 
it in this one case, you must add the return annotation as a declaration 
of intent.

However, when using '--strict' mode, you are declaring your intent to 
mypy that everything MUST be strictly typed, so perhaps in this case it 
would be possible to omit the annotation for __init__.

So maybe someday this will change; but given how uncommon it is to write 
no-argument init methods, I am hardly bothered by it. Mypy will remind 
you if you forget.

> 
> - Cleber.
>