Permanently get rid of bare except: statements

edd8e884-f507-429a-b577-5d554626c0fe commented 3 years ago

A while ago, @jdemeyer used to regularly take care of removing bare except: statements constantly introduced in Sage source code, se e.g. #27427, #14028, #11310, #21687, #24274.

The goal of this ticket (perhaps should it be a task ticket ?) is to take over the work without self-abnegation by:

removing the existing bare except: statements.
ensure that those will not be reintroduced by adding a dedicated plugin to the patchbot (i have code for it but it could only be merged once the exisiting bare except: statements are removed).

CC: @kliem @frederichan-IMJPRG @videlec

Component: misc

Author: Frédéric Chapoton, Jonathan Kliem, Thierry Monteil

Branch/Commit: de4faf6

Reviewer: Jonathan Kliem, Markus Wageringel

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

fchapoton commented 3 years ago

comment:1

this should rather be done using the pycodestyle linter : code E722

https://pycodestyle.pycqa.org/en/latest/intro.html#error-codes

fchapoton commented 3 years ago

comment:2

see #32680 and src/tox.ini

fchapoton commented 3 years ago

comment:3

but in fact, I would like that the linters first turn both green before switching on the check for E722,

so, let us hope that #32757 will at last give us green linters..

fchapoton commented 3 years ago

comment:4

another aim is to turn on E713 and E714, several recent tickets move on that way : #32766

fchapoton commented 3 years ago

comment:5

Current status of E722:

src/sage/schemes/projective/projective_space.py:1696:21: E722 do not use bare 'except'
src/sage/schemes/berkovich/berkovich_space.py:622:13: E722 do not use bare 'except'
src/sage/schemes/product_projective/rational_point.py:498:13: E722 do not use bare 'except'
src/sage/geometry/polyhedron/base.py:5278:17: E722 do not use bare 'except'
src/sage/geometry/polyhedron/library.py:2723:9: E722 do not use bare 'except'
src/sage/graphs/digraph.py:3355:17: E722 do not use bare 'except'
src/sage/graphs/digraph.py:4000:13: E722 do not use bare 'except'
src/sage/graphs/digraph.py:4217:13: E722 do not use bare 'except'
src/sage/rings/qqbar.py:2653:5: E722 do not use bare 'except'
src/sage/interfaces/polymake.py:365:13: E722 do not use bare 'except'
src/sage/interfaces/polymake.py:1596:9: E722 do not use bare 'except'
src/sage/modules/free_module.py:466:9: E722 do not use bare 'except'
src/sage/modules/matrix_morphism.py:1421:9: E722 do not use bare 'except'
src/sage/modules/matrix_morphism.py:1506:9: E722 do not use bare 'except'
src/sage/dynamics/arithmetic_dynamics/berkovich_ds.py:241:17: E722 do not use bare 'except'
src/sage/dynamics/arithmetic_dynamics/berkovich_ds.py:248:17: E722 do not use bare 'except'
src/sage/dynamics/arithmetic_dynamics/berkovich_ds.py:255:13: E722 do not use bare 'except'
src/sage/dynamics/arithmetic_dynamics/berkovich_ds.py:806:13: E722 do not use bare 'except'
src/sage/dynamics/arithmetic_dynamics/berkovich_ds.py:896:25: E722 do not use bare 'except'
src/sage/dynamics/arithmetic_dynamics/berkovich_ds.py:1081:13: E722 do not use bare 'except'

edd8e884-f507-429a-b577-5d554626c0fe commented 3 years ago

comment:6

Replying to @fchapoton:

Current status of E722:

