| Message ID | 20201005195158.2348217-20-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: static typing conversion, pt1 | expand |
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 <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" > [...]
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
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
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?
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.
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
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. >
John Snow <jsnow@redhat.com> writes:
[...]
> I just find it difficult to predict what reviewers will want.
Capricious bunch, those reviewers.
[...]
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