diff mbox series

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

Message ID 20201005195158.2348217-25-jsnow@redhat.com
State New, archived
Headers show
Series qapi: static typing conversion, pt1 | expand

Commit Message

John Snow Oct. 5, 2020, 7:51 p.m. UTC
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(-)

Comments

Markus Armbruster Oct. 7, 2020, 11:55 a.m. UTC | #1
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?

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?

> +        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()
John Snow Oct. 7, 2020, 4:04 p.m. UTC | #2
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()
Markus Armbruster Oct. 8, 2020, 8:42 a.m. UTC | #3
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?

[...]
John Snow Oct. 9, 2020, 2:30 p.m. UTC | #4
On 10/8/20 4:42 AM, Markus Armbruster wrote:
> 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?
> 

That's right.

(I'm realizing that this is maybe not a constraint that we should even 
anticipate here, because maybe we don't wish to say that the parent 
should always be of the same type. but hey, it led to a good mypy lesson.

I'm going to edit it to do the simpler thing for now and leave well 
enough alone. There's another chance to see an interesting pattern of 
TypeVars in the error series in part 4 that I think is actually more 
explicitly appropriate.)

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

Yup!

Here's a little sample program that shows what this kind of typing does:

```
from typing import TypeVar, Optional

class Example:
     T = TypeVar('T', bound='Example')
     def __init__(self: T, parent: Optional[T] = None):
         self.parent = parent

class SubExample(Example):
     pass


x = Example()
y = Example(x)
z = SubExample()
a = Example(x)            # OK
b = Example(z)            # OK
c = SubExample(x)         # BZZZT
d = SubExample(z)         # OK
```

If you check this with mypy, you'll get this error:

```
test.py:17: error: Argument 1 to "SubExample" has incompatible type 
"Example"; expected "Optional[SubExample]"
Found 1 error in 1 file (checked 1 source file)
```

> 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?
> 

You could, but I suggested on IRC the other day that I am not fully 
comfortable with the LSP rules that mypy (sometimes?) enforces. I tend 
not to want to override init with narrower types if I can avoid it, but 
it is true that we do this quite a lot in the codebase already.

(I believe I have seen mypy throw errors about this on occasion, but I 
can't pinpoint the exact circumstances in which it does. It's a point of 
confusion for me.)

> 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?
> 

I'm not sure if bound is strictly required or not. mypy docs just use it 
outright and don't mention why:

https://mypy.readthedocs.io/en/stable/generics.html#generic-methods-and-generic-self

Generally, this constrains a TypeVar to some upper bound, see --

https://mypy.readthedocs.io/en/stable/generics.html#type-variables-with-upper-bounds 


-- but in this case, we're only using that TypeVar for an init method 
that only exists for this type and below, so it might be redundant.

I modified my example program to remove the bound and it appears to fail 
in the same exact way, so it might be pointless in this case. It might 
have a stronger use if you want to re-use the 'T' typevar elsewhere 
where it isn't implicitly bound by the 'self' argument. Maybe it has 
implications for multi-inheritance too, I've not tested it to that level.


Thanks for the good question!

--js
John Snow Oct. 9, 2020, 2:37 p.m. UTC | #5
On 10/9/20 10:30 AM, John Snow wrote:
> On 10/8/20 4:42 AM, Markus Armbruster wrote:
>> 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?
>>
> 
> That's right.
> 
> (I'm realizing that this is maybe not a constraint that we should even 
> anticipate here, because maybe we don't wish to say that the parent 
> should always be of the same type. but hey, it led to a good mypy lesson.
> 
> I'm going to edit it to do the simpler thing for now and leave well 
> enough alone. There's another chance to see an interesting pattern of 
> TypeVars in the error series in part 4 that I think is actually more 
> explicitly appropriate.)
> 
>> If we used QAPISourceInfo instead of T for @parent, then it would be
>> QAPISourceInfo.  Correct?
>>
> 
> Yup!
> 
> Here's a little sample program that shows what this kind of typing does:
> 
> ```
> from typing import TypeVar, Optional
> 
> class Example:
>      T = TypeVar('T', bound='Example')
>      def __init__(self: T, parent: Optional[T] = None):
>          self.parent = parent
> 
> class SubExample(Example):
>      pass
> 
> 
> x = Example()
> y = Example(x)
> z = SubExample()
> a = Example(x)            # OK
> b = Example(z)            # OK
> c = SubExample(x)         # BZZZT
> d = SubExample(z)         # OK
> ```
> 
> If you check this with mypy, you'll get this error:
> 
> ```
> test.py:17: error: Argument 1 to "SubExample" has incompatible type 
> "Example"; expected "Optional[SubExample]"
> Found 1 error in 1 file (checked 1 source file)
> ```
> 
>> 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?
>>
> 
> You could, but I suggested on IRC the other day that I am not fully 
> comfortable with the LSP rules that mypy (sometimes?) enforces. I tend 
> not to want to override init with narrower types if I can avoid it, but 
> it is true that we do this quite a lot in the codebase already.
> 
> (I believe I have seen mypy throw errors about this on occasion, but I 
> can't pinpoint the exact circumstances in which it does. It's a point of 
> confusion for me.)
> 
>> 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?
>>
> 
> I'm not sure if bound is strictly required or not. mypy docs just use it 
> outright and don't mention why:
> 
> https://mypy.readthedocs.io/en/stable/generics.html#generic-methods-and-generic-self 
> 
> 
> Generally, this constrains a TypeVar to some upper bound, see --
> 
> https://mypy.readthedocs.io/en/stable/generics.html#type-variables-with-upper-bounds 
> 
> 
> -- but in this case, we're only using that TypeVar for an init method 
> that only exists for this type and below, so it might be redundant.
> 
> I modified my example program to remove the bound and it appears to fail 
> in the same exact way, so it might be pointless in this case. It might 
> have a stronger use if you want to re-use the 'T' typevar elsewhere 
> where it isn't implicitly bound by the 'self' argument. Maybe it has 
> implications for multi-inheritance too, I've not tested it to that level.
> 

SPOKE TOO SOON, ADDENDUM:

Adding bound= is still necessary to allow type inference to assume the 
minimal base form. It's not needed for my toy example because I don't do 
anything *with* the variable.

In this case, though, the next_line method needs help:

     def next_line(self: T) -> T:
	info = copy.copy(self)
         info.line += 1
         return info

Now, when we return a copy of ourselves, we are going to return 
something that is the same type as ourselves. If we remove the bound 
from the TypeVar here, "info.line" will become an error because mypy 
cannot infer the minimum form.

(It doesn't seem smart enough to notice that the class it is defined in 
does not inherit from anywhere, so it MUST be at least a QAPISourceInfo.)

So I am going to:

(1) Make __init__'s typing less dynamic, because it's not really needed.
(2) Leave the TypeVar in for the benefit of next_line().

--js
diff mbox series

Patch

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]):
         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()
+        )
+        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()