[PATCH v3 00/17] qapi: static typing conversion, pt1.5

[PATCH v3 00/17] 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.

V3:

Heavily reshuffled. QAPISourceInfo patches are removed in favor of patch
implications for later series, but none here in this series.

001/17:[----] [--] 'qapi/commands: assert arg_type is not None'
002/17:[----] [--] 'qapi/events: fix visit_event typing'
003/17:[----] [--] 'qapi/main: handle theoretical None-return from re.match()'
004/17:[down] 'qapi/gen: inline _wrap_ifcond into end_if()'

Module naming patches begin here. Their ordering is a little troubled,
but I've rebased and re-ordered them too many times and I'm spinning my wheels.
Suggestions welcome.

005/17:[down] 'qapi: pass QAPISchemaModule to visit_module instead of str'
006/17:[down] 'qapi: centralize is_[user|system|builtin]_module methods'
007/17:[down] 'qapi/gen: Replace ._begin_system_module()'
008/17:[down] 'qapi: use explicitly internal module names'
009/17:[down] 'qapi: use './builtin' as the built-in module name'
010/17:[down] 'qapi/gen: Combine ._add_[user|system]_module'
011/17:[down] 'qapi: centralize the built-in module name definition'

This segment of patches focuses on tightening the types in gen.py. #15
is the payoff for 13-14 and several patches in the last segment.

012/17:[----] [-C] 'qapi/gen: write _genc/_genh access shims'
013/17:[down] 'qapi/gen: Support for switching to another module temporarily'
014/17:[down] 'qapi/commands: Simplify command registry generation'
015/17:[down] 'qapi/gen: Drop support for QAPIGen without a file name'

This segment wraps up removing the last non-strict option in mypy.

016/17:[down] 'qapi: type 'info' as Optional[QAPISourceInfo]'
017/17:[----] [--] 'qapi: enable strict-optional checks'

John Snow (12):
  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: pass QAPISchemaModule to visit_module instead of str
  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

 docs/sphinx/qapidoc.py                   |  8 +-
 scripts/qapi/commands.py                 | 62 ++++++++-------
 scripts/qapi/events.py                   | 16 ++--
 scripts/qapi/gen.py                      | 96 ++++++++++++------------
 scripts/qapi/main.py                     |  2 +
 scripts/qapi/mypy.ini                    |  1 -
 scripts/qapi/schema.py                   | 43 +++++++++--
 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 +-
 tests/qapi-schema/test-qapi.py           |  4 +-
 18 files changed, 145 insertions(+), 113 deletions(-)

-- 
2.26.2

[PATCH v3 01/17] 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.26.2

[PATCH v3 02/17] 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.26.2

[PATCH v3 03/17] 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.26.2

[PATCH v3 04/17] 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.26.2

[PATCH v3 05/17] qapi: pass QAPISchemaModule to visit_module instead of str

[John Snow]

Modify visit_module to pass the module itself instead of just its
name. This allows for future patches to centralize some
module-interrogation behavior within the QAPISchemaModule class itself,
cutting down on duplication between gen.py and schema.py.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py         |  8 ++++----
 scripts/qapi/gen.py            | 16 ++++++++++------
 scripts/qapi/schema.py         |  4 ++--
 tests/qapi-schema/test-qapi.py |  4 ++--
 4 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index e03abcbb959..f754f675d66 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -463,11 +463,11 @@ def __init__(self, env, qapidir):
         self._env = env
         self._qapidir = qapidir
 
-    def visit_module(self, name):
-        if name is not None:
-            qapifile = self._qapidir + '/' + name
+    def visit_module(self, module):
+        if module.name:
+            qapifile = self._qapidir + '/' + module.name
             self._env.note_dependency(os.path.abspath(qapifile))
-        super().visit_module(name)
+        super().visit_module(module)
 
 
 class QAPIDocDirective(Directive):
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 3d81b90ab71..e73d3d61aac 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
 
 
@@ -304,19 +308,19 @@ def _begin_system_module(self, name: None) -> None:
     def _begin_user_module(self, name: str) -> None:
         pass
 
-    def visit_module(self, name: Optional[str]) -> None:
-        if name is None:
+    def visit_module(self, module: QAPISchemaModule) -> None:
+        if module.name is None:
             if self._builtin_blurb:
                 self._add_system_module(None, self._builtin_blurb)
-                self._begin_system_module(name)
+                self._begin_system_module(module.name)
             else:
                 # The built-in module has not been created.  No code may
                 # be generated.
                 self._genc = None
                 self._genh = None
         else:
