search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.3k stars 447 forks source link

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

Closed jdemeyer closed 12 years ago

jdemeyer commented 13 years ago

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 commented 12 years ago
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 commented 12 years ago
comment:3

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

jdemeyer commented 12 years ago
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 commented 12 years ago

Author: Jeroen Demeyer

jdemeyer commented 12 years ago

Description changed:

--- 
+++ 
@@ -1 +1,31 @@
+There are a lot places in the Sage library which use

+```
+except:
+```
+This is bad because it catches non-errors like `KeyboardInterrupt` and `StopIteration`.
+
+The patch [attachent:11310_auto.patch] has ben auto-generated by replacing all instances of
+
+```
+except:
+```
+by
+
+```
+except StandardError:
+```
+and all instances of
+
+```
+raise Exeption
+```
+by
+
+```
+raise RuntimeError
+```
+
+Then the patch [attachment: 11310.patch](https://github.com/sagemath/sage/files/ticket11310/11310.patch.gz) does some small miscellaneous fixes.
+
+**Apply** all patches.
jdemeyer commented 12 years ago

Description changed:

--- 
+++ 
@@ -5,7 +5,9 @@

This is bad because it catches non-errors like KeyboardInterrupt and StopIteration.

-The patch [attachent:11310_auto.patch] has ben auto-generated by replacing all instances of +--- + +The patch attachment: 11310_auto.patch has ben auto-generated by replacing all instances of

 except:
@@ -18,7 +20,7 @@
 and all instances of

-raise Exeption +raise Exception

 by

@@ -28,4 +30,6 @@

 Then the patch [attachment: 11310.patch](https://github.com/sagemath/sage/files/ticket11310/11310.patch.gz) does some small miscellaneous fixes.

-**Apply** all patches.
+---
+
+**Apply** [attachment: 11310.patch](https://github.com/sagemath/sage/files/ticket11310/11310.patch.gz) and [attachment: 11310_auto.patch](https://github.com/sagemath/sage-prod/files/10652810/11310_auto.patch.gz)
kini commented 12 years ago
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 commented 12 years ago
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 commented 12 years ago
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 commented 12 years ago
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 commented 12 years ago
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 commented 12 years ago
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 commented 12 years ago

Description changed:

--- 
+++ 
@@ -3,7 +3,7 @@

except:

-This is bad because it catches non-errors like `KeyboardInterrupt` and `StopIteration`.
+This is bad because it catches non-errors like `KeyboardInterrupt`.

 ---
jdemeyer commented 12 years ago
comment:14

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

jdemeyer commented 12 years ago

Description changed:

--- 
+++ 
@@ -7,7 +7,9 @@

 ---

-The patch [attachment: 11310_auto.patch](https://github.com/sagemath/sage-prod/files/10652810/11310_auto.patch.gz) has ben auto-generated by replacing all instances of
+Then the patch [attachment: 11310.patch](https://github.com/sagemath/sage/files/ticket11310/11310.patch.gz) fixes some of these.
+
+Then the patch [attachment: 11310_auto.patch](https://github.com/sagemath/sage-prod/files/10652810/11310_auto.patch.gz) has ben auto-generated to replace all remaining instances of

except: @@ -28,8 +30,6 @@ raise RuntimeError


-Then the patch [attachment: 11310.patch](https://github.com/sagemath/sage/files/ticket11310/11310.patch.gz) does some small miscellaneous fixes.
-
 ---

 **Apply** [attachment: 11310.patch](https://github.com/sagemath/sage/files/ticket11310/11310.patch.gz) and [attachment: 11310_auto.patch](https://github.com/sagemath/sage-prod/files/10652810/11310_auto.patch.gz)
kini commented 12 years ago
comment:16

Patchbot says trailing whitespace :P

ppurka commented 12 years ago
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 commented 12 years ago
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 commented 12 years ago
comment:19

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

kini commented 12 years ago

Reviewer: Keshav Kini

jdemeyer commented 12 years ago

Attachment: 11310_auto.patch.gz

jdemeyer commented 12 years ago
comment:20

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

kini commented 12 years ago

Dependencies: #11143

jdemeyer commented 12 years ago
comment:22

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

jdemeyer commented 12 years ago

Changed dependencies from #11143 to none

vbraun commented 12 years ago

Dependencies: #13109

vbraun commented 12 years ago

Rediffed patch

vbraun commented 12 years ago

Description changed:

--- 
+++ 
@@ -32,4 +32,4 @@

 ---

-**Apply** [attachment: 11310.patch](https://github.com/sagemath/sage/files/ticket11310/11310.patch.gz) and [attachment: 11310_auto.patch](https://github.com/sagemath/sage-prod/files/10652810/11310_auto.patch.gz)
+**Apply** [attachment: trac_11310.patch](https://github.com/sagemath/sage-prod/files/10652811/trac_11310.patch.gz) and [attachment: 11310_auto.patch](https://github.com/sagemath/sage-prod/files/10652810/11310_auto.patch.gz)
vbraun commented 12 years ago

Changed author from Jeroen Demeyer to Jeroen Demeyer, Volker Braun

vbraun commented 12 years ago
comment:24

Attachment: trac_11310.patch.gz

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

jdemeyer commented 12 years ago

Description changed:

--- 
+++ 
@@ -7,8 +7,7 @@

 ---

-Then the patch [attachment: 11310.patch](https://github.com/sagemath/sage/files/ticket11310/11310.patch.gz) fixes some of these.
-
+The patch [attachment: trac_11310.patch](https://github.com/sagemath/sage-prod/files/10652811/trac_11310.patch.gz) fixes some of these.
 Then the patch [attachment: 11310_auto.patch](https://github.com/sagemath/sage-prod/files/10652810/11310_auto.patch.gz) has ben auto-generated to replace all remaining instances of
jdemeyer commented 12 years ago

Additional patch

jdemeyer commented 12 years ago

Description changed:

--- 
+++ 
@@ -7,8 +7,7 @@

 ---

-The patch [attachment: trac_11310.patch](https://github.com/sagemath/sage-prod/files/10652811/trac_11310.patch.gz) fixes some of these.
-Then the patch [attachment: 11310_auto.patch](https://github.com/sagemath/sage-prod/files/10652810/11310_auto.patch.gz) has ben auto-generated to replace all remaining instances of
+Then the patch [attachment: 11310_auto.patch](https://github.com/sagemath/sage-prod/files/10652810/11310_auto.patch.gz) has ben auto-generated to replace all instances of

except: @@ -31,4 +30,4 @@

-Apply attachment: trac_11310.patch and attachment: 11310_auto.patch +Apply attachment: trac_11310.patch and attachment: 11310_auto.patch and attachment: 11310_psage.patch.

jdemeyer commented 12 years ago
comment:28

Attachment: 11310_psage.patch.gz

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

vbraun commented 12 years ago
comment:30

Looks good!

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

ppurka commented 12 years ago
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 commented 12 years ago

Merged: sage-5.3.beta1

ohanar commented 10 years ago
comment:33

This causes issues with Python 3. See #15620.