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.06k stars 104 forks source link

Assertion check fails on syntactically-valid Python code #138

Open lensvol opened 3 years ago

lensvol commented 3 years ago

Originally reported here: https://github.com/wemake-services/wemake-python-styleguide/issues/1599#issuecomment-703526278

There is a problem with the case of ast.Tuple containing anything other than class name or ast.Attribute: source code that uses such constructs in except handler actually trips assertion checks inside flake8-bugbear visitors:

Test file:

try:
    pass
except (ValueError, "oops"):
    print(123)

Error output:

❯ flake8 1.py
Traceback (most recent call last):
  File "/Users/lensvol/Library/Caches/pypoetry/virtualenvs/wemake-python-styleguide-py3.7/bin/flake8", line 10, in <module>
    sys.exit(main())
  File "/Users/lensvol/Library/Caches/pypoetry/virtualenvs/wemake-python-styleguide-py3.7/lib/python3.7/site-packages/flake8/main/cli.py", line 22, in main
    app.run(argv)
  File "/Users/lensvol/Library/Caches/pypoetry/virtualenvs/wemake-python-styleguide-py3.7/lib/python3.7/site-packages/flake8/main/application.py", line 360, in run
    self._run(argv)
  File "/Users/lensvol/Library/Caches/pypoetry/virtualenvs/wemake-python-styleguide-py3.7/lib/python3.7/site-packages/flake8/main/application.py", line 348, in _run
    self.run_checks()
  File "/Users/lensvol/Library/Caches/pypoetry/virtualenvs/wemake-python-styleguide-py3.7/lib/python3.7/site-packages/flake8/main/application.py", line 262, in run_checks
    self.file_checker_manager.run()
  File "/Users/lensvol/Library/Caches/pypoetry/virtualenvs/wemake-python-styleguide-py3.7/lib/python3.7/site-packages/flake8/checker.py", line 325, in run
    self.run_serial()
  File "/Users/lensvol/Library/Caches/pypoetry/virtualenvs/wemake-python-styleguide-py3.7/lib/python3.7/site-packages/flake8/checker.py", line 309, in run_serial
    checker.run_checks()
  File "/Users/lensvol/Library/Caches/pypoetry/virtualenvs/wemake-python-styleguide-py3.7/lib/python3.7/site-packages/flake8/checker.py", line 589, in run_checks
    self.run_ast_checks()
  File "/Users/lensvol/Library/Caches/pypoetry/virtualenvs/wemake-python-styleguide-py3.7/lib/python3.7/site-packages/flake8/checker.py", line 496, in run_ast_checks
    for (line_number, offset, text, _) in runner:
  File "/Users/lensvol/Library/Caches/pypoetry/virtualenvs/wemake-python-styleguide-py3.7/lib/python3.7/site-packages/bugbear.py", line 36, in run
    visitor.visit(self.tree)
  File "/Users/lensvol/Library/Caches/pypoetry/virtualenvs/wemake-python-styleguide-py3.7/lib/python3.7/site-packages/bugbear.py", line 156, in visit
    super().visit(node)
  File "/Users/lensvol/.pyenv/versions/3.7.4/lib/python3.7/ast.py", line 262, in visit
    return visitor(node)
  File "/Users/lensvol/.pyenv/versions/3.7.4/lib/python3.7/ast.py", line 270, in generic_visit
    self.visit(item)
  File "/Users/lensvol/Library/Caches/pypoetry/virtualenvs/wemake-python-styleguide-py3.7/lib/python3.7/site-packages/bugbear.py", line 156, in visit
    super().visit(node)
  File "/Users/lensvol/.pyenv/versions/3.7.4/lib/python3.7/ast.py", line 262, in visit
    return visitor(node)
  File "/Users/lensvol/Library/Caches/pypoetry/virtualenvs/wemake-python-styleguide-py3.7/lib/python3.7/site-packages/bugbear.py", line 292, in visit_Try
    self.generic_visit(node)
  File "/Users/lensvol/.pyenv/versions/3.7.4/lib/python3.7/ast.py", line 270, in generic_visit
    self.visit(item)
  File "/Users/lensvol/Library/Caches/pypoetry/virtualenvs/wemake-python-styleguide-py3.7/lib/python3.7/site-packages/bugbear.py", line 156, in visit
    super().visit(node)
  File "/Users/lensvol/.pyenv/versions/3.7.4/lib/python3.7/ast.py", line 262, in visit
    return visitor(node)
  File "/Users/lensvol/Library/Caches/pypoetry/virtualenvs/wemake-python-styleguide-py3.7/lib/python3.7/site-packages/bugbear.py", line 165, in visit_ExceptHandler
    names = [_to_name_str(e) for e in node.type.elts]
  File "/Users/lensvol/Library/Caches/pypoetry/virtualenvs/wemake-python-styleguide-py3.7/lib/python3.7/site-packages/bugbear.py", line 165, in <listcomp>
    names = [_to_name_str(e) for e in node.type.elts]
  File "/Users/lensvol/Library/Caches/pypoetry/virtualenvs/wemake-python-styleguide-py3.7/lib/python3.7/site-packages/bugbear.py", line 130, in _to_name_str
    assert isinstance(node, ast.Attribute)
AssertionError
cooperlees commented 3 years ago

Sorry for my ignorance, but is this valid Python? If I make it raise ValueError, it complains about trying to catch an exception that does not inherit from BaseException, thus a TypeError:

Code:

#!/usr/bin/env python3

try:
    raise ValueError("Bla")
except (ValueError, "oops"):
    print(123)

Error:

cooper-mbp1:~ cooper$ python3 -V
Python 3.7.5
cooper-mbp1:~ cooper$ python3 /tmp/test.py
Traceback (most recent call last):
  File "/tmp/test.py", line 4, in <module>
    raise ValueError("Bla")
ValueError: Bla

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/tmp/test.py", line 5, in <module>
    except (ValueError, "oops"):
TypeError: catching classes that do not inherit from BaseException is not allowed

Can I please see an example that shows how this is valid and actually used please. Otherwise, I don't feel we need to fix this.

lensvol commented 3 years ago

Sorry, I should have been more clear in my report. What I meant is that this snippet is syntactically valid and can be parsed by Python, as shown by your own example.

While this code is semantically incorrect, a user may benefit from having it reported in a more human-friendly manner.

There is also an issue with failed assertion essentially terminating execution of flake8 process, thus preventing other plugins from doing useful work.

lensvol commented 3 years ago

Would you mind if I tried to solve that and did a pull request?

cooperlees commented 3 years ago

If there is a clean way to know you're in an except and you find a str, to just report an error, sure.