-            self._add_user_module(name, self._user_blurb)
-            self._begin_user_module(name)
+            self._add_user_module(module.name, self._user_blurb)
+            self._begin_user_module(module.name)
 
     def visit_include(self, name: str, info: QAPISourceInfo) -> None:
         relname = os.path.relpath(self._module_filename(self._what, name),
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 720449feee4..69ba722c084 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -97,7 +97,7 @@ def visit_begin(self, schema):
     def visit_end(self):
         pass
 
-    def visit_module(self, name):
+    def visit_module(self, module):
         pass
 
     def visit_needed(self, entity):
@@ -145,7 +145,7 @@ def add_entity(self, ent):
         self._entity_list.append(ent)
 
     def visit(self, visitor):
-        visitor.visit_module(self.name)
+        visitor.visit_module(self)
         for entity in self._entity_list:
             if visitor.visit_needed(entity):
                 entity.visit(visitor)
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index e8db9d09d91..bec1ebff3db 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -24,8 +24,8 @@
 
 class QAPISchemaTestVisitor(QAPISchemaVisitor):
 
-    def visit_module(self, name):
-        print('module %s' % name)
+    def visit_module(self, module):
+        print('module %s' % module.name)
 
     def visit_include(self, name, info):
         print('include %s' % name)
-- 
2.26.2

[PATCH v3 06/17] 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>
---
 docs/sphinx/qapidoc.py |  2 +-
 scripts/qapi/gen.py    | 21 +++++++--------------
 scripts/qapi/schema.py | 28 ++++++++++++++++++++++++++--
 3 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index f754f675d66..08b9439de2b 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -464,7 +464,7 @@ def __init__(self, env, qapidir):
         self._qapidir = qapidir
 
     def visit_module(self, module):
-        if module.name:
+        if module.user_module:
             qapifile = self._qapidir + '/' + module.name
             self._env.note_dependency(os.path.abspath(qapifile))
         super().visit_module(module)
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index e73d3d61aac..3b7eddc1c21 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -250,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:
@@ -286,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)
@@ -296,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)
@@ -309,7 +302,7 @@ def _begin_user_module(self, name: str) -> None:
         pass
 
     def visit_module(self, module: QAPISchemaModule) -> None:
-        if module.name is None:
+        if module.system_module:
             if self._builtin_blurb:
                 self._add_system_module(None, self._builtin_blurb)
                 self._begin_system_module(module.name)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 69ba722c084..44a9232c881 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -141,6 +141,30 @@ def __init__(self, name):
         self.name = name
         self._entity_list = []
 
+    @classmethod
+    def is_system_module(cls, name: Optional[str]) -> bool:
+        return name is None or name.startswith('./')
+
+    @classmethod
+    def is_user_module(cls, name: Optional[str]) -> bool:
+        return not cls.is_system_module(name)
+
+    @classmethod
+    def is_builtin_module(cls, name: str) -> bool:
+        return name == './builtin'
+
+    @property
+    def system_module(self) -> bool:
+        return self.is_system_module(self.name)
+
+    @property
+    def user_module(self) -> bool:
+        return self.is_user_module(self.name)
+
+    @property
+    def builtin_module(self) -> bool:
+        return self.is_builtin_module(self.name)
+
     def add_entity(self, ent):
         self._entity_list.append(ent)
 
@@ -871,8 +895,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.26.2

[PATCH v3 07/17] 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.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/gen.py   | 4 ++--
 scripts/qapi/types.py | 2 +-
 scripts/qapi/visit.py | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 3b7eddc1c21..9f850d23646 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -295,7 +295,7 @@ 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:
@@ -305,7 +305,7 @@ def visit_module(self, module: QAPISchemaModule) -> None:
         if module.system_module:
             if self._builtin_blurb:
                 self._add_system_module(None, self._builtin_blurb)
-                self._begin_system_module(module.name)
+                self._begin_builtin_module()
             else:
                 # The built-in module has not been created.  No code may
                 # be generated.
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.26.2

[PATCH v3 08/17] qapi: use explicitly internal module names

[John Snow]

Use ./emit and ./init explicitly instead of "emit" and "init". Add
assertions that enforce this.

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 9f850d23646..18b72dd06c8 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.26.2

[PATCH v3 09/17] 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                   | 14 +++++++-------
 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, 24 insertions(+), 24 deletions(-)

diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 18b72dd06c8..55acd7e080d 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)
 
@@ -305,7 +305,7 @@ def _begin_user_module(self, name: str) -> None:
     def visit_module(self, module: QAPISchemaModule) -> None:
         if module.system_module:
             if self._builtin_blurb:
-                self._add_system_module(None, self._builtin_blurb)
+                self._add_system_module(module.name, self._builtin_blurb)
                 self._begin_builtin_module()
             else:
                 # The built-in module has not been created.  No code may
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 44a9232c881..c519c18a5ec 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,11 +143,11 @@ def __init__(self, name):
         self._entity_list = []
 
     @classmethod
-    def is_system_module(cls, name: Optional[str]) -> bool:
-        return name is None or name.startswith('./')
+    def is_system_module(cls, name: str) -> bool:
+        return name.startswith('./')
 
     @classmethod
-    def is_user_module(cls, name: Optional[str]) -> bool:
+    def is_user_module(cls, name: str) -> bool:
         return not cls.is_system_module(name)
 
     @classmethod
@@ -849,7 +850,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')  # built-ins
         self._make_module(fname)
         self._predefining = True
         self._def_predefineds()
@@ -894,7 +895,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)
@@ -907,7 +908,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.26.2

[PATCH v3 10/17] qapi/gen: Combine ._add_[user|system]_module

[John Snow]

From: Markus Armbruster <armbru@redhat.com>

QAPISchemaModularCVisitor attempts to encapsulate the way it splits
the module name space between user modules (name can't start with
'./') and system modules (name is None or starts with './') by
providing separate ._add_user_module() and ._add_system_module(),
where the latter prepends './' to names other than None.

Not worthwhile.  Dumb down to a single ._add_module().

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      | 20 +++++++-------------
 3 files changed, 9 insertions(+), 15 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 55acd7e080d..b5505685e6e 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:
@@ -303,9 +296,9 @@ def _begin_user_module(self, name: str) -> None:
         pass
 
     def visit_module(self, module: QAPISchemaModule) -> None:
-        if module.system_module:
+        if module.builtin_module:
             if self._builtin_blurb:
-                self._add_system_module(module.name, self._builtin_blurb)
+                self._add_module(module.name, self._builtin_blurb)
                 self._begin_builtin_module()
             else:
                 # The built-in module has not been created.  No code may
@@ -313,7 +306,8 @@ def visit_module(self, module: QAPISchemaModule) -> None:
                 self._genc = None
                 self._genh = None
         else:
-            self._add_user_module(module.name, self._user_blurb)
+            assert module.user_module, "Unexpected system module"
+            self._add_module(module.name, self._user_blurb)
             self._begin_user_module(module.name)
 
     def visit_include(self, name: str, info: QAPISourceInfo) -> None:
-- 
2.26.2

[PATCH v3 11/17] 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 | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index c519c18a5ec..887a1c82c6f 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 = []
@@ -152,7 +155,7 @@ def is_user_module(cls, name: str) -> bool:
 
     @classmethod
     def is_builtin_module(cls, name: str) -> bool:
-        return name == './builtin'
+        return name == cls.BUILTIN_MODULE_NAME
 
     @property
     def system_module(self) -> bool:
@@ -850,7 +853,7 @@ def __init__(self, fname):
         self._entity_dict = {}
         self._module_dict = OrderedDict()
         self._schema_dir = os.path.dirname(fname)
-        self._make_module('./builtin')  # built-ins
+        self._make_module(QAPISchemaModule.BUILTIN_MODULE_NAME)  # built-ins
         self._make_module(fname)
         self._predefining = True
         self._def_predefineds()
-- 
2.26.2

[PATCH v3 12/17] 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 b5505685e6e..4b5d3ea3a5b 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, module: QAPISchemaModule) -> 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 module.user_module, "Unexpected system module"
             self._add_module(module.name, self._user_blurb)
-- 
2.26.2

[PATCH v3 13/17] 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 4b5d3ea3a5b..86abdabb57a 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.26.2

[PATCH v3 14/17] 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.26.2

[PATCH v3 15/17] 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 86abdabb57a..87074335e6c 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.26.2

[PATCH v3 16/17] 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 87074335e6c..f3f4d2b011e 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -325,7 +325,7 @@ def visit_module(self, module: QAPISchemaModule) -> None:
             self._add_module(module.name, self._user_blurb)
             self._begin_user_module(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.26.2

[PATCH v3 17/17] 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.26.2

Re: [PATCH v3 05/17] qapi: pass QAPISchemaModule to visit_module instead of str

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Modify visit_module to pass the module itself instead of just its
> name. This allows for future patches to centralize some
> module-interrogation behavior within the QAPISchemaModule class itself,
> cutting down on duplication between gen.py and schema.py.

We've been tempted to make similar changes before (don't worry, I'm not
building a case for "no" here).

When I wrote the initial version of QAPISchemaVisitor (commit 3f7dc21be,
2015), I aimed for a loose coupling of backends and the internal
representation.  Instead of

    def visit_foo(self, foo):
        pass

where @foo is a QAPISchemaFooBar, I wrote

    def visit_foo_bar(self, name, info, [curated attributes of @foo]):
        pass

In theory, this is nice: the information exposed to the backends is
obvious, and the backends can't accidentally mutate @foo.

In practice, it kind of failed right then and there:

    def visit_object_type(self, name, info, base, members, variants):
        pass

We avoid passing the QAPISchemaObjectType (loose coupling, cool!), only
to pass member information as List[QAPISchemaObjectTypeMember].

Morever, passing "curated atttibutes" has led to visit_commands() taking
a dozen arguments.  Meh.

This had made Eric and me wonder whether we should write off the
decoupling idea as misguided, and just pass the object instead of
"curated attributes", always.  Thoughts?

>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  docs/sphinx/qapidoc.py         |  8 ++++----
>  scripts/qapi/gen.py            | 16 ++++++++++------
>  scripts/qapi/schema.py         |  4 ++--
>  tests/qapi-schema/test-qapi.py |  4 ++--
>  4 files changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index e03abcbb959..f754f675d66 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -463,11 +463,11 @@ def __init__(self, env, qapidir):
>          self._env = env
>          self._qapidir = qapidir
>  
> -    def visit_module(self, name):
> -        if name is not None:
> -            qapifile = self._qapidir + '/' + name
> +    def visit_module(self, module):
> +        if module.name:

Replacing the "is not None" test by (implicit) "is thruthy" changes
behavior for the empty string.  Intentional?

I've had the "pleasure" of debugging empty strings getting interpreted
like None where they should be interpreted like any other string.

> +            qapifile = self._qapidir + '/' + module.name
>              self._env.note_dependency(os.path.abspath(qapifile))
> -        super().visit_module(name)
> +        super().visit_module(module)
>  
>  
[...]

