[PATCH 0/6] qapi: static typing conversion, pt4

[PATCH 0/6] qapi: static typing conversion, pt4

[John Snow]

based-on: <20200922211802.4083666-1-jsnow@redhat.com>
          [PATCH 00/14] qapi: static typing conversion, pt3

Hi, this series adds static type hints to the QAPI module.
This is part four!

Part 4: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt4
Everything: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt6

- Requires Python 3.6+
- Requires mypy 0.770 or newer (for type analysis only)
- Requires pylint 2.6.0 or newer (for lint checking only)

This part of the series focuses on error.py.

Type hints are added in patches that add *only* type hints and change no
other behavior. Any necessary changes to behavior to accommodate typing
are split out into their own tiny patches.

Every commit should pass with:
 - flake8 qapi/
 - pylint --rcfile=qapi/pylintrc qapi/
 - mypy --config-file=qapi/mypy.ini qapi/

John Snow (6):
  qapi/error.py: refactor error classes
  qapi/parser.py: remove parser context from QAPIDoc
  qapi/error.py: move QAPIParseError to parser.py
  qapi/parser.py: refactor QAPIParseError
  qapi/error.py: enable mypy checks
  qapi/error.py: enable pylint checks

 scripts/qapi/error.py  |  41 +++++++-------
 scripts/qapi/main.py   |   2 +-
 scripts/qapi/mypy.ini  |   5 --
 scripts/qapi/parser.py | 120 ++++++++++++++++++++++++-----------------
 scripts/qapi/pylintrc  |   3 +-
 scripts/qapi/schema.py |   4 +-
 6 files changed, 94 insertions(+), 81 deletions(-)

-- 
2.26.2

[PATCH 1/6] qapi/error.py: refactor error classes

[John Snow]

Create a ubiquitous, context-free base error class that all exceptions
in the qapi package should inherit from.

Move the QAPISourceInfo relevant fields up into QAPIErrorLocation;
making sure the order of arguments is consistent between
QAPIErrorLocation and QAPISemError.

---

The order of arguments for QAPIParseError is inconsistent, but handled
explicitly in the __init__ method; this will be addressed in forthcoming
patches, but it works correctly here.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/error.py  | 35 +++++++++++++++++++++++------------
 scripts/qapi/main.py   |  2 +-
 scripts/qapi/schema.py |  4 ++--
 3 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/scripts/qapi/error.py b/scripts/qapi/error.py
index ae60d9e2fe..47f266f33d 100644
--- a/scripts/qapi/error.py
+++ b/scripts/qapi/error.py
@@ -11,23 +11,35 @@
 # This work is licensed under the terms of the GNU GPL, version 2.
 # See the COPYING file in the top-level directory.
 
+from typing import Optional
+
+from .source import QAPISourceInfo
+
 
 class QAPIError(Exception):
-    def __init__(self, info, col, msg):
-        Exception.__init__(self)
+    """Base class for all exceptions from the QAPI module."""
+
+
+class QAPISourceError(QAPIError):
+    """Error class for all exceptions identifying a source location."""
+    def __init__(self,
+                 info: QAPISourceInfo,
+                 msg: str,
+                 col: Optional[int] = None):
+        super().__init__()
         self.info = info
-        self.col = col
         self.msg = msg
+        self.col = col
 
-    def __str__(self):
+    def __str__(self) -> str:
         loc = str(self.info)
         if self.col is not None:
-            assert self.info.line is not None
-            loc += ':%s' % self.col
-        return loc + ': ' + self.msg
+            loc += f":{self.col}"
+        return f"{loc}: {self.msg}"
 
 
-class QAPIParseError(QAPIError):
+class QAPIParseError(QAPISourceError):
+    """Error class for all QAPI schema parsing errors."""
     def __init__(self, parser, msg):
         col = 1
         for ch in parser.src[parser.line_pos:parser.pos]:
@@ -35,9 +47,8 @@ def __init__(self, parser, msg):
                 col = (col + 7) % 8 + 1
             else:
                 col += 1
-        super().__init__(parser.info, col, msg)
+        super().__init__(parser.info, msg, col)
 
 
-class QAPISemError(QAPIError):
-    def __init__(self, info, msg):
-        super().__init__(info, None, msg)
+class QAPISemError(QAPISourceError):
+    """Error class for semantic QAPI errors."""
diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
index 3f8338ade8..302b9eee81 100644
--- a/scripts/qapi/main.py
+++ b/scripts/qapi/main.py
@@ -45,7 +45,7 @@ def generate(schema_file: str,
     if match and match.end() != len(prefix):
         msg = "funny character '{:s}' in prefix '{:s}'".format(
             prefix[match.end()], prefix)
-        raise QAPIError('', None, msg)
+        raise QAPIError(msg)
 
     schema = QAPISchema(schema_file)
     gen_types(schema, output_dir, prefix, builtins)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 3023bab44b..121d8488d2 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -20,7 +20,7 @@
 from typing import Optional
 
 from .common import c_name, POINTER_SUFFIX
-from .error import QAPIError, QAPISemError
+from .error import QAPISourceError, QAPISemError
 from .expr import check_exprs
 from .parser import QAPISchemaParser
 
@@ -841,7 +841,7 @@ def _def_entity(self, ent):
         other_ent = self._entity_dict.get(ent.name)
         if other_ent:
             if other_ent.info:
-                where = QAPIError(other_ent.info, None, "previous definition")
+                where = QAPISourceError(other_ent.info, "previous definition")
                 raise QAPISemError(
                     ent.info,
                     "'%s' is already defined\n%s" % (ent.name, where))
-- 
2.26.2

[PATCH 2/6] qapi/parser.py: remove parser context from QAPIDoc

[John Snow]

We only need the parser context for the purpose of raising
exceptions. What we can do instead is to raise a unique document parsing
error and affix additional context from the caller. This way, the
document parsing can be isolated.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/parser.py | 56 ++++++++++++++++++++++--------------------
 1 file changed, 30 insertions(+), 26 deletions(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 327cf05736..cb2595ce60 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -18,10 +18,14 @@
 import re
 from collections import OrderedDict
 
-from .error import QAPIParseError, QAPISemError
+from .error import QAPIError, QAPIParseError, QAPISemError
 from .source import QAPISourceInfo
 
 
+class QAPIDocError(QAPIError):
+    """Documentation parsing error."""
+
+
 class QAPISchemaParser:
 
     def __init__(self, fname, previously_included=None, incl_info=None):
@@ -265,13 +269,13 @@ def get_expr(self, nested):
                 self, "expected '{', '[', string, boolean or 'null'")
         return expr
 
-    def get_doc(self, info):
+    def _get_doc(self, info):
         if self.val != '##':
             raise QAPIParseError(
                 self, "junk after '##' at start of documentation comment")
 
         docs = []
-        cur_doc = QAPIDoc(self, info)
+        cur_doc = QAPIDoc(info)
         self.accept(False)
         while self.tok == '#':
             if self.val.startswith('##'):
@@ -292,12 +296,22 @@ def get_doc(self, info):
                 if cur_doc.body.text:
                     cur_doc.end_comment()
                     docs.append(cur_doc)
-                    cur_doc = QAPIDoc(self, info)
+                    cur_doc = QAPIDoc(info)
             cur_doc.append(self.val)
             self.accept(False)
 
         raise QAPIParseError(self, "documentation comment must end with '##'")
 
+    def get_doc(self, info):
+        try:
+            return self._get_doc(info)
+        except QAPIDocError as err:
+            # Tie the Doc parsing error to our parsing state. The
+            # resulting error position depends on the state of the
+            # parser. It happens to be the beginning of the comment.
+            # More or less servicable, but action at a distance.
+            raise QAPIParseError(self, str(err)) from err
+
 
 class QAPIDoc:
     """
@@ -335,12 +349,7 @@ def __init__(self, name):
         def connect(self, member):
             self.member = member
 
-    def __init__(self, parser, info):
-        # self._parser is used to report errors with QAPIParseError.  The
-        # resulting error position depends on the state of the parser.
-        # It happens to be the beginning of the comment.  More or less
-        # servicable, but action at a distance.
-        self._parser = parser
+    def __init__(self, info):
         self.info = info
         self.symbol = None
         self.body = QAPIDoc.Section()
@@ -377,7 +386,7 @@ def append(self, line):
             return
 
         if line[0] != ' ':
-            raise QAPIParseError(self._parser, "missing space after #")
+            raise QAPIDocError("missing space after #")
         line = line[1:]
         self._append_line(line)
 
@@ -411,11 +420,11 @@ def _append_body_line(self, line):
         # recognized, and get silently treated as ordinary text
         if not self.symbol and not self.body.text and line.startswith('@'):
             if not line.endswith(':'):
-                raise QAPIParseError(self._parser, "line should end with ':'")
+                raise QAPIDocError("line should end with ':'")
             self.symbol = line[1:-1]
             # FIXME invalid names other than the empty string aren't flagged
             if not self.symbol:
-                raise QAPIParseError(self._parser, "invalid name")
+                raise QAPIDocError("invalid name")
         elif self.symbol:
             # This is a definition documentation block
             if name.startswith('@') and name.endswith(':'):
@@ -498,9 +507,8 @@ def _append_various_line(self, line):
         name = line.split(' ', 1)[0]
 
         if name.startswith('@') and name.endswith(':'):
-            raise QAPIParseError(self._parser,
-                                 "'%s' can't follow '%s' section"
-                                 % (name, self.sections[0].name))
+            raise QAPIDocError("'%s' can't follow '%s' section"
+                               % (name, self.sections[0].name))
         if self._is_section_tag(name):
             line = line[len(name)+1:]
             self._start_section(name[:-1])
@@ -514,10 +522,9 @@ def _append_various_line(self, line):
     def _start_symbol_section(self, symbols_dict, name):
         # FIXME invalid names other than the empty string aren't flagged
         if not name:
-            raise QAPIParseError(self._parser, "invalid parameter name")
+            raise QAPIDocError("invalid parameter name")
         if name in symbols_dict:
-            raise QAPIParseError(self._parser,
-                                 "'%s' parameter name duplicated" % name)
+            raise QAPIDocError("'%s' parameter name duplicated" % name)
         assert not self.sections
         self._end_section()
         self._section = QAPIDoc.ArgSection(name)
@@ -531,8 +538,7 @@ def _start_features_section(self, name):
 
     def _start_section(self, name=None):
         if name in ('Returns', 'Since') and self.has_section(name):
-            raise QAPIParseError(self._parser,
-                                 "duplicated '%s' section" % name)
+            raise QAPIDocError("duplicated '%s' section" % name)
         self._end_section()
         self._section = QAPIDoc.Section(name)
         self.sections.append(self._section)
@@ -541,17 +547,15 @@ def _end_section(self):
         if self._section:
             text = self._section.text = self._section.text.strip()
             if self._section.name and (not text or text.isspace()):
-                raise QAPIParseError(
-                    self._parser,
+                raise QAPIDocError(
                     "empty doc section '%s'" % self._section.name)
             self._section = None
 
     def _append_freeform(self, line):
         match = re.match(r'(@\S+:)', line)
         if match:
-            raise QAPIParseError(self._parser,
-                                 "'%s' not allowed in free-form documentation"
-                                 % match.group(1))
+            raise QAPIDocError("'%s' not allowed in free-form documentation"
+                               % match.group(1))
         self._section.append(line)
 
     def connect_member(self, member):
-- 
2.26.2

[PATCH 3/6] qapi/error.py: move QAPIParseError to parser.py

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/error.py  | 12 ------------
 scripts/qapi/parser.py | 14 +++++++++++++-
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/scripts/qapi/error.py b/scripts/qapi/error.py
index 47f266f33d..ab6a0f6271 100644
--- a/scripts/qapi/error.py
+++ b/scripts/qapi/error.py
@@ -38,17 +38,5 @@ def __str__(self) -> str:
         return f"{loc}: {self.msg}"
 
 
-class QAPIParseError(QAPISourceError):
-    """Error class for all QAPI schema parsing errors."""
-    def __init__(self, parser, msg):
-        col = 1
-        for ch in parser.src[parser.line_pos:parser.pos]:
-            if ch == '\t':
-                col = (col + 7) % 8 + 1
-            else:
-                col += 1
-        super().__init__(parser.info, msg, col)
-
-
 class QAPISemError(QAPISourceError):
     """Error class for semantic QAPI errors."""
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index cb2595ce60..7e7eda9ed9 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -18,10 +18,22 @@
 import re
 from collections import OrderedDict
 
-from .error import QAPIError, QAPIParseError, QAPISemError
+from .error import QAPIError, QAPISourceError, QAPISemError
 from .source import QAPISourceInfo
 
 
+class QAPIParseError(QAPISourceError):
+    """Error class for all QAPI schema parsing errors."""
+    def __init__(self, parser, msg):
+        col = 1
+        for ch in parser.src[parser.line_pos:parser.pos]:
+            if ch == '\t':
+                col = (col + 7) % 8 + 1
+            else:
+                col += 1
+        super().__init__(parser.info, msg, col)
+
+
 class QAPIDocError(QAPIError):
     """Documentation parsing error."""
 
-- 
2.26.2

[PATCH 4/6] qapi/parser.py: refactor QAPIParseError

[John Snow]

Instead of rewriting the constructor with arguments incompatible to the
base class we're extending, add a constructor-like class method that
accepts a QAPISchemaParser as an argument and builds the correct
Exception type.

Because this is a little extra typing, create a new helper in
QAPISchemaParser to build these exception objects.

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 7e7eda9ed9..8be4570c31 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -17,6 +17,7 @@
 import os
 import re
 from collections import OrderedDict
+from typing import Type, TypeVar
 
 from .error import QAPIError, QAPISourceError, QAPISemError
 from .source import QAPISourceInfo
@@ -24,14 +25,18 @@
 
 class QAPIParseError(QAPISourceError):
     """Error class for all QAPI schema parsing errors."""
-    def __init__(self, parser, msg):
+
+    T = TypeVar('T', bound='QAPIParseError')
+
+    @classmethod
+    def make(cls: Type[T], parser: 'QAPISchemaParser', msg: str) -> T:
         col = 1
         for ch in parser.src[parser.line_pos:parser.pos]:
             if ch == '\t':
                 col = (col + 7) % 8 + 1
             else:
                 col += 1
-        super().__init__(parser.info, msg, col)
+        return cls(parser.info, msg, col)
 
 
 class QAPIDocError(QAPIError):
@@ -112,6 +117,9 @@ def __init__(self, fname, previously_included=None, incl_info=None):
             cur_doc = None
         self.reject_expr_doc(cur_doc)
 
+    def _parse_error(self, msg: str) -> QAPIParseError:
+        return QAPIParseError.make(self, msg)
+
     @staticmethod
     def reject_expr_doc(doc):
         if doc and doc.symbol:
@@ -183,13 +191,12 @@ def accept(self, skip_comment=True):
                     ch = self.src[self.cursor]
                     self.cursor += 1
                     if ch == '\n':
-                        raise QAPIParseError(self, "missing terminating \"'\"")
+                        raise self._parse_error("missing terminating \"'\"")
                     if esc:
                         # Note: we recognize only \\ because we have
                         # no use for funny characters in strings
                         if ch != '\\':
-                            raise QAPIParseError(self,
-                                                 "unknown escape \\%s" % ch)
+                            raise self._parse_error(f"unknown escape \\{ch}")
                         esc = False
                     elif ch == '\\':
                         esc = True
@@ -198,8 +205,7 @@ def accept(self, skip_comment=True):
                         self.val = string
                         return
                     if ord(ch) < 32 or ord(ch) >= 127:
-                        raise QAPIParseError(
-                            self, "funny character in string")
+                        raise self._parse_error("funny character in string")
                     string += ch
             elif self.src.startswith('true', self.pos):
                 self.val = True
@@ -220,7 +226,7 @@ def accept(self, skip_comment=True):
                 # character
                 match = re.match('[^[\\]{}:,\\s\'"]+',
                                  self.src[self.cursor-1:])
-                raise QAPIParseError(self, "stray '%s'" % match.group(0))
+                raise self._parse_error("stray '%s'" % match.group(0))
 
     def get_members(self):
         expr = OrderedDict()
@@ -228,24 +234,24 @@ def get_members(self):
             self.accept()
             return expr
         if self.tok != "'":
-            raise QAPIParseError(self, "expected string or '}'")
+            raise self._parse_error("expected string or '}'")
         while True:
             key = self.val
             self.accept()
             if self.tok != ':':
-                raise QAPIParseError(self, "expected ':'")
+                raise self._parse_error("expected ':'")
             self.accept()
             if key in expr:
-                raise QAPIParseError(self, "duplicate key '%s'" % key)
+                raise self._parse_error("duplicate key '%s'" % key)
             expr[key] = self.get_expr(True)
             if self.tok == '}':
                 self.accept()
                 return expr
             if self.tok != ',':
-                raise QAPIParseError(self, "expected ',' or '}'")
+                raise self._parse_error("expected ',' or '}'")
             self.accept()
             if self.tok != "'":
-                raise QAPIParseError(self, "expected string")
+                raise self._parse_error("expected string")
 
     def get_values(self):
         expr = []
@@ -253,20 +259,20 @@ def get_values(self):
             self.accept()
             return expr
         if self.tok not in "{['tfn":
-            raise QAPIParseError(
-                self, "expected '{', '[', ']', string, boolean or 'null'")
+            raise self._parse_error(
+                "expected '{', '[', ']', string, boolean or 'null'")
         while True:
             expr.append(self.get_expr(True))
             if self.tok == ']':
                 self.accept()
                 return expr
             if self.tok != ',':
-                raise QAPIParseError(self, "expected ',' or ']'")
+                raise self._parse_error("expected ',' or ']'")
             self.accept()
 
     def get_expr(self, nested):
         if self.tok != '{' and not nested:
-            raise QAPIParseError(self, "expected '{'")
+            raise self._parse_error("expected '{'")
         if self.tok == '{':
             self.accept()
             expr = self.get_members()
@@ -277,14 +283,14 @@ def get_expr(self, nested):
             expr = self.val
             self.accept()
         else:
-            raise QAPIParseError(
-                self, "expected '{', '[', string, boolean or 'null'")
+            raise self._parse_error(
+                "expected '{', '[', string, boolean or 'null'")
         return expr
 
     def _get_doc(self, info):
         if self.val != '##':
-            raise QAPIParseError(
-                self, "junk after '##' at start of documentation comment")
+            raise self._parse_error(
+                "junk after '##' at start of documentation comment")
 
         docs = []
         cur_doc = QAPIDoc(info)
@@ -293,8 +299,7 @@ def _get_doc(self, info):
             if self.val.startswith('##'):
                 # End of doc comment
                 if self.val != '##':
-                    raise QAPIParseError(
-                        self,
+                    raise self._parse_error(
                         "junk after '##' at end of documentation comment")
                 cur_doc.end_comment()
                 docs.append(cur_doc)
@@ -302,8 +307,7 @@ def _get_doc(self, info):
                 return docs
             if self.val.startswith('# ='):
                 if cur_doc.symbol:
-                    raise QAPIParseError(
-                        self,
+                    raise self._parse_error(
                         "unexpected '=' markup in definition documentation")
                 if cur_doc.body.text:
                     cur_doc.end_comment()
@@ -312,7 +316,7 @@ def _get_doc(self, info):
             cur_doc.append(self.val)
             self.accept(False)
 
-        raise QAPIParseError(self, "documentation comment must end with '##'")
+        raise self._parse_error("documentation comment must end with '##'")
 
     def get_doc(self, info):
         try:
@@ -322,7 +326,7 @@ def get_doc(self, info):
             # resulting error position depends on the state of the
             # parser. It happens to be the beginning of the comment.
             # More or less servicable, but action at a distance.
-            raise QAPIParseError(self, str(err)) from err
+            raise self._parse_error(str(err)) from err
 
 
 class QAPIDoc:
-- 
2.26.2

[PATCH 5/6] qapi/error.py: enable mypy checks

[John Snow]

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

diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
index df9b05e4ab..4d341c6b2d 100644
--- a/scripts/qapi/mypy.ini
+++ b/scripts/qapi/mypy.ini
@@ -4,11 +4,6 @@ strict_optional = False
 disallow_untyped_calls = False
 python_version = 3.6
 
-[mypy-qapi.error]
-disallow_untyped_defs = False
-disallow_incomplete_defs = False
-check_untyped_defs = False
-
 [mypy-qapi.parser]
 disallow_untyped_defs = False
 disallow_incomplete_defs = False
-- 
2.26.2

[PATCH 6/6] qapi/error.py: enable pylint checks

[John Snow]

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

diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
index fb0386d529..88efbf71cb 100644
--- a/scripts/qapi/pylintrc
+++ b/scripts/qapi/pylintrc
@@ -2,8 +2,7 @@
 
 # Add files or directories matching the regex patterns to the ignore list.
 # The regex matches against base names, not paths.
-ignore-patterns=error.py,
-                parser.py,
+ignore-patterns=parser.py,
                 schema.py,
 
 
-- 
2.26.2