flake8-bugbear icon indicating copy to clipboard operation
flake8-bugbear copied to clipboard

B001 misses some bare excepts

Open minusworld opened this issue 5 years ago • 15 comments

B001 and Flake8/pycodestyle E722 both check for bare_except, but B001 misses a lot of cases that E722 finds.

We noticed this when we tried running an internal tool that looks for overlaps in checks -- we are writing some of our own and don't want to overlap -- and found that every time B001 fires E722 also fires; but the reverse is not true.

Tool output: B001 (6786) <-> E722 (34093): 13572 occurred at same line+path B001 implies E722: 100% E722 implies B001: 19%

I took a look at the implementations for B001 and E722 and found that bugbear uses the AST and E722 uses a regex:

Bugbear implementation uses AST.

def visit_ExceptHandler(self, node):
    if node.type is None:
        self.errors.append(B001(node.lineno, node.col_offset))
    self.generic_visit(node)

Flake8 implementation uses regex.

def bare_except(logical_line, noqa):
    if noqa:
        return

    regex = re.compile(r"except\s*:")
    match = regex.match(logical_line)
    if match:
        yield match.start(), "E722 do not use bare 'except'"

From the implementation, it looks like B001 and E722 should hit the same locations every time.

We have a platform that lets us run static analysis over a bunch of open source repositories at the same time, so I ran vanilla flake8 and bugbear at the same time to see if there was a pattern, but one wasn't immediately obvious.

I thought this might be related to forgetting to call visit or something like that (I've been bitten by that before!) but the reason for this disparity wasn't clear to me... so I'm making this issue. Feel free to reach out to me if you have any other questions!

Here are some examples:

image

image

image

minusworld avatar Nov 19 '19 00:11 minusworld

Version

> flake8 --version
3.7.9 (flake8-bugbear: 19.8.0, mccabe: 0.6.1, pycodestyle: 2.5.0, pyflakes: 2.1.1) CPython 3.7.5 on Darwin

minusworld avatar Nov 19 '19 00:11 minusworld

This is a very interesting find! Looks like we might not be visiting certain paths through the tree. If you could create a minimal example with this, that'd be helpful to test against and fix.

ambv avatar Nov 19 '19 08:11 ambv

Here's a repo you can clone with some examples in it.

https://github.com/minusworld/flake8-B001-examples

The three files used were pulled from: https://raw.githubusercontent.com/bponsler/roslaunch_to_dot/92e1515bd2a8fa70dd41149e761183819f7ad532/tests/run_tests.py https://raw.githubusercontent.com/mozilla/release-services/7639c9b67bd3076348e0e682af6e2c77738ab07b/src/tooltool/client/tooltool.py https://github.com/BitconFeng/Deep-Feature-video/blob/fff73fbcd0e21d5db566d2b63c644e18b2732551/lib/dataset/imdb.py#L244

Let me know if there's anything else that would be helpful!

minusworld avatar Nov 19 '19 22:11 minusworld

The problem is your code is in Python 2 syntax. For instance, you have print statements without parenthesis, old style except clauses, and tuple expansion in function arguments. The file in the screenshot is https://raw.githubusercontent.com/caktux/pytrader/b45b216dab3db78d6028d85e9a6f80419c22cea0/balancer.py Bugbear is developed for Python 3, which uses the Python 3 ast module, which cannot parse Python 2. Flake8 skips bugbear entirely because it is registered as an ast_plugins plugin. So, this is a won't fix.

joaoe avatar Jan 18 '20 04:01 joaoe

Then why does it have a whole section of Python 3 warnings? That's confusing. Can you elaborate on which parts don't work with Python 2? And shouldn't it just use whichever AST module is available in the running python interpreter? Or does each flake8 plugin run in its own interpreter?

DylanYoung avatar Jul 03 '20 19:07 DylanYoung

Then why does it have a whole section of Python 3 warnings?

For when the syntax works in Python 3 but you have code using stuff that is from python 2, e.g., migrating or old habits.

joaoe avatar Jul 03 '20 20:07 joaoe

Many of the things those checks validate don't work in Python 3, so that doesn't really explain anything. In fact, none of them work in Python 3 except B303, right?

If those spurious checks can't be removed, it'd be nice if the README loudly proclaimed that it doesn't work in Python2 (I guess we just call it PyPy2 now since the EOL of CPython2, lol).

DylanYoung avatar Jul 03 '20 21:07 DylanYoung

Maybe it's time to just drop our "limited" Python 2 support all together. It's past midpoint 2020.

cooperlees avatar Jul 06 '20 14:07 cooperlees

+1

ambv avatar Jul 06 '20 14:07 ambv

+1

DylanYoung avatar Jul 07 '20 16:07 DylanYoung

Many of the things those checks validate don't work in Python 3, so that doesn't really explain anything. In fact, none of them work in Python 3 except B303, right?

All of them do not work in Python 3. That's the point, to warn about Python 2 code that does not work in Python 3, despite having the correct syntax.

joaoe avatar Jul 07 '20 16:07 joaoe

@joaoe And how long is any project in that state? It's an intermediate state that lasts maybe a day for most projects (if that) and there are existing tools that handle it better. That's precisely my point. I agree with @cooperlees.

DylanYoung avatar Jul 07 '20 16:07 DylanYoung

And how long is any project in that state?

As long as they don't use a tool like bugbear to fix the issues :) And as long as developers don't make mistakes.

joaoe avatar Jul 07 '20 17:07 joaoe

@cooperlees - time to close the issue, since Python 2 support has been dropped?

Zac-HD avatar Aug 17 '21 01:08 Zac-HD

Nothing has been done to complete this. I think we need to remove these checks and all B30X too right? https://github.com/PyCQA/flake8-bugbear/issues/177

I guess we should make a plan and get this shipped.

cooperlees avatar Aug 17 '21 03:08 cooperlees

Closed by #182 I think 🙂

Zac-HD avatar Nov 30 '22 06:11 Zac-HD