PyCQA / flake8-bugbear

A plugin for Flake8 finding likely bugs and design problems in your program. Contains warnings that don't belong in pyflakes and pycodestyle.
MIT License
1.05k stars 103 forks source link

add b040: exception with note added not reraised or used #477

Closed jakkdl closed 1 week ago

jakkdl commented 2 weeks ago

Fixes #474

There's some alternatives when it comes to the scope of this check, namely whether these two should be caught

1
def foo():
  my_exc = ValueError()
  my_exc.add_note("I want to use this variable")
  # ... but then completely forgets to raise

I.e. arbitrary exception created anywhere, that appears to only be used for having a note added. This one gets tricky as its complexity approaches that of a type checker, which usually handles unused variables. If we assume that only exceptions will ever have a method called add_note we can sidestep trying to do any type inference. Though this would hit false alarms with stuff like

def my_add_note(e: Exception) -> None:
  e.add_note("foo")

so it probably isn't worth it.

2
try:
  ...
except:
  f = ValueError()
  f.add_note("")

This is essentially the same as the first one, except it's happening within an except scope and much more clearly looks like a bug.

Handling either case will require exceptions_tracked to be a full dict, and the logic in a bunch of places to be beefed up a bit. If we don't care to go with that, I will simplify the type (and rename it, it's an awful name atm) of exceptions_tracked and simplify/clean up some code handling it.

Handling the lambda case, or other functions defined inside an except handler, wouldn't be super complicated - but I don't really think it's worth even the slight complexity.

jakkdl commented 1 week ago
  1. I think since it's in an except clause something should be done with the exception there .,.. so it's a bug we should care about.

This one is tricky if we want to avoid a false alarm for

except:
  f = ValueError()
  my_exceptions.append(f)
  f.add_note("")

We don't want to try to detect creation of exceptions, as handling anything other than built-in exceptions would require full-on type tracking. So what we can do is detect when add_note is used on an object. But in this example we would need to write a custom visitor to visit twice to pick up that f is being used because it happens before add_note. I guess doing an ast.walk for .add_note() calls is a decent solution though.

And there's other thorny cases that will raise false alarms even if we limit it to "the add_note is in an ExceptHandler".

my_exc = ValueError()
try:
  do_thing()
except ValueError as e:
  my_exc.add_note(str(e))
raise my_exc

The reason the straightforward case is not vulnerable to this kind of stuff is that the caught exception gets deleted at the end of the exception handler, so we know for sure it's limited to the scope of the ExceptHandler. So unless we're fine with the occasional false alarm I suspect I shouldn't bother trying to handle this case either. Maybe the ruff folks have sufficient infra that it's plausible for them to do so if/when they reimplement it.

cooperlees commented 1 week ago

Hmm, yeah. Some interesting false positives here. I'm happy with not covering this and starting simpler. You do what you want here. I think it's better to start with less false positives and do dedicated PRs handling more complex edge cases in general.

You have merge conflicts now to work out now too.

jakkdl commented 1 week ago

simplified and cleaned up the code, should be ready for final review.