Re: [PATCH v3 06/17] 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>
> ---
>  docs/sphinx/qapidoc.py |  2 +-
>  scripts/qapi/gen.py    | 21 +++++++--------------
>  scripts/qapi/schema.py | 28 ++++++++++++++++++++++++++--
>  3 files changed, 34 insertions(+), 17 deletions(-)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index f754f675d66..08b9439de2b 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -464,7 +464,7 @@ def __init__(self, env, qapidir):
>          self._qapidir = qapidir
>  
>      def visit_module(self, module):
> -        if module.name:
> +        if module.user_module:
>              qapifile = self._qapidir + '/' + module.name
>              self._env.note_dependency(os.path.abspath(qapifile))
>          super().visit_module(module)

Cleaner:

           qapifile = module.source_filename()
           if qapifile:
               self._env.note_dependency(os.path.abspath(qapifile))

Observation, not demand.

> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index e73d3d61aac..3b7eddc1c21 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -250,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 ''

No use of self left.  @staticmethod?

Or maybe ...

>  
>      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:
               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)
           return ret

       def _module_filename(self, what: str, name: Optional[str]) -> str:
           return os.path.join(self._module_dirname(name),
                               self._module_basename(what, name))

... replace this by

           return os.path.join(module.source_dirname(),
                               self._module_basename(what, name))

Requires plumbing @module from visit_module() to here.

Again, not a demand.

> @@ -286,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)
> @@ -296,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)
> @@ -309,7 +302,7 @@ def _begin_user_module(self, name: str) -> None:
>          pass
>  
>      def visit_module(self, module: QAPISchemaModule) -> None:
> -        if module.name is None:
> +        if module.system_module:
>              if self._builtin_blurb:
>                  self._add_system_module(None, self._builtin_blurb)
>                  self._begin_system_module(module.name)
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 69ba722c084..44a9232c881 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -141,6 +141,30 @@ def __init__(self, name):
>          self.name = name
>          self._entity_list = []
>  
> +    @classmethod
> +    def is_system_module(cls, name: Optional[str]) -> bool:
> +        return name is None or name.startswith('./')
> +
> +    @classmethod
> +    def is_user_module(cls, name: Optional[str]) -> bool:
> +        return not cls.is_system_module(name)
> +
> +    @classmethod
> +    def is_builtin_module(cls, name: str) -> bool:
> +        return name == './builtin'

The names suggest these are predicates on modules, but they're actually
predicates on module *names*.

@staticmethod would suffice.  Actually, *functions* would.  All the
"classiness" buys us here is having to write
QAPISchemaModule.is_FOO_module(name) instead of is_FOO_module(name), and
...

If you *want* classy, have a class QAPISchemaModuleName.

> +
> +    @property
> +    def system_module(self) -> bool:
> +        return self.is_system_module(self.name)
> +
> +    @property
> +    def user_module(self) -> bool:
> +        return self.is_user_module(self.name)
> +
> +    @property
> +    def builtin_module(self) -> bool:
> +        return self.is_builtin_module(self.name)

... module.FOO_module instead of module.is_FOO_module.

Any particular reason for the @property decorator?

> +
>      def add_entity(self, ent):
>          self._entity_list.append(ent)
>  
> @@ -871,8 +895,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):
           name = self._module_name(fname)
           if name not in self._module_dict:
               self._module_dict[name] = QAPISchemaModule(name)
           return self._module_dict[name]

I got one more issue.

Before the patch:

* QAPISchemaModule deals with the user's modules and a single, special
  module for built-ins.  The former's name is the schema module filename
  relative to the main schema's directory, the latter's name is None.

* QAPISchemaModularCVisitor supports both kinds, and adds a third kind:
  system modules.  Their name begins with './', which cannot clash.
  These guys are used for generated code that doesn't fit anywhere else.

Your patch drags system modules into QAPISchemaModule.  Somewhat
awkward.  Could use comments (but then, much of scripts/qapi/ could).

Subtyping QAPISchemaModule is yet another possible way to avoid putting
knowledge about QAPISchemaModularCVisitor's doings into
QAPISchemaModule.

Re: [PATCH v3 09/17] qapi: use './builtin' as the built-in module name

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> 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                   | 14 +++++++-------
>  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, 24 insertions(+), 24 deletions(-)
>
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index 18b72dd06c8..55acd7e080d 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)
>  
> @@ -305,7 +305,7 @@ def _begin_user_module(self, name: str) -> None:
>      def visit_module(self, module: QAPISchemaModule) -> None:
>          if module.system_module:
>              if self._builtin_blurb:
> -                self._add_system_module(None, self._builtin_blurb)
> +                self._add_system_module(module.name, self._builtin_blurb)
>                  self._begin_builtin_module()
>              else:
>                  # The built-in module has not been created.  No code may
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 44a9232c881..c519c18a5ec 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,11 +143,11 @@ def __init__(self, name):
>          self._entity_list = []
>  
>      @classmethod
> -    def is_system_module(cls, name: Optional[str]) -> bool:
> -        return name is None or name.startswith('./')
> +    def is_system_module(cls, name: str) -> bool:
> +        return name.startswith('./')
>  
>      @classmethod
> -    def is_user_module(cls, name: Optional[str]) -> bool:
> +    def is_user_module(cls, name: str) -> bool:
>          return not cls.is_system_module(name)
>  
>      @classmethod
> @@ -849,7 +850,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')  # built-ins

