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

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

[John Snow]

Hi, this series adds static type hints to the QAPI module.
This is part four, and focuses on error.py.

Part 4: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt4
CI: https://gitlab.com/jsnow/qemu/-/pipelines/290152364

Requirements:
- Python 3.6+
- mypy >= 0.770
- pylint >= 2.6.0 (2.7.0+ when using Python 3.9+)

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

V3:

```
001/8:[down]      'qapi/error: Repurpose QAPIError as an abstract base exception class'
002/8:[----] [--] 'qapi/error: Use Python3-style super()'
003/8:[----] [--] 'qapi/error: Make QAPISourceError 'col' parameter optional'
004/8:[down]      'qapi/error: assert QAPISourceInfo is not None'
005/8:[0010] [FC] 'qapi/error.py: move QAPIParseError to parser.py'
006/8:[----] [--] 'qapi/error.py: enable pylint checks'
007/8:[----] [-C] 'qapi/error: Add type hints'
008/8:[----] [--] 'qapi/error.py: enable mypy checks'
```

- 01: Updated commit message. (Is it sufficient to pass the sniff test?)
- 04: Changed commit title/message.
- 05: Added a small docstring (I know, hubris again) to add a small
      breadcrumb to the new error location for QAPIParseError.

John Snow (8):
  qapi/error: Repurpose QAPIError as an abstract base exception class
  qapi/error: Use Python3-style super()
  qapi/error: Make QAPISourceError 'col' parameter optional
  qapi/error: assert QAPISourceInfo is not None
  qapi/error.py: move QAPIParseError to parser.py
  qapi/error.py: enable pylint checks
  qapi/error: Add type hints
  qapi/error.py: enable mypy checks

 docs/sphinx/qapidoc.py |  3 ++-
 scripts/qapi/error.py  | 47 ++++++++++++++++++++++++------------------
 scripts/qapi/mypy.ini  |  5 -----
 scripts/qapi/parser.py | 14 ++++++++++++-
 scripts/qapi/pylintrc  |  3 +--
 scripts/qapi/schema.py |  4 ++--
 6 files changed, 45 insertions(+), 31 deletions(-)

-- 
2.30.2

[PATCH v3 1/8] qapi/error: Repurpose QAPIError as an abstract base exception class

[John Snow]

Rename QAPIError to QAPISourceError, and then create a new QAPIError
class that serves as the basis for all of our other custom exceptions,
without specifying any class properties.

This leaves QAPIError as a package-wide error class that's suitable for
any current or future errors.

(Right now, we don't have any errors that DON'T also want to specify a
Source location, but this MAY change. In these cases, a common abstract
ancestor would be desired.)

Add docstrings to explain the intended function of each error class.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py |  3 ++-
 scripts/qapi/error.py  | 11 +++++++++--
 scripts/qapi/schema.py |  5 +++--
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index b7a2d39c105..87c67ab23f8 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -34,7 +34,8 @@
 from sphinx.util.nodes import nested_parse_with_titles
 import sphinx
 from qapi.gen import QAPISchemaVisitor
-from qapi.schema import QAPIError, QAPISemError, QAPISchema
+from qapi.error import QAPIError, QAPISemError
+from qapi.schema import QAPISchema
 
 
 # Sphinx up to 1.6 uses AutodocReporter; 1.7 and later
diff --git a/scripts/qapi/error.py b/scripts/qapi/error.py
index ae60d9e2fe8..126dda7c9b2 100644
--- a/scripts/qapi/error.py
+++ b/scripts/qapi/error.py
@@ -13,6 +13,11 @@
 
 
 class QAPIError(Exception):
+    """Base class for all exceptions from the QAPI package."""
+
+
+class QAPISourceError(QAPIError):
+    """Error class for all exceptions identifying a source location."""
     def __init__(self, info, col, msg):
         Exception.__init__(self)
         self.info = info
@@ -27,7 +32,8 @@ def __str__(self):
         return 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]:
@@ -38,6 +44,7 @@ def __init__(self, parser, msg):
         super().__init__(parser.info, col, msg)
 
 
-class QAPISemError(QAPIError):
+class QAPISemError(QAPISourceError):
+    """Error class for semantic QAPI errors."""
     def __init__(self, info, msg):
         super().__init__(info, None, msg)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 703b446fd21..c277fcacc53 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -20,7 +20,7 @@
 from typing import Optional
 
 from .common import POINTER_SUFFIX, c_name
-from .error import QAPIError, QAPISemError
+from .error import QAPISemError, QAPISourceError
 from .expr import check_exprs
 from .parser import QAPISchemaParser
 
@@ -875,7 +875,8 @@ 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, None,
+                                        "previous definition")
                 raise QAPISemError(
                     ent.info,
                     "'%s' is already defined\n%s" % (ent.name, where))