src/sage/schemes/projective/projective_space.py:1696:21: E722 do not use bare 'except'
src/sage/schemes/berkovich/berkovich_space.py:622:13: E722 do not use bare 'except'
src/sage/schemes/product_projective/rational_point.py:498:13: E722 do not use bare 'except'
src/sage/geometry/polyhedron/base.py:5278:17: E722 do not use bare 'except'
src/sage/geometry/polyhedron/library.py:2723:9: E722 do not use bare 'except'
src/sage/graphs/digraph.py:3355:17: E722 do not use bare 'except'
src/sage/graphs/digraph.py:4000:13: E722 do not use bare 'except'
src/sage/graphs/digraph.py:4217:13: E722 do not use bare 'except'
src/sage/rings/qqbar.py:2653:5: E722 do not use bare 'except'
src/sage/interfaces/polymake.py:365:13: E722 do not use bare 'except'
src/sage/interfaces/polymake.py:1596:9: E722 do not use bare 'except'
src/sage/modules/free_module.py:466:9: E722 do not use bare 'except'
src/sage/modules/matrix_morphism.py:1421:9: E722 do not use bare 'except'
src/sage/modules/matrix_morphism.py:1506:9: E722 do not use bare 'except'
src/sage/dynamics/arithmetic_dynamics/berkovich_ds.py:241:17: E722 do not use bare 'except'
src/sage/dynamics/arithmetic_dynamics/berkovich_ds.py:248:17: E722 do not use bare 'except'
src/sage/dynamics/arithmetic_dynamics/berkovich_ds.py:255:13: E722 do not use bare 'except'
src/sage/dynamics/arithmetic_dynamics/berkovich_ds.py:806:13: E722 do not use bare 'except'
src/sage/dynamics/arithmetic_dynamics/berkovich_ds.py:896:25: E722 do not use bare 'except'
src/sage/dynamics/arithmetic_dynamics/berkovich_ds.py:1081:13: E722 do not use bare 'except'

A basic grep -R "except:" src/sage returns a few more cases:

src/sage/libs/giac/giac.pyx:        except:
src/sage/libs/giac/giac.pyx:          except:
src/sage/libs/giac/giac.pyx:                except:
src/sage/libs/giac/giac.pyx:        except:
src/sage/libs/giac/giac.pyx:         except:
src/sage/libs/giac/giac.pyx:         except:
src/sage/libs/giac/giac.pyx:               except:
src/sage/libs/giac/giac.pyx:         except:
src/sage/libs/giac/giac.pyx:         except:
src/sage/libs/giac/giac.pyx:         except:
src/sage/libs/giac/giac.pyx:         except:
src/sage/libs/giac/giac.pyx:         except:
src/sage/libs/giac/giac.pyx:         except:
src/sage/libs/giac/giac.pyx:         except:
src/sage/libs/giac/giac.pyx:        except:
src/sage/libs/giac/giac.pyx:     #     except:
src/sage/libs/giac/giac.pyx:            except:
src/sage/libs/giac/giac.pyx:        except:
src/sage/libs/giac/giac.pyx:         except:
src/sage/libs/giac/giac.pyx:#    except:
src/sage/libs/giac/giac.pyx:#    except:
src/sage/libs/giac/giac.pyx:#    except:
src/sage/libs/giac/giac.pyx:#    except:
src/sage/libs/giac/giac.pyx:#    except:
src/sage/libs/giac/giac.pyx:         except:
src/sage/libs/giac/giac.pyx:               except:
src/sage/libs/giac/giac.pyx:        except:
src/sage/graphs/digraph.py:                except:
src/sage/graphs/digraph.py:            except:
src/sage/graphs/digraph.py:            except:
src/sage/graphs/graph_decompositions/tree_decomposition.pyx:        except:
src/sage/interfaces/polymake.py:            except:
src/sage/interfaces/polymake.py:        except:
src/sage/geometry/polyhedron/base.py:                except:
src/sage/geometry/polyhedron/library.py:        except:
src/sage/modules/free_module.py:        except:
src/sage/modules/matrix_morphism.py:        except:
src/sage/modules/matrix_morphism.py:        except:
src/sage/schemes/product_projective/rational_point.py:            except:
src/sage/schemes/berkovich/berkovich_space.py:            except:
src/sage/schemes/projective/projective_space.py:                    except:
src/sage/dynamics/arithmetic_dynamics/berkovich_ds.py:                except:
src/sage/dynamics/arithmetic_dynamics/berkovich_ds.py:                except:
src/sage/dynamics/arithmetic_dynamics/berkovich_ds.py:            except:
src/sage/dynamics/arithmetic_dynamics/berkovich_ds.py:            except:
src/sage/dynamics/arithmetic_dynamics/berkovich_ds.py:                        except:
src/sage/dynamics/arithmetic_dynamics/berkovich_ds.py:            except:
src/sage/dynamics/complex_dynamics/mandel_julia_helper.pyx:    except:
src/sage/rings/polynomial/multi_polynomial_libsingular.pyx:                except:
src/sage/rings/polynomial/multi_polynomial_libsingular.pyx:    except:
src/sage/rings/ring_extension_element.pyx:            except:
src/sage/rings/complex_arb.pyx:        except:
src/sage/rings/qqbar.py:    except:
src/sage/plot/plot3d/shapes.pyx:        except:

fchapoton commented 3 years ago

Branch: u/chapoton/32788

fchapoton commented 3 years ago

