* [PATCH v2 01/12] qapi/commands: assert arg_type is not None
2020-12-17 1:59 [PATCH v2 00/12] qapi: static typing conversion, pt1.5 John Snow
@ 2020-12-17 1:59 ` John Snow
2020-12-17 1:59 ` [PATCH v2 02/12] qapi/events: fix visit_event typing John Snow
` (11 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2020-12-17 1:59 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Michael Roth, Markus Armbruster, Cleber Rosa,
Marc-André Lureau, John Snow
when boxed is true, expr.py asserts that we must have
arguments. Ultimately, this should mean that if boxed is True, that
arg_type should be defined. Mypy cannot infer this, and does not support
'stateful' type inference, e.g.:
```
if x:
assert y is not None
...
if x:
y.etc()
```
does not work, because mypy does not statefully remember the conditional
assertion in the second block. Help mypy out by creating a new local
that it can track more easily.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/commands.py | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 50978090b44..71744f48a35 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -126,6 +126,9 @@ def gen_marshal(name: str,
boxed: bool,
ret_type: Optional[QAPISchemaType]) -> str:
have_args = boxed or (arg_type and not arg_type.is_empty())
+ if have_args:
+ assert arg_type is not None
+ arg_type_c_name = arg_type.c_name()
ret = mcgen('''
@@ -147,7 +150,7 @@ def gen_marshal(name: str,
ret += mcgen('''
%(c_name)s arg = {0};
''',
- c_name=arg_type.c_name())
+ c_name=arg_type_c_name)
ret += mcgen('''
@@ -163,7 +166,7 @@ def gen_marshal(name: str,
ok = visit_check_struct(v, errp);
}
''',
- c_arg_type=arg_type.c_name())
+ c_arg_type=arg_type_c_name)
else:
ret += mcgen('''
ok = visit_check_struct(v, errp);
@@ -193,7 +196,7 @@ def gen_marshal(name: str,
ret += mcgen('''
visit_type_%(c_arg_type)s_members(v, &arg, NULL);
''',
- c_arg_type=arg_type.c_name())
+ c_arg_type=arg_type_c_name)
ret += mcgen('''
visit_end_struct(v, NULL);
--
2.26.2
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 02/12] qapi/events: fix visit_event typing
2020-12-17 1:59 [PATCH v2 00/12] qapi: static typing conversion, pt1.5 John Snow
2020-12-17 1:59 ` [PATCH v2 01/12] qapi/commands: assert arg_type is not None John Snow
@ 2020-12-17 1:59 ` John Snow
2020-12-17 1:59 ` [PATCH v2 03/12] qapi/main: handle theoretical None-return from re.match() John Snow
` (10 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2020-12-17 1:59 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Michael Roth, Markus Armbruster, Cleber Rosa,
Marc-André Lureau, John Snow
Actually, the arg_type can indeed be Optional.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/events.py | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index 599f3d1f564..9851653b9d1 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -12,7 +12,7 @@
See the COPYING file in the top-level directory.
"""
-from typing import List
+from typing import List, Optional
from .common import c_enum_const, c_name, mcgen
from .gen import QAPISchemaModularCVisitor, build_params, ifcontext
@@ -27,7 +27,7 @@
def build_event_send_proto(name: str,
- arg_type: QAPISchemaObjectType,
+ arg_type: Optional[QAPISchemaObjectType],
boxed: bool) -> str:
return 'void qapi_event_send_%(c_name)s(%(param)s)' % {
'c_name': c_name(name.lower()),
@@ -35,7 +35,7 @@ def build_event_send_proto(name: str,
def gen_event_send_decl(name: str,
- arg_type: QAPISchemaObjectType,
+ arg_type: Optional[QAPISchemaObjectType],
boxed: bool) -> str:
return mcgen('''
@@ -78,7 +78,7 @@ def gen_param_var(typ: QAPISchemaObjectType) -> str:
def gen_event_send(name: str,
- arg_type: QAPISchemaObjectType,
+ arg_type: Optional[QAPISchemaObjectType],
boxed: bool,
event_enum_name: str,
event_emit: str) -> str:
@@ -99,6 +99,7 @@ def gen_event_send(name: str,
proto=build_event_send_proto(name, arg_type, boxed))
if have_args:
+ assert arg_type is not None
ret += mcgen('''
QObject *obj;
Visitor *v;
@@ -114,6 +115,7 @@ def gen_event_send(name: str,
name=name)
if have_args:
+ assert arg_type is not None
ret += mcgen('''
v = qobject_output_visitor_new(&obj);
''')
@@ -214,7 +216,7 @@ def visit_event(self,
info: QAPISourceInfo,
ifcond: List[str],
features: List[QAPISchemaFeature],
- arg_type: QAPISchemaObjectType,
+ arg_type: Optional[QAPISchemaObjectType],
boxed: bool) -> None:
with ifcontext(ifcond, self._genh, self._genc):
self._genh.add(gen_event_send_decl(name, arg_type, boxed))
--
2.26.2
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 03/12] qapi/main: handle theoretical None-return from re.match()
2020-12-17 1:59 [PATCH v2 00/12] qapi: static typing conversion, pt1.5 John Snow
2020-12-17 1:59 ` [PATCH v2 01/12] qapi/commands: assert arg_type is not None John Snow
2020-12-17 1:59 ` [PATCH v2 02/12] qapi/events: fix visit_event typing John Snow
@ 2020-12-17 1:59 ` John Snow
2020-12-17 1:59 ` [PATCH v2 04/12] qapi/gen: assert that _start_if is not None in _wrap_ifcond John Snow
` (9 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2020-12-17 1:59 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Michael Roth, Markus Armbruster, Cleber Rosa,
Marc-André Lureau, John Snow
Mypy cannot understand that this match can never be None, so help it
along.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/main.py | 2 ++
1 file changed, 2 insertions(+)
diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
index 42517210b80..703e7ed1ed5 100644
--- a/scripts/qapi/main.py
+++ b/scripts/qapi/main.py
@@ -23,6 +23,8 @@
def invalid_prefix_char(prefix: str) -> Optional[str]:
match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
+ # match cannot be None, but mypy cannot infer that.
+ assert match is not None
if match.end() != len(prefix):
return prefix[match.end()]
return None
--
2.26.2
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 04/12] qapi/gen: assert that _start_if is not None in _wrap_ifcond
2020-12-17 1:59 [PATCH v2 00/12] qapi: static typing conversion, pt1.5 John Snow
` (2 preceding siblings ...)
2020-12-17 1:59 ` [PATCH v2 03/12] qapi/main: handle theoretical None-return from re.match() John Snow
@ 2020-12-17 1:59 ` John Snow
2021-01-13 15:14 ` Markus Armbruster
2020-12-17 1:59 ` [PATCH v2 05/12] qapi/gen: use './builtin' for the built-in module name John Snow
` (8 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: John Snow @ 2020-12-17 1:59 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Michael Roth, Markus Armbruster, Cleber Rosa,
Marc-André Lureau, John Snow
We already assert this in end_if, but that's opaque to mypy. Do it in
_wrap_ifcond instead. Same effect at runtime, but mypy can now infer
the type in _wrap_ifcond's body.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/gen.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index b40f18eee3c..a6dc991b1d0 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -130,11 +130,11 @@ def start_if(self, ifcond: List[str]) -> None:
self._start_if = (ifcond, self._body, self._preamble)
def end_if(self) -> None:
- assert self._start_if
self._wrap_ifcond()
self._start_if = None
def _wrap_ifcond(self) -> None:
+ assert self._start_if
self._body = _wrap_ifcond(self._start_if[0],
self._start_if[1], self._body)
self._preamble = _wrap_ifcond(self._start_if[0],
--
2.26.2
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 04/12] qapi/gen: assert that _start_if is not None in _wrap_ifcond
2020-12-17 1:59 ` [PATCH v2 04/12] qapi/gen: assert that _start_if is not None in _wrap_ifcond John Snow
@ 2021-01-13 15:14 ` Markus Armbruster
2021-01-13 21:34 ` John Snow
0 siblings, 1 reply; 26+ messages in thread
From: Markus Armbruster @ 2021-01-13 15:14 UTC (permalink / raw)
To: John Snow
Cc: Michael Roth, Cleber Rosa, qemu-devel, Marc-André Lureau,
Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> We already assert this in end_if, but that's opaque to mypy. Do it in
> _wrap_ifcond instead. Same effect at runtime, but mypy can now infer
> the type in _wrap_ifcond's body.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/gen.py | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index b40f18eee3c..a6dc991b1d0 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -130,11 +130,11 @@ def start_if(self, ifcond: List[str]) -> None:
> self._start_if = (ifcond, self._body, self._preamble)
>
> def end_if(self) -> None:
> - assert self._start_if
> self._wrap_ifcond()
> self._start_if = None
>
> def _wrap_ifcond(self) -> None:
> + assert self._start_if
> self._body = _wrap_ifcond(self._start_if[0],
> self._start_if[1], self._body)
> self._preamble = _wrap_ifcond(self._start_if[0],
Let's inline ._wrap_ifcond() into .end_if(). Not a blocker, of course.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 04/12] qapi/gen: assert that _start_if is not None in _wrap_ifcond
2021-01-13 15:14 ` Markus Armbruster
@ 2021-01-13 21:34 ` John Snow
0 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2021-01-13 21:34 UTC (permalink / raw)
To: Markus Armbruster
Cc: Michael Roth, Cleber Rosa, qemu-devel, Marc-André Lureau,
Eduardo Habkost
On 1/13/21 10:14 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> We already assert this in end_if, but that's opaque to mypy. Do it in
>> _wrap_ifcond instead. Same effect at runtime, but mypy can now infer
>> the type in _wrap_ifcond's body.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qapi/gen.py | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>> index b40f18eee3c..a6dc991b1d0 100644
>> --- a/scripts/qapi/gen.py
>> +++ b/scripts/qapi/gen.py
>> @@ -130,11 +130,11 @@ def start_if(self, ifcond: List[str]) -> None:
>> self._start_if = (ifcond, self._body, self._preamble)
>>
>> def end_if(self) -> None:
>> - assert self._start_if
>> self._wrap_ifcond()
>> self._start_if = None
>>
>> def _wrap_ifcond(self) -> None:
>> + assert self._start_if
>> self._body = _wrap_ifcond(self._start_if[0],
>> self._start_if[1], self._body)
>> self._preamble = _wrap_ifcond(self._start_if[0],
>
> Let's inline ._wrap_ifcond() into .end_if(). Not a blocker, of course.
>
I thought I did do this, but I guess it didn't make it out to list. I'll
look into that.
I was planning on taking your series and trying to splice it into this
one here and sending it back out as a conjoined thing.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 05/12] qapi/gen: use './builtin' for the built-in module name
2020-12-17 1:59 [PATCH v2 00/12] qapi: static typing conversion, pt1.5 John Snow
` (3 preceding siblings ...)
2020-12-17 1:59 ` [PATCH v2 04/12] qapi/gen: assert that _start_if is not None in _wrap_ifcond John Snow
@ 2020-12-17 1:59 ` John Snow
2020-12-17 1:59 ` [PATCH v2 06/12] qapi/source: Add builtin null-object sentinel John Snow
` (7 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2020-12-17 1:59 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Michael Roth, Markus Armbruster, Cleber Rosa,
Marc-André Lureau, John Snow
Use this in preference to 'None', which helps remove some edge cases in
the typing.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/gen.py | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index a6dc991b1d0..476d0adac4e 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -245,23 +245,23 @@ def __init__(self,
self._pydoc = pydoc
self._genc: Optional[QAPIGenC] = None
self._genh: Optional[QAPIGenH] = None
- self._module: Dict[Optional[str], Tuple[QAPIGenC, QAPIGenH]] = {}
+ self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH]] = {}
self._main_module: Optional[str] = None
@staticmethod
- def _is_user_module(name: Optional[str]) -> bool:
- return bool(name and not name.startswith('./'))
+ def _is_user_module(name: str) -> bool:
+ return not name.startswith('./')
@staticmethod
- def _is_builtin_module(name: Optional[str]) -> bool:
- return not name
+ def _is_builtin_module(name: str) -> bool:
+ return name == './builtin'
- def _module_dirname(self, name: Optional[str]) -> str:
+ def _module_dirname(self, name: str) -> str:
if self._is_user_module(name):
return os.path.dirname(name)
return ''
- def _module_basename(self, what: str, name: Optional[str]) -> str:
+ def _module_basename(self, what: str, name: str) -> str:
ret = '' if self._is_builtin_module(name) else self._prefix
if self._is_user_module(name):
basename = os.path.basename(name)
@@ -269,15 +269,15 @@ def _module_basename(self, what: str, name: Optional[str]) -> str:
if name != self._main_module:
ret += '-' + os.path.splitext(basename)[0]
else:
- name = name[2:] if name else 'builtin'
- ret += re.sub(r'-', '-' + name + '-', what)
+ assert name.startswith('./')
+ ret += re.sub(r'-', '-' + name[2:] + '-', what)
return ret
- def _module_filename(self, what: str, name: Optional[str]) -> str:
+ def _module_filename(self, what: str, name: str) -> str:
return os.path.join(self._module_dirname(name),
self._module_basename(what, name))
- def _add_module(self, name: Optional[str], blurb: str) -> None:
+ def _add_module(self, name: str, blurb: str) -> None:
basename = self._module_filename(self._what, name)
genc = QAPIGenC(basename + '.c', blurb, self._pydoc)
genh = QAPIGenH(basename + '.h', blurb, self._pydoc)
@@ -290,8 +290,8 @@ def _add_user_module(self, name: str, blurb: str) -> None:
self._main_module = name
self._add_module(name, blurb)
- def _add_system_module(self, name: Optional[str], blurb: str) -> None:
- self._add_module(name and './' + name, blurb)
+ def _add_system_module(self, name: str, blurb: str) -> None:
+ self._add_module('./' + name, blurb)
def write(self, output_dir: str, opt_builtins: bool = False) -> None:
for name in self._module:
@@ -310,7 +310,7 @@ def _begin_user_module(self, name: str) -> None:
def visit_module(self, name: Optional[str]) -> None:
if name is None:
if self._builtin_blurb:
- self._add_system_module(None, self._builtin_blurb)
+ self._add_system_module('builtin', self._builtin_blurb)
self._begin_system_module(name)
else:
# The built-in module has not been created. No code may
--
2.26.2
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 06/12] qapi/source: Add builtin null-object sentinel
2020-12-17 1:59 [PATCH v2 00/12] qapi: static typing conversion, pt1.5 John Snow
` (4 preceding siblings ...)
2020-12-17 1:59 ` [PATCH v2 05/12] qapi/gen: use './builtin' for the built-in module name John Snow
@ 2020-12-17 1:59 ` John Snow
2021-01-13 15:39 ` Markus Armbruster
2020-12-17 1:59 ` [PATCH v2 07/12] qapi/schema: make QAPISourceInfo mandatory John Snow
` (6 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: John Snow @ 2020-12-17 1:59 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Michael Roth, Markus Armbruster, Cleber Rosa,
Marc-André Lureau, John Snow
We use None to represent an object that has no source information
because it's a builtin. This complicates interface typing, since many
interfaces expect that there is an info object available to print errors
with.
Introduce a special QAPISourceInfo that represents these built-ins so
that if an error should so happen to occur relating to one of these
builtins that we will be able to print its information, and interface
typing becomes simpler: you will always have a source info object.
This object will evaluate as False, so "if info" remains a valid
idiomatic construct.
NB: It was intentional to not allow empty constructors or similar to
create "empty" source info objects; callers must explicitly invoke
'builtin()' to pro-actively opt into using the sentinel. This should
prevent use-by-accident.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/source.py | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
index d7a79a9b8ae..a049b73b57b 100644
--- a/scripts/qapi/source.py
+++ b/scripts/qapi/source.py
@@ -11,7 +11,12 @@
import copy
import sys
-from typing import List, Optional, TypeVar
+from typing import (
+ List,
+ Optional,
+ Type,
+ TypeVar,
+)
class QAPISchemaPragma:
@@ -41,6 +46,17 @@ def __init__(self, fname: str, line: int,
self.defn_meta: Optional[str] = None
self.defn_name: Optional[str] = None
+ @classmethod
+ def builtin(cls: Type[T]) -> T:
+ """
+ Create an instance corresponding to a built-in definition.
+ """
+ return cls('', -1, None)
+
+ def __bool__(self) -> bool:
+ # "if info" is false if @info corresponds to a built-in definition.
+ return bool(self.fname)
+
def set_defn(self, meta: str, name: str) -> None:
self.defn_meta = meta
self.defn_name = name
@@ -73,4 +89,6 @@ def include_path(self) -> str:
return ret
def __str__(self) -> str:
+ if not bool(self):
+ return '[builtin]'
return self.include_path() + self.in_defn() + self.loc()
--
2.26.2
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 06/12] qapi/source: Add builtin null-object sentinel
2020-12-17 1:59 ` [PATCH v2 06/12] qapi/source: Add builtin null-object sentinel John Snow
@ 2021-01-13 15:39 ` Markus Armbruster
2021-01-13 22:30 ` John Snow
0 siblings, 1 reply; 26+ messages in thread
From: Markus Armbruster @ 2021-01-13 15:39 UTC (permalink / raw)
To: John Snow
Cc: Michael Roth, Cleber Rosa, qemu-devel, Marc-André Lureau,
Eduardo Habkost
Spelling nitpick: s/builtin/built-in/ in the title.
John Snow <jsnow@redhat.com> writes:
> We use None to represent an object that has no source information
> because it's a builtin. This complicates interface typing, since many
> interfaces expect that there is an info object available to print errors
> with.
>
> Introduce a special QAPISourceInfo that represents these built-ins so
> that if an error should so happen to occur relating to one of these
> builtins that we will be able to print its information, and interface
> typing becomes simpler: you will always have a source info object.
>
> This object will evaluate as False, so "if info" remains a valid
> idiomatic construct.
>
> NB: It was intentional to not allow empty constructors or similar to
> create "empty" source info objects; callers must explicitly invoke
> 'builtin()' to pro-actively opt into using the sentinel. This should
> prevent use-by-accident.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
As I pointed out in review of v1, this patch has two aspects mixed up:
1. Represent "no source info" as special QAPISourceInfo instead of
None
2. On error with "no source info", don't crash.
The first one is what de-complicates interface typing. It's clearly
serving this patch series' stated purpose: "static typing conversion".
The second one is not. It sidetracks us into a design discussion that
isn't related to static typing. Maybe it's something we should discuss.
Maybe the discussion will make us conclude we want to do this. But
letting the static typing work get delayed by that discussion would be
stupid, and I'll do what I can to prevent that.
The stupidest possible solution that preserves the crash is adding an
assertion right where it crashes before this patch: in
QAPISourceInfo.__str__(). Yes, crashing in a __str__() method is not
nice, but it's no worse than before. Making it better than before is a
good idea, and you're quite welcome to try, but please not in this
series. Add a TODO comment asking for "make it better", then sit on
your hands.
The pipeline must move.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 06/12] qapi/source: Add builtin null-object sentinel
2021-01-13 15:39 ` Markus Armbruster
@ 2021-01-13 22:30 ` John Snow
2021-01-14 13:39 ` Markus Armbruster
0 siblings, 1 reply; 26+ messages in thread
From: John Snow @ 2021-01-13 22:30 UTC (permalink / raw)
To: Markus Armbruster
Cc: Michael Roth, Cleber Rosa, qemu-devel, Marc-André Lureau,
Eduardo Habkost
On 1/13/21 10:39 AM, Markus Armbruster wrote:
> Spelling nitpick: s/builtin/built-in/ in the title.
>
Sure.
> John Snow <jsnow@redhat.com> writes:
>
>> We use None to represent an object that has no source information
>> because it's a builtin. This complicates interface typing, since many
>> interfaces expect that there is an info object available to print errors
>> with.
>>
>> Introduce a special QAPISourceInfo that represents these built-ins so
>> that if an error should so happen to occur relating to one of these
>> builtins that we will be able to print its information, and interface
>> typing becomes simpler: you will always have a source info object.
>>
>> This object will evaluate as False, so "if info" remains a valid
>> idiomatic construct.
>>
>> NB: It was intentional to not allow empty constructors or similar to
>> create "empty" source info objects; callers must explicitly invoke
>> 'builtin()' to pro-actively opt into using the sentinel. This should
>> prevent use-by-accident.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>
> As I pointed out in review of v1, this patch has two aspects mixed up:
>
> 1. Represent "no source info" as special QAPISourceInfo instead of
> None
>
> 2. On error with "no source info", don't crash.
>
> The first one is what de-complicates interface typing. It's clearly
> serving this patch series' stated purpose: "static typing conversion".
>
> The second one is not. It sidetracks us into a design discussion that
> isn't related to static typing. Maybe it's something we should discuss.
> Maybe the discussion will make us conclude we want to do this. But
> letting the static typing work get delayed by that discussion would be
> stupid, and I'll do what I can to prevent that.
>
It's not unrelated. It's about finding the most tactical incision to
make the types as we actually use them correct from a static analysis
context.
Maybe there's another tactical incision to make that's "smaller", for
some perception of "smaller", but it's not unrelated.
> The stupidest possible solution that preserves the crash is adding an
> assertion right where it crashes before this patch: in
> QAPISourceInfo.__str__(). Yes, crashing in a __str__() method is not
> nice, but it's no worse than before. Making it better than before is a
> good idea, and you're quite welcome to try, but please not in this
> series. Add a TODO comment asking for "make it better", then sit on
> your hands.
I'm recently back from a fairly long PTO, so forgive me if I am
forgetting something, but I am not really sure I fundamentally
understand the nature of this critique.
Making functions not "crash" is a side-effect of making the types
correct. I don't see it as scope-creep, it's a solution to a problem
under active consideration.
In my reply to your earlier critique, I (think) I mentioned that I
didn't understand the difference between:
1. An exception handler itself crashes because it received a value of
unexpected type, or
2. The exception handler printed a message that indicates a problem with
a built-in source definition.
In either case, QAPI didn't get built and it printed some kind of error
spaghetti to the screen. In both cases, something much more seriously
wrong has happened and the error message likely does not prepare the
human user to really genuinely understand what that seriously wrong
thing is.
I think this is an on-mission patch that improves circumstances; with
regards to matters of taste I would see it as a lateral move at worst
(one weird error for another weird error).
I'm left a little confused by the pushback, so I don't feel equipped to
try and write code that addresses it.
Let's chat on IRC?
--js
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 06/12] qapi/source: Add builtin null-object sentinel
2021-01-13 22:30 ` John Snow
@ 2021-01-14 13:39 ` Markus Armbruster
2021-01-18 18:36 ` Eduardo Habkost
0 siblings, 1 reply; 26+ messages in thread
From: Markus Armbruster @ 2021-01-14 13:39 UTC (permalink / raw)
To: John Snow
Cc: Michael Roth, Eduardo Habkost, qemu-devel,
Marc-André Lureau, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> On 1/13/21 10:39 AM, Markus Armbruster wrote:
>> Spelling nitpick: s/builtin/built-in/ in the title.
>>
>
> Sure.
>
>> John Snow <jsnow@redhat.com> writes:
>>
>>> We use None to represent an object that has no source information
>>> because it's a builtin. This complicates interface typing, since many
>>> interfaces expect that there is an info object available to print errors
>>> with.
>>>
>>> Introduce a special QAPISourceInfo that represents these built-ins so
>>> that if an error should so happen to occur relating to one of these
>>> builtins that we will be able to print its information, and interface
>>> typing becomes simpler: you will always have a source info object.
>>>
>>> This object will evaluate as False, so "if info" remains a valid
>>> idiomatic construct.
>>>
>>> NB: It was intentional to not allow empty constructors or similar to
>>> create "empty" source info objects; callers must explicitly invoke
>>> 'builtin()' to pro-actively opt into using the sentinel. This should
>>> prevent use-by-accident.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> As I pointed out in review of v1, this patch has two aspects mixed up:
>>
>> 1. Represent "no source info" as special QAPISourceInfo instead of
>> None
>>
>> 2. On error with "no source info", don't crash.
>>
>> The first one is what de-complicates interface typing. It's clearly
>> serving this patch series' stated purpose: "static typing conversion".
>>
>> The second one is not. It sidetracks us into a design discussion that
>> isn't related to static typing. Maybe it's something we should discuss.
>> Maybe the discussion will make us conclude we want to do this. But
>> letting the static typing work get delayed by that discussion would be
>> stupid, and I'll do what I can to prevent that.
>>
>
> It's not unrelated. It's about finding the most tactical incision to
> make the types as we actually use them correct from a static analysis
> context.
>
> Maybe there's another tactical incision to make that's "smaller", for
> some perception of "smaller", but it's not unrelated.
We don't have to debate, let alone agree on relatedness.
>> The stupidest possible solution that preserves the crash is adding an
>> assertion right where it crashes before this patch: in
>> QAPISourceInfo.__str__(). Yes, crashing in a __str__() method is not
>> nice, but it's no worse than before. Making it better than before is a
>> good idea, and you're quite welcome to try, but please not in this
>> series. Add a TODO comment asking for "make it better", then sit on
>> your hands.
>
> I'm recently back from a fairly long PTO, so forgive me if I am
> forgetting something, but I am not really sure I fundamentally
> understand the nature of this critique.
>
> Making functions not "crash" is a side-effect of making the types
> correct. I don't see it as scope-creep, it's a solution to a problem
> under active consideration.
I disagree.
The crash you "fix" is *intentional*. I was too lazy to write something
like
assert self.info
and instead relied in self.info.whatever to crash. I don't care how it
crashes, as long as it does crash.
I *like* qapi-gen to crash on such internal errors. It's easy, and
makes "this is a bug, go report it" perfectly clear.
I'd also be fine with reporting "internal error, this is a bug, go
report it". Not in this series, unless it's utterly trivial, which I
doubt.
I'm *not* fine with feeding made-up info objects to the user error
reporting machinery without proof that it'll actually produce a useful
error message. Definitely not trivial, thus not in this series.
> In my reply to your earlier critique, I (think) I mentioned that I
> didn't understand the difference between:
>
> 1. An exception handler itself crashes because it received a value of
> unexpected type, or
>
> 2. The exception handler printed a message that indicates a problem with
> a built-in source definition.
>
> In either case, QAPI didn't get built and it printed some kind of error
> spaghetti to the screen. In both cases, something much more seriously
> wrong has happened and the error message likely does not prepare the
> human user to really genuinely understand what that seriously wrong
> thing is.
>
> I think this is an on-mission patch that improves circumstances; with
> regards to matters of taste I would see it as a lateral move at worst
> (one weird error for another weird error).
>
> I'm left a little confused by the pushback, so I don't feel equipped to
> try and write code that addresses it.
>
> Let's chat on IRC?
Gladly. If we can make out work days intersect...
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 06/12] qapi/source: Add builtin null-object sentinel
2021-01-14 13:39 ` Markus Armbruster
@ 2021-01-18 18:36 ` Eduardo Habkost
2021-01-19 10:21 ` Markus Armbruster
0 siblings, 1 reply; 26+ messages in thread
From: Eduardo Habkost @ 2021-01-18 18:36 UTC (permalink / raw)
To: Markus Armbruster
Cc: Michael Roth, John Snow, qemu-devel, Marc-André Lureau, Cleber Rosa
On Thu, Jan 14, 2021 at 02:39:35PM +0100, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
> > On 1/13/21 10:39 AM, Markus Armbruster wrote:
> >> Spelling nitpick: s/builtin/built-in/ in the title.
> >>
> >
> > Sure.
> >
> >> John Snow <jsnow@redhat.com> writes:
> >>
> >>> We use None to represent an object that has no source information
> >>> because it's a builtin. This complicates interface typing, since many
> >>> interfaces expect that there is an info object available to print errors
> >>> with.
> >>>
> >>> Introduce a special QAPISourceInfo that represents these built-ins so
> >>> that if an error should so happen to occur relating to one of these
> >>> builtins that we will be able to print its information, and interface
> >>> typing becomes simpler: you will always have a source info object.
> >>>
> >>> This object will evaluate as False, so "if info" remains a valid
> >>> idiomatic construct.
> >>>
> >>> NB: It was intentional to not allow empty constructors or similar to
> >>> create "empty" source info objects; callers must explicitly invoke
> >>> 'builtin()' to pro-actively opt into using the sentinel. This should
> >>> prevent use-by-accident.
> >>>
> >>> Signed-off-by: John Snow <jsnow@redhat.com>
> >>
> >> As I pointed out in review of v1, this patch has two aspects mixed up:
> >>
> >> 1. Represent "no source info" as special QAPISourceInfo instead of
> >> None
> >>
> >> 2. On error with "no source info", don't crash.
> >>
> >> The first one is what de-complicates interface typing. It's clearly
> >> serving this patch series' stated purpose: "static typing conversion".
> >>
> >> The second one is not. It sidetracks us into a design discussion that
> >> isn't related to static typing. Maybe it's something we should discuss.
> >> Maybe the discussion will make us conclude we want to do this. But
> >> letting the static typing work get delayed by that discussion would be
> >> stupid, and I'll do what I can to prevent that.
> >>
> >
> > It's not unrelated. It's about finding the most tactical incision to
> > make the types as we actually use them correct from a static analysis
> > context.
> >
> > Maybe there's another tactical incision to make that's "smaller", for
> > some perception of "smaller", but it's not unrelated.
>
> We don't have to debate, let alone agree on relatedness.
>
> >> The stupidest possible solution that preserves the crash is adding an
> >> assertion right where it crashes before this patch: in
> >> QAPISourceInfo.__str__(). Yes, crashing in a __str__() method is not
> >> nice, but it's no worse than before. Making it better than before is a
> >> good idea, and you're quite welcome to try, but please not in this
> >> series. Add a TODO comment asking for "make it better", then sit on
> >> your hands.
> >
> > I'm recently back from a fairly long PTO, so forgive me if I am
> > forgetting something, but I am not really sure I fundamentally
> > understand the nature of this critique.
> >
> > Making functions not "crash" is a side-effect of making the types
> > correct. I don't see it as scope-creep, it's a solution to a problem
> > under active consideration.
>
> I disagree.
>
> The crash you "fix" is *intentional*. I was too lazy to write something
> like
>
> assert self.info
>
> and instead relied in self.info.whatever to crash. I don't care how it
> crashes, as long as it does crash.
>
> I *like* qapi-gen to crash on such internal errors. It's easy, and
> makes "this is a bug, go report it" perfectly clear.
>
> I'd also be fine with reporting "internal error, this is a bug, go
> report it". Not in this series, unless it's utterly trivial, which I
> doubt.
>
> I'm *not* fine with feeding made-up info objects to the user error
> reporting machinery without proof that it'll actually produce a useful
> error message. Definitely not trivial, thus not in this series.
If you really don't want to change the existing behavior of the
code, I believe we have only two options:
1) Annotate self.info as QAPISourceInfo (not Optional),
and add a hack to make the expression `self.info` crash if the
argument to __init__() was None.
2) Annotate self.info as Optional[QAPISourceInfo], and adding
manual asserts everywhere self.info is used.
Which of those two options do you find acceptable, Markus?
--
Eduardo
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 06/12] qapi/source: Add builtin null-object sentinel
2021-01-18 18:36 ` Eduardo Habkost
@ 2021-01-19 10:21 ` Markus Armbruster
2021-01-19 16:10 ` Eduardo Habkost
0 siblings, 1 reply; 26+ messages in thread
From: Markus Armbruster @ 2021-01-19 10:21 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Michael Roth, Cleber Rosa, John Snow, qemu-devel, Marc-André Lureau
Eduardo Habkost <ehabkost@redhat.com> writes:
> On Thu, Jan 14, 2021 at 02:39:35PM +0100, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>> > On 1/13/21 10:39 AM, Markus Armbruster wrote:
>> >> Spelling nitpick: s/builtin/built-in/ in the title.
>> >>
>> >
>> > Sure.
>> >
>> >> John Snow <jsnow@redhat.com> writes:
>> >>
>> >>> We use None to represent an object that has no source information
>> >>> because it's a builtin. This complicates interface typing, since many
>> >>> interfaces expect that there is an info object available to print errors
>> >>> with.
>> >>>
>> >>> Introduce a special QAPISourceInfo that represents these built-ins so
>> >>> that if an error should so happen to occur relating to one of these
>> >>> builtins that we will be able to print its information, and interface
>> >>> typing becomes simpler: you will always have a source info object.
>> >>>
>> >>> This object will evaluate as False, so "if info" remains a valid
>> >>> idiomatic construct.
>> >>>
>> >>> NB: It was intentional to not allow empty constructors or similar to
>> >>> create "empty" source info objects; callers must explicitly invoke
>> >>> 'builtin()' to pro-actively opt into using the sentinel. This should
>> >>> prevent use-by-accident.
>> >>>
>> >>> Signed-off-by: John Snow <jsnow@redhat.com>
>> >>
>> >> As I pointed out in review of v1, this patch has two aspects mixed up:
>> >>
>> >> 1. Represent "no source info" as special QAPISourceInfo instead of
>> >> None
>> >>
>> >> 2. On error with "no source info", don't crash.
>> >>
>> >> The first one is what de-complicates interface typing. It's clearly
>> >> serving this patch series' stated purpose: "static typing conversion".
>> >>
>> >> The second one is not. It sidetracks us into a design discussion that
>> >> isn't related to static typing. Maybe it's something we should discuss.
>> >> Maybe the discussion will make us conclude we want to do this. But
>> >> letting the static typing work get delayed by that discussion would be
>> >> stupid, and I'll do what I can to prevent that.
>> >>
>> >
>> > It's not unrelated. It's about finding the most tactical incision to
>> > make the types as we actually use them correct from a static analysis
>> > context.
>> >
>> > Maybe there's another tactical incision to make that's "smaller", for
>> > some perception of "smaller", but it's not unrelated.
>>
>> We don't have to debate, let alone agree on relatedness.
>>
>> >> The stupidest possible solution that preserves the crash is adding an
>> >> assertion right where it crashes before this patch: in
>> >> QAPISourceInfo.__str__(). Yes, crashing in a __str__() method is not
>> >> nice, but it's no worse than before. Making it better than before is a
>> >> good idea, and you're quite welcome to try, but please not in this
>> >> series. Add a TODO comment asking for "make it better", then sit on
>> >> your hands.
>> >
>> > I'm recently back from a fairly long PTO, so forgive me if I am
>> > forgetting something, but I am not really sure I fundamentally
>> > understand the nature of this critique.
>> >
>> > Making functions not "crash" is a side-effect of making the types
>> > correct. I don't see it as scope-creep, it's a solution to a problem
>> > under active consideration.
>>
>> I disagree.
>>
>> The crash you "fix" is *intentional*. I was too lazy to write something
>> like
>>
>> assert self.info
>>
>> and instead relied in self.info.whatever to crash. I don't care how it
>> crashes, as long as it does crash.
>>
>> I *like* qapi-gen to crash on such internal errors. It's easy, and
>> makes "this is a bug, go report it" perfectly clear.
>>
>> I'd also be fine with reporting "internal error, this is a bug, go
>> report it". Not in this series, unless it's utterly trivial, which I
>> doubt.
>>
>> I'm *not* fine with feeding made-up info objects to the user error
>> reporting machinery without proof that it'll actually produce a useful
>> error message. Definitely not trivial, thus not in this series.
>
> If you really don't want to change the existing behavior of the
> code, I believe we have only two options:
>
> 1) Annotate self.info as QAPISourceInfo (not Optional),
> and add a hack to make the expression `self.info` crash if the
> argument to __init__() was None.
I figure you mean
* Represent "no info" as a special QAPISourceInfo (instead of None), so
we can annotate .info as QAPISourceInfo (not Optional).
* When we report a QAPIError, assert .info is not this special value.
This preserves the existing (and intentional) behavior: we crash when we
dot into QAPISourceInfo, and we do that only when we report a QAPIError
with that info.
The only change in behavior is AssertionError instead of AttributeError.
Minor improvement.
We could replace the AssertionError crash by a fatal error with suitably
worded error message. I'd prefer not to, because I'd rather keep the
stack backtrace. Admittedly not something I'd fight for.
> 2) Annotate self.info as Optional[QAPISourceInfo], and adding
> manual asserts everywhere self.info is used.
>
> Which of those two options do you find acceptable, Markus?
I think John prefers (1), because the typing gets simpler. I'm inclined
to leave the decision to him.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 06/12] qapi/source: Add builtin null-object sentinel
2021-01-19 10:21 ` Markus Armbruster
@ 2021-01-19 16:10 ` Eduardo Habkost
0 siblings, 0 replies; 26+ messages in thread
From: Eduardo Habkost @ 2021-01-19 16:10 UTC (permalink / raw)
To: Markus Armbruster
Cc: Michael Roth, Cleber Rosa, John Snow, qemu-devel, Marc-André Lureau
On Tue, Jan 19, 2021 at 11:21:16AM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
>
> > On Thu, Jan 14, 2021 at 02:39:35PM +0100, Markus Armbruster wrote:
> >> John Snow <jsnow@redhat.com> writes:
> >>
> >> > On 1/13/21 10:39 AM, Markus Armbruster wrote:
> >> >> Spelling nitpick: s/builtin/built-in/ in the title.
> >> >>
> >> >
> >> > Sure.
> >> >
> >> >> John Snow <jsnow@redhat.com> writes:
> >> >>
> >> >>> We use None to represent an object that has no source information
> >> >>> because it's a builtin. This complicates interface typing, since many
> >> >>> interfaces expect that there is an info object available to print errors
> >> >>> with.
> >> >>>
> >> >>> Introduce a special QAPISourceInfo that represents these built-ins so
> >> >>> that if an error should so happen to occur relating to one of these
> >> >>> builtins that we will be able to print its information, and interface
> >> >>> typing becomes simpler: you will always have a source info object.
> >> >>>
> >> >>> This object will evaluate as False, so "if info" remains a valid
> >> >>> idiomatic construct.
> >> >>>
> >> >>> NB: It was intentional to not allow empty constructors or similar to
> >> >>> create "empty" source info objects; callers must explicitly invoke
> >> >>> 'builtin()' to pro-actively opt into using the sentinel. This should
> >> >>> prevent use-by-accident.
> >> >>>
> >> >>> Signed-off-by: John Snow <jsnow@redhat.com>
> >> >>
> >> >> As I pointed out in review of v1, this patch has two aspects mixed up:
> >> >>
> >> >> 1. Represent "no source info" as special QAPISourceInfo instead of
> >> >> None
> >> >>
> >> >> 2. On error with "no source info", don't crash.
> >> >>
> >> >> The first one is what de-complicates interface typing. It's clearly
> >> >> serving this patch series' stated purpose: "static typing conversion".
> >> >>
> >> >> The second one is not. It sidetracks us into a design discussion that
> >> >> isn't related to static typing. Maybe it's something we should discuss.
> >> >> Maybe the discussion will make us conclude we want to do this. But
> >> >> letting the static typing work get delayed by that discussion would be
> >> >> stupid, and I'll do what I can to prevent that.
> >> >>
> >> >
> >> > It's not unrelated. It's about finding the most tactical incision to
> >> > make the types as we actually use them correct from a static analysis
> >> > context.
> >> >
> >> > Maybe there's another tactical incision to make that's "smaller", for
> >> > some perception of "smaller", but it's not unrelated.
> >>
> >> We don't have to debate, let alone agree on relatedness.
> >>
> >> >> The stupidest possible solution that preserves the crash is adding an
> >> >> assertion right where it crashes before this patch: in
> >> >> QAPISourceInfo.__str__(). Yes, crashing in a __str__() method is not
> >> >> nice, but it's no worse than before. Making it better than before is a
> >> >> good idea, and you're quite welcome to try, but please not in this
> >> >> series. Add a TODO comment asking for "make it better", then sit on
> >> >> your hands.
> >> >
> >> > I'm recently back from a fairly long PTO, so forgive me if I am
> >> > forgetting something, but I am not really sure I fundamentally
> >> > understand the nature of this critique.
> >> >
> >> > Making functions not "crash" is a side-effect of making the types
> >> > correct. I don't see it as scope-creep, it's a solution to a problem
> >> > under active consideration.
> >>
> >> I disagree.
> >>
> >> The crash you "fix" is *intentional*. I was too lazy to write something
> >> like
> >>
> >> assert self.info
> >>
> >> and instead relied in self.info.whatever to crash. I don't care how it
> >> crashes, as long as it does crash.
> >>
> >> I *like* qapi-gen to crash on such internal errors. It's easy, and
> >> makes "this is a bug, go report it" perfectly clear.
> >>
> >> I'd also be fine with reporting "internal error, this is a bug, go
> >> report it". Not in this series, unless it's utterly trivial, which I
> >> doubt.
> >>
> >> I'm *not* fine with feeding made-up info objects to the user error
> >> reporting machinery without proof that it'll actually produce a useful
> >> error message. Definitely not trivial, thus not in this series.
> >
> > If you really don't want to change the existing behavior of the
> > code, I believe we have only two options:
> >
> > 1) Annotate self.info as QAPISourceInfo (not Optional),
> > and add a hack to make the expression `self.info` crash if the
> > argument to __init__() was None.
>
> I figure you mean
>
> * Represent "no info" as a special QAPISourceInfo (instead of None), so
> we can annotate .info as QAPISourceInfo (not Optional).
>
> * When we report a QAPIError, assert .info is not this special value.
Not necessarily. Creating a special QAPISourceInfo would be one
solution to let us annotate self.info as non-Optional, but not
the only one.
Possibly the simplest way to declare self.info as non-Optional is
to make it a property that hides an Optional attribute. e.g.:
class ...:
_info: Optional[QAPISourceInfo]
@property
def info(self) -> QAPISourceInfo:
assert self._info is not None
return self._info
>
> This preserves the existing (and intentional) behavior: we crash when we
> dot into QAPISourceInfo, and we do that only when we report a QAPIError
> with that info.
I'm not sure about the "only when we report a QAPIError" part.
We seem to have multiple places in the code where self.info is
assumed to never be None, and I'm not sure all of them involve
QAPIError.
>
> The only change in behavior is AssertionError instead of AttributeError.
> Minor improvement.
>
> We could replace the AssertionError crash by a fatal error with suitably
> worded error message. I'd prefer not to, because I'd rather keep the
> stack backtrace. Admittedly not something I'd fight for.
>
> > 2) Annotate self.info as Optional[QAPISourceInfo], and adding
> > manual asserts everywhere self.info is used.
> >
> > Which of those two options do you find acceptable, Markus?
>
> I think John prefers (1), because the typing gets simpler. I'm inclined
> to leave the decision to him.
--
Eduardo
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 07/12] qapi/schema: make QAPISourceInfo mandatory
2020-12-17 1:59 [PATCH v2 00/12] qapi: static typing conversion, pt1.5 John Snow
` (5 preceding siblings ...)
2020-12-17 1:59 ` [PATCH v2 06/12] qapi/source: Add builtin null-object sentinel John Snow
@ 2020-12-17 1:59 ` John Snow
2021-01-13 16:12 ` Markus Armbruster
2020-12-17 1:59 ` [PATCH v2 08/12] qapi/gen: write _genc/_genh access shims John Snow
` (5 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: John Snow @ 2020-12-17 1:59 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Michael Roth, Markus Armbruster, Cleber Rosa,
Marc-André Lureau, John Snow
Signed-off-by: John Snow <jsnow@redhat.com>
---
The event_enum_members change might become irrelevant after a
forthcoming (?) patch by Markus, but didn't have it in-hand at time of
publishing.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/events.py | 2 +-
scripts/qapi/schema.py | 25 ++++++++++++++-----------
scripts/qapi/types.py | 9 +++++----
scripts/qapi/visit.py | 6 +++---
4 files changed, 23 insertions(+), 19 deletions(-)
diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index 9851653b9d1..9ba4f109028 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -225,7 +225,7 @@ def visit_event(self,
self._event_emit_name))
# Note: we generate the enum member regardless of @ifcond, to
# keep the enumeration usable in target-independent code.
- self._event_enum_members.append(QAPISchemaEnumMember(name, None))
+ self._event_enum_members.append(QAPISchemaEnumMember(name, info))
def gen_events(schema: QAPISchema,
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 720449feee4..0449771dfe5 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -23,6 +23,7 @@
from .error import QAPIError, QAPISemError
from .expr import check_exprs
from .parser import QAPISchemaParser
+from .source import QAPISourceInfo
class QAPISchemaEntity:
@@ -36,10 +37,10 @@ def __init__(self, name, info, doc, ifcond=None, features=None):
self.name = name
self._module = None
# For explicitly defined entities, info points to the (explicit)
- # definition. For builtins (and their arrays), info is None.
- # For implicitly defined entities, info points to a place that
- # triggered the implicit definition (there may be more than one
- # such place).
+ # definition. For built-in types (and their arrays), info is a
+ # special object that evaluates to False. For implicitly defined
+ # entities, info points to a place that triggered the implicit
+ # definition (there may be more than one such place).
self.info = info
self.doc = doc
self._ifcond = ifcond or []
@@ -68,7 +69,7 @@ def check_doc(self):
def _set_module(self, schema, info):
assert self._checked
- self._module = schema.module_by_fname(info and info.fname)
+ self._module = schema.module_by_fname(info.fname if info else None)
self._module.add_entity(self)
def set_module(self, schema):
@@ -209,7 +210,7 @@ class QAPISchemaBuiltinType(QAPISchemaType):
meta = 'built-in'
def __init__(self, name, json_type, c_type):
- super().__init__(name, None, None)
+ super().__init__(name, QAPISourceInfo.builtin(), None)
assert not c_type or isinstance(c_type, str)
assert json_type in ('string', 'number', 'int', 'boolean', 'null',
'value')
@@ -897,7 +898,7 @@ def _def_builtin_type(self, name, json_type, c_type):
# be nice, but we can't as long as their generated code
# (qapi-builtin-types.[ch]) may be shared by some other
# schema.
- self._make_array_type(name, None)
+ self._make_array_type(name, QAPISourceInfo.builtin())
def _def_predefineds(self):
for t in [('str', 'string', 'char' + POINTER_SUFFIX),
@@ -917,16 +918,18 @@ def _def_predefineds(self):
('null', 'null', 'QNull' + POINTER_SUFFIX)]:
self._def_builtin_type(*t)
self.the_empty_object_type = QAPISchemaObjectType(
- 'q_empty', None, None, None, None, None, [], None)
+ 'q_empty', QAPISourceInfo.builtin(),
+ None, None, None, None, [], None)
self._def_entity(self.the_empty_object_type)
qtypes = ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist',
'qbool']
qtype_values = self._make_enum_members(
- [{'name': n} for n in qtypes], None)
+ [{'name': n} for n in qtypes], QAPISourceInfo.builtin())
- self._def_entity(QAPISchemaEnumType('QType', None, None, None, None,
- qtype_values, 'QTYPE'))
+ self._def_entity(QAPISchemaEnumType(
+ 'QType', QAPISourceInfo.builtin(), None,
+ None, None, qtype_values, 'QTYPE'))
def _make_features(self, features, info):
if features is None:
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index 2b4916cdaa1..a3a16284006 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -71,7 +71,8 @@ def gen_enum(name: str,
members: List[QAPISchemaEnumMember],
prefix: Optional[str] = None) -> str:
# append automatically generated _MAX value
- enum_members = members + [QAPISchemaEnumMember('_MAX', None)]
+ enum_members = members + [
+ QAPISchemaEnumMember('_MAX', QAPISourceInfo.builtin())]
ret = mcgen('''
@@ -306,7 +307,7 @@ def _gen_type_cleanup(self, name: str) -> None:
def visit_enum_type(self,
name: str,
- info: Optional[QAPISourceInfo],
+ info: QAPISourceInfo,
ifcond: List[str],
features: List[QAPISchemaFeature],
members: List[QAPISchemaEnumMember],
@@ -317,7 +318,7 @@ def visit_enum_type(self,
def visit_array_type(self,
name: str,
- info: Optional[QAPISourceInfo],
+ info: QAPISourceInfo,
ifcond: List[str],
element_type: QAPISchemaType) -> None:
with ifcontext(ifcond, self._genh, self._genc):
@@ -327,7 +328,7 @@ def visit_array_type(self,
def visit_object_type(self,
name: str,
- info: Optional[QAPISourceInfo],
+ info: QAPISourceInfo,
ifcond: List[str],
features: List[QAPISchemaFeature],
base: Optional[QAPISchemaObjectType],
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 339f1521524..3f49c307c56 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -336,7 +336,7 @@ def _begin_user_module(self, name: str) -> None:
def visit_enum_type(self,
name: str,
- info: QAPISourceInfo,
+ info: Optional[QAPISourceInfo],
ifcond: List[str],
features: List[QAPISchemaFeature],
members: List[QAPISchemaEnumMember],
@@ -347,7 +347,7 @@ def visit_enum_type(self,
def visit_array_type(self,
name: str,
- info: Optional[QAPISourceInfo],
+ info: QAPISourceInfo,
ifcond: List[str],
element_type: QAPISchemaType) -> None:
with ifcontext(ifcond, self._genh, self._genc):
@@ -356,7 +356,7 @@ def visit_array_type(self,
def visit_object_type(self,
name: str,
- info: Optional[QAPISourceInfo],
+ info: QAPISourceInfo,
ifcond: List[str],
features: List[QAPISchemaFeature],
base: Optional[QAPISchemaObjectType],
--
2.26.2
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 07/12] qapi/schema: make QAPISourceInfo mandatory
2020-12-17 1:59 ` [PATCH v2 07/12] qapi/schema: make QAPISourceInfo mandatory John Snow
@ 2021-01-13 16:12 ` Markus Armbruster
2021-01-13 23:04 ` John Snow
0 siblings, 1 reply; 26+ messages in thread
From: Markus Armbruster @ 2021-01-13 16:12 UTC (permalink / raw)
To: John Snow
Cc: Michael Roth, Cleber Rosa, qemu-devel, Marc-André Lureau,
Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> Signed-off-by: John Snow <jsnow@redhat.com>
>
> ---
>
> The event_enum_members change might become irrelevant after a
> forthcoming (?) patch by Markus, but didn't have it in-hand at time of
> publishing.
It's in my "[PATCH 00/11] Drop support for QAPIGen without a file name",
which includes parts of your v1. The parts that are new should be
injected into your series so they replace your "[PATCH v2 09/12]
qapi/gen: move write method to QAPIGenC, make fname a str". Holler if
you need help.
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/events.py | 2 +-
> scripts/qapi/schema.py | 25 ++++++++++++++-----------
> scripts/qapi/types.py | 9 +++++----
> scripts/qapi/visit.py | 6 +++---
> 4 files changed, 23 insertions(+), 19 deletions(-)
>
> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
> index 9851653b9d1..9ba4f109028 100644
> --- a/scripts/qapi/events.py
> +++ b/scripts/qapi/events.py
> @@ -225,7 +225,7 @@ def visit_event(self,
> self._event_emit_name))
> # Note: we generate the enum member regardless of @ifcond, to
> # keep the enumeration usable in target-independent code.
> - self._event_enum_members.append(QAPISchemaEnumMember(name, None))
> + self._event_enum_members.append(QAPISchemaEnumMember(name, info))
This "enum" is not supposed to be erroneous. If it is, it's a bug.
Your patch changes how the code behaves should such a bug bite here.
Before, we crash. Afterwards, we report the bug using @info, which I'd
expect to produce utterly confusing error messages.
My comments on PATCH 06 apply: how the code should behave here is a
design issue that should be kept out of this patch series.
If you need to pass a value other than None to help with static typing,
then pass a suitable poison info that will crash right where None
crashes now.
> def gen_events(schema: QAPISchema,
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 720449feee4..0449771dfe5 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -23,6 +23,7 @@
> from .error import QAPIError, QAPISemError
> from .expr import check_exprs
> from .parser import QAPISchemaParser
> +from .source import QAPISourceInfo
>
>
> class QAPISchemaEntity:
> @@ -36,10 +37,10 @@ def __init__(self, name, info, doc, ifcond=None, features=None):
> self.name = name
> self._module = None
> # For explicitly defined entities, info points to the (explicit)
> - # definition. For builtins (and their arrays), info is None.
> - # For implicitly defined entities, info points to a place that
> - # triggered the implicit definition (there may be more than one
> - # such place).
> + # definition. For built-in types (and their arrays), info is a
> + # special object that evaluates to False. For implicitly defined
> + # entities, info points to a place that triggered the implicit
> + # definition (there may be more than one such place).
> self.info = info
> self.doc = doc
> self._ifcond = ifcond or []
> @@ -68,7 +69,7 @@ def check_doc(self):
>
> def _set_module(self, schema, info):
> assert self._checked
> - self._module = schema.module_by_fname(info and info.fname)
> + self._module = schema.module_by_fname(info.fname if info else None)
Looks unrelated.
> self._module.add_entity(self)
>
> def set_module(self, schema):
[...]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 07/12] qapi/schema: make QAPISourceInfo mandatory
2021-01-13 16:12 ` Markus Armbruster
@ 2021-01-13 23:04 ` John Snow
2021-01-14 0:29 ` Eduardo Habkost
0 siblings, 1 reply; 26+ messages in thread
From: John Snow @ 2021-01-13 23:04 UTC (permalink / raw)
To: Markus Armbruster
Cc: Michael Roth, Cleber Rosa, qemu-devel, Marc-André Lureau,
Eduardo Habkost
On 1/13/21 11:12 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> ---
>>
>> The event_enum_members change might become irrelevant after a
>> forthcoming (?) patch by Markus, but didn't have it in-hand at time of
>> publishing.
>
> It's in my "[PATCH 00/11] Drop support for QAPIGen without a file name",
> which includes parts of your v1. The parts that are new should be
> injected into your series so they replace your "[PATCH v2 09/12]
> qapi/gen: move write method to QAPIGenC, make fname a str". Holler if
> you need help.
>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qapi/events.py | 2 +-
>> scripts/qapi/schema.py | 25 ++++++++++++++-----------
>> scripts/qapi/types.py | 9 +++++----
>> scripts/qapi/visit.py | 6 +++---
>> 4 files changed, 23 insertions(+), 19 deletions(-)
>>
>> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
>> index 9851653b9d1..9ba4f109028 100644
>> --- a/scripts/qapi/events.py
>> +++ b/scripts/qapi/events.py
>> @@ -225,7 +225,7 @@ def visit_event(self,
>> self._event_emit_name))
>> # Note: we generate the enum member regardless of @ifcond, to
>> # keep the enumeration usable in target-independent code.
>> - self._event_enum_members.append(QAPISchemaEnumMember(name, None))
>> + self._event_enum_members.append(QAPISchemaEnumMember(name, info))
>
> This "enum" is not supposed to be erroneous. If it is, it's a bug.
>
> Your patch changes how the code behaves should such a bug bite here.
> Before, we crash. Afterwards, we report the bug using @info, which I'd
> expect to produce utterly confusing error messages.
>
It doesn't change the behavior *here*, though. It changes it whenever
this info object is used in another context. ... and who knows when or
where or why it is being used, or by whom.
I'll have to play with this. I'm not sure there's any way to coax a bug
to happen here that I am aware of right away. Can you think of how to
will one into existence?
> My comments on PATCH 06 apply: how the code should behave here is a
> design issue that should be kept out of this patch series.
>
> If you need to pass a value other than None to help with static typing,
> then pass a suitable poison info that will crash right where None
> crashes now.
>
I think we need to, yes; or we probably really, really want to. Making
the info parameter optional really adds a lot of unidiomatic
type-checking confetti when we go to use info, and in many more contexts
than just this sorta-built-in-enum; it will creep badly.
So, I gotta pass ...something here. but what? You want poison, but I
think it's not right to fundamentally poison all built-ins.
Mmmmmmm. Maybe per-instance poison can be done? We actually share info
objects, but I can make poisoned copies. Using next_line() as a basis:
def poison(self: T) -> T:
info = copy.copy(self)
info.poisoned = True
return info
probably almost anything I do is not going to make a lot of sense unless
I can actually replicate and test the different error scenarios to prove
that we didn't make the error spaghetti unknowably worse. I see it as
functionally inevitable that I have to audit this and make sure we get
good error messages anyway, so ... maybe I just ought to do that now anyway.
>> def gen_events(schema: QAPISchema,
>> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>> index 720449feee4..0449771dfe5 100644
>> --- a/scripts/qapi/schema.py
>> +++ b/scripts/qapi/schema.py
>> @@ -23,6 +23,7 @@
>> from .error import QAPIError, QAPISemError
>> from .expr import check_exprs
>> from .parser import QAPISchemaParser
>> +from .source import QAPISourceInfo
>>
>>
>> class QAPISchemaEntity:
>> @@ -36,10 +37,10 @@ def __init__(self, name, info, doc, ifcond=None, features=None):
>> self.name = name
>> self._module = None
>> # For explicitly defined entities, info points to the (explicit)
>> - # definition. For builtins (and their arrays), info is None.
>> - # For implicitly defined entities, info points to a place that
>> - # triggered the implicit definition (there may be more than one
>> - # such place).
>> + # definition. For built-in types (and their arrays), info is a
>> + # special object that evaluates to False. For implicitly defined
>> + # entities, info points to a place that triggered the implicit
>> + # definition (there may be more than one such place).
>> self.info = info
>> self.doc = doc
>> self._ifcond = ifcond or []
>> @@ -68,7 +69,7 @@ def check_doc(self):
>>
>> def _set_module(self, schema, info):
>> assert self._checked
>> - self._module = schema.module_by_fname(info and info.fname)
>> + self._module = schema.module_by_fname(info.fname if info else None)
>
> Looks unrelated.
>
Hmm, it sorta is. I have apparently edited this patch since I sent it,
but there was some tomfoolery over how "x and y" statements behave and
this edit was necessary at the time.
"info and info.fname" returned None when info could actually be None,
but when it was updated to be a special source object, we could
accidentally pass that special source object as the name -- instead of
None. Not good.
I think I re-ordered some patches such that I can just pass in
"info.fname" unconditionally instead as of this patch.
>> self._module.add_entity(self)
>>
>> def set_module(self, schema):
> [...]
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 07/12] qapi/schema: make QAPISourceInfo mandatory
2021-01-13 23:04 ` John Snow
@ 2021-01-14 0:29 ` Eduardo Habkost
2021-01-14 0:47 ` John Snow
0 siblings, 1 reply; 26+ messages in thread
From: Eduardo Habkost @ 2021-01-14 0:29 UTC (permalink / raw)
To: John Snow
Cc: Michael Roth, Cleber Rosa, Markus Armbruster,
Marc-André Lureau, qemu-devel
On Wed, Jan 13, 2021 at 06:04:29PM -0500, John Snow wrote:
> On 1/13/21 11:12 AM, Markus Armbruster wrote:
> > John Snow <jsnow@redhat.com> writes:
> >
> > > Signed-off-by: John Snow <jsnow@redhat.com>
> > >
> > > ---
> > >
> > > The event_enum_members change might become irrelevant after a
> > > forthcoming (?) patch by Markus, but didn't have it in-hand at time of
> > > publishing.
> >
> > It's in my "[PATCH 00/11] Drop support for QAPIGen without a file name",
> > which includes parts of your v1. The parts that are new should be
> > injected into your series so they replace your "[PATCH v2 09/12]
> > qapi/gen: move write method to QAPIGenC, make fname a str". Holler if
> > you need help.
> >
> > > Signed-off-by: John Snow <jsnow@redhat.com>
> > > ---
> > > scripts/qapi/events.py | 2 +-
> > > scripts/qapi/schema.py | 25 ++++++++++++++-----------
> > > scripts/qapi/types.py | 9 +++++----
> > > scripts/qapi/visit.py | 6 +++---
> > > 4 files changed, 23 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
> > > index 9851653b9d1..9ba4f109028 100644
> > > --- a/scripts/qapi/events.py
> > > +++ b/scripts/qapi/events.py
> > > @@ -225,7 +225,7 @@ def visit_event(self,
> > > self._event_emit_name))
> > > # Note: we generate the enum member regardless of @ifcond, to
> > > # keep the enumeration usable in target-independent code.
> > > - self._event_enum_members.append(QAPISchemaEnumMember(name, None))
> > > + self._event_enum_members.append(QAPISchemaEnumMember(name, info))
> >
> > This "enum" is not supposed to be erroneous. If it is, it's a bug.
> >
> > Your patch changes how the code behaves should such a bug bite here.
> > Before, we crash. Afterwards, we report the bug using @info, which I'd
> > expect to produce utterly confusing error messages.
> >
>
> It doesn't change the behavior *here*, though. It changes it whenever this
> info object is used in another context. ... and who knows when or where or
> why it is being used, or by whom.
>
> I'll have to play with this. I'm not sure there's any way to coax a bug to
> happen here that I am aware of right away. Can you think of how to will one
> into existence?
>
> > My comments on PATCH 06 apply: how the code should behave here is a
> > design issue that should be kept out of this patch series.
> >
> > If you need to pass a value other than None to help with static typing,
> > then pass a suitable poison info that will crash right where None
> > crashes now.
I don't understand what would be the point of inventing something
that behaves exactly like None but makes type checking less
useful.
With None/Optional, mypy gives us a way to be 100% sure the
object isn't going to invalid. With a poison value, mypy can't
tell us anymore if the code risks crashing at runtime.
> >
>
> I think we need to, yes; or we probably really, really want to. Making the
> info parameter optional really adds a lot of unidiomatic type-checking
> confetti when we go to use info, and in many more contexts than just this
> sorta-built-in-enum; it will creep badly.
Which methods would require unidiomatic type-checking because of
None/Optional?
>
> So, I gotta pass ...something here. but what? You want poison, but I think
> it's not right to fundamentally poison all built-ins.
>
> Mmmmmmm. Maybe per-instance poison can be done? We actually share info
> objects, but I can make poisoned copies. Using next_line() as a basis:
>
> def poison(self: T) -> T:
> info = copy.copy(self)
> info.poisoned = True
> return info
>
> probably almost anything I do is not going to make a lot of sense unless I
> can actually replicate and test the different error scenarios to prove that
> we didn't make the error spaghetti unknowably worse. I see it as
> functionally inevitable that I have to audit this and make sure we get good
> error messages anyway, so ... maybe I just ought to do that now anyway.
>
> > > def gen_events(schema: QAPISchema,
> > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > > index 720449feee4..0449771dfe5 100644
> > > --- a/scripts/qapi/schema.py
> > > +++ b/scripts/qapi/schema.py
> > > @@ -23,6 +23,7 @@
> > > from .error import QAPIError, QAPISemError
> > > from .expr import check_exprs
> > > from .parser import QAPISchemaParser
> > > +from .source import QAPISourceInfo
> > > class QAPISchemaEntity:
> > > @@ -36,10 +37,10 @@ def __init__(self, name, info, doc, ifcond=None, features=None):
> > > self.name = name
> > > self._module = None
> > > # For explicitly defined entities, info points to the (explicit)
> > > - # definition. For builtins (and their arrays), info is None.
> > > - # For implicitly defined entities, info points to a place that
> > > - # triggered the implicit definition (there may be more than one
> > > - # such place).
> > > + # definition. For built-in types (and their arrays), info is a
> > > + # special object that evaluates to False. For implicitly defined
> > > + # entities, info points to a place that triggered the implicit
> > > + # definition (there may be more than one such place).
> > > self.info = info
> > > self.doc = doc
> > > self._ifcond = ifcond or []
> > > @@ -68,7 +69,7 @@ def check_doc(self):
> > > def _set_module(self, schema, info):
> > > assert self._checked
> > > - self._module = schema.module_by_fname(info and info.fname)
> > > + self._module = schema.module_by_fname(info.fname if info else None)
> >
> > Looks unrelated.
> >
>
> Hmm, it sorta is. I have apparently edited this patch since I sent it, but
> there was some tomfoolery over how "x and y" statements behave and this edit
> was necessary at the time.
>
> "info and info.fname" returned None when info could actually be None, but
> when it was updated to be a special source object, we could accidentally
> pass that special source object as the name -- instead of None. Not good.
>
> I think I re-ordered some patches such that I can just pass in "info.fname"
> unconditionally instead as of this patch.
>
> > > self._module.add_entity(self)
> > > def set_module(self, schema):
> > [...]
> >
>
--
Eduardo
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 07/12] qapi/schema: make QAPISourceInfo mandatory
2021-01-14 0:29 ` Eduardo Habkost
@ 2021-01-14 0:47 ` John Snow
0 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2021-01-14 0:47 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Michael Roth, Cleber Rosa, Markus Armbruster,
Marc-André Lureau, qemu-devel
On 1/13/21 7:29 PM, Eduardo Habkost wrote:
> On Wed, Jan 13, 2021 at 06:04:29PM -0500, John Snow wrote:
>> On 1/13/21 11:12 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
...
>>
>> I think we need to, yes; or we probably really, really want to. Making the
>> info parameter optional really adds a lot of unidiomatic type-checking
>> confetti when we go to use info, and in many more contexts than just this
>> sorta-built-in-enum; it will creep badly.
>
> Which methods would require unidiomatic type-checking because of
> None/Optional?
>
Virtually everything that accepts a QAPISourceInfo has to do something
like this:
def foo(info: Optional[QAPISourceInfo]):
if info is None:
raise Exception("Something very bad has happened!")
...
...
Since the great majority of cases *will* have an info, and we take
careful pains to make sure this info is preserved, it's less invasive to
just assert that info isn't Optional.
This is especially true for the QAPISchemaMember initializer, which
several other classes inherit -- if this is allowed to be
Optional[QAPISourceInfo], any and all users of a QAPISchemaMember or any
derived classes will have to check -- on every access -- to see if
'member.info' is set or not.
Since we expect it to be set 100% of the time for all user-defined code,
it's a lot less "if member.info is not None" checks everywhere.
Adding a special "built-in" source info object to make this claim helps
avoid making confusing type signatures for the visitors; i.e.
def visit_thing(info: Optional[QAPISourceInfo]): ...
is misleading, because we actually expect thing to *always* have a
SourceInfo. To make the documentation be a little more ... statistically
truthful, it's easier to bend the rules in the other direction and just
fill in the scant few cases where we don't have a QAPISourceInfo.
--js
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 08/12] qapi/gen: write _genc/_genh access shims
2020-12-17 1:59 [PATCH v2 00/12] qapi: static typing conversion, pt1.5 John Snow
` (6 preceding siblings ...)
2020-12-17 1:59 ` [PATCH v2 07/12] qapi/schema: make QAPISourceInfo mandatory John Snow
@ 2020-12-17 1:59 ` John Snow
2020-12-17 1:59 ` [PATCH v2 09/12] qapi/gen: move write method to QAPIGenC, make fname a str John Snow
` (4 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2020-12-17 1:59 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Michael Roth, Markus Armbruster, Cleber Rosa,
Marc-André Lureau, John Snow
Many places assume they can access these fields without checking them
first to ensure they are defined. Eliminating the _genc and _genh fields
and replacing them with functional properties that check for correct
state can ease the typing overhead by eliminating the Optional[T] return
type.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/gen.py | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 476d0adac4e..12ea98fb61e 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -243,11 +243,20 @@ def __init__(self,
self._user_blurb = user_blurb
self._builtin_blurb = builtin_blurb
self._pydoc = pydoc
- self._genc: Optional[QAPIGenC] = None
- self._genh: Optional[QAPIGenH] = None
+ self._current_module: Optional[str] = None
self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH]] = {}
self._main_module: Optional[str] = None
+ @property
+ def _genc(self) -> QAPIGenC:
+ assert self._current_module is not None
+ return self._module[self._current_module][0]
+
+ @property
+ def _genh(self) -> QAPIGenH:
+ assert self._current_module is not None
+ return self._module[self._current_module][1]
+
@staticmethod
def _is_user_module(name: str) -> bool:
return not name.startswith('./')
@@ -282,7 +291,7 @@ def _add_module(self, name: str, blurb: str) -> None:
genc = QAPIGenC(basename + '.c', blurb, self._pydoc)
genh = QAPIGenH(basename + '.h', blurb, self._pydoc)
self._module[name] = (genc, genh)
- self._genc, self._genh = self._module[name]
+ self._current_module = name
def _add_user_module(self, name: str, blurb: str) -> None:
assert self._is_user_module(name)
@@ -315,8 +324,7 @@ def visit_module(self, name: Optional[str]) -> None:
else:
# The built-in module has not been created. No code may
# be generated.
- self._genc = None
- self._genh = None
+ self._current_module = None
else:
self._add_user_module(name, self._user_blurb)
self._begin_user_module(name)
--
2.26.2
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 09/12] qapi/gen: move write method to QAPIGenC, make fname a str
2020-12-17 1:59 [PATCH v2 00/12] qapi: static typing conversion, pt1.5 John Snow
` (7 preceding siblings ...)
2020-12-17 1:59 ` [PATCH v2 08/12] qapi/gen: write _genc/_genh access shims John Snow
@ 2020-12-17 1:59 ` John Snow
2020-12-17 1:59 ` [PATCH v2 10/12] tests/qapi-schema: Add quotes to module name in test output John Snow
` (3 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2020-12-17 1:59 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Michael Roth, Markus Armbruster, Cleber Rosa,
Marc-André Lureau, John Snow
QAPIGenC and QAPIGenH in particular depend on fname being defined, but
we have a usage of QAPIGenCCode that isn't intended to be associated
with a particular file.
No problem, move the write method down to the class that actually needs
it, and keep QAPIGenCCode more abstract.
Signed-off-by: John Snow <jsnow@redhat.com>
---
Possibly un-needed in concert with a forthcoming patch by Markus, but I
didn't have it in my hands at time of publishing.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/commands.py | 2 +-
scripts/qapi/gen.py | 54 ++++++++++++++++++++--------------------
2 files changed, 28 insertions(+), 28 deletions(-)
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 71744f48a35..b346676d15a 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -258,7 +258,7 @@ def __init__(self, prefix: str):
super().__init__(
prefix, 'qapi-commands',
' * Schema-defined QAPI/QMP commands', None, __doc__)
- self._regy = QAPIGenCCode(None)
+ self._regy = QAPIGenCCode()
self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]] = {}
def _begin_user_module(self, name: str) -> None:
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 12ea98fb61e..2dd99635e74 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -36,8 +36,7 @@
class QAPIGen:
- def __init__(self, fname: Optional[str]):
- self.fname = fname
+ def __init__(self) -> None:
self._preamble = ''
self._body = ''
@@ -58,28 +57,6 @@ def _bottom(self) -> str:
# pylint: disable=no-self-use
return ''
- def write(self, output_dir: str) -> None:
- # Include paths starting with ../ are used to reuse modules of the main
- # schema in specialised schemas. Don't overwrite the files that are
- # already generated for the main schema.
- if self.fname.startswith('../'):
- return
- pathname = os.path.join(output_dir, self.fname)
- odir = os.path.dirname(pathname)
-
- if odir:
- os.makedirs(odir, exist_ok=True)
-
- # use os.open for O_CREAT to create and read a non-existant file
- fd = os.open(pathname, os.O_RDWR | os.O_CREAT, 0o666)
- with os.fdopen(fd, 'r+', encoding='utf-8') as fp:
- text = self.get_content()
- oldtext = fp.read(len(text) + 1)
- if text != oldtext:
- fp.seek(0)
- fp.truncate(0)
- fp.write(text)
-
def _wrap_ifcond(ifcond: List[str], before: str, after: str) -> str:
if before == after:
@@ -121,8 +98,8 @@ def build_params(arg_type: Optional[QAPISchemaObjectType],
class QAPIGenCCode(QAPIGen):
- def __init__(self, fname: Optional[str]):
- super().__init__(fname)
+ def __init__(self) -> None:
+ super().__init__()
self._start_if: Optional[Tuple[List[str], str, str]] = None
def start_if(self, ifcond: List[str]) -> None:
@@ -147,11 +124,34 @@ def get_content(self) -> str:
class QAPIGenC(QAPIGenCCode):
def __init__(self, fname: str, blurb: str, pydoc: str):
- super().__init__(fname)
+ super().__init__()
+ self.fname = fname
self._blurb = blurb
self._copyright = '\n * '.join(re.findall(r'^Copyright .*', pydoc,
re.MULTILINE))
+ def write(self, output_dir: str) -> None:
+ # Include paths starting with ../ are used to reuse modules of the main
+ # schema in specialised schemas. Don't overwrite the files that are
+ # already generated for the main schema.
+ if self.fname.startswith('../'):
+ return
+ pathname = os.path.join(output_dir, self.fname)
+ odir = os.path.dirname(pathname)
+
+ if odir:
+ os.makedirs(odir, exist_ok=True)
+
+ # use os.open for O_CREAT to create and read a non-existant file
+ fd = os.open(pathname, os.O_RDWR | os.O_CREAT, 0o666)
+ with os.fdopen(fd, 'r+', encoding='utf-8') as fp:
+ text = self.get_content()
+ oldtext = fp.read(len(text) + 1)
+ if text != oldtext:
+ fp.seek(0)
+ fp.truncate(0)
+ fp.write(text)
+
def _top(self) -> str:
return mcgen('''
/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
--
2.26.2
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 10/12] tests/qapi-schema: Add quotes to module name in test output
2020-12-17 1:59 [PATCH v2 00/12] qapi: static typing conversion, pt1.5 John Snow
` (8 preceding siblings ...)
2020-12-17 1:59 ` [PATCH v2 09/12] qapi/gen: move write method to QAPIGenC, make fname a str John Snow
@ 2020-12-17 1:59 ` John Snow
2020-12-17 1:59 ` [PATCH v2 11/12] qapi/schema: Name the builtin module "" instead of None John Snow
` (2 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2020-12-17 1:59 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Michael Roth, Markus Armbruster, Cleber Rosa,
Marc-André Lureau, John Snow
A forthcoming patch is going to allow the empty string as a name for the
builtin module, and quotes will help us see that in test output. Without
this, git will be upset about trailing empty spaces in test output, so
the quotes are necessary.
Signed-off-by: John Snow <jsnow@redhat.com>
---
| 4 ++--
tests/qapi-schema/doc-good.out | 4 ++--
tests/qapi-schema/empty.out | 4 ++--
tests/qapi-schema/event-case.out | 4 ++--
tests/qapi-schema/include-repetition.out | 8 ++++----
tests/qapi-schema/include-simple.out | 6 +++---
tests/qapi-schema/indented-expr.out | 4 ++--
tests/qapi-schema/qapi-schema-test.out | 8 ++++----
tests/qapi-schema/test-qapi.py | 2 +-
9 files changed, 22 insertions(+), 22 deletions(-)
--git a/tests/qapi-schema/comments.out b/tests/qapi-schema/comments.out
index 273f0f54e16..08aba8354e2 100644
--- a/tests/qapi-schema/comments.out
+++ b/tests/qapi-schema/comments.out
@@ -1,4 +1,4 @@
-module None
+module "None"
object q_empty
enum QType
prefix QTYPE
@@ -9,7 +9,7 @@ enum QType
member qdict
member qlist
member qbool
-module comments.json
+module "comments.json"
enum Status
member good
member bad
diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
index 419284dae29..83a3d9bd69b 100644
--- a/tests/qapi-schema/doc-good.out
+++ b/tests/qapi-schema/doc-good.out
@@ -1,4 +1,4 @@
-module None
+module "None"
object q_empty
enum QType
prefix QTYPE
@@ -9,7 +9,7 @@ enum QType
member qdict
member qlist
member qbool
-module doc-good.json
+module "doc-good.json"
enum Enum
member one
if ['defined(IFONE)']
diff --git a/tests/qapi-schema/empty.out b/tests/qapi-schema/empty.out
index 69666c39ad2..0dac23c80c1 100644
--- a/tests/qapi-schema/empty.out
+++ b/tests/qapi-schema/empty.out
@@ -1,4 +1,4 @@
-module None
+module "None"
object q_empty
enum QType
prefix QTYPE
@@ -9,4 +9,4 @@ enum QType
member qdict
member qlist
member qbool
-module empty.json
+module "empty.json"
diff --git a/tests/qapi-schema/event-case.out b/tests/qapi-schema/event-case.out
index 42ae519656d..ace511ba5a9 100644
--- a/tests/qapi-schema/event-case.out
+++ b/tests/qapi-schema/event-case.out
@@ -1,4 +1,4 @@
-module None
+module "None"
object q_empty
enum QType
prefix QTYPE
@@ -9,6 +9,6 @@ enum QType
member qdict
member qlist
member qbool
-module event-case.json
+module "event-case.json"
event oops None
boxed=False
diff --git a/tests/qapi-schema/include-repetition.out b/tests/qapi-schema/include-repetition.out
index 0b654ddebb6..f7ab4987943 100644
--- a/tests/qapi-schema/include-repetition.out
+++ b/tests/qapi-schema/include-repetition.out
@@ -1,4 +1,4 @@
-module None
+module "None"
object q_empty
enum QType
prefix QTYPE
@@ -9,15 +9,15 @@ enum QType
member qdict
member qlist
member qbool
-module include-repetition.json
+module "include-repetition.json"
include comments.json
include include-repetition-sub.json
include comments.json
-module comments.json
+module "comments.json"
enum Status
member good
member bad
member ugly
-module include-repetition-sub.json
+module "include-repetition-sub.json"
include comments.json
include comments.json
diff --git a/tests/qapi-schema/include-simple.out b/tests/qapi-schema/include-simple.out
index 061f81e5090..81bdeb887b6 100644
--- a/tests/qapi-schema/include-simple.out
+++ b/tests/qapi-schema/include-simple.out
@@ -1,4 +1,4 @@
-module None
+module "None"
object q_empty
enum QType
prefix QTYPE
@@ -9,9 +9,9 @@ enum QType
member qdict
member qlist
member qbool
-module include-simple.json
+module "include-simple.json"
include include-simple-sub.json
-module include-simple-sub.json
+module "include-simple-sub.json"
enum Status
member good
member bad
diff --git a/tests/qapi-schema/indented-expr.out b/tests/qapi-schema/indented-expr.out
index 04356775cd1..361a58185e6 100644
--- a/tests/qapi-schema/indented-expr.out
+++ b/tests/qapi-schema/indented-expr.out
@@ -1,4 +1,4 @@
-module None
+module "None"
object q_empty
enum QType
prefix QTYPE
@@ -9,7 +9,7 @@ enum QType
member qdict
member qlist
member qbool
-module indented-expr.json
+module "indented-expr.json"
command eins None -> None
gen=True success_response=True boxed=False oob=False preconfig=False
command zwei None -> None
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 8868ca0dca9..4f5ab9fd596 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -1,4 +1,4 @@
-module None
+module "None"
object q_empty
enum QType
prefix QTYPE
@@ -9,7 +9,7 @@ enum QType
member qdict
member qlist
member qbool
-module qapi-schema-test.json
+module "qapi-schema-test.json"
object TestStruct
member integer: int optional=False
member boolean: bool optional=False
@@ -443,11 +443,11 @@ command test-command-cond-features3 None -> None
event TEST-EVENT-FEATURES1 None
boxed=False
feature deprecated
-module include/sub-module.json
+module "include/sub-module.json"
include sub-sub-module.json
object SecondArrayRef
member s: StatusList optional=False
-module sub-sub-module.json
+module "sub-sub-module.json"
array StatusList Status
enum Status
member good
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index e8db9d09d91..4adf0b3c185 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -25,7 +25,7 @@
class QAPISchemaTestVisitor(QAPISchemaVisitor):
def visit_module(self, name):
- print('module %s' % name)
+ print('module "%s"' % name)
def visit_include(self, name, info):
print('include %s' % name)
--
2.26.2
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 11/12] qapi/schema: Name the builtin module "" instead of None
2020-12-17 1:59 [PATCH v2 00/12] qapi: static typing conversion, pt1.5 John Snow
` (9 preceding siblings ...)
2020-12-17 1:59 ` [PATCH v2 10/12] tests/qapi-schema: Add quotes to module name in test output John Snow
@ 2020-12-17 1:59 ` John Snow
2020-12-17 1:59 ` [PATCH v2 12/12] qapi: enable strict-optional checks John Snow
2021-01-13 16:23 ` [PATCH v2 00/12] qapi: static typing conversion, pt1.5 Markus Armbruster
12 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2020-12-17 1:59 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Michael Roth, Markus Armbruster, Cleber Rosa,
Marc-André Lureau, John Snow
Instead of using None as the built-in module filename, use an empty
string instead. This allows us to clarify the type of various interfaces
dealing with module names as always taking a string, which saves us from
having to use Optional[str] everywhere.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/gen.py | 6 +++---
scripts/qapi/schema.py | 12 ++++++------
scripts/qapi/types.py | 2 +-
scripts/qapi/visit.py | 2 +-
| 2 +-
tests/qapi-schema/doc-good.out | 2 +-
tests/qapi-schema/empty.out | 2 +-
tests/qapi-schema/event-case.out | 2 +-
tests/qapi-schema/include-repetition.out | 2 +-
tests/qapi-schema/include-simple.out | 2 +-
tests/qapi-schema/indented-expr.out | 2 +-
tests/qapi-schema/qapi-schema-test.out | 2 +-
12 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 2dd99635e74..1933090a2a0 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -310,14 +310,14 @@ def write(self, output_dir: str, opt_builtins: bool = False) -> None:
genc.write(output_dir)
genh.write(output_dir)
- def _begin_system_module(self, name: None) -> None:
+ def _begin_system_module(self, name: str) -> None:
pass
def _begin_user_module(self, name: str) -> None:
pass
- def visit_module(self, name: Optional[str]) -> None:
- if name is None:
+ def visit_module(self, name: str) -> None:
+ if not name:
if self._builtin_blurb:
self._add_system_module('builtin', self._builtin_blurb)
self._begin_system_module(name)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 0449771dfe5..0235208966a 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -69,7 +69,7 @@ def check_doc(self):
def _set_module(self, schema, info):
assert self._checked
- self._module = schema.module_by_fname(info.fname if info else None)
+ self._module = schema.module_by_fname(info.fname)
self._module.add_entity(self)
def set_module(self, schema):
@@ -826,7 +826,7 @@ def __init__(self, fname):
self._entity_dict = {}
self._module_dict = OrderedDict()
self._schema_dir = os.path.dirname(fname)
- self._make_module(None) # built-ins
+ self._make_module(QAPISourceInfo.builtin().fname) # built-ins
self._make_module(fname)
self._predefining = True
self._def_predefineds()
@@ -871,10 +871,10 @@ def resolve_type(self, name, info, what):
info, "%s uses unknown type '%s'" % (what, name))
return typ
- def _module_name(self, fname):
- if fname is None:
- return None
- return os.path.relpath(fname, self._schema_dir)
+ def _module_name(self, fname: str) -> str:
+ if fname:
+ return os.path.relpath(fname, self._schema_dir)
+ return fname
def _make_module(self, fname):
name = self._module_name(fname)
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index a3a16284006..12eeea3aaff 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -272,7 +272,7 @@ def __init__(self, prefix: str):
prefix, 'qapi-types', ' * Schema-defined QAPI types',
' * Built-in QAPI types', __doc__)
- def _begin_system_module(self, name: None) -> None:
+ def _begin_system_module(self, name: str) -> None:
self._genc.preamble_add(mcgen('''
#include "qemu/osdep.h"
#include "qapi/dealloc-visitor.h"
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 3f49c307c56..76e34ee7f02 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -305,7 +305,7 @@ def __init__(self, prefix: str):
prefix, 'qapi-visit', ' * Schema-defined QAPI visitors',
' * Built-in QAPI visitors', __doc__)
- def _begin_system_module(self, name: None) -> None:
+ def _begin_system_module(self, name: str) -> None:
self._genc.preamble_add(mcgen('''
#include "qemu/osdep.h"
#include "qapi/error.h"
--git a/tests/qapi-schema/comments.out b/tests/qapi-schema/comments.out
index 08aba8354e2..02000c06e5e 100644
--- a/tests/qapi-schema/comments.out
+++ b/tests/qapi-schema/comments.out
@@ -1,4 +1,4 @@
-module "None"
+module ""
object q_empty
enum QType
prefix QTYPE
diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
index 83a3d9bd69b..494533d7479 100644
--- a/tests/qapi-schema/doc-good.out
+++ b/tests/qapi-schema/doc-good.out
@@ -1,4 +1,4 @@
-module "None"
+module ""
object q_empty
enum QType
prefix QTYPE
diff --git a/tests/qapi-schema/empty.out b/tests/qapi-schema/empty.out
index 0dac23c80c1..059caa4e1d2 100644
--- a/tests/qapi-schema/empty.out
+++ b/tests/qapi-schema/empty.out
@@ -1,4 +1,4 @@
-module "None"
+module ""
object q_empty
enum QType
prefix QTYPE
diff --git a/tests/qapi-schema/event-case.out b/tests/qapi-schema/event-case.out
index ace511ba5a9..4d9d2b519f4 100644
--- a/tests/qapi-schema/event-case.out
+++ b/tests/qapi-schema/event-case.out
@@ -1,4 +1,4 @@
-module "None"
+module ""
object q_empty
enum QType
prefix QTYPE
diff --git a/tests/qapi-schema/include-repetition.out b/tests/qapi-schema/include-repetition.out
index f7ab4987943..31d64631b66 100644
--- a/tests/qapi-schema/include-repetition.out
+++ b/tests/qapi-schema/include-repetition.out
@@ -1,4 +1,4 @@
-module "None"
+module ""
object q_empty
enum QType
prefix QTYPE
diff --git a/tests/qapi-schema/include-simple.out b/tests/qapi-schema/include-simple.out
index 81bdeb887b6..1b35b329571 100644
--- a/tests/qapi-schema/include-simple.out
+++ b/tests/qapi-schema/include-simple.out
@@ -1,4 +1,4 @@
-module "None"
+module ""
object q_empty
enum QType
prefix QTYPE
diff --git a/tests/qapi-schema/indented-expr.out b/tests/qapi-schema/indented-expr.out
index 361a58185e6..aed689e7bd6 100644
--- a/tests/qapi-schema/indented-expr.out
+++ b/tests/qapi-schema/indented-expr.out
@@ -1,4 +1,4 @@
-module "None"
+module ""
object q_empty
enum QType
prefix QTYPE
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 4f5ab9fd596..4a899ba63ec 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -1,4 +1,4 @@
-module "None"
+module ""
object q_empty
enum QType
prefix QTYPE
--
2.26.2
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 12/12] qapi: enable strict-optional checks
2020-12-17 1:59 [PATCH v2 00/12] qapi: static typing conversion, pt1.5 John Snow
` (10 preceding siblings ...)
2020-12-17 1:59 ` [PATCH v2 11/12] qapi/schema: Name the builtin module "" instead of None John Snow
@ 2020-12-17 1:59 ` John Snow
2021-01-13 16:23 ` [PATCH v2 00/12] qapi: static typing conversion, pt1.5 Markus Armbruster
12 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2020-12-17 1:59 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Michael Roth, Markus Armbruster, Cleber Rosa,
Marc-André Lureau, John Snow
In the modules that we are checking so far, we can be stricter about the
difference between Optional[T] and T types. Enable that check.
Enabling it now will assist review on further typing and cleanup work.
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/mypy.ini | 1 -
1 file changed, 1 deletion(-)
diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
index 74fc6c82153..04bd5db5278 100644
--- a/scripts/qapi/mypy.ini
+++ b/scripts/qapi/mypy.ini
@@ -1,6 +1,5 @@
[mypy]
strict = True
-strict_optional = False
disallow_untyped_calls = False
python_version = 3.6
--
2.26.2
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 00/12] qapi: static typing conversion, pt1.5
2020-12-17 1:59 [PATCH v2 00/12] qapi: static typing conversion, pt1.5 John Snow
` (11 preceding siblings ...)
2020-12-17 1:59 ` [PATCH v2 12/12] qapi: enable strict-optional checks John Snow
@ 2021-01-13 16:23 ` Markus Armbruster
12 siblings, 0 replies; 26+ messages in thread
From: Markus Armbruster @ 2021-01-13 16:23 UTC (permalink / raw)
To: John Snow
Cc: Michael Roth, Cleber Rosa, qemu-devel, Marc-André Lureau,
Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> Hi, this patchset enables strict optional checking in mypy for
> everything we have typed so far.
>
> In general, this patchset seeks to eliminate Optional[T] in favor of T
> wherever possible. Optional types used for object properties,
> function/method parameters and return values where we expect, in most
> cases, to be non-None is troublesome as it requires peppering the code
> with assertions about state. If and whenever possible, prefer using
> non-Optional types.
>
> Ironing out these issues allows us to be even stricter with our type
> checking, which improves consistency in subclass interface types and
> should make review a little nicer.
>
> This series is based on (but does not require) the 'qapi: sphinx-autodoc
> for qapi module' series.
Just two issues left that aren't entirely trivial: 1. mission creep
[PATCH 6+7], 2. integrating my "[PATCH 00/11] Drop support for QAPIGen
without a file name".
^ permalink raw reply [flat|nested] 26+ messages in thread