* [PATCH 01/12] qapi/commands: assert arg_type is not None
2020-12-14 23:53 [PATCH 00/12] qapi: static typing conversion, pt1.5 John Snow
@ 2020-12-14 23:53 ` John Snow
2020-12-14 23:53 ` [PATCH 02/12] qapi/events: fix visit_event typing John Snow
` (10 subsequent siblings)
11 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2020-12-14 23:53 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, Cleber Rosa, John Snow, Eduardo Habkost,
Markus Armbruster
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 50978090b440..71744f48a353 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] 41+ messages in thread* [PATCH 02/12] qapi/events: fix visit_event typing
2020-12-14 23:53 [PATCH 00/12] qapi: static typing conversion, pt1.5 John Snow
2020-12-14 23:53 ` [PATCH 01/12] qapi/commands: assert arg_type is not None John Snow
@ 2020-12-14 23:53 ` John Snow
2020-12-14 23:53 ` [PATCH 03/12] qapi/main: handle theoretical None-return from re.match() John Snow
` (9 subsequent siblings)
11 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2020-12-14 23:53 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, Cleber Rosa, John Snow, Eduardo Habkost,
Markus Armbruster
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 599f3d1f564b..9851653b9d11 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] 41+ messages in thread* [PATCH 03/12] qapi/main: handle theoretical None-return from re.match()
2020-12-14 23:53 [PATCH 00/12] qapi: static typing conversion, pt1.5 John Snow
2020-12-14 23:53 ` [PATCH 01/12] qapi/commands: assert arg_type is not None John Snow
2020-12-14 23:53 ` [PATCH 02/12] qapi/events: fix visit_event typing John Snow
@ 2020-12-14 23:53 ` John Snow
2020-12-16 8:23 ` Markus Armbruster
2020-12-14 23:53 ` [PATCH 04/12] qapi/gen: assert that _start_if is not None in _wrap_ifcond John Snow
` (8 subsequent siblings)
11 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2020-12-14 23:53 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, Cleber Rosa, John Snow, Eduardo Habkost,
Markus Armbruster
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 | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
index 42517210b805..271d9e84da94 100644
--- a/scripts/qapi/main.py
+++ b/scripts/qapi/main.py
@@ -23,7 +23,8 @@
def invalid_prefix_char(prefix: str) -> Optional[str]:
match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
- if match.end() != len(prefix):
+ # match cannot be None, but mypy cannot infer that.
+ if match and match.end() != len(prefix):
return prefix[match.end()]
return None
--
2.26.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH 03/12] qapi/main: handle theoretical None-return from re.match()
2020-12-14 23:53 ` [PATCH 03/12] qapi/main: handle theoretical None-return from re.match() John Snow
@ 2020-12-16 8:23 ` Markus Armbruster
2020-12-16 17:11 ` John Snow
0 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2020-12-16 8:23 UTC (permalink / raw)
To: John Snow
Cc: Marc-André Lureau, qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> 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 | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
> index 42517210b805..271d9e84da94 100644
> --- a/scripts/qapi/main.py
> +++ b/scripts/qapi/main.py
> @@ -23,7 +23,8 @@
>
> def invalid_prefix_char(prefix: str) -> Optional[str]:
> match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
@match can't be None because the regexp always matches a prefix,
possibly the empty prefix.
> - if match.end() != len(prefix):
> + # match cannot be None, but mypy cannot infer that.
> + if match and match.end() != len(prefix):
> return prefix[match.end()]
> return None
High-level logic:
if there is an invalid prefix character:
return it
return None
The actual code takes the return None when the match fails. If this
could happen, it would be wrong. I can't, so it doesn't matter, but I
dislike it anyway.
Alternative 1: turn "match cannot be None" from comment to code
match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
+ assert match
if match.end() != len(prefix):
return prefix[match.end()]
return None
Alternative 2: turn empty prefix into a special case
- match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
+ match = re.match(r'[A-Za-z_.-][A-Za-z0-9_.-]*', prefix)
+ if not match:
+ return prefix[0]
if match.end() != len(prefix):
return prefix[match.end()]
return None
I'd prefer alternative 1.
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 03/12] qapi/main: handle theoretical None-return from re.match()
2020-12-16 8:23 ` Markus Armbruster
@ 2020-12-16 17:11 ` John Snow
0 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2020-12-16 17:11 UTC (permalink / raw)
To: Markus Armbruster
Cc: Marc-André Lureau, qemu-devel, Eduardo Habkost, Cleber Rosa
On 12/16/20 3:23 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> 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 | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
>> index 42517210b805..271d9e84da94 100644
>> --- a/scripts/qapi/main.py
>> +++ b/scripts/qapi/main.py
>> @@ -23,7 +23,8 @@
>>
>> def invalid_prefix_char(prefix: str) -> Optional[str]:
>> match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
>
> @match can't be None because the regexp always matches a prefix,
> possibly the empty prefix.
>
>> - if match.end() != len(prefix):
>> + # match cannot be None, but mypy cannot infer that.
>> + if match and match.end() != len(prefix):
>> return prefix[match.end()]
>> return None
>
> High-level logic:
>
> if there is an invalid prefix character:
> return it
> return None
>
> The actual code takes the return None when the match fails. If this
> could happen, it would be wrong. I can't, so it doesn't matter, but I
> dislike it anyway.
>
> Alternative 1: turn "match cannot be None" from comment to code
>
> match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
> + assert match
> if match.end() != len(prefix):
> return prefix[match.end()]
> return None
>
> Alternative 2: turn empty prefix into a special case
>
> - match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
> + match = re.match(r'[A-Za-z_.-][A-Za-z0-9_.-]*', prefix)
> + if not match:
> + return prefix[0]
> if match.end() != len(prefix):
> return prefix[match.end()]
> return None
>
> I'd prefer alternative 1.
>
OK, no problem.
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 04/12] qapi/gen: assert that _start_if is not None in _wrap_ifcond
2020-12-14 23:53 [PATCH 00/12] qapi: static typing conversion, pt1.5 John Snow
` (2 preceding siblings ...)
2020-12-14 23:53 ` [PATCH 03/12] qapi/main: handle theoretical None-return from re.match() John Snow
@ 2020-12-14 23:53 ` John Snow
2020-12-16 8:26 ` Markus Armbruster
2020-12-14 23:53 ` [PATCH 05/12] qapi/gen: use './builtin' for the built-in module name John Snow
` (7 subsequent siblings)
11 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2020-12-14 23:53 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, Cleber Rosa, John Snow, Eduardo Habkost,
Markus Armbruster
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 b40f18eee3cd..a6dc991b1d03 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] 41+ messages in thread* Re: [PATCH 04/12] qapi/gen: assert that _start_if is not None in _wrap_ifcond
2020-12-14 23:53 ` [PATCH 04/12] qapi/gen: assert that _start_if is not None in _wrap_ifcond John Snow
@ 2020-12-16 8:26 ` Markus Armbruster
2020-12-16 17:13 ` John Snow
0 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2020-12-16 8:26 UTC (permalink / raw)
To: John Snow
Cc: Marc-André Lureau, qemu-devel, Eduardo Habkost, Cleber Rosa
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 b40f18eee3cd..a6dc991b1d03 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],
Drawback: the public method's precondition is now more opaque. Do we
care?
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 04/12] qapi/gen: assert that _start_if is not None in _wrap_ifcond
2020-12-16 8:26 ` Markus Armbruster
@ 2020-12-16 17:13 ` John Snow
2020-12-17 7:24 ` Markus Armbruster
0 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2020-12-16 17:13 UTC (permalink / raw)
To: Markus Armbruster
Cc: Marc-André Lureau, qemu-devel, Eduardo Habkost, Cleber Rosa
On 12/16/20 3:26 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 b40f18eee3cd..a6dc991b1d03 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],
>
> Drawback: the public method's precondition is now more opaque. Do we
> care?
>
Ish. If you call end_if before start_if, what did you want to have happen?
Or more to the point: do you want the assertion in both places?
--js
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 04/12] qapi/gen: assert that _start_if is not None in _wrap_ifcond
2020-12-16 17:13 ` John Snow
@ 2020-12-17 7:24 ` Markus Armbruster
0 siblings, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2020-12-17 7:24 UTC (permalink / raw)
To: John Snow
Cc: Marc-André Lureau, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> On 12/16/20 3:26 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 b40f18eee3cd..a6dc991b1d03 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],
>>
>> Drawback: the public method's precondition is now more opaque. Do we
>> care?
>>
>
> Ish. If you call end_if before start_if, what did you want to have happen?
Point.
> Or more to the point: do you want the assertion in both places?
What about inlining QAPIGenCCode._wrap_ifcond() into .end_if()?
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 05/12] qapi/gen: use './builtin' for the built-in module name
2020-12-14 23:53 [PATCH 00/12] qapi: static typing conversion, pt1.5 John Snow
` (3 preceding siblings ...)
2020-12-14 23:53 ` [PATCH 04/12] qapi/gen: assert that _start_if is not None in _wrap_ifcond John Snow
@ 2020-12-14 23:53 ` John Snow
2020-12-16 8:35 ` Markus Armbruster
2020-12-14 23:53 ` [PATCH 06/12] qapi/source: Add builtin null-object sentinel John Snow
` (6 subsequent siblings)
11 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2020-12-14 23:53 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, Cleber Rosa, John Snow, Eduardo Habkost,
Markus Armbruster
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 | 27 +++++++++++++--------------
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index a6dc991b1d03..0c5d1fee6088 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,14 @@ 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)
+ 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 +289,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(f"./{name}", blurb)
def write(self, output_dir: str, opt_builtins: bool = False) -> None:
for name in self._module:
@@ -310,7 +309,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] 41+ messages in thread* Re: [PATCH 05/12] qapi/gen: use './builtin' for the built-in module name
2020-12-14 23:53 ` [PATCH 05/12] qapi/gen: use './builtin' for the built-in module name John Snow
@ 2020-12-16 8:35 ` Markus Armbruster
2020-12-16 17:27 ` John Snow
0 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2020-12-16 8:35 UTC (permalink / raw)
To: John Snow
Cc: Marc-André Lureau, qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> Use this in preference to 'None', which helps remove some edge cases in
> the typing.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
Clearly better. Should've done it this way in commit c2e196a9b4 "qapi:
Prepare for system modules other than 'builtin'".
> ---
> scripts/qapi/gen.py | 27 +++++++++++++--------------
> 1 file changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index a6dc991b1d03..0c5d1fee6088 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,14 @@ def _module_basename(self, what: str, name: Optional[str]) -> str:
> if name != self._main_module:
> ret += '-' + os.path.splitext(basename)[0]
> else:
Possible drive-by improvement:
assert name.startswith('./')
> - name = name[2:] if name else 'builtin'
> - ret += re.sub(r'-', '-' + name + '-', what)
> + 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 +289,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(f"./{name}", blurb)
I like f-strings, I really do, but is
f"./{name}"
really clearer than
'./' + name
?
>
> def write(self, output_dir: str, opt_builtins: bool = False) -> None:
> for name in self._module:
> @@ -310,7 +309,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
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 05/12] qapi/gen: use './builtin' for the built-in module name
2020-12-16 8:35 ` Markus Armbruster
@ 2020-12-16 17:27 ` John Snow
0 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2020-12-16 17:27 UTC (permalink / raw)
To: Markus Armbruster
Cc: Marc-André Lureau, qemu-devel, Eduardo Habkost, Cleber Rosa
On 12/16/20 3:35 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Use this in preference to 'None', which helps remove some edge cases in
>> the typing.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>
> Clearly better. Should've done it this way in commit c2e196a9b4 "qapi:
> Prepare for system modules other than 'builtin'".
>
>> ---
>> scripts/qapi/gen.py | 27 +++++++++++++--------------
>> 1 file changed, 13 insertions(+), 14 deletions(-)
>>
>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>> index a6dc991b1d03..0c5d1fee6088 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,14 @@ def _module_basename(self, what: str, name: Optional[str]) -> str:
>> if name != self._main_module:
>> ret += '-' + os.path.splitext(basename)[0]
>> else:
>
> Possible drive-by improvement:
>
> assert name.startswith('./')
>
As long as nobody tells me to split that unrelated change into a new
patch I'll happily add drive-by improvements upon request ;)
>> - name = name[2:] if name else 'builtin'
>> - ret += re.sub(r'-', '-' + name + '-', what)
>> + 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 +289,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(f"./{name}", blurb)
>
> I like f-strings, I really do, but is
>
> f"./{name}"
>
> really clearer than
>
> './' + name
>
> ?
>
I tend to avoid string concatenation as a habit, but there's no greater
reason; the old value needed updating and that's what fell out of my
keyboard, I suppose.
>>
>> def write(self, output_dir: str, opt_builtins: bool = False) -> None:
>> for name in self._module:
>> @@ -310,7 +309,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
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 06/12] qapi/source: Add builtin null-object sentinel
2020-12-14 23:53 [PATCH 00/12] qapi: static typing conversion, pt1.5 John Snow
` (4 preceding siblings ...)
2020-12-14 23:53 ` [PATCH 05/12] qapi/gen: use './builtin' for the built-in module name John Snow
@ 2020-12-14 23:53 ` John Snow
2020-12-16 9:22 ` Markus Armbruster
2020-12-14 23:53 ` [PATCH 07/12] qapi/gen: write _genc/_genh access shims John Snow
` (5 subsequent siblings)
11 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2020-12-14 23:53 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, Cleber Rosa, John Snow, Eduardo Habkost,
Markus Armbruster
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" is 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 d7a79a9b8aee..64af7318cb67 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 a SourceInfo object corresponding to a builtin definition.
+ """
+ return cls('', -1, None)
+
+ def __bool__(self) -> bool:
+ # "if info: ..." is false if info is the builtin sentinel.
+ 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] 41+ messages in thread* Re: [PATCH 06/12] qapi/source: Add builtin null-object sentinel
2020-12-14 23:53 ` [PATCH 06/12] qapi/source: Add builtin null-object sentinel John Snow
@ 2020-12-16 9:22 ` Markus Armbruster
2020-12-16 17:53 ` John Snow
2020-12-16 19:11 ` John Snow
0 siblings, 2 replies; 41+ messages in thread
From: Markus Armbruster @ 2020-12-16 9:22 UTC (permalink / raw)
To: John Snow
Cc: Marc-André Lureau, qemu-devel, Eduardo Habkost, Cleber Rosa
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.
Two aspects mixed up:
1. Represent "no source info" as special QAPISourceInfo instead of
None
This is what de-complicates interface typing.
2. On error with "no source info", don't crash.
I have my doubts on this one.
Such an error means the QAPI code generator screwed up, at least in
theory. Crashing is only proper. It gets the screwup fixed.
In practice, errors due to interactions between built-in stuff and
user-defined stuff could conceivably escape testing. I can't
remember such a case offhand.
Will the "no source info" error be more useful than a crash?
Possibly. Will it get the screwup fixed? Maybe not.
Can we separate the two aspects?
>
> This object will evaluate as False, so "if info" is a valid idiomatic
> construct.
Suggest s/is a valid/remains a valid/.
Not 100% sure we'll want to keep this idiom, but now is not the time to
worry about that.
>
> 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 d7a79a9b8aee..64af7318cb67 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 a SourceInfo object corresponding to a builtin definition.
Let's spell it built-in for consistency with existing comments.
Could perhaps shorten "a SourceInfo object" to "an instance".
> + """
> + return cls('', -1, None)
No users? Peeking ahead... aha, they are in Patch 08. Any particular
reason for putting PATCH 07 between the two? Could PATCH 08 be squashed
into this one?
> +
> + def __bool__(self) -> bool:
> + # "if info: ..." is false if info is the builtin sentinel.
> + return bool(self.fname)
Nitpicking... "The builtin sentinel" suggests there is just one. PATCH
08 creates several. I don't mind, but let's say something like "if
@info corresponds to a built-in definition".
> +
> 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()
Looks like we can separate the two aspects easily:
def __str__(self) -> str:
+ assert not bool(self)
return self.include_path() + self.in_defn() + self.loc()
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 06/12] qapi/source: Add builtin null-object sentinel
2020-12-16 9:22 ` Markus Armbruster
@ 2020-12-16 17:53 ` John Snow
2020-12-17 12:33 ` Markus Armbruster
2020-12-16 19:11 ` John Snow
1 sibling, 1 reply; 41+ messages in thread
From: John Snow @ 2020-12-16 17:53 UTC (permalink / raw)
To: Markus Armbruster
Cc: Marc-André Lureau, qemu-devel, Eduardo Habkost, Cleber Rosa
On 12/16/20 4:22 AM, Markus Armbruster wrote:
> 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.
>
> Two aspects mixed up:
>
> 1. Represent "no source info" as special QAPISourceInfo instead of
> None
>
> This is what de-complicates interface typing.
>
Yup.
> 2. On error with "no source info", don't crash.
>
> I have my doubts on this one.
>
> Such an error means the QAPI code generator screwed up, at least in
> theory. Crashing is only proper. It gets the screwup fixed.
>
> In practice, errors due to interactions between built-in stuff and
> user-defined stuff could conceivably escape testing. I can't
> remember such a case offhand.
>
> Will the "no source info" error be more useful than a crash?
> Possibly. Will it get the screwup fixed? Maybe not.
>
> Can we separate the two aspects?
>
We can add an intentional assertion, if you'd like, that makes such
cases obvious -- but if we are already in the error printer, QAPI is
likely already in the process of crashing and printing an error.
So, Is this really an issue?
>>
>> This object will evaluate as False, so "if info" is a valid idiomatic
>> construct.
>
> Suggest s/is a valid/remains a valid/.
>
> Not 100% sure we'll want to keep this idiom, but now is not the time to
> worry about that.
>
OK.
>>
>> 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 d7a79a9b8aee..64af7318cb67 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 a SourceInfo object corresponding to a builtin definition.
>
> Let's spell it built-in for consistency with existing comments.
>
> Could perhaps shorten "a SourceInfo object" to "an instance".
>
OK.
>> + """
>> + return cls('', -1, None)
>
> No users? Peeking ahead... aha, they are in Patch 08. Any particular
> reason for putting PATCH 07 between the two? Could PATCH 08 be squashed
> into this one?
>
Too much soup in one pot: this patch highlights the "trick" and the
subsequent patch shows the adoption of it. Seemed safe.
Goofy ordering, though. I've pushed the genc/genh patch downwards
instead; you can squash them on commit if you'd like.
>> +
>> + def __bool__(self) -> bool:
>> + # "if info: ..." is false if info is the builtin sentinel.
>> + return bool(self.fname)
>
> Nitpicking... "The builtin sentinel" suggests there is just one. PATCH
> 08 creates several. I don't mind, but let's say something like "if
> @info corresponds to a built-in definition".
>
Fair enough. I don't mind nitpicks on comments and docstrings so much if
it helps make things clearer for more people.
(And they don't cause me rebase pain as much as other nitpicks ;)
>> +
>> 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()
>
> Looks like we can separate the two aspects easily:
>
> def __str__(self) -> str:
> + assert not bool(self)
> return self.include_path() + self.in_defn() + self.loc()
>
Feels like abusing __str__ to prevent application logic we don't like
elsewhere and unrelated to this class; I am still leaning on "If we are
printing this, it's likely we're already crashing" unless you have news
to the contrary for me.
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 06/12] qapi/source: Add builtin null-object sentinel
2020-12-16 17:53 ` John Snow
@ 2020-12-17 12:33 ` Markus Armbruster
2020-12-18 19:14 ` John Snow
0 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2020-12-17 12:33 UTC (permalink / raw)
To: John Snow
Cc: Marc-André Lureau, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> On 12/16/20 4:22 AM, Markus Armbruster wrote:
>> 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.
>>
>> Two aspects mixed up:
>>
>> 1. Represent "no source info" as special QAPISourceInfo instead of
>> None
>>
>> This is what de-complicates interface typing.
>>
>
> Yup.
>
>> 2. On error with "no source info", don't crash.
>>
>> I have my doubts on this one.
>>
>> Such an error means the QAPI code generator screwed up, at least in
>> theory. Crashing is only proper. It gets the screwup fixed.
>>
>> In practice, errors due to interactions between built-in stuff and
>> user-defined stuff could conceivably escape testing. I can't
>> remember such a case offhand.
>>
>> Will the "no source info" error be more useful than a crash?
>> Possibly. Will it get the screwup fixed? Maybe not.
>>
>> Can we separate the two aspects?
>>
>
> We can add an intentional assertion, if you'd like, that makes such
> cases obvious -- but if we are already in the error printer, QAPI is
> likely already in the process of crashing and printing an error.
>
> So, Is this really an issue?
A patch limited to the first aspect merely tweaks an implementation
detail.
As soon as we include the second aspect, we get to debate how to handle
programming errors, and maybe whether any of the errors involving a
QAPISourceInfo.builtin() are *not* programming errors.
I'd prefer this series to remain focused on "enabling strict optional
checking in mypy for everything we have typed so far".
>>>
>>> This object will evaluate as False, so "if info" is a valid idiomatic
>>> construct.
>>
>> Suggest s/is a valid/remains a valid/.
>>
>> Not 100% sure we'll want to keep this idiom, but now is not the time to
>> worry about that.
>>
>
> OK.
>
>>>
>>> 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 d7a79a9b8aee..64af7318cb67 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 a SourceInfo object corresponding to a builtin definition.
>>
>> Let's spell it built-in for consistency with existing comments.
>>
>> Could perhaps shorten "a SourceInfo object" to "an instance".
>>
>
> OK.
>
>>> + """
>>> + return cls('', -1, None)
>>
>> No users? Peeking ahead... aha, they are in Patch 08. Any particular
>> reason for putting PATCH 07 between the two? Could PATCH 08 be squashed
>> into this one?
>>
>
> Too much soup in one pot: this patch highlights the "trick" and the
> subsequent patch shows the adoption of it. Seemed safe.
>
> Goofy ordering, though. I've pushed the genc/genh patch downwards
> instead; you can squash them on commit if you'd like.
>
>>> +
>>> + def __bool__(self) -> bool:
>>> + # "if info: ..." is false if info is the builtin sentinel.
>>> + return bool(self.fname)
>>
>> Nitpicking... "The builtin sentinel" suggests there is just one. PATCH
>> 08 creates several. I don't mind, but let's say something like "if
>> @info corresponds to a built-in definition".
>>
>
> Fair enough. I don't mind nitpicks on comments and docstrings so much if
> it helps make things clearer for more people.
>
> (And they don't cause me rebase pain as much as other nitpicks ;)
>
>>> +
>>> 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()
>>
>> Looks like we can separate the two aspects easily:
>>
>> def __str__(self) -> str:
>> + assert not bool(self)
>> return self.include_path() + self.in_defn() + self.loc()
>>
>
> Feels like abusing __str__ to prevent application logic we don't like
> elsewhere and unrelated to this class; I am still leaning on "If we are
> printing this, it's likely we're already crashing" unless you have news
> to the contrary for me.
You're right, making __str__() fail is not nice. It has other uses,
e.g. when messing around interactively.
Ways out:
1. Avoid abuse of __str__() by naming the thing differently.
2. Lift the assertion into the relevant caller(s). Unfortunately, the
relevant caller is another __str__(): QAPIError's. Next level up:
the except suite in main() and also test-qapi.py's test_and_diff().
Keeping the stack backtrace useful might require care.
3. Find a simpler way to signal "oops, programming error".
For a simple batch program like this one, crashing is a perfectly
fine reaction to programming errors. Most of the time, it's also the
simplest one. Simple is *good*. *Especially* when it's something
that should never happen.
If reporting an error is genuinely simpler than crashing: simple is
good. But the error message should clearly express "this is a bug,
please report it", like a crash does.
Drawbacks: we'd have to relax our rule "no error without a test
case", and we'd lose the stack backtrace.
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 06/12] qapi/source: Add builtin null-object sentinel
2020-12-17 12:33 ` Markus Armbruster
@ 2020-12-18 19:14 ` John Snow
0 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2020-12-18 19:14 UTC (permalink / raw)
To: Markus Armbruster
Cc: Marc-André Lureau, Cleber Rosa, qemu-devel, Eduardo Habkost
On 12/17/20 7:33 AM, Markus Armbruster wrote:
> A patch limited to the first aspect merely tweaks an implementation
> detail.
>
> As soon as we include the second aspect, we get to debate how to handle
> programming errors, and maybe whether any of the errors involving a
> QAPISourceInfo.builtin() are*not* programming errors.
>
> I'd prefer this series to remain focused on "enabling strict optional
> checking in mypy for everything we have typed so far".
That is still the focus -- I believe adding a poison pill as you suggest
actually causes more errors instead of "maintaining" them; it doesn't
maintain a status quo.
Now, I understand the concern that this is opening a chance to allow a
new class of errors to slip by undetected, but I don't believe it does
in a meaningful way.
I didn't intend for this info object to be a poison; it wasn't designed
as one, nor should it be. It's merely a SourceInfo that represents the
degenerate case that something-or-other does not have an explicit user
source.
You're right that it probably shouldn't be seen in normal conditions,
but if it has; my take is that something *else* has already gone
horribly wrong and we aren't just sweeping dust under the mat, actually.
Here's a recap of my thinking process in identifying the problem and my
fix for it:
- Many functions expect that QAPISourceInfo *will* be present. Largely,
they are correct in expecting this, but very few of them check or assert
this. This is perfectly normal for untyped Python.
- Taxonomically, however, this is untrue, because the built-in entities
do not need to specify one.
- This can be addressed in typed python by adding assertions everywhere
("assert info is not None"), or it can be addressed by making the core
assumption true ("QAPISourceInfo is not Optional").
I opted to make the core assumption true; which does introduce a new
ambiguity that may not have been present before:
"If we have an info object, is it a real user-source info object, or is
it the built-in sentinel?"
I *think* this is where you start to get worried; we have "lost" the
ability to know, in an "obvious" way if this object is "valid" for a
given context. The assumption is that the code will explode violently if
we are passed None instead of a valid source info.
I don't think this assumption is true, though. mypy informs me that we
were never checking to see if it was valid anyway, so we already have
this ambiguity in the code today. Given that the 'info' object is, in
most cases, not used until *after we have detected another error*,
changing the info object to be non-none does not suppress any errors
that didn't already exist in these contexts.
What about places where QAPISourceInfo is used outside of an
error-handling context to gain contextual information about an entity?
Do we open ourselves up to new cases where we may have had an implicit
error, but now we do not?
Explicit usages of QAPISourceInfo are here:
1. QAPISchemaEntity._set_module(), which uses the info fname to find the
QAPISchemaModule object. This already checks for "None" info now, and
will check for the "./builtin" info after the series.
2. QAPISchemaEntity.is_implicit(), which uses the falsey nature of info
to intuit that it is a built-in definition. No change in behavior.
3. QAPISchemaArrayType.check(), which uses the falsey nature of
self.info to conditionally pass self.info.defn_meta to
QAPISchema.resolve_type().
Found a bug (previously fixed in part 6 prior to enabling type checking
on schema.py, which is why it went undetected here. I am moving the fix
forward into this series):
info and info.defn_meta will evaluate to the sentinel QAPISourceInfo
instead of an empty string or None, but it can be changed simply to just
"info.defn_meta" to take the None value from the builtin source info's
defn_meta field; easy.
4. QAPISchemaMember.describe(), which uses info.defn_name. This is only
ever called in error-handling contexts, so we aren't reducing total
error coverage here, either.
5. QAPISchemaCommand.check(), which uses info.pragma.returns_whitelist.
This might legitimately create a new avenue where we succeed where we
used to fail, if we were to create a built-in command. Whether or not
this is desirable depends on how you want pragma directives to work
long-term; per-file or per-schema. (Do you want to let user-defined
schemas dictate how built-in commands work?)
I don't think this is an issue.
6. QAPISchema._def_entity(), which asserts that info must be true-ish,
or we are in the predefining state. This logic doesn't change.
7. QAPISchema._def_entity(), which uses the true-ish nature of
other_ent.info to print a different error message. This logic doesn't
change.
8. expr.py:check_type(), which uses info.pragma.returns_whitelist. This
theoretically adds slack, but parser.py always gives us real info objects.
9. expr.py:check_exprs(), which calls info.set_defn(meta, name).
Obviously this must not be None because parser.py does not create
"built-in" source objects; we know with certainty here that we cannot
get such a thing.
10. expr.py:check_exprs() again, checking info.pragma.doc_required. As
above, we know this is always a true source info.
11. QAPIError's str() method itself will call str(info), but if info is
None, it doesn't crash -- you just get "None". Now you'll get
"[builtin]". Doesn't permit any new error cases.
Conclusions:
- There are users that use info to decide on behavior outside of an
error context, but they are already cared for.
- There are some pre-schema.py uses of info that may cause less problems
than they used to, but would require someone to add the placeholder
object into parser.py. We can throw a single assertion in expr.py to
make sure it never happens by accident.
- There are no, and I doubt there ever will be, users who use str(info)
to decide on behavior. There are only exception handlers that use this
method. There is no value to making str(info) in particular a poison pill.
- There might be some limited benefit to poisoning info.pragma, depending.
Ultimately, I consider this the standard idiomatic usage of info:
```
def some_func(info):
if (error):
raise QAPISemError(info, "you forgot to defrobulate")
```
When we go to print the error message to the user, the exception handler
is going to fault because str(info) is poisoned. Is it useful to poison
the exception handler here?
I don't think so; especially considering that even "None" here will work
just fine!
So I suppose I would prefer not to add poison, because I don't think
it's necessary, or improves anything materially.
--js
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 06/12] qapi/source: Add builtin null-object sentinel
2020-12-16 9:22 ` Markus Armbruster
2020-12-16 17:53 ` John Snow
@ 2020-12-16 19:11 ` John Snow
2020-12-17 11:56 ` Markus Armbruster
1 sibling, 1 reply; 41+ messages in thread
From: John Snow @ 2020-12-16 19:11 UTC (permalink / raw)
To: Markus Armbruster
Cc: Marc-André Lureau, qemu-devel, Eduardo Habkost, Cleber Rosa
On 12/16/20 4:22 AM, Markus Armbruster wrote:
> 2. On error with "no source info", don't crash.
>
> I have my doubts on this one.
>
> Such an error means the QAPI code generator screwed up, at least in
> theory. Crashing is only proper. It gets the screwup fixed.
>
QAPISemError and friends will also halt the generator and don't produce
output and will fail tests. They aren't less visible or more ignorable
somehow.
> In practice, errors due to interactions between built-in stuff and
> user-defined stuff could conceivably escape testing. I can't
> remember such a case offhand.
>
> Will the "no source info" error be more useful than a crash?
> Possibly. Will it get the screwup fixed? Maybe not.
I don't understand this; if it's an error -- there's no QAPI, there's no
QEMU. It's definitely getting fixed.
If QAPISourceInfo is primarily used for printing error information, we
are already in a situation where the generator is hosed and has wandered
itself into a problem that can't be ignored.
There's no additional value in having python crash twice per every crash
because we have bad types in our error reporting functions.
--js
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 06/12] qapi/source: Add builtin null-object sentinel
2020-12-16 19:11 ` John Snow
@ 2020-12-17 11:56 ` Markus Armbruster
2020-12-18 19:22 ` John Snow
0 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2020-12-17 11:56 UTC (permalink / raw)
To: John Snow
Cc: Marc-André Lureau, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> On 12/16/20 4:22 AM, Markus Armbruster wrote:
>> 2. On error with "no source info", don't crash.
>> I have my doubts on this one.
>> Such an error means the QAPI code generator screwed up, at least
>> in
>> theory. Crashing is only proper. It gets the screwup fixed.
>>
>
> QAPISemError and friends will also halt the generator and don't
> produce output and will fail tests. They aren't less visible or more
> ignorable somehow.
>
>> In practice, errors due to interactions between built-in stuff and
>> user-defined stuff could conceivably escape testing. I can't
>> remember such a case offhand.
>> Will the "no source info" error be more useful than a crash?
>> Possibly. Will it get the screwup fixed? Maybe not.
>
> I don't understand this; if it's an error -- there's no QAPI, there's
> no QEMU. It's definitely getting fixed.
>
> If QAPISourceInfo is primarily used for printing error information, we
> are already in a situation where the generator is hosed and has
> wandered itself into a problem that can't be ignored.
>
> There's no additional value in having python crash twice per every
> crash because we have bad types in our error reporting functions.
Consider the following scenario. The average developer knows just
enough about QAPI to be dangerous. That's okay; if you had to be a QAPI
expert to modify the QAPI schema, we would have failed. Now meet Joe
Average. He's a good guy. Today his job happens to require extending
the QAPI schema. In a hurry, as usual. So Joe brings his well-honed
voodoo programming skills to bear, and writes something that looks
plausible to him. His build fails. He's not surprised; he's voodoo-
programming after all. However, the error message is less clear than
usual. Something about a '[builtin]' file. There is no '[builtin]'
file. What to do? Obvious! If a bit of voodoo doesn't get you over
the finish line, use more: twiddle the schema until it works.
If his build failed with a Python backtrace instead, Joe would
immediately know that he ran into a bug in our tooling he should report.
Again, I don't mean to criticize Joe. I've walked in his shoes myself.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 06/12] qapi/source: Add builtin null-object sentinel
2020-12-17 11:56 ` Markus Armbruster
@ 2020-12-18 19:22 ` John Snow
0 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2020-12-18 19:22 UTC (permalink / raw)
To: Markus Armbruster
Cc: Marc-André Lureau, qemu-devel, Eduardo Habkost, Cleber Rosa
On 12/17/20 6:56 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> On 12/16/20 4:22 AM, Markus Armbruster wrote:
>>> 2. On error with "no source info", don't crash.
>>> I have my doubts on this one.
>>> Such an error means the QAPI code generator screwed up, at least
>>> in
>>> theory. Crashing is only proper. It gets the screwup fixed.
>>>
>>
>> QAPISemError and friends will also halt the generator and don't
>> produce output and will fail tests. They aren't less visible or more
>> ignorable somehow.
>>
>>> In practice, errors due to interactions between built-in stuff and
>>> user-defined stuff could conceivably escape testing. I can't
>>> remember such a case offhand.
>>> Will the "no source info" error be more useful than a crash?
>>> Possibly. Will it get the screwup fixed? Maybe not.
>>
>> I don't understand this; if it's an error -- there's no QAPI, there's
>> no QEMU. It's definitely getting fixed.
>>
>> If QAPISourceInfo is primarily used for printing error information, we
>> are already in a situation where the generator is hosed and has
>> wandered itself into a problem that can't be ignored.
>>
>> There's no additional value in having python crash twice per every
>> crash because we have bad types in our error reporting functions.
>
> Consider the following scenario. The average developer knows just
> enough about QAPI to be dangerous. That's okay; if you had to be a QAPI
> expert to modify the QAPI schema, we would have failed. Now meet Joe
> Average. He's a good guy. Today his job happens to require extending
> the QAPI schema. In a hurry, as usual. So Joe brings his well-honed
> voodoo programming skills to bear, and writes something that looks
> plausible to him. His build fails. He's not surprised; he's voodoo-
> programming after all. However, the error message is less clear than
> usual. Something about a '[builtin]' file. There is no '[builtin]'
> file. What to do? Obvious! If a bit of voodoo doesn't get you over
> the finish line, use more: twiddle the schema until it works.
>
> If his build failed with a Python backtrace instead, Joe would
> immediately know that he ran into a bug in our tooling he should report.
>
> Again, I don't mean to criticize Joe. I've walked in his shoes myself.
>
>
Worse, Joe was going to see an error about the "None" file or an error
about the "[builtin]" file.
I understand, though: don't suppress crash messages with opaque crap
that doesn't help spot the error. It's just that I don't think that's
what is happening here.
See lengthy response in other email.
--js
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 07/12] qapi/gen: write _genc/_genh access shims
2020-12-14 23:53 [PATCH 00/12] qapi: static typing conversion, pt1.5 John Snow
` (5 preceding siblings ...)
2020-12-14 23:53 ` [PATCH 06/12] qapi/source: Add builtin null-object sentinel John Snow
@ 2020-12-14 23:53 ` John Snow
2020-12-14 23:53 ` [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory John Snow
` (4 subsequent siblings)
11 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2020-12-14 23:53 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, Cleber Rosa, John Snow, Eduardo Habkost,
Markus Armbruster
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 0c5d1fee6088..17ae9f4af703 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('./')
@@ -281,7 +290,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)
@@ -314,8 +323,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] 41+ messages in thread* [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory
2020-12-14 23:53 [PATCH 00/12] qapi: static typing conversion, pt1.5 John Snow
` (6 preceding siblings ...)
2020-12-14 23:53 ` [PATCH 07/12] qapi/gen: write _genc/_genh access shims John Snow
@ 2020-12-14 23:53 ` John Snow
2020-12-16 10:18 ` Markus Armbruster
2020-12-14 23:53 ` [PATCH 09/12] qapi/gen: move write method to QAPIGenC, make fname a str John Snow
` (3 subsequent siblings)
11 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2020-12-14 23:53 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, Cleber Rosa, John Snow, Eduardo Habkost,
Markus Armbruster
--
events.py had an info to route, was it by choice that it wasn't before?
Signed-off-by: John Snow <jsnow@redhat.com>
---
scripts/qapi/events.py | 2 +-
scripts/qapi/schema.py | 23 +++++++++++++----------
scripts/qapi/types.py | 9 +++++----
scripts/qapi/visit.py | 6 +++---
4 files changed, 22 insertions(+), 18 deletions(-)
diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index 9851653b9d11..9ba4f109028d 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 720449feee4d..d5f19732b516 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 builtins (and their arrays), info is a null-object
+ # sentinel 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 []
@@ -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')
@@ -871,7 +872,7 @@ def resolve_type(self, name, info, what):
return typ
def _module_name(self, fname):
- if fname is None:
+ if not fname:
return None
return os.path.relpath(fname, self._schema_dir)
@@ -897,9 +898,11 @@ 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):
+ info = QAPISourceInfo.builtin()
+
for t in [('str', 'string', 'char' + POINTER_SUFFIX),
('number', 'number', 'double'),
('int', 'int', 'int64_t'),
@@ -917,15 +920,15 @@ 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', info, 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], info)
- self._def_entity(QAPISchemaEnumType('QType', None, None, None, None,
+ self._def_entity(QAPISchemaEnumType('QType', info, None, None, None,
qtype_values, 'QTYPE'))
def _make_features(self, features, info):
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index 2b4916cdaa1b..a3a16284006b 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 339f1521524c..3f49c307c566 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] 41+ messages in thread* Re: [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory
2020-12-14 23:53 ` [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory John Snow
@ 2020-12-16 10:18 ` Markus Armbruster
2020-12-16 18:41 ` John Snow
0 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2020-12-16 10:18 UTC (permalink / raw)
To: John Snow
Cc: Marc-André Lureau, qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> --
>
> events.py had an info to route, was it by choice that it wasn't before?
See below.
I figure this is intentionally below the -- line, but ...
> Signed-off-by: John Snow <jsnow@redhat.com>
... this should be above it.
> ---
> scripts/qapi/events.py | 2 +-
> scripts/qapi/schema.py | 23 +++++++++++++----------
> scripts/qapi/types.py | 9 +++++----
> scripts/qapi/visit.py | 6 +++---
> 4 files changed, 22 insertions(+), 18 deletions(-)
>
> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
> index 9851653b9d11..9ba4f109028d 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))
We pass None because errors should never happen, and None makes that
quite clear.
We don't actually have a built-in QAPISchemaEnumType for the event enum.
We merely generate a C enum QAPIEvent along with macro QAPIEvent_str(),
by passing self._event_emit_name, self._event_enum_members straight to
gen_enum() and gen_enum_lookup().
If we did build a QAPISchemaEnumType, and used it to diagnose clashes,
then clashes would be reported like
mumble.json: In event 'A-B':
mumble.json: 2: value 'A-B' collides with value 'A_B'
leaving you guessing what "value" means, and where 'A_B' may be.
Bug: we don't diagnose certain event name clashes. I'll fix it.
>
>
> def gen_events(schema: QAPISchema,
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 720449feee4d..d5f19732b516 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 builtins (and their arrays), info is a null-object
> + # sentinel 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).
s/builtins/built-in types/
The meaning of "a null object sentinel" is less than clear. Perhaps "a
special object".
> self.info = info
> self.doc = doc
> self._ifcond = ifcond or []
> @@ -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')
> @@ -871,7 +872,7 @@ def resolve_type(self, name, info, what):
> return typ
>
> def _module_name(self, fname):
> - if fname is None:
> + if not fname:
> return None
> return os.path.relpath(fname, self._schema_dir)
>
Sure this hunk belongs to this patch?
> @@ -897,9 +898,11 @@ 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):
> + info = QAPISourceInfo.builtin()
> +
Everything else gets its very own copy of the "no source info" object,
except for the stuff defined here, which gets to share one. Isn't that
odd?
> for t in [('str', 'string', 'char' + POINTER_SUFFIX),
> ('number', 'number', 'double'),
> ('int', 'int', 'int64_t'),
> @@ -917,15 +920,15 @@ 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', info, 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], info)
>
> - self._def_entity(QAPISchemaEnumType('QType', None, None, None, None,
> + self._def_entity(QAPISchemaEnumType('QType', info, None, None, None,
> qtype_values, 'QTYPE'))
>
> def _make_features(self, features, info):
> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> index 2b4916cdaa1b..a3a16284006b 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 339f1521524c..3f49c307c566 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],
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory
2020-12-16 10:18 ` Markus Armbruster
@ 2020-12-16 18:41 ` John Snow
2020-12-17 8:02 ` Markus Armbruster
0 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2020-12-16 18:41 UTC (permalink / raw)
To: Markus Armbruster
Cc: Marc-André Lureau, qemu-devel, Eduardo Habkost, Cleber Rosa
On 12/16/20 5:18 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> --
>>
>> events.py had an info to route, was it by choice that it wasn't before?
>
> See below.
>
> I figure this is intentionally below the -- line, but ...
>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>
> ... this should be above it.
>
Script failure. Or human failure, because I used '--' instead of '---'.
>> ---
>> scripts/qapi/events.py | 2 +-
>> scripts/qapi/schema.py | 23 +++++++++++++----------
>> scripts/qapi/types.py | 9 +++++----
>> scripts/qapi/visit.py | 6 +++---
>> 4 files changed, 22 insertions(+), 18 deletions(-)
>>
>> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
>> index 9851653b9d11..9ba4f109028d 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))
>
> We pass None because errors should never happen, and None makes that
> quite clear.
>
Not clear: *why* should errors never happen? This is the only place we
pass None for SourceInfo that isn't explicitly a literal built-in type.
This is not obvious.
> We don't actually have a built-in QAPISchemaEnumType for the event enum.
> We merely generate a C enum QAPIEvent along with macro QAPIEvent_str(),
> by passing self._event_emit_name, self._event_enum_members straight to
> gen_enum() and gen_enum_lookup().
>
> If we did build a QAPISchemaEnumType, and used it to diagnose clashes,
> then clashes would be reported like
>
> mumble.json: In event 'A-B':
> mumble.json: 2: value 'A-B' collides with value 'A_B'
>
> leaving you guessing what "value" means, and where 'A_B' may be.
>
> Bug: we don't diagnose certain event name clashes. I'll fix it.
>
Not clear to me: If I want interface consistency, what *should* be
passed downwards as the info? it's not quite a builtin as much as it is
an inferred enum, so should I just ... leave it like this, or wait for
you to offer a better fix?
>>
>>
>> def gen_events(schema: QAPISchema,
>> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>> index 720449feee4d..d5f19732b516 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 builtins (and their arrays), info is a null-object
>> + # sentinel 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).
>
> s/builtins/built-in types/
>
> The meaning of "a null object sentinel" is less than clear. Perhaps "a
> special object".
>
OK.
>> self.info = info
>> self.doc = doc
>> self._ifcond = ifcond or []
>> @@ -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')
>> @@ -871,7 +872,7 @@ def resolve_type(self, name, info, what):
>> return typ
>>
>> def _module_name(self, fname):
>> - if fname is None:
>> + if not fname:
>> return None
>> return os.path.relpath(fname, self._schema_dir)
>>
>
> Sure this hunk belongs to this patch?
>
Accident.
"info and info.fname" does not behave the same with a falsey info object
as it does when info was genuinely None; I compensated for that *here*,
but I should have compensated for it elsewhere.
>> @@ -897,9 +898,11 @@ 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):
>> + info = QAPISourceInfo.builtin()
>> +
>
> Everything else gets its very own copy of the "no source info" object,
> except for the stuff defined here, which gets to share one. Isn't that
> odd?
>
It's also the only function where we define so many built-ins in the
same place, so spiritually they do have the same SourceInfo, right? :)
(OK, no, it wasn't a conscious design choice, but it also seems
harmless. I am going to assume you'd prefer I not do this?)
>> for t in [('str', 'string', 'char' + POINTER_SUFFIX),
>> ('number', 'number', 'double'),
>> ('int', 'int', 'int64_t'),
>> @@ -917,15 +920,15 @@ 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', info, 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], info)
>>
>> - self._def_entity(QAPISchemaEnumType('QType', None, None, None, None,
>> + self._def_entity(QAPISchemaEnumType('QType', info, None, None, None,
>> qtype_values, 'QTYPE'))
>>
>> def _make_features(self, features, info):
>> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
>> index 2b4916cdaa1b..a3a16284006b 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 339f1521524c..3f49c307c566 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],
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory
2020-12-16 18:41 ` John Snow
@ 2020-12-17 8:02 ` Markus Armbruster
2020-12-17 17:02 ` John Snow
0 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2020-12-17 8:02 UTC (permalink / raw)
To: John Snow
Cc: Marc-André Lureau, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> On 12/16/20 5:18 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> --
>>>
>>> events.py had an info to route, was it by choice that it wasn't before?
>>
>> See below.
>>
>> I figure this is intentionally below the -- line, but ...
>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> ... this should be above it.
>>
>
> Script failure. Or human failure, because I used '--' instead of '---'.
>
>>> ---
>>> scripts/qapi/events.py | 2 +-
>>> scripts/qapi/schema.py | 23 +++++++++++++----------
>>> scripts/qapi/types.py | 9 +++++----
>>> scripts/qapi/visit.py | 6 +++---
>>> 4 files changed, 22 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
>>> index 9851653b9d11..9ba4f109028d 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))
>>
>> We pass None because errors should never happen, and None makes that
>> quite clear.
>>
>
> Not clear: *why* should errors never happen? This is the only place we
> pass None for SourceInfo that isn't explicitly a literal built-in type.
> This is not obvious.
You're right, but there are two separate "unclarities".
Passing None effectively asserts "errors never happen". We neglect to
explain why, leaving the reader guessing.
Passing @info "fixes" that by removing the assertion. Now we have a
more subtle problem: will errors make sense with this @info? Normally,
all members of an enum share one info. Only this enum's members don't.
It turns out the question is moot because @info is not actually used.
But will it remain moot? My point isn't that these are important
questions to answer. It is that we're replacing the relatively obvious
question "why are we asserting errors can't happen" by more subtle ones.
Feels like sweeping dirt under the rug.
>> We don't actually have a built-in QAPISchemaEnumType for the event enum.
>> We merely generate a C enum QAPIEvent along with macro QAPIEvent_str(),
>> by passing self._event_emit_name, self._event_enum_members straight to
>> gen_enum() and gen_enum_lookup().
>>
>> If we did build a QAPISchemaEnumType, and used it to diagnose clashes,
>> then clashes would be reported like
>>
>> mumble.json: In event 'A-B':
>> mumble.json: 2: value 'A-B' collides with value 'A_B'
>>
>> leaving you guessing what "value" means, and where 'A_B' may be.
>>
>> Bug: we don't diagnose certain event name clashes. I'll fix it.
>>
>
> Not clear to me: If I want interface consistency, what *should* be
> passed downwards as the info? it's not quite a builtin as much as it is
> an inferred enum,
True.
> so should I just ... leave it like this, or wait for
> you to offer a better fix?
Waiting for some better fix feels undavisable. We want to get type
checking in place sooner rather than later.
Aside: a possible fix is decoupling gen_enum_lookup() and gen_enum()
from QAPISchemaEnumMember, so we don't have to make up
QAPISchemaEnumMembers here.
I think there are two interpretations of your QAPISourceInfo work's aim:
1. Narrow: use a special QAPISourceInfo rather then None as "no errors
shall happen" poison.
QAPISourceInfo.builtin() returns this poison. The name "builtin" is
less than ideal.
2. Ambitious: eliminate "no errors shall happen".
We're discussing this in reply of PATCH 06. I think we need to reach a
conclusion there before we can decide here.
>>>
>>>
>>> def gen_events(schema: QAPISchema,
>>> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>>> index 720449feee4d..d5f19732b516 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 builtins (and their arrays), info is a null-object
>>> + # sentinel 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).
>>
>> s/builtins/built-in types/
>>
>> The meaning of "a null object sentinel" is less than clear. Perhaps "a
>> special object".
>>
>
> OK.
>
>>> self.info = info
>>> self.doc = doc
>>> self._ifcond = ifcond or []
>>> @@ -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')
>>> @@ -871,7 +872,7 @@ def resolve_type(self, name, info, what):
>>> return typ
>>>
>>> def _module_name(self, fname):
>>> - if fname is None:
>>> + if not fname:
>>> return None
>>> return os.path.relpath(fname, self._schema_dir)
>>>
>>
>> Sure this hunk belongs to this patch?
>>
>
> Accident.
>
> "info and info.fname" does not behave the same with a falsey info object
> as it does when info was genuinely None; I compensated for that *here*,
> but I should have compensated for it elsewhere.
>
>>> @@ -897,9 +898,11 @@ 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):
>>> + info = QAPISourceInfo.builtin()
>>> +
>>
>> Everything else gets its very own copy of the "no source info" object,
>> except for the stuff defined here, which gets to share one. Isn't that
>> odd?
>>
>
> It's also the only function where we define so many built-ins in the
> same place, so spiritually they do have the same SourceInfo, right? :)
>
> (OK, no, it wasn't a conscious design choice, but it also seems
> harmless. I am going to assume you'd prefer I not do this?)
It is harmless. It just made me wonder why we create more than one such
QAPISourceInfo in the first place. Method builtin() could return the
same one every time. It could even be an attribute instead of a method.
Anyway, no big deal.
>>> for t in [('str', 'string', 'char' + POINTER_SUFFIX),
>>> ('number', 'number', 'double'),
>>> ('int', 'int', 'int64_t'),
>>> @@ -917,15 +920,15 @@ 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', info, 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], info)
>>>
>>> - self._def_entity(QAPISchemaEnumType('QType', None, None, None, None,
>>> + self._def_entity(QAPISchemaEnumType('QType', info, None, None, None,
>>> qtype_values, 'QTYPE'))
>>>
>>> def _make_features(self, features, info):
>>> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
>>> index 2b4916cdaa1b..a3a16284006b 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 339f1521524c..3f49c307c566 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],
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory
2020-12-17 8:02 ` Markus Armbruster
@ 2020-12-17 17:02 ` John Snow
2020-12-18 5:24 ` Markus Armbruster
0 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2020-12-17 17:02 UTC (permalink / raw)
To: Markus Armbruster
Cc: Marc-André Lureau, Cleber Rosa, qemu-devel, Eduardo Habkost
On 12/17/20 3:02 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> On 12/16/20 5:18 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> --
>>>>
>>>> events.py had an info to route, was it by choice that it wasn't before?
>>>
>>> See below.
>>>
>>> I figure this is intentionally below the -- line, but ...
>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>
>>> ... this should be above it.
>>>
>>
>> Script failure. Or human failure, because I used '--' instead of '---'.
>>
>>>> ---
>>>> scripts/qapi/events.py | 2 +-
>>>> scripts/qapi/schema.py | 23 +++++++++++++----------
>>>> scripts/qapi/types.py | 9 +++++----
>>>> scripts/qapi/visit.py | 6 +++---
>>>> 4 files changed, 22 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
>>>> index 9851653b9d11..9ba4f109028d 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))
>>>
>>> We pass None because errors should never happen, and None makes that
>>> quite clear.
>>>
>>
>> Not clear: *why* should errors never happen? This is the only place we
>> pass None for SourceInfo that isn't explicitly a literal built-in type.
>> This is not obvious.
>
> You're right, but there are two separate "unclarities".
>
> Passing None effectively asserts "errors never happen". We neglect to
> explain why, leaving the reader guessing.
>
> Passing @info "fixes" that by removing the assertion. Now we have a
> more subtle problem: will errors make sense with this @info? Normally,
> all members of an enum share one info. Only this enum's members don't.
> It turns out the question is moot because @info is not actually used.
> But will it remain moot? My point isn't that these are important
> questions to answer. It is that we're replacing the relatively obvious
> question "why are we asserting errors can't happen" by more subtle ones.
> Feels like sweeping dirt under the rug.
>
I'll grant you that. I'm going wide instead of deep, here. There were
200+ errors to fix, and not every last one got the same level of attention.
I definitely did just sweep some dirt under the rug.
>>> We don't actually have a built-in QAPISchemaEnumType for the event enum.
>>> We merely generate a C enum QAPIEvent along with macro QAPIEvent_str(),
>>> by passing self._event_emit_name, self._event_enum_members straight to
>>> gen_enum() and gen_enum_lookup().
>>>
>>> If we did build a QAPISchemaEnumType, and used it to diagnose clashes,
>>> then clashes would be reported like
>>>
>>> mumble.json: In event 'A-B':
>>> mumble.json: 2: value 'A-B' collides with value 'A_B'
>>>
>>> leaving you guessing what "value" means, and where 'A_B' may be.
>>>
>>> Bug: we don't diagnose certain event name clashes. I'll fix it.
>>>
>>
>> Not clear to me: If I want interface consistency, what *should* be
>> passed downwards as the info? it's not quite a builtin as much as it is
>> an inferred enum,
>
> True.
>
>> so should I just ... leave it like this, or wait for
>> you to offer a better fix?
>
> Waiting for some better fix feels undavisable. We want to get type
> checking in place sooner rather than later.
>
OK. You mentioned fixing the conflicts, so I had thought maybe that was
near-term instead of long-term. We have cleared that misunderstanding up ;)
> Aside: a possible fix is decoupling gen_enum_lookup() and gen_enum()
> from QAPISchemaEnumMember, so we don't have to make up
> QAPISchemaEnumMembers here.
>
I'm not sure I have the broader picture here to do this, or the time to
focus on it. It's a bit of a deeper fix than the rest of the minor
refactorings I do in these series.
> I think there are two interpretations of your QAPISourceInfo work's aim:
>
> 1. Narrow: use a special QAPISourceInfo rather then None as "no errors
> shall happen" poison.
>
> QAPISourceInfo.builtin() returns this poison. The name "builtin" is
> less than ideal.
>
I did intend it to be used for explicit built-in types; this is an
"inferred" type, and I'd use a differently named poison for it, I suppose.
> 2. Ambitious: eliminate "no errors shall happen".
>
> We're discussing this in reply of PATCH 06. I think we need to reach a
> conclusion there before we can decide here.
>
OK, I'll head over there. I'm still a bit confused over here, but we'll
get to it.
>>>>
>>>>
>>>> def gen_events(schema: QAPISchema,
>>>> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>>>> index 720449feee4d..d5f19732b516 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 builtins (and their arrays), info is a null-object
>>>> + # sentinel 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).
>>>
>>> s/builtins/built-in types/
>>>
>>> The meaning of "a null object sentinel" is less than clear. Perhaps "a
>>> special object".
>>>
>>
>> OK.
>>
>>>> self.info = info
>>>> self.doc = doc
>>>> self._ifcond = ifcond or []
>>>> @@ -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')
>>>> @@ -871,7 +872,7 @@ def resolve_type(self, name, info, what):
>>>> return typ
>>>>
>>>> def _module_name(self, fname):
>>>> - if fname is None:
>>>> + if not fname:
>>>> return None
>>>> return os.path.relpath(fname, self._schema_dir)
>>>>
>>>
>>> Sure this hunk belongs to this patch?
>>>
>>
>> Accident.
>>
>> "info and info.fname" does not behave the same with a falsey info object
>> as it does when info was genuinely None; I compensated for that *here*,
>> but I should have compensated for it elsewhere.
>>
>>>> @@ -897,9 +898,11 @@ 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):
>>>> + info = QAPISourceInfo.builtin()
>>>> +
>>>
>>> Everything else gets its very own copy of the "no source info" object,
>>> except for the stuff defined here, which gets to share one. Isn't that
>>> odd?
>>>
>>
>> It's also the only function where we define so many built-ins in the
>> same place, so spiritually they do have the same SourceInfo, right? :)
>>
>> (OK, no, it wasn't a conscious design choice, but it also seems
>> harmless. I am going to assume you'd prefer I not do this?)
>
> It is harmless. It just made me wonder why we create more than one such
> QAPISourceInfo in the first place. Method builtin() could return the
> same one every time. It could even be an attribute instead of a method.
> Anyway, no big deal.
>
I could conceivably use source line information and stuff, to be
needlessly fancy about it. Nah. I just think singleton patterns are kind
of weird to implement in Python, so I didn't.
--js
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory
2020-12-17 17:02 ` John Snow
@ 2020-12-18 5:24 ` Markus Armbruster
2020-12-18 19:17 ` John Snow
0 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2020-12-18 5:24 UTC (permalink / raw)
To: John Snow
Cc: Marc-André Lureau, qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> On 12/17/20 3:02 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> On 12/16/20 5:18 AM, Markus Armbruster wrote:
>>>> John Snow <jsnow@redhat.com> writes:
>>>>
>>>>> --
>>>>>
>>>>> events.py had an info to route, was it by choice that it wasn't before?
>>>>
>>>> See below.
>>>>
>>>> I figure this is intentionally below the -- line, but ...
>>>>
>>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>>
>>>> ... this should be above it.
>>>>
>>>
>>> Script failure. Or human failure, because I used '--' instead of '---'.
>>>
>>>>> ---
>>>>> scripts/qapi/events.py | 2 +-
>>>>> scripts/qapi/schema.py | 23 +++++++++++++----------
>>>>> scripts/qapi/types.py | 9 +++++----
>>>>> scripts/qapi/visit.py | 6 +++---
>>>>> 4 files changed, 22 insertions(+), 18 deletions(-)
>>>>>
>>>>> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
>>>>> index 9851653b9d11..9ba4f109028d 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))
>>>>
>>>> We pass None because errors should never happen, and None makes that
>>>> quite clear.
>>>>
>>>
>>> Not clear: *why* should errors never happen? This is the only place we
>>> pass None for SourceInfo that isn't explicitly a literal built-in type.
>>> This is not obvious.
>>
>> You're right, but there are two separate "unclarities".
>>
>> Passing None effectively asserts "errors never happen". We neglect to
>> explain why, leaving the reader guessing.
>>
>> Passing @info "fixes" that by removing the assertion. Now we have a
>> more subtle problem: will errors make sense with this @info? Normally,
>> all members of an enum share one info. Only this enum's members don't.
>> It turns out the question is moot because @info is not actually used.
>> But will it remain moot? My point isn't that these are important
>> questions to answer. It is that we're replacing the relatively obvious
>> question "why are we asserting errors can't happen" by more subtle ones.
>> Feels like sweeping dirt under the rug.
>>
>
> I'll grant you that. I'm going wide instead of deep, here. There were
> 200+ errors to fix, and not every last one got the same level of attention.
>
> I definitely did just sweep some dirt under the rug.
>
>>>> We don't actually have a built-in QAPISchemaEnumType for the event enum.
>>>> We merely generate a C enum QAPIEvent along with macro QAPIEvent_str(),
>>>> by passing self._event_emit_name, self._event_enum_members straight to
>>>> gen_enum() and gen_enum_lookup().
>>>>
>>>> If we did build a QAPISchemaEnumType, and used it to diagnose clashes,
>>>> then clashes would be reported like
>>>>
>>>> mumble.json: In event 'A-B':
>>>> mumble.json: 2: value 'A-B' collides with value 'A_B'
>>>>
>>>> leaving you guessing what "value" means, and where 'A_B' may be.
>>>>
>>>> Bug: we don't diagnose certain event name clashes. I'll fix it.
>>>>
>>>
>>> Not clear to me: If I want interface consistency, what *should* be
>>> passed downwards as the info? it's not quite a builtin as much as it is
>>> an inferred enum,
>>
>> True.
>>
>>> so should I just ... leave it like this, or wait for
>>> you to offer a better fix?
>>
>> Waiting for some better fix feels undavisable. We want to get type
>> checking in place sooner rather than later.
>>
>
> OK. You mentioned fixing the conflicts, so I had thought maybe that was
> near-term instead of long-term. We have cleared that misunderstanding up ;)
>
>> Aside: a possible fix is decoupling gen_enum_lookup() and gen_enum()
>> from QAPISchemaEnumMember, so we don't have to make up
>> QAPISchemaEnumMembers here.
>>
>
> I'm not sure I have the broader picture here to do this, or the time to
> focus on it. It's a bit of a deeper fix than the rest of the minor
> refactorings I do in these series.
>
>> I think there are two interpretations of your QAPISourceInfo work's aim:
>>
>> 1. Narrow: use a special QAPISourceInfo rather then None as "no errors
>> shall happen" poison.
>>
>> QAPISourceInfo.builtin() returns this poison. The name "builtin" is
>> less than ideal.
>>
>
> I did intend it to be used for explicit built-in types; this is an
> "inferred" type, and I'd use a differently named poison for it, I suppose.
In the narrow interpretation, I'd use just one poison, and I'd name it
in a way that signals "no error shall happen".
>> 2. Ambitious: eliminate "no errors shall happen".
>>
>> We're discussing this in reply of PATCH 06. I think we need to reach a
>> conclusion there before we can decide here.
>>
>
> OK, I'll head over there. I'm still a bit confused over here, but we'll
> get to it.
Just one more thought: narrow vs. ambitious need not be final. We can
pass "no error shall happen" poison now, and still eliminate or reduce
the poison later on.
>>>>>
>>>>>
>>>>> def gen_events(schema: QAPISchema,
>>>>> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>>>>> index 720449feee4d..d5f19732b516 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 builtins (and their arrays), info is a null-object
>>>>> + # sentinel 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).
>>>>
>>>> s/builtins/built-in types/
>>>>
>>>> The meaning of "a null object sentinel" is less than clear. Perhaps "a
>>>> special object".
>>>>
>>>
>>> OK.
>>>
>>>>> self.info = info
>>>>> self.doc = doc
>>>>> self._ifcond = ifcond or []
>>>>> @@ -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')
>>>>> @@ -871,7 +872,7 @@ def resolve_type(self, name, info, what):
>>>>> return typ
>>>>>
>>>>> def _module_name(self, fname):
>>>>> - if fname is None:
>>>>> + if not fname:
>>>>> return None
>>>>> return os.path.relpath(fname, self._schema_dir)
>>>>>
>>>>
>>>> Sure this hunk belongs to this patch?
>>>>
>>>
>>> Accident.
>>>
>>> "info and info.fname" does not behave the same with a falsey info object
>>> as it does when info was genuinely None; I compensated for that *here*,
>>> but I should have compensated for it elsewhere.
>>>
>>>>> @@ -897,9 +898,11 @@ 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):
>>>>> + info = QAPISourceInfo.builtin()
>>>>> +
>>>>
>>>> Everything else gets its very own copy of the "no source info" object,
>>>> except for the stuff defined here, which gets to share one. Isn't that
>>>> odd?
>>>>
>>>
>>> It's also the only function where we define so many built-ins in the
>>> same place, so spiritually they do have the same SourceInfo, right? :)
>>>
>>> (OK, no, it wasn't a conscious design choice, but it also seems
>>> harmless. I am going to assume you'd prefer I not do this?)
>>
>> It is harmless. It just made me wonder why we create more than one such
>> QAPISourceInfo in the first place. Method builtin() could return the
>> same one every time. It could even be an attribute instead of a method.
>> Anyway, no big deal.
>>
>
> I could conceivably use source line information and stuff, to be
> needlessly fancy about it. Nah. I just think singleton patterns are kind
> of weird to implement in Python, so I didn't.
Stupidest singleton that could possibly work: in __init__,
self.singleton = ...
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory
2020-12-18 5:24 ` Markus Armbruster
@ 2020-12-18 19:17 ` John Snow
2020-12-18 20:57 ` Markus Armbruster
0 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2020-12-18 19:17 UTC (permalink / raw)
To: Markus Armbruster
Cc: Marc-André Lureau, Cleber Rosa, qemu-devel, Eduardo Habkost
On 12/18/20 12:24 AM, Markus Armbruster wrote:
>> I could conceivably use source line information and stuff, to be
>> needlessly fancy about it. Nah. I just think singleton patterns are kind
>> of weird to implement in Python, so I didn't.
> Stupidest singleton that could possibly work: in __init__,
> self.singleton = ...
>
>
Yeah, you can make a class variable that has a builtin singleton, then
make the class method return that class variable.
Feels fancier than my laziness permits. I just put it back to using one
copy per definition.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory
2020-12-18 19:17 ` John Snow
@ 2020-12-18 20:57 ` Markus Armbruster
2020-12-18 21:30 ` John Snow
0 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2020-12-18 20:57 UTC (permalink / raw)
To: John Snow
Cc: Marc-André Lureau, qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> On 12/18/20 12:24 AM, Markus Armbruster wrote:
>>> I could conceivably use source line information and stuff, to be
>>> needlessly fancy about it. Nah. I just think singleton patterns are kind
>>> of weird to implement in Python, so I didn't.
>> Stupidest singleton that could possibly work: in __init__,
>> self.singleton = ...
>>
>
> Yeah, you can make a class variable that has a builtin singleton, then
> make the class method return that class variable.
>
> Feels fancier than my laziness permits. I just put it back to using
> one copy per definition.
Why have a class method around the attribute? Just use the stoopid
attribute already ;-P
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory
2020-12-18 20:57 ` Markus Armbruster
@ 2020-12-18 21:30 ` John Snow
0 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2020-12-18 21:30 UTC (permalink / raw)
To: Markus Armbruster
Cc: Marc-André Lureau, qemu-devel, Eduardo Habkost, Cleber Rosa
On 12/18/20 3:57 PM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> On 12/18/20 12:24 AM, Markus Armbruster wrote:
>>>> I could conceivably use source line information and stuff, to be
>>>> needlessly fancy about it. Nah. I just think singleton patterns are kind
>>>> of weird to implement in Python, so I didn't.
>>> Stupidest singleton that could possibly work: in __init__,
>>> self.singleton = ...
>>>
>>
>> Yeah, you can make a class variable that has a builtin singleton, then
>> make the class method return that class variable.
>>
>> Feels fancier than my laziness permits. I just put it back to using
>> one copy per definition.
>
> Why have a class method around the attribute? Just use the stoopid
> attribute already ;-P
>
Something has to initialize it:
```
class Blah:
magic = Blah()
def __init__(self):
pass
```
Won't work; Blah isn't defined yet. a classmethod works though:
```
class Blah:
magic = None
def __init__(self) -> None:
pass
@classmethod
def make(cls) -> 'Blah':
if cls.magic is None:
cls.magic = cls()
return cls.magic
```
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 09/12] qapi/gen: move write method to QAPIGenC, make fname a str
2020-12-14 23:53 [PATCH 00/12] qapi: static typing conversion, pt1.5 John Snow
` (7 preceding siblings ...)
2020-12-14 23:53 ` [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory John Snow
@ 2020-12-14 23:53 ` John Snow
2020-12-16 10:31 ` Markus Armbruster
2020-12-14 23:53 ` [PATCH 10/12] tests/qapi-schema: Add quotes to module name in test output John Snow
` (2 subsequent siblings)
11 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2020-12-14 23:53 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, Cleber Rosa, John Snow, Eduardo Habkost,
Markus Armbruster
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>
---
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 71744f48a353..b346676d15a0 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 17ae9f4af703..b2c89213d838 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] 41+ messages in thread* Re: [PATCH 09/12] qapi/gen: move write method to QAPIGenC, make fname a str
2020-12-14 23:53 ` [PATCH 09/12] qapi/gen: move write method to QAPIGenC, make fname a str John Snow
@ 2020-12-16 10:31 ` Markus Armbruster
0 siblings, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2020-12-16 10:31 UTC (permalink / raw)
To: John Snow
Cc: Marc-André Lureau, qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> 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.
Which one?
Hmm, it's this one:
class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
def __init__(self, prefix: str):
super().__init__(
prefix, 'qapi-commands',
' * Schema-defined QAPI/QMP commands', None, __doc__)
---> self._regy = QAPIGenCCode(None)
self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]] = {}
Let me try to get rid of it.
> 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>
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 10/12] tests/qapi-schema: Add quotes to module name in test output
2020-12-14 23:53 [PATCH 00/12] qapi: static typing conversion, pt1.5 John Snow
` (8 preceding siblings ...)
2020-12-14 23:53 ` [PATCH 09/12] qapi/gen: move write method to QAPIGenC, make fname a str John Snow
@ 2020-12-14 23:53 ` John Snow
2020-12-14 23:53 ` [PATCH 11/12] qapi/schema: Name the builtin module "" instead of None John Snow
2020-12-14 23:53 ` [PATCH 12/12] qapi: enable strict-optional checks John Snow
11 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2020-12-14 23:53 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, Cleber Rosa, John Snow, Eduardo Habkost,
Markus Armbruster
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 273f0f54e16c..08aba8354e2a 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 419284dae29b..83a3d9bd69b5 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 69666c39ad2d..0dac23c80c15 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 42ae519656dc..ace511ba5a96 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 0b654ddebb6a..f7ab4987943c 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 061f81e50904..81bdeb887b66 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 04356775cd16..361a58185e67 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 8868ca0dca9e..4f5ab9fd596c 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 e8db9d09d914..4adf0b3c1857 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] 41+ messages in thread* [PATCH 11/12] qapi/schema: Name the builtin module "" instead of None
2020-12-14 23:53 [PATCH 00/12] qapi: static typing conversion, pt1.5 John Snow
` (9 preceding siblings ...)
2020-12-14 23:53 ` [PATCH 10/12] tests/qapi-schema: Add quotes to module name in test output John Snow
@ 2020-12-14 23:53 ` John Snow
2020-12-16 10:42 ` Markus Armbruster
2020-12-14 23:53 ` [PATCH 12/12] qapi: enable strict-optional checks John Snow
11 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2020-12-14 23:53 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, Cleber Rosa, John Snow, Eduardo Habkost,
Markus Armbruster
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 b2c89213d838..a577a4a7f002 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -309,14 +309,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 d5f19732b516..8d8b8758f65e 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 and info.fname)
+ 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 not fname:
- 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 a3a16284006b..12eeea3aaffe 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 3f49c307c566..76e34ee7f02e 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 08aba8354e2a..02000c06e5e0 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 83a3d9bd69b5..494533d74793 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 0dac23c80c15..059caa4e1d2a 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 ace511ba5a96..4d9d2b519f4b 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 f7ab4987943c..31d64631b665 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 81bdeb887b66..1b35b3295713 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 361a58185e67..aed689e7bd67 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 4f5ab9fd596c..4a899ba63ecb 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] 41+ messages in thread* Re: [PATCH 11/12] qapi/schema: Name the builtin module "" instead of None
2020-12-14 23:53 ` [PATCH 11/12] qapi/schema: Name the builtin module "" instead of None John Snow
@ 2020-12-16 10:42 ` Markus Armbruster
2020-12-16 18:57 ` John Snow
0 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2020-12-16 10:42 UTC (permalink / raw)
To: John Snow
Cc: Marc-André Lureau, qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> Instead of using None as the built-in module filename, use an empty
> string instead.
PATCH 05's changes the module name of the special system module for
built-in stuff from None to './builtin'. The other system modules are
named like './FOO': './init' and './emit' currently.
This one changes the module *filename* from None to "", and not just for
the builtin module, but for *all* system modules. Your commit message
claims only "the built-in module", which is not true as far as I can
tell.
Should we use the opportunity to make the filename match the module
name?
> 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>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 11/12] qapi/schema: Name the builtin module "" instead of None
2020-12-16 10:42 ` Markus Armbruster
@ 2020-12-16 18:57 ` John Snow
2020-12-17 11:09 ` Markus Armbruster
0 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2020-12-16 18:57 UTC (permalink / raw)
To: Markus Armbruster
Cc: Marc-André Lureau, qemu-devel, Eduardo Habkost, Cleber Rosa
On 12/16/20 5:42 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Instead of using None as the built-in module filename, use an empty
>> string instead.
>
> PATCH 05's changes the module name of the special system module for
> built-in stuff from None to './builtin'. The other system modules are
> named like './FOO': './init' and './emit' currently.
>
> This one changes the module *filename* from None to "", and not just for
> the builtin module, but for *all* system modules. Your commit message
> claims only "the built-in module", which is not true as far as I can
> tell.
>
Is this true? ... "./init" and "./emit" are defined only in the
generators, outside of the schema entirely. They don't even have
"QAPISchemaModule" objects associated with them.
I changed:
self._make_module(None) # built-ins
to
self._make_module(QAPISourceInfo.builtin().fname) # built-ins
that should be precisely only "the" built-in module, shouldn't it? the
other "system" modules don't even pass through _make_module.
> Should we use the opportunity to make the filename match the module
> name?
>
If that's something you want to have happen, it can be done, yes.
I had a draft that did it that way initially; I was afraid I was mixing
up two distinct things: the module fname (schema.py's concept of
modules) and module name (gen.py's concept of modules). This version of
my patch kept the two more distinct like they are currently.
We can change "the" built-in module to have an fname of "./builtin"
which works just fine; gen.py just has to change to not add "./" to
modules already declared with the leading slash.
Up for debate is if you want the system modules declared in the code
generators to have to declare 'emit' or './emit'; I left them alone and
allowed them to say 'event'.
Downside: the ./ prefix takes on special meaning in both gen.py and
schema.py. the module organization feels decentralized and fragile.
>> 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>
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 11/12] qapi/schema: Name the builtin module "" instead of None
2020-12-16 18:57 ` John Snow
@ 2020-12-17 11:09 ` Markus Armbruster
2020-12-17 21:07 ` John Snow
0 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2020-12-17 11:09 UTC (permalink / raw)
To: John Snow
Cc: Marc-André Lureau, Cleber Rosa, qemu-devel, Eduardo Habkost
John Snow <jsnow@redhat.com> writes:
> On 12/16/20 5:42 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> Instead of using None as the built-in module filename, use an empty
>>> string instead.
>>
>> PATCH 05's changes the module name of the special system module for
>> built-in stuff from None to './builtin'. The other system modules are
>> named like './FOO': './init' and './emit' currently.
>>
>> This one changes the module *filename* from None to "", and not just for
>> the builtin module, but for *all* system modules. Your commit message
>> claims only "the built-in module", which is not true as far as I can
>> tell.
>>
>
> Is this true? ... "./init" and "./emit" are defined only in the
> generators, outside of the schema entirely. They don't even have
> "QAPISchemaModule" objects associated with them.
>
> I changed:
>
> self._make_module(None) # built-ins
>
>
> to
>
> self._make_module(QAPISourceInfo.builtin().fname) # built-ins
>
>
>
> that should be precisely only "the" built-in module, shouldn't it? the
> other "system" modules don't even pass through _make_module.
You're right.
The schema IR has only user-defined modules and the built-ins module.
Each module has a name. We use the source file name for the
user-defined modules. The built-ins module has none, so we use None.
The QAPISchemaModularCVisitor generates "modular" output. It has
per-module data, keyed by module name. It supports user-defined and
system modules. We set them up as follows. The user-defined modules
are exactly the schema IR's (same name). The system modules are the
schema IR's built-ins module (same name) plus whatever the user of
QAPISchemaModularCVisitor adds (convention: name starts with ./, so it
can't clash).
Why let generators add system modules that don't exist in the schema IR?
It's a reasonably clean way to generate stuff that doesn't fit elsewhere
into separate .[ch] files.
PATCH 05 changes the name of the built-ins module from None to
'./builtins' in the QAPISchemaModularCVisitor only. Correct?
This patch changes the name of the built-ins module from None to "" in
the schema IR only. Correct?
>> Should we use the opportunity to make the filename match the module
>> name?
>>
>
> If that's something you want to have happen, it can be done, yes.
Having different "module names" for the built-ins module in different
places could be confusing.
The issue appears in PATCH 05. I'm fine with the two module names
diverging temporarily in this series. I'd prefer them to be the same at
the end.
> I had a draft that did it that way initially; I was afraid I was mixing
> up two distinct things: the module fname (schema.py's concept of
> modules) and module name (gen.py's concept of modules). This version of
> my patch kept the two more distinct like they are currently.
>
> We can change "the" built-in module to have an fname of "./builtin"
> which works just fine; gen.py just has to change to not add "./" to
> modules already declared with the leading slash.
>
> Up for debate is if you want the system modules declared in the code
> generators to have to declare 'emit' or './emit'; I left them alone and
> allowed them to say 'event'.
I pass 'emit' to argument name, so a simple './' + name obviously
results in a system module name.
With './emit', we achieve "obviously" by assert name.startswith('./').
Matter of taste.
> Downside: the ./ prefix takes on special meaning in both gen.py and
> schema.py. the module organization feels decentralized and fragile.
Making the './' prefix special is fine. But you're right, doing it in
two places and with precious little explanation is not so fine.
We could have schema.py provide some notion of "module name". Weasel
words "some notion" for artistic license.
If we'd prefer not to do it now, a few judicious comments should suffice
for now.
>>> 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>
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 11/12] qapi/schema: Name the builtin module "" instead of None
2020-12-17 11:09 ` Markus Armbruster
@ 2020-12-17 21:07 ` John Snow
2020-12-18 5:31 ` Markus Armbruster
0 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2020-12-17 21:07 UTC (permalink / raw)
To: Markus Armbruster
Cc: Marc-André Lureau, Cleber Rosa, qemu-devel, Eduardo Habkost
On 12/17/20 6:09 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> On 12/16/20 5:42 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> Instead of using None as the built-in module filename, use an empty
>>>> string instead.
>>>
>>> PATCH 05's changes the module name of the special system module for
>>> built-in stuff from None to './builtin'. The other system modules are
>>> named like './FOO': './init' and './emit' currently.
>>>
>>> This one changes the module *filename* from None to "", and not just for
>>> the builtin module, but for *all* system modules. Your commit message
>>> claims only "the built-in module", which is not true as far as I can
>>> tell.
>>>
>>
>> Is this true? ... "./init" and "./emit" are defined only in the
>> generators, outside of the schema entirely. They don't even have
>> "QAPISchemaModule" objects associated with them.
>>
>> I changed:
>>
>> self._make_module(None) # built-ins
>>
>>
>> to
>>
>> self._make_module(QAPISourceInfo.builtin().fname) # built-ins
>>
>>
>>
>> that should be precisely only "the" built-in module, shouldn't it? the
>> other "system" modules don't even pass through _make_module.
>
> You're right.
>
> The schema IR has only user-defined modules and the built-ins module.
> Each module has a name. We use the source file name for the
> user-defined modules. The built-ins module has none, so we use None.
>
> The QAPISchemaModularCVisitor generates "modular" output. It has
> per-module data, keyed by module name. It supports user-defined and
> system modules. We set them up as follows. The user-defined modules
> are exactly the schema IR's (same name). The system modules are the
> schema IR's built-ins module (same name) plus whatever the user of
> QAPISchemaModularCVisitor adds (convention: name starts with ./, so it
> can't clash).
>
> Why let generators add system modules that don't exist in the schema IR?
> It's a reasonably clean way to generate stuff that doesn't fit elsewhere
> into separate .[ch] files.
>
> PATCH 05 changes the name of the built-ins module from None to
> './builtins' in the QAPISchemaModularCVisitor only. Correct?
>
That's right. That was a useful change all by itself, and winds up being
useful for the genc/genh typing.
> This patch changes the name of the built-ins module from None to "" in
> the schema IR only. Correct?
>
As far as I know, yes. ("Schema IR -- Internal Registry?")
>>> Should we use the opportunity to make the filename match the module
>>> name?
>>>
>>
>> If that's something you want to have happen, it can be done, yes.
>
> Having different "module names" for the built-ins module in different
> places could be confusing.
>
Yes, but we technically didn't use the generator-only names in the
Schema before, so I didn't want to assume we'd want them here.
> The issue appears in PATCH 05. I'm fine with the two module names
> diverging temporarily in this series. I'd prefer them to be the same at
> the end.
>
OK, no problem. I'll try to make this nicer and unify things just a
pinch more.
--js
>> I had a draft that did it that way initially; I was afraid I was mixing
>> up two distinct things: the module fname (schema.py's concept of
>> modules) and module name (gen.py's concept of modules). This version of
>> my patch kept the two more distinct like they are currently.
>>
>> We can change "the" built-in module to have an fname of "./builtin"
>> which works just fine; gen.py just has to change to not add "./" to
>> modules already declared with the leading slash.
>>
>> Up for debate is if you want the system modules declared in the code
>> generators to have to declare 'emit' or './emit'; I left them alone and
>> allowed them to say 'event'.
>
> I pass 'emit' to argument name, so a simple './' + name obviously
> results in a system module name.
>
> With './emit', we achieve "obviously" by assert name.startswith('./').
> Matter of taste.
>
>> Downside: the ./ prefix takes on special meaning in both gen.py and
>> schema.py. the module organization feels decentralized and fragile.
>
> Making the './' prefix special is fine. But you're right, doing it in
> two places and with precious little explanation is not so fine.
>
> We could have schema.py provide some notion of "module name". Weasel
> words "some notion" for artistic license.
>
> If we'd prefer not to do it now, a few judicious comments should suffice
> for now.
>
>>>> 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>
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 11/12] qapi/schema: Name the builtin module "" instead of None
2020-12-17 21:07 ` John Snow
@ 2020-12-18 5:31 ` Markus Armbruster
0 siblings, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2020-12-18 5:31 UTC (permalink / raw)
To: John Snow
Cc: Marc-André Lureau, qemu-devel, Eduardo Habkost, Cleber Rosa
John Snow <jsnow@redhat.com> writes:
> On 12/17/20 6:09 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> On 12/16/20 5:42 AM, Markus Armbruster wrote:
>>>> John Snow <jsnow@redhat.com> writes:
>>>>
>>>>> Instead of using None as the built-in module filename, use an empty
>>>>> string instead.
>>>>
>>>> PATCH 05's changes the module name of the special system module for
>>>> built-in stuff from None to './builtin'. The other system modules are
>>>> named like './FOO': './init' and './emit' currently.
>>>>
>>>> This one changes the module *filename* from None to "", and not just for
>>>> the builtin module, but for *all* system modules. Your commit message
>>>> claims only "the built-in module", which is not true as far as I can
>>>> tell.
>>>>
>>>
>>> Is this true? ... "./init" and "./emit" are defined only in the
>>> generators, outside of the schema entirely. They don't even have
>>> "QAPISchemaModule" objects associated with them.
>>>
>>> I changed:
>>>
>>> self._make_module(None) # built-ins
>>>
>>>
>>> to
>>>
>>> self._make_module(QAPISourceInfo.builtin().fname) # built-ins
>>>
>>>
>>>
>>> that should be precisely only "the" built-in module, shouldn't it? the
>>> other "system" modules don't even pass through _make_module.
>>
>> You're right.
>>
>> The schema IR has only user-defined modules and the built-ins module.
>> Each module has a name. We use the source file name for the
>> user-defined modules. The built-ins module has none, so we use None.
>>
>> The QAPISchemaModularCVisitor generates "modular" output. It has
>> per-module data, keyed by module name. It supports user-defined and
>> system modules. We set them up as follows. The user-defined modules
>> are exactly the schema IR's (same name). The system modules are the
>> schema IR's built-ins module (same name) plus whatever the user of
>> QAPISchemaModularCVisitor adds (convention: name starts with ./, so it
>> can't clash).
>>
>> Why let generators add system modules that don't exist in the schema IR?
>> It's a reasonably clean way to generate stuff that doesn't fit elsewhere
>> into separate .[ch] files.
>>
>> PATCH 05 changes the name of the built-ins module from None to
>> './builtins' in the QAPISchemaModularCVisitor only. Correct?
>>
>
> That's right. That was a useful change all by itself, and winds up being
> useful for the genc/genh typing.
>
>> This patch changes the name of the built-ins module from None to "" in
>> the schema IR only. Correct?
>>
>
> As far as I know, yes. ("Schema IR -- Internal Registry?")
Internal representation (the stuff in schema.py). Compiler jargon,
sorry.
>>>> Should we use the opportunity to make the filename match the module
>>>> name?
>>>>
>>>
>>> If that's something you want to have happen, it can be done, yes.
>>
>> Having different "module names" for the built-ins module in different
>> places could be confusing.
>>
>
> Yes, but we technically didn't use the generator-only names in the
> Schema before, so I didn't want to assume we'd want them here.
We can't have them there, because the things they name don't exist
there.
What we can have is a consistent naming convention that leads to same
things having the same names in both places.
>> The issue appears in PATCH 05. I'm fine with the two module names
>> diverging temporarily in this series. I'd prefer them to be the same at
>> the end.
>>
>
> OK, no problem. I'll try to make this nicer and unify things just a
> pinch more.
>
> --js
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 12/12] qapi: enable strict-optional checks
2020-12-14 23:53 [PATCH 00/12] qapi: static typing conversion, pt1.5 John Snow
` (10 preceding siblings ...)
2020-12-14 23:53 ` [PATCH 11/12] qapi/schema: Name the builtin module "" instead of None John Snow
@ 2020-12-14 23:53 ` John Snow
11 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2020-12-14 23:53 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, Cleber Rosa, John Snow, Eduardo Habkost,
Markus Armbruster
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 74fc6c82153e..04bd5db5278d 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] 41+ messages in thread