The comment it now redundant.

>          self._make_module(fname)
>          self._predefining = True
>          self._def_predefineds()
> @@ -894,7 +895,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)
> @@ -907,7 +908,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):
[...]

Re: [PATCH v3 10/17] qapi/gen: Combine ._add_[user|system]_module

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> From: Markus Armbruster <armbru@redhat.com>
>
> QAPISchemaModularCVisitor attempts to encapsulate the way it splits
> the module name space between user modules (name can't start with
> './') and system modules (name is None or starts with './') by

Is this still accurate?

> providing separate ._add_user_module() and ._add_system_module(),
> where the latter prepends './' to names other than None.
>
> Not worthwhile.  Dumb down to a single ._add_module().
>
> 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      | 20 +++++++-------------
>  3 files changed, 9 insertions(+), 15 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 55acd7e080d..b5505685e6e 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:
> @@ -303,9 +296,9 @@ def _begin_user_module(self, name: str) -> None:
>          pass
>  
>      def visit_module(self, module: QAPISchemaModule) -> None:
> -        if module.system_module:
> +        if module.builtin_module:

Looks like you're fixing a slip-up in PATCH 06.  If yes, squash.  If no,
I'm confused.

>              if self._builtin_blurb:
> -                self._add_system_module(module.name, self._builtin_blurb)
> +                self._add_module(module.name, self._builtin_blurb)
>                  self._begin_builtin_module()
>              else:
>                  # The built-in module has not been created.  No code may
> @@ -313,7 +306,8 @@ def visit_module(self, module: QAPISchemaModule) -> None:
>                  self._genc = None
>                  self._genh = None
>          else:
> -            self._add_user_module(module.name, self._user_blurb)
> +            assert module.user_module, "Unexpected system module"

The string provides no value.

> +            self._add_module(module.name, self._user_blurb)
>              self._begin_user_module(module.name)
>  
>      def visit_include(self, name: str, info: QAPISourceInfo) -> None:

Re: [PATCH v3 05/17] qapi: pass QAPISchemaModule to visit_module instead of str

[John Snow]

On 1/20/21 7:07 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> Modify visit_module to pass the module itself instead of just its
>> name. This allows for future patches to centralize some
>> module-interrogation behavior within the QAPISchemaModule class itself,
>> cutting down on duplication between gen.py and schema.py.
> 
> We've been tempted to make similar changes before (don't worry, I'm not
> building a case for "no" here).
> 

It's fine: you'll probably notice later I don't go the full distance and 
rely on both object and class methods anyway, so this isn't strictly 
needed right now.

(It was not possible to go the full distance without heavier, more 
invasive changes, so...)

> When I wrote the initial version of QAPISchemaVisitor (commit 3f7dc21be,
> 2015), I aimed for a loose coupling of backends and the internal
> representation.  Instead of
> 
>      def visit_foo(self, foo):
>          pass
> 
> where @foo is a QAPISchemaFooBar, I wrote
> 
>      def visit_foo_bar(self, name, info, [curated attributes of @foo]):
>          pass
> 
> In theory, this is nice: the information exposed to the backends is
> obvious, and the backends can't accidentally mutate @foo.
> 
> In practice, it kind of failed right then and there:
> 
>      def visit_object_type(self, name, info, base, members, variants):
>          pass
> 
> We avoid passing the QAPISchemaObjectType (loose coupling, cool!), only
> to pass member information as List[QAPISchemaObjectTypeMember].
> 
> Morever, passing "curated atttibutes" has led to visit_commands() taking
> a dozen arguments.  Meh.
> 
> This had made Eric and me wonder whether we should write off the
> decoupling idea as misguided, and just pass the object instead of
> "curated attributes", always.  Thoughts?
> 

I'm not sure. Just taking the object would avoid a lot of duplicated 
interface typing, and type hints would allow editors to know what fields 
are available.

>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   docs/sphinx/qapidoc.py         |  8 ++++----
>>   scripts/qapi/gen.py            | 16 ++++++++++------
>>   scripts/qapi/schema.py         |  4 ++--
>>   tests/qapi-schema/test-qapi.py |  4 ++--
>>   4 files changed, 18 insertions(+), 14 deletions(-)
>>
>> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
>> index e03abcbb959..f754f675d66 100644
>> --- a/docs/sphinx/qapidoc.py
>> +++ b/docs/sphinx/qapidoc.py
>> @@ -463,11 +463,11 @@ def __init__(self, env, qapidir):
>>           self._env = env
>>           self._qapidir = qapidir
>>   
>> -    def visit_module(self, name):
>> -        if name is not None:
>> -            qapifile = self._qapidir + '/' + name
>> +    def visit_module(self, module):
>> +        if module.name:
> 
> Replacing the "is not None" test by (implicit) "is thruthy" changes
> behavior for the empty string.  Intentional?
> 

Instinctively it was intentional, consciously it wasn't. I was worried 
about what "qapifile" would produce if the string happened to be empty.

