Opened 8 years ago

Closed 7 years ago

Last modified 5 years ago

#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 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 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 jdemeyer 7 years ago.
trac_11310.patch (47.7 KB) - added by vbraun 7 years ago.
Rediffed patch
11310_psage.patch (878 bytes) - added by jdemeyer 7 years ago.
Additional patch

Download all attachments as: .zip

Change History (36)

comment:1 Changed 8 years ago by kini

  • Cc kini added

comment:2 Changed 7 years ago by ppurka

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 7 years ago by kini

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 jdemeyer

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 jdemeyer

  • Authors set to Jeroen Demeyer
  • Description modified (diff)

comment:6 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:7 follow-up: Changed 7 years ago by 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 7 years ago by ppurka

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 7 years ago by 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 follow-ups: Changed 7 years ago by 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.

comment:11 in reply to: ↑ 10 Changed 7 years ago by ppurka

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 7 years ago by kini

  • 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 jdemeyer

  • Description modified (diff)

comment:14 Changed 7 years ago by jdemeyer

  • 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 jdemeyer

  • Description modified (diff)

comment:16 Changed 7 years ago by kini

Patchbot says trailing whitespace :P

comment:17 Changed 7 years ago by ppurka

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 ppurka

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 kini

  • 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 jdemeyer

comment:20 Changed 7 years ago by jdemeyer

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

Last edited 7 years ago by jdemeyer (previous) (diff)

comment:21 Changed 7 years ago by kini

  • Dependencies set to #11143

comment:22 Changed 7 years ago by jdemeyer

  • Dependencies #11143 deleted

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

comment:23 Changed 7 years ago by vbraun

  • Dependencies set to #13109
  • Status changed from positive_review to needs_work

Changed 7 years ago by vbraun

Rediffed patch

comment:24 Changed 7 years ago by vbraun

  • Authors changed from Jeroen Demeyer to Jeroen Demeyer, Volker Braun
  • 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 jdemeyer

  • Milestone changed from sage-5.2 to sage-pending

comment:26 Changed 7 years ago by vbraun

  • Milestone changed from sage-pending to sage-5.3

comment:27 Changed 7 years ago by jdemeyer

  • Description modified (diff)

Changed 7 years ago by jdemeyer

Additional patch

comment:28 Changed 7 years ago by jdemeyer

  • 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 jdemeyer

  • Status changed from needs_work to needs_review

comment:30 Changed 7 years ago by vbraun

  • 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 ppurka

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 jdemeyer

  • Merged in set to sage-5.3.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:33 Changed 5 years ago by ohanar

This causes issues with Python 3. See #15620.

Note: See TracTickets for help on using tickets.