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

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

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/

John Snow (8):
  qapi/error: Repurpose QAPIError as a generic exception base class
  qapi/error: Use Python3-style super()
  qapi/error: Make QAPISourceError 'col' parameter optional
  qapi/error: Change assertion
  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  | 37 +++++++++++++++++++------------------
 scripts/qapi/mypy.ini  |  5 -----
 scripts/qapi/parser.py | 14 +++++++++++++-
 scripts/qapi/pylintrc  |  3 +--
 scripts/qapi/schema.py |  4 ++--
 6 files changed, 37 insertions(+), 29 deletions(-)

-- 
2.30.2

[PATCH v2 1/8] qapi/error: Repurpose QAPIError as a generic exception base 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.

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 b7b86b5dff..458b1f477e 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 ae60d9e2fe..126dda7c9b 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 01cdd753cd..1849c3e45f 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
 
@@ -878,7 +878,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 v2 2/8] qapi/error: Use Python3-style super()

[John Snow]

Signed-off-by: John Snow <jsnow@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 126dda7c9b..38bd7c4dd6 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 v2 3/8] qapi/error: Make QAPISourceError 'col' parameter optional

[John Snow]

It's already treated as optional, with some callers and subclasses
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 38bd7c4dd6..d179a3bd0c 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 1849c3e45f..dcf8a86139 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -878,8 +878,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 v2 4/8] qapi/error: Change assertion

[John Snow]

Eventually, we'll be able to prove that 'info.line' must be an int and
is never None at static analysis time, and this assert can go
away. Until then, it's a type error to assume that self.info is not
None.

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 d179a3bd0c..d0bc7af6e7 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 v2 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.

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 d0bc7af6e7..2183b8c6b7 100644
--- a/scripts/qapi/error.py
+++ b/scripts/qapi/error.py
@@ -33,17 +33,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 d5bf91f2b0..140f582ad9 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 v2 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 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.30.2

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

[John Snow]

No functional change.

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 2183b8c6b7..6ba54821c9 100644
--- a/scripts/qapi/error.py
+++ b/scripts/qapi/error.py
@@ -11,6 +11,10 @@
 # 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):
     """Base class for all exceptions from the QAPI package."""
@@ -18,13 +22,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 v2 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 7797c83432..54ca4483d6 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 v2 4/8] qapi/error: Change assertion

[John Snow]

On 3/30/21 1:18 PM, John Snow wrote:

Realizing now that this commit topic is wrong :)

A prior version modified the assertion, I decided it was less churn to 
simply add one.

I think ideally we'd have no assertions here and we'd rely on the type 
hints, but I don't think I can prove that this is safe until after part 
6, so I did this for now instead.

> Eventually, we'll be able to prove that 'info.line' must be an int and
> is never None at static analysis time, and this assert can go
> away. Until then, it's a type error to assume that self.info is not
> None.
> 
> 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 d179a3bd0c..d0bc7af6e7 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
> 

Re: [PATCH v2 1/8] qapi/error: Repurpose QAPIError as a generic exception base class

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

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

Isn't the existing QAPIError such a base class already?  Peeking
ahead...  aha, your new base class is abstract.  Can you explain why we
that's useful?

> 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 b7b86b5dff..458b1f477e 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 ae60d9e2fe..126dda7c9b 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 01cdd753cd..1849c3e45f 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
>  
> @@ -878,7 +878,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))

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Signed-off-by: John Snow <jsnow@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 126dda7c9b..38bd7c4dd6 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

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

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

Re: [PATCH v2 4/8] qapi/error: Change assertion

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 3/30/21 1:18 PM, John Snow wrote:
>
> Realizing now that this commit topic is wrong :)
>
> A prior version modified the assertion, I decided it was less churn to 
> simply add one.
>
> I think ideally we'd have no assertions here and we'd rely on the type 
> hints, but I don't think I can prove that this is safe until after part 
> 6, so I did this for now instead.
>
>> Eventually, we'll be able to prove that 'info.line' must be an int and
>> is never None at static analysis time, and this assert can go
>> away. Until then, it's a type error to assume that self.info is not
>> None.
>> 
>> 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 d179a3bd0c..d0bc7af6e7 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
>> 

Please show us the revised commit message.  I'm asking because the
patch's purpose isn't quite clear to me.  Peeking ahead at PATCH 7, I
see that info becomes Optional[QAPISourceInfo].  This means something
passes None for info (else you wouldn't make it optional).  None info
Works (in the sense of "doesn't crash") as long as col is also None.
After the patch, it doesn't.  I'm not saying that's bad, only that I
don't quite understand what you're trying to accomplish here.

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> No functional change.
>
> 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 2183b8c6b7..6ba54821c9 100644
> --- a/scripts/qapi/error.py
> +++ b/scripts/qapi/error.py
> @@ -11,6 +11,10 @@
>  # 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):
>      """Base class for all exceptions from the QAPI package."""
> @@ -18,13 +22,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],

