Hi guys,

Via this issue, I will attach a patch where I replace all the "except: pass" in the source code by a clear "except Exception: pass".

I attach the patch. Need review.

Thanks

You shouldn't change the grammar the tests, nor the pybench source.
Also, in some places it would probably be interesting to narrow down the exception type more.

I disabled the patch for pybench and the grammar.

Thanks for your review.

FYI, the two are not equivalent.   A bare except is equivalent to BaseException.  Even then, the new code would be slower than the original, so I'm not sure this change should be made at all (it makes the code more verbose, slower, and risks introducing a semantic change).

Also, Guido traditionally tries to discourage across the board changes like this.  Instead, we aim for "holistic refactoring" where code improvements are being made one module at a time by someone who deeply groks the code and is thinking through all the changes.  The problem with across-the-board edits is that they tend to be very shallow "change thing x to thing y" without looking at what the surrounding code is doing or whether the code ever made sense in the first place.

Another part of the risk of semantic change is that we likely do not have specific tests for Exception vs BaseException so it would be easy to make a mistake and not have the test suite catch it.

After more consideration, I'm going to reject the patch because it doesn't really make the code better and may make it worse (changing semantics, slower, etc).   Any such changes should be done holistically as part of a deeper refactoring on an individual module.  

I appreciate the patch effort but would like to avoid code churn for nearly zero benefit.

> FYI, the two are not equivalent.

I don't get your point, the purpose of the change is to get ride of "except: pass" which is *bad*.

> Even then, the new code would be slower than the original,

I don't understand why you are talking about performances here. Ignore SystemExit and KeyboardInterrupt is a huge bug, performances don't matter here.

I don't want to benchmark, but I expect that performances are exactly the same if no exception is raised.

Please don't close the issue, Stéphane is fixing real bug.

I agree that it would be better to split the large patch is shorter parts. Or all changes replacing "except: pass" should be grouped into the same patch.

Replacing "except: <code>; raise" with "except Exception: <code>; raise" is wrong.

> Please don't close the issue, Stéphane is fixing real bug.

He appears to be doing a blanket search and replace without adding tests, without evaluating each change to see if makes sense, and without consulting the original author of each affected piece of code.

We have never received a bug report on any one of these.  If you think any one of them is an actually bug, it should be carefully considered one at a time, evaluating why the bare except was put there in the first place.  

Guido pushes for holistic refactoring for a reason.  It is too easy to introduce bugs into stable code by these kind of wholesale changes.

If you (Victor) want to individually study each one to make sure it is the right thing to do, I would place more faith in the patch.  But as it stands now, reviewing the patch for correctness will take substantially more care and thought than it took to create it in the first place.

I reviewed  issue21259-replace_except_pass-2.patch. Please split this huge patch into smaller patches:

1) remove dummy "except: raise"
2) use os.unlink()
3) replace "except: pass" with more precise exceptions like "except OSError:" => please write multiple small patches for this part
4) generic change "except: pass" to "except Exception: pass"

You might open a new issue for these 4 kind of patches (4 issues).

> 3) replace "except: pass" with more precise exceptions like "except OSError:" => please write multiple small patches for this part

You may even open a new issue just for "except: pass" => "except OSError: pass"

@Raymond: To give you more context, Stéphane is sprinting at Pycon on Pycon. I suggested him to fix all bare "except: pass". His first patch is wrong, but ignoring any exception is even worse.

> If you (Victor) want to individually study each one to make sure it is the right thing to do, I would place more faith in the patch. But as it stands now, reviewing the patch for correctness will take substantially more care and thought than it took to create it in the first place.

Agreed. I would like to help Stéphane to fix these issues. I agree
that the huge patch must be splitted, I just explained how the patch
should be splitted and how Stéphane can provide more useful patches.

Several areas for attention:
* Changes to the test suite probably should not be made
  The user doesn't benefit in any way and you risk
  unintentionally breaking the test suite in a way that
  isn't obvious (we have no tests for the tests themselves
  so changing tests is like refactoring without a safety net).
* Some of the exception handling in IDLE needs to catch
  all exceptions (i.e. SystemExit and KeyboardInterrupt
  are supposed to display tracebacks and not terminate
  IDLE itself).
* Changing the exceptions in threading.py is worrisome.
* The Pdb debugger may have legitimate reasons to catch
  all exceptions (like IDLE, we want don't want to 
  terminate the debugger itself).

In other words, many of the proposed changes should not
be made.  

For the rest, be careful to not change semantics
unintentionally and consider adding tests if you think
there is a real bug.   In cases where there is doubt
about the right thing to do, consider assigning the
original author or maintainer of the code.

If you do go ahead and add “except Exception” clauses, maybe look around at what other exceptions are being handled. The other day I stumbled across this in “tkinter.font” module:

try:
    ...
except (KeyboardInterrupt, SystemExit):
    raise
except Exception:
    pass

It would have been simpler (and semantically equivalent) to write

try:
    ...
except Exception:
    pass

This looks like a duplicate of issue 16261.

This is definitely a duplicate of #16261 and should be closed at such.

On that issue, in msg202448, I explained that #15313 is about except: in idlelib and idlelib must be patched separately for the reason Raymond repeated (point 2). The other reason (point 1) is to keep different versions the same as much as possible.

I agree that bare excepts should eventually be replaced -- but on a case by case basis to the right specific exception. A bare except is easy to grep for. Once converted to something else, it is no longer visible as needing attention.

Just committed smaller patch in issue16261 and closed the issue.

Not all changes in the patch in this issue look innocent and correct.

A couple of years ago, when I pushed 'except: pass', I was told in post-review that grandfathered bad code is no excuse for more bad code and that I should be explicit, including if I actually meant "except BaseException:", which in this case I did.  No other developer said otherwise.  I took the above to be the general consensus. I agree with it, one reason being that bare excepts are speed bumps when reading someone else's code.

Victor> generic change "except: pass" to "except Exception: pass"
This is not correct without case-by-case examination.

#16261 had 2 patches.  The patch for doc examples changed 3 'except:'s to 'except Exception:'  I believe these are correct, or correct enough.  They all need to *not* catch KeyboardInterrupt.  The patch for lib code never changed to Exception, but something tighter.

The patch committed for #16261 patched 7 files (down from the original proposed 11).  Even that took a couple of years to get a second review.  I think further followup patches should probably change even fewer files and be attached to new, narrowly focused issues.

Raymond, since you closed this once, and since no new patch has been submitted, I presume you do not mind if I reclose this.

> I presume you do not mind if I reclose this.

Thanks, please do reclose this.