Skip to content

Commit

Permalink
Implement B006 (#515)
Browse files Browse the repository at this point in the history
  • Loading branch information
harupy committed Oct 30, 2022
1 parent d824810 commit 7ecbfe4
Show file tree
Hide file tree
Showing 9 changed files with 361 additions and 1 deletion.
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,7 @@ The 🛠 emoji indicates that a rule is automatically fixable by the `--fix` com
| Code | Name | Message | Fix |
| ---- | ---- | ------- | --- |
| B002 | UnaryPrefixIncrement | Python does not support the unary prefix increment. | |
| B006 | MutableArgumentDefault | Do not use mutable data structures for argument defaults. | |
| B007 | UnusedLoopControlVariable | Loop control variable `i` not used within the loop body. | 🛠 |
| B011 | DoNotAssertFalse | Do not `assert False` (`python -O` removes these calls), raise `AssertionError()` | 🛠 |
| B013 | RedundantTupleInExceptionHandler | A length-one tuple literal is redundant. Write `except ValueError:` instead of `except (ValueError,):`. | |
Expand Down
187 changes: 187 additions & 0 deletions resources/test/fixtures/B006_B008.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,187 @@
import collections
import datetime as dt
import logging
import operator
import random
import re
import time
import types
from operator import attrgetter, itemgetter, methodcaller
from types import MappingProxyType


# B006
# Allow immutable literals/calls/comprehensions
def this_is_okay(value=(1, 2, 3)):
...


async def and_this_also(value=tuple()):
pass


def frozenset_also_okay(value=frozenset()):
pass


def mappingproxytype_okay(
value=MappingProxyType({}), value2=types.MappingProxyType({})
):
pass


def re_compile_ok(value=re.compile("foo")):
pass


def operators_ok(
v=operator.attrgetter("foo"),
v2=operator.itemgetter("foo"),
v3=operator.methodcaller("foo"),
):
pass


def operators_ok_unqualified(
v=attrgetter("foo"),
v2=itemgetter("foo"),
v3=methodcaller("foo"),
):
pass


def kwonlyargs_immutable(*, value=()):
...


# Flag mutable literals/comprehensions


def this_is_wrong(value=[1, 2, 3]):
...


def this_is_also_wrong(value={}):
...


def and_this(value=set()):
...


def this_too(value=collections.OrderedDict()):
...


async def async_this_too(value=collections.defaultdict()):
...


def dont_forget_me(value=collections.deque()):
...


# N.B. we're also flagging the function call in the comprehension
def list_comprehension_also_not_okay(default=[i**2 for i in range(3)]):
pass


def dict_comprehension_also_not_okay(default={i: i**2 for i in range(3)}):
pass


def set_comprehension_also_not_okay(default={i**2 for i in range(3)}):
pass


def kwonlyargs_mutable(*, value=[]):
...


# Recommended approach for mutable defaults
def do_this_instead(value=None):
if value is None:
value = set()


# B008
# Flag function calls as default args (including if they are part of a sub-expression)
def in_fact_all_calls_are_wrong(value=time.time()):
...


def f(when=dt.datetime.now() + dt.timedelta(days=7)):
pass


def can_even_catch_lambdas(a=(lambda x: x)()):
...


# Recommended approach for function calls as default args
LOGGER = logging.getLogger(__name__)


def do_this_instead_of_calls_in_defaults(logger=LOGGER):
# That makes it more obvious that this one value is reused.
...


# Handle inf/infinity/nan special case
def float_inf_okay(value=float("inf")):
pass


def float_infinity_okay(value=float("infinity")):
pass


def float_plus_infinity_okay(value=float("+infinity")):
pass


def float_minus_inf_okay(value=float("-inf")):
pass


def float_nan_okay(value=float("nan")):
pass


def float_minus_NaN_okay(value=float("-NaN")):
pass


def float_infinity_literal(value=float("1e999")):
pass


# But don't allow standard floats
def float_int_is_wrong(value=float(3)):
pass


def float_str_not_inf_or_nan_is_wrong(value=float("3.14")):
pass


# B006 and B008
# We should handle arbitrary nesting of these B008.
def nested_combo(a=[float(3), dt.datetime.now()]):
pass


# Don't flag nested B006 since we can't guarantee that
# it isn't made mutable by the outer operation.
def no_nested_b006(a=map(lambda s: s.upper(), ["a", "b", "c"])):
pass


# B008-ception.
def nested_b008(a=random.randint(0, dt.datetime.now().year)):
pass


# Ignore lambda contents since they are evaluated at call time.
def foo(f=lambda x: print(x)):
f(1)
3 changes: 3 additions & 0 deletions src/check_ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1502,6 +1502,9 @@ where
self.checks
.extend(pyflakes::checks::duplicate_arguments(arguments));
}
if self.settings.enabled.contains(&CheckCode::B006) {
flake8_bugbear::plugins::mutable_argument_default(self, arguments)
}

