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

[Sjoerd Job Postmus]

Hi again everybody,

So I've had a couple of days extra to think about it, and yes: it might still break some things, but I expect extremely little.

After the discussions here, I came to think it would require a lot of changes to the codebase, and decided to let it rest. 

Last night, it dawned to me that there might be a fix with very little code-changes needed:

* add an extra operator `PyCmp_EXC_VAL_MATCH`
* in the compiler, change `DUP_TOP` to `DUP_TOP_TWO` just above the generation of the compare. I don't have the exact location, as I'm typing on my phone right now.
* create an extra function PyErr_GivenExceptionValueMatches which calls PyObject_isInstance
* call that from ceval for the `PyCmp_EXC_VAL_MATCH.
* most important: update the docs.

A little bit of code duplication, but not that much.

As for the scary part.
* existing code: Because this would only introduce changes for Python code, existing C-extensions would not be affected. Python code would, but I think the fallout would be very little, as the only noticeable changes are when a class overrides its __instancecheck__, which I believe is too negligible a corner-case.
* performance: with the changes already proposed for the moving towards __subclasscheck__ from the current behaviour, there are some optimizations included which should also benefit this proposal. Exact measurements are of course needed.

One of the changes I do see needed in Python-code is having the unit-testing libraries change their 'assertRaises' code to reflect the new behavior.

I agree this might cause a backward-compatibility issue, however after some thinking about it, I don't think it will affect more than 1 percent of already existing code, probably even way less. I don't have any numbers to support that feeling, though.

As far as an important project: if you're depending on obscure features, you should probably be ready to expect changes there. Yes, the current behavior is precisely documented, but in nearly all of the cases the actual implementation and the proposed change agree, unless one did magic.

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.

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/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/python-ideas/attachments/20151109/371b658a/attachment.html>