[PATCH v4 00/16] qapi: static typing conversion, pt1.5

[PATCH v4 00/16] qapi: static typing conversion, pt1.5

[John Snow]

Hi, this patchset enables strict optional checking in mypy for
everything we have typed so far.

In general, this patchset seeks to clarify Optional[T] vs T and lift the
no-strict-optional restriction in mypy.

Ironing out these issues allows us to be even stricter with our type
checking, which improves consistency in subclass interface types and
should make review a little nicer.

This series is based on (but does not require) the 'qapi: sphinx-autodoc
for qapi module' series.

V4:

001/16:[----] [--] 'qapi/commands: assert arg_type is not None'
002/16:[----] [--] 'qapi/events: fix visit_event typing'
003/16:[----] [--] 'qapi/main: handle theoretical None-return from re.match()'
004/16:[----] [--] 'qapi/gen: inline _wrap_ifcond into end_if()'
005/16:[0047] [FC] 'qapi: centralize is_[user|system|builtin]_module methods'
006/16:[0007] [FC] 'qapi/gen: Replace ._begin_system_module()'
007/16:[----] [--] 'qapi: use explicitly internal module names'
008/16:[0016] [FC] 'qapi: use './builtin' as the built-in module name'
009/16:[0011] [FC] 'qapi/gen: Combine ._add_[user|system]_module'
010/16:[0008] [FC] 'qapi: centralize the built-in module name definition'
011/16:[----] [-C] 'qapi/gen: write _genc/_genh access shims'
012/16:[----] [--] 'qapi/gen: Support for switching to another module temporarily'
013/16:[----] [--] 'qapi/commands: Simplify command registry generation'
014/16:[----] [--] 'qapi/gen: Drop support for QAPIGen without a file name'
015/16:[----] [-C] 'qapi: type 'info' as Optional[QAPISourceInfo]'
016/16:[----] [--] 'qapi: enable strict-optional checks'

Removed the patch that made visit_module work in terms of QAPISchemaModule
instead of the module name.

05: Remove the properties, keep just the static/classmethods.
    Convert classmethod to staticmethod where possible.
    Add docstrings to alleviate confusion about what these methods do.
    Leave qapidoc generator alone for now.
    Remove future-bleed from is_system_module (check for None, not "./builtin")

06: Change the conditionals in gen.py visit_module to use is_builtin_module.

08: Some contextual difference now that modules are no longer being passed
    via visit_module.
    Code added by patch 05 is now amended here to make it consistent in history.

09: Removed assertion. Changed commit message.

10: Contextual fallout; removed end-of-line comment that was unneeded.

Notes:

Admittedly, at this point it feels like style and taste. I played with
several other alternatives, but felt I wasn't making good progress in
any direction that was more defensible than what I have here.

If there are nits at this point, I think they generally require more
engineering than what is present here to resolve them satisfactorily; to
that end I feel like this is close to the fewest SLOC changes that felt
defensible. Concrete counter-proposals would be preferred to minimize
guess-work when it comes to matters of style.

Namely, I would vastly prefer "module.system_module" as a property of
the module object and to pass those to visitors, but the work involved
in doing this is not trivial, because the generators that "add a system
module" generally only add a system module *name* to the generator, as
if it was visited -- but this is an emulated behavior as a
code-generation technique, the module does not exist. Changing the
gen.py functions to *actually* create a module is more hassle than it is
worth at present, so that python module ought to remain working in terms
of module *names*. Renaming the functions to reflect that made them a
little too cumbersome/long for my tastes, so I left them alone.

However, I still felt it important to begin making the claim that module
namespaces are global and shared between the generator and the schema;
so I still opted to use QAPISchemaModule methods to formalize the module
type definitions, even if we aren't using them in a classy way (yet).

You may argue that these can be functions instead, and they can; but it
is just an organizational preference that they live within the
QAPISchemaModule namespace because (to me) this solidifies the idea that
schema.py and QAPISchemaModule themselves are the solely responsible
entities for these definitions. (i.e., it makes it clear to me that
these namespaces aren't purely an invention of gen.py -- which they can
no longer be, as we are declaring and naming "./builtin", the built-in
schema module that contains our predefined types in schema.py.)

