Opened 12 years ago

Closed 10 years ago

Last modified 9 years ago

#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:

Status badges

Description (last modified by Jeroen Demeyer)

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)

11310_auto.patch (107.0 KB) - added by Jeroen Demeyer 10 years ago.
trac_11310.patch (47.7 KB) - added by Volker Braun 10 years ago.
Rediffed patch
11310_psage.patch (878 bytes) - added by Jeroen Demeyer 10 years ago.
Additional patch

Download all attachments as: .zip

Change History (36)

comment:1 Changed 12 years ago by Keshav Kini

Cc: Keshav Kini added

comment:2 Changed 11 years ago by Punarbasu Purkayastha

What's the problem with this ticket? I think it is a small enough change that it can be at least set for review.

comment:3 Changed 11 years ago by Keshav Kini

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 Jeroen Demeyer

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 Jeroen Demeyer

Authors: Jeroen Demeyer
Description: modified (diff)

comment:6 Changed 10 years ago by Jeroen Demeyer

Description: modified (diff)

comment:7 Changed 10 years ago by Keshav 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.

comment:8 in reply to:  7 Changed 10 years ago by Punarbasu Purkayastha

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 RuntimeErrors I think some of them should be ValueError and I spotted one which should be an AttributeError.

comment:9 Changed 10 years ago by Keshav Kini

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 Changed 10 years ago by Jeroen Demeyer

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 10 years ago by Punarbasu Purkayastha

Replying to jdemeyer:

The main point is to not catch KeyboardInterrupt errors.

Other than some of the RuntimeErrors, I think the patch is fine, if the objective was this.

comment:12 in reply to:  10 Changed 10 years ago by Keshav Kini

Summary: Get rid of some bare "except:" statementsMonkey-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 Jeroen Demeyer

Description: modified (diff)

comment:14 Changed 10 years ago by Jeroen Demeyer

Milestone: sage-5.1sage-5.2
Status: newneeds_review

Okay, I changed a lot of things by hand, needs_review.

comment:15 Changed 10 years ago by Jeroen Demeyer

Description: modified (diff)

comment:16 Changed 10 years ago by Keshav Kini

Patchbot says trailing whitespace :P

comment:17 Changed 10 years ago by Punarbasu Purkayastha

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 Punarbasu Purkayastha

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 Keshav Kini

Reviewers: Keshav Kini
Status: needs_reviewpositive_review

Patchbot says it's fine over the whole library. Looks good to me.

Changed 10 years ago by Jeroen Demeyer

Attachment: 11310_auto.patch added

comment:20 Changed 10 years ago by Jeroen Demeyer

Rebased to #11143 (where one of the except: statements was removed).

Last edited 10 years ago by Jeroen Demeyer (previous) (diff)

comment:21 Changed 10 years ago by Keshav Kini

Dependencies: #11143

comment:22 Changed 10 years ago by Jeroen Demeyer

Dependencies: #11143

Not really, I just removed a hunk to resolve the conflict.

comment:23 Changed 10 years ago by Volker Braun

Dependencies: #13109
Status: positive_reviewneeds_work

Changed 10 years ago by Volker Braun

Attachment: trac_11310.patch added

Rediffed patch

comment:24 Changed 10 years ago by Volker Braun

Authors: Jeroen DemeyerJeroen Demeyer, Volker Braun
Description: modified (diff)
Status: needs_workpositive_review

The new patch just fixes some fuzz 2 with #13109, back to positive review.

comment:25 Changed 10 years ago by Jeroen Demeyer

Milestone: sage-5.2sage-pending

comment:26 Changed 10 years ago by Volker Braun

Milestone: sage-pendingsage-5.3

comment:27 Changed 10 years ago by Jeroen Demeyer

Description: modified (diff)

Changed 10 years ago by Jeroen Demeyer

Attachment: 11310_psage.patch added

Additional patch

comment:28 Changed 10 years ago by Jeroen Demeyer

Description: modified (diff)
Status: positive_reviewneeds_work

Additional patch 11310_psage.patch needs review (there were sporadic doctest failures in psage.py).

comment:29 Changed 10 years ago by Jeroen Demeyer

Status: needs_workneeds_review

comment:30 Changed 10 years ago by Volker Braun

Status: needs_reviewpositive_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 Punarbasu Purkayastha

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 Jeroen Demeyer

Merged in: sage-5.3.beta1
Resolution: fixed
Status: positive_reviewclosed

comment:33 Changed 9 years ago by R. Andrew Ohana

This causes issues with Python 3. See #15620.

Note: See TracTickets for help on using tickets.