Pycodestyle: E722: (bare excepts) too strict (reraise)

[snoack]

How I read PEP-8, it doesn't outlaw bare excepts, but merely recommends to catch more specific exceptions when possible. But what if there is no particular exception you want to catch, but when you just want to do some cleanup before propagating any exception?

try:
    self.connection.send(...)
except:
    # We only close the connection on failure, otherwise we keep reusing it.
    self.connection.close()
    raise

Using try...finally isn't an option here, since we want to reuse the resource on success, and only clean up on failure. I could just explicitly catch BaseException instead, but there is no indication in PEP-8 that this is preferable (otherwise why would bare except be supported in the first place).

So how about suppressing E722 if there is a raise statement in the except block?

[guyskk]

Using bare except for resource closing is a common behavior, especially when I implement context manager. The check of do not use bare except should be changed to do not use bare except without raise.

[spaceone]

The error code E722 was implemented in #592 / (#579) and I warned about it but sigmavirus24 wasn't interested and told that I did not read the whole thread....

[sigmavirus24]

To be clear, pycodestyle doesn't do look-ahead's so we cannot silence this if there is a raise in the following block. That's just not how pycodestyle has ever worked and it would require a significant rewrite to enable that kind of behaviour.

---

Quoting the PEP

When catching exceptions, mention specific exceptions whenever possible instead of using a bare except: clause.

For example, use:

try:
    import platform_specific_module
except ImportError:
    platform_specific_module = None

A bare except: clause will catch SystemExit and KeyboardInterrupt exceptions, making it harder to interrupt a program with Control-C, and can disguise other problems. If you want to catch all exceptions that signal program errors, use except Exception: (bare except is equivalent to except BaseException:).

A good rule of thumb is to limit use of bare 'except' clauses to two cases:

  1.  If the exception handler will be printing out or logging the traceback; at least the user will be aware that an error has occurred.
  2.  If the code needs to do some cleanup work, but then lets the exception propagate upwards with raise. try...finally can be a better way to handle this case.

As a general rule, this new check is good. There are specific cases where people will need to do general clean-up work, as described above. Since we cannot check for people re-raising the exception in the except block, it truly is up to the user to determine whether:

1. This check is useful to them at all or whether they feel it should be ignored globally
2. Their particular use of a bare except is necessary and it should be ignored in that particular case (e.g., with # noqa or # noqa: E722).

I would hazard a guess that 90% of pycodestyle's users will find this check worthwhile, useful, and helpful. We can not ever satisfy 100% of our users so I am happy to settle for 90%.

[snoack]

I have two (real world) scenarios:

1. Legacy code, initially written by novices, with inappropriate use of bare except all over the place. E722 is helpful there (somewhat since this usually isn't the only issue in such legacy code, and you end up ignoring most errors there anyway).
2. Code written by me (and similarly experienced Python developers), where bare except is used occasionally, but never without re-raising the exception. For reference, E722 is reported inappropriately three times in this project of mine, where the exception is re-raised.

If the design of pycodestyle doesn't allow considering the following block, in order to avoid inappropriate errors, my understanding is, that due to the potential of false positives this check should not be enabled by default, or such a check should rather be implemented in pyflakes which considers the AST and therefore is able to ignore bare excepts that re-raise the exception.

I'm not going to clutter my code with #noqa comments. IMO the purpose of a linter is to help you writing cleaner code, not to require additional boilerplate for legit practices. This isn't any better than using except BaseException.

[sigmavirus24]

I'm not going to clutter my code with #noqa comments.

I'm not sure if you're intentionally ignoring my suggestion to include it in the ignore list for your projects or if you just want to try to argue. Either way, I'm not here to argue with you. Pycodestyle is a tool used by novices and experts alike. Novices will learn from this and so will some experts. Since this is a style tool, there will always be places where some people disagree with the checks and will disable them. That's normal. Just because a handful of folks object to a rule doesn't mean we will put it in the DEFAULT_IGNORE list even if generally speaking it has value for everyone else.

As for the suggestion that pyflakes entertain a style check, you can make that argument to them, but that is typically entirely against their philosophy.

[lordmauve]

I just hit this as well. In my experience false positives can have a damaging effect. I've seen novices commit naive "fixes" for correct code. I believe this issue will cause novices to rewrite what I wrote

try:
    ...
except:
    destfile_tmp.unlink()
    raise

to

try:
    ...
except Exception:
    destfile_tmp.unlink()
    raise

which will of course leave temporary files around in the case of KeyboardInterrupt/SystemExit.

This check needs to see the raise, or be better worded, to steer people away from naive fixes.

[FichteFoll]

That's kind of a bad example though because you should be using finally to clean up resources. It could lead to novices getting a "wrong" idea, I agree, but they're already doing it wrong in the first place.

[lordmauve]

No, as in @snoack's example, this is not an unconditional clean-up. In the happy path the file gets moved into a permanent location.

[kingsj0405]

autopep8 said use BaseException.

[hoylemd]

I'm with @sigmavirus24 on this one, but I'd take it a little further: a bare except is inherently unpythonic. Two important parts of python philosophy (import this) are 'Explicit is better than implicit' and 'Special cases aren't special enough to break the rules.'. I point these out because a bare except is a special, highly implicit case. Every other except has an 'argument', indicating the scope of the error to be handles. Even if that's 'everything', it's better to explicitly state that.

'There should be one-- and preferably only one --obvious way to do it.' also applies, since you can accomplish the same thing for the low-low price of 14 characters. That's simply not enough of a 'savings' to make it worth it.

There's also the fact that things like KeyboardInterrupt aren't caught by Exception for a reason. Those represent very exceptional cases - cases which shouldn't occur in normal (e.g. production) operation unless something has gone very wrong or the user is explicitly requesting an immediate shutdown.

In regards to @lordmauve's case: Why do you want to clean up those temporary files in every possible exception case? except: tells me that you don't know what exceptions may be raised. In those (hopefully rare) cases, your app will certainly crash (unless you have a bare except that silences every possible exception, but that's a whole new level of bad design), and you will probably need to investigate why. A temporary file is part of the state of the application at the point in time, and therefore will be forensically valuable.

If you know for sure that the temporary file will be forensically worthless, then except Exception: or except BaseException: makes that clear, and indicates that you're fully aware of the implications. If you're more worried about old temporary files accumulating in some way, consider having your app do cleanup as part of initialization. That idea comes from the Crash-only software pattern (It's a lot more reasonable than it sounds, I promise). Basically, it's a lot easier to assume that your app always terminates unexpectedly and code against that than to handle unexpected termination as a special case (i.e. Turning off the power is guaranteed to be an option, but a shut down command may fail or be unavailable).

As a last resort, there's always # noqa. If you absolutely must break from best practices, then 8 characters is a very small price to pay.

edit: I r gud riter 😜

[sigmavirus24]

@hoylemd I appreciate the support.

Those represent very exceptional cases - cases which shouldn't occur in normal (e.g. production) operation unless something has gone very wrong or the user is explicitly requesting an immediate shutdown.

This sounds like you're assuming all development is on continuously running applications on a remote server. Things like pycodestyle and flake8 are "production" applications that should handle KeyboardInterrupt. That said, both projects handle it explicitly. I think I understand your point, but it feels like it's imposing a false dichotomy around what is "production".

Why do you want to clean up those temporary files in every possible exception case?

I can imagine a few cases where @lordmauve would want to (and should!) clean up the temporary files. Perhaps those temporary files contain some sensitive information and cleaning them up is the secure thing to do. In reality, neither of us know the details and I think we should be assuming that they know their constraints better than us. Of course having some non-sensitive temporary files lying around can be useful for debugging, but there are certainly valid cases where those shouldn't be left around no matter what exception happens.

[hoylemd]

@sigmavirus24

I think you're right re: imposing a false dichotomy - I'm primarily a web developer, where the dichotomy is pretty reliable.

In regards to temporary files containing sensititive information, Wouldn't it be a risk to write them to disk at all? If the power is shut off at the right moment, the data would still be there on disk, and no exception handling would have a chance to clean it up. So that makes me think that a temporary file is the wrong tool in the first place. I'd want to keep that in something volatile (like memory) so that it's sort of fail-safe.

You're more general point is a good one. - We can't know the real requirements on a project we're not a part of. But if those requirements do require a cleanup in all exception cases, I think we probably agree that it should be done explicitly.

[sigmavirus24]

I'd want to keep that in something volatile (like memory) so that it's sort of fail-safe.

Again, there are constraints where it makes sense. I agree with you in general, but there are always exceptions to the rule.

Also we're getting off topic :smile:

[lordmauve]

except BaseException doesn't mean the same thing as except: in Python 2; in Python 2 you could raise old-style classes as exceptions, which do not inherit from BaseException.

Regarding "I could never imagine a situation where I'd want to unconditionally clean up in the case of any exception":

• In the case of temporary files, never leave them around on a production server because they can cause additional impact (eg. filling a disk volume) which can be much more serious than the original crash. Reproduce the issue in dev and diagnose.
• Just came across another example: in the case that my event loop crashes due to programming error, I must kill my subprocesses which are now in an unknown state (code).

[hoylemd]

@lordmauve

I'll admit that there are some cases where except: is the right call, but they're cases that pycodestyle isn't intended to handle out of the box. Marking those with # noqa might seem a little cluttery, but it's much more helpful to those same junior devs who would be confused by the error. It explicitly indicates a code smell that is a necessary evil. Ideally, you'd document why it is necessary as well so that they understand why the exception had to be made.

So to go back to your initial comment, Pycodestyle isn't designed in a way that it can detect the raise, so that's a non-starter. As for steering novices away from naive changes, that's the purpose of code review, not linting. A better solution is to prevent them from even knowing about it in the first place though, hence # noqa and documentation.

[lordmauve]

Pycodestyle isn't designed in a way that it can detect the raise, so that's a non-starter

Or evidence that this check needs to be moved to Pyflakes, which does consider the AST, in order to eliminate this false positive.

As for steering novices away from naive changes, that's the purpose of code review, not linting.

In a large organisation that is impossible. Junior people are often two or three steps removed from people who could confidently tell you what good practice looks like. We rely on linters to do this, and both the false positive and false negative rates are important. I'm not saying I'm not worried about the false negative. I'm arguing that this creates an undesirable false positive.

A better solution is to prevent [novices] from even knowing about it in the first place though [by putting #noqa on that line]

You're assuming I'm writing code now for future novices to read. I'm concerned about the novices now maintaining the code I wrote 5 years ago when this check wasn't a thing.

[sigmavirus24]

Or evidence that this check needs to be moved to Pyflakes, which does consider the AST, in order to eliminate this false positive.

PyFlakes wouldn't accept this for the very reason that it's so controversial. They only accept obvious problems, e.g., unused imports. If this isn't satisfactory here, a different stylistic option for having it would be via a Flake8 AST plugin. Luckily, flake8-bugbear already provides this check using the AST but a quick check of the source indicates that it's as strict as this check is. I'd suggest either collaborating with bugbear or creating a new plugin that meets the specific needs.

As it stands, this discussion seems to be getting heated. I'm going to lock the thread because it's devolved from constructive conversation to far less productive conversation.