> I've had the "pleasure" of debugging empty strings getting interpreted
> like None where they should be interpreted like any other string.
> 

assert module.name, then?

>> +            qapifile = self._qapidir + '/' + module.name
>>               self._env.note_dependency(os.path.abspath(qapifile))
>> -        super().visit_module(name)
>> +        super().visit_module(module)
>>   
>>   
> [...]
> 

Re: [PATCH v3 05/17] qapi: pass QAPISchemaModule to visit_module instead of str

[Eric Blake]

On 1/20/21 6:07 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> Modify visit_module to pass the module itself instead of just its
>> name. This allows for future patches to centralize some
>> module-interrogation behavior within the QAPISchemaModule class itself,
>> cutting down on duplication between gen.py and schema.py.
> 
> We've been tempted to make similar changes before (don't worry, I'm not
> building a case for "no" here).
> 
> When I wrote the initial version of QAPISchemaVisitor (commit 3f7dc21be,
> 2015), I aimed for a loose coupling of backends and the internal
> representation.  Instead of
> 
>     def visit_foo(self, foo):
>         pass
> 
> where @foo is a QAPISchemaFooBar, I wrote
> 
>     def visit_foo_bar(self, name, info, [curated attributes of @foo]):
>         pass
> 
> In theory, this is nice: the information exposed to the backends is
> obvious, and the backends can't accidentally mutate @foo.
> 
> In practice, it kind of failed right then and there:
> 
>     def visit_object_type(self, name, info, base, members, variants):
>         pass
> 
> We avoid passing the QAPISchemaObjectType (loose coupling, cool!), only
> to pass member information as List[QAPISchemaObjectTypeMember].
> 
> Morever, passing "curated atttibutes" has led to visit_commands() taking
> a dozen arguments.  Meh.
> 
> This had made Eric and me wonder whether we should write off the
> decoupling idea as misguided, and just pass the object instead of
> "curated attributes", always.  Thoughts?

I'm open to the idea of passing just the larger object instead of the
curated list of relevant attributes.  It's a bit more coupling, but I
don't see any of our qapi code being reused outside its current scope
where the extra coupling will bite us.  But I'm not volunteering for the
refactoring work, because I'm not an expert on python typing hints.  If
consolidating parameters into the larger object makes for fewer
parameters and easier typing hints, I'm assuming the work can be done as
part of static typing; but if leaving things as they currently are is
manageable, that's also fine by me.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v3 10/17] qapi/gen: Combine ._add_[user|system]_module

[John Snow]

On 1/20/21 9:20 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> From: Markus Armbruster <armbru@redhat.com>
>>
>> QAPISchemaModularCVisitor attempts to encapsulate the way it splits
>> the module name space between user modules (name can't start with
>> './') and system modules (name is None or starts with './') by
> 
> Is this still accurate?
> 

Ah, not exactly; sorry I mangled your patches here. I had written my own 
version of what this patch used to be, but your patch went one step 
further, but the ordering got weird.

>> providing separate ._add_user_module() and ._add_system_module(),
>> where the latter prepends './' to names other than None.
>>
>> Not worthwhile.  Dumb down to a single ._add_module().
>>
>> 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      | 20 +++++++-------------
>>   3 files changed, 9 insertions(+), 15 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 55acd7e080d..b5505685e6e 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:
>> @@ -303,9 +296,9 @@ def _begin_user_module(self, name: str) -> None:
>>           pass
>>   
>>       def visit_module(self, module: QAPISchemaModule) -> None:
>> -        if module.system_module:
>> +        if module.builtin_module:
> 
> Looks like you're fixing a slip-up in PATCH 06.  If yes, squash.  If no,
> I'm confused.
> 

Or patch 7, actually -- where we clarify that we are checking for the 
built-in module and not any old system module.

>>               if self._builtin_blurb:
>> -                self._add_system_module(module.name, self._builtin_blurb)
>> +                self._add_module(module.name, self._builtin_blurb)
>>                   self._begin_builtin_module()
>>               else:
>>                   # The built-in module has not been created.  No code may
>> @@ -313,7 +306,8 @@ def visit_module(self, module: QAPISchemaModule) -> None:
>>                   self._genc = None
>>                   self._genh = None
>>           else:
>> -            self._add_user_module(module.name, self._user_blurb)
>> +            assert module.user_module, "Unexpected system module"
> 
> The string provides no value.
> 

That's just, like, your opinion, man. It has value to me.


Compare:

```
#!/usr/bin/env python3

def main():
     assert False

if __name__ == '__main__':
     main()
```

```
# ./foo.py

Traceback (most recent call last):
   File "./foo.py", line 7, in <module>
     main()
   File "./foo.py", line 4, in main
     assert False
AssertionError
```

With:


```
#!/usr/bin/env python3

def main():
     assert False, "Unexpected system module"

if __name__ == '__main__':
     main()
```

```
# ./foo.py

Traceback (most recent call last):
   File "./foo.py", line 7, in <module>
     main()
   File "./foo.py", line 4, in main
     assert False, "Unexpected system module"
AssertionError: Unexpected system module
```

