diff mbox series

[v5,19/36] qapi/events.py: add type hint annotations

Message ID 20201005195158.2348217-20-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.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
---
 scripts/qapi/events.py | 46 ++++++++++++++++++++++++++++++++----------
 scripts/qapi/mypy.ini  |  5 -----
 2 files changed, 35 insertions(+), 16 deletions(-)

Comments

Markus Armbruster Oct. 7, 2020, 11:32 a.m. UTC | #1
John Snow <jsnow@redhat.com> writes:

> 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>
> Reviewed-by: Cleber Rosa <crosa@redhat.com>
> ---
>  scripts/qapi/events.py | 46 ++++++++++++++++++++++++++++++++----------
>  scripts/qapi/mypy.ini  |  5 -----
>  2 files changed, 35 insertions(+), 16 deletions(-)
>
> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
> index f840a62ed92..57e0939e963 100644
> --- a/scripts/qapi/events.py
> +++ b/scripts/qapi/events.py
> @@ -12,19 +12,31 @@
>  See the COPYING file in the top-level directory.
>  """
>  
> +from typing import List
> +
>  from .common import c_enum_const, c_name, mcgen
>  from .gen import QAPISchemaModularCVisitor, build_params, ifcontext
> -from .schema import QAPISchemaEnumMember
> +from .schema import (
> +    QAPISchema,
> +    QAPISchemaEnumMember,
> +    QAPISchemaFeature,
> +    QAPISchemaObjectType,
> +)
> +from .source import QAPISourceInfo
>  from .types import gen_enum, gen_enum_lookup
>  
>  
> -def build_event_send_proto(name, arg_type, boxed):
> +def build_event_send_proto(name: str,
> +                           arg_type: QAPISchemaObjectType,
> +                           boxed: bool) -> str:
>      return 'void qapi_event_send_%(c_name)s(%(param)s)' % {
>          'c_name': c_name(name.lower()),
>          'param': build_params(arg_type, boxed)}
>  
>  
> -def gen_event_send_decl(name, arg_type, boxed):
> +def gen_event_send_decl(name: str,
> +                        arg_type: QAPISchemaObjectType,
> +                        boxed: bool) -> str:
>      return mcgen('''
>  
>  %(proto)s;
> @@ -33,7 +45,7 @@ def gen_event_send_decl(name, arg_type, boxed):
>  
>  
>  # Declare and initialize an object 'qapi' using parameters from build_params()
> -def gen_param_var(typ):
> +def gen_param_var(typ: QAPISchemaObjectType) -> str:
>      assert not typ.variants
>      ret = mcgen('''
>      %(c_name)s param = {
> @@ -61,7 +73,11 @@ def gen_param_var(typ):
>      return ret
>  
>  
> -def gen_event_send(name, arg_type, boxed, event_enum_name, event_emit):
> +def gen_event_send(name: str,
> +                   arg_type: QAPISchemaObjectType,
> +                   boxed: bool,
> +                   event_enum_name: str,
> +                   event_emit: str) -> str:
>      # FIXME: Our declaration of local variables (and of 'errp' in the
>      # parameter list) can collide with exploded members of the event's
>      # data type passed in as parameters.  If this collision ever hits in
> @@ -137,15 +153,15 @@ def gen_event_send(name, arg_type, boxed, event_enum_name, event_emit):
>  
>  class QAPISchemaGenEventVisitor(QAPISchemaModularCVisitor):
>  
> -    def __init__(self, prefix):
> +    def __init__(self, prefix: str):
>          super().__init__(
>              prefix, 'qapi-events',
>              ' * Schema-defined QAPI/QMP events', None, __doc__)
>          self._event_enum_name = c_name(prefix + 'QAPIEvent', protect=False)
> -        self._event_enum_members = []
> +        self._event_enum_members: List[QAPISchemaEnumMember] = []
>          self._event_emit_name = c_name(prefix + 'qapi_event_emit')
>  
> -    def _begin_user_module(self, name):
> +    def _begin_user_module(self, name: str) -> None:
>          events = self._module_basename('qapi-events', name)
>          types = self._module_basename('qapi-types', name)
>          visit = self._module_basename('qapi-visit', name)
> @@ -168,7 +184,7 @@ def _begin_user_module(self, name):
>  ''',
>                               types=types))
>  
> -    def visit_end(self):
> +    def visit_end(self) -> None:

Ignorant question: what's the difference between -> None (like here) and
nothing (like __init__() above?

>          self._add_system_module('emit', ' * QAPI Events emission')
>          self._genc.preamble_add(mcgen('''
>  #include "qemu/osdep.h"
[...]
Markus Armbruster Oct. 7, 2020, 11:49 a.m. UTC | #2
Markus Armbruster <armbru@redhat.com> writes:

> John Snow <jsnow@redhat.com> writes:
>
>> 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>
>> Reviewed-by: Cleber Rosa <crosa@redhat.com>
>> ---
>>  scripts/qapi/events.py | 46 ++++++++++++++++++++++++++++++++----------
>>  scripts/qapi/mypy.ini  |  5 -----
>>  2 files changed, 35 insertions(+), 16 deletions(-)
>>
>> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
>> index f840a62ed92..57e0939e963 100644
>> --- a/scripts/qapi/events.py
>> +++ b/scripts/qapi/events.py
>> @@ -12,19 +12,31 @@
[...]
>> @@ -137,15 +153,15 @@ def gen_event_send(name, arg_type, boxed, event_enum_name, event_emit):
>>  
>>  class QAPISchemaGenEventVisitor(QAPISchemaModularCVisitor):
>>  
>> -    def __init__(self, prefix):
>> +    def __init__(self, prefix: str):
>>          super().__init__(
>>              prefix, 'qapi-events',
>>              ' * Schema-defined QAPI/QMP events', None, __doc__)
>>          self._event_enum_name = c_name(prefix + 'QAPIEvent', protect=False)
>> -        self._event_enum_members = []
>> +        self._event_enum_members: List[QAPISchemaEnumMember] = []
>>          self._event_emit_name = c_name(prefix + 'qapi_event_emit')
>>  
>> -    def _begin_user_module(self, name):
>> +    def _begin_user_module(self, name: str) -> None:
>>          events = self._module_basename('qapi-events', name)
>>          types = self._module_basename('qapi-types', name)
>>          visit = self._module_basename('qapi-visit', name)
>> @@ -168,7 +184,7 @@ def _begin_user_module(self, name):
>>  ''',
>>                               types=types))
>>  
>> -    def visit_end(self):
>> +    def visit_end(self) -> None:
>
> Ignorant question: what's the difference between -> None (like here) and
> nothing (like __init__() above?

Looks like commit message of PATCH 24 has an answer.

Copy to all the commits that omit -> None similarly?

>>          self._add_system_module('emit', ' * QAPI Events emission')
>>          self._genc.preamble_add(mcgen('''
>>  #include "qemu/osdep.h"
> [...]
John Snow Oct. 7, 2020, 3:39 p.m. UTC | #3
On 10/7/20 7:32 AM, Markus Armbruster wrote:
> Ignorant question: what's the difference between -> None (like here) and
> nothing (like __init__() above?

This came up in Cleber's review, too.

Mypy supports a gradual typing paradigm; it is designed to facilitate 
what we are doing here: the gradual adding of types until we are able to 
enforce static typing everywhere.

To that end, mypy uses a simple heuristic to determine if a function is 
"typed" or "untyped": did you use any type annotations for it?

Meanwhile, __init__ never returns anything. You do not need to annotate 
its return type. mypy knows what the return type is and must be. In the 
case of __init__ with no parameters, it is both untyped and strictly typed!

Annotating the return type for no-parameter init methods convinces mypy 
that this is a strictly typed method. It doesn't do this automatically, 
because doing so might enable more checks than you were ready for simply 
because mypy was able to accurately surmise the typing of just __init__.

So it's just a little flip switch to enable strict typing, really.

--js

https://mypy.readthedocs.io/en/stable/class_basics.html#annotating-init-methods
John Snow Oct. 7, 2020, 3:46 p.m. UTC | #4
On 10/7/20 7:49 AM, Markus Armbruster wrote:
> Looks like commit message of PATCH 24 has an answer.
> 
> Copy to all the commits that omit -> None similarly?

Probably not needed.

It's covered by the class basics section in the mypy manual; 
https://mypy.readthedocs.io/en/stable/class_basics.html#annotating-init-methods

and if you should happen to omit annotations for __init__ entirely as a 
novice, you will be treated to messages such as these:

qapi/source.py:18: error: Function is missing a return type annotation
qapi/source.py:18: note: Use "-> None" if function does not return a value
Found 1 error in 1 file (checked 14 source files)

Pretty good error!

There's no error if you DO explicitly add a -> None from __init__, but 
at worst it's just extraneous (but correct) information.

I could add a note to the style guide that I prefer omitting the return 
from __init__. I like omitting as much as I possibly can.

(You'll notice I don't always type every local, either -- when local 
inference is accurate, I leave it alone.)

--js
Markus Armbruster Oct. 8, 2020, 7:41 a.m. UTC | #5
John Snow <jsnow@redhat.com> writes:

> On 10/7/20 7:32 AM, Markus Armbruster wrote:
>> Ignorant question: what's the difference between -> None (like here) and
>> nothing (like __init__() above?
>
> This came up in Cleber's review, too.
>
> Mypy supports a gradual typing paradigm; it is designed to facilitate
> what we are doing here: the gradual adding of types until we are able
> to enforce static typing everywhere.
>
> To that end, mypy uses a simple heuristic to determine if a function
> is "typed" or "untyped": did you use any type annotations for it?
>
> Meanwhile, __init__ never returns anything. You do not need to
> annotate its return type. mypy knows what the return type is and must
> be. In the case of __init__ with no parameters, it is both untyped and
> strictly typed!
>
> Annotating the return type for no-parameter init methods convinces
> mypy that this is a strictly typed method. It doesn't do this
> automatically, because doing so might enable more checks than you were
> ready for simply because mypy was able to accurately surmise the
> typing of just __init__.
>
> So it's just a little flip switch to enable strict typing, really.
>
> --js
>
> https://mypy.readthedocs.io/en/stable/class_basics.html#annotating-init-methods

I now understand we can omit -> None, except when it's the only type
hint, because omitting it then would make it untyped, which is not what
we want.

Is this just for __init__(), or is it for any function that doesn't
return anything?
Markus Armbruster Oct. 8, 2020, 9:16 a.m. UTC | #6
John Snow <jsnow@redhat.com> writes:

> On 10/7/20 7:49 AM, Markus Armbruster wrote:
>> Looks like commit message of PATCH 24 has an answer.
>> Copy to all the commits that omit -> None similarly?
>
> Probably not needed.
>
> It's covered by the class basics section in the mypy manual;
> https://mypy.readthedocs.io/en/stable/class_basics.html#annotating-init-methods
>
> and if you should happen to omit annotations for __init__ entirely as
> a novice, you will be treated to messages such as these:
>
> qapi/source.py:18: error: Function is missing a return type annotation
> qapi/source.py:18: note: Use "-> None" if function does not return a value
> Found 1 error in 1 file (checked 14 source files)
>
> Pretty good error!
>
> There's no error if you DO explicitly add a -> None from __init__, but
> at worst it's just extraneous (but correct) information.

Let me apply the zero-one-infinity rule:

* Zero: explain it in none of the commit messages, i.e. dumb down PATCH
  24.

* One: explain it in one.  Do it in the first one, please (here, I
  think).

* Infinity: explain it in every one.

Up to you!

> I could add a note to the style guide that I prefer omitting the
> return from __init__. I like omitting as much as I possibly can.
>
> (You'll notice I don't always type every local, either -- when local
> inference is accurate, I leave it alone.)

Type inference can save us from repeating the obvious over and over, and
that's lovely.
John Snow Oct. 8, 2020, 3:35 p.m. UTC | #7
On 10/8/20 3:41 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 10/7/20 7:32 AM, Markus Armbruster wrote:
>>> Ignorant question: what's the difference between -> None (like here) and
>>> nothing (like __init__() above?
>>
>> This came up in Cleber's review, too.
>>
>> Mypy supports a gradual typing paradigm; it is designed to facilitate
>> what we are doing here: the gradual adding of types until we are able
>> to enforce static typing everywhere.
>>
>> To that end, mypy uses a simple heuristic to determine if a function
>> is "typed" or "untyped": did you use any type annotations for it?
>>
>> Meanwhile, __init__ never returns anything. You do not need to
>> annotate its return type. mypy knows what the return type is and must
>> be. In the case of __init__ with no parameters, it is both untyped and
>> strictly typed!
>>
>> Annotating the return type for no-parameter init methods convinces
>> mypy that this is a strictly typed method. It doesn't do this
>> automatically, because doing so might enable more checks than you were
>> ready for simply because mypy was able to accurately surmise the
>> typing of just __init__.
>>
>> So it's just a little flip switch to enable strict typing, really.
>>
>> --js
>>
>> https://mypy.readthedocs.io/en/stable/class_basics.html#annotating-init-methods
> 
> I now understand we can omit -> None, except when it's the only type
> hint, because omitting it then would make it untyped, which is not what
> we want.
> 
> Is this just for __init__(), or is it for any function that doesn't
> return anything?
> 

*just* __init__. Some other dunders have types that could be assumed, 
but aren't. I asked about this for some other well-known but 
hard-to-type dunders like __exit__ on the Typing-SIG list. Not a strong 
response yet, I might push again later.

--js
John Snow Oct. 8, 2020, 4:19 p.m. UTC | #8
On 10/8/20 5:16 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 10/7/20 7:49 AM, Markus Armbruster wrote:
>>> Looks like commit message of PATCH 24 has an answer.
>>> Copy to all the commits that omit -> None similarly?
>>
>> Probably not needed.
>>
>> It's covered by the class basics section in the mypy manual;
>> https://mypy.readthedocs.io/en/stable/class_basics.html#annotating-init-methods
>>
>> and if you should happen to omit annotations for __init__ entirely as
>> a novice, you will be treated to messages such as these:
>>
>> qapi/source.py:18: error: Function is missing a return type annotation
>> qapi/source.py:18: note: Use "-> None" if function does not return a value
>> Found 1 error in 1 file (checked 14 source files)
>>
>> Pretty good error!
>>
>> There's no error if you DO explicitly add a -> None from __init__, but
>> at worst it's just extraneous (but correct) information.
> 
> Let me apply the zero-one-infinity rule:
> 
> * Zero: explain it in none of the commit messages, i.e. dumb down PATCH
>    24.
> 
> * One: explain it in one.  Do it in the first one, please (here, I
>    think).
> 
> * Infinity: explain it in every one.
> 
> Up to you!
> 

I'm just bad at predicting which things people will want explained. I 
know people don't read the cover letters already, so I'd rather go for 
less instead of more.

I think you and Cleber each noticed a different angle of this: Cleber 
noticed the first time I *did* annotate __init__'s return and you 
noticed the first time I *didn't*.

I'll just add it here too, but I have doubts it will be useful reference 
once it's merged. I guess it doesn't hurt to add it either, I just find 
it difficult to predict what reviewers will want.

>> I could add a note to the style guide that I prefer omitting the
>> return from __init__. I like omitting as much as I possibly can.
>>
>> (You'll notice I don't always type every local, either -- when local
>> inference is accurate, I leave it alone.)
> 
> Type inference can save us from repeating the obvious over and over, and
> that's lovely.
>
Markus Armbruster Oct. 9, 2020, 7:21 a.m. UTC | #9
John Snow <jsnow@redhat.com> writes:

[...]
> I just find it difficult to predict what reviewers will want.

Capricious bunch, those reviewers.

[...]
diff mbox series

Patch

diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index f840a62ed92..57e0939e963 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -12,19 +12,31 @@ 
 See the COPYING file in the top-level directory.
 """
 