The Optional is a bit surprising.  Mind pointing to the / a reason in
the commit message?

> +                 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:

Re: [PATCH v2 1/8] qapi/error: Repurpose QAPIError as a generic exception base class

[John Snow]

On 4/15/21 2:44 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> Rename QAPIError to QAPISourceError, and then create a new QAPIError
>> class that serves as the basis for all of our other custom exceptions.
> 
> Isn't the existing QAPIError such a base class already?  Peeking
> ahead...  aha, your new base class is abstract.  Can you explain why we
> that's useful?
> 

Sure. The existing QAPIError class assumes that it's an error that has a 
source location, but not all errors will. The idea is that an abstract 
error class can be used as the ancestor for all other errors in a 
type-safe way, such that:

try:
     qapi.something_or_other()
except QAPIError as err:
     err.some_sort_of_method()

(1) This is a type-safe construct, and
(2) This is sufficient to catch all errors that originate from within 
the library, regardless of their exact type.

Primarily, it's a ploy to get the SourceInfo stuff out of the 
common/root exception for type safety reasons.

(Later in the series, you'll see there's a few places where we try to 
fake SourceInfo stuff in order to use QAPIError, by shuffling this 
around, we allow ourselves to raise exceptions that don't need this 
hackery.)

>> 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 b7b86b5dff..458b1f477e 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 ae60d9e2fe..126dda7c9b 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 01cdd753cd..1849c3e45f 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
>>   
>> @@ -878,7 +878,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))

Re: [PATCH v2 4/8] qapi/error: Change assertion

[John Snow]

On 4/15/21 3:00 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 3/30/21 1:18 PM, John Snow wrote:
>>
>> Realizing now that this commit topic is wrong :)
>>
>> A prior version modified the assertion, I decided it was less churn to
>> simply add one.
>>
>> I think ideally we'd have no assertions here and we'd rely on the type
>> hints, but I don't think I can prove that this is safe until after part
>> 6, so I did this for now instead.
>>
>>> Eventually, we'll be able to prove that 'info.line' must be an int and
>>> is never None at static analysis time, and this assert can go
>>> away. Until then, it's a type error to assume that self.info is not
>>> None.
>>>
>>> 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 d179a3bd0c..d0bc7af6e7 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
>>>
> 
> Please show us the revised commit message.  I'm asking because the
> patch's purpose isn't quite clear to me.  Peeking ahead at PATCH 7, I
> see that info becomes Optional[QAPISourceInfo].  This means something
> passes None for info (else you wouldn't make it optional).  None info
> Works (in the sense of "doesn't crash") as long as col is also None.
> After the patch, it doesn't.  I'm not saying that's bad, only that I
> don't quite understand what you're trying to accomplish here.
> 

Sure.

If you recall, I tried to enforce that QAPISourceInfo was *never* None 
by creating a special value for QSI that represented "No Source Info". 
Ultimately, that idea didn't go through and we solidified that the 
'info' parameter that gets passed around can sometimes be None.

Hence, this property is Optional[QAPISourceInfo].

Since I know you wanted to crash messily in the case that we allowed a 
None-info to leak into a context like this, I added the new assertion to 
make sure this remains the case.

(since str(None) evaluates to "None", it seemed like there was already 
the implicit assumption that info would never be None. Plus, if 'col' is 
set, mypy notices that we are performing an unsafe check on 
self.info.line, which had to be remedied.)

Later in the series, after schema.py is typed, it may be possible to 
remove these assertions as we may be able to rely on the strict typing 
to prove that this situation can never occur. However, since schema.py 
is not yet typed, we can't yet.



Alright. So given that, I think what you'd like to see for a commit 
message is:

qapi/error: assert QAPISourceInfo is not None

We implicitly assume that self.info will never be None, as the error 
reporting function will not produce meaningful output in this case, and 
will crash if self.col was set. Assert that self.info is not None in 
order to formalize this assumption.

We can not yet change the type of the initializer to prove that this is 
provably true at static analysis time until the rest of this library is 
fully typed.


--js

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

[John Snow]

On 4/15/21 3:15 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> No functional change.
>>
>> 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 2183b8c6b7..6ba54821c9 100644
>> --- a/scripts/qapi/error.py
>> +++ b/scripts/qapi/error.py
>> @@ -11,6 +11,10 @@
>>   # 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):
>>       """Base class for all exceptions from the QAPI package."""
>> @@ -18,13 +22,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],
> 
> The Optional is a bit surprising.  Mind pointing to the / a reason in
> the commit message?
> 