comment:7

pycodestyle probably only looks at py files

New commits:

db41390 fix most E722 (first pass)

fchapoton commented 3 years ago

Commit: db41390

fchapoton commented 3 years ago

comment:8

after my branch for py files, there remains

git grep -c "except\s*:" src/
src/doc/en/developer/coding_in_python.rst:1
src/sage/dynamics/complex_dynamics/mandel_julia_helper.pyx:1
src/sage/graphs/graph_decompositions/tree_decomposition.pyx:1
src/sage/libs/giac/giac.pyx:27
src/sage/plot/plot3d/shapes.pyx:1
src/sage/rings/complex_arb.pyx:1
src/sage/rings/polynomial/multi_polynomial_libsingular.pyx:2
src/sage/rings/ring_extension_element.pyx:1

EDIT: the one in the doc is explaining that one should not do that..

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

cffd35e no except: in most pyx files

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from db41390 to cffd35e

fchapoton commented 3 years ago

comment:10

new commit for pyx files except the giac.pyx mess.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

279b592 fix typo

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from cffd35e to 279b592

edd8e884-f507-429a-b577-5d554626c0fe commented 3 years ago

comment:12

Replying to @fchapoton:

after my branch for py files, there remains

git grep -c "except\s*:" src/
src/doc/en/developer/coding_in_python.rst:1
src/sage/dynamics/complex_dynamics/mandel_julia_helper.pyx:1
src/sage/graphs/graph_decompositions/tree_decomposition.pyx:1
src/sage/libs/giac/giac.pyx:27
src/sage/plot/plot3d/shapes.pyx:1
src/sage/rings/complex_arb.pyx:1
src/sage/rings/polynomial/multi_polynomial_libsingular.pyx:2
src/sage/rings/ring_extension_element.pyx:1

EDIT: the one in the doc is explaining that one should not do that..

Out of curiosity, did the branch comes from a script ?

edd8e884-f507-429a-b577-5d554626c0fe commented 3 years ago

comment:13

After a diagonal view at the commit, the except Exception: in src/sage/interfaces/polymake.py is dubious, and seems to be a consequence of a try:/except: encompassing a pretty long list of tests, which is not recommended as it might mask bugs, see https://www.python.org/dev/peps/pep-0008/#programming-recommendations

fchapoton commented 3 years ago

comment:14

Replying to @sagetrac-tmonteil:

Replying to @fchapoton:
after my branch for py files, there remains
git grep -c "except\s*:" src/
src/doc/en/developer/coding_in_python.rst:1
src/sage/dynamics/complex_dynamics/mandel_julia_helper.pyx:1
src/sage/graphs/graph_decompositions/tree_decomposition.pyx:1
src/sage/libs/giac/giac.pyx:27
src/sage/plot/plot3d/shapes.pyx:1
src/sage/rings/complex_arb.pyx:1
src/sage/rings/polynomial/multi_polynomial_libsingular.pyx:2
src/sage/rings/ring_extension_element.pyx:1
EDIT: the one in the doc is explaining that one should not do that..
Out of curiosity, did the branch comes from a script ?

Not a script, just my poor old hands and fatiguated brain..

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 279b592 to eb4ae95

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

eb4ae95 add import

edd8e884-f507-429a-b577-5d554626c0fe commented 3 years ago

comment:16

Replying to @fchapoton:

Replying to @sagetrac-tmonteil:

Out of curiosity, did the branch comes from a script ?

Not a script, just my poor old hands and fatiguated brain..

Then i should stop developing for Sage given my extremely weak relative productivity in inspecting such issues.

edd8e884-f507-429a-b577-5d554626c0fe commented 3 years ago

comment:17

Replying to @fchapoton:

new commit for pyx files except the giac.pyx mess.

I am trying to look at it, it seems that all indentation is shifted by one.

edd8e884-f507-429a-b577-5d554626c0fe commented 3 years ago

Changed branch from u/chapoton/32788 to u/tmonteil/32788

edd8e884-f507-429a-b577-5d554626c0fe commented 3 years ago

comment:19

I added 2 commits dealing with giac.pyx to your branch. The first commit only fixes bad indentation that made my editor crazy, the second one fixes most try/except statements (most of them seem just useless).

The last two remaining cases are related with some pari_lock and need to be explored.

New commits:

`bcdb620`	`#32788 : fix intentations for libs/giac/giac.pyx`
`da3e437`	`#32788 : remove useless try/except statements in src/sage/libs/giac/giac.pyx`

