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

[Andrew Barnert]

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.