classification
Title: contextlib.suppress: Add whitelist argument
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.6
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: SilentGhost, barry, mb_, ncoghlan, rhettinger, yselivanov
Priority: normal Keywords: patch

Created on 2016-08-20 12:28 by mb_, last changed 2016-08-23 12:02 by ncoghlan. This issue is now closed.

Files
File name Uploaded Description Edit
contextlib-suppress-whitelist.patch mb_, 2016-08-20 12:28 review
contextlib-suppress-whitelist-v2.patch mb_, 2016-08-20 15:06 review
Messages (13)
msg273206 - (view) Author: Michael Büsch (mb_) * Date: 2016-08-20 12:28
This adds a whitelist parameter with the name 'unless' to contextlib.suppress, so one can specify exceptions that will not be suppressed.

This is useful for specifying single sub-exceptions that we still want to catch, even of we want to suppress all other siblings.

Or it can be used to suppress all exceptions except some very specific ones by suppressing BaseException and whitelisting things like SyntaxError, NameError and the like.

Usage looks like this:

with suppress(OSError, unless = PermissionError):
    with open("foobar", "wb") as f:
        f.write(...


I selected the name 'unless' instead of 'whitelist' or such, because I think that pronounces nicely in the 'with' line context. However please feel free to make better suggestions.
msg273211 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2016-08-20 14:12
Could you please produce a patch that conforms to PEP-8.
msg273212 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2016-08-20 14:15
Also, these would need documentation changes.
msg273213 - (view) Author: Michael Büsch (mb_) * Date: 2016-08-20 14:18
>Could you please produce a patch that conforms to PEP-8.

I tried hard to do so. Could you please tell me what parts are not compliant, so I can fix them?
msg273215 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2016-08-20 14:25
Spaces around '=' sign in function definitions/calls.
msg273217 - (view) Author: Michael Büsch (mb_) * Date: 2016-08-20 15:06
Thanks for your comments.
Here's version 2 of the patch.

Changes:
- A typo in the docstring was fixed.
- Space was removed in keyword assignments.
- Documentation was added.

(Note that I signed and sent the contributor agreement. It should arrive soon.)
msg273226 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-08-20 17:00
Michael, first of all, thanks for taking the time to propose this idea, and put together a patch for it.

Unfortunately, I'm going to have to decline the proposal for API design reasons (which I'll explain below), and instead point you towards https://bugs.python.org/issue12029 which aims to fix a limitation in the normal exception handling that prevents custom __subclasscheck__ hooks from being usable in normal except clauses.

This is a more significant proposed change to the language than it first appears, as Python has never really had a concise fully standardised "catch these exceptions, but not these ones" syntax, with your options being limited to a preceding "except <excluded types>: raise" clause, or doing an isinstance() check on an already caught exception.

I'm also not sure it's an idiom we really want to encourage, as it tends to age poorly as new exception subclasses are added, as well as confusing exception flow logic like the example given in the documentation, which would be clearer with a single inline exception statement and a well-named flag variable:

    run_optional_code = True
    try:
        os.remove(somefile)
    except PermissionError:
        run_optional_code = False
    except OSError:
        pass
    if run_optional_code:
        # Executed on success and on OSError
        # other than PermissionError
        # In a real example, "run_optional_code" would
        # be replaced by a name that actually conveyed
        # useful information to future readers

For folks that do need this capability, building it themselves is pretty straightforward, and if they go so far as to define a custom __instancecheck__ and __subclasscheck__ implementation, that will already work with contextlib.suppress(), but would need https://bugs.python.org/issue12029 to land to work with ordinary except clauses.
msg273231 - (view) Author: Michael Büsch (mb_) * Date: 2016-08-20 17:45
>and instead point you towards https://bugs.python.org/issue12029 

Fair enough.
But how would a 'suppress OSError, but catch FileNotFoundError' look like with this for example?
(Note that I can't subclass the exception)

>I'm also not sure it's an idiom we really want to encourage, as it tends to age poorly as new exception subclasses are added,

I partially agree. But there's one important spot where I need this behaviour: It is cleanup paths where I cannot react to most exceptions. For example because I already am handling exceptions and am already trying to tear the whole thing down anyway.

>as well as confusing exception flow logic like the example given in the documentation

I disagree.
The control flow does not change with this patch at all.
It either pops out of the with-statement with an exception, or it does not.
The only thing this patch does is _reduce_ the set of exceptions it suppresses.

>For folks that do need this capability, building it themselves is pretty straightforward, 

Nah. The whole point of contextlib.suppress is to avoid boilerplate code. People could do their own stuff. In fact, that is what I did. But I prefer a standard solution in the standard library for this really common problem instead.
msg273234 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-08-20 18:07
It's the "really common problem" assertion that I'm disputing - when particular instances of "catch this exception, but not these ones" become common, we tend to *change the standard exception hierarchy* to eliminate them (e.g. StopIteration, KeyboardError, GeneratorExit no longer inheriting from Exception).

So if you have new cases where that's happening frequently for you (presumably as a result of the https://www.python.org/dev/peps/pep-3151/ reworking of the OSError hierarchy, given the documented examples), then it would be better to take that *problem* to python-ideas for discussion and brainstorming, before coming back to the issue tracker to propose a possible solution (whether that's changing the hierarchy, enhancing contextlib.suppress, or doing something else)
msg273239 - (view) Author: Michael Büsch (mb_) * Date: 2016-08-20 18:29
>when particular instances of "catch this exception, but not these ones" become common, we tend to *change the standard exception hierarchy* to eliminate them (e.g. StopIteration, KeyboardError, GeneratorExit no longer inheriting from Exception).