I like the extra string for giving some semantic context as to 
specifically what broke (We don't expect to see system modules here) 
instead of just a stack trace.

>> +            self._add_module(module.name, self._user_blurb)
>>               self._begin_user_module(module.name)
>>   
>>       def visit_include(self, name: str, info: QAPISourceInfo) -> None:

Re: [PATCH v3 05/17] qapi: pass QAPISchemaModule to visit_module instead of str

[John Snow]

On 1/20/21 11:02 AM, Eric Blake wrote:
> On 1/20/21 6:07 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> Modify visit_module to pass the module itself instead of just its
>>> name. This allows for future patches to centralize some
>>> module-interrogation behavior within the QAPISchemaModule class itself,
>>> cutting down on duplication between gen.py and schema.py.
>>
>> We've been tempted to make similar changes before (don't worry, I'm not
>> building a case for "no" here).
>>
>> When I wrote the initial version of QAPISchemaVisitor (commit 3f7dc21be,
>> 2015), I aimed for a loose coupling of backends and the internal
>> representation.  Instead of
>>
>>      def visit_foo(self, foo):
>>          pass
>>
>> where @foo is a QAPISchemaFooBar, I wrote
>>
>>      def visit_foo_bar(self, name, info, [curated attributes of @foo]):
>>          pass
>>
>> In theory, this is nice: the information exposed to the backends is
>> obvious, and the backends can't accidentally mutate @foo.
>>
>> In practice, it kind of failed right then and there:
>>
>>      def visit_object_type(self, name, info, base, members, variants):
>>          pass
>>
>> We avoid passing the QAPISchemaObjectType (loose coupling, cool!), only
>> to pass member information as List[QAPISchemaObjectTypeMember].
>>
>> Morever, passing "curated atttibutes" has led to visit_commands() taking
>> a dozen arguments.  Meh.
>>
>> This had made Eric and me wonder whether we should write off the
>> decoupling idea as misguided, and just pass the object instead of
>> "curated attributes", always.  Thoughts?
> 
> I'm open to the idea of passing just the larger object instead of the
> curated list of relevant attributes.  It's a bit more coupling, but I
> don't see any of our qapi code being reused outside its current scope
> where the extra coupling will bite us.  But I'm not volunteering for the
> refactoring work, because I'm not an expert on python typing hints.  If
> consolidating parameters into the larger object makes for fewer
> parameters and easier typing hints, I'm assuming the work can be done as
> part of static typing; but if leaving things as they currently are is
> manageable, that's also fine by me.
> 

Yeah, it can definitely be left as-is for now. I've already gone through 
all the effort of typing out all of the interfaces, so it's not really a 
huge ordeal to just leave it as-is.

Passing the objects might be nicer for the future, though, as routing 
new information or features will involve less churn. (And the signatures 
will be a lot smaller.)

I suppose it does open us up to callers mutating the schema in the 
visitors, but they could already do that for the reasons that Markus 
points out. It's possible that the visitor dispatch could be modified to 
make deep copies of schema objects, but that adds overhead.

I can just revert this change for now and leave the topic for another day.

--js

Re: [PATCH v3 05/17] qapi: pass QAPISchemaModule to visit_module instead of str

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 1/20/21 7:07 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
[...]
>>> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
>>> index e03abcbb959..f754f675d66 100644
>>> --- a/docs/sphinx/qapidoc.py
>>> +++ b/docs/sphinx/qapidoc.py
>>> @@ -463,11 +463,11 @@ def __init__(self, env, qapidir):
>>>           self._env = env
>>>           self._qapidir = qapidir
>>>   -    def visit_module(self, name):
>>> -        if name is not None:
>>> -            qapifile = self._qapidir + '/' + name
>>> +    def visit_module(self, module):
>>> +        if module.name:
>> Replacing the "is not None" test by (implicit) "is thruthy" changes
>> behavior for the empty string.  Intentional?
>> 
>
> Instinctively it was intentional, consciously it wasn't. I was worried
> about what "qapifile" would produce if the string happened to be
> empty.

It would produce a dependency on the directory.

>> I've had the "pleasure" of debugging empty strings getting interpreted
>> like None where they should be interpreted like any other string.
>> 
>
> assert module.name, then?

Let's avoid changing behavior in a refactoring patch.  If you want an
assertion to ease your worry, separate patch.

>>> +            qapifile = self._qapidir + '/' + module.name
>>>               self._env.note_dependency(os.path.abspath(qapifile))
>>> -        super().visit_module(name)
>>> +        super().visit_module(module)
>>>     
>> [...]
>> 

