#11310 closed enhancement (fixed)
Monkey-patch catchall `except:` statements so they at least don't catch `KeyboardInterrupt` errors
Reported by: | Jeroen Demeyer | Owned by: | Jason Grout |
---|---|---|---|
Priority: | minor | Milestone: | sage-5.3 |
Component: | misc | Keywords: | |
Cc: | Keshav 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 12 years ago by
Cc: | Keshav Kini added |
---|
comment:2 Changed 11 years ago by
comment:3 Changed 11 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 11 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 10 years ago by
Authors: | → Jeroen Demeyer |
---|---|
Description: | modified (diff) |
comment:6 Changed 10 years ago by
Description: | modified (diff) |
---|
comment:7 follow-up: 8 Changed 10 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 Changed 10 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 10 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 10 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 Changed 10 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 Changed 10 years ago by
Summary: | Get rid of some bare "except:" statements → 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 10 years ago by
Description: | modified (diff) |
---|
comment:14 Changed 10 years ago by
Milestone: | sage-5.1 → sage-5.2 |
---|---|
Status: | new → needs_review |
Okay, I changed a lot of things by hand, needs_review.
comment:15 Changed 10 years ago by
Description: | modified (diff) |
---|
comment:17 Changed 10 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 10 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 10 years ago by
Reviewers: | → Keshav Kini |
---|---|
Status: | needs_review → positive_review |
Patchbot says it's fine over the whole library. Looks good to me.
Changed 10 years ago by
Attachment: | 11310_auto.patch added |
---|
comment:20 Changed 10 years ago by
Rebased to #11143 (where one of the except:
statements was removed).
comment:21 Changed 10 years ago by
Dependencies: | → #11143 |
---|
comment:22 Changed 10 years ago by
Dependencies: | #11143 |
---|
Not really, I just removed a hunk to resolve the conflict.
comment:23 Changed 10 years ago by
Dependencies: | → #13109 |
---|---|
Status: | positive_review → needs_work |
comment:24 Changed 10 years ago by
Authors: | Jeroen Demeyer → Jeroen Demeyer, Volker Braun |
---|---|
Description: | modified (diff) |
Status: | needs_work → positive_review |
The new patch just fixes some fuzz 2 with #13109, back to positive review.
comment:25 Changed 10 years ago by
Milestone: | sage-5.2 → sage-pending |
---|
comment:26 Changed 10 years ago by
Milestone: | sage-pending → sage-5.3 |
---|
comment:27 Changed 10 years ago by
Description: | modified (diff) |
---|
comment:28 Changed 10 years ago by
Description: | modified (diff) |
---|---|
Status: | positive_review → needs_work |
Additional patch 11310_psage.patch needs review (there were sporadic doctest failures in psage.py
).
comment:29 Changed 10 years ago by
Status: | needs_work → needs_review |
---|
comment:30 Changed 10 years ago by
Status: | needs_review → positive_review |
---|
Looks good!
In a perfect world ExceptionPexpect
would derive from StandardError
and not the raw exceptions, but oh well.
comment:31 Changed 10 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 10 years ago by
Merged in: | → sage-5.3.beta1 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
What's the problem with this ticket? I think it is a small enough change that it can be at least set for review.