Add B904 - Use raise ... from err in except clauses

[Zac-HD]

Closes #180, by adding a new check B018 B904 to recommend exception chaining. For example,

try:
    assert False
except AssertionError:
    raise RuntimeError  # B904: Within an except clause, use `raise ... from err` ...

This turned out to be remarkably easy to add to the existing code 😁

[cooperlees]

Looks good - Thanks!

My only questions are (And this is open to others):

• Should this be optional?
• Should this be only on >= 3.9 (Is that when this feature was introduced?)

Also, what triggers W503? I'm guessing some code you didn't add ...

• Can we # nova: W503 them so we can look to clean them up?

[cooperlees]

Why add this? Just your personal preference? I usually follow it and move the operator.

[Zac-HD]

Following black's recommendation - W503 is actually disabled by default, but enabled for bugbear by the W in select = B,C,E,F,W,B9.

I updated the config because I also added the first case of this pattern, in check_for_b018() here, and black insists on that placement of the operator.

[hukkin]

Maybe use extend-ignore instead of ignore here to not override the defaults?

W503 for instance is off by default unless defaults are overridden by ignore configuration.

[cooperlees]

Do we need to care to make this only care if we're >= 3.9?

Suggested change

	**B018**: Within an ``except`` clause, raise exceptions with ``raise ... from err``
	or ``raise ... from None`` to distinguish them from errors in exception handling.
	See [the exception chaining tutorial](https://docs.python.org/3/tutorial/errors.html#exception-chaining)
	for details.
	**B018**: Within an ``except`` clause, raise exceptions with ``raise ... from err``
	or ``raise ... from None`` to distinguish them from errors in exception handling (i.e. prefer exception chaining).
	See [the exception chaining tutorial](https://docs.python.org/3/tutorial/errors.html#exception-chaining)
	for details.

[Zac-HD]

Exception chaining was added in Python 3.0! It's just not in widespread use yet, due to the syntatic incompatibility with Python 2 😅

[cooperlees]

O - I went to docs here: https://docs.python.org/3.9/tutorial/errors.html#exception-chaining

• Set it to 3.8 and this section disappears ...

[Zac-HD]

Wow, I guess it's not been well documented... the reference docs still don't have a heading for it, though ctrl-f chain does find some mentions.

[cooperlees]

Will give it till the morning pacific - If no one says anything against this I'll merge.

[Zac-HD]

I've added a cheeky extra clause to allow raise some_lowercase_name, because both Hypothesis and Trio use this pattern when constructing an exception object with a specific traceback - and it seems unlikely to silence the warning on code which is of concern.

[hukkin]

Should this be optional?

I do think that the from e syntax should in theory always be used when raising within an exception handler. In practice however, I find myself too lazy to write out the extra characters, as the benefit is very little, especially if the re-raised exception will be caught later on and never bubbles up to a traceback. Sometimes the added characters make the line long enough that it needs to be wrapped in more than one line making the expression slightly less readable.

Just some reasoning why making this optional should perhaps be considered. Personally don't feel strongly either way.

[Zac-HD]

Fair points @hukkin - you can always disable it in global config though, and IMO if something is off by default it might as well not exist for most users 😕

[hukkin]

Yeah I agree that off-by-default warnings are not much use.

Thinking a bit more about this from a library developer perspective, there's two types of errors that will be raised:

• Errors that are raised when a downstream developer (consumer of the library) makes incorrect use of the API --> we want the best traceback possible
• Errors that are handled internally within the library and should never be seen by downstream developers --> we don't care about the traceback

What would be great is if this warning could only be applied to the first error type, but that isn't possible really. So I feel the question is which one is more important: having a warning for the first error type, or not needlessly annoying developers raising errors of the second error type.

Application developers also may find this annoying as they should generally always shut down gracefully and never let an error bubble all the way up.

Then there's the argument that all software has bugs, so any error may bubble up even though not intended to do so...

I think I'm starting to lean more towards "this is probably more annoying than useful".

[Zac-HD]

Here's the diff in Hypothesis, which has plenty of both examples:

Patched	from err	from None
user-facing	2	6
internal	4	1

Personally I think that even the internal changes are net-positive, in that the code now expresses that these exceptions were raised because of rather than while handling the previous error, and that the user-facing improvements are enough to be worth some inconvenience anyway.

It's easy to turn off the check if you don't like it, but exception chaining is amazingly under-used simply because very few people know it exists and it wasn't viable for libraries until we stopped supporting Python 2 - even just saying "hey, this exists" is useful!

[cooperlees]

I for one, am in the haven't used it bucket. I need to sit down and really understand when it's valuable and worth using etc. (If anyone has great tutorial they recommend shoot them my way or it might even be worth adding to the readme here)

I've enjoyed this discussion. But now I'm torn with default or optional. Any people passing by I'd love your thoughts before we merge this. I'll give it another few days. I'm leaning towards the default. Why:

• This reminds me of B015 (pointless comparison) in that people loved it found unittests doing nothing and we received thanks (which is rare for an OSS project) for it!
• I feel this check will open people up to making libraries now with more helpful exceptions etc.
• As you've both stated, disabling is easy in config
  • Thanks to things like black most people have .flake8 configs now ...

[hukkin]

I think the best way to understand this is to try out the three different ways of exception chaining:

Disable chaining

>>> try:
...     raise Exception("one")
... except Exception:
...     raise Exception("two") from None
... 
Traceback (most recent call last):
  File "<stdin>", line 4, in <module>
Exception: two

Implicit chaining

>>> try:
...     raise Exception("one")
... except Exception:
...     raise Exception("two")
... 
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
Exception: one

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 4, in <module>
Exception: two

Explicit chaining

>>> try:
...     raise Exception("one")
... except Exception as e:
...     raise Exception("two") from e
... 
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
Exception: one

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<stdin>", line 4, in <module>
Exception: two

Why I think the feature isn't that widely used (in addition to Python 2 compat) is that the default (implicit chaining) is pretty good: it doesn't hide information. I fear that rather than receiving thanks, we may receive complaints for an overly opinionated check with this one.

[cooperlees]

I think the best way to understand this is to try out the three different ways of exception chaining:

Disable chaining

>>> try:
...     raise Exception("one")
... except Exception:
...     raise Exception("two") from None
... 
Traceback (most recent call last):
  File "<stdin>", line 4, in <module>
Exception: two

Implicit chaining

>>> try:
...     raise Exception("one")
... except Exception:
...     raise Exception("two")
... 
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
Exception: one

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 4, in <module>
Exception: two

Explicit chaining

>>> try:
...     raise Exception("one")
... except Exception as e:
...     raise Exception("two") from e
... 
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
Exception: one

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<stdin>", line 4, in <module>
Exception: two

Why I think the feature isn't that widely used (in addition to Python 2 compat) is that the default (implicit chaining) is pretty good: it doesn't hide information. I fear that rather than receiving thanks, we may receive complaints for an overly opinionated check with this one.

I kind of also like the default behavior and am leaning towards hukkin's points now. So we have people each way here. What do we want to do? I think there is noise here. We could make it optional for people who want it @Zac-HD ?

[Zac-HD]

I'd still prefer it on-by-default, but you're the maintainer - if you want it optional, I'll make it optional 🙂

[cooperlees]

I would love a second maintainer (all thought most are not active these days) thoughts if any swing by ....

[Zac-HD]

@asottile @The-Compiler @graingert - do you think we should recommend raise ... from err by default, or only if specifically enabled?

[asottile]

some questions:

• without from you still get exception chaining, but with from it changes the message slightly right?
• how does this interact with from_traceback?
• will individuals respond to the error incorrectly and slap from None on things? (many times the correct response is raise e => raise)

(I actually don't think I have the commit bit here, but stop by from time to time so feel free to discount my questions / opinions)

[The-Compiler]

To get an idea of the impact, I ran this over both qutebrowser (~160 instances) and pytest (~300 instances). Both probably wouldn't profit too much from the new warning: qutebrowser because it's an application so it hardly matters; pytest due to how it's in charge of how it prints its own tracebacks anyways.

I guess flake8-bugbear users appreciate a more nitpicky linter compared to "plain" flake8 users, but on the other side that's often a reason people don't like pylint, because despite all configurability, at some point it drives people away if a linter produces more noise than signal by default.

I suppose it boils down to discoverability vs. a low-noise default configuration. For some projects I suspect this will be rather useless as others said, but of course bugbear doesn't know which ones those would be.

FWIW I've also rarely seen raise ... from ... in the wild, so even if useful, it's not exactly a best practice (yet?) I suppose.

I'm avoiding a concrete answer to the question because I really don't know. I'd end up disabling the check and not mind much, but others might feel differently.

[Zac-HD]

OK; that's enough to convince me that disabled-by-default is sensible - and change made, it's now B904.

• without from you still get exception chaining, but with from it changes the message slightly right?

Yep, you get exception changing regardless, from just improves the message (or from None hides earlier tracebacks).

• how does this interact with from_traceback?

If you mean dis.Bytecode.from_traceback() (or BaseException.with_traceback()), it's handled the same way regardless of from. The docs aren't great, but introspection uses the __context__ attribute which isn't affected - from just sets the __cause__.

• will individuals respond to the error incorrectly and slap from None on things? (many times the correct response is raise e => raise)

Quite possibly - though raise e wouldn't trigger this check, since I allow raising an all-lowercase name. This does suggest another new lint rule though...

[cooperlees]

Thanks. I think this will be safer from noise. If people come round wanting this on by default we can look at changing.

Will try update CHANGES etc. + release this week.

[yuwash]

Sorry I’m really not sure about. With the controversy already shown above, I think it should be off by default. It only “improves” the phrasing of the error message a bit although chaining itself happens by default. It seems rather hacky to try to change the wording by introducing such a linter rule rather than making the change to python itself. The referenced part of the docs notably doesn’t say one should always use from. It only says “useful when you are transforming exceptions” whereas I’m not completely sure what counts as “transforming”.