#11310 closed enhancement (fixed)
Monkey-patch catchall `except:` statements so they at least don't catch `KeyboardInterrupt` errors
Reported by: | jdemeyer | Owned by: | jason |
---|---|---|---|
Priority: | minor | Milestone: | sage-5.3 |
Component: | misc | Keywords: | |
Cc: | kini | Merged in: | sage-5.3.beta1 |
Authors: | Jeroen Demeyer, Volker Braun | Reviewers: | Keshav Kini |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #13109 | Stopgaps: |
Description (last modified by )
There are a lot places in the Sage library which use
except:
This is bad because it catches non-errors like KeyboardInterrupt
.
Then the patch 11310_auto.patch has ben auto-generated to replace all instances of
except:
by
except StandardError:
and all instances of
raise Exception
by
raise RuntimeError
Apply trac_11310.patch and 11310_auto.patch and 11310_psage.patch.
Attachments (3)
Change History (36)
comment:1 Changed 8 years ago by
- Cc kini added
comment:2 Changed 7 years ago by
comment:3 Changed 7 years ago by
Tests pass on 5.0. Jeroen, is this ready for review, or do you want to add more stuff?
comment:4 Changed 7 years ago by
I would like to fix the following from devel/sage/sage/structure/parent.pyx
, starting at line 2577:
try: return super(Parent, self)._an_element_() except EmptySetError: raise except: _record_exception() pass try: return self.gen(0) except: _record_exception() pass try: return self.gen() except: _record_exception() pass
comment:5 Changed 7 years ago by
- Description modified (diff)
comment:6 Changed 7 years ago by
- Description modified (diff)
comment:7 follow-up: ↓ 8 Changed 7 years ago by
Changing these bare except statements to except StandardError
seems like not enough, don't you think? Except statements should always be written to accept the narrowest possible expected exception class only.
comment:8 in reply to: ↑ 7 Changed 7 years ago by
Replying to kini:
Changing these bare except statements to
except StandardError
seems like not enough, don't you think? Except statements should always be written to accept the narrowest possible expected exception class only.
Then what should it be? According to the hierarchy of exceptions I think StandardError
encapsulates most of what can go wrong. It is at least better than a blank except:
.
About the RuntimeError
s I think some of them should be ValueError
and I spotted one which should be an AttributeError
.
comment:9 Changed 7 years ago by
I mean that the code should be inspected and the appropriate exception class should be used in each case. Of course, this is not easy, considering that the autogenerated patch is more than 100 KB. But still, it should be done.
comment:10 follow-ups: ↓ 11 ↓ 12 Changed 7 years ago by
I'm currently going though the auto-generated patch and fixing the obvious things by hand. Note that the title of this ticket isn't "totally fix all exceptions inside Sage".
The main point is to not catch KeyboardInterrupt
errors.
comment:11 in reply to: ↑ 10 Changed 7 years ago by
Replying to jdemeyer:
The main point is to not catch
KeyboardInterrupt
errors.
Other than some of the RuntimeError
s, I think the patch is fine, if the objective was this.
comment:12 in reply to: ↑ 10 Changed 7 years ago by
- Summary changed from Get rid of some bare "except:" statements to Monkey-patch catchall `except:` statements so they at least don't catch `KeyboardInterrupt` errors
Replying to jdemeyer:
I'm currently going though the auto-generated patch and fixing the obvious things by hand. Note that the title of this ticket isn't "totally fix all exceptions inside Sage".
The main point is to not catch
KeyboardInterrupt
errors.
Well, the title is a bit misleading I guess. Sure, it's not "totally fix all exceptions inside Sage", but it does seem like "totally fix some exceptions inside Sage" :)
comment:13 Changed 7 years ago by
- Description modified (diff)
comment:14 Changed 7 years ago by
- Milestone changed from sage-5.1 to sage-5.2
- Status changed from new to needs_review
Okay, I changed a lot of things by hand, needs_review.
comment:15 Changed 7 years ago by
- Description modified (diff)
comment:16 Changed 7 years ago by
Patchbot says trailing whitespace :P
comment:17 Changed 7 years ago by
Hmm.. a lot of doctests are failing:
The following tests failed: sage -t --long -force_lib devel/sage/doc/en/thematic_tutorials/lie/weight_ring.rst # 2 doctests failed sage -t --long -force_lib devel/sage/sage/rings/polynomial/pbori.pyx # 3 doctests failed sage -t --long -force_lib devel/sage/sage/combinat/free_module.py # 3 doctests failed sage -t --long -force_lib devel/sage/sage/combinat/posets/poset_examples.py # 1 doctests failed sage -t --long -force_lib devel/sage/sage/combinat/root_system/weyl_characters.py # 11 doctests failed sage -t --long -force_lib devel/sage/sage/matrix/matrix_double_dense.pyx # 1 doctests failed sage -t --long -force_lib devel/sage/sage/modules/vector_double_dense.pyx # 1 doctests failed sage -t --long -force_lib devel/sage/sage/graphs/isgci.py # 2 doctests failed sage -t --long -force_lib devel/sage/sage/graphs/generic_graph.py # 3 doctests failed sage -t --long -force_lib devel/sage/sage/graphs/graph_decompositions/rankwidth.pyx # 1 doctests failed sage -t --long -force_lib devel/sage/sage/graphs/graph_generators.py # 2 doctests failed sage -t --long -force_lib devel/sage/sage/interfaces/chomp.py # 3 doctests failed sage -t --long -force_lib devel/sage/sage/interfaces/expect.py # 1 doctests failed sage -t --long -force_lib devel/sage/sage/interfaces/psage.py # Time out sage -t --long -force_lib devel/sage/sage/homology/simplicial_complex_morphism.py # 8 doctests failed sage -t --long -force_lib devel/sage/sage/homology/delta_complex.py # 5 doctests failed sage -t --long -force_lib devel/sage/sage/homology/cell_complex.py # 5 doctests failed sage -t --long -force_lib devel/sage/sage/homology/simplicial_complex_homset.py # 7 doctests failed sage -t --long -force_lib devel/sage/sage/homology/simplicial_complex.py # 33 doctests failed sage -t --long -force_lib devel/sage/sage/homology/examples.py # 8 doctests failed
comment:18 Changed 7 years ago by
I think something changed in the patches between the last time I ran make ptestlong
(I had to leave it running and go home) and the new patches uploaded 12h ago. At present, running the doctest on only these few files gives no errors.
comment:19 Changed 7 years ago by
- Reviewers set to Keshav Kini
- Status changed from needs_review to positive_review
Patchbot says it's fine over the whole library. Looks good to me.
Changed 7 years ago by
comment:20 Changed 7 years ago by
Rebased to #11143 (where one of the except:
statements was removed).
comment:21 Changed 7 years ago by
- Dependencies set to #11143
comment:22 Changed 7 years ago by
- Dependencies #11143 deleted
Not really, I just removed a hunk to resolve the conflict.
comment:23 Changed 7 years ago by
- Dependencies set to #13109
- Status changed from positive_review to needs_work
comment:24 Changed 7 years ago by
- Description modified (diff)
- Status changed from needs_work to positive_review
The new patch just fixes some fuzz 2 with #13109, back to positive review.
comment:25 Changed 7 years ago by
- Milestone changed from sage-5.2 to sage-pending
comment:26 Changed 7 years ago by
- Milestone changed from sage-pending to sage-5.3
comment:27 Changed 7 years ago by
- Description modified (diff)
comment:28 Changed 7 years ago by
- Description modified (diff)
- Status changed from positive_review to needs_work
Additional patch 11310_psage.patch needs review (there were sporadic doctest failures in psage.py
).
comment:29 Changed 7 years ago by
- Status changed from needs_work to needs_review
comment:30 Changed 7 years ago by
- Status changed from needs_review to positive_review
Looks good!
In a perfect world ExceptionPexpect
would derive from StandardError
and not the raw exceptions, but oh well.
comment:31 Changed 7 years ago by
psage is a weird beast. I have seen doctest errors in psage which change from version to version. I couldn't explain it, nor do I know the fix. See #12061.
comment:32 Changed 7 years ago by
- Merged in set to sage-5.3.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
comment:33 Changed 6 years ago by
This causes issues with Python 3. See #15620.
What's the problem with this ticket? I think it is a small enough change that it can be at least set for review.