-- 
2.30.2

[PATCH v3 2/8] qapi/error: Use Python3-style super()

[John Snow]

Missed in commit 2cae67bcb5 "qapi: Use super() now we have Python 3".

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

diff --git a/scripts/qapi/error.py b/scripts/qapi/error.py
index 126dda7c9b2..38bd7c4dd6a 100644
--- a/scripts/qapi/error.py
+++ b/scripts/qapi/error.py
@@ -19,7 +19,7 @@ class QAPIError(Exception):
 class QAPISourceError(QAPIError):
     """Error class for all exceptions identifying a source location."""
     def __init__(self, info, col, msg):
-        Exception.__init__(self)
+        super().__init__()
         self.info = info
         self.col = col
         self.msg = msg
-- 
2.30.2

[PATCH v3 3/8] qapi/error: Make QAPISourceError 'col' parameter optional

[John Snow]

It's already treated as optional, with one direct caller and some
subclass callers passing 'None'. Make it officially optional, which
requires moving the position of the argument to come after all required
parameters.

QAPISemError becomes functionally identical to QAPISourceError. Keep the
name to preserve its semantic meaning and avoid code churn, but remove
the now-useless __init__ wrapper.

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

diff --git a/scripts/qapi/error.py b/scripts/qapi/error.py
index 38bd7c4dd6a..d179a3bd0c7 100644
--- a/scripts/qapi/error.py
+++ b/scripts/qapi/error.py
@@ -18,11 +18,11 @@ class QAPIError(Exception):
 
 class QAPISourceError(QAPIError):
     """Error class for all exceptions identifying a source location."""
-    def __init__(self, info, col, msg):
+    def __init__(self, info, msg, col=None):
         super().__init__()
         self.info = info
-        self.col = col
         self.msg = msg
+        self.col = col
 
     def __str__(self):
         loc = str(self.info)
@@ -41,10 +41,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(QAPISourceError):
     """Error class for semantic QAPI errors."""
-    def __init__(self, info, msg):
-        super().__init__(info, None, msg)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index c277fcacc53..3a4172fb749 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -875,8 +875,7 @@ def _def_entity(self, ent):
         other_ent = self._entity_dict.get(ent.name)
         if other_ent:
             if other_ent.info:
