Check bare raise occurs in except clause

[jayvdb]

Fixes lp:1528539

[jayvdb]

https://bugs.launchpad.net/pyflakes/+bug/1528539

[myint]

Any thoughts on the below case? I haven't seen/written code like this before, but it seems like valid code that would result in a false positive from pyflakes with this pull request.

def foo():
    raise


try:
    raise Exception()
except:
    foo()

[bitglue]

On one hand, I can't find anything in the 2.7 language reference that says raise must occur within an except: block. What it does say is:

If no expressions are present, raise re-raises the last exception that was active in the current scope. If no exception is active in the current scope, a TypeError exception is raised indicating that this is an error

On the other hand, in the example above I'm not sure we could say the exception that is re-raised by raise is "in the current scope". Maybe we could say that the exception was raised in a global scope, so it's in foo's scope, too, except that this code seems equally valid:

def foo():
    raise

def bar():
    try:
        raise Exception()
    except:
        foo()

Certainly in this case, foo and bar do not share the same scope.

Probably it's a reasonable assumption that the behavior of a raise without an exception argument is to re-raise whatever sys.exc_info() returns. And on that subject, the standard library reference says:

This function returns a tuple of three values that give information about the exception that is currently being handled. The information returned is specific both to the current thread and to the current stack frame. If the current stack frame is not handling an exception, the information is taken from the calling stack frame, or its caller, and so on until a stack frame is found that is handling an exception. Here, “handling an exception” is defined as “executing or having executed an except clause.”

In this interpretation, the calling stack frame(s) can determine whether a bare raise is valid or not. Since we can't know those until runtime, I guess Pyflakes can't check validity in this case.

[asmeurer]

What's the difference between node.parent and self.getParent(node)?

[asmeurer]

I ask because CONTINUE below uses node.parent. I can't remember if I didn't use getParent on purpose or not.

[jayvdb]

getParent also skips some parents when if not hasattr(node, 'elts') and not hasattr(node, 'ctx') is not True.

However, not isnt really needed in this case, and inline-ing getParent will like be faster, or at least more clear anyway.

Also, this routine is probably better named has_ancestor_type.

[asmeurer]

What about Python 3. I seem to remember this giving a RuntimeError.

[bitglue]

The only difference in Python 3 I noticed was it raises RuntimeError instead of TypeError in the cases where there's no active exception. Otherwise it's the same: the bare raise in a function called from an except: block raises the exception as 2.7 does.

[asmeurer]

Yeah it looks like it even works at the module scope

rse.py:

print("raising")
raise

test.py:

try:
    raise ValueError
except ValueError:
    import rse

$ python test.py
raising
Traceback (most recent call last):
  File "test.py", line 4, in <module>
    import rse
  File "test.py", line 2, in <module>
    raise ValueError
ValueError

What an obscure Python feature.

[jayvdb]

regarding re-raise in a different scope, I doubt there are going to be many instances of this 'feature' being used in serious code. Any function which re-raises an exception encountered elsewhere is very likely to be using sys.exc_info() somehow.

I would expect anyone using that 'feature' would happily adjust their code to raise *sys.exc_info() (not valid syntax) to workaround this new pyflakes error. i.e. the following is identical, and much clearer.

def fu():
    exc = sys.exc_info()
    raise exc[0], exc[1], exc[2]

I obviously think pyflakes should report insane code, even if it could be legally used. flake8 users can always add a noqa if they need it.

That said, of course it is possible to reduce when this new error occurs.

• re-raise in module scope not in except: clause looks 99.999% an error. An import that re-raises an exception in another module is so bizarre.
• re-raise inside a try: clause is the case I was original facing (https://gerrit.wikimedia.org/r/260557). Again, Python allows this, but I think we shouldnt.

def fu():
    try:
        raise
    except Exception:
        raise

try:
    raise Exception('foo')
except:
    fu()

If pyflakes isnt interested in preventing bare raise in try: clause, IMO this PR should be abandoned or greatly optimised by only handling the very limited module scope re-raises not in a tryexcept block (and I'll create a flake8 plugin instead to do a sane implementation of this check, which over a few years should help surface any strange legal uses).

[asmeurer]

-1 because this would give people the incorrect impression that this sort of thing isn't allowed, when it actually is.

[bitglue]

As a matter of design principle I can't accept a change that emits an error on legitimate code, even if that code is obscure or odd. Thanks for the PR though, I did learn something new about Python.

[myint]

I agree that it doesn't belong in pyflakes. Though, in case anyone does want to use this feature, note that it exists in pylint.

foo.py:

try:
    raise
except:
    pass

$ pylint --errors-only foo.py
************* Module foo
E:  2, 4: The raise statement is not inside an except clause (misplaced-bare-raise)

[jayvdb]

I've checked ~200 projects I have a local copy of. 73 hits in 19 projects:

1. bzr
2. cypthon
3. django
4. epytext
5. google_appengine
6. mercurial
7. peak
8. pip
9. pywin32
10. reportlab
11. roundup
12. setuptools
13. sage
14. sympy
15. trac
16. traits
17. twisted
18. urlgrabber
19. webapp2

There are zero cases of re-raise at module level not in except: clause. IMO this should be considered to be a pyflakes error, in the same way that undefined imports are reported as an error when * is imported - importing * is valid syntax, however it breaks a very fundamental aspect of pyflakes - pyflakes is a module static checker, as cross-module dependencies like re-raising exceptions created in another module is a violation of that.

The only case of bare raise in a try, finally or else blocks of a try except clause is:
https://github.com/enthought/traits/blob/master/traits/util/resource.py#L139
That looks like a bug, which is probably working correctly as it will raise an exception, even if is a new TypeError because there is no current exception to be re-raised, which does get caught by the outer bare except: clause later in the function.

[bitglue]

OK, I concede that's a pretty good argument. I'm reopening the PR for consideration. But I also have to get ready for holiday parties so I can't say much more right now.

I will say one thing that would make me feel better is if this issue were raised on the Python mailing list. I don't have time to do that myself right now. It does very much seem like a thing which works by mistake, which is a bad idea almost always. The language reference is unclear, so I think either the language reference needs clarification, or we've uncovered a bug in Python (or at least something that can be acknowledged as unspecified behavior).

[jayvdb]

Thanks. Bub is awake, so I am also out of time for now at least, and also wont get much time over the next few days. When I get time, I'll first revise the PR to handle those two specific cases only, so we can resume the discussion about those specific cases (including widening that discussion to include the Python mailing list) and can test those specific cases against more repos.