// Bind, but intentionally avoid walking default expressions, as we handle them upstream.
for arg in &arguments.posonlyargs {
Expand Down
6 changes: 6 additions & 0 deletions src/checks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ pub enum CheckCode {
A003,
// flake8-bugbear
B002,
B006,
B007,
B011,
B013,
Expand Down Expand Up @@ -287,6 +288,7 @@ pub enum CheckKind {
BuiltinAttributeShadowing(String),
// flake8-bugbear
UnaryPrefixIncrement,
MutableArgumentDefault,
UnusedLoopControlVariable(String),
DoNotAssertFalse,
RedundantTupleInExceptionHandler(String),
Expand Down Expand Up @@ -465,6 +467,7 @@ impl CheckCode {
CheckCode::A003 => CheckKind::BuiltinAttributeShadowing("...".to_string()),
// flake8-bugbear
CheckCode::B002 => CheckKind::UnaryPrefixIncrement,
CheckCode::B006 => CheckKind::MutableArgumentDefault,
CheckCode::B007 => CheckKind::UnusedLoopControlVariable("i".to_string()),
CheckCode::B011 => CheckKind::DoNotAssertFalse,
CheckCode::B013 => {
Expand Down Expand Up @@ -655,6 +658,7 @@ impl CheckCode {
CheckCode::A002 => CheckCategory::Flake8Builtins,
CheckCode::A003 => CheckCategory::Flake8Builtins,
CheckCode::B002 => CheckCategory::Flake8Bugbear,
CheckCode::B006 => CheckCategory::Flake8Bugbear,
CheckCode::B007 => CheckCategory::Flake8Bugbear,
CheckCode::B011 => CheckCategory::Flake8Bugbear,
CheckCode::B013 => CheckCategory::Flake8Bugbear,
Expand Down Expand Up @@ -811,6 +815,7 @@ impl CheckKind {
CheckKind::BuiltinAttributeShadowing(_) => &CheckCode::A003,
// flake8-bugbear
CheckKind::UnaryPrefixIncrement => &CheckCode::B002,
CheckKind::MutableArgumentDefault => &CheckCode::B006,
CheckKind::UnusedLoopControlVariable(_) => &CheckCode::B007,
CheckKind::DoNotAssertFalse => &CheckCode::B011,
CheckKind::RedundantTupleInExceptionHandler(_) => &CheckCode::B013,
Expand Down Expand Up @@ -1067,6 +1072,7 @@ impl CheckKind {
}
// flake8-bugbear
CheckKind::UnaryPrefixIncrement => "Python does not support the unary prefix increment. Writing `++n` is equivalent to `+(+(n))`, which equals `n`. You meant `n += 1`.".to_string(),
CheckKind::MutableArgumentDefault => "Do not use mutable data structures for argument defaults.".to_string(),
CheckKind::UnusedLoopControlVariable(name) => format!("Loop control variable `{name}` not used within the loop body. If this is intended, start the name with an underscore."),
CheckKind::DoNotAssertFalse => {
"Do not `assert False` (`python -O` removes these calls), raise `AssertionError()`"
Expand Down
7 changes: 6 additions & 1 deletion src/checks_gen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ pub enum CheckCodePrefix {
B0,
B00,
B002,
B006,
B007,
B01,
B011,
Expand Down Expand Up @@ -247,6 +248,7 @@ impl CheckCodePrefix {
CheckCodePrefix::A003 => vec![CheckCode::A003],
CheckCodePrefix::B => vec![
CheckCode::B002,
CheckCode::B006,
CheckCode::B007,
CheckCode::B011,
CheckCode::B013,
Expand All @@ -256,15 +258,17 @@ impl CheckCodePrefix {
],
CheckCodePrefix::B0 => vec![
CheckCode::B002,
CheckCode::B006,
CheckCode::B007,
CheckCode::B011,
CheckCode::B013,
CheckCode::B014,
CheckCode::B017,
CheckCode::B025,
],
CheckCodePrefix::B00 => vec![CheckCode::B002, CheckCode::B007],
CheckCodePrefix::B00 => vec![CheckCode::B002, CheckCode::B006, CheckCode::B007],
CheckCodePrefix::B002 => vec![CheckCode::B002],
CheckCodePrefix::B006 => vec![CheckCode::B006],
CheckCodePrefix::B007 => vec![CheckCode::B007],
CheckCodePrefix::B01 => vec![
CheckCode::B011,
Expand Down Expand Up @@ -887,6 +891,7 @@ impl CheckCodePrefix {
CheckCodePrefix::B0 => PrefixSpecificity::Hundreds,
CheckCodePrefix::B00 => PrefixSpecificity::Tens,
CheckCodePrefix::B002 => PrefixSpecificity::Explicit,
CheckCodePrefix::B006 => PrefixSpecificity::Explicit,
CheckCodePrefix::B007 => PrefixSpecificity::Explicit,
CheckCodePrefix::B01 => PrefixSpecificity::Tens,
CheckCodePrefix::B011 => PrefixSpecificity::Explicit,
Expand Down
2 changes: 2 additions & 0 deletions src/flake8_bugbear/plugins/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ pub use assert_false::assert_false;
pub use assert_raises_exception::assert_raises_exception;
pub use duplicate_exceptions::duplicate_exceptions;
pub use duplicate_exceptions::duplicate_handler_exceptions;
pub use mutable_argument_default::mutable_argument_default;
pub use redundant_tuple_in_exception_handler::redundant_tuple_in_exception_handler;
pub use unary_prefix_increment::unary_prefix_increment;
pub use unused_loop_control_variable::unused_loop_control_variable;

mod assert_false;
mod assert_raises_exception;
mod duplicate_exceptions;
mod mutable_argument_default;
mod redundant_tuple_in_exception_handler;
mod unary_prefix_increment;
mod unused_loop_control_variable;
62 changes: 62 additions & 0 deletions src/flake8_bugbear/plugins/mutable_argument_default.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
use rustpython_ast::{Arguments, ExprKind};

use crate::ast::types::{CheckLocator, Range};
use crate::check_ast::Checker;
use crate::checks::{Check, CheckKind};

/// B006
pub fn mutable_argument_default(checker: &mut Checker, arguments: &Arguments) {
for expr in arguments
.defaults
.iter()
.chain(arguments.kw_defaults.iter())
{
match &expr.node {
ExprKind::List { .. }
| ExprKind::Dict { .. }
| ExprKind::Set { .. }
| ExprKind::ListComp { .. }
| ExprKind::DictComp { .. }
| ExprKind::SetComp { .. } => {
checker.add_check(Check::new(
CheckKind::MutableArgumentDefault,
checker.locate_check(Range::from_located(expr)),
));
}
ExprKind::Call { func, .. } => match &func.node {
ExprKind::Name { id, .. }
if id == "dict"
|| id == "list"
|| id == "set"
|| id == "Counter"
|| id == "OrderedDict"
|| id == "defaultdict"
|| id == "deque" =>
{
checker.add_check(Check::new(
CheckKind::MutableArgumentDefault,
checker.locate_check(Range::from_located(expr)),
));
}
ExprKind::Attribute { value, attr, .. }
if (attr == "Counter"
|| attr == "OrderedDict"
|| attr == "defaultdict"
|| attr == "deque") =>
{
match &value.node {
ExprKind::Name { id, .. } if id == "collections" => {
checker.add_check(Check::new(
CheckKind::MutableArgumentDefault,
checker.locate_check(Range::from_located(expr)),
));
}
_ => {}
}
}
_ => {}
},
_ => {}
}
}
}
1 change: 1 addition & 0 deletions src/linter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,7 @@ mod tests {
#[test_case(CheckCode::A002, Path::new("A002.py"); "A002")]
#[test_case(CheckCode::A003, Path::new("A003.py"); "A003")]
#[test_case(CheckCode::B002, Path::new("B002.py"); "B002")]
#[test_case(CheckCode::B006, Path::new("B006_B008.py"); "B006")]
#[test_case(CheckCode::B007, Path::new("B007.py"); "B007")]
#[test_case(CheckCode::B011, Path::new("B011.py"); "B011")]
#[test_case(CheckCode::B013, Path::new("B013.py"); "B013")]
Expand Down

0 comments on commit 7ecbfe4

Please sign in to comment.