edd8e884-f507-429a-b577-5d554626c0fe commented 3 years ago

Changed commit from eb4ae95 to da3e437

edd8e884-f507-429a-b577-5d554626c0fe commented 3 years ago

comment:20

fchapoton commented 3 years ago

Changed commit from da3e437 to be872c1

fchapoton commented 3 years ago

comment:21

Sur ta branche, j'ai ajouté un commit pour corriger les doctests.

New commits:

`e98b6e3`	`Merge branch 'u/tmonteil/32788' in 9.5.beta5`
`be872c1`	`some fixes in giac, schemes, geometry`

fchapoton commented 3 years ago

Changed branch from u/tmonteil/32788 to u/chapoton/32788

kliem commented 3 years ago

comment:22

I hope that makes any sense:

Cleaned up the syntax.
pari_unlock shouldn't haven been called like that, because we are in sig_on ... sig_off and one should not call python functions in that case: https://cysignals.readthedocs.io/en/latest/interrupt.html.
_wrap_gen(result) doesn't belong in the try ... except block, because it has its own error handling (and if it was to raise an error, we would slip into the except block calling sig_off again.
Use finally to call sig_off before exit.
Apparently giac raises RuntimeError in case something goes wrong. So I used that.

New commits:

`fadbade`	`Merge branch 'u/tmonteil/32788' of git://trac.sagemath.org/sage into u/gh-kliem/32788`
`66ec40e`	`codestyle`
`35cab30`	`Merge branch 'u/chapoton/32788' of git://trac.sagemath.org/sage into u/gh-kliem/32788`
`0113b72`	`code style again`
`6c08ce1`	`remove blank except in GIAC call`

kliem commented 3 years ago

Changed commit from be872c1 to 6c08ce1

kliem commented 3 years ago

Changed branch from u/chapoton/32788 to u/gh-kliem/32788

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

`04a1f01`	`fix distinction between two calls`
`9905930`	`remove code duplication`

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 6c08ce1 to 9905930

fchapoton commented 3 years ago

comment:24

bot is morally green, linter is green. Maybe we can stop here and keep further care for giac for another ticket ?

fchapoton commented 3 years ago

comment:25

one should look more into the case of src/sage/interfaces/polymake.py

kliem commented 3 years ago

comment:26

Replying to @fchapoton:

bot is morally green, linter is green. Maybe we can stop here and keep further care for giac for another ticket ?

Sure, I only touched the __call__ function that made trouble here. And I didn't feel like we should keep two copies of the same function, if we modify it anyway. I thought it was entirely the same, but there is a tiny difference, which should be more apparent with the current version. I don't think we should work further on this then necessary.

fchapoton commented 3 years ago

Changed branch from u/gh-kliem/32788 to public/ticket/32788

fchapoton commented 3 years ago

comment:27

new branch on top of u/gh-kliem/32788, where I removed a try/except in singular interface

New commits:

9368c2d remove one try/except in singular interface

fchapoton commented 3 years ago

Changed commit from 9905930 to 9368c2d

edd8e884-f507-429a-b577-5d554626c0fe commented 3 years ago

comment:28

Replying to @fchapoton:
9368c2d	`remove one try/except in singular interface`

+1, this try/except was too wide.

fchapoton commented 3 years ago

comment:29

bot is morally green. Ready to go ?

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 9368c2d to a903f72

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

a903f72 #32788 : ensure bare except: statements will not reappear

edd8e884-f507-429a-b577-5d554626c0fe commented 3 years ago

comment:31

I added E722 to src/tox.ini so that the second part of the ticked get fixed too (using tox as @fchapoton suggested). Regarding comment 7, will the pyx be tested as well ? Otherwise, how to deal with them ?

edd8e884-f507-429a-b577-5d554626c0fe commented 3 years ago

Author: Frédéric Chapoton, Jonathan Kliem, Thierry Monteil

fchapoton commented 3 years ago

comment:33

Les fichiers pyx ne sont pas regardés par tox, il me semble. Tu peux ajouter un plugin au patchbot qui ne regarde que les fichiers pyx. Un mécanisme pour ca est en place dans le patchbot.

fchapoton commented 3 years ago

comment:34

there seems to be something broken in polyhedron..

kliem commented 3 years ago

comment:35

But not by our current ticket, this also appears in other reports of the same bot.

edd8e884-f507-429a-b577-5d554626c0fe commented 3 years ago

comment:36

Indeed, I stopped the patchbot and will see if i can restart it cleanly.

sagemath / sage

Permanently get rid of bare except: statements #32788