+from typing import List
+
 from .common import c_enum_const, c_name, mcgen
 from .gen import QAPISchemaModularCVisitor, build_params, ifcontext
-from .schema import QAPISchemaEnumMember
+from .schema import (
+    QAPISchema,
+    QAPISchemaEnumMember,
+    QAPISchemaFeature,
+    QAPISchemaObjectType,
+)
+from .source import QAPISourceInfo
 from .types import gen_enum, gen_enum_lookup
 
 
-def build_event_send_proto(name, arg_type, boxed):
+def build_event_send_proto(name: str,
+                           arg_type: QAPISchemaObjectType,
+                           boxed: bool) -> str:
     return 'void qapi_event_send_%(c_name)s(%(param)s)' % {
         'c_name': c_name(name.lower()),
         'param': build_params(arg_type, boxed)}
 
 
-def gen_event_send_decl(name, arg_type, boxed):
+def gen_event_send_decl(name: str,
+                        arg_type: QAPISchemaObjectType,
+                        boxed: bool) -> str:
     return mcgen('''
 
 %(proto)s;
@@ -33,7 +45,7 @@  def gen_event_send_decl(name, arg_type, boxed):
 
 
 # Declare and initialize an object 'qapi' using parameters from build_params()
-def gen_param_var(typ):
+def gen_param_var(typ: QAPISchemaObjectType) -> str:
     assert not typ.variants
     ret = mcgen('''
     %(c_name)s param = {
@@ -61,7 +73,11 @@  def gen_param_var(typ):
     return ret
 
 
-def gen_event_send(name, arg_type, boxed, event_enum_name, event_emit):
+def gen_event_send(name: str,
+                   arg_type: QAPISchemaObjectType,
+                   boxed: bool,
+                   event_enum_name: str,
+                   event_emit: str) -> str:
     # FIXME: Our declaration of local variables (and of 'errp' in the
     # parameter list) can collide with exploded members of the event's
     # data type passed in as parameters.  If this collision ever hits in
@@ -137,15 +153,15 @@  def gen_event_send(name, arg_type, boxed, event_enum_name, event_emit):
 
 class QAPISchemaGenEventVisitor(QAPISchemaModularCVisitor):
 
-    def __init__(self, prefix):
+    def __init__(self, prefix: str):
         super().__init__(
             prefix, 'qapi-events',
             ' * Schema-defined QAPI/QMP events', None, __doc__)
         self._event_enum_name = c_name(prefix + 'QAPIEvent', protect=False)
-        self._event_enum_members = []
+        self._event_enum_members: List[QAPISchemaEnumMember] = []
         self._event_emit_name = c_name(prefix + 'qapi_event_emit')
 
-    def _begin_user_module(self, name):
+    def _begin_user_module(self, name: str) -> None:
         events = self._module_basename('qapi-events', name)
         types = self._module_basename('qapi-types', name)
         visit = self._module_basename('qapi-visit', name)
@@ -168,7 +184,7 @@  def _begin_user_module(self, name):
 ''',
                              types=types))
 
-    def visit_end(self):
+    def visit_end(self) -> None:
         self._add_system_module('emit', ' * QAPI Events emission')
         self._genc.preamble_add(mcgen('''
 #include "qemu/osdep.h"
@@ -189,7 +205,13 @@  def visit_end(self):
                              event_emit=self._event_emit_name,
                              event_enum=self._event_enum_name))
 