We don't have a special object representing "No Source Info", so 
schema.py passes around None objects to mean the same.

The problem, ultimately, is that a QAPISchemaEntity may or may not have 
a source info; and various error reporting paths that in practice will 
not involve such a type in its reporting cannot distinguish that using 
the type system, because from the root-down, info is codified as 
Optional[QAPISourceInfo].

There might be a way to introduce a constraint in schema.py later on, 
but for now (prior to part 6, especially), we cannot.

The assertion should help remind a reader that we don't expect it to be 
true.

--js

>> +                 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:

Re: [PATCH v2 1/8] qapi/error: Repurpose QAPIError as a generic exception base class

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 4/15/21 2:44 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> Rename QAPIError to QAPISourceError, and then create a new QAPIError
>>> class that serves as the basis for all of our other custom exceptions.
>> 
>> Isn't the existing QAPIError such a base class already?  Peeking
>> ahead...  aha, your new base class is abstract.  Can you explain why we
>> that's useful?
>> 
>
> Sure. The existing QAPIError class assumes that it's an error that has a 
> source location, but not all errors will. The idea is that an abstract 
> error class can be used as the ancestor for all other errors in a 
> type-safe way, such that:
>
> try:
>      qapi.something_or_other()
> except QAPIError as err:
>      err.some_sort_of_method()
>
> (1) This is a type-safe construct, and
> (2) This is sufficient to catch all errors that originate from within 
> the library, regardless of their exact type.
>
> Primarily, it's a ploy to get the SourceInfo stuff out of the 
> common/root exception for type safety reasons.
>
> (Later in the series, you'll see there's a few places where we try to 
> fake SourceInfo stuff in order to use QAPIError, by shuffling this 
> around, we allow ourselves to raise exceptions that don't need this 
> hackery.)

I think you're conflating a real issue with a non-issue.

The real issue: you want to get rid of fake QAPISourceInfo.  This isn't
terribly important, but small cleanups count, too.  I can't see the "few
places where we try to fake" in this series, though.  Is it in a later
part, or am I just blind?

The non-issue: wanting a common base class.  Yes, we want one, but we
already got one: QAPIError.

I think the commit message should explain the real issue more clearly,
without confusing readers with the non-issue.

Makes sense?

