[Python-ideas] Use `isinstance` check during exception handling

[sjoerdjob at sjec.nl]

> I made the changes I listed above and ran `make; make test`, and got only
> 1 error due to socket.socket running out of memory or something. I'll
> re-run the tests on a clean checkout as well, but for now I think it's
> unrelated.

After running the tests against a clean checkout (without my changes): I
get the same errors from test_socket. So it's unrelated.

> Regards,
> Sjoerd Job Postmus
>
>> On 7 Nov 2015, at 20:18, Michael Selik <mike at selik.org> wrote:
>>
>> Sjoerd, glad we agree, happy to help.
>>
>> In case someone is still interested in making a patch, I'd like to
>> clarify why it makes me nervous.
>>
>> Though Sjoerd led with a TL;DR that summarized his suggestion as a swap
>> of isinstance for issubclass, a better summary would have been: "I
>> prefer more specific exception types". The proposed solution was to
>> enable custom exception types that conditionally insert themselves into
>> the class hierarchy depending on data stored on a raised exception
>> instance.
>>
>> Unfortunately, that solution requires changing an obscure error handling
>> feature. As Sjoerd noted when he sent the first message in this thread,
>> it is possible this would introduce a backwards-incompatibility. Perhaps
>> no one in the standard library or major public projects would write such
>> weird code as to get tripped up by the proposed change, but it's always
>> possible that someone out there is using this feature for an important
>> project. I think we've learned the lesson from the long slog to Python 3
>> adoption (and how Perl 6 harmed that community, and countless other
>> examples) that backwards compatibility is of utmost importance.
>>
>> The current code pattern for a single try/except is to catch the general
>> exception and have an if-condition in the except block.That's a frequent
>> and readable practice.
>>
>>     try:
>>         cursor.execute(query)
>>     except sqlite3.OperationalError as str(e):
>>         if 'syntax error' in str(e):
>>             print('Your SQL is bad')
>>
>> If that code or similar code appears many times, a good way to refactor
>> with the current features of Python is to use a context manager.
>>
>>     class SqlSyntaxErrorHandler:
>>         def __enter__(self):
>>             return self
>>         def __exit__(self, etyp, einstance, etraceback):
>>             if (issubclass(etyp, sqlite3.OperationalError)
>>                 and 'syntax error' in str(einstance)):
>>                     print('Your SQL is bad')
>>
>>     with SqlSyntaxErrorHandler:
>>         cursor.execute(first_query)
>>
>>     with SqlSyntaxErrorHandler:
>>         cursor.execute(second_query)
>>
>> While context managers may not be a beginner topic, they are typically
>> easier to read and debug than using metaclasses for conditional runtime
>> inheritance. Even if there would be some benefit to the metaclass style,
>> it's not worth the risk of changing current error handling behavior.
>>
>> -- Michael
>>
>>
>>> On Fri, Nov 6, 2015, 2:49 AM Andrew Barnert via Python-ideas
>>> <python-ideas at python.org> wrote:
>>> On Thursday, November 5, 2015 9:57 PM, "sjoerdjob at sjec.nl"
>>> <sjoerdjob at sjec.nl> wrote:
>>>
>>> >>  Andrew Barnert via Python-ideas writes:
>>> >>   > Sjoerd Job Postmus <sjoerdjob at sjec.nl> wrote:
>>> >>
>>> >>   > > It might seem troublesome to change these locations to raise
>>> >>   > > actual instances instead of strings, but those might actually
>>> >>   > > just be hidden bugs just the same.
>>> >>
>>> >>  No, I'm sure mostly they're not.  They're working code that has
>>> > been
>>> >>  there since the early days, perhaps a few from the 2.x days in
>>> >>  modules.  Python is generally very conservative about changing
>>> working
>>> >>  code.
>>> >>
>>> >>   > > Do you think anyone would object if I'd go over the code
>>> > looking
>>> >>   > > for places where type(tstate->cur_value) !=
>>> > tstate->cur_type and
>>> >>   > > propose patches for these?
>>> >>
>>> >>  I think, yes, though I don't know enough about it to object myself.
>>> >>  Code churn of this kind is generally frowned upon, unless you can
>>> show
>>> >>  that the risk of hidden bugs of the kind you refer to above is
>>> >>  substantially greater than the risk that *you* will introduce bugs
>>> >>  with your patches.
>>> >
>>> > After digging more into the code, I concur. It's quite easy to make
>>> some
>>> > changes that break some stuff in this logic.
>>>
>>> Looking at this a bit further, I think there may be something you can
>>> do that gets everything you actually wanted, without changes all over
>>> the place and serious risks.
>>>
>>> Basically, it's just two pretty small changes in errors.c:
>>>
>>> * Change PyErr_GivenExceptionMatches so that if given a value that's an
>>> instance of an exception type (PyExceptionInstance_Check(err) is true),
>>> instead of just getting its class and then ignoring the value, it does
>>> a standard isinstance check on the value.
>>>
>>> * Change PyErr_ExceptionMatches so that if there is an exception value,
>>> and its type matches the exception type, it is passed to
>>> PyErr_GivenExceptionMatches; otherwise, the exception type is passed.
>>>
>>>
>>> So, all Python code that raises and handles exceptions will get the
>>> benefit of isinstance checks. Any new C code that wants that benefit
>>> can get it very easily (only raise with PyErr_SetObject or something
>>> that uses it like PyErr_SetFromErrno; only handle with
>>> PyErr_GivenExceptionMatches). Any existing C code that skips creating
>>> an instance will continue to do so, and to get the same fast-path check
>>> used today, except for one extra call to
>>> PyExceptionInstance_Check(tstate->curexc_value).
>>>
>>> Of course it's not just that small patch to errors.c. You also need
>>> tests to exercise the new functionality. And a docs patch for the
>>> Exceptions section of the C API. And also a docs patch to the
>>> Exceptions section in the Reference. But no other code patches.
>>>
>>> And you'd definitely need to performance-test two things: First, that
>>> the extra PyExceptionInstance_Check doesn't slow down fast-path
>>> handling (e.g., in an empty for loops). Second, whatever performance
>>> tests your original proposal would have needed, to make sure the
>>> isinstance check doesn't noticeably slow down pure-Python code that
>>> handles a lot of exceptions.
>>>
>>>
>>> I think the only C API docs change needed is to PyErr_ExceptionMatches.
>>> However, a note explaining how to make sure to get and/or to bypass the
>>> isinstance check as desired might be useful.
>>>
>>> For the reference docs, I think just this note at the end of the
>>> section:
>>>
>>> > Note: Exceptions raised and handled by Python code use the same
>>> mechanism as the isinstance function for determining whether an
>>> exception matches. However, exceptions raised or handled by the
>>> implementation (or implementation-level extensions) may bypass that
>>> and check the type directly.
>>>
>>>
>>> > New in 3.6: In previous versions of Python, exception handling always
>>> bypassed the isinstance function and checked the type directly.
>>> _______________________________________________
>>> Python-ideas mailing list
>>> Python-ideas at python.org
>>> https://mail.python.org/mailman/listinfo/python-ideas
>>> Code of Conduct: http://python.org/psf/codeofconduct/
> _______________________________________________
> Python-ideas mailing list
> Python-ideas at python.org
> https://mail.python.org/mailman/listinfo/python-ideas
> Code of Conduct: http://python.org/psf/codeofconduct/