From ec9697ab3fe2174a865d0ac2bbc572cbd5981d94 Mon Sep 17 00:00:00 2001 From: John Snow Date: Mon, 1 Feb 2021 14:37:32 -0500 Subject: qapi/commands: assert arg_type is not None 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 Message-Id: <20210201193747.2169670-2-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- 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 50978090b4..71744f48a3 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 @@ out: 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); -- cgit v1.2.3 From 3cc01c546ba508222e0ccda775bce30e07c0461f Mon Sep 17 00:00:00 2001 From: John Snow Date: Mon, 1 Feb 2021 14:37:33 -0500 Subject: qapi/events: fix visit_event typing Actually, the arg_type can indeed be Optional. Signed-off-by: John Snow Message-Id: <20210201193747.2169670-3-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- 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 599f3d1f56..9851653b9d 100644 --- a/scripts/qapi/events.py +++ b/scripts/qapi/events.py @@ -12,7 +12,7 @@ This work is licensed under the terms of the GNU GPL, version 2. 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 @@ from .types import gen_enum, gen_enum_lookup 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 @@ void %(event_emit)s(%(event_enum)s event, QDict *qdict); 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)) -- cgit v1.2.3 From ad1218086efb469d292f5cdd3e8299e5d2e7d8d6 Mon Sep 17 00:00:00 2001 From: John Snow Date: Mon, 1 Feb 2021 14:37:34 -0500 Subject: qapi/main: handle theoretical None-return from re.match() Mypy cannot understand that this match can never be None, so help it along. Signed-off-by: John Snow Message-Id: <20210201193747.2169670-4-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/main.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py index 42517210b8..703e7ed1ed 100644 --- a/scripts/qapi/main.py +++ b/scripts/qapi/main.py @@ -23,6 +23,8 @@ from .visit import gen_visit def invalid_prefix_char(prefix: str) -> Optional[str]: match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix) + # match cannot be None, but mypy cannot infer that. + assert match is not None if match.end() != len(prefix): return prefix[match.end()] return None -- cgit v1.2.3 From a253b3eb9a194a0b2e8b55095ce5f28c2d5cfa11 Mon Sep 17 00:00:00 2001 From: John Snow Date: Mon, 1 Feb 2021 14:37:35 -0500 Subject: qapi/gen: inline _wrap_ifcond into end_if() We assert _start_if is not None in end_if, but that's opaque to mypy. By inlining _wrap_ifcond, that constraint becomes provable to mypy. Signed-off-by: John Snow Message-Id: <20210201193747.2169670-5-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/gen.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py index b40f18eee3..3d81b90ab7 100644 --- a/scripts/qapi/gen.py +++ b/scripts/qapi/gen.py @@ -130,15 +130,12 @@ class QAPIGenCCode(QAPIGen): 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 is not None self._body = _wrap_ifcond(self._start_if[0], self._start_if[1], self._body) self._preamble = _wrap_ifcond(self._start_if[0], self._start_if[2], self._preamble) + self._start_if = None def get_content(self) -> str: assert self._start_if is None -- cgit v1.2.3 From 98967c248c4c01085af2ff49ed3d378f79019902 Mon Sep 17 00:00:00 2001 From: John Snow Date: Mon, 1 Feb 2021 14:37:36 -0500 Subject: qapi: centralize is_[user|system|builtin]_module methods Define what a module is and define what kind of a module it is once and for all, in one place. Signed-off-by: John Snow Message-Id: <20210201193747.2169670-6-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/gen.py | 25 +++++++++++-------------- scripts/qapi/schema.py | 31 +++++++++++++++++++++++++++++-- 2 files changed, 40 insertions(+), 16 deletions(-) diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py index 3d81b90ab7..2aec6d3436 100644 --- a/scripts/qapi/gen.py +++ b/scripts/qapi/gen.py @@ -31,7 +31,11 @@ from .common import ( guardstart, mcgen, ) -from .schema import QAPISchemaObjectType, QAPISchemaVisitor +from .schema import ( + QAPISchemaModule, + QAPISchemaObjectType, + QAPISchemaVisitor, +) from .source import QAPISourceInfo @@ -246,21 +250,14 @@ class QAPISchemaModularCVisitor(QAPISchemaVisitor): self._main_module: Optional[str] = None @staticmethod - def _is_user_module(name: Optional[str]) -> bool: - return bool(name and not name.startswith('./')) - - @staticmethod - def _is_builtin_module(name: Optional[str]) -> bool: - return not name - - def _module_dirname(self, name: Optional[str]) -> str: - if self._is_user_module(name): + def _module_dirname(name: Optional[str]) -> str: + if QAPISchemaModule.is_user_module(name): return os.path.dirname(name) return '' def _module_basename(self, what: str, name: Optional[str]) -> str: - ret = '' if self._is_builtin_module(name) else self._prefix - if self._is_user_module(name): + ret = '' if QAPISchemaModule.is_builtin_module(name) else self._prefix + if QAPISchemaModule.is_user_module(name): basename = os.path.basename(name) ret += what if name != self._main_module: @@ -282,7 +279,7 @@ class QAPISchemaModularCVisitor(QAPISchemaVisitor): self._genc, self._genh = self._module[name] def _add_user_module(self, name: str, blurb: str) -> None: - assert self._is_user_module(name) + assert QAPISchemaModule.is_user_module(name) if self._main_module is None: self._main_module = name self._add_module(name, blurb) @@ -292,7 +289,7 @@ class QAPISchemaModularCVisitor(QAPISchemaVisitor): def write(self, output_dir: str, opt_builtins: bool = False) -> None: for name in self._module: - if self._is_builtin_module(name) and not opt_builtins: + if QAPISchemaModule.is_builtin_module(name) and not opt_builtins: continue (genc, genh) = self._module[name] genc.write(output_dir) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 720449feee..e80d9320ed 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -141,6 +141,33 @@ class QAPISchemaModule: self.name = name self._entity_list = [] + @staticmethod + def is_system_module(name: Optional[str]) -> bool: + """ + System modules are internally defined modules. + + Their names start with the "./" prefix. + """ + return name is None or name.startswith('./') + + @classmethod + def is_user_module(cls, name: Optional[str]) -> bool: + """ + User modules are those defined by the user in qapi JSON files. + + They do not start with the "./" prefix. + """ + return not cls.is_system_module(name) + + @staticmethod + def is_builtin_module(name: Optional[str]) -> bool: + """ + The built-in module is a single System module for the built-in types. + + It is presently always the value 'None'. + """ + return name is None + def add_entity(self, ent): self._entity_list.append(ent) @@ -871,8 +898,8 @@ class QAPISchema: return typ def _module_name(self, fname): - if fname is None: - return None + if QAPISchemaModule.is_system_module(fname): + return fname return os.path.relpath(fname, self._schema_dir) def _make_module(self, fname): -- cgit v1.2.3 From f3a705928a7b1825678ff510843702652bc15f1a Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 1 Feb 2021 14:37:37 -0500 Subject: qapi/gen: Replace ._begin_system_module() QAPISchemaModularCVisitor._begin_system_module() is actually just for the builtin module. Rename it to ._begin_builtin_module() and drop its useless @name parameter. Clarify conditionals in visit_module to make this clear. Signed-off-by: Markus Armbruster Signed-off-by: John Snow Message-Id: <20210201193747.2169670-7-jsnow@redhat.com> --- scripts/qapi/gen.py | 9 +++++---- scripts/qapi/types.py | 2 +- scripts/qapi/visit.py | 2 +- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py index 2aec6d3436..aaed78eed5 100644 --- a/scripts/qapi/gen.py +++ b/scripts/qapi/gen.py @@ -295,23 +295,24 @@ class QAPISchemaModularCVisitor(QAPISchemaVisitor): genc.write(output_dir) genh.write(output_dir) - def _begin_system_module(self, name: None) -> None: + def _begin_builtin_module(self) -> None: pass def _begin_user_module(self, name: str) -> None: pass def visit_module(self, name: Optional[str]) -> None: - if name is None: + if QAPISchemaModule.is_builtin_module(name): if self._builtin_blurb: - self._add_system_module(None, self._builtin_blurb) - self._begin_system_module(name) + self._add_system_module(name, self._builtin_blurb) + self._begin_builtin_module() else: # The built-in module has not been created. No code may # be generated. self._genc = None self._genh = None else: + assert QAPISchemaModule.is_user_module(name) self._add_user_module(name, self._user_blurb) self._begin_user_module(name) diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py index 2b4916cdaa..dbf58c0b91 100644 --- a/scripts/qapi/types.py +++ b/scripts/qapi/types.py @@ -271,7 +271,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor): prefix, 'qapi-types', ' * Schema-defined QAPI types', ' * Built-in QAPI types', __doc__) - def _begin_system_module(self, name: None) -> None: + def _begin_builtin_module(self) -> 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 339f152152..568ba35592 100644 --- a/scripts/qapi/visit.py +++ b/scripts/qapi/visit.py @@ -305,7 +305,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor): prefix, 'qapi-visit', ' * Schema-defined QAPI visitors', ' * Built-in QAPI visitors', __doc__) - def _begin_system_module(self, name: None) -> None: + def _begin_builtin_module(self) -> None: self._genc.preamble_add(mcgen(''' #include "qemu/osdep.h" #include "qapi/error.h" -- cgit v1.2.3 From 12893a8ea7163e871abae05b5a42cdce1ad45225 Mon Sep 17 00:00:00 2001 From: John Snow Date: Mon, 1 Feb 2021 14:37:38 -0500 Subject: qapi: use explicitly internal module names QAPISchemaModularCVisitor._add_system_module() prefixes './' to its name argument to make it a module name. Pass the module name instead. This will allow us to coalesce the methods to add modules later on. Signed-off-by: John Snow Message-Id: <20210201193747.2169670-8-jsnow@redhat.com> Reviewed-by: Markus Armbruster [Commit message reworded] Signed-off-by: Markus Armbruster --- scripts/qapi/commands.py | 2 +- scripts/qapi/events.py | 2 +- scripts/qapi/gen.py | 3 ++- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py index 71744f48a3..fc5fe27c47 100644 --- a/scripts/qapi/commands.py +++ b/scripts/qapi/commands.py @@ -286,7 +286,7 @@ class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor): types=types)) def visit_end(self) -> None: - self._add_system_module('init', ' * QAPI Commands initialization') + self._add_system_module('./init', ' * QAPI Commands initialization') self._genh.add(mcgen(''' #include "qapi/qmp/dispatch.h" diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py index 9851653b9d..26faa82989 100644 --- a/scripts/qapi/events.py +++ b/scripts/qapi/events.py @@ -191,7 +191,7 @@ class QAPISchemaGenEventVisitor(QAPISchemaModularCVisitor): types=types)) def visit_end(self) -> None: - self._add_system_module('emit', ' * QAPI Events emission') + self._add_system_module('./emit', ' * QAPI Events emission') self._genc.preamble_add(mcgen(''' #include "qemu/osdep.h" #include "%(prefix)sqapi-emit-events.h" diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py index aaed78eed5..da9d4d2d37 100644 --- a/scripts/qapi/gen.py +++ b/scripts/qapi/gen.py @@ -285,7 +285,8 @@ class QAPISchemaModularCVisitor(QAPISchemaVisitor): self._add_module(name, blurb) def _add_system_module(self, name: Optional[str], blurb: str) -> None: - self._add_module(name and './' + name, blurb) + assert QAPISchemaModule.is_system_module(name) + self._add_module(name, blurb) def write(self, output_dir: str, opt_builtins: bool = False) -> None: for name in self._module: -- cgit v1.2.3 From e2bbc4eaa7f0d8efb8f49705bae0fecd3356f417 Mon Sep 17 00:00:00 2001 From: John Snow Date: Mon, 1 Feb 2021 14:37:39 -0500 Subject: qapi: use './builtin' as the built-in module name Use './builtin' as the built-in module name instead of None. Clarify the typing that this is now always a string. Signed-off-by: John Snow Message-Id: <20210201193747.2169670-9-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/gen.py | 18 +++++++++--------- scripts/qapi/schema.py | 20 ++++++++++---------- tests/qapi-schema/comments.out | 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 +- 10 files changed, 27 insertions(+), 27 deletions(-) diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py index da9d4d2d37..9352d79c3a 100644 --- a/scripts/qapi/gen.py +++ b/scripts/qapi/gen.py @@ -246,16 +246,16 @@ class QAPISchemaModularCVisitor(QAPISchemaVisitor): 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 _module_dirname(name: Optional[str]) -> str: + def _module_dirname(name: str) -> str: if QAPISchemaModule.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 QAPISchemaModule.is_builtin_module(name) else self._prefix if QAPISchemaModule.is_user_module(name): basename = os.path.basename(name) @@ -263,15 +263,15 @@ class QAPISchemaModularCVisitor(QAPISchemaVisitor): if name != self._main_module: ret += '-' + os.path.splitext(basename)[0] else: - name = name[2:] if name else 'builtin' - ret += re.sub(r'-', '-' + name + '-', what) + assert QAPISchemaModule.is_system_module(name) + 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) @@ -284,7 +284,7 @@ class QAPISchemaModularCVisitor(QAPISchemaVisitor): self._main_module = name self._add_module(name, blurb) - def _add_system_module(self, name: Optional[str], blurb: str) -> None: + def _add_system_module(self, name: str, blurb: str) -> None: assert QAPISchemaModule.is_system_module(name) self._add_module(name, blurb) @@ -302,7 +302,7 @@ class QAPISchemaModularCVisitor(QAPISchemaVisitor): def _begin_user_module(self, name: str) -> None: pass - def visit_module(self, name: Optional[str]) -> None: + def visit_module(self, name: str) -> None: if QAPISchemaModule.is_builtin_module(name): if self._builtin_blurb: self._add_system_module(name, self._builtin_blurb) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index e80d9320ed..14cf9da784 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -68,7 +68,8 @@ class QAPISchemaEntity: def _set_module(self, schema, info): assert self._checked - self._module = schema.module_by_fname(info and info.fname) + fname = info.fname if info else './builtin' + self._module = schema.module_by_fname(fname) self._module.add_entity(self) def set_module(self, schema): @@ -142,16 +143,16 @@ class QAPISchemaModule: self._entity_list = [] @staticmethod - def is_system_module(name: Optional[str]) -> bool: + def is_system_module(name: str) -> bool: """ System modules are internally defined modules. Their names start with the "./" prefix. """ - return name is None or name.startswith('./') + return name.startswith('./') @classmethod - def is_user_module(cls, name: Optional[str]) -> bool: + def is_user_module(cls, name: str) -> bool: """ User modules are those defined by the user in qapi JSON files. @@ -160,13 +161,13 @@ class QAPISchemaModule: return not cls.is_system_module(name) @staticmethod - def is_builtin_module(name: Optional[str]) -> bool: + def is_builtin_module(name: str) -> bool: """ The built-in module is a single System module for the built-in types. - It is presently always the value 'None'. + It is always "./builtin". """ - return name is None + return name == './builtin' def add_entity(self, ent): self._entity_list.append(ent) @@ -852,7 +853,7 @@ class QAPISchema: self._entity_dict = {} self._module_dict = OrderedDict() self._schema_dir = os.path.dirname(fname) - self._make_module(None) # built-ins + self._make_module('./builtin') self._make_module(fname) self._predefining = True self._def_predefineds() @@ -897,7 +898,7 @@ class QAPISchema: info, "%s uses unknown type '%s'" % (what, name)) return typ - def _module_name(self, fname): + def _module_name(self, fname: str) -> str: if QAPISchemaModule.is_system_module(fname): return fname return os.path.relpath(fname, self._schema_dir) @@ -910,7 +911,6 @@ class QAPISchema: def module_by_fname(self, fname): name = self._module_name(fname) - assert name in self._module_dict return self._module_dict[name] def _def_include(self, expr, info, doc): diff --git a/tests/qapi-schema/comments.out b/tests/qapi-schema/comments.out index 273f0f54e1..ce4f6a4f0f 100644 --- a/tests/qapi-schema/comments.out +++ b/tests/qapi-schema/comments.out @@ -1,4 +1,4 @@ -module None +module ./builtin object q_empty enum QType prefix QTYPE diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out index 419284dae2..715b0bbc1a 100644 --- a/tests/qapi-schema/doc-good.out +++ b/tests/qapi-schema/doc-good.out @@ -1,4 +1,4 @@ -module None +module ./builtin object q_empty enum QType prefix QTYPE diff --git a/tests/qapi-schema/empty.out b/tests/qapi-schema/empty.out index 69666c39ad..3feb3f69d3 100644 --- a/tests/qapi-schema/empty.out +++ b/tests/qapi-schema/empty.out @@ -1,4 +1,4 @@ -module None +module ./builtin object q_empty enum QType prefix QTYPE diff --git a/tests/qapi-schema/event-case.out b/tests/qapi-schema/event-case.out index 42ae519656..9ae44052ac 100644 --- a/tests/qapi-schema/event-case.out +++ b/tests/qapi-schema/event-case.out @@ -1,4 +1,4 @@ -module None +module ./builtin object q_empty enum QType prefix QTYPE diff --git a/tests/qapi-schema/include-repetition.out b/tests/qapi-schema/include-repetition.out index 0b654ddebb..16dbd9b819 100644 --- a/tests/qapi-schema/include-repetition.out +++ b/tests/qapi-schema/include-repetition.out @@ -1,4 +1,4 @@ -module None +module ./builtin object q_empty enum QType prefix QTYPE diff --git a/tests/qapi-schema/include-simple.out b/tests/qapi-schema/include-simple.out index 061f81e509..48e923bfbc 100644 --- a/tests/qapi-schema/include-simple.out +++ b/tests/qapi-schema/include-simple.out @@ -1,4 +1,4 @@ -module None +module ./builtin object q_empty enum QType prefix QTYPE diff --git a/tests/qapi-schema/indented-expr.out b/tests/qapi-schema/indented-expr.out index 04356775cd..6a30ded3fa 100644 --- a/tests/qapi-schema/indented-expr.out +++ b/tests/qapi-schema/indented-expr.out @@ -1,4 +1,4 @@ -module None +module ./builtin 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 8868ca0dca..3b1387d9f1 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -1,4 +1,4 @@ -module None +module ./builtin object q_empty enum QType prefix QTYPE -- cgit v1.2.3 From 4ab0ff6da0828e6499d633a4801f4e6b6e96d21b Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 1 Feb 2021 14:37:40 -0500 Subject: qapi/gen: Combine ._add_[user|system]_module With callers to _add_system_module now explicitly using the './' prefix to indicate a system module, there is no longer any reason to have separate interfaces for adding system vs user modules; use a unified interface that differentiates based on the name. Signed-off-by: Markus Armbruster Signed-off-by: John Snow Message-Id: <20210201193747.2169670-10-jsnow@redhat.com> --- scripts/qapi/commands.py | 2 +- scripts/qapi/events.py | 2 +- scripts/qapi/gen.py | 17 +++++------------ 3 files changed, 7 insertions(+), 14 deletions(-) diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py index fc5fe27c47..4911166339 100644 --- a/scripts/qapi/commands.py +++ b/scripts/qapi/commands.py @@ -286,7 +286,7 @@ class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor): types=types)) def visit_end(self) -> None: - self._add_system_module('./init', ' * QAPI Commands initialization') + self._add_module('./init', ' * QAPI Commands initialization') self._genh.add(mcgen(''' #include "qapi/qmp/dispatch.h" diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py index 26faa82989..079c666ec6 100644 --- a/scripts/qapi/events.py +++ b/scripts/qapi/events.py @@ -191,7 +191,7 @@ class QAPISchemaGenEventVisitor(QAPISchemaModularCVisitor): types=types)) def visit_end(self) -> None: - self._add_system_module('./emit', ' * QAPI Events emission') + self._add_module('./emit', ' * QAPI Events emission') self._genc.preamble_add(mcgen(''' #include "qemu/osdep.h" #include "%(prefix)sqapi-emit-events.h" diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py index 9352d79c3a..8ded0f7e5a 100644 --- a/scripts/qapi/gen.py +++ b/scripts/qapi/gen.py @@ -272,22 +272,15 @@ class QAPISchemaModularCVisitor(QAPISchemaVisitor): self._module_basename(what, name)) def _add_module(self, name: str, blurb: str) -> None: + if QAPISchemaModule.is_user_module(name): + if self._main_module is None: + self._main_module = name basename = self._module_filename(self._what, name) 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] - def _add_user_module(self, name: str, blurb: str) -> None: - assert QAPISchemaModule.is_user_module(name) - if self._main_module is None: - self._main_module = name - self._add_module(name, blurb) - - def _add_system_module(self, name: str, blurb: str) -> None: - assert QAPISchemaModule.is_system_module(name) - self._add_module(name, blurb) - def write(self, output_dir: str, opt_builtins: bool = False) -> None: for name in self._module: if QAPISchemaModule.is_builtin_module(name) and not opt_builtins: @@ -305,7 +298,7 @@ class QAPISchemaModularCVisitor(QAPISchemaVisitor): def visit_module(self, name: str) -> None: if QAPISchemaModule.is_builtin_module(name): if self._builtin_blurb: - self._add_system_module(name, self._builtin_blurb) + self._add_module(name, self._builtin_blurb) self._begin_builtin_module() else: # The built-in module has not been created. No code may @@ -314,7 +307,7 @@ class QAPISchemaModularCVisitor(QAPISchemaVisitor): self._genh = None else: assert QAPISchemaModule.is_user_module(name) - self._add_user_module(name, self._user_blurb) + self._add_module(name, self._user_blurb) self._begin_user_module(name) def visit_include(self, name: str, info: QAPISourceInfo) -> None: -- cgit v1.2.3 From 39b2d838f1f8a6c3d2fc4e2eecc45e5554d02af7 Mon Sep 17 00:00:00 2001 From: John Snow Date: Mon, 1 Feb 2021 14:37:41 -0500 Subject: qapi: centralize the built-in module name definition Use a constant to make it obvious we're referring to a very specific thing. Signed-off-by: John Snow Message-Id: <20210201193747.2169670-11-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/schema.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 14cf9da784..353e8020a2 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -68,7 +68,7 @@ class QAPISchemaEntity: def _set_module(self, schema, info): assert self._checked - fname = info.fname if info else './builtin' + fname = info.fname if info else QAPISchemaModule.BUILTIN_MODULE_NAME self._module = schema.module_by_fname(fname) self._module.add_entity(self) @@ -138,6 +138,9 @@ class QAPISchemaVisitor: class QAPISchemaModule: + + BUILTIN_MODULE_NAME = './builtin' + def __init__(self, name): self.name = name self._entity_list = [] @@ -160,14 +163,14 @@ class QAPISchemaModule: """ return not cls.is_system_module(name) - @staticmethod - def is_builtin_module(name: str) -> bool: + @classmethod + def is_builtin_module(cls, name: str) -> bool: """ The built-in module is a single System module for the built-in types. It is always "./builtin". """ - return name == './builtin' + return name == cls.BUILTIN_MODULE_NAME def add_entity(self, ent): self._entity_list.append(ent) @@ -853,7 +856,7 @@ class QAPISchema: self._entity_dict = {} self._module_dict = OrderedDict() self._schema_dir = os.path.dirname(fname) - self._make_module('./builtin') + self._make_module(QAPISchemaModule.BUILTIN_MODULE_NAME) self._make_module(fname) self._predefining = True self._def_predefineds() -- cgit v1.2.3 From fd9b1603843df86b430083b583157fe0c352901e Mon Sep 17 00:00:00 2001 From: John Snow Date: Mon, 1 Feb 2021 14:37:42 -0500 Subject: qapi/gen: write _genc/_genh access shims 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 Message-Id: <20210201193747.2169670-12-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- 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 8ded0f7e5a..b2bb9d12ff 100644 --- a/scripts/qapi/gen.py +++ b/scripts/qapi/gen.py @@ -244,11 +244,20 @@ class QAPISchemaModularCVisitor(QAPISchemaVisitor): 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 _module_dirname(name: str) -> str: if QAPISchemaModule.is_user_module(name): @@ -279,7 +288,7 @@ class QAPISchemaModularCVisitor(QAPISchemaVisitor): 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 write(self, output_dir: str, opt_builtins: bool = False) -> None: for name in self._module: @@ -303,8 +312,7 @@ class QAPISchemaModularCVisitor(QAPISchemaVisitor): 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: assert QAPISchemaModule.is_user_module(name) self._add_module(name, self._user_blurb) -- cgit v1.2.3 From d921d27c1bac0765370a9b9b891f9f0429f4c7c3 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 1 Feb 2021 14:37:43 -0500 Subject: qapi/gen: Support switching to another module temporarily Signed-off-by: Markus Armbruster Signed-off-by: John Snow Message-Id: <20210201193747.2169670-13-jsnow@redhat.com> [Commit message tweaked] --- scripts/qapi/gen.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py index b2bb9d12ff..a0a5df333e 100644 --- a/scripts/qapi/gen.py +++ b/scripts/qapi/gen.py @@ -290,6 +290,13 @@ class QAPISchemaModularCVisitor(QAPISchemaVisitor): self._module[name] = (genc, genh) self._current_module = name + @contextmanager + def _temp_module(self, name: str) -> Iterator[None]: + old_module = self._current_module + self._current_module = name + yield + self._current_module = old_module + def write(self, output_dir: str, opt_builtins: bool = False) -> None: for name in self._module: if QAPISchemaModule.is_builtin_module(name) and not opt_builtins: -- cgit v1.2.3 From c6cd7e4151794194f804ac3b8d2bc9347142c024 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 1 Feb 2021 14:37:44 -0500 Subject: qapi/commands: Simplify command registry generation QAPISchemaGenCommandVisitor.visit_command() needs to generate the marshalling function into the current module, and also generate its registration into the ./init system module. The latter is done somewhat awkwardly: .__init__() creates a QAPIGenCCode that will not be written out, each .visit_command() adds its registration to it, and .visit_end() copies its contents into the ./init module it creates. Instead provide the means to temporarily switch to another module. Create the ./init module in .visit_begin(), and generate its initial part. Add registrations to it in .visit_command(). Finish it in .visit_end(). Signed-off-by: Markus Armbruster Signed-off-by: John Snow Message-Id: <20210201193747.2169670-14-jsnow@redhat.com> --- scripts/qapi/commands.py | 49 ++++++++++++++++++++++-------------------------- 1 file changed, 22 insertions(+), 27 deletions(-) diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py index 4911166339..13a9dcaf89 100644 --- a/scripts/qapi/commands.py +++ b/scripts/qapi/commands.py @@ -23,7 +23,6 @@ from typing import ( from .common import c_name, mcgen from .gen import ( QAPIGenC, - QAPIGenCCode, QAPISchemaModularCVisitor, build_params, ifcontext, @@ -237,28 +236,11 @@ def gen_register_command(name: str, return ret -def gen_registry(registry: str, prefix: str) -> str: - ret = mcgen(''' - -void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds) -{ - QTAILQ_INIT(cmds); - -''', - c_prefix=c_name(prefix, protect=False)) - ret += registry - ret += mcgen(''' -} -''') - return ret - - 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]] = {} def _begin_user_module(self, name: str) -> None: @@ -285,7 +267,7 @@ class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor): ''', types=types)) - def visit_end(self) -> None: + def visit_begin(self, schema: QAPISchema) -> None: self._add_module('./init', ' * QAPI Commands initialization') self._genh.add(mcgen(''' #include "qapi/qmp/dispatch.h" @@ -293,13 +275,24 @@ class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor): void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds); ''', c_prefix=c_name(self._prefix, protect=False))) - self._genc.preamble_add(mcgen(''' + self._genc.add(mcgen(''' #include "qemu/osdep.h" #include "%(prefix)sqapi-commands.h" #include "%(prefix)sqapi-init-commands.h" + +void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds) +{ + QTAILQ_INIT(cmds); + ''', - prefix=self._prefix)) - self._genc.add(gen_registry(self._regy.get_content(), self._prefix)) + prefix=self._prefix, + c_prefix=c_name(self._prefix, protect=False))) + + def visit_end(self) -> None: + with self._temp_module('./init'): + self._genc.add(mcgen(''' +} +''')) def visit_command(self, name: str, @@ -324,15 +317,17 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds); if ret_type and ret_type not in self._visited_ret_types[self._genc]: self._visited_ret_types[self._genc].add(ret_type) with ifcontext(ret_type.ifcond, - self._genh, self._genc, self._regy): + self._genh, self._genc): self._genc.add(gen_marshal_output(ret_type)) - with ifcontext(ifcond, self._genh, self._genc, self._regy): + with ifcontext(ifcond, self._genh, self._genc): self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type)) self._genh.add(gen_marshal_decl(name)) self._genc.add(gen_marshal(name, arg_type, boxed, ret_type)) - self._regy.add(gen_register_command(name, success_response, - allow_oob, allow_preconfig, - coroutine)) + with self._temp_module('./init'): + with ifcontext(ifcond, self._genh, self._genc): + self._genc.add(gen_register_command(name, success_response, + allow_oob, allow_preconfig, + coroutine)) def gen_commands(schema: QAPISchema, -- cgit v1.2.3 From cc0747f6b709c197b077cd313f37617fc80c3be1 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 1 Feb 2021 14:37:45 -0500 Subject: qapi/gen: Drop support for QAPIGen without a file name The previous commit removed the only user of QAPIGen(None). Tighten the type hint. Signed-off-by: Markus Armbruster Signed-off-by: John Snow Message-Id: <20210201193747.2169670-15-jsnow@redhat.com> --- scripts/qapi/gen.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py index a0a5df333e..ac3d3e687e 100644 --- a/scripts/qapi/gen.py +++ b/scripts/qapi/gen.py @@ -40,7 +40,7 @@ from .source import QAPISourceInfo class QAPIGen: - def __init__(self, fname: Optional[str]): + def __init__(self, fname: str): self.fname = fname self._preamble = '' self._body = '' @@ -125,7 +125,7 @@ def build_params(arg_type: Optional[QAPISchemaObjectType], class QAPIGenCCode(QAPIGen): - def __init__(self, fname: Optional[str]): + def __init__(self, fname: str): super().__init__(fname) self._start_if: Optional[Tuple[List[str], str, str]] = None -- cgit v1.2.3 From 4a82e468e76f67901a7fcaeaf2d31904664658cc Mon Sep 17 00:00:00 2001 From: John Snow Date: Mon, 1 Feb 2021 14:37:46 -0500 Subject: qapi: type 'info' as Optional[QAPISourceInfo] For everything typed so far, type this parameter as Optional[QAPISourceInfo]. In the most generic case, QAPISchemaEntity's info field may be None to represent types that come from built-in definitions. Although some Entity types may not currently have any built-in definitions, it is not easily possible to constrain the type except on an ad-hoc basis using assertions. It's easier and simpler, then, to just say it's always an Optional type. Signed-off-by: John Snow Message-Id: <20210201193747.2169670-16-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/commands.py | 2 +- scripts/qapi/events.py | 2 +- scripts/qapi/gen.py | 2 +- scripts/qapi/types.py | 2 +- scripts/qapi/visit.py | 4 ++-- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py index 13a9dcaf89..54af519f44 100644 --- a/scripts/qapi/commands.py +++ b/scripts/qapi/commands.py @@ -296,7 +296,7 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds) def visit_command(self, name: str, - info: QAPISourceInfo, + info: Optional[QAPISourceInfo], ifcond: List[str], features: List[QAPISchemaFeature], arg_type: Optional[QAPISchemaObjectType], diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py index 079c666ec6..8c57deb2b8 100644 --- a/scripts/qapi/events.py +++ b/scripts/qapi/events.py @@ -213,7 +213,7 @@ void %(event_emit)s(%(event_enum)s event, QDict *qdict); def visit_event(self, name: str, - info: QAPISourceInfo, + info: Optional[QAPISourceInfo], ifcond: List[str], features: List[QAPISchemaFeature], arg_type: Optional[QAPISchemaObjectType], diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py index ac3d3e687e..63549cc8d4 100644 --- a/scripts/qapi/gen.py +++ b/scripts/qapi/gen.py @@ -325,7 +325,7 @@ class QAPISchemaModularCVisitor(QAPISchemaVisitor): self._add_module(name, self._user_blurb) self._begin_user_module(name) - def visit_include(self, name: str, info: QAPISourceInfo) -> None: + def visit_include(self, name: str, info: Optional[QAPISourceInfo]) -> None: relname = os.path.relpath(self._module_filename(self._what, name), os.path.dirname(self._genh.fname)) self._genh.preamble_add(mcgen(''' diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py index dbf58c0b91..2bdd626847 100644 --- a/scripts/qapi/types.py +++ b/scripts/qapi/types.py @@ -350,7 +350,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor): def visit_alternate_type(self, name: str, - info: QAPISourceInfo, + info: Optional[QAPISourceInfo], ifcond: List[str], features: List[QAPISchemaFeature], variants: QAPISchemaVariants) -> None: diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py index 568ba35592..22e62df901 100644 --- a/scripts/qapi/visit.py +++ b/scripts/qapi/visit.py @@ -336,7 +336,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor): def visit_enum_type(self, name: str, - info: QAPISourceInfo, + info: Optional[QAPISourceInfo], ifcond: List[str], features: List[QAPISchemaFeature], members: List[QAPISchemaEnumMember], @@ -378,7 +378,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor): def visit_alternate_type(self, name: str, - info: QAPISourceInfo, + info: Optional[QAPISourceInfo], ifcond: List[str], features: List[QAPISchemaFeature], variants: QAPISchemaVariants) -> None: -- cgit v1.2.3 From c51172667b64abc570640f141ca3bf7109fbbd17 Mon Sep 17 00:00:00 2001 From: John Snow Date: Mon, 1 Feb 2021 14:37:47 -0500 Subject: qapi: enable strict-optional checks 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 Message-Id: <20210201193747.2169670-17-jsnow@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/mypy.ini | 1 - 1 file changed, 1 deletion(-) diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini index 74fc6c8215..04bd5db527 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 -- cgit v1.2.3