I didn't think it was worth making a class for the names alone and going
that route; that felt like more work than was warranted, too. So this
represents roughly the bottom floor of what seemed subjectively
sensible.

--js

John Snow (11):
  qapi/commands: assert arg_type is not None
  qapi/events: fix visit_event typing
  qapi/main: handle theoretical None-return from re.match()
  qapi/gen: inline _wrap_ifcond into end_if()
  qapi: centralize is_[user|system|builtin]_module methods
  qapi: use explicitly internal module names
  qapi: use './builtin' as the built-in module name
  qapi: centralize the built-in module name definition
  qapi/gen: write _genc/_genh access shims
  qapi: type 'info' as Optional[QAPISourceInfo]
  qapi: enable strict-optional checks

Markus Armbruster (5):
  qapi/gen: Replace ._begin_system_module()
  qapi/gen: Combine ._add_[user|system]_module
  qapi/gen: Support for switching to another module temporarily
  qapi/commands: Simplify command registry generation
  qapi/gen: Drop support for QAPIGen without a file name

 scripts/qapi/commands.py                 | 62 ++++++++--------
 scripts/qapi/events.py                   | 16 ++--
 scripts/qapi/gen.py                      | 94 ++++++++++++------------
 scripts/qapi/main.py                     |  2 +
 scripts/qapi/mypy.ini                    |  1 -
 scripts/qapi/schema.py                   | 42 +++++++++--
 scripts/qapi/types.py                    |  4 +-
 scripts/qapi/visit.py                    |  6 +-
 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 +-
 16 files changed, 139 insertions(+), 104 deletions(-)

-- 
2.29.2

[PATCH v4 01/16] qapi/commands: assert arg_type is not None

[John Snow]

When boxed is True, expr.py asserts that we must have
arguments. Ultimately, this should mean that if boxed is True that
arg_type should be defined. Mypy cannot infer this, and does not support
'stateful' type inference, e.g.:

```
if x:
    assert y is not None

...

if x:
    y.etc()
```

does not work, because mypy does not statefully remember the conditional
assertion in the second block. Help mypy out by creating a new local
that it can track more easily.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/commands.py | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 50978090b44..71744f48a35 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -126,6 +126,9 @@ def gen_marshal(name: str,
                 boxed: bool,
                 ret_type: Optional[QAPISchemaType]) -> str:
     have_args = boxed or (arg_type and not arg_type.is_empty())
