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

[Michael Selik]

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/20151107/f1073fea/attachment-0001.html>