But my patch does not change that behaviour.
We already have "catch this exception, but not these ones". It's called contextlib.suppress.
I'm not adding this behaviour to the library/language.
This patch just improves contextlib.suppress in the way that we can now easily remove exceptions from the suppress-set.

>So if you have new cases where that's happening frequently for you 

Ok, let me explain the real world scenario that I have here.
Imagine some application that does something over the network and an exception occurs. We catch that and decide that the program must die. However before doing so we need to send stuff over the network to cleanly tear down things. This is done on a best-effort basis. So we try to send these messages, but we silently ignore _all_ failures. What would we do anyway? We are exiting already.

And that's what I do. Wrap these actions in helpers that ignore all exceptions, except for SyntaxError and NameError and such, so that I can still see programming errors in the source code.
So I would do 'with suppress(Exception, unless=(SyntaxError, ...)'.

I do think that tearing down stuff and ignoring every exception really is a common thing to do. See __del__ for example. There's a reason we must make sure to ignore all exceptions before we return from __del__.
We could actually use suppress with unless for this.

If we try to handle exceptions in a tear-down situation instead, we will end in an infinite loop most of the time.

But suppress.unless is useful for other things, too.
It's a way to say: I want to ignore this set of errors, except this/these single ones from the set.
This patch adds the 'except' part of this sentence to contextlib.suppress.
msg273242 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-08-20 19:18
contextlib.suppress provides a contextmanager spelling for the following pattern:

    try:
        <body>
    except <expr>:
        pass

That's a very common pattern worth having in the standard library, even though it's only a 5 line context manager.

The proposed API change would make it instead an implementation of the vastly *less* common pattern:

    try:
        <body>
    except <unless-expr>:
        raise
    except <expr>:
        pass

For the use case you're discussing (trying to shut down, potentially failing, but also not wanting to hide genuine programming errors), I'd be more amenable to introducing a comparable context manager to the logging module that, instead of silently ignoring caught exceptions, logged them, and also allowed you to restrict which exceptions were logged.
msg273400 - (view) Author: Michael Büsch (mb_) * Date: 2016-08-22 20:04
>an implementation of the vastly *less* common pattern:

Ok, here are some numbers.
My codebase is about 32 kLOC.

$ git grep suppressAllExc |wc -l
20
$ git grep contextlib\\.suppress |wc -l
17

(suppressAllExc being my local version to suppress Exception, but exclude a few)

So _neither_ one is pretty common. And that's not surprising, as exceptions ought to be properly handled most of the time.
However the ratio between the two forms is about 1:1. So I don't agree that it would be "vastly *less* common".

But I do accept if such a thing is not wanted upsteam, Altough it basically is a twoliner. I'll simply have to ship my own (I'll have to do that anyway for older Python versions. meh :)

Thanks for the discussion.
msg273434 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-08-23 12:02
Thanks for being understanding of the decision. Regarding my comment above about __subclasscheck__ potentially letting you implement this without changing the signature of suppress, here's an example of how you might be able to do that.

First, define a metaclass that delegates type checks to the type itself (similar to abc.ABCMeta):

class FilteredExceptionMeta(type):
    def __subclasscheck__(cls, other):
        return cls.__subclasshook__(other)
    def __instancecheck__(cls, other):
        return cls.__subclasshook__(type(other))

Then, define a factory function to create custom classes based on that metaclass:

def filtered_exc(exc_type, *, unless=()):
    class _FilteredException(metaclass=FilteredExceptionMeta):
        @classmethod
        def __subclasshook__(cls, other):
            return (issubclass(other, exc_type)
                    and not issubclass(other, unless))
    return _FilteredException


>>> from contextlib import suppress
>>> selective_filter = suppress(filtered_exc(OSError, unless=FileNotFoundError))
>>> with selective_filter:
...     raise OSError("Suppressed")
... 
>>> with selective_filter:
...     raise FileNotFoundError("Not suppressed")
... 
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
FileNotFoundError: Not suppressed

This works because suppress() calls issubclass() explicitly, unlike the current implementation of except clause processing.
History
Date User Action Args
2016-08-23 12:02:28ncoghlansetmessages: + msg273434
2016-08-22 20:04:34mb_setmessages: + msg273400
2016-08-20 19:18:23ncoghlansetmessages: + msg273242
2016-08-20 18:29:56mb_setmessages: + msg273239
2016-08-20 18:07:04ncoghlansetmessages: + msg273234
2016-08-20 17:45:10mb_setmessages: + msg273231
2016-08-20 17:00:22ncoghlansetstatus: open -> closed
resolution: rejected
messages: + msg273226

stage: patch review -> resolved
2016-08-20 15:06:31mb_setfiles: + contextlib-suppress-whitelist-v2.patch

messages: + msg273217
2016-08-20 14:25:32SilentGhostsetmessages: + msg273215
2016-08-20 14:18:49mb_setmessages: + msg273213
2016-08-20 14:15:36SilentGhostsetmessages: + msg273212
2016-08-20 14:12:41SilentGhostsetversions: + Python 3.6
nosy: + SilentGhost

messages: + msg273211

stage: patch review
2016-08-20 14:01:47barrysetnosy: + barry
2016-08-20 13:00:48mb_setnosy: + rhettinger
2016-08-20 12:28:02mb_create