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

[PATCH v2 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.

V2:

001/12:[----] [--] 'qapi/commands: assert arg_type is not None'
002/12:[----] [--] 'qapi/events: fix visit_event typing'
003/12:[0003] [FC] 'qapi/main: handle theoretical None-return from re.match()'
004/12:[----] [--] 'qapi/gen: assert that _start_if is not None in _wrap_ifcond'
005/12:[0003] [FC] 'qapi/gen: use './builtin' for the built-in module name'
006/12:[0004] [FC] 'qapi/source: Add builtin null-object sentinel'
007/12:[0024] [FC] 'qapi/schema: make QAPISourceInfo mandatory'
008/12:[----] [--] 'qapi/gen: write _genc/_genh access shims'
009/12:[----] [--] 'qapi/gen: move write method to QAPIGenC, make fname a str'
010/12:[----] [--] 'tests/qapi-schema: Add quotes to module name in test output'
011/12:[0004] [FC] 'qapi/schema: Name the builtin module "" instead of None'
012/12:[----] [--] 'qapi: enable strict-optional checks'

- This revision isn't quite complete, but due to deadlines and
  timezones, opted to send the "revision so far". It keeps some
  imperfect fixes that Markus is devising better alternatives for.

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/schema: make QAPISourceInfo mandatory
  qapi/gen: write _genc/_genh access shims
  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                      | 108 ++++++++++++-----------
 scripts/qapi/main.py                     |   2 +
 scripts/qapi/mypy.ini                    |   1 -
 scripts/qapi/schema.py                   |  35 ++++----
 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, 145 insertions(+), 109 deletions(-)

-- 
2.26.2

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

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

[PATCH v2 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 | 2 ++
 1 file changed, 2 insertions(+)

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

[PATCH v2 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 b40f18eee3c..a6dc991b1d0 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 v2 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 | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index a6dc991b1d0..476d0adac4e 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,15 @@ def _module_basename(self, what: str, name: Optional[str]) -> str:
             if name != self._main_module:
                 ret += '-' + os.path.splitext(basename)[0]
         else:
-            name = name[2:] if name else 'builtin'
-            ret += re.sub(r'-', '-' + name + '-', what)
+            assert name.startswith('./')
+            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 +290,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('./' + name, blurb)
 
     def write(self, output_dir: str, opt_builtins: bool = False) -> None:
         for name in self._module:
@@ -310,7 +310,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 v2 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" remains 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 d7a79a9b8ae..a049b73b57b 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 an instance corresponding to a built-in definition.
+        """
+        return cls('', -1, None)
+
+    def __bool__(self) -> bool:
+        # "if info" is false if @info corresponds to a built-in definition.
+        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 v2 07/12] qapi/schema: make QAPISourceInfo mandatory

[John Snow]

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

---

The event_enum_members change might become irrelevant after a
forthcoming (?) patch by Markus, but didn't have it in-hand at time of
publishing.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/events.py |  2 +-
 scripts/qapi/schema.py | 25 ++++++++++++++-----------
 scripts/qapi/types.py  |  9 +++++----
 scripts/qapi/visit.py  |  6 +++---
 4 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index 9851653b9d1..9ba4f109028 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 720449feee4..0449771dfe5 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 built-in types (and their arrays), info is a
+        # special object 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 []
@@ -68,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 if info else None)
         self._module.add_entity(self)
 
     def set_module(self, schema):
@@ -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')
@@ -897,7 +898,7 @@ 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):
         for t in [('str',    'string',  'char' + POINTER_SUFFIX),
@@ -917,16 +918,18 @@ 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', QAPISourceInfo.builtin(),
+            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], QAPISourceInfo.builtin())
 
-        self._def_entity(QAPISchemaEnumType('QType', None, None, None, None,
-                                            qtype_values, 'QTYPE'))
+        self._def_entity(QAPISchemaEnumType(
+            'QType', QAPISourceInfo.builtin(), None,
+            None, None, qtype_values, 'QTYPE'))
 
     def _make_features(self, features, info):
         if features is None:
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index 2b4916cdaa1..a3a16284006 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 339f1521524..3f49c307c56 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 v2 08/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 476d0adac4e..12ea98fb61e 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('./')
@@ -282,7 +291,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)
@@ -315,8 +324,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 v2 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>

---

Possibly un-needed in concert with a forthcoming patch by Markus, but I
didn't have it in my hands at time of publishing.

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 71744f48a35..b346676d15a 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 12ea98fb61e..2dd99635e74 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 v2 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 273f0f54e16..08aba8354e2 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 419284dae29..83a3d9bd69b 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 69666c39ad2..0dac23c80c1 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 42ae519656d..ace511ba5a9 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 0b654ddebb6..f7ab4987943 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 061f81e5090..81bdeb887b6 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 04356775cd1..361a58185e6 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 8868ca0dca9..4f5ab9fd596 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 e8db9d09d91..4adf0b3c185 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 v2 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 2dd99635e74..1933090a2a0 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -310,14 +310,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 0449771dfe5..0235208966a 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.fname if info else None)
+        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 fname is None:
-            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 a3a16284006..12eeea3aaff 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 3f49c307c56..76e34ee7f02 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 08aba8354e2..02000c06e5e 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 83a3d9bd69b..494533d7479 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 0dac23c80c1..059caa4e1d2 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 ace511ba5a9..4d9d2b519f4 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 f7ab4987943..31d64631b66 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 81bdeb887b6..1b35b329571 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 361a58185e6..aed689e7bd6 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 4f5ab9fd596..4a899ba63ec 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 v2 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 74fc6c82153..04bd5db5278 100644
--- a/scripts/qapi/mypy.ini
+++ b/scripts/qapi/mypy.ini
@@ -1,6 +1,5 @@
 [mypy]
 strict = True
-strict_optional = False
 disallow_untyped_calls = False
 python_version = 3.6
 
-- 
2.26.2

Re: [PATCH v2 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 b40f18eee3c..a6dc991b1d0 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],

Let's inline ._wrap_ifcond() into .end_if().  Not a blocker, of course.

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

[Markus Armbruster]

Spelling nitpick: s/builtin/built-in/ in the title.

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.
>
> This object will evaluate as False, so "if info" remains 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>

As I pointed out in review of v1, this patch has two aspects mixed up:

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

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

The first one is what de-complicates interface typing.  It's clearly
serving this patch series' stated purpose: "static typing conversion".

The second one is not.  It sidetracks us into a design discussion that
isn't related to static typing.  Maybe it's something we should discuss.
Maybe the discussion will make us conclude we want to do this.  But
letting the static typing work get delayed by that discussion would be
stupid, and I'll do what I can to prevent that.

The stupidest possible solution that preserves the crash is adding an
assertion right where it crashes before this patch: in
QAPISourceInfo.__str__().  Yes, crashing in a __str__() method is not
nice, but it's no worse than before.  Making it better than before is a
good idea, and you're quite welcome to try, but please not in this
series.  Add a TODO comment asking for "make it better", then sit on
your hands.

The pipeline must move.

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Signed-off-by: John Snow <jsnow@redhat.com>
>
> ---
>
> The event_enum_members change might become irrelevant after a
> forthcoming (?) patch by Markus, but didn't have it in-hand at time of
> publishing.

It's in my "[PATCH 00/11] Drop support for QAPIGen without a file name",
which includes parts of your v1.  The parts that are new should be
injected into your series so they replace your "[PATCH v2 09/12]
qapi/gen: move write method to QAPIGenC, make fname a str".  Holler if
you need help.

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/events.py |  2 +-
>  scripts/qapi/schema.py | 25 ++++++++++++++-----------
>  scripts/qapi/types.py  |  9 +++++----
>  scripts/qapi/visit.py  |  6 +++---
>  4 files changed, 23 insertions(+), 19 deletions(-)
>
> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
> index 9851653b9d1..9ba4f109028 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))

This "enum" is not supposed to be erroneous.  If it is, it's a bug.

Your patch changes how the code behaves should such a bug bite here.
Before, we crash.  Afterwards, we report the bug using @info, which I'd
expect to produce utterly confusing error messages.

My comments on PATCH 06 apply: how the code should behave here is a
design issue that should be kept out of this patch series.

If you need to pass a value other than None to help with static typing,
then pass a suitable poison info that will crash right where None
crashes now.

>  def gen_events(schema: QAPISchema,
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 720449feee4..0449771dfe5 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 built-in types (and their arrays), info is a
> +        # special object 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 []
> @@ -68,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 if info else None)

Looks unrelated.

>          self._module.add_entity(self)
>  
>      def set_module(self, schema):
[...]

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Hi, this patchset enables strict optional checking in mypy for
> everything we have typed so far.
>
> In general, this patchset seeks to 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.

Just two issues left that aren't entirely trivial: 1. mission creep
[PATCH 6+7], 2. integrating my "[PATCH 00/11] Drop support for QAPIGen
without a file name".

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

[John Snow]

On 1/13/21 10:14 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 b40f18eee3c..a6dc991b1d0 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],
> 
> Let's inline ._wrap_ifcond() into .end_if().  Not a blocker, of course.
> 

I thought I did do this, but I guess it didn't make it out to list. I'll 
look into that.

I was planning on taking your series and trying to splice it into this 
one here and sending it back out as a conjoined thing.

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

[John Snow]

On 1/13/21 10:39 AM, Markus Armbruster wrote:
> Spelling nitpick: s/builtin/built-in/ in the title.
> 

Sure.

> 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.
>>
>> This object will evaluate as False, so "if info" remains 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>
> 
> As I pointed out in review of v1, this patch has two aspects mixed up:
> 
> 1. Represent "no source info" as special QAPISourceInfo instead of
>     None
> 
> 2. On error with "no source info", don't crash.
> 
> The first one is what de-complicates interface typing.  It's clearly
> serving this patch series' stated purpose: "static typing conversion".
> 
> The second one is not.  It sidetracks us into a design discussion that
> isn't related to static typing.  Maybe it's something we should discuss.
> Maybe the discussion will make us conclude we want to do this.  But
> letting the static typing work get delayed by that discussion would be
> stupid, and I'll do what I can to prevent that.
> 

It's not unrelated. It's about finding the most tactical incision to 
make the types as we actually use them correct from a static analysis 
context.

Maybe there's another tactical incision to make that's "smaller", for 
some perception of "smaller", but it's not unrelated.

> The stupidest possible solution that preserves the crash is adding an
> assertion right where it crashes before this patch: in
> QAPISourceInfo.__str__().  Yes, crashing in a __str__() method is not
> nice, but it's no worse than before.  Making it better than before is a
> good idea, and you're quite welcome to try, but please not in this
> series.  Add a TODO comment asking for "make it better", then sit on
> your hands.

I'm recently back from a fairly long PTO, so forgive me if I am 
forgetting something, but I am not really sure I fundamentally 
understand the nature of this critique.

Making functions not "crash" is a side-effect of making the types 
correct. I don't see it as scope-creep, it's a solution to a problem 
under active consideration.

In my reply to your earlier critique, I (think) I mentioned that I 
didn't understand the difference between:

1. An exception handler itself crashes because it received a value of 
unexpected type, or

2. The exception handler printed a message that indicates a problem with 
a built-in source definition.

In either case, QAPI didn't get built and it printed some kind of error 
spaghetti to the screen. In both cases, something much more seriously 
wrong has happened and the error message likely does not prepare the 
human user to really genuinely understand what that seriously wrong 
thing is.

I think this is an on-mission patch that improves circumstances; with 
regards to matters of taste I would see it as a lateral move at worst 
(one weird error for another weird error).

I'm left a little confused by the pushback, so I don't feel equipped to 
try and write code that addresses it.

Let's chat on IRC?

--js

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

[John Snow]

On 1/13/21 11:12 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> ---
>>
>> The event_enum_members change might become irrelevant after a
>> forthcoming (?) patch by Markus, but didn't have it in-hand at time of
>> publishing.
> 
> It's in my "[PATCH 00/11] Drop support for QAPIGen without a file name",
> which includes parts of your v1.  The parts that are new should be
> injected into your series so they replace your "[PATCH v2 09/12]
> qapi/gen: move write method to QAPIGenC, make fname a str".  Holler if
> you need help.
> 
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qapi/events.py |  2 +-
>>   scripts/qapi/schema.py | 25 ++++++++++++++-----------
>>   scripts/qapi/types.py  |  9 +++++----
>>   scripts/qapi/visit.py  |  6 +++---
>>   4 files changed, 23 insertions(+), 19 deletions(-)
>>
>> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
>> index 9851653b9d1..9ba4f109028 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))
> 
> This "enum" is not supposed to be erroneous.  If it is, it's a bug.
> 
> Your patch changes how the code behaves should such a bug bite here.
> Before, we crash.  Afterwards, we report the bug using @info, which I'd
> expect to produce utterly confusing error messages.
> 

It doesn't change the behavior *here*, though. It changes it whenever 
this info object is used in another context. ... and who knows when or 
where or why it is being used, or by whom.

I'll have to play with this. I'm not sure there's any way to coax a bug 
to happen here that I am aware of right away. Can you think of how to 
will one into existence?

> My comments on PATCH 06 apply: how the code should behave here is a
> design issue that should be kept out of this patch series.
> 
> If you need to pass a value other than None to help with static typing,
> then pass a suitable poison info that will crash right where None
> crashes now.
> 

I think we need to, yes; or we probably really, really want to. Making 
the info parameter optional really adds a lot of unidiomatic 
type-checking confetti when we go to use info, and in many more contexts 
than just this sorta-built-in-enum; it will creep badly.

So, I gotta pass ...something here. but what? You want poison, but I 
think it's not right to fundamentally poison all built-ins.

Mmmmmmm. Maybe per-instance poison can be done? We actually share info 
objects, but I can make poisoned copies. Using next_line() as a basis:

     def poison(self: T) -> T:
         info = copy.copy(self)
         info.poisoned = True
         return info

probably almost anything I do is not going to make a lot of sense unless 
I can actually replicate and test the different error scenarios to prove 
that we didn't make the error spaghetti unknowably worse. I see it as 
functionally inevitable that I have to audit this and make sure we get 
good error messages anyway, so ... maybe I just ought to do that now anyway.

>>   def gen_events(schema: QAPISchema,
>> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>> index 720449feee4..0449771dfe5 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 built-in types (and their arrays), info is a
>> +        # special object 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 []
>> @@ -68,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 if info else None)
> 
> Looks unrelated.
> 

Hmm, it sorta is. I have apparently edited this patch since I sent it, 
but there was some tomfoolery over how "x and y" statements behave and 
this edit was necessary at the time.

"info and info.fname" returned None when info could actually be None, 
but when it was updated to be a special source object, we could 
accidentally pass that special source object as the name -- instead of 
None. Not good.

I think I re-ordered some patches such that I can just pass in 
"info.fname" unconditionally instead as of this patch.

>>           self._module.add_entity(self)
>>   
>>       def set_module(self, schema):
> [...]
> 

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

[Eduardo Habkost]

On Wed, Jan 13, 2021 at 06:04:29PM -0500, John Snow wrote:
> On 1/13/21 11:12 AM, Markus Armbruster wrote:
> > John Snow <jsnow@redhat.com> writes:
> > 
> > > Signed-off-by: John Snow <jsnow@redhat.com>
> > > 
> > > ---
> > > 
> > > The event_enum_members change might become irrelevant after a
> > > forthcoming (?) patch by Markus, but didn't have it in-hand at time of
> > > publishing.
> > 
> > It's in my "[PATCH 00/11] Drop support for QAPIGen without a file name",
> > which includes parts of your v1.  The parts that are new should be
> > injected into your series so they replace your "[PATCH v2 09/12]
> > qapi/gen: move write method to QAPIGenC, make fname a str".  Holler if
> > you need help.
> > 
> > > Signed-off-by: John Snow <jsnow@redhat.com>
> > > ---
> > >   scripts/qapi/events.py |  2 +-
> > >   scripts/qapi/schema.py | 25 ++++++++++++++-----------
> > >   scripts/qapi/types.py  |  9 +++++----
> > >   scripts/qapi/visit.py  |  6 +++---
> > >   4 files changed, 23 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
> > > index 9851653b9d1..9ba4f109028 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))
> > 
> > This "enum" is not supposed to be erroneous.  If it is, it's a bug.
> > 
> > Your patch changes how the code behaves should such a bug bite here.
> > Before, we crash.  Afterwards, we report the bug using @info, which I'd
> > expect to produce utterly confusing error messages.
> > 
> 
> It doesn't change the behavior *here*, though. It changes it whenever this
> info object is used in another context. ... and who knows when or where or
> why it is being used, or by whom.
> 
> I'll have to play with this. I'm not sure there's any way to coax a bug to
> happen here that I am aware of right away. Can you think of how to will one
> into existence?
> 
> > My comments on PATCH 06 apply: how the code should behave here is a
> > design issue that should be kept out of this patch series.
> > 
> > If you need to pass a value other than None to help with static typing,
> > then pass a suitable poison info that will crash right where None
> > crashes now.

I don't understand what would be the point of inventing something
that behaves exactly like None but makes type checking less
useful.

With None/Optional, mypy gives us a way to be 100% sure the
object isn't going to invalid.  With a poison value, mypy can't
tell us anymore if the code risks crashing at runtime.

> > 
> 
> I think we need to, yes; or we probably really, really want to. Making the
> info parameter optional really adds a lot of unidiomatic type-checking
> confetti when we go to use info, and in many more contexts than just this
> sorta-built-in-enum; it will creep badly.

Which methods would require unidiomatic type-checking because of
None/Optional?

> 
> So, I gotta pass ...something here. but what? You want poison, but I think
> it's not right to fundamentally poison all built-ins.
> 
> Mmmmmmm. Maybe per-instance poison can be done? We actually share info
> objects, but I can make poisoned copies. Using next_line() as a basis:
> 
>     def poison(self: T) -> T:
>         info = copy.copy(self)
>         info.poisoned = True
>         return info
> 
> probably almost anything I do is not going to make a lot of sense unless I
> can actually replicate and test the different error scenarios to prove that
> we didn't make the error spaghetti unknowably worse. I see it as
> functionally inevitable that I have to audit this and make sure we get good
> error messages anyway, so ... maybe I just ought to do that now anyway.
> 
> > >   def gen_events(schema: QAPISchema,
> > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > > index 720449feee4..0449771dfe5 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 built-in types (and their arrays), info is a
> > > +        # special object 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 []
> > > @@ -68,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 if info else None)
> > 
> > Looks unrelated.
> > 
> 
> Hmm, it sorta is. I have apparently edited this patch since I sent it, but
> there was some tomfoolery over how "x and y" statements behave and this edit
> was necessary at the time.
> 
> "info and info.fname" returned None when info could actually be None, but
> when it was updated to be a special source object, we could accidentally
> pass that special source object as the name -- instead of None. Not good.
> 
> I think I re-ordered some patches such that I can just pass in "info.fname"
> unconditionally instead as of this patch.
> 
> > >           self._module.add_entity(self)
> > >       def set_module(self, schema):
> > [...]
> > 
> 

-- 
Eduardo

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

[John Snow]

On 1/13/21 7:29 PM, Eduardo Habkost wrote:
> On Wed, Jan 13, 2021 at 06:04:29PM -0500, John Snow wrote:
>> On 1/13/21 11:12 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:

...

>>
>> I think we need to, yes; or we probably really, really want to. Making the
>> info parameter optional really adds a lot of unidiomatic type-checking
>> confetti when we go to use info, and in many more contexts than just this
>> sorta-built-in-enum; it will creep badly.
> 
> Which methods would require unidiomatic type-checking because of
> None/Optional?
> 

Virtually everything that accepts a QAPISourceInfo has to do something 
like this:

def foo(info: Optional[QAPISourceInfo]):
     if info is None:
         raise Exception("Something very bad has happened!")
     ...
     ...


Since the great majority of cases *will* have an info, and we take 
careful pains to make sure this info is preserved, it's less invasive to 
just assert that info isn't Optional.

This is especially true for the QAPISchemaMember initializer, which 
several other classes inherit -- if this is allowed to be 
Optional[QAPISourceInfo], any and all users of a QAPISchemaMember or any 
derived classes will have to check -- on every access -- to see if 
'member.info' is set or not.

Since we expect it to be set 100% of the time for all user-defined code, 
it's a lot less "if member.info is not None" checks everywhere.

Adding a special "built-in" source info object to make this claim helps 
avoid making confusing type signatures for the visitors; i.e.

def visit_thing(info: Optional[QAPISourceInfo]): ...

is misleading, because we actually expect thing to *always* have a 
SourceInfo. To make the documentation be a little more ... statistically 
truthful, it's easier to bend the rules in the other direction and just 
fill in the scant few cases where we don't have a QAPISourceInfo.

--js

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 1/13/21 10:39 AM, Markus Armbruster wrote:
>> Spelling nitpick: s/builtin/built-in/ in the title.
>> 
>
> Sure.
>
>> 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.
>>>
>>> This object will evaluate as False, so "if info" remains 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>
>> 
>> As I pointed out in review of v1, this patch has two aspects mixed up:
>> 
>> 1. Represent "no source info" as special QAPISourceInfo instead of
>>     None
>> 
>> 2. On error with "no source info", don't crash.
>> 
>> The first one is what de-complicates interface typing.  It's clearly
>> serving this patch series' stated purpose: "static typing conversion".
>> 
>> The second one is not.  It sidetracks us into a design discussion that
>> isn't related to static typing.  Maybe it's something we should discuss.
>> Maybe the discussion will make us conclude we want to do this.  But
>> letting the static typing work get delayed by that discussion would be
>> stupid, and I'll do what I can to prevent that.
>> 
>
> It's not unrelated. It's about finding the most tactical incision to 
> make the types as we actually use them correct from a static analysis 
> context.
>
> Maybe there's another tactical incision to make that's "smaller", for 
> some perception of "smaller", but it's not unrelated.

We don't have to debate, let alone agree on relatedness.

>> The stupidest possible solution that preserves the crash is adding an
>> assertion right where it crashes before this patch: in
>> QAPISourceInfo.__str__().  Yes, crashing in a __str__() method is not
>> nice, but it's no worse than before.  Making it better than before is a
>> good idea, and you're quite welcome to try, but please not in this
>> series.  Add a TODO comment asking for "make it better", then sit on
>> your hands.
>
> I'm recently back from a fairly long PTO, so forgive me if I am 
> forgetting something, but I am not really sure I fundamentally 
> understand the nature of this critique.
>
> Making functions not "crash" is a side-effect of making the types 
> correct. I don't see it as scope-creep, it's a solution to a problem 
> under active consideration.

I disagree.

The crash you "fix" is *intentional*.  I was too lazy to write something
like

    assert self.info

and instead relied in self.info.whatever to crash.  I don't care how it
crashes, as long as it does crash.

I *like* qapi-gen to crash on such internal errors.  It's easy, and
makes "this is a bug, go report it" perfectly clear.

I'd also be fine with reporting "internal error, this is a bug, go
report it".  Not in this series, unless it's utterly trivial, which I
doubt.

I'm *not* fine with feeding made-up info objects to the user error
reporting machinery without proof that it'll actually produce a useful
error message.  Definitely not trivial, thus not in this series.

> In my reply to your earlier critique, I (think) I mentioned that I 
> didn't understand the difference between:
>
> 1. An exception handler itself crashes because it received a value of 
> unexpected type, or
>
> 2. The exception handler printed a message that indicates a problem with 
> a built-in source definition.
>
> In either case, QAPI didn't get built and it printed some kind of error 
> spaghetti to the screen. In both cases, something much more seriously 
> wrong has happened and the error message likely does not prepare the 
> human user to really genuinely understand what that seriously wrong 
> thing is.
>
> I think this is an on-mission patch that improves circumstances; with 
> regards to matters of taste I would see it as a lateral move at worst 
> (one weird error for another weird error).
>
> I'm left a little confused by the pushback, so I don't feel equipped to 
> try and write code that addresses it.
>
> Let's chat on IRC?

Gladly.  If we can make out work days intersect...

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

[Eduardo Habkost]

On Thu, Jan 14, 2021 at 02:39:35PM +0100, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
> > On 1/13/21 10:39 AM, Markus Armbruster wrote:
> >> Spelling nitpick: s/builtin/built-in/ in the title.
> >> 
> >
> > Sure.
> >
> >> 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.
> >>>
> >>> This object will evaluate as False, so "if info" remains 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>
> >> 
> >> As I pointed out in review of v1, this patch has two aspects mixed up:
> >> 
> >> 1. Represent "no source info" as special QAPISourceInfo instead of
> >>     None
> >> 
> >> 2. On error with "no source info", don't crash.
> >> 
> >> The first one is what de-complicates interface typing.  It's clearly
> >> serving this patch series' stated purpose: "static typing conversion".
> >> 
> >> The second one is not.  It sidetracks us into a design discussion that
> >> isn't related to static typing.  Maybe it's something we should discuss.
> >> Maybe the discussion will make us conclude we want to do this.  But
> >> letting the static typing work get delayed by that discussion would be
> >> stupid, and I'll do what I can to prevent that.
> >> 
> >
> > It's not unrelated. It's about finding the most tactical incision to 
> > make the types as we actually use them correct from a static analysis 
> > context.
> >
> > Maybe there's another tactical incision to make that's "smaller", for 
> > some perception of "smaller", but it's not unrelated.
> 
> We don't have to debate, let alone agree on relatedness.
> 
> >> The stupidest possible solution that preserves the crash is adding an
> >> assertion right where it crashes before this patch: in
> >> QAPISourceInfo.__str__().  Yes, crashing in a __str__() method is not
> >> nice, but it's no worse than before.  Making it better than before is a
> >> good idea, and you're quite welcome to try, but please not in this
> >> series.  Add a TODO comment asking for "make it better", then sit on
> >> your hands.
> >
> > I'm recently back from a fairly long PTO, so forgive me if I am 
> > forgetting something, but I am not really sure I fundamentally 
> > understand the nature of this critique.
> >
> > Making functions not "crash" is a side-effect of making the types 
> > correct. I don't see it as scope-creep, it's a solution to a problem 
> > under active consideration.
> 
> I disagree.
> 
> The crash you "fix" is *intentional*.  I was too lazy to write something
> like
> 
>     assert self.info
> 
> and instead relied in self.info.whatever to crash.  I don't care how it
> crashes, as long as it does crash.
> 
> I *like* qapi-gen to crash on such internal errors.  It's easy, and
> makes "this is a bug, go report it" perfectly clear.
> 
> I'd also be fine with reporting "internal error, this is a bug, go
> report it".  Not in this series, unless it's utterly trivial, which I
> doubt.
> 
> I'm *not* fine with feeding made-up info objects to the user error
> reporting machinery without proof that it'll actually produce a useful
> error message.  Definitely not trivial, thus not in this series.

If you really don't want to change the existing behavior of the
code, I believe we have only two options:

1) Annotate self.info as QAPISourceInfo (not Optional),
   and add a hack to make the expression `self.info` crash if the
   argument to __init__() was None.

2) Annotate self.info as Optional[QAPISourceInfo], and adding
   manual asserts everywhere self.info is used.

Which of those two options do you find acceptable, Markus?

-- 
Eduardo

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

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Thu, Jan 14, 2021 at 02:39:35PM +0100, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>> > On 1/13/21 10:39 AM, Markus Armbruster wrote:
>> >> Spelling nitpick: s/builtin/built-in/ in the title.
>> >> 
>> >
>> > Sure.
>> >
>> >> 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.
>> >>>
>> >>> This object will evaluate as False, so "if info" remains 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>
>> >> 
>> >> As I pointed out in review of v1, this patch has two aspects mixed up:
>> >> 
>> >> 1. Represent "no source info" as special QAPISourceInfo instead of
>> >>     None
>> >> 
>> >> 2. On error with "no source info", don't crash.
>> >> 
>> >> The first one is what de-complicates interface typing.  It's clearly
>> >> serving this patch series' stated purpose: "static typing conversion".
>> >> 
>> >> The second one is not.  It sidetracks us into a design discussion that
>> >> isn't related to static typing.  Maybe it's something we should discuss.
>> >> Maybe the discussion will make us conclude we want to do this.  But
>> >> letting the static typing work get delayed by that discussion would be
>> >> stupid, and I'll do what I can to prevent that.
>> >> 
>> >
>> > It's not unrelated. It's about finding the most tactical incision to 
>> > make the types as we actually use them correct from a static analysis 
>> > context.
>> >
>> > Maybe there's another tactical incision to make that's "smaller", for 
>> > some perception of "smaller", but it's not unrelated.
>> 
>> We don't have to debate, let alone agree on relatedness.
>> 
>> >> The stupidest possible solution that preserves the crash is adding an
>> >> assertion right where it crashes before this patch: in
>> >> QAPISourceInfo.__str__().  Yes, crashing in a __str__() method is not
>> >> nice, but it's no worse than before.  Making it better than before is a
>> >> good idea, and you're quite welcome to try, but please not in this
>> >> series.  Add a TODO comment asking for "make it better", then sit on
>> >> your hands.
>> >
>> > I'm recently back from a fairly long PTO, so forgive me if I am 
>> > forgetting something, but I am not really sure I fundamentally 
>> > understand the nature of this critique.
>> >
>> > Making functions not "crash" is a side-effect of making the types 
>> > correct. I don't see it as scope-creep, it's a solution to a problem 
>> > under active consideration.
>> 
>> I disagree.
>> 
>> The crash you "fix" is *intentional*.  I was too lazy to write something
>> like
>> 
>>     assert self.info
>> 
>> and instead relied in self.info.whatever to crash.  I don't care how it
>> crashes, as long as it does crash.
>> 
>> I *like* qapi-gen to crash on such internal errors.  It's easy, and
>> makes "this is a bug, go report it" perfectly clear.
>> 
>> I'd also be fine with reporting "internal error, this is a bug, go
>> report it".  Not in this series, unless it's utterly trivial, which I
>> doubt.
>> 
>> I'm *not* fine with feeding made-up info objects to the user error
>> reporting machinery without proof that it'll actually produce a useful
>> error message.  Definitely not trivial, thus not in this series.
>
> If you really don't want to change the existing behavior of the
> code, I believe we have only two options:
>
> 1) Annotate self.info as QAPISourceInfo (not Optional),
>    and add a hack to make the expression `self.info` crash if the
>    argument to __init__() was None.

I figure you mean

* Represent "no info" as a special QAPISourceInfo (instead of None), so
  we can annotate .info as QAPISourceInfo (not Optional).

* When we report a QAPIError, assert .info is not this special value.

This preserves the existing (and intentional) behavior: we crash when we
dot into QAPISourceInfo, and we do that only when we report a QAPIError
with that info.

The only change in behavior is AssertionError instead of AttributeError.
Minor improvement.

We could replace the AssertionError crash by a fatal error with suitably
worded error message.  I'd prefer not to, because I'd rather keep the
stack backtrace.  Admittedly not something I'd fight for.

> 2) Annotate self.info as Optional[QAPISourceInfo], and adding
>    manual asserts everywhere self.info is used.
>
> Which of those two options do you find acceptable, Markus?

I think John prefers (1), because the typing gets simpler.  I'm inclined
to leave the decision to him.

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

[Eduardo Habkost]

On Tue, Jan 19, 2021 at 11:21:16AM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Thu, Jan 14, 2021 at 02:39:35PM +0100, Markus Armbruster wrote:
> >> John Snow <jsnow@redhat.com> writes:
> >> 
> >> > On 1/13/21 10:39 AM, Markus Armbruster wrote:
> >> >> Spelling nitpick: s/builtin/built-in/ in the title.
> >> >> 
> >> >
> >> > Sure.
> >> >
> >> >> 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.
> >> >>>
> >> >>> This object will evaluate as False, so "if info" remains 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>
> >> >> 
> >> >> As I pointed out in review of v1, this patch has two aspects mixed up:
> >> >> 
> >> >> 1. Represent "no source info" as special QAPISourceInfo instead of
> >> >>     None
> >> >> 
> >> >> 2. On error with "no source info", don't crash.
> >> >> 
> >> >> The first one is what de-complicates interface typing.  It's clearly
> >> >> serving this patch series' stated purpose: "static typing conversion".
> >> >> 
> >> >> The second one is not.  It sidetracks us into a design discussion that
> >> >> isn't related to static typing.  Maybe it's something we should discuss.
> >> >> Maybe the discussion will make us conclude we want to do this.  But
> >> >> letting the static typing work get delayed by that discussion would be
> >> >> stupid, and I'll do what I can to prevent that.
> >> >> 
> >> >
> >> > It's not unrelated. It's about finding the most tactical incision to 
> >> > make the types as we actually use them correct from a static analysis 
> >> > context.
> >> >
> >> > Maybe there's another tactical incision to make that's "smaller", for 
> >> > some perception of "smaller", but it's not unrelated.
> >> 
> >> We don't have to debate, let alone agree on relatedness.
> >> 
> >> >> The stupidest possible solution that preserves the crash is adding an
> >> >> assertion right where it crashes before this patch: in
> >> >> QAPISourceInfo.__str__().  Yes, crashing in a __str__() method is not
> >> >> nice, but it's no worse than before.  Making it better than before is a
> >> >> good idea, and you're quite welcome to try, but please not in this
> >> >> series.  Add a TODO comment asking for "make it better", then sit on
> >> >> your hands.
> >> >
> >> > I'm recently back from a fairly long PTO, so forgive me if I am 
> >> > forgetting something, but I am not really sure I fundamentally 
> >> > understand the nature of this critique.
> >> >
> >> > Making functions not "crash" is a side-effect of making the types 
> >> > correct. I don't see it as scope-creep, it's a solution to a problem 
> >> > under active consideration.
> >> 
> >> I disagree.
> >> 
> >> The crash you "fix" is *intentional*.  I was too lazy to write something
> >> like
> >> 
> >>     assert self.info
> >> 
> >> and instead relied in self.info.whatever to crash.  I don't care how it
> >> crashes, as long as it does crash.
> >> 
> >> I *like* qapi-gen to crash on such internal errors.  It's easy, and
> >> makes "this is a bug, go report it" perfectly clear.
> >> 
> >> I'd also be fine with reporting "internal error, this is a bug, go
> >> report it".  Not in this series, unless it's utterly trivial, which I
> >> doubt.
> >> 
> >> I'm *not* fine with feeding made-up info objects to the user error
> >> reporting machinery without proof that it'll actually produce a useful
> >> error message.  Definitely not trivial, thus not in this series.
> >
> > If you really don't want to change the existing behavior of the
> > code, I believe we have only two options:
> >
> > 1) Annotate self.info as QAPISourceInfo (not Optional),
> >    and add a hack to make the expression `self.info` crash if the
> >    argument to __init__() was None.
> 
> I figure you mean
> 
> * Represent "no info" as a special QAPISourceInfo (instead of None), so
>   we can annotate .info as QAPISourceInfo (not Optional).
> 
> * When we report a QAPIError, assert .info is not this special value.

Not necessarily.  Creating a special QAPISourceInfo would be one
solution to let us annotate self.info as non-Optional, but not
the only one.

Possibly the simplest way to declare self.info as non-Optional is
to make it a property that hides an Optional attribute.  e.g.:

    class ...:
        _info: Optional[QAPISourceInfo]

        @property
        def info(self) -> QAPISourceInfo:
            assert self._info is not None
            return self._info

> 
> This preserves the existing (and intentional) behavior: we crash when we
> dot into QAPISourceInfo, and we do that only when we report a QAPIError
> with that info.

I'm not sure about the "only when we report a QAPIError" part.
We seem to have multiple places in the code where self.info is
assumed to never be None, and I'm not sure all of them involve
QAPIError.

> 
> The only change in behavior is AssertionError instead of AttributeError.
> Minor improvement.
> 
> We could replace the AssertionError crash by a fatal error with suitably
> worded error message.  I'd prefer not to, because I'd rather keep the
> stack backtrace.  Admittedly not something I'd fight for.
> 
> > 2) Annotate self.info as Optional[QAPISourceInfo], and adding
> >    manual asserts everywhere self.info is used.
> >
> > Which of those two options do you find acceptable, Markus?
> 
> I think John prefers (1), because the typing gets simpler.  I'm inclined
> to leave the decision to him.

-- 
Eduardo