diff mbox series

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

Message ID 20200922210101.4081073-23-jsnow@redhat.com
State Superseded
Headers show
Series qapi: static typing conversion, pt1 | expand

Commit Message

John Snow Sept. 22, 2020, 9 p.m. UTC
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(-)

Comments

Eduardo Habkost Sept. 23, 2020, 2:52 p.m. UTC | #1
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>
Cleber Rosa Sept. 23, 2020, 10:36 p.m. UTC | #2
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.
John Snow Sept. 23, 2020, 11:55 p.m. UTC | #3
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.

>
Markus Armbruster Sept. 25, 2020, 12:22 p.m. UTC | #4
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?
John Snow Sept. 25, 2020, 4:20 p.m. UTC | #5
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
Cleber Rosa Sept. 25, 2020, 5:05 p.m. UTC | #6
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.
John Snow Sept. 25, 2020, 5:20 p.m. UTC | #7
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.

>
diff mbox series

Patch

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