+    if have_args:
+        assert arg_type is not None
+        arg_type_c_name = arg_type.c_name()
 
     ret = mcgen('''
 
@@ -147,7 +150,7 @@ def gen_marshal(name: str,
         ret += mcgen('''
     %(c_name)s arg = {0};
 ''',
-                     c_name=arg_type.c_name())
+                     c_name=arg_type_c_name)
 
     ret += mcgen('''
 
@@ -163,7 +166,7 @@ def gen_marshal(name: str,
         ok = visit_check_struct(v, errp);
     }
 ''',
-                     c_arg_type=arg_type.c_name())
+                     c_arg_type=arg_type_c_name)
     else:
         ret += mcgen('''
     ok = visit_check_struct(v, errp);
@@ -193,7 +196,7 @@ def gen_marshal(name: str,
         ret += mcgen('''
     visit_type_%(c_arg_type)s_members(v, &arg, NULL);
 ''',
-                     c_arg_type=arg_type.c_name())
+                     c_arg_type=arg_type_c_name)
 
     ret += mcgen('''
     visit_end_struct(v, NULL);
-- 
2.29.2

[PATCH v4 02/16] qapi/events: fix visit_event typing

[John Snow]

Actually, the arg_type can indeed be Optional.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/events.py | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index 599f3d1f564..9851653b9d1 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -12,7 +12,7 @@
 See the COPYING file in the top-level directory.
 """
 
-from typing import List
+from typing import List, Optional
 
 from .common import c_enum_const, c_name, mcgen
 from .gen import QAPISchemaModularCVisitor, build_params, ifcontext
@@ -27,7 +27,7 @@
 
 
 def build_event_send_proto(name: str,
-                           arg_type: QAPISchemaObjectType,
+                           arg_type: Optional[QAPISchemaObjectType],
                            boxed: bool) -> str:
     return 'void qapi_event_send_%(c_name)s(%(param)s)' % {
         'c_name': c_name(name.lower()),
@@ -35,7 +35,7 @@ def build_event_send_proto(name: str,
 
 
 def gen_event_send_decl(name: str,
-                        arg_type: QAPISchemaObjectType,
+                        arg_type: Optional[QAPISchemaObjectType],
                         boxed: bool) -> str:
     return mcgen('''
 
@@ -78,7 +78,7 @@ def gen_param_var(typ: QAPISchemaObjectType) -> str:
 
 
 def gen_event_send(name: str,
-                   arg_type: QAPISchemaObjectType,
+                   arg_type: Optional[QAPISchemaObjectType],
                    boxed: bool,
                    event_enum_name: str,
                    event_emit: str) -> str:
@@ -99,6 +99,7 @@ def gen_event_send(name: str,
                 proto=build_event_send_proto(name, arg_type, boxed))
 
     if have_args:
+        assert arg_type is not None
         ret += mcgen('''
     QObject *obj;
     Visitor *v;
@@ -114,6 +115,7 @@ def gen_event_send(name: str,
                  name=name)
 
     if have_args:
+        assert arg_type is not None
         ret += mcgen('''
     v = qobject_output_visitor_new(&obj);
 ''')
@@ -214,7 +216,7 @@ def visit_event(self,
                     info: QAPISourceInfo,
                     ifcond: List[str],
                     features: List[QAPISchemaFeature],
-                    arg_type: QAPISchemaObjectType,
+                    arg_type: Optional[QAPISchemaObjectType],
                     boxed: bool) -> None:
         with ifcontext(ifcond, self._genh, self._genc):
             self._genh.add(gen_event_send_decl(name, arg_type, boxed))
-- 
2.29.2

[PATCH v4 03/16] qapi/main: handle theoretical None-return from re.match()

[John Snow]

Mypy cannot understand that this match can never be None, so help it
along.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/main.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
index 42517210b80..703e7ed1ed5 100644
--- a/scripts/qapi/main.py
+++ b/scripts/qapi/main.py
@@ -23,6 +23,8 @@
 
 def invalid_prefix_char(prefix: str) -> Optional[str]:
     match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
+    # match cannot be None, but mypy cannot infer that.
+    assert match is not None
     if match.end() != len(prefix):
         return prefix[match.end()]
     return None
-- 
2.29.2

[PATCH v4 04/16] qapi/gen: inline _wrap_ifcond into end_if()

[John Snow]

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 <jsnow@redhat.com>
---
 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 b40f18eee3c..3d81b90ab71 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -130,15 +130,12 @@ 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 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
-- 
2.29.2

[PATCH v4 05/16] qapi: centralize is_[user|system|builtin]_module methods

[John Snow]

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 <jsnow@redhat.com>
---
 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 3d81b90ab71..2aec6d34365 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -31,7 +31,11 @@
     guardstart,
     mcgen,
 )
-from .schema import QAPISchemaObjectType, QAPISchemaVisitor
+from .schema import (
+    QAPISchemaModule,
+    QAPISchemaObjectType,
+    QAPISchemaVisitor,
+)
 from .source import QAPISourceInfo
 
 
@@ -246,21 +250,14 @@ def __init__(self,
         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 @@ def _add_module(self, name: Optional[str], blurb: str) -> None:
         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 @@ def _add_system_module(self, name: Optional[str], blurb: str) -> None:
 
     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 720449feee4..e80d9320eda 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -141,6 +141,33 @@ def __init__(self, name):
         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 @@ def resolve_type(self, name, info, what):
         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):
-- 
2.29.2

[PATCH v4 06/16] qapi/gen: Replace ._begin_system_module()

[John Snow]

From: Markus Armbruster <armbru@redhat.com>

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 <armbru@redhat.com>
Signed-off-by: John Snow <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 2aec6d34365..aaed78eed5e 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -295,23 +295,24 @@ 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_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 2b4916cdaa1..dbf58c0b91d 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -271,7 +271,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_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 339f1521524..568ba35592c 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_builtin_module(self) -> None:
         self._genc.preamble_add(mcgen('''
 #include "qemu/osdep.h"
 #include "qapi/error.h"
-- 
2.29.2

[PATCH v4 07/16] qapi: use explicitly internal module names

[John Snow]

Use ./emit and ./init explicitly instead of "emit" and "init" and adding
the prefix based on the specific method called, which later allows us to
coalesce the two different methods into one.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 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 71744f48a35..fc5fe27c472 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -286,7 +286,7 @@ def _begin_user_module(self, name: str) -> None:
                              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 9851653b9d1..26faa829898 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -191,7 +191,7 @@ def _begin_user_module(self, name: str) -> None:
                              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 aaed78eed5e..da9d4d2d373 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -285,7 +285,8 @@ def _add_user_module(self, name: str, blurb: str) -> None:
         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:
-- 
2.29.2

[PATCH v4 08/16] qapi: use './builtin' as the built-in module name

[John Snow]

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 <jsnow@redhat.com>
---
 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 da9d4d2d373..9352d79c3a4 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -246,16 +246,16 @@ 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 _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 @@ def _module_basename(self, what: str, name: Optional[str]) -> str:
             if name != self._main_module:
                 ret += '-' + os.path.splitext(basename)[0]
         else:
-            name = name[2:] if name else 'builtin'
-            ret += re.sub(r'-', '-' + name + '-', what)
+            assert 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 @@ 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:
+    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 @@ def _begin_builtin_module(self) -> None:
     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 e80d9320eda..14cf9da7842 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -68,7 +68,8 @@ def check_doc(self):
 
     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 @@ def __init__(self, name):
         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 @@ def is_user_module(cls, name: Optional[str]) -> bool:
         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 @@ 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('./builtin')
         self._make_module(fname)
         self._predefining = True
         self._def_predefineds()
@@ -897,7 +898,7 @@ def resolve_type(self, name, info, what):
                 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 @@ def _make_module(self, fname):
 
     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 273f0f54e16..ce4f6a4f0f5 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 419284dae29..715b0bbc1af 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 69666c39ad2..3feb3f69d3d 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 42ae519656d..9ae44052ac4 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 0b654ddebb6..16dbd9b8194 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 061f81e5090..48e923bfbc8 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 04356775cd1..6a30ded3fa6 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 8868ca0dca9..3b1387d9f18 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
-- 
2.29.2

[PATCH v4 09/16] qapi/gen: Combine ._add_[user|system]_module

[John Snow]

From: Markus Armbruster <armbru@redhat.com>

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 <armbru@redhat.com>
Signed-off-by: John Snow <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 fc5fe27c472..49111663394 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -286,7 +286,7 @@ def _begin_user_module(self, name: str) -> None:
                              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 26faa829898..079c666ec69 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -191,7 +191,7 @@ def _begin_user_module(self, name: str) -> None:
                              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 9352d79c3a4..8ded0f7e5ad 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -272,22 +272,15 @@ def _module_filename(self, what: str, name: str) -> str:
                             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 @@ def _begin_user_module(self, name: 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)
+                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 @@ def visit_module(self, name: str) -> None:
                 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:
-- 
2.29.2

[PATCH v4 10/16] qapi: centralize the built-in module name definition

[John Snow]

Use a constant to make it obvious we're referring to a very specific thing.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 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 14cf9da7842..353e8020a27 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -68,7 +68,7 @@ def check_doc(self):
 
     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 @@ def visit_event(self, name, info, ifcond, features, arg_type, boxed):
 
 
 class QAPISchemaModule:
+
+    BUILTIN_MODULE_NAME = './builtin'
+
     def __init__(self, name):
         self.name = name
         self._entity_list = []
@@ -160,14 +163,14 @@ def is_user_module(cls, name: str) -> bool:
         """
         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 @@ def __init__(self, fname):
         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()
-- 
2.29.2

[PATCH v4 11/16] qapi/gen: write _genc/_genh access shims

[John Snow]

Many places assume they can access these fields without checking them
first to ensure they are defined. Eliminating the _genc and _genh fields
and replacing them with functional properties that check for correct
state can ease the typing overhead by eliminating the Optional[T] return
type.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/gen.py | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 8ded0f7e5ad..b2bb9d12ffc 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -244,11 +244,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 _module_dirname(name: str) -> str:
         if QAPISchemaModule.is_user_module(name):
@@ -279,7 +288,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 write(self, output_dir: str, opt_builtins: bool = False) -> None:
         for name in self._module:
@@ -303,8 +312,7 @@ def visit_module(self, name: 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:
             assert QAPISchemaModule.is_user_module(name)
             self._add_module(name, self._user_blurb)
-- 
2.29.2

[PATCH v4 12/16] qapi/gen: Support for switching to another module temporarily

[John Snow]

From: Markus Armbruster <armbru@redhat.com>

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/gen.py | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index b2bb9d12ffc..a0a5df333e0 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -290,6 +290,13 @@ def _add_module(self, name: str, blurb: str) -> None:
         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:
-- 
2.29.2

[PATCH v4 13/16] qapi/commands: Simplify command registry generation

[John Snow]

From: Markus Armbruster <armbru@redhat.com>

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 <armbru@redhat.com>
Signed-off-by: John Snow <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 49111663394..13a9dcaf894 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -23,7 +23,6 @@
 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 @@ def _begin_user_module(self, name: str) -> None:
 ''',
                              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 @@ def visit_end(self) -> None:
 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 @@ def visit_command(self,
         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,
-- 
2.29.2

[PATCH v4 14/16] qapi/gen: Drop support for QAPIGen without a file name

[John Snow]

From: Markus Armbruster <armbru@redhat.com>

The previous commit removed the only user of QAPIGen(None).  Tighten
the type hint.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: John Snow <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 a0a5df333e0..ac3d3e687ef 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -40,7 +40,7 @@
 
 
 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
 
-- 
2.29.2

[PATCH v4 15/16] qapi: type 'info' as Optional[QAPISourceInfo]

[John Snow]

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 <jsnow@redhat.com>
---
 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 13a9dcaf894..54af519f44d 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -296,7 +296,7 @@ def visit_end(self) -> None:
 
     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 079c666ec69..8c57deb2b89 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -213,7 +213,7 @@ def visit_end(self) -> None:
 
     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 ac3d3e687ef..63549cc8d47 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -325,7 +325,7 @@ def visit_module(self, name: str) -> None:
             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 dbf58c0b91d..2bdd6268476 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -350,7 +350,7 @@ def visit_object_type(self,
 
     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 568ba35592c..22e62df9017 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],
@@ -378,7 +378,7 @@ def visit_object_type(self,
 
     def visit_alternate_type(self,
                              name: str,
-                             info: QAPISourceInfo,
+                             info: Optional[QAPISourceInfo],
                              ifcond: List[str],
                              features: List[QAPISchemaFeature],
                              variants: QAPISchemaVariants) -> None:
-- 
2.29.2

[PATCH v4 16/16] qapi: enable strict-optional checks

[John Snow]

In the modules that we are checking so far, we can be stricter about the
difference between Optional[T] and T types. Enable that check.

Enabling it now will assist review on further typing and cleanup work.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/mypy.ini | 1 -
 1 file changed, 1 deletion(-)

diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
index 74fc6c82153..04bd5db5278 100644
--- a/scripts/qapi/mypy.ini
+++ b/scripts/qapi/mypy.ini
@@ -1,6 +1,5 @@
 [mypy]
 strict = True
-strict_optional = False
 disallow_untyped_calls = False
 python_version = 3.6
 
-- 
2.29.2

Re: [PATCH v4 05/16] qapi: centralize is_[user|system|builtin]_module methods

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> 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 <jsnow@redhat.com>
> ---
>  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 3d81b90ab71..2aec6d34365 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -31,7 +31,11 @@
>      guardstart,
>      mcgen,
>  )
> -from .schema import QAPISchemaObjectType, QAPISchemaVisitor
> +from .schema import (
> +    QAPISchemaModule,
> +    QAPISchemaObjectType,
> +    QAPISchemaVisitor,
> +)
>  from .source import QAPISourceInfo
>  
>  
> @@ -246,21 +250,14 @@ def __init__(self,
>          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 @@ def _add_module(self, name: Optional[str], blurb: str) -> None:
>          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 @@ def _add_system_module(self, name: Optional[str], blurb: str) -> None:
>  
>      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 720449feee4..e80d9320eda 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -141,6 +141,33 @@ def __init__(self, name):
>          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)
>  

Putting these functions into a class feels awkward.  But it does the
job.

> @@ -871,8 +898,8 @@ def resolve_type(self, name, info, what):
>          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):

Re: [PATCH v4 07/16] qapi: use explicitly internal module names

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Use ./emit and ./init explicitly instead of "emit" and "init" and adding
> the prefix based on the specific method called, which later allows us to
> coalesce the two different methods into one.

"Bandwurmsatz" (literally "tapeworm sentence").  Perhaps something like:

    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.

Happy to tweak the commit message in my tree.

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  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 71744f48a35..fc5fe27c472 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -286,7 +286,7 @@ def _begin_user_module(self, name: str) -> None:
>                               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 9851653b9d1..26faa829898 100644
> --- a/scripts/qapi/events.py
> +++ b/scripts/qapi/events.py
> @@ -191,7 +191,7 @@ def _begin_user_module(self, name: str) -> None:
>                               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 aaed78eed5e..da9d4d2d373 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -285,7 +285,8 @@ def _add_user_module(self, name: str, blurb: str) -> None:
>          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:

Re: [PATCH v4 00/16] qapi: static typing conversion, pt1.5

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Hi, this patchset enables strict optional checking in mypy for
> everything we have typed so far.
>
> In general, this patchset seeks to clarify Optional[T] vs T and lift the
> no-strict-optional restriction in mypy.
>
> Ironing out these issues allows us to be even stricter with our type
> checking, which improves consistency in subclass interface types and
> should make review a little nicer.
>
> This series is based on (but does not require) the 'qapi: sphinx-autodoc
> for qapi module' series.

Series
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v4 05/16] qapi: centralize is_[user|system|builtin]_module methods

[John Snow]

On 2/2/21 4:16 AM, Markus Armbruster wrote:
> Putting these functions into a class feels awkward.  But it does the
> job.

Yes, I recognize that. I really wanted:

1) To centralize them somewhere, so there was somewhere obvious to look 
for these definitions, but

2) We aren't really using the classiness in a meaningful way, so

3) The names are a little awkward as you mentioned.

We can rewrite them as functions if you want, maybe with a comment for 
the class that says "Hey, go look at these functions!" but that didn't 
feel less messy to me. I couldn't really find anything that I actually 
genuinely liked. I went with a subjective least-worst.

--js

Re: [PATCH v4 07/16] qapi: use explicitly internal module names

[John Snow]

On 2/2/21 4:16 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> Use ./emit and ./init explicitly instead of "emit" and "init" and adding
>> the prefix based on the specific method called, which later allows us to
>> coalesce the two different methods into one.
> 
> "Bandwurmsatz" (literally "tapeworm sentence").  Perhaps something like:
> 
>      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.
> 
> Happy to tweak the commit message in my tree.
> 

Yep, with my blessing.

Re: [PATCH v4 07/16] qapi: use explicitly internal module names

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 2/2/21 4:16 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> Use ./emit and ./init explicitly instead of "emit" and "init" and adding
>>> the prefix based on the specific method called, which later allows us to
>>> coalesce the two different methods into one.
>> "Bandwurmsatz" (literally "tapeworm sentence").  Perhaps something
>> like:
>>      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.
>> Happy to tweak the commit message in my tree.
>> 
>
> Yep, with my blessing.

Done!