Wemake-python-styleguide: Forbid `raise MyExc()` in except handler without `from`

[uhbif19]

Rule request

Thesis

This is the follow-up to #1184 issue. R100 rule is not covered in WPS.

Reasoning

This example does not see to catch problems with from absence:

try:
    some_dct['bar']
except KeyError:
    raise ValueError()

Also WPS329 doc cite such raise as example of correct code.

[uhbif19]

On realization plan:

1. I think this is Best practice violation
2. Doc must be corrected too

[orsinium]

What's the motivation? Python 3 does Implicit Exception Chaining for this case. The exception always has context except explicit suppressing (PEP-409)

[sobolevn]

Oh, I didn't know that. In this case we can reconsider this task.

[webknjaz]

Also, there's just bare raise w/o args that re-rases exception cleanly and doesn't require that "from" part either.

[sobolevn]

Then it can be closed, thanks everyone!

[uhbif19]

@orsinium @webknjaz @sobolevn

Okay, that's my fault, not to provide an explanation for from. Context is passed for both raise E() and raise E() from e, but where are differences:

1. Traceback messages are different. In the first case, it is ... During handling of the above exception, another exception occurred: ..., and in the second ... The above exception was the direct cause of the following exception: ....
2. Exception instance if the first case constrains only e.__context__, and in the second contains also e.__cause__. I discovered it in REPL.

So, as I understand plain re-raise is for situations, where something unexpected happened during handling. So from-style re-raise must be used for marking that nothing unforeseen happened.

[webknjaz]

So from-style re-raise must be used for regular situations.

This really depends on the intent of the code writer and the message they want to convey.
Enforcing "the direct cause" style is wrong when it's not meant to be used.

[uhbif19]

@webknjaz I can not come up with cases, then plain rase in expect is required. Can you give some examples?

[webknjaz]

Sometimes you don't want to process an exception on the current layer but want to log it there. In this case, you'd use a bare raise

def func2():
    try:
        do_something()
    except Exc1:
        log_it()
        raise


def func1():
    try:
        func2()
    except Exc1 as exc:
        actually_process_the_exception(exc)

Another case would be filtering out certain exception attributes. Like when an OSError happens, sometimes you want to only ignore one error code but you still want to pass-through other instances.

import errno

try:
    access_a_file()
except OSError as os_err:
    if os_err.errno != errno.ENOENT:  # we don't care if that thing does not exist but any other error mustn't be tollerated
        raise
    actually_process_the_exception(os_err)

[uhbif19]

@webknjaz That is right, but it is just a different case. This issue is about raise E() vs raise E() from e scenario. raise works differently and has correct usecases.

So I am looking for examples where raise E() is being used in except block. That would mean something unexpected happened, during exception handling (by raise E() semantics), but it was literally coded in the same block, so could not be unexpected by programmer.

[uhbif19]

Clarified title.

[webknjaz]

Ah, so I parsed it wrong. But this case also doesn't really need enforcement. Implicit chaining already works great. Explicit cause specification is useful when you want to skip a few exceptions in the chain to better clarify the root cause:

try:
    try:
        try:
            try:
                do_smth()
            except ExcT1 as exc1:
                raise
        except ExcT2:
            raise
    except ExcT3:
        raise
except ExcT4:
    raise NewExc from exc1

Or if you want to hide the chain completely:

try:
    try:
        try:
            try:
                do_smth()
            except ExcT1:
                raise
        except ExcT2:
            raise
    except ExcT3:
        raise
except ExcT4:
    raise NewExc from None

[uhbif19]

@webknjaz As I understand PEP 3134, it suggests using raise ... from for any intentional re-raise.

Example code from PEP confirming this:

class DatabaseError(Exception):
    pass

class FileDatabase(Database):
    def __init__(self, filename):
        try:
            self.file = open(filename)
        except IOError, exc:
            raise DatabaseError('failed to open') from exc

Also, quote:

To handle the unexpected raising of a secondary exception, the exception must be retained implicitly. To support intentional translation of an exception, there must be a way to chain exceptions explicitly. This PEP addresses both.

Also, another small argument, is that funcy library uses raise ... from for their re-raise decorator:

https://github.com/Suor/funcy/blob/master/funcy/flow.py#L108
https://github.com/Suor/funcy/blob/master/funcy/compat.py#L34

On the other side, I understand, that this is a minor change affecting only a traceback message. It may be not necessary to follow PEP in this case.

But in my feelings the different traceback message may improve DX for re-raising code users.

[webknjaz]

Well, I think it should only be used when the dev wants to signal a direct dependency between exceptions. Adding such a rule would introduce more copy-paste work, hence it'll make the DX of the original developer (not the user of their lib) worse.