Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

B904: ensure the raise is in the same context with the except #191

Merged
merged 5 commits into from
Nov 28, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 36 additions & 2 deletions bugbear.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,19 @@
__version__ = "21.9.2"

LOG = logging.getLogger("flake8.bugbear")
CONTEXTFUL_NODES = (
ast.Module,
ast.ClassDef,
ast.AsyncFunctionDef,
ast.FunctionDef,
ast.Lambda,
ast.ListComp,
ast.SetComp,
ast.DictComp,
ast.GeneratorExp,
)

Context = namedtuple("Context", ["node", "stack"])


@attr.s(hash=False)
Expand Down Expand Up @@ -168,6 +181,7 @@ class BugBearVisitor(ast.NodeVisitor):
node_window = attr.ib(default=attr.Factory(list))
errors = attr.ib(default=attr.Factory(list))
futures = attr.ib(default=attr.Factory(set))
context = attr.ib(default=attr.Factory(list))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this all that is needed? How did this ever work if so :|

Suggested change
context = attr.ib(default=attr.Factory(list))
contexts = attr.ib(default=attr.Factory(list))


NODE_WINDOW_SIZE = 4

Expand All @@ -178,13 +192,30 @@ def __getattr__(self, name):
print(name)
return self.__getattribute__(name)

@property
def node_stack(self):
if len(self.contexts) == 0:
return []

context, stack = self.contexts[-1]
return stack

def visit(self, node):
is_contextful = isinstance(node, CONTEXTFUL_NODES)

if is_contextful:
context = Context(node, [])
self.contexts.append(context)

self.node_stack.append(node)
self.node_window.append(node)
self.node_window = self.node_window[-self.NODE_WINDOW_SIZE :]
super().visit(node)
self.node_stack.pop()

if is_contextful:
self.contexts.pop()

def visit_ExceptHandler(self, node):
if node.type is None:
self.errors.append(
Expand Down Expand Up @@ -500,9 +531,12 @@ def check_for_b901(self, node):
break

def check_for_b902(self, node):
if not isinstance(self.node_stack[-2], ast.ClassDef):
if len(self.contexts) < 2 or not isinstance(
self.contexts[-2].node, ast.ClassDef
):
return

cls = self.contexts[-2].node
decorators = NameFinder()
decorators.visit(node.decorator_list)

Expand All @@ -511,7 +545,7 @@ def check_for_b902(self, node):
# `cls`?
return

bases = {b.id for b in self.node_stack[-2].bases if isinstance(b, ast.Name)}
bases = {b.id for b in cls.bases if isinstance(b, ast.Name)}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleaner, but it's not really used anywhere else, but I do agree this makes it easier to know what it is, if you know what cls generally means ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think cls makes a bit more sense than something[-2] (which is a lot less clear).

if "type" in bases:
if (
"classmethod" in decorators.names
Expand Down
31 changes: 31 additions & 0 deletions tests/b904.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,34 @@
raise base_err
finally:
raise Exception("Nothing to chain from, so no warning here")

try:
raise ValueError
except ValueError:
# should not emit, since we are not raising something
def proxy():
raise NameError

try:
from preferred_library import Thing
except ImportError:
try:
from fallback_library import Thing
except ImportError:
class Thing:
def __getattr__(self, name):
# same as the case above, should not emit.
raise AttributeError


try:
from preferred_library import Thing
except ImportError:
try:
from fallback_library import Thing
except ImportError:
def context_switch():
try:
raise ValueError
except ValueError:
raise Exception
1 change: 1 addition & 0 deletions tests/test_bugbear.py
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,7 @@ def test_b904(self):
B904(10, 8),
B904(11, 4),
B904(16, 4),
B904(52, 16),
]
self.assertEqual(errors, self.errors(*expected))

Expand Down