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

[John Snow <jsnow@redhat.com>]

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.)

> Why do you use T instead of QAPISourceInfo?
> 
>>           self.fname = fname
>>           self.line = line
>>           self.parent = parent
>> -        self.pragma = parent.pragma if parent else QAPISchemaPragma()
>> -        self.defn_meta = None
>> -        self.defn_name = None
>> +        self.pragma: QAPISchemaPragma = (
>> +            parent.pragma if parent else QAPISchemaPragma()
>> +        )
> 
> Type inference fail?
> 

Yes:

qapi/source.py:34: error: Cannot determine type of 'pragma'
Found 1 error in 1 file (checked 14 source files)

>> +        self.defn_meta: Optional[str] = None
>> +        self.defn_name: Optional[str] = None
>>   
>> -    def set_defn(self, meta, name):
>> +    def set_defn(self, meta: str, name: str) -> None:
>>           self.defn_meta = meta
>>           self.defn_name = name
>>   
>> -    def next_line(self):
>> +    def next_line(self: T) -> T:
>>           info = copy.copy(self)
>>           info.line += 1
>>           return info
>>   
>> -    def loc(self):
>> +    def loc(self) -> str:
>>           if self.fname is None:
>>               return sys.argv[0]
>>           ret = self.fname
>> @@ -49,13 +54,13 @@ def loc(self):
>>               ret += ':%d' % self.line
>>           return ret
>>   
>> -    def in_defn(self):
>> +    def in_defn(self) -> str:
>>           if self.defn_name:
>>               return "%s: In %s '%s':\n" % (self.fname,
>>                                             self.defn_meta, self.defn_name)
>>           return ''
>>   
>> -    def include_path(self):
>> +    def include_path(self) -> str:
>>           ret = ''
>>           parent = self.parent
>>           while parent:
>> @@ -63,5 +68,5 @@ def include_path(self):
>>               parent = parent.parent
>>           return ret
>>   
>> -    def __str__(self):
>> +    def __str__(self) -> str:
>>           return self.include_path() + self.in_defn() + self.loc()