1. Patchew
  2. QEMU
  3. [PATCH v2 00/38] qapi: static typing conversion, pt1
  4. View patch

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

John Snow posted 38 patches 5 years, 4 months ago
Maintainers: Eduardo Habkost <ehabkost@redhat.com>, Markus Armbruster <armbru@redhat.com>, Michael Roth <mdroth@linux.vnet.ibm.com>, Cleber Rosa <crosa@redhat.com>
There is a newer version of this series
[PATCH v2 22/38] qapi/source.py: add type hint annotations
Posted by John Snow 5 years, 4 months ago 
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
-
 [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:
         # 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()
-- 
2.26.2
Re: [PATCH v2 22/38] qapi/source.py: add type hint annotations
Posted by Eduardo Habkost 5 years, 4 months ago 
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>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo
Re: [PATCH v2 22/38] qapi/source.py: add type hint annotations
Posted by Cleber Rosa 5 years, 4 months ago 
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.

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

- Cleber.
Re: [PATCH v2 22/38] qapi/source.py: add type hint annotations
Posted by John Snow 5 years, 4 months ago 
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!"

When __init__ has some arguments, you only need to type those arguments 
and not the return type. The sentinel is not needed.

__init__ *never* returns anything, so I opted to omit this useless 
annotation whenever it was possible to do so.

> - Cleber.
>
Re: [PATCH v2 22/38] qapi/source.py: add type hint annotations
Posted by Markus Armbruster 5 years, 4 months ago 
John Snow <jsnow@redhat.com> writes:

> 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>
[...]
>>> 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!"
>
> When __init__ has some arguments, you only need to type those
> arguments and not the return type. The sentinel is not needed.
>
> __init__ *never* returns anything, so I opted to omit this useless
> annotation whenever it was possible to do so.

Worth capturing in a comment and/or commit message?
Re: [PATCH v2 22/38] qapi/source.py: add type hint annotations
Posted by John Snow 5 years, 4 months ago 
On 9/25/20 8:22 AM, Markus Armbruster wrote:
> Worth capturing in a comment and/or commit message?

Doesn't hurt me any to do so.

It's also good fodder for a style guide document, which would centralize 
such things.

Here is a formal IOU: https://gitlab.com/jsnow/qemu/-/issues/7

--js
Re: [PATCH v2 22/38] qapi/source.py: add type hint annotations
Posted by Cleber Rosa 5 years, 4 months ago 
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?

- Cleber.
Re: [PATCH v2 22/38] qapi/source.py: add type hint annotations
Posted by John Snow 5 years, 4 months ago 
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.
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.