Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ second usage. Save the result to a list if the result is needed multiple times.

**B036**: Found ``except BaseException:`` without re-raising (no ``raise`` in the top-level of the ``except`` block). This catches all kinds of things (Exception, SystemExit, KeyboardInterrupt...) and may prevent a program from exiting as expected.

**B037**: Found ``return <value>``, ``yield``, ``yield <value>``, or ``yield from <value>`` in class ``__init__()`` method. No values should be returned or yielded, only bare ``return``s are ok.
Opinionated warnings
~~~~~~~~~~~~~~~~~~~~

Expand Down
32 changes: 31 additions & 1 deletion bugbear.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from __future__ import annotations

import ast
import builtins
import itertools
Expand Down Expand Up @@ -379,6 +381,30 @@ def node_stack(self):
context, stack = self.contexts[-1]
return stack

def in_class_init(self) -> bool:
return (
len(self.contexts) >= 2
and isinstance(self.contexts[-2].node, ast.ClassDef)
and isinstance(self.contexts[-1].node, ast.FunctionDef)
and self.contexts[-1].node.name == "__init__"
)

def visit_Return(self, node: ast.Return) -> None:
if self.in_class_init():
if node.value is not None:
self.errors.append(B037(node.lineno, node.col_offset))
self.generic_visit(node)

def visit_Yield(self, node: ast.Yield) -> None:
if self.in_class_init():
self.errors.append(B037(node.lineno, node.col_offset))
self.generic_visit(node)

def visit_YieldFrom(self, node: ast.YieldFrom) -> None:
if self.in_class_init():
self.errors.append(B037(node.lineno, node.col_offset))
self.generic_visit(node)

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

Expand Down Expand Up @@ -540,7 +566,7 @@ def visit_FunctionDef(self, node):
self.check_for_b906(node)
self.generic_visit(node)

def visit_ClassDef(self, node):
def visit_ClassDef(self, node: ast.ClassDef):
self.check_for_b903(node)
self.check_for_b021(node)
self.check_for_b024_and_b027(node)
Expand Down Expand Up @@ -1986,6 +2012,10 @@ def visit_Lambda(self, node):
message="B036 Don't except `BaseException` unless you plan to re-raise it."
)

B037 = Error(
message="B037 Class `__init__` methods must not return or yield and any values."
)

# Warnings disabled by default.
B901 = Error(
message=(
Expand Down
33 changes: 33 additions & 0 deletions tests/b037.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@

class A:
def __init__(self) -> None:
return 1 # bad

class B:
def __init__(self, x) -> None:
if x:
return # ok
else:
return [] # bad

class BNested:
def __init__(self) -> None:
yield # bad


class C:
def func(self):
pass

def __init__(self, k="") -> None:
yield from [] # bad


class D(C):
def __init__(self, k="") -> None:
super().__init__(k)
return None # bad
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can see people disliking this due to

Explicit is better than implicit

In the Zen of Python.

Can we just check if a None is being explicitly returned and not error in that case since we allow return ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it's easy enough to do if you prefer, but my argument against allowing return None is that, while return is functionally equivalent to return None in python, the latter should only be used in cases where you might return something (like a C function that returns either a valid pointer or NULL). Whereas just bare return is ok, when used as a early exit point from a function (like a C void function).

some quick examples:

def get_thing() -> Thing | None:
    """return a thing if possible"""
    for thing in things:
        if condition:
            return thing
    return None  # this line could be omitted, but functions that have a non-empty return
                 # in some case should have non-empty return in all cases (pylint would 
                 # complain about this for example)

vs

class Example:
    def __init__(self, size, do_extra_stuff = False):
        self.x = ...

        if not do_extra_stuff:
            return  # early exit. Could do `return None` but that implies
                    # that we might return something somewhere else?

        # do some more stuff

let me know what you think, thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that it's better to lint against return None.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is __init__ only, I can live with it. I'll have code to 'fix' cause of it.


class E:
def __init__(self) -> None:
yield "a"
15 changes: 15 additions & 0 deletions tests/test_bugbear.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
B034,
B035,
B036,
B037,
B901,
B902,
B903,
Expand Down Expand Up @@ -619,6 +620,20 @@ def test_b036(self) -> None:
)
self.assertEqual(errors, expected)

def test_b037(self) -> None:
filename = Path(__file__).absolute().parent / "b037.py"
bbc = BugBearChecker(filename=str(filename))
errors = list(bbc.run())
expected = self.errors(
B037(4, 8),
B037(11, 12),
B037(15, 12),
B037(23, 8),
B037(29, 8),
B037(33, 8),
)
self.assertEqual(errors, expected)

def test_b908(self):
filename = Path(__file__).absolute().parent / "b908.py"
bbc = BugBearChecker(filename=str(filename))
Expand Down