-                where = QAPISourceError(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.30.2

[PATCH v3 4/8] qapi/error: assert QAPISourceInfo is not None

[John Snow]

Built-in stuff is not parsed from a source file, and therefore have no
QAPISourceInfo. If such None info was used for reporting an error,
built-in stuff would be broken. Programming error. Instead of reporting
a confusing error with bogus source location then, we better crash.

We currently crash only if self.col was set. Assert that self.info is
not None in order to crash reliably.

We can not yet change the type of the initializer to prove this cannot
happen at static analysis time before the remainder of the code is fully
typed.

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

diff --git a/scripts/qapi/error.py b/scripts/qapi/error.py
index d179a3bd0c7..d0bc7af6e76 100644
--- a/scripts/qapi/error.py
+++ b/scripts/qapi/error.py
@@ -25,6 +25,7 @@ def __init__(self, info, msg, col=None):
         self.col = col
 
     def __str__(self):
+        assert self.info is not None
         loc = str(self.info)
         if self.col is not None:
             assert self.info.line is not None
-- 
2.30.2

[PATCH v3 5/8] qapi/error.py: move QAPIParseError to parser.py

[John Snow]

Keeping it in error.py will create some cyclic import problems when we
add types to the QAPISchemaParser. Callers don't need to know the
details of QAPIParseError unless they are parsing or dealing directly
with the parser, so this won't create any harsh new requirements for
callers in the general case.

Update error.py with a little docstring that gives a nod to where the
error may now be found.

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

diff --git a/scripts/qapi/error.py b/scripts/qapi/error.py
index d0bc7af6e76..6723c5a9d9a 100644
--- a/scripts/qapi/error.py
+++ b/scripts/qapi/error.py
@@ -1,7 +1,5 @@
 # -*- coding: utf-8 -*-
 #
-# QAPI error classes
-#
 # Copyright (c) 2017-2019 Red Hat Inc.
 #
 # Authors:
@@ -11,6 +9,14 @@
 # This work is licensed under the terms of the GNU GPL, version 2.
 # See the COPYING file in the top-level directory.
 
+"""
+QAPI error classes
+
+Common error classes used throughout the package.  Additional errors may
+be defined in other modules.  At present, `QAPIParseError` is defined in
+parser.py.
+"""
+
 
 class QAPIError(Exception):
     """Base class for all exceptions from the QAPI package."""
@@ -33,17 +39,5 @@ def __str__(self):
         return 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 58267c3db9e..ca5e8e18e00 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -18,10 +18,22 @@
 import os
 import re
 
-from .error import QAPIParseError, QAPISemError
+from .error import QAPISemError, QAPISourceError
 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 QAPISchemaParser:
 
     def __init__(self, fname, previously_included=None, incl_info=None):
-- 
2.30.2

[PATCH v3 6/8] 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 fb0386d529a..88efbf71cb2 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.30.2

[PATCH v3 7/8] qapi/error: Add type hints

[John Snow]

No functional change.

Note: QAPISourceError's info parameter is Optional[] because schema.py
treats the info property of its various classes as Optional to
accommodate built-in types, which have no source. See prior commit
'qapi/error: assert QAPISourceInfo is not None'.

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

---

Random aside: It may appear clunky to have to type __str__, but after I
mentioned it on the Python typing SIG list, it was noted that this is
actually one of very few magic methods that has a "known" type in
advance, so there wasn't much desire to special case the typing for it.

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

diff --git a/scripts/qapi/error.py b/scripts/qapi/error.py
index 6723c5a9d9a..e35e4ddb26a 100644
--- a/scripts/qapi/error.py
+++ b/scripts/qapi/error.py
@@ -17,6 +17,10 @@
 parser.py.
 """
 
+from typing import Optional
+
+from .source import QAPISourceInfo
+
 
 class QAPIError(Exception):
     """Base class for all exceptions from the QAPI package."""
@@ -24,13 +28,16 @@ class QAPIError(Exception):
 
 class QAPISourceError(QAPIError):
     """Error class for all exceptions identifying a source location."""
-    def __init__(self, info, msg, col=None):
+    def __init__(self,
+                 info: Optional[QAPISourceInfo],
+                 msg: str,
+                 col: Optional[int] = None):
         super().__init__()
         self.info = info
         self.msg = msg
         self.col = col
 
-    def __str__(self):
+    def __str__(self) -> str:
         assert self.info is not None
         loc = str(self.info)
         if self.col is not None:
-- 
2.30.2

[PATCH v3 8/8] qapi/error.py: enable mypy checks

[John Snow]

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

---

(This can be squashed with the previous commit when staged.)

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 7797c834328..54ca4483d6d 100644
--- a/scripts/qapi/mypy.ini
+++ b/scripts/qapi/mypy.ini
@@ -3,11 +3,6 @@ strict = True
 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.30.2

Re: [PATCH v3 0/8] qapi: static typing conversion, pt4

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Hi, this series adds static type hints to the QAPI module.
> This is part four, and focuses on error.py.
>
> Part 4: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt4
> CI: https://gitlab.com/jsnow/qemu/-/pipelines/290152364
>
> Requirements:
> - Python 3.6+
> - mypy >= 0.770
> - pylint >= 2.6.0 (2.7.0+ when using Python 3.9+)
>
> Every commit should pass with:
>  - `isort -c qapi/`
>  - `flake8 qapi/`
>  - `pylint --rcfile=qapi/pylintrc qapi/`
>  - `mypy --config-file=qapi/mypy.ini qapi/`

I doubt PATCH 1 is worth the trouble without an actual concrete
location-less error.  However, arguing about it some more would also not
worth the trouble, so:

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

and queued.  Thanks!

Re: [PATCH v3 0/8] qapi: static typing conversion, pt4

[John Snow]

On 4/22/21 4:09 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> Hi, this series adds static type hints to the QAPI module.
>> This is part four, and focuses on error.py.
>>
>> Part 4: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt4
>> CI: https://gitlab.com/jsnow/qemu/-/pipelines/290152364
>>
>> Requirements:
>> - Python 3.6+
>> - mypy >= 0.770
>> - pylint >= 2.6.0 (2.7.0+ when using Python 3.9+)
>>
>> Every commit should pass with:
>>   - `isort -c qapi/`
>>   - `flake8 qapi/`
>>   - `pylint --rcfile=qapi/pylintrc qapi/`
>>   - `mypy --config-file=qapi/mypy.ini qapi/`
> 
> I doubt PATCH 1 is worth the trouble without an actual concrete
> location-less error.  However, arguing about it some more would also not
> worth the trouble, so:
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 
> and queued.  Thanks!
> 

FWIW, I use it a few times in Part 5a:

- Once in main.py, to wrap an OSError (You'll see why)
- Again in qapidoc to create a "QAPIDocError" class, but this is only 
used in PoC code to demonstrate that QAPISchemaParser is fully typed.

I am finding it handy, though it may be true we wind up not needing it, 
based on reviews in part 5.

Thanks for queueing it up!

--js

(Why does tbird spellcheck not think 'queueing' is a word?)