Monkey-patch catchall except: statements so they at least don't catch KeyboardInterrupt errors

[jdemeyer]

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 attachment: 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 attachment: trac_11310.patch and attachment: 11310_auto.patch and attachment: 11310_psage.patch.

Depends on #13109

CC: @kini

Component: misc

Author: Jeroen Demeyer, Volker Braun

Reviewer: Keshav Kini

Merged: sage-5.3.beta1

Issue created by migration from https://trac.sagemath.org/ticket/11310

[ppurka]

comment:2

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

[kini]

comment:3

Tests pass on 5.0. Jeroen, is this ready for review, or do you want to add more stuff?

[jdemeyer]

comment:4

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

[jdemeyer]

Author: Jeroen Demeyer

[kini]

comment:7

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.

[ppurka]

comment:8

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.

[kini]

comment:9

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.

[jdemeyer]

comment:10

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.

[ppurka]

comment:11

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.

[kini]

comment:12

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" :)

[jdemeyer]

comment:14

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

[kini]

comment:16

Patchbot says trailing whitespace :P

[ppurka]

comment:17

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

[ppurka]

comment:18

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.

[kini]

comment:19

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

[kini]

Reviewer: Keshav Kini

[vbraun]

Rediffed patch

[vbraun]

Changed author from Jeroen Demeyer to Jeroen Demeyer, Volker Braun

[vbraun]

comment:24

Attachment: trac_11310.patch.gz

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

[jdemeyer]

Additional patch

[jdemeyer]

comment:28

Attachment: 11310_psage.patch.gz

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

[vbraun]

comment:30

Looks good!

In a perfect world ExceptionPexpect would derive from StandardError and not the raw exceptions, but oh well.

[ppurka]

comment:31

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.

[jdemeyer]

Merged: sage-5.3.beta1

[ohanar]

comment:33

This causes issues with Python 3. See #15620.