Re: [PATCH v2 4/8] qapi/error: Change assertion

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 4/15/21 3:00 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> On 3/30/21 1:18 PM, John Snow wrote:
>>>
>>> Realizing now that this commit topic is wrong :)
>>>
>>> A prior version modified the assertion, I decided it was less churn to
>>> simply add one.
>>>
>>> I think ideally we'd have no assertions here and we'd rely on the type
>>> hints, but I don't think I can prove that this is safe until after part
>>> 6, so I did this for now instead.
>>>
>>>> Eventually, we'll be able to prove that 'info.line' must be an int and
>>>> is never None at static analysis time, and this assert can go
>>>> away. Until then, it's a type error to assume that self.info is not
>>>> None.
>>>>
>>>> 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 d179a3bd0c..d0bc7af6e7 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
>>>>
>> Please show us the revised commit message.  I'm asking because the
>> patch's purpose isn't quite clear to me.  Peeking ahead at PATCH 7, I
>> see that info becomes Optional[QAPISourceInfo].  This means something
>> passes None for info (else you wouldn't make it optional).  None info
>> Works (in the sense of "doesn't crash") as long as col is also None.
>> After the patch, it doesn't.  I'm not saying that's bad, only that I
>> don't quite understand what you're trying to accomplish here.
>> 
>
> Sure.
>
> If you recall, I tried to enforce that QAPISourceInfo was *never* None
> by creating a special value for QSI that represented "No Source
> Info". Ultimately, that idea didn't go through and we solidified that
> the 'info' parameter that gets passed around can sometimes be None.
>
> Hence, this property is Optional[QAPISourceInfo].
>
> Since I know you wanted to crash messily in the case that we allowed a
> None-info to leak into a context like this, I added the new assertion
> to make sure this remains the case.
>
> (since str(None) evaluates to "None", it seemed like there was already
> the implicit assumption that info would never be None. Plus, if 'col'
> is set, mypy notices that we are performing an unsafe check on 
> self.info.line, which had to be remedied.)
>
> Later in the series, after schema.py is typed, it may be possible to
> remove these assertions as we may be able to rely on the strict typing 
> to prove that this situation can never occur. However, since schema.py
> is not yet typed, we can't yet.
>
>
>
> Alright. So given that, I think what you'd like to see for a commit
> message is:
>
> qapi/error: assert QAPISourceInfo is not None
>
> We implicitly assume that self.info will never be None, as the error
> reporting function will not produce meaningful output in this case,
> and will crash if self.col was set. Assert that self.info is not None
> in order to formalize this assumption.
>
> We can not yet change the type of the initializer to prove that this
> is provably true at static analysis time until the rest of this
> library is fully typed.

Let's insert another paragraph to make the intent even clearer, and
adjust the remainder for it:

  qapi/error: assert QAPISourceInfo is not None

  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.

Note I also replaced "this library" because I'm not sure what it means.

What do you think?

Re: [PATCH v2 1/8] qapi/error: Repurpose QAPIError as a generic exception base class

[John Snow]

On 4/16/21 2:04 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 4/15/21 2:44 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> Rename QAPIError to QAPISourceError, and then create a new QAPIError
>>>> class that serves as the basis for all of our other custom exceptions.
>>>
>>> Isn't the existing QAPIError such a base class already?  Peeking
>>> ahead...  aha, your new base class is abstract.  Can you explain why we
>>> that's useful?
>>>
>>
>> Sure. The existing QAPIError class assumes that it's an error that has a
>> source location, but not all errors will. The idea is that an abstract
>> error class can be used as the ancestor for all other errors in a
>> type-safe way, such that:
>>
>> try:
>>       qapi.something_or_other()
>> except QAPIError as err:
>>       err.some_sort_of_method()
>>
>> (1) This is a type-safe construct, and
>> (2) This is sufficient to catch all errors that originate from within
>> the library, regardless of their exact type.
>>
>> Primarily, it's a ploy to get the SourceInfo stuff out of the
>> common/root exception for type safety reasons.
>>
>> (Later in the series, you'll see there's a few places where we try to
>> fake SourceInfo stuff in order to use QAPIError, by shuffling this
>> around, we allow ourselves to raise exceptions that don't need this
>> hackery.)
> 
> I think you're conflating a real issue with a non-issue.
> 
> The real issue: you want to get rid of fake QAPISourceInfo.  This isn't
> terribly important, but small cleanups count, too.  I can't see the "few
> places where we try to fake" in this series, though.  Is it in a later
> part, or am I just blind?
> 

I was mistaken, we don't fudge it except in one place, and that gets 
fixed in the parser.py series, not this one.

What I really wanted: I wanted to make the base error object something 
that doesn't have an info field at all, fake or not, so that it can be 
ubiquitously re-used as an abstract, common ancestor.

A separate issue: Sometimes, we want to raise errors that *usually* have 
source information, but *might* not sometimes, because of reasons.

So, I guess I don't really have a particularly strong technical 
justification for this anymore, beyond "following a pattern I see and 
use in other projects":

https://docs.python.org/3/tutorial/errors.html#user-defined-exceptions

"When creating a module that can raise several distinct errors, a common 
practice is to create a base class for exceptions defined by that 
module, and subclass that to create specific exception classes for 
different error conditions"

Which QAPIError does not violate, though I usually see this pattern used 
with a tabula rasa class to maximize re-use.

Some of my parser.py drafts that attempted to split out QAPIDoc using 
the Exception-chaining method we discussed on call winds up using this 
abstract class more directly, but we aren't sure we want that yet. (Or, 
we're fairly sure we want to try to delay thinking about it. I am still 
working on re-cleaning part 5.)

> The non-issue: wanting a common base class.  Yes, we want one, but we
> already got one: QAPIError.
> 
> I think the commit message should explain the real issue more clearly,
> without confusing readers with the non-issue.
> 
> Makes sense?
> 

I'll spend a few minutes and see if dropping this patch doesn't deeply 
disturb later patches (or if it can be shuffled backwards to a point 
where it makes more sense contextually).

I genuinely can't remember if it's going to wrench something else up 
later or not right now, though; and I still haven't finished rebasing 
part 5 so I don't have a "finished" product repository to test on anymore.

--js

Re: [PATCH v2 4/8] qapi/error: Change assertion

[John Snow]