Re: [PATCH v3 05/17] qapi: pass QAPISchemaModule to visit_module instead of str

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 1/20/21 11:02 AM, Eric Blake wrote:
>> On 1/20/21 6:07 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> Modify visit_module to pass the module itself instead of just its
>>>> name. This allows for future patches to centralize some
>>>> module-interrogation behavior within the QAPISchemaModule class itself,
>>>> cutting down on duplication between gen.py and schema.py.
>>>
>>> We've been tempted to make similar changes before (don't worry, I'm not
>>> building a case for "no" here).
>>>
>>> When I wrote the initial version of QAPISchemaVisitor (commit 3f7dc21be,
>>> 2015), I aimed for a loose coupling of backends and the internal
>>> representation.  Instead of
>>>
>>>      def visit_foo(self, foo):
>>>          pass
>>>
>>> where @foo is a QAPISchemaFooBar, I wrote
>>>
>>>      def visit_foo_bar(self, name, info, [curated attributes of @foo]):
>>>          pass
>>>
>>> In theory, this is nice: the information exposed to the backends is
>>> obvious, and the backends can't accidentally mutate @foo.
>>>
>>> In practice, it kind of failed right then and there:
>>>
>>>      def visit_object_type(self, name, info, base, members, variants):
>>>          pass
>>>
>>> We avoid passing the QAPISchemaObjectType (loose coupling, cool!), only
>>> to pass member information as List[QAPISchemaObjectTypeMember].
>>>
>>> Morever, passing "curated atttibutes" has led to visit_commands() taking
>>> a dozen arguments.  Meh.
>>>
>>> This had made Eric and me wonder whether we should write off the
>>> decoupling idea as misguided, and just pass the object instead of
>>> "curated attributes", always.  Thoughts?
>> I'm open to the idea of passing just the larger object instead of
>> the
>> curated list of relevant attributes.  It's a bit more coupling, but I
>> don't see any of our qapi code being reused outside its current scope
>> where the extra coupling will bite us.  But I'm not volunteering for the
>> refactoring work, because I'm not an expert on python typing hints.  If
>> consolidating parameters into the larger object makes for fewer
>> parameters and easier typing hints, I'm assuming the work can be done as
>> part of static typing; but if leaving things as they currently are is
>> manageable, that's also fine by me.
>> 
>
> Yeah, it can definitely be left as-is for now. I've already gone
> through all the effort of typing out all of the interfaces, so it's
> not really a huge ordeal to just leave it as-is.
>
> Passing the objects might be nicer for the future, though, as routing
> new information or features will involve less churn. (And the
> signatures will be a lot smaller.)
>
> I suppose it does open us up to callers mutating the schema in the
> visitors, but they could already do that for the reasons that Markus 
> points out. It's possible that the visitor dispatch could be modified
> to make deep copies of schema objects, but that adds overhead.
>
> I can just revert this change for now and leave the topic for another day.

Works for me.  Thanks!

Re: [PATCH v3 10/17] qapi/gen: Combine ._add_[user|system]_module

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 1/20/21 9:20 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
[...]
>>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>>> index 55acd7e080d..b5505685e6e 100644
>>> --- a/scripts/qapi/gen.py
>>> +++ b/scripts/qapi/gen.py
[...]
>>> @@ -313,7 +306,8 @@ def visit_module(self, module: QAPISchemaModule) -> None:
>>>                   self._genc = None
>>>                   self._genh = None
>>>           else:
>>> -            self._add_user_module(module.name, self._user_blurb)
>>> +            assert module.user_module, "Unexpected system module"
>> The string provides no value.
>> 
>
> That's just, like, your opinion, man. It has value to me.
>
>
> Compare:
>
> ```
> #!/usr/bin/env python3
>
> def main():
>     assert False
>
> if __name__ == '__main__':
>     main()
> ```
>
> ```
> # ./foo.py
>
> Traceback (most recent call last):
>   File "./foo.py", line 7, in <module>
>     main()
>   File "./foo.py", line 4, in main
>     assert False
> AssertionError
> ```
>
> With:
>
>
> ```
> #!/usr/bin/env python3
>
> def main():
>     assert False, "Unexpected system module"
>
> if __name__ == '__main__':
>     main()
> ```
>
> ```
> # ./foo.py
>
> Traceback (most recent call last):
>   File "./foo.py", line 7, in <module>
>     main()
>   File "./foo.py", line 4, in main
>     assert False, "Unexpected system module"
> AssertionError: Unexpected system module
> ```
>
> I like the extra string for giving some semantic context as to
> specifically what broke (We don't expect to see system modules here) 
> instead of just a stack trace.

Your test uses assert with an argument that tells us nothing.  But the
assert we're arguing about has a nice, expressive argument.

The string improves the report from

      File "/work/armbru/qemu/scripts/qapi/gen.py", line 325, in visit_module
        assert module.user_module
    AssertionError

to

      File "/work/armbru/qemu/scripts/qapi/gen.py", line 325, in visit_module
        assert module.user_module, "Unexpected system module"
    AssertionError: Unexpected system module

Even if you value the difference, I very much doubt it justifies the
clutter.  Also, slippery slope towards pigs wearing way too much
lipstick.

Tested with

    diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
    index f3f4d2b011..bbfb30dc5a 100644
    --- a/scripts/qapi/gen.py
    +++ b/scripts/qapi/gen.py
    @@ -321,6 +321,7 @@ class QAPISchemaModularCVisitor(QAPISchemaVisitor):
                     # be generated.
                     self._current_module = None
             else:
    +            module.name = "./HACK"
                 assert module.user_module, "Unexpected system module"
                 self._add_module(module.name, self._user_blurb)
                 self._begin_user_module(module.name)


>
>>> +            self._add_module(module.name, self._user_blurb)
>>>               self._begin_user_module(module.name)
>>>         def visit_include(self, name: str, info: QAPISourceInfo) ->
>>> None: