Warn about bare except clause #592

Merged
merged 1 commit into from Jan 1, 2017

Projects

None yet

6 participants

@rogalski
Contributor
rogalski commented Nov 6, 2016

Solves #579

@spaceone
Contributor
spaceone commented Nov 6, 2016

Does this also blame the following?

try:
    foo()
except:
    self.log('foo failed')
    raise

It would be nice if this detects re-raising and don't blame then.

@spaceone
Contributor
spaceone commented Nov 7, 2016 edited

It would also be nice if it doesn't blame the following code:

try:
    foo()
except (SystemExit, KeyboardInterrupt, GeneratorExit, SyntaxError):
    raise
except:
    bar()
@rogalski
Contributor
rogalski commented Nov 7, 2016 edited

I'm quite sure that won't be possible in current pycodestyle philosophy, where tool is lightweight and no AST is built for purpose of yielding warnings. If check in current form is too restrictive, it likely should be rejected. Hopefully one of maintainers can confirm.

@sigmavirus24
Member

@spaceone the correct forms of both of those constructs is to use except Exception unless you're writing Python that is meant to run on Python 2.4 or earlier.

@sigmavirus24 sigmavirus24 added this to the 2.2 milestone Nov 7, 2016
@sigmavirus24
Member

New checks are only added in minor releases, so I've marked this for the 2.2.0 release (or the 2.2 milestone). A quick look tells me this looks good @rogalski. Thanks!

@The-Compiler
The-Compiler commented Nov 7, 2016 edited

I think there are good reasons to use a bare except: and re-raising - imagine something like this with a library not providing context managers:

transaction = ...
try:
    ...
except:
    transaction.rollback()
    raise

Here except: is the right thing to use, as you'd want to do the rollback even when there's e.g. a KeyboardInterrupt.

You could use except BaseException: on Python 3, but I feel like that is pretty subjective. Even pep8 (the document) says:

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

  • If the exception handler will be printing out or logging the traceback; at least the user will be aware that an error has occurred.
  • 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.

(my emphasis).

IMHO, this kind of check is too aggressive for (what I preceive to be) pycodestyle's philosophy, and better left to more aggressive and sophisticated checkers like pylint (which already does this check, and doesn't emit the message when re-raising).

@sigmavirus24
Member

IMHO, this kind of check is too aggressive for (what I preceive to be) pycodestyle's philosophy

I don't agree (but I also don't entirely disagree). Here's why: Those two cases are two very special cases and what the part you're not quoting is specifically warning about is what I expect this check would cat 90+% of the time.

For the cases you're discussing, I expect those to be rare enough and advanced enough that the developer would know when to use # noqa to silence that particular item. So, from that perspective I think this is perfectly acceptable.

That said, I'm also not convinced that this won't cause a glut of headaches for folks using bare excepts correctly today. Perhaps a decent compromise would be to add this but also include it in the default ignore list so that people who do not want this style used (except in very rare cases) can turn it on without us causing anyone else significant pain and suffering.

@spaceone
Contributor
spaceone commented Nov 7, 2016 edited

This case is not rarely used!

@rogalski
Contributor
rogalski commented Nov 7, 2016

Are any major open source libraries using bare excepts in this manner?

On the other hand, I honestly feel like 99% of bare excepts implemented in Python code everyday are done because of sloppiness or unawareness and not because it's a correct approach for solving given problem.

Having it as an opt-in seems like a reasonable compromise, but rejecting it entirely is also fine. I guess maintainers have to make a decision.

@sigmavirus24
Member

Are any major open source libraries using bare excepts in this manner?

The largest body of open source Python I've seen (OpenStack) explicitly avoids bare except clauses like this and tends to use either Exception or BaseException. In fact I'd be intrigued to see if this would catch any bare except clauses in those projects (my guess is no).

I honestly feel like 99% of bare excepts implemented in Python code everyday are done because of sloppiness or unawareness and not because it's a correct approach for solving given problem.

I think sloppiness isn't the correct thing here. I think it's more of "Person X over in Department Y needs this to be working by the end of the day and I can't handle every exception case right now. I'll fix this later." In other words, they didn't have the time to do it. I would hesitate to call that anything other than efficiency.

I guess maintainers have to make a decision.

As a maintainer, I'd like this check included. I'll leave the final call (as to whether this is on by default or ignored by default) to @IanLee1521 though.

@rogalski
Contributor
rogalski commented Nov 7, 2016

Person X over in Department Y needs this to be working by the end of the day and I can't handle every exception case right now. I'll fix this later

The thing is - correct solution for this case in Python is to use except Exception:. You may call it an efficiency, but for me it's unawareness of Python internals...

pycodestyle.py
+ match = regex.match(logical_line)
+ if match:
+ exception_name = match.group(1)
+ if exception_name in (None, 'BaseException'):
@rogalski
rogalski Nov 7, 2016 Contributor

Maybe except BaseException: should be allowed.

@sigmavirus24
sigmavirus24 Nov 7, 2016 Member

Or it should generate a different error code. Either one works for me.

@spaceone
spaceone Nov 8, 2016 Contributor

The code also doesn't detect except (ValueError, BaseException):

@spaceone
Contributor
spaceone commented Nov 7, 2016

except: and except BaseException: is the same.
https://docs.python.org/3/library/exceptions.html#exception-hierarchy

@sigmavirus24
Member

You may call it an efficiency, but for me it's unawareness of Python internals...

A lot of the people I work with might call it both. ;)

@IanLee1521 IanLee1521 self-assigned this Nov 7, 2016
@spaceone
Contributor
spaceone commented Nov 8, 2016
You may call it an efficiency, but for me it's unawareness of
Python internals...

A lot of the people I work with might call it both. ;)

And what about the people who know about the python internals? I have
valid reasons to use "except:" and I don't want to waste my code with
different '# noqa'-like statements for every style check tool which
exists. (also '#noqa is unspecific about what shouldn't be QAed).

I don't want this code to be false-positive detected and yes I want to
catch also KeyboardInterrupt, SystemExit and GeneratorExit.

  2429 »   »   try:
  2430 »   »   »   super(Object, self)._move(newdn, modify_childs)
  2431 »   »   except:
  2432 »   »   »   # self couldn't be moved
  2433 »   »   »   # move back subelements and reraise
  2434 »   »   »   self.move_subelements(tmpdn, olddn, subelements)
  2435 »   »   »   raise

@sigmavirus24
Member

@spaceone it seems you did not read the entire thread. Please do that before you continue with these aggressive comments. If you are confused about what we're discussing, feel free to ask questions and I'll be happy to clarify things. I think there's some agreement that this should start out (or permanently exist) in the DEFAULT_IGNORE list, so your comments are not constructive.

@spaceone
Contributor
spaceone commented Nov 8, 2016

Sorry for the agressiveness. I of course read and am involved in the whole thread since the beginning.
And it seems my suggestion to ignore re-raised exception blocks will still cause this error (even if the check is disabled by default, or in some DEFAULT_IGNORE list whatever this is).

@sigmavirus24
Member

@spaceone Right, it was becoming apparent that you didn't know what DEFAULT_IGNORE was. This is a collection of error codes that pycodestyle does not check for in a default configuration (i.e., you don't specify your own --ignore) In other words, adding this there means it won't be checked for unless people select it or remove it from their ignore list.

@rogalski
Contributor
rogalski commented Dec 4, 2016

Is there a reasonable consensus on what should be covered by this checker?

I want to clean up my dangling pull requests - either rebase and finish implementation or throw them away.

@ambv
ambv commented Dec 22, 2016

Bugbear started out as a project because of bare except: clauses in codebases I work with. See the error message for B001.

What I learned with experience and was confirmed by talking with Raymond and Guido is that "except:" is often used because the user didn't know any better. @spaceone's use case for catching BaseException is reasonable, there's cases we really need that. In this case it's better to be explicit is write "except BaseException". This is what Guido himself is doing when coding, see example: https://hg.python.org/cpython/file/3.6/Lib/asyncio/selector_events.py#l875

My advice would be to warn against using bare except: and allow except BaseException:.

@rogalski
Contributor

After re-reading whole pull request comment history, I really feel like warning strictly about bare except: and allowing all other versions will be a most reasonable choice, especially because explicit is better than implicit. I'll revisit this change and push new revision soon.

@ambv do you know if BDFL have ever made public statement on this matter? I think that having it 'officially' (instead of confirmed by Guido in (private, I assume) talks) would be a significant factor in determining if PR should be accepted or rejected.

@sigmavirus24
Member

I'm rather comfortable with this. Thanks for submitting this @rogalski and thanks for the input @spaceone and @ambv :)

@sigmavirus24 sigmavirus24 merged commit d7d6154 into PyCQA:master Jan 1, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment