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

[John Snow <jsnow@redhat.com>]

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))