-    def visit_event(self, name, info, ifcond, features, arg_type, boxed):
+    def visit_event(self,
+                    name: str,
+                    info: QAPISourceInfo,
+                    ifcond: List[str],
+                    features: List[QAPISchemaFeature],
+                    arg_type: QAPISchemaObjectType,
+                    boxed: bool) -> None:
         with ifcontext(ifcond, self._genh, self._genc):
             self._genh.add(gen_event_send_decl(name, arg_type, boxed))
             self._genc.add(gen_event_send(name, arg_type, boxed,
@@ -200,7 +222,9 @@  def visit_event(self, name, info, ifcond, features, arg_type, boxed):
         self._event_enum_members.append(QAPISchemaEnumMember(name, None))
 
 
-def gen_events(schema, output_dir, prefix):
+def gen_events(schema: QAPISchema,
+               output_dir: str,
+               prefix: str) -> None:
     vis = QAPISchemaGenEventVisitor(prefix)
     schema.visit(vis)
     vis.write(output_dir)
diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
index 00fac15dc6e..5df11e53fd1 100644
--- a/scripts/qapi/mypy.ini
+++ b/scripts/qapi/mypy.ini
@@ -14,11 +14,6 @@  disallow_untyped_defs = False
 disallow_incomplete_defs = False
 check_untyped_defs = False
 
-[mypy-qapi.events]
-disallow_untyped_defs = False
-disallow_incomplete_defs = False
-check_untyped_defs = False
-
 [mypy-qapi.expr]
 disallow_untyped_defs = False
 disallow_incomplete_defs = False