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

[PATCH 00/12] 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 eliminate Optional[T] in favor of T
wherever possible. Optional types used for object properties,
function/method parameters and return values where we expect, in most
cases, to be non-None is troublesome as it requires peppering the code
with assertions about state. If and whenever possible, prefer using
non-Optional types.

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

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

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: assert that _start_if is not None in _wrap_ifcond
  qapi/gen: use './builtin' for the built-in module name
  qapi/source: Add builtin null-object sentinel
  qapi/gen: write _genc/_genh access shims
  qapi/schema: make QAPISourceInfo mandatory
  qapi/gen: move write method to QAPIGenC, make fname a str
  tests/qapi-schema: Add quotes to module name in test output
  qapi/schema: Name the builtin module "" instead of None
  qapi: enable strict-optional checks

 scripts/qapi/commands.py                 |  11 ++-
 scripts/qapi/events.py                   |  14 +--
 scripts/qapi/gen.py                      | 107 ++++++++++++-----------
 scripts/qapi/main.py                     |   3 +-
 scripts/qapi/mypy.ini                    |   1 -
 scripts/qapi/schema.py                   |  33 +++----
 scripts/qapi/source.py                   |  20 ++++-
 scripts/qapi/types.py                    |  11 +--
 scripts/qapi/visit.py                    |   8 +-
 tests/qapi-schema/comments.out           |   4 +-
 tests/qapi-schema/doc-good.out           |   4 +-
 tests/qapi-schema/empty.out              |   4 +-
 tests/qapi-schema/event-case.out         |   4 +-
 tests/qapi-schema/include-repetition.out |   8 +-
 tests/qapi-schema/include-simple.out     |   6 +-
 tests/qapi-schema/indented-expr.out      |   4 +-
 tests/qapi-schema/qapi-schema-test.out   |   8 +-
 tests/qapi-schema/test-qapi.py           |   2 +-
 18 files changed, 143 insertions(+), 109 deletions(-)

-- 
2.26.2

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

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

[PATCH 03/12] 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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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

[PATCH 04/12] qapi/gen: assert that _start_if is not None in _wrap_ifcond

[John Snow]

We already assert this in end_if, but that's opaque to mypy. Do it in
_wrap_ifcond instead. Same effect at runtime, but mypy can now infer
the type in _wrap_ifcond's body.

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

diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index b40f18eee3cd..a6dc991b1d03 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -130,11 +130,11 @@ def start_if(self, ifcond: List[str]) -> None:
         self._start_if = (ifcond, self._body, self._preamble)
 
     def end_if(self) -> None:
-        assert self._start_if
         self._wrap_ifcond()
         self._start_if = None
 
     def _wrap_ifcond(self) -> None:
+        assert self._start_if
         self._body = _wrap_ifcond(self._start_if[0],
                                   self._start_if[1], self._body)
         self._preamble = _wrap_ifcond(self._start_if[0],
-- 
2.26.2

[PATCH 05/12] qapi/gen: use './builtin' for the built-in module name

[John Snow]

Use this in preference to 'None', which helps remove some edge cases in
the typing.

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

diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index a6dc991b1d03..0c5d1fee6088 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -245,23 +245,23 @@ def __init__(self,
         self._pydoc = pydoc
         self._genc: Optional[QAPIGenC] = None
         self._genh: Optional[QAPIGenH] = None
-        self._module: Dict[Optional[str], Tuple[QAPIGenC, QAPIGenH]] = {}
+        self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH]] = {}
         self._main_module: Optional[str] = None
 
     @staticmethod
-    def _is_user_module(name: Optional[str]) -> bool:
-        return bool(name and not name.startswith('./'))
+    def _is_user_module(name: str) -> bool:
+        return not name.startswith('./')
 
     @staticmethod
-    def _is_builtin_module(name: Optional[str]) -> bool:
-        return not name
+    def _is_builtin_module(name: str) -> bool:
+        return name == './builtin'
 
-    def _module_dirname(self, name: Optional[str]) -> str:
+    def _module_dirname(self, name: str) -> str:
         if self._is_user_module(name):
             return os.path.dirname(name)
         return ''
 
-    def _module_basename(self, what: str, name: Optional[str]) -> str:
+    def _module_basename(self, what: str, name: str) -> str:
         ret = '' if self._is_builtin_module(name) else self._prefix
         if self._is_user_module(name):
             basename = os.path.basename(name)
@@ -269,15 +269,14 @@ def _module_basename(self, what: str, name: Optional[str]) -> str:
             if name != self._main_module:
                 ret += '-' + os.path.splitext(basename)[0]
         else:
-            name = name[2:] if name else 'builtin'
-            ret += re.sub(r'-', '-' + name + '-', what)
+            ret += re.sub(r'-', '-' + name[2:] + '-', what)
         return ret
 
-    def _module_filename(self, what: str, name: Optional[str]) -> str:
+    def _module_filename(self, what: str, name: str) -> str:
         return os.path.join(self._module_dirname(name),
                             self._module_basename(what, name))
 
-    def _add_module(self, name: Optional[str], blurb: str) -> None:
+    def _add_module(self, name: str, blurb: str) -> None:
         basename = self._module_filename(self._what, name)
         genc = QAPIGenC(basename + '.c', blurb, self._pydoc)
         genh = QAPIGenH(basename + '.h', blurb, self._pydoc)
@@ -290,8 +289,8 @@ def _add_user_module(self, name: str, blurb: str) -> None:
             self._main_module = name
         self._add_module(name, blurb)
 
-    def _add_system_module(self, name: Optional[str], blurb: str) -> None:
-        self._add_module(name and './' + name, blurb)
+    def _add_system_module(self, name: str, blurb: str) -> None:
+        self._add_module(f"./{name}", blurb)
 
     def write(self, output_dir: str, opt_builtins: bool = False) -> None:
         for name in self._module:
@@ -310,7 +309,7 @@ def _begin_user_module(self, name: str) -> None:
     def visit_module(self, name: Optional[str]) -> None:
         if name is None:
             if self._builtin_blurb:
-                self._add_system_module(None, self._builtin_blurb)
+                self._add_system_module('builtin', self._builtin_blurb)
                 self._begin_system_module(name)
             else:
                 # The built-in module has not been created.  No code may
-- 
2.26.2

[PATCH 06/12] qapi/source: Add builtin null-object sentinel

[John Snow]

We use None to represent an object that has no source information
because it's a builtin. This complicates interface typing, since many
interfaces expect that there is an info object available to print errors
with.

Introduce a special QAPISourceInfo that represents these built-ins so
that if an error should so happen to occur relating to one of these
builtins that we will be able to print its information, and interface
typing becomes simpler: you will always have a source info object.

This object will evaluate as False, so "if info" is a valid idiomatic
construct.

NB: It was intentional to not allow empty constructors or similar to
create "empty" source info objects; callers must explicitly invoke
'builtin()' to pro-actively opt into using the sentinel. This should
prevent use-by-accident.

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

diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
index d7a79a9b8aee..64af7318cb67 100644
--- a/scripts/qapi/source.py
+++ b/scripts/qapi/source.py
@@ -11,7 +11,12 @@
 
 import copy
 import sys
-from typing import List, Optional, TypeVar
+from typing import (
+    List,
+    Optional,
+    Type,
+    TypeVar,
+)
 
 
 class QAPISchemaPragma:
@@ -41,6 +46,17 @@ def __init__(self, fname: str, line: int,
         self.defn_meta: Optional[str] = None
         self.defn_name: Optional[str] = None
 
+    @classmethod
+    def builtin(cls: Type[T]) -> T:
+        """
+        Create a SourceInfo object corresponding to a builtin definition.
+        """
+        return cls('', -1, None)
+
+    def __bool__(self) -> bool:
+        # "if info: ..." is false if info is the builtin sentinel.
+        return bool(self.fname)
+
     def set_defn(self, meta: str, name: str) -> None:
         self.defn_meta = meta
         self.defn_name = name
@@ -73,4 +89,6 @@ def include_path(self) -> str:
         return ret
 
     def __str__(self) -> str:
+        if not bool(self):
+            return '[builtin]'
         return self.include_path() + self.in_defn() + self.loc()
-- 
2.26.2

[PATCH 07/12] 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 0c5d1fee6088..17ae9f4af703 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -243,11 +243,20 @@ def __init__(self,
         self._user_blurb = user_blurb
         self._builtin_blurb = builtin_blurb
         self._pydoc = pydoc
-        self._genc: Optional[QAPIGenC] = None
-        self._genh: Optional[QAPIGenH] = None
+        self._current_module: Optional[str] = None
         self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH]] = {}
         self._main_module: Optional[str] = None
 
+    @property
+    def _genc(self) -> QAPIGenC:
+        assert self._current_module is not None
+        return self._module[self._current_module][0]
+
+    @property
+    def _genh(self) -> QAPIGenH:
+        assert self._current_module is not None
+        return self._module[self._current_module][1]
+
     @staticmethod
     def _is_user_module(name: str) -> bool:
         return not name.startswith('./')
@@ -281,7 +290,7 @@ def _add_module(self, name: str, blurb: str) -> None:
         genc = QAPIGenC(basename + '.c', blurb, self._pydoc)
         genh = QAPIGenH(basename + '.h', blurb, self._pydoc)
         self._module[name] = (genc, genh)
-        self._genc, self._genh = self._module[name]
+        self._current_module = name
 
     def _add_user_module(self, name: str, blurb: str) -> None:
         assert self._is_user_module(name)
@@ -314,8 +323,7 @@ def visit_module(self, name: Optional[str]) -> None:
             else:
                 # The built-in module has not been created.  No code may
                 # be generated.
-                self._genc = None
-                self._genh = None
+                self._current_module = None
         else:
             self._add_user_module(name, self._user_blurb)
             self._begin_user_module(name)
-- 
2.26.2

[PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory

[John Snow]

--

events.py had an info to route, was it by choice that it wasn't before?
Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/events.py |  2 +-
 scripts/qapi/schema.py | 23 +++++++++++++----------
 scripts/qapi/types.py  |  9 +++++----
 scripts/qapi/visit.py  |  6 +++---
 4 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index 9851653b9d11..9ba4f109028d 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -225,7 +225,7 @@ def visit_event(self,
                                           self._event_emit_name))
         # Note: we generate the enum member regardless of @ifcond, to
         # keep the enumeration usable in target-independent code.
-        self._event_enum_members.append(QAPISchemaEnumMember(name, None))
+        self._event_enum_members.append(QAPISchemaEnumMember(name, info))
 
 
 def gen_events(schema: QAPISchema,
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 720449feee4d..d5f19732b516 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -23,6 +23,7 @@
 from .error import QAPIError, QAPISemError
 from .expr import check_exprs
 from .parser import QAPISchemaParser
+from .source import QAPISourceInfo
 
 
 class QAPISchemaEntity:
@@ -36,10 +37,10 @@ def __init__(self, name, info, doc, ifcond=None, features=None):
         self.name = name
         self._module = None
         # For explicitly defined entities, info points to the (explicit)
-        # definition.  For builtins (and their arrays), info is None.
-        # For implicitly defined entities, info points to a place that
-        # triggered the implicit definition (there may be more than one
-        # such place).
+        # definition.  For builtins (and their arrays), info is a null-object
+        # sentinel that evaluates to False. For implicitly defined entities,
+        # info points to a place that triggered the implicit definition
+        # (there may be more than one such place).
         self.info = info
         self.doc = doc
         self._ifcond = ifcond or []
@@ -209,7 +210,7 @@ class QAPISchemaBuiltinType(QAPISchemaType):
     meta = 'built-in'
 
     def __init__(self, name, json_type, c_type):
-        super().__init__(name, None, None)
+        super().__init__(name, QAPISourceInfo.builtin(), None)
         assert not c_type or isinstance(c_type, str)
         assert json_type in ('string', 'number', 'int', 'boolean', 'null',
                              'value')
@@ -871,7 +872,7 @@ def resolve_type(self, name, info, what):
         return typ
 
     def _module_name(self, fname):
-        if fname is None:
+        if not fname:
             return None
         return os.path.relpath(fname, self._schema_dir)
 
@@ -897,9 +898,11 @@ def _def_builtin_type(self, name, json_type, c_type):
         # be nice, but we can't as long as their generated code
         # (qapi-builtin-types.[ch]) may be shared by some other
         # schema.
-        self._make_array_type(name, None)
+        self._make_array_type(name, QAPISourceInfo.builtin())
 
     def _def_predefineds(self):
+        info = QAPISourceInfo.builtin()
+
         for t in [('str',    'string',  'char' + POINTER_SUFFIX),
                   ('number', 'number',  'double'),
                   ('int',    'int',     'int64_t'),
@@ -917,15 +920,15 @@ def _def_predefineds(self):
                   ('null',   'null',    'QNull' + POINTER_SUFFIX)]:
             self._def_builtin_type(*t)
         self.the_empty_object_type = QAPISchemaObjectType(
-            'q_empty', None, None, None, None, None, [], None)
+            'q_empty', info, None, None, None, None, [], None)
         self._def_entity(self.the_empty_object_type)
 
         qtypes = ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist',
                   'qbool']
         qtype_values = self._make_enum_members(
-            [{'name': n} for n in qtypes], None)
+            [{'name': n} for n in qtypes], info)
 
