[Python-ideas] Use `isinstance` check during exception handling
Michael Selik
mike at selik.org
Mon Nov 9 20:14:05 EST 2015
I searched cpython/Lib/*.py for except blocks that have an if-statement as
the first line. r'except \w+ as\s+\w+:\n\s+if .*:' Not the world's most
thorough regex, but it seemed to do the trick.
There are very few repeated except/if blocks that would be appropriate for
refactoring with any technique. The majority were in two modules --
mailbox.py and pathlib.py -- checking if the errorno was ENOINT and/or
ENOTDIR. I suppose those could be improved with FileNotFound and
NotADirectoryError, but it doesn't look like we need anything more potent.
Is there a different place we should look for refactoring candidates?
On Mon, Nov 9, 2015, 12:41 AM <sjoerdjob at sjec.nl> wrote:
> > 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/
>
> _______________________________________________
> 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/20151110/3182236d/attachment-0001.html>
More information about the Python-ideas
mailing list