On 4/16/21 2:17 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 4/15/21 3:00 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> On 3/30/21 1:18 PM, John Snow wrote:
>>>>
>>>> Realizing now that this commit topic is wrong :)
>>>>
>>>> A prior version modified the assertion, I decided it was less churn to
>>>> simply add one.
>>>>
>>>> I think ideally we'd have no assertions here and we'd rely on the type
>>>> hints, but I don't think I can prove that this is safe until after part
>>>> 6, so I did this for now instead.
>>>>
>>>>> Eventually, we'll be able to prove that 'info.line' must be an int and
>>>>> is never None at static analysis time, and this assert can go
>>>>> away. Until then, it's a type error to assume that self.info is not
>>>>> None.
>>>>>
>>>>> 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 d179a3bd0c..d0bc7af6e7 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
>>>>>
>>> Please show us the revised commit message.  I'm asking because the
>>> patch's purpose isn't quite clear to me.  Peeking ahead at PATCH 7, I
>>> see that info becomes Optional[QAPISourceInfo].  This means something
>>> passes None for info (else you wouldn't make it optional).  None info
>>> Works (in the sense of "doesn't crash") as long as col is also None.
>>> After the patch, it doesn't.  I'm not saying that's bad, only that I
>>> don't quite understand what you're trying to accomplish here.
>>>
>>
>> Sure.
>>
>> If you recall, I tried to enforce that QAPISourceInfo was *never* None
>> by creating a special value for QSI that represented "No Source
>> Info". Ultimately, that idea didn't go through and we solidified that
>> the 'info' parameter that gets passed around can sometimes be None.
>>
>> Hence, this property is Optional[QAPISourceInfo].
>>
>> Since I know you wanted to crash messily in the case that we allowed a
>> None-info to leak into a context like this, I added the new assertion
>> to make sure this remains the case.
>>
>> (since str(None) evaluates to "None", it seemed like there was already
>> the implicit assumption that info would never be None. Plus, if 'col'
>> is set, mypy notices that we are performing an unsafe check on
>> self.info.line, which had to be remedied.)
>>
>> Later in the series, after schema.py is typed, it may be possible to
>> remove these assertions as we may be able to rely on the strict typing
>> to prove that this situation can never occur. However, since schema.py
>> is not yet typed, we can't yet.
>>
>>
>>
>> Alright. So given that, I think what you'd like to see for a commit
>> message is:
>>
>> qapi/error: assert QAPISourceInfo is not None
>>
>> We implicitly assume that self.info will never be None, as the error
>> reporting function will not produce meaningful output in this case,
>> and will crash if self.col was set. Assert that self.info is not None
>> in order to formalize this assumption.
>>
>> We can not yet change the type of the initializer to prove that this
>> is provably true at static analysis time until the rest of this
>> library is fully typed.
> 
> Let's insert another paragraph to make the intent even clearer, and
> adjust the remainder for it:
> 
>    qapi/error: assert QAPISourceInfo is not None
> 
>    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.
> 
> Note I also replaced "this library" because I'm not sure what it means.
> 

Wiggly-brain, wiggly-mouth. I meant "package". I get these terms 
consistently wrong, and I need to really make a very direct effort to 
treat them precisely correctly in the future.

MODULE: A single Python file. It may be a script, or serve only as a 
library meant for consumption by other scripts.

PACKAGE: A collection of one or more Python modules. QAPI is a package 
because it's a folder with an __init__.py file, containing several modules.

LIBRARY: No formal definition; however, I think of it as a Python module 
that is meant primarily to be consumed by another script or entry-point. 
It has some implications for things like Python's hierarchical logging 
library where the distinction is important for how logger setup is 
handled. Modules that use the "if __name__ == '__main__'" stanza serve 
dual purpose as a script *and* library.


In this case, I meant "package". I think I accidentally avoid this term 
because I don't want to imply that it is a "distributed package" in the 
sense of a "PyPI package" and my brain skips a clock cycle and picks 
another random word.

Bad habit. :(

> What do you think?
> 
> 

TLDR: I took your phrasing wholesale. Thanks!

--js

(Random aside on patch submission process: I do dislike how when I 
change the topic of a commit, I lose out on git-backport-diff 
functionality. I wish I had a persistent ID for commits that survived 
beyond title changes. Sometimes I debate scripting adding some kind of 
Patch-GUID: <...> tag, but I don't know if that would look like 
undesirable clutter in the git log to everyone else. Maybe a "WAS: 
old-topic-name-here" in the comment section would suffice well enough?)

Re: [PATCH v2 4/8] qapi/error: Change assertion

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

[...]

> (Random aside on patch submission process: I do dislike how when I
> change the topic of a commit, I lose out on git-backport-diff 
> functionality. I wish I had a persistent ID for commits that survived
> beyond title changes. Sometimes I debate scripting adding some kind of 
> Patch-GUID: <...> tag, but I don't know if that would look like
> undesirable clutter in the git log to everyone else. Maybe a "WAS: 
> old-topic-name-here" in the comment section would suffice well enough?)

Have you tried git-range-diff?