-        self._def_entity(QAPISchemaEnumType('QType', None, None, None, None,
+        self._def_entity(QAPISchemaEnumType('QType', info, None, None, None,
                                             qtype_values, 'QTYPE'))
 
     def _make_features(self, features, info):
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index 2b4916cdaa1b..a3a16284006b 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -71,7 +71,8 @@ def gen_enum(name: str,
              members: List[QAPISchemaEnumMember],
              prefix: Optional[str] = None) -> str:
     # append automatically generated _MAX value
-    enum_members = members + [QAPISchemaEnumMember('_MAX', None)]
+    enum_members = members + [
+        QAPISchemaEnumMember('_MAX', QAPISourceInfo.builtin())]
 
     ret = mcgen('''
 
@@ -306,7 +307,7 @@ def _gen_type_cleanup(self, name: str) -> None:
 
     def visit_enum_type(self,
                         name: str,
-                        info: Optional[QAPISourceInfo],
+                        info: QAPISourceInfo,
                         ifcond: List[str],
                         features: List[QAPISchemaFeature],
                         members: List[QAPISchemaEnumMember],
@@ -317,7 +318,7 @@ def visit_enum_type(self,
 
     def visit_array_type(self,
                          name: str,
-                         info: Optional[QAPISourceInfo],
+                         info: QAPISourceInfo,
                          ifcond: List[str],
                          element_type: QAPISchemaType) -> None:
         with ifcontext(ifcond, self._genh, self._genc):
@@ -327,7 +328,7 @@ def visit_array_type(self,
 
     def visit_object_type(self,
                           name: str,
-                          info: Optional[QAPISourceInfo],
+                          info: QAPISourceInfo,
                           ifcond: List[str],
                           features: List[QAPISchemaFeature],
                           base: Optional[QAPISchemaObjectType],
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 339f1521524c..3f49c307c566 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -336,7 +336,7 @@ def _begin_user_module(self, name: str) -> None:
 
     def visit_enum_type(self,
                         name: str,
-                        info: QAPISourceInfo,
+                        info: Optional[QAPISourceInfo],
                         ifcond: List[str],
                         features: List[QAPISchemaFeature],
                         members: List[QAPISchemaEnumMember],
@@ -347,7 +347,7 @@ def visit_enum_type(self,
 
     def visit_array_type(self,
                          name: str,
-                         info: Optional[QAPISourceInfo],
+                         info: QAPISourceInfo,
                          ifcond: List[str],
                          element_type: QAPISchemaType) -> None:
         with ifcontext(ifcond, self._genh, self._genc):
@@ -356,7 +356,7 @@ def visit_array_type(self,
 
     def visit_object_type(self,
                           name: str,
-                          info: Optional[QAPISourceInfo],
+                          info: QAPISourceInfo,
                           ifcond: List[str],
                           features: List[QAPISchemaFeature],
                           base: Optional[QAPISchemaObjectType],
-- 
2.26.2

[PATCH 09/12] qapi/gen: move write method to QAPIGenC, make fname a str

[John Snow]

QAPIGenC and QAPIGenH in particular depend on fname being defined, but
we have a usage of QAPIGenCCode that isn't intended to be associated
with a particular file.

No problem, move the write method down to the class that actually needs
it, and keep QAPIGenCCode more abstract.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/commands.py |  2 +-
 scripts/qapi/gen.py      | 54 ++++++++++++++++++++--------------------
 2 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 71744f48a353..b346676d15a0 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -258,7 +258,7 @@ def __init__(self, prefix: str):
         super().__init__(
             prefix, 'qapi-commands',
             ' * Schema-defined QAPI/QMP commands', None, __doc__)
-        self._regy = QAPIGenCCode(None)
+        self._regy = QAPIGenCCode()
         self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]] = {}
 
     def _begin_user_module(self, name: str) -> None:
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 17ae9f4af703..b2c89213d838 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -36,8 +36,7 @@
 
 
 class QAPIGen:
-    def __init__(self, fname: Optional[str]):
-        self.fname = fname
+    def __init__(self) -> None:
         self._preamble = ''
         self._body = ''
 
@@ -58,28 +57,6 @@ def _bottom(self) -> str:
         # pylint: disable=no-self-use
         return ''
 
-    def write(self, output_dir: str) -> None:
-        # Include paths starting with ../ are used to reuse modules of the main
-        # schema in specialised schemas. Don't overwrite the files that are
-        # already generated for the main schema.
-        if self.fname.startswith('../'):
-            return
-        pathname = os.path.join(output_dir, self.fname)
-        odir = os.path.dirname(pathname)
-
-        if odir:
-            os.makedirs(odir, exist_ok=True)
-
-        # use os.open for O_CREAT to create and read a non-existant file
-        fd = os.open(pathname, os.O_RDWR | os.O_CREAT, 0o666)
-        with os.fdopen(fd, 'r+', encoding='utf-8') as fp:
-            text = self.get_content()
-            oldtext = fp.read(len(text) + 1)
-            if text != oldtext:
-                fp.seek(0)
-                fp.truncate(0)
-                fp.write(text)
-
 
 def _wrap_ifcond(ifcond: List[str], before: str, after: str) -> str:
     if before == after:
@@ -121,8 +98,8 @@ def build_params(arg_type: Optional[QAPISchemaObjectType],
 
 
 class QAPIGenCCode(QAPIGen):
-    def __init__(self, fname: Optional[str]):
-        super().__init__(fname)
+    def __init__(self) -> None:
+        super().__init__()
         self._start_if: Optional[Tuple[List[str], str, str]] = None
 
     def start_if(self, ifcond: List[str]) -> None:
@@ -147,11 +124,34 @@ def get_content(self) -> str:
 
 class QAPIGenC(QAPIGenCCode):
     def __init__(self, fname: str, blurb: str, pydoc: str):
-        super().__init__(fname)
+        super().__init__()
+        self.fname = fname
         self._blurb = blurb
         self._copyright = '\n * '.join(re.findall(r'^Copyright .*', pydoc,
                                                   re.MULTILINE))
 
+    def write(self, output_dir: str) -> None:
+        # Include paths starting with ../ are used to reuse modules of the main
+        # schema in specialised schemas. Don't overwrite the files that are
+        # already generated for the main schema.
+        if self.fname.startswith('../'):
+            return
+        pathname = os.path.join(output_dir, self.fname)
+        odir = os.path.dirname(pathname)
+
+        if odir:
+            os.makedirs(odir, exist_ok=True)
+
+        # use os.open for O_CREAT to create and read a non-existant file
+        fd = os.open(pathname, os.O_RDWR | os.O_CREAT, 0o666)
+        with os.fdopen(fd, 'r+', encoding='utf-8') as fp:
+            text = self.get_content()
+            oldtext = fp.read(len(text) + 1)
+            if text != oldtext:
+                fp.seek(0)
+                fp.truncate(0)
+                fp.write(text)
+
     def _top(self) -> str:
         return mcgen('''
 /* AUTOMATICALLY GENERATED, DO NOT MODIFY */
-- 
2.26.2

[PATCH 10/12] tests/qapi-schema: Add quotes to module name in test output

[John Snow]

A forthcoming patch is going to allow the empty string as a name for the
builtin module, and quotes will help us see that in test output. Without
this, git will be upset about trailing empty spaces in test output, so
the quotes are necessary.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qapi-schema/comments.out           | 4 ++--
 tests/qapi-schema/doc-good.out           | 4 ++--
 tests/qapi-schema/empty.out              | 4 ++--
 tests/qapi-schema/event-case.out         | 4 ++--
 tests/qapi-schema/include-repetition.out | 8 ++++----
 tests/qapi-schema/include-simple.out     | 6 +++---
 tests/qapi-schema/indented-expr.out      | 4 ++--
 tests/qapi-schema/qapi-schema-test.out   | 8 ++++----
 tests/qapi-schema/test-qapi.py           | 2 +-
 9 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/tests/qapi-schema/comments.out b/tests/qapi-schema/comments.out
index 273f0f54e16c..08aba8354e2a 100644
--- a/tests/qapi-schema/comments.out
+++ b/tests/qapi-schema/comments.out
@@ -1,4 +1,4 @@
-module None
+module "None"
 object q_empty
 enum QType
     prefix QTYPE
@@ -9,7 +9,7 @@ enum QType
     member qdict
     member qlist
     member qbool
-module comments.json
+module "comments.json"
 enum Status
     member good
     member bad
diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
index 419284dae29b..83a3d9bd69b5 100644
--- a/tests/qapi-schema/doc-good.out
+++ b/tests/qapi-schema/doc-good.out
@@ -1,4 +1,4 @@
-module None
+module "None"
 object q_empty
 enum QType
     prefix QTYPE
@@ -9,7 +9,7 @@ enum QType
     member qdict
     member qlist
     member qbool
-module doc-good.json
+module "doc-good.json"
 enum Enum
     member one
         if ['defined(IFONE)']
diff --git a/tests/qapi-schema/empty.out b/tests/qapi-schema/empty.out
index 69666c39ad2d..0dac23c80c15 100644
--- a/tests/qapi-schema/empty.out
+++ b/tests/qapi-schema/empty.out
@@ -1,4 +1,4 @@
-module None
+module "None"
 object q_empty
 enum QType
     prefix QTYPE
@@ -9,4 +9,4 @@ enum QType
     member qdict
     member qlist
     member qbool
-module empty.json
+module "empty.json"
diff --git a/tests/qapi-schema/event-case.out b/tests/qapi-schema/event-case.out
index 42ae519656dc..ace511ba5a96 100644
--- a/tests/qapi-schema/event-case.out
+++ b/tests/qapi-schema/event-case.out
@@ -1,4 +1,4 @@
-module None
+module "None"
 object q_empty
 enum QType
     prefix QTYPE
@@ -9,6 +9,6 @@ enum QType
     member qdict
     member qlist
     member qbool
-module event-case.json
+module "event-case.json"
 event oops None
     boxed=False
diff --git a/tests/qapi-schema/include-repetition.out b/tests/qapi-schema/include-repetition.out
index 0b654ddebb6a..f7ab4987943c 100644
--- a/tests/qapi-schema/include-repetition.out
+++ b/tests/qapi-schema/include-repetition.out
@@ -1,4 +1,4 @@
-module None
+module "None"
 object q_empty
 enum QType
     prefix QTYPE
@@ -9,15 +9,15 @@ enum QType
     member qdict
     member qlist
     member qbool
-module include-repetition.json
+module "include-repetition.json"
 include comments.json
 include include-repetition-sub.json
 include comments.json
-module comments.json
+module "comments.json"
 enum Status
     member good
     member bad
     member ugly
-module include-repetition-sub.json
+module "include-repetition-sub.json"
 include comments.json
 include comments.json
diff --git a/tests/qapi-schema/include-simple.out b/tests/qapi-schema/include-simple.out
index 061f81e50904..81bdeb887b66 100644
--- a/tests/qapi-schema/include-simple.out
+++ b/tests/qapi-schema/include-simple.out
@@ -1,4 +1,4 @@
-module None
+module "None"
 object q_empty
 enum QType
     prefix QTYPE
@@ -9,9 +9,9 @@ enum QType
     member qdict
     member qlist
     member qbool
-module include-simple.json
+module "include-simple.json"
 include include-simple-sub.json
-module include-simple-sub.json
+module "include-simple-sub.json"
 enum Status
     member good
     member bad
diff --git a/tests/qapi-schema/indented-expr.out b/tests/qapi-schema/indented-expr.out
index 04356775cd16..361a58185e67 100644
--- a/tests/qapi-schema/indented-expr.out
+++ b/tests/qapi-schema/indented-expr.out
@@ -1,4 +1,4 @@
-module None
+module "None"
 object q_empty
 enum QType
     prefix QTYPE
@@ -9,7 +9,7 @@ enum QType
     member qdict
     member qlist
     member qbool
-module indented-expr.json
+module "indented-expr.json"
 command eins None -> None
     gen=True success_response=True boxed=False oob=False preconfig=False
 command zwei None -> None
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 8868ca0dca9e..4f5ab9fd596c 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -1,4 +1,4 @@
-module None
+module "None"
 object q_empty
 enum QType
     prefix QTYPE
@@ -9,7 +9,7 @@ enum QType
     member qdict
     member qlist
     member qbool
-module qapi-schema-test.json
+module "qapi-schema-test.json"
 object TestStruct
     member integer: int optional=False
     member boolean: bool optional=False
@@ -443,11 +443,11 @@ command test-command-cond-features3 None -> None
 event TEST-EVENT-FEATURES1 None
     boxed=False
     feature deprecated
-module include/sub-module.json
+module "include/sub-module.json"
 include sub-sub-module.json
 object SecondArrayRef
     member s: StatusList optional=False
-module sub-sub-module.json
+module "sub-sub-module.json"
 array StatusList Status
 enum Status
     member good
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index e8db9d09d914..4adf0b3c1857 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -25,7 +25,7 @@
 class QAPISchemaTestVisitor(QAPISchemaVisitor):
 
     def visit_module(self, name):
-        print('module %s' % name)
+        print('module "%s"' % name)
 
     def visit_include(self, name, info):
         print('include %s' % name)
-- 
2.26.2

[PATCH 11/12] qapi/schema: Name the builtin module "" instead of None

[John Snow]

Instead of using None as the built-in module filename, use an empty
string instead. This allows us to clarify the type of various interfaces
dealing with module names as always taking a string, which saves us from
having to use Optional[str] everywhere.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/gen.py                      |  6 +++---
 scripts/qapi/schema.py                   | 12 ++++++------
 scripts/qapi/types.py                    |  2 +-
 scripts/qapi/visit.py                    |  2 +-
 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 +-
 12 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index b2c89213d838..a577a4a7f002 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -309,14 +309,14 @@ def write(self, output_dir: str, opt_builtins: bool = False) -> None:
             genc.write(output_dir)
             genh.write(output_dir)
 
-    def _begin_system_module(self, name: None) -> None:
+    def _begin_system_module(self, name: str) -> None:
         pass
 
     def _begin_user_module(self, name: str) -> None:
         pass
 
-    def visit_module(self, name: Optional[str]) -> None:
-        if name is None:
+    def visit_module(self, name: str) -> None:
+        if not name:
             if self._builtin_blurb:
                 self._add_system_module('builtin', self._builtin_blurb)
                 self._begin_system_module(name)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index d5f19732b516..8d8b8758f65e 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -69,7 +69,7 @@ def check_doc(self):
 
     def _set_module(self, schema, info):
         assert self._checked
-        self._module = schema.module_by_fname(info and info.fname)
+        self._module = schema.module_by_fname(info.fname)
         self._module.add_entity(self)
 
     def set_module(self, schema):
@@ -826,7 +826,7 @@ def __init__(self, fname):
         self._entity_dict = {}
         self._module_dict = OrderedDict()
         self._schema_dir = os.path.dirname(fname)
-        self._make_module(None)  # built-ins
+        self._make_module(QAPISourceInfo.builtin().fname)  # built-ins
         self._make_module(fname)
         self._predefining = True
         self._def_predefineds()
@@ -871,10 +871,10 @@ def resolve_type(self, name, info, what):
                 info, "%s uses unknown type '%s'" % (what, name))
         return typ
 
-    def _module_name(self, fname):
-        if not fname:
-            return None
-        return os.path.relpath(fname, self._schema_dir)
+    def _module_name(self, fname: str) -> str:
+        if fname:
+            return os.path.relpath(fname, self._schema_dir)
+        return fname
 
     def _make_module(self, fname):
         name = self._module_name(fname)
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index a3a16284006b..12eeea3aaffe 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -272,7 +272,7 @@ def __init__(self, prefix: str):
             prefix, 'qapi-types', ' * Schema-defined QAPI types',
             ' * Built-in QAPI types', __doc__)
 
-    def _begin_system_module(self, name: None) -> None:
+    def _begin_system_module(self, name: str) -> None:
         self._genc.preamble_add(mcgen('''
 #include "qemu/osdep.h"
 #include "qapi/dealloc-visitor.h"
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 3f49c307c566..76e34ee7f02e 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -305,7 +305,7 @@ def __init__(self, prefix: str):
             prefix, 'qapi-visit', ' * Schema-defined QAPI visitors',
             ' * Built-in QAPI visitors', __doc__)
 
-    def _begin_system_module(self, name: None) -> None:
+    def _begin_system_module(self, name: str) -> None:
         self._genc.preamble_add(mcgen('''
 #include "qemu/osdep.h"
 #include "qapi/error.h"
diff --git a/tests/qapi-schema/comments.out b/tests/qapi-schema/comments.out
index 08aba8354e2a..02000c06e5e0 100644
--- a/tests/qapi-schema/comments.out
+++ b/tests/qapi-schema/comments.out
@@ -1,4 +1,4 @@
-module "None"
+module ""
 object q_empty
 enum QType
     prefix QTYPE
diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
index 83a3d9bd69b5..494533d74793 100644
--- a/tests/qapi-schema/doc-good.out
+++ b/tests/qapi-schema/doc-good.out
@@ -1,4 +1,4 @@
-module "None"
+module ""
 object q_empty
 enum QType
     prefix QTYPE
diff --git a/tests/qapi-schema/empty.out b/tests/qapi-schema/empty.out
index 0dac23c80c15..059caa4e1d2a 100644
--- a/tests/qapi-schema/empty.out
+++ b/tests/qapi-schema/empty.out
@@ -1,4 +1,4 @@
-module "None"
+module ""
 object q_empty
 enum QType
     prefix QTYPE
diff --git a/tests/qapi-schema/event-case.out b/tests/qapi-schema/event-case.out
index ace511ba5a96..4d9d2b519f4b 100644
--- a/tests/qapi-schema/event-case.out
+++ b/tests/qapi-schema/event-case.out
@@ -1,4 +1,4 @@
-module "None"
+module ""
 object q_empty
 enum QType
     prefix QTYPE
diff --git a/tests/qapi-schema/include-repetition.out b/tests/qapi-schema/include-repetition.out
index f7ab4987943c..31d64631b665 100644
--- a/tests/qapi-schema/include-repetition.out
+++ b/tests/qapi-schema/include-repetition.out
@@ -1,4 +1,4 @@
-module "None"
+module ""
 object q_empty
 enum QType
     prefix QTYPE
diff --git a/tests/qapi-schema/include-simple.out b/tests/qapi-schema/include-simple.out
index 81bdeb887b66..1b35b3295713 100644
--- a/tests/qapi-schema/include-simple.out
+++ b/tests/qapi-schema/include-simple.out
@@ -1,4 +1,4 @@
-module "None"
+module ""
 object q_empty
 enum QType
     prefix QTYPE
diff --git a/tests/qapi-schema/indented-expr.out b/tests/qapi-schema/indented-expr.out
index 361a58185e67..aed689e7bd67 100644
--- a/tests/qapi-schema/indented-expr.out
+++ b/tests/qapi-schema/indented-expr.out
@@ -1,4 +1,4 @@
-module "None"
+module ""
 object q_empty
 enum QType
     prefix QTYPE
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 4f5ab9fd596c..4a899ba63ecb 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -1,4 +1,4 @@
-module "None"
+module ""
 object q_empty
 enum QType
     prefix QTYPE
-- 
2.26.2

[PATCH 12/12] 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 74fc6c82153e..04bd5db5278d 100644
--- a/scripts/qapi/mypy.ini
+++ b/scripts/qapi/mypy.ini
@@ -1,6 +1,5 @@
 [mypy]
 strict = True
-strict_optional = False
 disallow_untyped_calls = False
 python_version = 3.6
 
-- 
2.26.2

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Mypy cannot understand that this match can never be None, so help it
> along.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/main.py | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
> index 42517210b805..271d9e84da94 100644
> --- a/scripts/qapi/main.py
> +++ b/scripts/qapi/main.py
> @@ -23,7 +23,8 @@
>  
>  def invalid_prefix_char(prefix: str) -> Optional[str]:
>      match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)

@match can't be None because the regexp always matches a prefix,
possibly the empty prefix.

> -    if match.end() != len(prefix):
> +    # match cannot be None, but mypy cannot infer that.
> +    if match and match.end() != len(prefix):
>          return prefix[match.end()]
>      return None

High-level logic:

       if there is an invalid prefix character:
           return it
       return None

The actual code takes the return None when the match fails.  If this
could happen, it would be wrong.  I can't, so it doesn't matter, but I
dislike it anyway.

Alternative 1: turn "match cannot be None" from comment to code

       match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
  +    assert match
       if match.end() != len(prefix):
           return prefix[match.end()]
       return None

Alternative 2: turn empty prefix into a special case

  -    match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
  +    match = re.match(r'[A-Za-z_.-][A-Za-z0-9_.-]*', prefix)
  +    if not match:
  +        return prefix[0]
       if match.end() != len(prefix):
           return prefix[match.end()]
       return None

I'd prefer alternative 1.

Re: [PATCH 04/12] qapi/gen: assert that _start_if is not None in _wrap_ifcond

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> We already assert this in end_if, but that's opaque to mypy. Do it in
> _wrap_ifcond instead. Same effect at runtime, but mypy can now infer
> the type in _wrap_ifcond's body.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/gen.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index b40f18eee3cd..a6dc991b1d03 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -130,11 +130,11 @@ def start_if(self, ifcond: List[str]) -> None:
>          self._start_if = (ifcond, self._body, self._preamble)
>  
>      def end_if(self) -> None:
> -        assert self._start_if
>          self._wrap_ifcond()
>          self._start_if = None
>  
>      def _wrap_ifcond(self) -> None:
> +        assert self._start_if
>          self._body = _wrap_ifcond(self._start_if[0],
>                                    self._start_if[1], self._body)
>          self._preamble = _wrap_ifcond(self._start_if[0],

Drawback: the public method's precondition is now more opaque.  Do we
care?

Re: [PATCH 05/12] qapi/gen: use './builtin' for the built-in module name

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Use this in preference to 'None', which helps remove some edge cases in
> the typing.
>
> Signed-off-by: John Snow <jsnow@redhat.com>

Clearly better.  Should've done it this way in commit c2e196a9b4 "qapi:
Prepare for system modules other than 'builtin'".

> ---
>  scripts/qapi/gen.py | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index a6dc991b1d03..0c5d1fee6088 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -245,23 +245,23 @@ def __init__(self,
>          self._pydoc = pydoc
>          self._genc: Optional[QAPIGenC] = None
>          self._genh: Optional[QAPIGenH] = None
> -        self._module: Dict[Optional[str], Tuple[QAPIGenC, QAPIGenH]] = {}
> +        self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH]] = {}
>          self._main_module: Optional[str] = None
>  
>      @staticmethod
> -    def _is_user_module(name: Optional[str]) -> bool:
> -        return bool(name and not name.startswith('./'))
> +    def _is_user_module(name: str) -> bool:
> +        return not name.startswith('./')
>  
>      @staticmethod
> -    def _is_builtin_module(name: Optional[str]) -> bool:
> -        return not name
> +    def _is_builtin_module(name: str) -> bool:
> +        return name == './builtin'
>  
> -    def _module_dirname(self, name: Optional[str]) -> str:
> +    def _module_dirname(self, name: str) -> str:
>          if self._is_user_module(name):
>              return os.path.dirname(name)
>          return ''
>  
> -    def _module_basename(self, what: str, name: Optional[str]) -> str:
> +    def _module_basename(self, what: str, name: str) -> str:
>          ret = '' if self._is_builtin_module(name) else self._prefix
>          if self._is_user_module(name):
>              basename = os.path.basename(name)
> @@ -269,15 +269,14 @@ def _module_basename(self, what: str, name: Optional[str]) -> str:
>              if name != self._main_module:
>                  ret += '-' + os.path.splitext(basename)[0]
>          else:

Possible drive-by improvement:

               assert name.startswith('./')

> -            name = name[2:] if name else 'builtin'
> -            ret += re.sub(r'-', '-' + name + '-', what)
> +            ret += re.sub(r'-', '-' + name[2:] + '-', what)
>          return ret
>  
> -    def _module_filename(self, what: str, name: Optional[str]) -> str:
> +    def _module_filename(self, what: str, name: str) -> str:
>          return os.path.join(self._module_dirname(name),
>                              self._module_basename(what, name))
>  
> -    def _add_module(self, name: Optional[str], blurb: str) -> None:
> +    def _add_module(self, name: str, blurb: str) -> None:
>          basename = self._module_filename(self._what, name)
>          genc = QAPIGenC(basename + '.c', blurb, self._pydoc)
>          genh = QAPIGenH(basename + '.h', blurb, self._pydoc)
> @@ -290,8 +289,8 @@ def _add_user_module(self, name: str, blurb: str) -> None:
>              self._main_module = name
>          self._add_module(name, blurb)
>  
> -    def _add_system_module(self, name: Optional[str], blurb: str) -> None:
> -        self._add_module(name and './' + name, blurb)
> +    def _add_system_module(self, name: str, blurb: str) -> None:
> +        self._add_module(f"./{name}", blurb)

I like f-strings, I really do, but is

    f"./{name}"

really clearer than

    './' + name

?

>  
>      def write(self, output_dir: str, opt_builtins: bool = False) -> None:
>          for name in self._module:
> @@ -310,7 +309,7 @@ def _begin_user_module(self, name: str) -> None:
>      def visit_module(self, name: Optional[str]) -> None:
>          if name is None:
>              if self._builtin_blurb:
> -                self._add_system_module(None, self._builtin_blurb)
> +                self._add_system_module('builtin', self._builtin_blurb)
>                  self._begin_system_module(name)
>              else:
>                  # The built-in module has not been created.  No code may

Re: [PATCH 06/12] qapi/source: Add builtin null-object sentinel

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> We use None to represent an object that has no source information
> because it's a builtin. This complicates interface typing, since many
> interfaces expect that there is an info object available to print errors
> with.
>
> Introduce a special QAPISourceInfo that represents these built-ins so
> that if an error should so happen to occur relating to one of these
> builtins that we will be able to print its information, and interface
> typing becomes simpler: you will always have a source info object.

Two aspects mixed up:

1. Represent "no source info" as special QAPISourceInfo instead of
   None

   This is what de-complicates interface typing.

2. On error with "no source info", don't crash.

   I have my doubts on this one.

   Such an error means the QAPI code generator screwed up, at least in
   theory.  Crashing is only proper.  It gets the screwup fixed.

   In practice, errors due to interactions between built-in stuff and
   user-defined stuff could conceivably escape testing.  I can't
   remember such a case offhand.

   Will the "no source info" error be more useful than a crash?
   Possibly.  Will it get the screwup fixed?  Maybe not.

Can we separate the two aspects?

>
> This object will evaluate as False, so "if info" is a valid idiomatic
> construct.

Suggest s/is a valid/remains a valid/.

Not 100% sure we'll want to keep this idiom, but now is not the time to
worry about that.

>
> NB: It was intentional to not allow empty constructors or similar to
> create "empty" source info objects; callers must explicitly invoke
> 'builtin()' to pro-actively opt into using the sentinel. This should
> prevent use-by-accident.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/source.py | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
> index d7a79a9b8aee..64af7318cb67 100644
> --- a/scripts/qapi/source.py
> +++ b/scripts/qapi/source.py
> @@ -11,7 +11,12 @@
>  
>  import copy
>  import sys
> -from typing import List, Optional, TypeVar
> +from typing import (
> +    List,
> +    Optional,
> +    Type,
> +    TypeVar,
> +)
>  
>  
>  class QAPISchemaPragma:
> @@ -41,6 +46,17 @@ def __init__(self, fname: str, line: int,
>          self.defn_meta: Optional[str] = None
>          self.defn_name: Optional[str] = None
>  
> +    @classmethod
> +    def builtin(cls: Type[T]) -> T:
> +        """
> +        Create a SourceInfo object corresponding to a builtin definition.

Let's spell it built-in for consistency with existing comments.

Could perhaps shorten "a SourceInfo object" to "an instance".

> +        """
> +        return cls('', -1, None)

No users?  Peeking ahead... aha, they are in Patch 08.  Any particular
reason for putting PATCH 07 between the two?  Could PATCH 08 be squashed
into this one?

> +
> +    def __bool__(self) -> bool:
> +        # "if info: ..." is false if info is the builtin sentinel.
> +        return bool(self.fname)

Nitpicking...  "The builtin sentinel" suggests there is just one.  PATCH
08 creates several.  I don't mind, but let's say something like "if
@info corresponds to a built-in definition".

> +
>      def set_defn(self, meta: str, name: str) -> None:
>          self.defn_meta = meta
>          self.defn_name = name
> @@ -73,4 +89,6 @@ def include_path(self) -> str:
>          return ret
>  
>      def __str__(self) -> str:
> +        if not bool(self):
> +            return '[builtin]'
>          return self.include_path() + self.in_defn() + self.loc()

Looks like we can separate the two aspects easily:

       def __str__(self) -> str:
  +        assert not bool(self)
           return self.include_path() + self.in_defn() + self.loc()

Re: [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> --
>
> events.py had an info to route, was it by choice that it wasn't before?

See below.

I figure this is intentionally below the -- line, but ...

> Signed-off-by: John Snow <jsnow@redhat.com>

... this should be above it.

> ---
>  scripts/qapi/events.py |  2 +-
>  scripts/qapi/schema.py | 23 +++++++++++++----------
>  scripts/qapi/types.py  |  9 +++++----
>  scripts/qapi/visit.py  |  6 +++---
>  4 files changed, 22 insertions(+), 18 deletions(-)
>
> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
> index 9851653b9d11..9ba4f109028d 100644
> --- a/scripts/qapi/events.py
> +++ b/scripts/qapi/events.py
> @@ -225,7 +225,7 @@ def visit_event(self,
>                                            self._event_emit_name))
>          # Note: we generate the enum member regardless of @ifcond, to
>          # keep the enumeration usable in target-independent code.
> -        self._event_enum_members.append(QAPISchemaEnumMember(name, None))
> +        self._event_enum_members.append(QAPISchemaEnumMember(name, info))

We pass None because errors should never happen, and None makes that
quite clear.

We don't actually have a built-in QAPISchemaEnumType for the event enum.
We merely generate a C enum QAPIEvent along with macro QAPIEvent_str(),
by passing self._event_emit_name, self._event_enum_members straight to
gen_enum() and gen_enum_lookup().

If we did build a QAPISchemaEnumType, and used it to diagnose clashes,
then clashes would be reported like

    mumble.json: In event 'A-B':
    mumble.json: 2: value 'A-B' collides with value 'A_B'

leaving you guessing what "value" means, and where 'A_B' may be.

Bug: we don't diagnose certain event name clashes.  I'll fix it.

>  
>  
>  def gen_events(schema: QAPISchema,
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 720449feee4d..d5f19732b516 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -23,6 +23,7 @@
>  from .error import QAPIError, QAPISemError
>  from .expr import check_exprs
>  from .parser import QAPISchemaParser
> +from .source import QAPISourceInfo
>  
>  
>  class QAPISchemaEntity:
> @@ -36,10 +37,10 @@ def __init__(self, name, info, doc, ifcond=None, features=None):
>          self.name = name
>          self._module = None
>          # For explicitly defined entities, info points to the (explicit)
> -        # definition.  For builtins (and their arrays), info is None.
> -        # For implicitly defined entities, info points to a place that
> -        # triggered the implicit definition (there may be more than one
> -        # such place).
> +        # definition.  For builtins (and their arrays), info is a null-object
> +        # sentinel that evaluates to False. For implicitly defined entities,
> +        # info points to a place that triggered the implicit definition
> +        # (there may be more than one such place).

s/builtins/built-in types/

The meaning of "a null object sentinel" is less than clear.  Perhaps "a
special object".

>          self.info = info
>          self.doc = doc
>          self._ifcond = ifcond or []
> @@ -209,7 +210,7 @@ class QAPISchemaBuiltinType(QAPISchemaType):
>      meta = 'built-in'
>  
>      def __init__(self, name, json_type, c_type):
> -        super().__init__(name, None, None)
> +        super().__init__(name, QAPISourceInfo.builtin(), None)
>          assert not c_type or isinstance(c_type, str)
>          assert json_type in ('string', 'number', 'int', 'boolean', 'null',
>                               'value')
> @@ -871,7 +872,7 @@ def resolve_type(self, name, info, what):
>          return typ
>  
>      def _module_name(self, fname):
> -        if fname is None:
> +        if not fname:
>              return None
>          return os.path.relpath(fname, self._schema_dir)
>  

Sure this hunk belongs to this patch?

> @@ -897,9 +898,11 @@ def _def_builtin_type(self, name, json_type, c_type):
>          # be nice, but we can't as long as their generated code
>          # (qapi-builtin-types.[ch]) may be shared by some other
>          # schema.
> -        self._make_array_type(name, None)
> +        self._make_array_type(name, QAPISourceInfo.builtin())
>  
>      def _def_predefineds(self):
> +        info = QAPISourceInfo.builtin()
> +

Everything else gets its very own copy of the "no source info" object,
except for the stuff defined here, which gets to share one.  Isn't that
odd?

>          for t in [('str',    'string',  'char' + POINTER_SUFFIX),
>                    ('number', 'number',  'double'),
>                    ('int',    'int',     'int64_t'),
> @@ -917,15 +920,15 @@ def _def_predefineds(self):
>                    ('null',   'null',    'QNull' + POINTER_SUFFIX)]:
>              self._def_builtin_type(*t)
>          self.the_empty_object_type = QAPISchemaObjectType(
> -            'q_empty', None, None, None, None, None, [], None)
> +            'q_empty', info, None, None, None, None, [], None)
>          self._def_entity(self.the_empty_object_type)
>  
>          qtypes = ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist',
>                    'qbool']
>          qtype_values = self._make_enum_members(
> -            [{'name': n} for n in qtypes], None)
> +            [{'name': n} for n in qtypes], info)
>  
> -        self._def_entity(QAPISchemaEnumType('QType', None, None, None, None,
> +        self._def_entity(QAPISchemaEnumType('QType', info, None, None, None,
>                                              qtype_values, 'QTYPE'))
>  
>      def _make_features(self, features, info):
> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> index 2b4916cdaa1b..a3a16284006b 100644
> --- a/scripts/qapi/types.py
> +++ b/scripts/qapi/types.py
> @@ -71,7 +71,8 @@ def gen_enum(name: str,
>               members: List[QAPISchemaEnumMember],
>               prefix: Optional[str] = None) -> str:
>      # append automatically generated _MAX value
> -    enum_members = members + [QAPISchemaEnumMember('_MAX', None)]
> +    enum_members = members + [
> +        QAPISchemaEnumMember('_MAX', QAPISourceInfo.builtin())]
>  
>      ret = mcgen('''
>  
> @@ -306,7 +307,7 @@ def _gen_type_cleanup(self, name: str) -> None:
>  
>      def visit_enum_type(self,
>                          name: str,
> -                        info: Optional[QAPISourceInfo],
> +                        info: QAPISourceInfo,
>                          ifcond: List[str],
>                          features: List[QAPISchemaFeature],
>                          members: List[QAPISchemaEnumMember],
> @@ -317,7 +318,7 @@ def visit_enum_type(self,
>  
>      def visit_array_type(self,
>                           name: str,
> -                         info: Optional[QAPISourceInfo],
> +                         info: QAPISourceInfo,
>                           ifcond: List[str],
>                           element_type: QAPISchemaType) -> None:
>          with ifcontext(ifcond, self._genh, self._genc):
> @@ -327,7 +328,7 @@ def visit_array_type(self,
>  
>      def visit_object_type(self,
>                            name: str,
> -                          info: Optional[QAPISourceInfo],
> +                          info: QAPISourceInfo,
>                            ifcond: List[str],
>                            features: List[QAPISchemaFeature],
>                            base: Optional[QAPISchemaObjectType],
> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> index 339f1521524c..3f49c307c566 100644
> --- a/scripts/qapi/visit.py
> +++ b/scripts/qapi/visit.py
> @@ -336,7 +336,7 @@ def _begin_user_module(self, name: str) -> None:
>  
>      def visit_enum_type(self,
>                          name: str,
> -                        info: QAPISourceInfo,
> +                        info: Optional[QAPISourceInfo],
>                          ifcond: List[str],
>                          features: List[QAPISchemaFeature],
>                          members: List[QAPISchemaEnumMember],
> @@ -347,7 +347,7 @@ def visit_enum_type(self,
>  
>      def visit_array_type(self,
>                           name: str,
> -                         info: Optional[QAPISourceInfo],
> +                         info: QAPISourceInfo,
>                           ifcond: List[str],
>                           element_type: QAPISchemaType) -> None:
>          with ifcontext(ifcond, self._genh, self._genc):
> @@ -356,7 +356,7 @@ def visit_array_type(self,
>  
>      def visit_object_type(self,
>                            name: str,
> -                          info: Optional[QAPISourceInfo],
> +                          info: QAPISourceInfo,
>                            ifcond: List[str],
>                            features: List[QAPISchemaFeature],
>                            base: Optional[QAPISchemaObjectType],

Re: [PATCH 09/12] qapi/gen: move write method to QAPIGenC, make fname a str

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> QAPIGenC and QAPIGenH in particular depend on fname being defined, but
> we have a usage of QAPIGenCCode that isn't intended to be associated
> with a particular file.

Which one?

Hmm, it's this one:

    class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
        def __init__(self, prefix: str):
            super().__init__(
                prefix, 'qapi-commands',
                ' * Schema-defined QAPI/QMP commands', None, __doc__)
--->        self._regy = QAPIGenCCode(None)
            self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]] = {}

Let me try to get rid of it.

> No problem, move the write method down to the class that actually needs
> it, and keep QAPIGenCCode more abstract.
>
> Signed-off-by: John Snow <jsnow@redhat.com>

Re: [PATCH 11/12] qapi/schema: Name the builtin module "" instead of None

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Instead of using None as the built-in module filename, use an empty
> string instead.

PATCH 05's changes the module name of the special system module for
built-in stuff from None to './builtin'.  The other system modules are
named like './FOO': './init' and './emit' currently.

This one changes the module *filename* from None to "", and not just for
the builtin module, but for *all* system modules.  Your commit message
claims only "the built-in module", which is not true as far as I can
tell.

Should we use the opportunity to make the filename match the module
name?

>                 This allows us to clarify the type of various interfaces
> dealing with module names as always taking a string, which saves us from
> having to use Optional[str] everywhere.
>
> Signed-off-by: John Snow <jsnow@redhat.com>

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

[John Snow]

On 12/16/20 3:23 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> Mypy cannot understand that this match can never be None, so help it
>> along.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qapi/main.py | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
>> index 42517210b805..271d9e84da94 100644
>> --- a/scripts/qapi/main.py
>> +++ b/scripts/qapi/main.py
>> @@ -23,7 +23,8 @@
>>   
>>   def invalid_prefix_char(prefix: str) -> Optional[str]:
>>       match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
> 
> @match can't be None because the regexp always matches a prefix,
> possibly the empty prefix.
> 
>> -    if match.end() != len(prefix):
>> +    # match cannot be None, but mypy cannot infer that.
>> +    if match and match.end() != len(prefix):
>>           return prefix[match.end()]
>>       return None
> 
> High-level logic:
> 
>         if there is an invalid prefix character:
>             return it
>         return None
> 
> The actual code takes the return None when the match fails.  If this
> could happen, it would be wrong.  I can't, so it doesn't matter, but I
> dislike it anyway.
> 
> Alternative 1: turn "match cannot be None" from comment to code
> 
>         match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
>    +    assert match
>         if match.end() != len(prefix):
>             return prefix[match.end()]
>         return None
> 
> Alternative 2: turn empty prefix into a special case
> 
>    -    match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
>    +    match = re.match(r'[A-Za-z_.-][A-Za-z0-9_.-]*', prefix)
>    +    if not match:
>    +        return prefix[0]
>         if match.end() != len(prefix):
>             return prefix[match.end()]
>         return None
> 
> I'd prefer alternative 1.
> 

OK, no problem.

Re: [PATCH 04/12] qapi/gen: assert that _start_if is not None in _wrap_ifcond

[John Snow]

On 12/16/20 3:26 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> We already assert this in end_if, but that's opaque to mypy. Do it in
>> _wrap_ifcond instead. Same effect at runtime, but mypy can now infer
>> the type in _wrap_ifcond's body.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qapi/gen.py | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>> index b40f18eee3cd..a6dc991b1d03 100644
>> --- a/scripts/qapi/gen.py
>> +++ b/scripts/qapi/gen.py
>> @@ -130,11 +130,11 @@ def start_if(self, ifcond: List[str]) -> None:
>>           self._start_if = (ifcond, self._body, self._preamble)
>>   
>>       def end_if(self) -> None:
>> -        assert self._start_if
>>           self._wrap_ifcond()
>>           self._start_if = None
>>   
>>       def _wrap_ifcond(self) -> None:
>> +        assert self._start_if
>>           self._body = _wrap_ifcond(self._start_if[0],
>>                                     self._start_if[1], self._body)
>>           self._preamble = _wrap_ifcond(self._start_if[0],
> 
> Drawback: the public method's precondition is now more opaque.  Do we
> care?
> 

Ish. If you call end_if before start_if, what did you want to have happen?

Or more to the point: do you want the assertion in both places?

--js

Re: [PATCH 05/12] qapi/gen: use './builtin' for the built-in module name

[John Snow]

On 12/16/20 3:35 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> Use this in preference to 'None', which helps remove some edge cases in
>> the typing.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> Clearly better.  Should've done it this way in commit c2e196a9b4 "qapi:
> Prepare for system modules other than 'builtin'".
> 
>> ---
>>   scripts/qapi/gen.py | 27 +++++++++++++--------------
>>   1 file changed, 13 insertions(+), 14 deletions(-)
>>
>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>> index a6dc991b1d03..0c5d1fee6088 100644
>> --- a/scripts/qapi/gen.py
>> +++ b/scripts/qapi/gen.py
>> @@ -245,23 +245,23 @@ def __init__(self,
>>           self._pydoc = pydoc
>>           self._genc: Optional[QAPIGenC] = None
>>           self._genh: Optional[QAPIGenH] = None
>> -        self._module: Dict[Optional[str], Tuple[QAPIGenC, QAPIGenH]] = {}
>> +        self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH]] = {}
>>           self._main_module: Optional[str] = None
>>   
>>       @staticmethod
>> -    def _is_user_module(name: Optional[str]) -> bool:
>> -        return bool(name and not name.startswith('./'))
>> +    def _is_user_module(name: str) -> bool:
>> +        return not name.startswith('./')
>>   
>>       @staticmethod
>> -    def _is_builtin_module(name: Optional[str]) -> bool:
>> -        return not name
>> +    def _is_builtin_module(name: str) -> bool:
>> +        return name == './builtin'
>>   
>> -    def _module_dirname(self, name: Optional[str]) -> str:
>> +    def _module_dirname(self, name: str) -> str:
>>           if self._is_user_module(name):
>>               return os.path.dirname(name)
>>           return ''
>>   
>> -    def _module_basename(self, what: str, name: Optional[str]) -> str:
>> +    def _module_basename(self, what: str, name: str) -> str:
>>           ret = '' if self._is_builtin_module(name) else self._prefix
>>           if self._is_user_module(name):
>>               basename = os.path.basename(name)
>> @@ -269,15 +269,14 @@ def _module_basename(self, what: str, name: Optional[str]) -> str:
>>               if name != self._main_module:
>>                   ret += '-' + os.path.splitext(basename)[0]
>>           else:
> 
> Possible drive-by improvement:
> 
>                 assert name.startswith('./')
> 

As long as nobody tells me to split that unrelated change into a new 
patch I'll happily add drive-by improvements upon request ;)

>> -            name = name[2:] if name else 'builtin'
>> -            ret += re.sub(r'-', '-' + name + '-', what)
>> +            ret += re.sub(r'-', '-' + name[2:] + '-', what)
>>           return ret
>>   
>> -    def _module_filename(self, what: str, name: Optional[str]) -> str:
>> +    def _module_filename(self, what: str, name: str) -> str:
>>           return os.path.join(self._module_dirname(name),
>>                               self._module_basename(what, name))
>>   
>> -    def _add_module(self, name: Optional[str], blurb: str) -> None:
>> +    def _add_module(self, name: str, blurb: str) -> None:
>>           basename = self._module_filename(self._what, name)
>>           genc = QAPIGenC(basename + '.c', blurb, self._pydoc)
>>           genh = QAPIGenH(basename + '.h', blurb, self._pydoc)
>> @@ -290,8 +289,8 @@ def _add_user_module(self, name: str, blurb: str) -> None:
>>               self._main_module = name
>>           self._add_module(name, blurb)
>>   
>> -    def _add_system_module(self, name: Optional[str], blurb: str) -> None:
>> -        self._add_module(name and './' + name, blurb)
>> +    def _add_system_module(self, name: str, blurb: str) -> None:
>> +        self._add_module(f"./{name}", blurb)
> 
> I like f-strings, I really do, but is
> 
>      f"./{name}"
> 
> really clearer than
> 
>      './' + name
> 
> ?
> 

I tend to avoid string concatenation as a habit, but there's no greater 
reason; the old value needed updating and that's what fell out of my 
keyboard, I suppose.

>>   
>>       def write(self, output_dir: str, opt_builtins: bool = False) -> None:
>>           for name in self._module:
>> @@ -310,7 +309,7 @@ def _begin_user_module(self, name: str) -> None:
>>       def visit_module(self, name: Optional[str]) -> None:
>>           if name is None:
>>               if self._builtin_blurb:
>> -                self._add_system_module(None, self._builtin_blurb)
>> +                self._add_system_module('builtin', self._builtin_blurb)
>>                   self._begin_system_module(name)
>>               else:
>>                   # The built-in module has not been created.  No code may

Re: [PATCH 06/12] qapi/source: Add builtin null-object sentinel

[John Snow]

On 12/16/20 4:22 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> We use None to represent an object that has no source information
>> because it's a builtin. This complicates interface typing, since many
>> interfaces expect that there is an info object available to print errors
>> with.
>>
>> Introduce a special QAPISourceInfo that represents these built-ins so
>> that if an error should so happen to occur relating to one of these
>> builtins that we will be able to print its information, and interface
>> typing becomes simpler: you will always have a source info object.
> 
> Two aspects mixed up:
> 
> 1. Represent "no source info" as special QAPISourceInfo instead of
>     None
> 
>     This is what de-complicates interface typing.
> 

Yup.

> 2. On error with "no source info", don't crash.
> 
>     I have my doubts on this one.
> 
>     Such an error means the QAPI code generator screwed up, at least in
>     theory.  Crashing is only proper.  It gets the screwup fixed.
> 
>     In practice, errors due to interactions between built-in stuff and
>     user-defined stuff could conceivably escape testing.  I can't
>     remember such a case offhand.
> 
>     Will the "no source info" error be more useful than a crash?
>     Possibly.  Will it get the screwup fixed?  Maybe not.
> 
> Can we separate the two aspects?
> 

We can add an intentional assertion, if you'd like, that makes such 
cases obvious -- but if we are already in the error printer, QAPI is 
likely already in the process of crashing and printing an error.

So, Is this really an issue?

>>
>> This object will evaluate as False, so "if info" is a valid idiomatic
>> construct.
> 
> Suggest s/is a valid/remains a valid/.
> 
> Not 100% sure we'll want to keep this idiom, but now is not the time to
> worry about that.
> 

OK.

>>
>> NB: It was intentional to not allow empty constructors or similar to
>> create "empty" source info objects; callers must explicitly invoke
>> 'builtin()' to pro-actively opt into using the sentinel. This should
>> prevent use-by-accident.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qapi/source.py | 20 +++++++++++++++++++-
>>   1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
>> index d7a79a9b8aee..64af7318cb67 100644
>> --- a/scripts/qapi/source.py
>> +++ b/scripts/qapi/source.py
>> @@ -11,7 +11,12 @@
>>   
>>   import copy
>>   import sys
>> -from typing import List, Optional, TypeVar
>> +from typing import (
>> +    List,
>> +    Optional,
>> +    Type,
>> +    TypeVar,
>> +)
>>   
>>   
>>   class QAPISchemaPragma:
>> @@ -41,6 +46,17 @@ def __init__(self, fname: str, line: int,
>>           self.defn_meta: Optional[str] = None
>>           self.defn_name: Optional[str] = None
>>   
>> +    @classmethod
>> +    def builtin(cls: Type[T]) -> T:
>> +        """
>> +        Create a SourceInfo object corresponding to a builtin definition.
> 
> Let's spell it built-in for consistency with existing comments.
> 
> Could perhaps shorten "a SourceInfo object" to "an instance".
> 

OK.

>> +        """
>> +        return cls('', -1, None)
> 
> No users?  Peeking ahead... aha, they are in Patch 08.  Any particular
> reason for putting PATCH 07 between the two?  Could PATCH 08 be squashed
> into this one?
> 

Too much soup in one pot: this patch highlights the "trick" and the 
subsequent patch shows the adoption of it. Seemed safe.

Goofy ordering, though. I've pushed the genc/genh patch downwards 
instead; you can squash them on commit if you'd like.

>> +
>> +    def __bool__(self) -> bool:
>> +        # "if info: ..." is false if info is the builtin sentinel.
>> +        return bool(self.fname)
> 
> Nitpicking...  "The builtin sentinel" suggests there is just one.  PATCH
> 08 creates several.  I don't mind, but let's say something like "if
> @info corresponds to a built-in definition".
> 

Fair enough. I don't mind nitpicks on comments and docstrings so much if 
it helps make things clearer for more people.

(And they don't cause me rebase pain as much as other nitpicks ;)

>> +
>>       def set_defn(self, meta: str, name: str) -> None:
>>           self.defn_meta = meta
>>           self.defn_name = name
>> @@ -73,4 +89,6 @@ def include_path(self) -> str:
>>           return ret
>>   
>>       def __str__(self) -> str:
>> +        if not bool(self):
>> +            return '[builtin]'
>>           return self.include_path() + self.in_defn() + self.loc()
> 
> Looks like we can separate the two aspects easily:
> 
>         def __str__(self) -> str:
>    +        assert not bool(self)
>             return self.include_path() + self.in_defn() + self.loc()
> 

Feels like abusing __str__ to prevent application logic we don't like 
elsewhere and unrelated to this class; I am still leaning on "If we are 
printing this, it's likely we're already crashing" unless you have news 
to the contrary for me.

Re: [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory

[John Snow]

On 12/16/20 5:18 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> --
>>
>> events.py had an info to route, was it by choice that it wasn't before?
> 
> See below.
> 
> I figure this is intentionally below the -- line, but ...
> 
>> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> ... this should be above it.
> 

Script failure. Or human failure, because I used '--' instead of '---'.

>> ---
>>   scripts/qapi/events.py |  2 +-
>>   scripts/qapi/schema.py | 23 +++++++++++++----------
>>   scripts/qapi/types.py  |  9 +++++----
>>   scripts/qapi/visit.py  |  6 +++---
>>   4 files changed, 22 insertions(+), 18 deletions(-)
>>
>> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
>> index 9851653b9d11..9ba4f109028d 100644
>> --- a/scripts/qapi/events.py
>> +++ b/scripts/qapi/events.py
>> @@ -225,7 +225,7 @@ def visit_event(self,
>>                                             self._event_emit_name))
>>           # Note: we generate the enum member regardless of @ifcond, to
>>           # keep the enumeration usable in target-independent code.
>> -        self._event_enum_members.append(QAPISchemaEnumMember(name, None))
>> +        self._event_enum_members.append(QAPISchemaEnumMember(name, info))
> 
> We pass None because errors should never happen, and None makes that
> quite clear.
> 

Not clear: *why* should errors never happen? This is the only place we 
pass None for SourceInfo that isn't explicitly a literal built-in type. 
This is not obvious.

> We don't actually have a built-in QAPISchemaEnumType for the event enum.
> We merely generate a C enum QAPIEvent along with macro QAPIEvent_str(),
> by passing self._event_emit_name, self._event_enum_members straight to
> gen_enum() and gen_enum_lookup().
> 
> If we did build a QAPISchemaEnumType, and used it to diagnose clashes,
> then clashes would be reported like
> 
>      mumble.json: In event 'A-B':
>      mumble.json: 2: value 'A-B' collides with value 'A_B'
> 
> leaving you guessing what "value" means, and where 'A_B' may be.
> 
> Bug: we don't diagnose certain event name clashes.  I'll fix it.
> 

Not clear to me: If I want interface consistency, what *should* be 
passed downwards as the info? it's not quite a builtin as much as it is 
an inferred enum, so should I just ... leave it like this, or wait for 
you to offer a better fix?

>>   
>>   
>>   def gen_events(schema: QAPISchema,
>> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>> index 720449feee4d..d5f19732b516 100644
>> --- a/scripts/qapi/schema.py
>> +++ b/scripts/qapi/schema.py
>> @@ -23,6 +23,7 @@
>>   from .error import QAPIError, QAPISemError
>>   from .expr import check_exprs
>>   from .parser import QAPISchemaParser
>> +from .source import QAPISourceInfo
>>   
>>   
>>   class QAPISchemaEntity:
>> @@ -36,10 +37,10 @@ def __init__(self, name, info, doc, ifcond=None, features=None):
>>           self.name = name
>>           self._module = None
>>           # For explicitly defined entities, info points to the (explicit)
>> -        # definition.  For builtins (and their arrays), info is None.
>> -        # For implicitly defined entities, info points to a place that
>> -        # triggered the implicit definition (there may be more than one
>> -        # such place).
>> +        # definition.  For builtins (and their arrays), info is a null-object
>> +        # sentinel that evaluates to False. For implicitly defined entities,
>> +        # info points to a place that triggered the implicit definition
>> +        # (there may be more than one such place).
> 
> s/builtins/built-in types/
> 
> The meaning of "a null object sentinel" is less than clear.  Perhaps "a
> special object".
> 

OK.

>>           self.info = info
>>           self.doc = doc
>>           self._ifcond = ifcond or []
>> @@ -209,7 +210,7 @@ class QAPISchemaBuiltinType(QAPISchemaType):
>>       meta = 'built-in'
>>   
>>       def __init__(self, name, json_type, c_type):
>> -        super().__init__(name, None, None)
>> +        super().__init__(name, QAPISourceInfo.builtin(), None)
>>           assert not c_type or isinstance(c_type, str)
>>           assert json_type in ('string', 'number', 'int', 'boolean', 'null',
>>                                'value')
>> @@ -871,7 +872,7 @@ def resolve_type(self, name, info, what):
>>           return typ
>>   
>>       def _module_name(self, fname):
>> -        if fname is None:
>> +        if not fname:
>>               return None
>>           return os.path.relpath(fname, self._schema_dir)
>>   
> 
> Sure this hunk belongs to this patch?
> 

Accident.

"info and info.fname" does not behave the same with a falsey info object 
as it does when info was genuinely None; I compensated for that *here*, 
but I should have compensated for it elsewhere.

>> @@ -897,9 +898,11 @@ def _def_builtin_type(self, name, json_type, c_type):
>>           # be nice, but we can't as long as their generated code
>>           # (qapi-builtin-types.[ch]) may be shared by some other
>>           # schema.
>> -        self._make_array_type(name, None)
>> +        self._make_array_type(name, QAPISourceInfo.builtin())
>>   
>>       def _def_predefineds(self):
>> +        info = QAPISourceInfo.builtin()
>> +
> 
> Everything else gets its very own copy of the "no source info" object,
> except for the stuff defined here, which gets to share one.  Isn't that
> odd?
> 

It's also the only function where we define so many built-ins in the 
same place, so spiritually they do have the same SourceInfo, right? :)

(OK, no, it wasn't a conscious design choice, but it also seems 
harmless. I am going to assume you'd prefer I not do this?)

>>           for t in [('str',    'string',  'char' + POINTER_SUFFIX),
>>                     ('number', 'number',  'double'),
>>                     ('int',    'int',     'int64_t'),
>> @@ -917,15 +920,15 @@ def _def_predefineds(self):
>>                     ('null',   'null',    'QNull' + POINTER_SUFFIX)]:
>>               self._def_builtin_type(*t)
>>           self.the_empty_object_type = QAPISchemaObjectType(
>> -            'q_empty', None, None, None, None, None, [], None)
>> +            'q_empty', info, None, None, None, None, [], None)
>>           self._def_entity(self.the_empty_object_type)
>>   
>>           qtypes = ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist',
>>                     'qbool']
>>           qtype_values = self._make_enum_members(
>> -            [{'name': n} for n in qtypes], None)
>> +            [{'name': n} for n in qtypes], info)
>>   
>> -        self._def_entity(QAPISchemaEnumType('QType', None, None, None, None,
>> +        self._def_entity(QAPISchemaEnumType('QType', info, None, None, None,
>>                                               qtype_values, 'QTYPE'))
>>   
>>       def _make_features(self, features, info):
>> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
>> index 2b4916cdaa1b..a3a16284006b 100644
>> --- a/scripts/qapi/types.py
>> +++ b/scripts/qapi/types.py
>> @@ -71,7 +71,8 @@ def gen_enum(name: str,
>>                members: List[QAPISchemaEnumMember],
>>                prefix: Optional[str] = None) -> str:
>>       # append automatically generated _MAX value
>> -    enum_members = members + [QAPISchemaEnumMember('_MAX', None)]
>> +    enum_members = members + [
>> +        QAPISchemaEnumMember('_MAX', QAPISourceInfo.builtin())]
>>   
>>       ret = mcgen('''
>>   
>> @@ -306,7 +307,7 @@ def _gen_type_cleanup(self, name: str) -> None:
>>   
>>       def visit_enum_type(self,
>>                           name: str,
>> -                        info: Optional[QAPISourceInfo],
>> +                        info: QAPISourceInfo,
>>                           ifcond: List[str],
>>                           features: List[QAPISchemaFeature],
>>                           members: List[QAPISchemaEnumMember],
>> @@ -317,7 +318,7 @@ def visit_enum_type(self,
>>   
>>       def visit_array_type(self,
>>                            name: str,
>> -                         info: Optional[QAPISourceInfo],
>> +                         info: QAPISourceInfo,
>>                            ifcond: List[str],
>>                            element_type: QAPISchemaType) -> None:
>>           with ifcontext(ifcond, self._genh, self._genc):
>> @@ -327,7 +328,7 @@ def visit_array_type(self,
>>   
>>       def visit_object_type(self,
>>                             name: str,
>> -                          info: Optional[QAPISourceInfo],
>> +                          info: QAPISourceInfo,
>>                             ifcond: List[str],
>>                             features: List[QAPISchemaFeature],
>>                             base: Optional[QAPISchemaObjectType],
>> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
>> index 339f1521524c..3f49c307c566 100644
>> --- a/scripts/qapi/visit.py
>> +++ b/scripts/qapi/visit.py
>> @@ -336,7 +336,7 @@ def _begin_user_module(self, name: str) -> None:
>>   
>>       def visit_enum_type(self,
>>                           name: str,
>> -                        info: QAPISourceInfo,
>> +                        info: Optional[QAPISourceInfo],
>>                           ifcond: List[str],
>>                           features: List[QAPISchemaFeature],
>>                           members: List[QAPISchemaEnumMember],
>> @@ -347,7 +347,7 @@ def visit_enum_type(self,
>>   
>>       def visit_array_type(self,
>>                            name: str,
>> -                         info: Optional[QAPISourceInfo],
>> +                         info: QAPISourceInfo,
>>                            ifcond: List[str],
>>                            element_type: QAPISchemaType) -> None:
>>           with ifcontext(ifcond, self._genh, self._genc):
>> @@ -356,7 +356,7 @@ def visit_array_type(self,
>>   
>>       def visit_object_type(self,
>>                             name: str,
>> -                          info: Optional[QAPISourceInfo],
>> +                          info: QAPISourceInfo,
>>                             ifcond: List[str],
>>                             features: List[QAPISchemaFeature],
>>                             base: Optional[QAPISchemaObjectType],

Re: [PATCH 11/12] qapi/schema: Name the builtin module "" instead of None

[John Snow]

On 12/16/20 5:42 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> Instead of using None as the built-in module filename, use an empty
>> string instead.
> 
> PATCH 05's changes the module name of the special system module for
> built-in stuff from None to './builtin'.  The other system modules are
> named like './FOO': './init' and './emit' currently.
> 
> This one changes the module *filename* from None to "", and not just for
> the builtin module, but for *all* system modules.  Your commit message
> claims only "the built-in module", which is not true as far as I can
> tell.
> 

Is this true? ... "./init" and "./emit" are defined only in the 
generators, outside of the schema entirely. They don't even have 
"QAPISchemaModule" objects associated with them.

I changed:

         self._make_module(None)  # built-ins 


to

         self._make_module(QAPISourceInfo.builtin().fname)  # built-ins 



that should be precisely only "the" built-in module, shouldn't it? the 
other "system" modules don't even pass through _make_module.

> Should we use the opportunity to make the filename match the module
> name?
> 

If that's something you want to have happen, it can be done, yes.

I had a draft that did it that way initially; I was afraid I was mixing 
up two distinct things: the module fname (schema.py's concept of 
modules) and module name (gen.py's concept of modules). This version of 
my patch kept the two more distinct like they are currently.

We can change "the" built-in module to have an fname of "./builtin" 
which works just fine; gen.py just has to change to not add "./" to 
modules already declared with the leading slash.

Up for debate is if you want the system modules declared in the code 
generators to have to declare 'emit' or './emit'; I left them alone and 
allowed them to say 'event'.

Downside: the ./ prefix takes on special meaning in both gen.py and 
schema.py. the module organization feels decentralized and fragile.

>>                  This allows us to clarify the type of various interfaces
>> dealing with module names as always taking a string, which saves us from
>> having to use Optional[str] everywhere.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>

Re: [PATCH 06/12] qapi/source: Add builtin null-object sentinel

[John Snow]

On 12/16/20 4:22 AM, Markus Armbruster wrote:
> 2. On error with "no source info", don't crash.
> 
>     I have my doubts on this one.
> 
>     Such an error means the QAPI code generator screwed up, at least in
>     theory.  Crashing is only proper.  It gets the screwup fixed.
> 

QAPISemError and friends will also halt the generator and don't produce 
output and will fail tests. They aren't less visible or more ignorable 
somehow.

>     In practice, errors due to interactions between built-in stuff and
>     user-defined stuff could conceivably escape testing.  I can't
>     remember such a case offhand.
> 
>     Will the "no source info" error be more useful than a crash?
>     Possibly.  Will it get the screwup fixed?  Maybe not.

I don't understand this; if it's an error -- there's no QAPI, there's no 
QEMU. It's definitely getting fixed.

If QAPISourceInfo is primarily used for printing error information, we 
are already in a situation where the generator is hosed and has wandered 
itself into a problem that can't be ignored.

There's no additional value in having python crash twice per every crash 
because we have bad types in our error reporting functions.

--js

Re: [PATCH 04/12] qapi/gen: assert that _start_if is not None in _wrap_ifcond

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 12/16/20 3:26 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> We already assert this in end_if, but that's opaque to mypy. Do it in
>>> _wrap_ifcond instead. Same effect at runtime, but mypy can now infer
>>> the type in _wrap_ifcond's body.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   scripts/qapi/gen.py | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>>> index b40f18eee3cd..a6dc991b1d03 100644
>>> --- a/scripts/qapi/gen.py
>>> +++ b/scripts/qapi/gen.py
>>> @@ -130,11 +130,11 @@ def start_if(self, ifcond: List[str]) -> None:
>>>           self._start_if = (ifcond, self._body, self._preamble)
>>>   
>>>       def end_if(self) -> None:
>>> -        assert self._start_if
>>>           self._wrap_ifcond()
>>>           self._start_if = None
>>>   
>>>       def _wrap_ifcond(self) -> None:
>>> +        assert self._start_if
>>>           self._body = _wrap_ifcond(self._start_if[0],
>>>                                     self._start_if[1], self._body)
>>>           self._preamble = _wrap_ifcond(self._start_if[0],
>> 
>> Drawback: the public method's precondition is now more opaque.  Do we
>> care?
>> 
>
> Ish. If you call end_if before start_if, what did you want to have happen?

Point.

> Or more to the point: do you want the assertion in both places?

What about inlining QAPIGenCCode._wrap_ifcond() into .end_if()?

Re: [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 12/16/20 5:18 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> --
>>>
>>> events.py had an info to route, was it by choice that it wasn't before?
>> 
>> See below.
>> 
>> I figure this is intentionally below the -- line, but ...
>> 
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>> 
>> ... this should be above it.
>> 
>
> Script failure. Or human failure, because I used '--' instead of '---'.
>
>>> ---
>>>   scripts/qapi/events.py |  2 +-
>>>   scripts/qapi/schema.py | 23 +++++++++++++----------
>>>   scripts/qapi/types.py  |  9 +++++----
>>>   scripts/qapi/visit.py  |  6 +++---
>>>   4 files changed, 22 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
>>> index 9851653b9d11..9ba4f109028d 100644
>>> --- a/scripts/qapi/events.py
>>> +++ b/scripts/qapi/events.py
>>> @@ -225,7 +225,7 @@ def visit_event(self,
>>>                                             self._event_emit_name))
>>>           # Note: we generate the enum member regardless of @ifcond, to
>>>           # keep the enumeration usable in target-independent code.
>>> -        self._event_enum_members.append(QAPISchemaEnumMember(name, None))
>>> +        self._event_enum_members.append(QAPISchemaEnumMember(name, info))
>> 
>> We pass None because errors should never happen, and None makes that
>> quite clear.
>> 
>
> Not clear: *why* should errors never happen? This is the only place we 
> pass None for SourceInfo that isn't explicitly a literal built-in type. 
> This is not obvious.

You're right, but there are two separate "unclarities".

Passing None effectively asserts "errors never happen".  We neglect to
explain why, leaving the reader guessing.

Passing @info "fixes" that by removing the assertion.  Now we have a
more subtle problem: will errors make sense with this @info?  Normally,
all members of an enum share one info.  Only this enum's members don't.
It turns out the question is moot because @info is not actually used.
But will it remain moot?  My point isn't that these are important
questions to answer.  It is that we're replacing the relatively obvious
question "why are we asserting errors can't happen" by more subtle ones.
Feels like sweeping dirt under the rug.

>> We don't actually have a built-in QAPISchemaEnumType for the event enum.
>> We merely generate a C enum QAPIEvent along with macro QAPIEvent_str(),
>> by passing self._event_emit_name, self._event_enum_members straight to
>> gen_enum() and gen_enum_lookup().
>> 
>> If we did build a QAPISchemaEnumType, and used it to diagnose clashes,
>> then clashes would be reported like
>> 
>>      mumble.json: In event 'A-B':
>>      mumble.json: 2: value 'A-B' collides with value 'A_B'
>> 
>> leaving you guessing what "value" means, and where 'A_B' may be.
>> 
>> Bug: we don't diagnose certain event name clashes.  I'll fix it.
>> 
>
> Not clear to me: If I want interface consistency, what *should* be 
> passed downwards as the info? it's not quite a builtin as much as it is 
> an inferred enum,

True.

>                   so should I just ... leave it like this, or wait for 
> you to offer a better fix?

Waiting for some better fix feels undavisable.  We want to get type
checking in place sooner rather than later.

Aside: a possible fix is decoupling gen_enum_lookup() and gen_enum()
from QAPISchemaEnumMember, so we don't have to make up
QAPISchemaEnumMembers here.

I think there are two interpretations of your QAPISourceInfo work's aim:

1. Narrow: use a special QAPISourceInfo rather then None as "no errors
   shall happen" poison.

   QAPISourceInfo.builtin() returns this poison.  The name "builtin" is
   less than ideal.

2. Ambitious: eliminate "no errors shall happen".

We're discussing this in reply of PATCH 06.  I think we need to reach a
conclusion there before we can decide here.

>>>   
>>>   
>>>   def gen_events(schema: QAPISchema,
>>> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>>> index 720449feee4d..d5f19732b516 100644
>>> --- a/scripts/qapi/schema.py
>>> +++ b/scripts/qapi/schema.py
>>> @@ -23,6 +23,7 @@
>>>   from .error import QAPIError, QAPISemError
>>>   from .expr import check_exprs
>>>   from .parser import QAPISchemaParser
>>> +from .source import QAPISourceInfo
>>>   
>>>   
>>>   class QAPISchemaEntity:
>>> @@ -36,10 +37,10 @@ def __init__(self, name, info, doc, ifcond=None, features=None):
>>>           self.name = name
>>>           self._module = None
>>>           # For explicitly defined entities, info points to the (explicit)
>>> -        # definition.  For builtins (and their arrays), info is None.
>>> -        # For implicitly defined entities, info points to a place that
>>> -        # triggered the implicit definition (there may be more than one
>>> -        # such place).
>>> +        # definition.  For builtins (and their arrays), info is a null-object
>>> +        # sentinel that evaluates to False. For implicitly defined entities,
>>> +        # info points to a place that triggered the implicit definition
>>> +        # (there may be more than one such place).
>> 
>> s/builtins/built-in types/
>> 
>> The meaning of "a null object sentinel" is less than clear.  Perhaps "a
>> special object".
>> 
>
> OK.
>
>>>           self.info = info
>>>           self.doc = doc
>>>           self._ifcond = ifcond or []
>>> @@ -209,7 +210,7 @@ class QAPISchemaBuiltinType(QAPISchemaType):
>>>       meta = 'built-in'
>>>   
>>>       def __init__(self, name, json_type, c_type):
>>> -        super().__init__(name, None, None)
>>> +        super().__init__(name, QAPISourceInfo.builtin(), None)
>>>           assert not c_type or isinstance(c_type, str)
>>>           assert json_type in ('string', 'number', 'int', 'boolean', 'null',
>>>                                'value')
>>> @@ -871,7 +872,7 @@ def resolve_type(self, name, info, what):
>>>           return typ
>>>   
>>>       def _module_name(self, fname):
>>> -        if fname is None:
>>> +        if not fname:
>>>               return None
>>>           return os.path.relpath(fname, self._schema_dir)
>>>   
>> 
>> Sure this hunk belongs to this patch?
>> 
>
> Accident.
>
> "info and info.fname" does not behave the same with a falsey info object 
> as it does when info was genuinely None; I compensated for that *here*, 
> but I should have compensated for it elsewhere.
>
>>> @@ -897,9 +898,11 @@ def _def_builtin_type(self, name, json_type, c_type):
>>>           # be nice, but we can't as long as their generated code
>>>           # (qapi-builtin-types.[ch]) may be shared by some other
>>>           # schema.
>>> -        self._make_array_type(name, None)
>>> +        self._make_array_type(name, QAPISourceInfo.builtin())
>>>   
>>>       def _def_predefineds(self):
>>> +        info = QAPISourceInfo.builtin()
>>> +
>> 
>> Everything else gets its very own copy of the "no source info" object,
>> except for the stuff defined here, which gets to share one.  Isn't that
>> odd?
>> 
>
> It's also the only function where we define so many built-ins in the 
> same place, so spiritually they do have the same SourceInfo, right? :)
>
> (OK, no, it wasn't a conscious design choice, but it also seems 
> harmless. I am going to assume you'd prefer I not do this?)

It is harmless.  It just made me wonder why we create more than one such
QAPISourceInfo in the first place.  Method builtin() could return the
same one every time.  It could even be an attribute instead of a method.
Anyway, no big deal.

>>>           for t in [('str',    'string',  'char' + POINTER_SUFFIX),
>>>                     ('number', 'number',  'double'),
>>>                     ('int',    'int',     'int64_t'),
>>> @@ -917,15 +920,15 @@ def _def_predefineds(self):
>>>                     ('null',   'null',    'QNull' + POINTER_SUFFIX)]:
>>>               self._def_builtin_type(*t)
>>>           self.the_empty_object_type = QAPISchemaObjectType(
>>> -            'q_empty', None, None, None, None, None, [], None)
>>> +            'q_empty', info, None, None, None, None, [], None)
>>>           self._def_entity(self.the_empty_object_type)
>>>   
>>>           qtypes = ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist',
>>>                     'qbool']
>>>           qtype_values = self._make_enum_members(
>>> -            [{'name': n} for n in qtypes], None)
>>> +            [{'name': n} for n in qtypes], info)
>>>   
>>> -        self._def_entity(QAPISchemaEnumType('QType', None, None, None, None,
>>> +        self._def_entity(QAPISchemaEnumType('QType', info, None, None, None,
>>>                                               qtype_values, 'QTYPE'))
>>>   
>>>       def _make_features(self, features, info):
>>> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
>>> index 2b4916cdaa1b..a3a16284006b 100644
>>> --- a/scripts/qapi/types.py
>>> +++ b/scripts/qapi/types.py
>>> @@ -71,7 +71,8 @@ def gen_enum(name: str,
>>>                members: List[QAPISchemaEnumMember],
>>>                prefix: Optional[str] = None) -> str:
>>>       # append automatically generated _MAX value
>>> -    enum_members = members + [QAPISchemaEnumMember('_MAX', None)]
>>> +    enum_members = members + [
>>> +        QAPISchemaEnumMember('_MAX', QAPISourceInfo.builtin())]
>>>   
>>>       ret = mcgen('''
>>>   
>>> @@ -306,7 +307,7 @@ def _gen_type_cleanup(self, name: str) -> None:
>>>   
>>>       def visit_enum_type(self,
>>>                           name: str,
>>> -                        info: Optional[QAPISourceInfo],
>>> +                        info: QAPISourceInfo,
>>>                           ifcond: List[str],
>>>                           features: List[QAPISchemaFeature],
>>>                           members: List[QAPISchemaEnumMember],
>>> @@ -317,7 +318,7 @@ def visit_enum_type(self,
>>>   
>>>       def visit_array_type(self,
>>>                            name: str,
>>> -                         info: Optional[QAPISourceInfo],
>>> +                         info: QAPISourceInfo,
>>>                            ifcond: List[str],
>>>                            element_type: QAPISchemaType) -> None:
>>>           with ifcontext(ifcond, self._genh, self._genc):
>>> @@ -327,7 +328,7 @@ def visit_array_type(self,
>>>   
>>>       def visit_object_type(self,
>>>                             name: str,
>>> -                          info: Optional[QAPISourceInfo],
>>> +                          info: QAPISourceInfo,
>>>                             ifcond: List[str],
>>>                             features: List[QAPISchemaFeature],
>>>                             base: Optional[QAPISchemaObjectType],
>>> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
>>> index 339f1521524c..3f49c307c566 100644
>>> --- a/scripts/qapi/visit.py
>>> +++ b/scripts/qapi/visit.py
>>> @@ -336,7 +336,7 @@ def _begin_user_module(self, name: str) -> None:
>>>   
>>>       def visit_enum_type(self,
>>>                           name: str,
>>> -                        info: QAPISourceInfo,
>>> +                        info: Optional[QAPISourceInfo],
>>>                           ifcond: List[str],
>>>                           features: List[QAPISchemaFeature],
>>>                           members: List[QAPISchemaEnumMember],
>>> @@ -347,7 +347,7 @@ def visit_enum_type(self,
>>>   
>>>       def visit_array_type(self,
>>>                            name: str,
>>> -                         info: Optional[QAPISourceInfo],
>>> +                         info: QAPISourceInfo,
>>>                            ifcond: List[str],
>>>                            element_type: QAPISchemaType) -> None:
>>>           with ifcontext(ifcond, self._genh, self._genc):
>>> @@ -356,7 +356,7 @@ def visit_array_type(self,
>>>   
>>>       def visit_object_type(self,
>>>                             name: str,
>>> -                          info: Optional[QAPISourceInfo],
>>> +                          info: QAPISourceInfo,
>>>                             ifcond: List[str],
>>>                             features: List[QAPISchemaFeature],
>>>                             base: Optional[QAPISchemaObjectType],

Re: [PATCH 11/12] qapi/schema: Name the builtin module "" instead of None

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 12/16/20 5:42 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> Instead of using None as the built-in module filename, use an empty
>>> string instead.
>> 
>> PATCH 05's changes the module name of the special system module for
>> built-in stuff from None to './builtin'.  The other system modules are
>> named like './FOO': './init' and './emit' currently.
>> 
>> This one changes the module *filename* from None to "", and not just for
>> the builtin module, but for *all* system modules.  Your commit message
>> claims only "the built-in module", which is not true as far as I can
>> tell.
>> 
>
> Is this true? ... "./init" and "./emit" are defined only in the 
> generators, outside of the schema entirely. They don't even have 
> "QAPISchemaModule" objects associated with them.
>
> I changed:
>
>          self._make_module(None)  # built-ins 
>
>
> to
>
>          self._make_module(QAPISourceInfo.builtin().fname)  # built-ins 
>
>
>
> that should be precisely only "the" built-in module, shouldn't it? the 
> other "system" modules don't even pass through _make_module.

You're right.

The schema IR has only user-defined modules and the built-ins module.
Each module has a name.  We use the source file name for the
user-defined modules.  The built-ins module has none, so we use None.

The QAPISchemaModularCVisitor generates "modular" output.  It has
per-module data, keyed by module name.  It supports user-defined and
system modules.  We set them up as follows.  The user-defined modules
are exactly the schema IR's (same name).  The system modules are the
schema IR's built-ins module (same name) plus whatever the user of
QAPISchemaModularCVisitor adds (convention: name starts with ./, so it
can't clash).

Why let generators add system modules that don't exist in the schema IR?
It's a reasonably clean way to generate stuff that doesn't fit elsewhere
into separate .[ch] files.

PATCH 05 changes the name of the built-ins module from None to
'./builtins' in the QAPISchemaModularCVisitor only.  Correct?

This patch changes the name of the built-ins module from None to "" in
the schema IR only.  Correct?

>> Should we use the opportunity to make the filename match the module
>> name?
>> 
>
> If that's something you want to have happen, it can be done, yes.

Having different "module names" for the built-ins module in different
places could be confusing.

The issue appears in PATCH 05.  I'm fine with the two module names
diverging temporarily in this series.  I'd prefer them to be the same at
the end.

> I had a draft that did it that way initially; I was afraid I was mixing 
> up two distinct things: the module fname (schema.py's concept of 
> modules) and module name (gen.py's concept of modules). This version of 
> my patch kept the two more distinct like they are currently.
>
> We can change "the" built-in module to have an fname of "./builtin" 
> which works just fine; gen.py just has to change to not add "./" to 
> modules already declared with the leading slash.
>
> Up for debate is if you want the system modules declared in the code 
> generators to have to declare 'emit' or './emit'; I left them alone and 
> allowed them to say 'event'.

I pass 'emit' to argument name, so a simple './' + name obviously
results in a system module name.

With './emit', we achieve "obviously" by assert name.startswith('./').
Matter of taste.

> Downside: the ./ prefix takes on special meaning in both gen.py and 
> schema.py. the module organization feels decentralized and fragile.

Making the './' prefix special is fine.  But you're right, doing it in
two places and with precious little explanation is not so fine.

We could have schema.py provide some notion of "module name".  Weasel
words "some notion" for artistic license.

If we'd prefer not to do it now, a few judicious comments should suffice
for now.

>>>                  This allows us to clarify the type of various interfaces
>>> dealing with module names as always taking a string, which saves us from
>>> having to use Optional[str] everywhere.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>

Re: [PATCH 06/12] qapi/source: Add builtin null-object sentinel

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 12/16/20 4:22 AM, Markus Armbruster wrote:
>> 2. On error with "no source info", don't crash.
>>     I have my doubts on this one.
>>     Such an error means the QAPI code generator screwed up, at least
>> in
>>     theory.  Crashing is only proper.  It gets the screwup fixed.
>> 
>
> QAPISemError and friends will also halt the generator and don't
> produce output and will fail tests. They aren't less visible or more
> ignorable somehow.
>
>>     In practice, errors due to interactions between built-in stuff and
>>     user-defined stuff could conceivably escape testing.  I can't
>>     remember such a case offhand.
>>     Will the "no source info" error be more useful than a crash?
>>     Possibly.  Will it get the screwup fixed?  Maybe not.
>
> I don't understand this; if it's an error -- there's no QAPI, there's
> no QEMU. It's definitely getting fixed.
>
> If QAPISourceInfo is primarily used for printing error information, we
> are already in a situation where the generator is hosed and has
> wandered itself into a problem that can't be ignored.
>
> There's no additional value in having python crash twice per every
> crash because we have bad types in our error reporting functions.

Consider the following scenario.  The average developer knows just
enough about QAPI to be dangerous.  That's okay; if you had to be a QAPI
expert to modify the QAPI schema, we would have failed.  Now meet Joe
Average.  He's a good guy.  Today his job happens to require extending
the QAPI schema.  In a hurry, as usual.  So Joe brings his well-honed
voodoo programming skills to bear, and writes something that looks
plausible to him.  His build fails.  He's not surprised; he's voodoo-
programming after all.  However, the error message is less clear than
usual.  Something about a '[builtin]' file.  There is no '[builtin]'
file.  What to do?  Obvious!  If a bit of voodoo doesn't get you over
the finish line, use more: twiddle the schema until it works.

If his build failed with a Python backtrace instead, Joe would
immediately know that he ran into a bug in our tooling he should report.

Again, I don't mean to criticize Joe.  I've walked in his shoes myself.

Re: [PATCH 06/12] qapi/source: Add builtin null-object sentinel

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 12/16/20 4:22 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> We use None to represent an object that has no source information
>>> because it's a builtin. This complicates interface typing, since many
>>> interfaces expect that there is an info object available to print errors
>>> with.
>>>
>>> Introduce a special QAPISourceInfo that represents these built-ins so
>>> that if an error should so happen to occur relating to one of these
>>> builtins that we will be able to print its information, and interface
>>> typing becomes simpler: you will always have a source info object.
>> 
>> Two aspects mixed up:
>> 
>> 1. Represent "no source info" as special QAPISourceInfo instead of
>>     None
>> 
>>     This is what de-complicates interface typing.
>> 
>
> Yup.
>
>> 2. On error with "no source info", don't crash.
>> 
>>     I have my doubts on this one.
>> 
>>     Such an error means the QAPI code generator screwed up, at least in
>>     theory.  Crashing is only proper.  It gets the screwup fixed.
>> 
>>     In practice, errors due to interactions between built-in stuff and
>>     user-defined stuff could conceivably escape testing.  I can't
>>     remember such a case offhand.
>> 
>>     Will the "no source info" error be more useful than a crash?
>>     Possibly.  Will it get the screwup fixed?  Maybe not.
>> 
>> Can we separate the two aspects?
>> 
>
> We can add an intentional assertion, if you'd like, that makes such 
> cases obvious -- but if we are already in the error printer, QAPI is 
> likely already in the process of crashing and printing an error.
>
> So, Is this really an issue?

A patch limited to the first aspect merely tweaks an implementation
detail.

As soon as we include the second aspect, we get to debate how to handle
programming errors, and maybe whether any of the errors involving a
QAPISourceInfo.builtin() are *not* programming errors.

I'd prefer this series to remain focused on "enabling strict optional
checking in mypy for everything we have typed so far".

>>>
>>> This object will evaluate as False, so "if info" is a valid idiomatic
>>> construct.
>> 
>> Suggest s/is a valid/remains a valid/.
>> 
>> Not 100% sure we'll want to keep this idiom, but now is not the time to
>> worry about that.
>> 
>
> OK.
>
>>>
>>> NB: It was intentional to not allow empty constructors or similar to
>>> create "empty" source info objects; callers must explicitly invoke
>>> 'builtin()' to pro-actively opt into using the sentinel. This should
>>> prevent use-by-accident.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   scripts/qapi/source.py | 20 +++++++++++++++++++-
>>>   1 file changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
>>> index d7a79a9b8aee..64af7318cb67 100644
>>> --- a/scripts/qapi/source.py
>>> +++ b/scripts/qapi/source.py
>>> @@ -11,7 +11,12 @@
>>>   
>>>   import copy
>>>   import sys
>>> -from typing import List, Optional, TypeVar
>>> +from typing import (
>>> +    List,
>>> +    Optional,
>>> +    Type,
>>> +    TypeVar,
>>> +)
>>>   
>>>   
>>>   class QAPISchemaPragma:
>>> @@ -41,6 +46,17 @@ def __init__(self, fname: str, line: int,
>>>           self.defn_meta: Optional[str] = None
>>>           self.defn_name: Optional[str] = None
>>>   
>>> +    @classmethod
>>> +    def builtin(cls: Type[T]) -> T:
>>> +        """
>>> +        Create a SourceInfo object corresponding to a builtin definition.
>> 
>> Let's spell it built-in for consistency with existing comments.
>> 
>> Could perhaps shorten "a SourceInfo object" to "an instance".
>> 
>
> OK.
>
>>> +        """
>>> +        return cls('', -1, None)
>> 
>> No users?  Peeking ahead... aha, they are in Patch 08.  Any particular
>> reason for putting PATCH 07 between the two?  Could PATCH 08 be squashed
>> into this one?
>> 
>
> Too much soup in one pot: this patch highlights the "trick" and the 
> subsequent patch shows the adoption of it. Seemed safe.
>
> Goofy ordering, though. I've pushed the genc/genh patch downwards 
> instead; you can squash them on commit if you'd like.
>
>>> +
>>> +    def __bool__(self) -> bool:
>>> +        # "if info: ..." is false if info is the builtin sentinel.
>>> +        return bool(self.fname)
>> 
>> Nitpicking...  "The builtin sentinel" suggests there is just one.  PATCH
>> 08 creates several.  I don't mind, but let's say something like "if
>> @info corresponds to a built-in definition".
>> 
>
> Fair enough. I don't mind nitpicks on comments and docstrings so much if 
> it helps make things clearer for more people.
>
> (And they don't cause me rebase pain as much as other nitpicks ;)
>
>>> +
>>>       def set_defn(self, meta: str, name: str) -> None:
>>>           self.defn_meta = meta
>>>           self.defn_name = name
>>> @@ -73,4 +89,6 @@ def include_path(self) -> str:
>>>           return ret
>>>   
>>>       def __str__(self) -> str:
>>> +        if not bool(self):
>>> +            return '[builtin]'
>>>           return self.include_path() + self.in_defn() + self.loc()
>> 
>> Looks like we can separate the two aspects easily:
>> 
>>         def __str__(self) -> str:
>>    +        assert not bool(self)
>>             return self.include_path() + self.in_defn() + self.loc()
>> 
>
> Feels like abusing __str__ to prevent application logic we don't like 
> elsewhere and unrelated to this class; I am still leaning on "If we are 
> printing this, it's likely we're already crashing" unless you have news 
> to the contrary for me.

You're right, making __str__() fail is not nice.  It has other uses,
e.g. when messing around interactively.

Ways out:

1. Avoid abuse of __str__() by naming the thing differently.

2. Lift the assertion into the relevant caller(s).  Unfortunately, the
   relevant caller is another __str__(): QAPIError's.  Next level up:
   the except suite in main() and also test-qapi.py's test_and_diff().
   Keeping the stack backtrace useful might require care.

3. Find a simpler way to signal "oops, programming error".

   For a simple batch program like this one, crashing is a perfectly
   fine reaction to programming errors.  Most of the time, it's also the
   simplest one.  Simple is *good*.  *Especially* when it's something
   that should never happen.

   If reporting an error is genuinely simpler than crashing: simple is
   good.  But the error message should clearly express "this is a bug,
   please report it", like a crash does.

   Drawbacks: we'd have to relax our rule "no error without a test
   case", and we'd lose the stack backtrace.

Re: [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory

[John Snow]

On 12/17/20 3:02 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 12/16/20 5:18 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> --
>>>>
>>>> events.py had an info to route, was it by choice that it wasn't before?
>>>
>>> See below.
>>>
>>> I figure this is intentionally below the -- line, but ...
>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>
>>> ... this should be above it.
>>>
>>
>> Script failure. Or human failure, because I used '--' instead of '---'.
>>
>>>> ---
>>>>    scripts/qapi/events.py |  2 +-
>>>>    scripts/qapi/schema.py | 23 +++++++++++++----------
>>>>    scripts/qapi/types.py  |  9 +++++----
>>>>    scripts/qapi/visit.py  |  6 +++---
>>>>    4 files changed, 22 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
>>>> index 9851653b9d11..9ba4f109028d 100644
>>>> --- a/scripts/qapi/events.py
>>>> +++ b/scripts/qapi/events.py
>>>> @@ -225,7 +225,7 @@ def visit_event(self,
>>>>                                              self._event_emit_name))
>>>>            # Note: we generate the enum member regardless of @ifcond, to
>>>>            # keep the enumeration usable in target-independent code.
>>>> -        self._event_enum_members.append(QAPISchemaEnumMember(name, None))
>>>> +        self._event_enum_members.append(QAPISchemaEnumMember(name, info))
>>>
>>> We pass None because errors should never happen, and None makes that
>>> quite clear.
>>>
>>
>> Not clear: *why* should errors never happen? This is the only place we
>> pass None for SourceInfo that isn't explicitly a literal built-in type.
>> This is not obvious.
> 
> You're right, but there are two separate "unclarities".
> 
> Passing None effectively asserts "errors never happen".  We neglect to
> explain why, leaving the reader guessing.
> 
> Passing @info "fixes" that by removing the assertion.  Now we have a
> more subtle problem: will errors make sense with this @info?  Normally,
> all members of an enum share one info.  Only this enum's members don't.
> It turns out the question is moot because @info is not actually used.
> But will it remain moot?  My point isn't that these are important
> questions to answer.  It is that we're replacing the relatively obvious
> question "why are we asserting errors can't happen" by more subtle ones.
> Feels like sweeping dirt under the rug.
> 

I'll grant you that. I'm going wide instead of deep, here. There were 
200+ errors to fix, and not every last one got the same level of attention.

I definitely did just sweep some dirt under the rug.

>>> We don't actually have a built-in QAPISchemaEnumType for the event enum.
>>> We merely generate a C enum QAPIEvent along with macro QAPIEvent_str(),
>>> by passing self._event_emit_name, self._event_enum_members straight to
>>> gen_enum() and gen_enum_lookup().
>>>
>>> If we did build a QAPISchemaEnumType, and used it to diagnose clashes,
>>> then clashes would be reported like
>>>
>>>       mumble.json: In event 'A-B':
>>>       mumble.json: 2: value 'A-B' collides with value 'A_B'
>>>
>>> leaving you guessing what "value" means, and where 'A_B' may be.
>>>
>>> Bug: we don't diagnose certain event name clashes.  I'll fix it.
>>>
>>
>> Not clear to me: If I want interface consistency, what *should* be
>> passed downwards as the info? it's not quite a builtin as much as it is
>> an inferred enum,
> 
> True.
> 
>>                    so should I just ... leave it like this, or wait for
>> you to offer a better fix?
> 
> Waiting for some better fix feels undavisable.  We want to get type
> checking in place sooner rather than later.
> 

OK. You mentioned fixing the conflicts, so I had thought maybe that was 
near-term instead of long-term. We have cleared that misunderstanding up ;)

> Aside: a possible fix is decoupling gen_enum_lookup() and gen_enum()
> from QAPISchemaEnumMember, so we don't have to make up
> QAPISchemaEnumMembers here.
> 

I'm not sure I have the broader picture here to do this, or the time to 
focus on it. It's a bit of a deeper fix than the rest of the minor 
refactorings I do in these series.

> I think there are two interpretations of your QAPISourceInfo work's aim:
> 
> 1. Narrow: use a special QAPISourceInfo rather then None as "no errors
>     shall happen" poison.
> 
>     QAPISourceInfo.builtin() returns this poison.  The name "builtin" is
>     less than ideal.
> 

I did intend it to be used for explicit built-in types; this is an 
"inferred" type, and I'd use a differently named poison for it, I suppose.

> 2. Ambitious: eliminate "no errors shall happen".
> 
> We're discussing this in reply of PATCH 06.  I think we need to reach a
> conclusion there before we can decide here.
> 

OK, I'll head over there. I'm still a bit confused over here, but we'll 
get to it.


>>>>    
>>>>    
>>>>    def gen_events(schema: QAPISchema,
>>>> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>>>> index 720449feee4d..d5f19732b516 100644
>>>> --- a/scripts/qapi/schema.py
>>>> +++ b/scripts/qapi/schema.py
>>>> @@ -23,6 +23,7 @@
>>>>    from .error import QAPIError, QAPISemError
>>>>    from .expr import check_exprs
>>>>    from .parser import QAPISchemaParser
>>>> +from .source import QAPISourceInfo
>>>>    
>>>>    
>>>>    class QAPISchemaEntity:
>>>> @@ -36,10 +37,10 @@ def __init__(self, name, info, doc, ifcond=None, features=None):
>>>>            self.name = name
>>>>            self._module = None
>>>>            # For explicitly defined entities, info points to the (explicit)
>>>> -        # definition.  For builtins (and their arrays), info is None.
>>>> -        # For implicitly defined entities, info points to a place that
>>>> -        # triggered the implicit definition (there may be more than one
>>>> -        # such place).
>>>> +        # definition.  For builtins (and their arrays), info is a null-object
>>>> +        # sentinel that evaluates to False. For implicitly defined entities,
>>>> +        # info points to a place that triggered the implicit definition
>>>> +        # (there may be more than one such place).
>>>
>>> s/builtins/built-in types/
>>>
>>> The meaning of "a null object sentinel" is less than clear.  Perhaps "a
>>> special object".
>>>
>>
>> OK.
>>
>>>>            self.info = info
>>>>            self.doc = doc
>>>>            self._ifcond = ifcond or []
>>>> @@ -209,7 +210,7 @@ class QAPISchemaBuiltinType(QAPISchemaType):
>>>>        meta = 'built-in'
>>>>    
>>>>        def __init__(self, name, json_type, c_type):
>>>> -        super().__init__(name, None, None)
>>>> +        super().__init__(name, QAPISourceInfo.builtin(), None)
>>>>            assert not c_type or isinstance(c_type, str)
>>>>            assert json_type in ('string', 'number', 'int', 'boolean', 'null',
>>>>                                 'value')
>>>> @@ -871,7 +872,7 @@ def resolve_type(self, name, info, what):
>>>>            return typ
>>>>    
>>>>        def _module_name(self, fname):
>>>> -        if fname is None:
>>>> +        if not fname:
>>>>                return None
>>>>            return os.path.relpath(fname, self._schema_dir)
>>>>    
>>>
>>> Sure this hunk belongs to this patch?
>>>
>>
>> Accident.
>>
>> "info and info.fname" does not behave the same with a falsey info object
>> as it does when info was genuinely None; I compensated for that *here*,
>> but I should have compensated for it elsewhere.
>>
>>>> @@ -897,9 +898,11 @@ def _def_builtin_type(self, name, json_type, c_type):
>>>>            # be nice, but we can't as long as their generated code
>>>>            # (qapi-builtin-types.[ch]) may be shared by some other
>>>>            # schema.
>>>> -        self._make_array_type(name, None)
>>>> +        self._make_array_type(name, QAPISourceInfo.builtin())
>>>>    
>>>>        def _def_predefineds(self):
>>>> +        info = QAPISourceInfo.builtin()
>>>> +
>>>
>>> Everything else gets its very own copy of the "no source info" object,
>>> except for the stuff defined here, which gets to share one.  Isn't that
>>> odd?
>>>
>>
>> It's also the only function where we define so many built-ins in the
>> same place, so spiritually they do have the same SourceInfo, right? :)
>>
>> (OK, no, it wasn't a conscious design choice, but it also seems
>> harmless. I am going to assume you'd prefer I not do this?)
> 
> It is harmless.  It just made me wonder why we create more than one such
> QAPISourceInfo in the first place.  Method builtin() could return the
> same one every time.  It could even be an attribute instead of a method.
> Anyway, no big deal.
> 

I could conceivably use source line information and stuff, to be 
needlessly fancy about it. Nah. I just think singleton patterns are kind 
of weird to implement in Python, so I didn't.

--js

Re: [PATCH 11/12] qapi/schema: Name the builtin module "" instead of None

[John Snow]

On 12/17/20 6:09 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 12/16/20 5:42 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> Instead of using None as the built-in module filename, use an empty
>>>> string instead.
>>>
>>> PATCH 05's changes the module name of the special system module for
>>> built-in stuff from None to './builtin'.  The other system modules are
>>> named like './FOO': './init' and './emit' currently.
>>>
>>> This one changes the module *filename* from None to "", and not just for
>>> the builtin module, but for *all* system modules.  Your commit message
>>> claims only "the built-in module", which is not true as far as I can
>>> tell.
>>>
>>
>> Is this true? ... "./init" and "./emit" are defined only in the
>> generators, outside of the schema entirely. They don't even have
>> "QAPISchemaModule" objects associated with them.
>>
>> I changed:
>>
>>           self._make_module(None)  # built-ins
>>
>>
>> to
>>
>>           self._make_module(QAPISourceInfo.builtin().fname)  # built-ins
>>
>>
>>
>> that should be precisely only "the" built-in module, shouldn't it? the
>> other "system" modules don't even pass through _make_module.
> 
> You're right.
> 
> The schema IR has only user-defined modules and the built-ins module.
> Each module has a name.  We use the source file name for the
> user-defined modules.  The built-ins module has none, so we use None.
> 
> The QAPISchemaModularCVisitor generates "modular" output.  It has
> per-module data, keyed by module name.  It supports user-defined and
> system modules.  We set them up as follows.  The user-defined modules
> are exactly the schema IR's (same name).  The system modules are the
> schema IR's built-ins module (same name) plus whatever the user of
> QAPISchemaModularCVisitor adds (convention: name starts with ./, so it
> can't clash).
> 
> Why let generators add system modules that don't exist in the schema IR?
> It's a reasonably clean way to generate stuff that doesn't fit elsewhere
> into separate .[ch] files.
> 
> PATCH 05 changes the name of the built-ins module from None to
> './builtins' in the QAPISchemaModularCVisitor only.  Correct?
> 

That's right. That was a useful change all by itself, and winds up being 
useful for the genc/genh typing.

> This patch changes the name of the built-ins module from None to "" in
> the schema IR only.  Correct?
> 

As far as I know, yes. ("Schema IR -- Internal Registry?")

>>> Should we use the opportunity to make the filename match the module
>>> name?
>>>
>>
>> If that's something you want to have happen, it can be done, yes.
> 
> Having different "module names" for the built-ins module in different
> places could be confusing.
> 

Yes, but we technically didn't use the generator-only names in the 
Schema before, so I didn't want to assume we'd want them here.

> The issue appears in PATCH 05.  I'm fine with the two module names
> diverging temporarily in this series.  I'd prefer them to be the same at
> the end.
> 

OK, no problem. I'll try to make this nicer and unify things just a 
pinch more.

--js

>> I had a draft that did it that way initially; I was afraid I was mixing
>> up two distinct things: the module fname (schema.py's concept of
>> modules) and module name (gen.py's concept of modules). This version of
>> my patch kept the two more distinct like they are currently.
>>
>> We can change "the" built-in module to have an fname of "./builtin"
>> which works just fine; gen.py just has to change to not add "./" to
>> modules already declared with the leading slash.
>>
>> Up for debate is if you want the system modules declared in the code
>> generators to have to declare 'emit' or './emit'; I left them alone and
>> allowed them to say 'event'.
> 
> I pass 'emit' to argument name, so a simple './' + name obviously
> results in a system module name.
> 
> With './emit', we achieve "obviously" by assert name.startswith('./').
> Matter of taste.
> 
>> Downside: the ./ prefix takes on special meaning in both gen.py and
>> schema.py. the module organization feels decentralized and fragile.
> 
> Making the './' prefix special is fine.  But you're right, doing it in
> two places and with precious little explanation is not so fine.
> 
> We could have schema.py provide some notion of "module name".  Weasel
> words "some notion" for artistic license.
> 
> If we'd prefer not to do it now, a few judicious comments should suffice
> for now.
> 
>>>>                   This allows us to clarify the type of various interfaces
>>>> dealing with module names as always taking a string, which saves us from
>>>> having to use Optional[str] everywhere.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>

Re: [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 12/17/20 3:02 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> On 12/16/20 5:18 AM, Markus Armbruster wrote:
>>>> John Snow <jsnow@redhat.com> writes:
>>>>
>>>>> --
>>>>>
>>>>> events.py had an info to route, was it by choice that it wasn't before?
>>>>
>>>> See below.
>>>>
>>>> I figure this is intentionally below the -- line, but ...
>>>>
>>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>>
>>>> ... this should be above it.
>>>>
>>>
>>> Script failure. Or human failure, because I used '--' instead of '---'.
>>>
>>>>> ---
>>>>>    scripts/qapi/events.py |  2 +-
>>>>>    scripts/qapi/schema.py | 23 +++++++++++++----------
>>>>>    scripts/qapi/types.py  |  9 +++++----
>>>>>    scripts/qapi/visit.py  |  6 +++---
>>>>>    4 files changed, 22 insertions(+), 18 deletions(-)
>>>>>
>>>>> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
>>>>> index 9851653b9d11..9ba4f109028d 100644
>>>>> --- a/scripts/qapi/events.py
>>>>> +++ b/scripts/qapi/events.py
>>>>> @@ -225,7 +225,7 @@ def visit_event(self,
>>>>>                                              self._event_emit_name))
>>>>>            # Note: we generate the enum member regardless of @ifcond, to
>>>>>            # keep the enumeration usable in target-independent code.
>>>>> -        self._event_enum_members.append(QAPISchemaEnumMember(name, None))
>>>>> +        self._event_enum_members.append(QAPISchemaEnumMember(name, info))
>>>>
>>>> We pass None because errors should never happen, and None makes that
>>>> quite clear.
>>>>
>>>
>>> Not clear: *why* should errors never happen? This is the only place we
>>> pass None for SourceInfo that isn't explicitly a literal built-in type.
>>> This is not obvious.
>> 
>> You're right, but there are two separate "unclarities".
>> 
>> Passing None effectively asserts "errors never happen".  We neglect to
>> explain why, leaving the reader guessing.
>> 
>> Passing @info "fixes" that by removing the assertion.  Now we have a
>> more subtle problem: will errors make sense with this @info?  Normally,
>> all members of an enum share one info.  Only this enum's members don't.
>> It turns out the question is moot because @info is not actually used.
>> But will it remain moot?  My point isn't that these are important
>> questions to answer.  It is that we're replacing the relatively obvious
>> question "why are we asserting errors can't happen" by more subtle ones.
>> Feels like sweeping dirt under the rug.
>> 
>
> I'll grant you that. I'm going wide instead of deep, here. There were 
> 200+ errors to fix, and not every last one got the same level of attention.
>
> I definitely did just sweep some dirt under the rug.
>
>>>> We don't actually have a built-in QAPISchemaEnumType for the event enum.
>>>> We merely generate a C enum QAPIEvent along with macro QAPIEvent_str(),
>>>> by passing self._event_emit_name, self._event_enum_members straight to
>>>> gen_enum() and gen_enum_lookup().
>>>>
>>>> If we did build a QAPISchemaEnumType, and used it to diagnose clashes,
>>>> then clashes would be reported like
>>>>
>>>>       mumble.json: In event 'A-B':
>>>>       mumble.json: 2: value 'A-B' collides with value 'A_B'
>>>>
>>>> leaving you guessing what "value" means, and where 'A_B' may be.
>>>>
>>>> Bug: we don't diagnose certain event name clashes.  I'll fix it.
>>>>
>>>
>>> Not clear to me: If I want interface consistency, what *should* be
>>> passed downwards as the info? it's not quite a builtin as much as it is
>>> an inferred enum,
>> 
>> True.
>> 
>>>                    so should I just ... leave it like this, or wait for
>>> you to offer a better fix?
>> 
>> Waiting for some better fix feels undavisable.  We want to get type
>> checking in place sooner rather than later.
>> 
>
> OK. You mentioned fixing the conflicts, so I had thought maybe that was 
> near-term instead of long-term. We have cleared that misunderstanding up ;)
>
>> Aside: a possible fix is decoupling gen_enum_lookup() and gen_enum()
>> from QAPISchemaEnumMember, so we don't have to make up
>> QAPISchemaEnumMembers here.
>> 
>
> I'm not sure I have the broader picture here to do this, or the time to 
> focus on it. It's a bit of a deeper fix than the rest of the minor 
> refactorings I do in these series.
>
>> I think there are two interpretations of your QAPISourceInfo work's aim:
>> 
>> 1. Narrow: use a special QAPISourceInfo rather then None as "no errors
>>     shall happen" poison.
>> 
>>     QAPISourceInfo.builtin() returns this poison.  The name "builtin" is
>>     less than ideal.
>> 
>
> I did intend it to be used for explicit built-in types; this is an 
> "inferred" type, and I'd use a differently named poison for it, I suppose.

In the narrow interpretation, I'd use just one poison, and I'd name it
in a way that signals "no error shall happen".

>> 2. Ambitious: eliminate "no errors shall happen".
>> 
>> We're discussing this in reply of PATCH 06.  I think we need to reach a
>> conclusion there before we can decide here.
>> 
>
> OK, I'll head over there. I'm still a bit confused over here, but we'll 
> get to it.

Just one more thought: narrow vs. ambitious need not be final.  We can
pass "no error shall happen" poison now, and still eliminate or reduce
the poison later on.

>>>>>    
>>>>>    
>>>>>    def gen_events(schema: QAPISchema,
>>>>> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>>>>> index 720449feee4d..d5f19732b516 100644
>>>>> --- a/scripts/qapi/schema.py
>>>>> +++ b/scripts/qapi/schema.py
>>>>> @@ -23,6 +23,7 @@
>>>>>    from .error import QAPIError, QAPISemError
>>>>>    from .expr import check_exprs
>>>>>    from .parser import QAPISchemaParser
>>>>> +from .source import QAPISourceInfo
>>>>>    
>>>>>    
>>>>>    class QAPISchemaEntity:
>>>>> @@ -36,10 +37,10 @@ def __init__(self, name, info, doc, ifcond=None, features=None):
>>>>>            self.name = name
>>>>>            self._module = None
>>>>>            # For explicitly defined entities, info points to the (explicit)
>>>>> -        # definition.  For builtins (and their arrays), info is None.
>>>>> -        # For implicitly defined entities, info points to a place that
>>>>> -        # triggered the implicit definition (there may be more than one
>>>>> -        # such place).
>>>>> +        # definition.  For builtins (and their arrays), info is a null-object
>>>>> +        # sentinel that evaluates to False. For implicitly defined entities,
>>>>> +        # info points to a place that triggered the implicit definition
>>>>> +        # (there may be more than one such place).
>>>>
>>>> s/builtins/built-in types/
>>>>
>>>> The meaning of "a null object sentinel" is less than clear.  Perhaps "a
>>>> special object".
>>>>
>>>
>>> OK.
>>>
>>>>>            self.info = info
>>>>>            self.doc = doc
>>>>>            self._ifcond = ifcond or []
>>>>> @@ -209,7 +210,7 @@ class QAPISchemaBuiltinType(QAPISchemaType):
>>>>>        meta = 'built-in'
>>>>>    
>>>>>        def __init__(self, name, json_type, c_type):
>>>>> -        super().__init__(name, None, None)
>>>>> +        super().__init__(name, QAPISourceInfo.builtin(), None)
>>>>>            assert not c_type or isinstance(c_type, str)
>>>>>            assert json_type in ('string', 'number', 'int', 'boolean', 'null',
>>>>>                                 'value')
>>>>> @@ -871,7 +872,7 @@ def resolve_type(self, name, info, what):
>>>>>            return typ
>>>>>    
>>>>>        def _module_name(self, fname):
>>>>> -        if fname is None:
>>>>> +        if not fname:
>>>>>                return None
>>>>>            return os.path.relpath(fname, self._schema_dir)
>>>>>    
>>>>
>>>> Sure this hunk belongs to this patch?
>>>>
>>>
>>> Accident.
>>>
>>> "info and info.fname" does not behave the same with a falsey info object
>>> as it does when info was genuinely None; I compensated for that *here*,
>>> but I should have compensated for it elsewhere.
>>>
>>>>> @@ -897,9 +898,11 @@ def _def_builtin_type(self, name, json_type, c_type):
>>>>>            # be nice, but we can't as long as their generated code
>>>>>            # (qapi-builtin-types.[ch]) may be shared by some other
>>>>>            # schema.
>>>>> -        self._make_array_type(name, None)
>>>>> +        self._make_array_type(name, QAPISourceInfo.builtin())
>>>>>    
>>>>>        def _def_predefineds(self):
>>>>> +        info = QAPISourceInfo.builtin()
>>>>> +
>>>>
>>>> Everything else gets its very own copy of the "no source info" object,
>>>> except for the stuff defined here, which gets to share one.  Isn't that
>>>> odd?
>>>>
>>>
>>> It's also the only function where we define so many built-ins in the
>>> same place, so spiritually they do have the same SourceInfo, right? :)
>>>
>>> (OK, no, it wasn't a conscious design choice, but it also seems
>>> harmless. I am going to assume you'd prefer I not do this?)
>> 
>> It is harmless.  It just made me wonder why we create more than one such
>> QAPISourceInfo in the first place.  Method builtin() could return the
>> same one every time.  It could even be an attribute instead of a method.
>> Anyway, no big deal.
>> 
>
> I could conceivably use source line information and stuff, to be 
> needlessly fancy about it. Nah. I just think singleton patterns are kind 
> of weird to implement in Python, so I didn't.

Stupidest singleton that could possibly work: in __init__,
self.singleton = ...

Re: [PATCH 11/12] qapi/schema: Name the builtin module "" instead of None

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 12/17/20 6:09 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> On 12/16/20 5:42 AM, Markus Armbruster wrote:
>>>> John Snow <jsnow@redhat.com> writes:
>>>>
>>>>> Instead of using None as the built-in module filename, use an empty
>>>>> string instead.
>>>>
>>>> PATCH 05's changes the module name of the special system module for
>>>> built-in stuff from None to './builtin'.  The other system modules are
>>>> named like './FOO': './init' and './emit' currently.
>>>>
>>>> This one changes the module *filename* from None to "", and not just for
>>>> the builtin module, but for *all* system modules.  Your commit message
>>>> claims only "the built-in module", which is not true as far as I can
>>>> tell.
>>>>
>>>
>>> Is this true? ... "./init" and "./emit" are defined only in the
>>> generators, outside of the schema entirely. They don't even have
>>> "QAPISchemaModule" objects associated with them.
>>>
>>> I changed:
>>>
>>>           self._make_module(None)  # built-ins
>>>
>>>
>>> to
>>>
>>>           self._make_module(QAPISourceInfo.builtin().fname)  # built-ins
>>>
>>>
>>>
>>> that should be precisely only "the" built-in module, shouldn't it? the
>>> other "system" modules don't even pass through _make_module.
>> 
>> You're right.
>> 
>> The schema IR has only user-defined modules and the built-ins module.
>> Each module has a name.  We use the source file name for the
>> user-defined modules.  The built-ins module has none, so we use None.
>> 
>> The QAPISchemaModularCVisitor generates "modular" output.  It has
>> per-module data, keyed by module name.  It supports user-defined and
>> system modules.  We set them up as follows.  The user-defined modules
>> are exactly the schema IR's (same name).  The system modules are the
>> schema IR's built-ins module (same name) plus whatever the user of
>> QAPISchemaModularCVisitor adds (convention: name starts with ./, so it
>> can't clash).
>> 
>> Why let generators add system modules that don't exist in the schema IR?
>> It's a reasonably clean way to generate stuff that doesn't fit elsewhere
>> into separate .[ch] files.
>> 
>> PATCH 05 changes the name of the built-ins module from None to
>> './builtins' in the QAPISchemaModularCVisitor only.  Correct?
>> 
>
> That's right. That was a useful change all by itself, and winds up being 
> useful for the genc/genh typing.
>
>> This patch changes the name of the built-ins module from None to "" in
>> the schema IR only.  Correct?
>> 
>
> As far as I know, yes. ("Schema IR -- Internal Registry?")

Internal representation (the stuff in schema.py).  Compiler jargon,
sorry.

>>>> Should we use the opportunity to make the filename match the module
>>>> name?
>>>>
>>>
>>> If that's something you want to have happen, it can be done, yes.
>> 
>> Having different "module names" for the built-ins module in different
>> places could be confusing.
>> 
>
> Yes, but we technically didn't use the generator-only names in the 
> Schema before, so I didn't want to assume we'd want them here.

We can't have them there, because the things they name don't exist
there.

What we can have is a consistent naming convention that leads to same
things having the same names in both places.

>> The issue appears in PATCH 05.  I'm fine with the two module names
>> diverging temporarily in this series.  I'd prefer them to be the same at
>> the end.
>> 
>
> OK, no problem. I'll try to make this nicer and unify things just a 
> pinch more.
>
> --js

Re: [PATCH 06/12] qapi/source: Add builtin null-object sentinel

[John Snow]

On 12/17/20 7:33 AM, Markus Armbruster wrote:
> A patch limited to the first aspect merely tweaks an implementation
> detail.
> 
> As soon as we include the second aspect, we get to debate how to handle
> programming errors, and maybe whether any of the errors involving a
> QAPISourceInfo.builtin() are*not*  programming errors.
> 
> I'd prefer this series to remain focused on "enabling strict optional
> checking in mypy for everything we have typed so far".

That is still the focus -- I believe adding a poison pill as you suggest 
actually causes more errors instead of "maintaining" them; it doesn't 
maintain a status quo.

Now, I understand the concern that this is opening a chance to allow a 
new class of errors to slip by undetected, but I don't believe it does 
in a meaningful way.

I didn't intend for this info object to be a poison; it wasn't designed 
as one, nor should it be. It's merely a SourceInfo that represents the 
degenerate case that something-or-other does not have an explicit user 
source.

You're right that it probably shouldn't be seen in normal conditions, 
but if it has; my take is that something *else* has already gone 
horribly wrong and we aren't just sweeping dust under the mat, actually.


Here's a recap of my thinking process in identifying the problem and my 
fix for it:

- Many functions expect that QAPISourceInfo *will* be present. Largely, 
they are correct in expecting this, but very few of them check or assert 
this. This is perfectly normal for untyped Python.

- Taxonomically, however, this is untrue, because the built-in entities 
do not need to specify one.

- This can be addressed in typed python by adding assertions everywhere 
("assert info is not None"), or it can be addressed by making the core 
assumption true ("QAPISourceInfo is not Optional").

I opted to make the core assumption true; which does introduce a new 
ambiguity that may not have been present before:

"If we have an info object, is it a real user-source info object, or is 
it the built-in sentinel?"

I *think* this is where you start to get worried; we have "lost" the 
ability to know, in an "obvious" way if this object is "valid" for a 
given context. The assumption is that the code will explode violently if 
we are passed None instead of a valid source info.

I don't think this assumption is true, though. mypy informs me that we 
were never checking to see if it was valid anyway, so we already have 
this ambiguity in the code today. Given that the 'info' object is, in 
most cases, not used until *after we have detected another error*, 
changing the info object to be non-none does not suppress any errors 
that didn't already exist in these contexts.

What about places where QAPISourceInfo is used outside of an 
error-handling context to gain contextual information about an entity? 
Do we open ourselves up to new cases where we may have had an implicit 
error, but now we do not?


Explicit usages of QAPISourceInfo are here:

1. QAPISchemaEntity._set_module(), which uses the info fname to find the 
QAPISchemaModule object. This already checks for "None" info now, and 
will check for the "./builtin" info after the series.

2. QAPISchemaEntity.is_implicit(), which uses the falsey nature of info 
to intuit that it is a built-in definition. No change in behavior.

3. QAPISchemaArrayType.check(), which uses the falsey nature of 
self.info to conditionally pass self.info.defn_meta to 
QAPISchema.resolve_type().

Found a bug (previously fixed in part 6 prior to enabling type checking 
on schema.py, which is why it went undetected here. I am moving the fix 
forward into this series):

info and info.defn_meta will evaluate to the sentinel QAPISourceInfo 
instead of an empty string or None, but it can be changed simply to just 
"info.defn_meta" to take the None value from the builtin source info's 
defn_meta field; easy.

4. QAPISchemaMember.describe(), which uses info.defn_name. This is only 
ever called in error-handling contexts, so we aren't reducing total 
error coverage here, either.

5. QAPISchemaCommand.check(), which uses info.pragma.returns_whitelist. 
This might legitimately create a new avenue where we succeed where we 
used to fail, if we were to create a built-in command. Whether or not 
this is desirable depends on how you want pragma directives to work 
long-term; per-file or per-schema. (Do you want to let user-defined 
schemas dictate how built-in commands work?)

I don't think this is an issue.

6. QAPISchema._def_entity(), which asserts that info must be true-ish, 
or we are in the predefining state. This logic doesn't change.

7. QAPISchema._def_entity(), which uses the true-ish nature of 
other_ent.info to print a different error message. This logic doesn't 
change.

8. expr.py:check_type(), which uses info.pragma.returns_whitelist. This 
theoretically adds slack, but parser.py always gives us real info objects.

9. expr.py:check_exprs(), which calls info.set_defn(meta, name). 
Obviously this must not be None because parser.py does not create 
"built-in" source objects; we know with certainty here that we cannot 
get such a thing.

10. expr.py:check_exprs() again, checking info.pragma.doc_required. As 
above, we know this is always a true source info.

11. QAPIError's str() method itself will call str(info), but if info is 
None, it doesn't crash -- you just get "None". Now you'll get 
"[builtin]". Doesn't permit any new error cases.



Conclusions:

- There are users that use info to decide on behavior outside of an 
error context, but they are already cared for.

- There are some pre-schema.py uses of info that may cause less problems 
than they used to, but would require someone to add the placeholder 
object into parser.py. We can throw a single assertion in expr.py to 
make sure it never happens by accident.

- There are no, and I doubt there ever will be, users who use str(info) 
to decide on behavior. There are only exception handlers that use this 
method. There is no value to making str(info) in particular a poison pill.

- There might be some limited benefit to poisoning info.pragma, depending.


Ultimately, I consider this the standard idiomatic usage of info:

```
def some_func(info):
     if (error):
         raise QAPISemError(info, "you forgot to defrobulate")
```

When we go to print the error message to the user, the exception handler 
is going to fault because str(info) is poisoned. Is it useful to poison 
the exception handler here?

I don't think so; especially considering that even "None" here will work 
just fine!


So I suppose I would prefer not to add poison, because I don't think 
it's necessary, or improves anything materially.

--js

Re: [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory

[John Snow]

On 12/18/20 12:24 AM, Markus Armbruster wrote:
>> I could conceivably use source line information and stuff, to be
>> needlessly fancy about it. Nah. I just think singleton patterns are kind
>> of weird to implement in Python, so I didn't.
> Stupidest singleton that could possibly work: in __init__,
> self.singleton = ...
> 
> 

Yeah, you can make a class variable that has a builtin singleton, then 
make the class method return that class variable.

Feels fancier than my laziness permits. I just put it back to using one 
copy per definition.

Re: [PATCH 06/12] qapi/source: Add builtin null-object sentinel

[John Snow]

On 12/17/20 6:56 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 12/16/20 4:22 AM, Markus Armbruster wrote:
>>> 2. On error with "no source info", don't crash.
>>>      I have my doubts on this one.
>>>      Such an error means the QAPI code generator screwed up, at least
>>> in
>>>      theory.  Crashing is only proper.  It gets the screwup fixed.
>>>
>>
>> QAPISemError and friends will also halt the generator and don't
>> produce output and will fail tests. They aren't less visible or more
>> ignorable somehow.
>>
>>>      In practice, errors due to interactions between built-in stuff and
>>>      user-defined stuff could conceivably escape testing.  I can't
>>>      remember such a case offhand.
>>>      Will the "no source info" error be more useful than a crash?
>>>      Possibly.  Will it get the screwup fixed?  Maybe not.
>>
>> I don't understand this; if it's an error -- there's no QAPI, there's
>> no QEMU. It's definitely getting fixed.
>>
>> If QAPISourceInfo is primarily used for printing error information, we
>> are already in a situation where the generator is hosed and has
>> wandered itself into a problem that can't be ignored.
>>
>> There's no additional value in having python crash twice per every
>> crash because we have bad types in our error reporting functions.
> 
> Consider the following scenario.  The average developer knows just
> enough about QAPI to be dangerous.  That's okay; if you had to be a QAPI
> expert to modify the QAPI schema, we would have failed.  Now meet Joe
> Average.  He's a good guy.  Today his job happens to require extending
> the QAPI schema.  In a hurry, as usual.  So Joe brings his well-honed
> voodoo programming skills to bear, and writes something that looks
> plausible to him.  His build fails.  He's not surprised; he's voodoo-
> programming after all.  However, the error message is less clear than
> usual.  Something about a '[builtin]' file.  There is no '[builtin]'
> file.  What to do?  Obvious!  If a bit of voodoo doesn't get you over
> the finish line, use more: twiddle the schema until it works.
> 
> If his build failed with a Python backtrace instead, Joe would
> immediately know that he ran into a bug in our tooling he should report.
> 
> Again, I don't mean to criticize Joe.  I've walked in his shoes myself.
> 
> 

Worse, Joe was going to see an error about the "None" file or an error 
about the "[builtin]" file.

I understand, though: don't suppress crash messages with opaque crap 
that doesn't help spot the error. It's just that I don't think that's 
what is happening here.

See lengthy response in other email.

--js

Re: [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 12/18/20 12:24 AM, Markus Armbruster wrote:
>>> I could conceivably use source line information and stuff, to be
>>> needlessly fancy about it. Nah. I just think singleton patterns are kind
>>> of weird to implement in Python, so I didn't.
>> Stupidest singleton that could possibly work: in __init__,
>> self.singleton = ...
>> 
>
> Yeah, you can make a class variable that has a builtin singleton, then
> make the class method return that class variable.
>
> Feels fancier than my laziness permits. I just put it back to using
> one copy per definition.

Why have a class method around the attribute?  Just use the stoopid
attribute already ;-P

Re: [PATCH 08/12] qapi/schema: make QAPISourceInfo mandatory

[John Snow]

On 12/18/20 3:57 PM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 12/18/20 12:24 AM, Markus Armbruster wrote:
>>>> I could conceivably use source line information and stuff, to be
>>>> needlessly fancy about it. Nah. I just think singleton patterns are kind
>>>> of weird to implement in Python, so I didn't.
>>> Stupidest singleton that could possibly work: in __init__,
>>> self.singleton = ...
>>>
>>
>> Yeah, you can make a class variable that has a builtin singleton, then
>> make the class method return that class variable.
>>
>> Feels fancier than my laziness permits. I just put it back to using
>> one copy per definition.
> 
> Why have a class method around the attribute?  Just use the stoopid
> attribute already ;-P
> 

Something has to initialize it:

```
class Blah:
     magic = Blah()

     def __init__(self):
         pass
```

Won't work; Blah isn't defined yet. a classmethod works though:

```
class Blah:
     magic = None

     def __init__(self) -> None:
         pass

     @classmethod
     def make(cls) -> 'Blah':
         if cls.magic is None:
             cls.magic = cls()
         return cls.magic
```