flake8-bugbear
flake8-bugbear copied to clipboard
PyCQA
B011 is misleading, and the advice doesn't sound good
B011 Do not call assert False since python -O removes these calls. Instead callers should raise AssertionError().
There are two problems with this:
- Removal of assertions with
-Ois not limited toassert Falsecases, it removes all assertions. So the message is misleading. - The suggestion to replace this assert with
raise AssertionError()is not a good one, because such a construct cannot be disabled by disabling assertions when one explicitly wants to do that.
Given these considerations, I cannot think of a good use case or a fix for B011 in the first place, so I suggest removing it altogether.
I authored this check and would like to understand your concern. I have not seen a use of assert False where the author somehow wanted to disable it via python -O. Could you point me at an example? I also do not understand how the message misleads readers since it only flags for assert False and not for other assert call sites.
Maybe this isn't in perfect alignment with the OP, but I'm curious why we only flag assert False and not all assertions. I only recently learned that using assert to detect a failure mode is an antipattern, and had to remove various assertions for this reason. If bugbear had told me sooner, I might not have put them in to begin with.
Yeah, @rpdelaney's comment is a variant of the problem, although slightly unrelated in the sense that the advice given by this check is quite likely the best possible fix for that case: using AssertionError() would not in my opinion very rarely if ever be an appropriate way to signal failure, there are likely better exception types for that depending on case.
But I'll try to elaborate. So, the current implementation asks to use AssertionError instead of using assert False. The reason it gives for that is that those asserts get removed by -O. But that reason is valid for (well it isn't actually a valid reason IMO, but applies similarly to) all asserts, not just assert False. So, to make the check consistent and not misleading, all asserts should be flagged. But that would not be a good thing to do, because using asserts is a valid pattern, when used properly. Hence I don't think this check is fixable.
I dunno. If you print a warning, then people who understand not to do it can disable the warning. If you don't print the warning, then people don't know they need it.
Sure, but the warning needs to be valid in some scenarios on its own. If this lead you to avoid using asserts to signal failure, that I think was a coincidence, and thus should not count in this check's favor.
Quite bluntly, as it stands here, people who understand this thing will turn it off, and those who don't are subject to make misinformed changes and develop bad practices.
In my code base I have expensive assert sanity checks on data structures which python -O will remove in production. It has check_true, check_not_none, etc. helper functions for preconditions that do not use assert. Thus some of the assert are valid for this code base but assert false should always be replaced with raise AssertionError.
I modeled this check after a similar one in Java error-prone which might explain my motivation more clearly:
http://errorprone.info/bugpattern/AssertFalse
If this lead you to avoid using asserts to signal failure, that I think was a coincidence, and thus should not count in this check's favor.
It certainly didn't -- I didn't use bugbear until very recently. But if I had been using it, and there had been a warning for all assertions, then I would have learned about this antipattern sooner. That's what I'm saying. I think I agree that it's bad to warn only for assert False, but it would be better to warn for all assertions than none.
assert definitely has its place, so warning for every occurrence seems misleading. The entire point of assertions is that they can be switched off. So one actually should use them similar to how @gaul describes it: Have some costly checks that can be disabled in production.
However, -O implies the assertion machinery is switched off. Entirely. That means two things:
- Explicitly raising
AssertionErrordefeats the purpose ofassertbeing optional. That means the suggestion of B011 encourages misuse of assertions. - Explicitly catching
AssertionErrorleads to different-O-behaviour at a distance. It is a better indication of misusing assertions.
A better replacement for assert False is raise NotImplementedError, since it usually indicates a dead code path.
However, I would rather suggest to flag except AssertionError, perhaps flag raise AssertionError as well. They indicate that assertions are misused for control flow, which they are not suitable for.
Thanks, @maxfischer2781 ; I was using assert False in some tests I hadn't written yet (and marking them as xfail), and then hit B011, which led me to this page, and your reminder that raise NotImplementedError() is a better approach in that case.
I like the check (because there is never a need to assert False that I can see), but can certainly get behind modifying the advice ("replace this with by raising an appropriate exception"). What that appropriate exception is seems to be beyond the scope of a tool like this since it's highly dependent on the code and the exception hierarchy in use.
I also like the idea of catching places where AssertionError is caught, but it should be a separate check and disabled by default (it's valid in many cases: e.g. a unit test runner that runs other unit tests and gathers up the errors).
Came here to report basically the same problem. I see this is more controversial than I expected.
Would it be acceptable to have an opinionated version of this warning (B911?) that would flag all assertions outside of tests functions and classes, assuming those are all named with test as a case-insensitive prefix ?
My 2c: I also landed here because I was looking for how to disable this check within our test suite - which uses pytest runner - where using assert is exactly the right way to go (https://docs.pytest.org/en/7.2.x/how-to/assert.html#assert)
