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

Exempt parameters with immutable annotations from B006 #821

Merged
merged 1 commit into from
Nov 20, 2022
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
20 changes: 20 additions & 0 deletions resources/test/fixtures/B006_B008.py
Original file line number Diff line number Diff line change
Expand Up @@ -185,3 +185,23 @@ def nested_b008(a=random.randint(0, dt.datetime.now().year)):
# Ignore lambda contents since they are evaluated at call time.
def foo(f=lambda x: print(x)):
f(1)


from collections import abc
from typing import Annotated, Dict, Optional, Sequence, Union, Set


def immutable_annotations(
a: Sequence[int] | None = [],
b: Optional[abc.Mapping[int, int]] = {},
c: Annotated[Union[abc.Set[str], abc.Sized], "annotation"] = set(),
):
pass


def mutable_annotations(
a: list[int] | None = [],
b: Optional[Dict[int, int]] = {},
c: Annotated[Union[Set[str], abc.Sized], "annotation"] = set(),
):
pass
165 changes: 139 additions & 26 deletions src/flake8_bugbear/plugins/mutable_argument_default.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
use fnv::{FnvHashMap, FnvHashSet};
use rustpython_ast::{Arguments, Expr, ExprKind};
use rustpython_ast::{Arguments, Constant, Expr, ExprKind, Operator};

use crate::ast::helpers::{collect_call_paths, dealias_call_path, match_call_path};
use crate::ast::types::Range;
use crate::check_ast::Checker;
use crate::checks::{Check, CheckKind};

const MUTABLE_FUNCS: [(&str, &str); 7] = [
const MUTABLE_FUNCS: &[(&str, &str)] = &[
("", "dict"),
("", "list"),
("", "set"),
Expand All @@ -16,6 +16,47 @@ const MUTABLE_FUNCS: [(&str, &str); 7] = [
("collections", "deque"),
];

const IMMUTABLE_TYPES: &[(&str, &str)] = &[
("", "bool"),
("", "bytes"),
("", "complex"),
("", "float"),
("", "frozenset"),
("", "int"),
("", "object"),
("", "range"),
("", "str"),
("collections.abc", "Sized"),
("typing", "LiteralString"),
("typing", "Sized"),
];

const IMMUTABLE_GENERIC_TYPES: &[(&str, &str)] = &[
("", "tuple"),
("collections.abc", "ByteString"),
("collections.abc", "Collection"),
("collections.abc", "Container"),
("collections.abc", "Iterable"),
("collections.abc", "Mapping"),
("collections.abc", "Reversible"),
("collections.abc", "Sequence"),
("collections.abc", "Set"),
("typing", "AbstractSet"),
("typing", "ByteString"),
("typing", "Callable"),
("typing", "Collection"),
("typing", "Container"),
("typing", "FrozenSet"),
("typing", "Iterable"),
("typing", "Literal"),
("typing", "Mapping"),
("typing", "Never"),
("typing", "NoReturn"),
("typing", "Reversible"),
("typing", "Sequence"),
("typing", "Tuple"),
];

pub fn is_mutable_func(
expr: &Expr,
from_imports: &FnvHashMap<&str, FnvHashSet<&str>>,
Expand All @@ -27,34 +68,106 @@ pub fn is_mutable_func(
.any(|(module, member)| match_call_path(&call_path, module, member, from_imports))
}

fn is_mutable_expr(
expr: &Expr,
from_imports: &FnvHashMap<&str, FnvHashSet<&str>>,
import_aliases: &FnvHashMap<&str, &str>,
) -> bool {
match &expr.node {
ExprKind::List { .. }
| ExprKind::Dict { .. }
| ExprKind::Set { .. }
| ExprKind::ListComp { .. }
| ExprKind::DictComp { .. }
| ExprKind::SetComp { .. } => true,
ExprKind::Call { func, .. } => is_mutable_func(func, from_imports, import_aliases),
_ => false,
}
}

fn is_immutable_annotation(
expr: &Expr,
from_imports: &FnvHashMap<&str, FnvHashSet<&str>>,
import_aliases: &FnvHashMap<&str, &str>,
) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

Nice.

match &expr.node {
ExprKind::Name { .. } | ExprKind::Attribute { .. } => {
let call_path = dealias_call_path(collect_call_paths(expr), import_aliases);
IMMUTABLE_TYPES
.iter()
.chain(IMMUTABLE_GENERIC_TYPES)
.any(|(module, member)| match_call_path(&call_path, module, member, from_imports))
}
ExprKind::Subscript { value, slice, .. } => {
let call_path = dealias_call_path(collect_call_paths(value), import_aliases);
if IMMUTABLE_GENERIC_TYPES
.iter()
.any(|(module, member)| match_call_path(&call_path, module, member, from_imports))
{
true
} else if match_call_path(&call_path, "typing", "Union", from_imports) {
if let ExprKind::Tuple { elts, .. } = &slice.node {
elts.iter()
.all(|elt| is_immutable_annotation(elt, from_imports, import_aliases))
} else {
false
}
} else if match_call_path(&call_path, "typing", "Optional", from_imports) {
is_immutable_annotation(slice, from_imports, import_aliases)
} else if match_call_path(&call_path, "typing", "Annotated", from_imports) {
if let ExprKind::Tuple { elts, .. } = &slice.node {
elts.first().map_or(false, |elt| {
is_immutable_annotation(elt, from_imports, import_aliases)
})
} else {
false
}
} else {
false
}
}
ExprKind::BinOp {
left,
op: Operator::BitOr,
right,
} => {
is_immutable_annotation(left, from_imports, import_aliases)
&& is_immutable_annotation(right, from_imports, import_aliases)
}
ExprKind::Constant {
value: Constant::None,
..
} => true,
_ => false,
}
}

/// B006
pub fn mutable_argument_default(checker: &mut Checker, arguments: &Arguments) {
for expr in arguments
.defaults
// Scan in reverse order to right-align zip()
for (arg, default) in arguments
.kwonlyargs
.iter()
.chain(arguments.kw_defaults.iter())
.rev()
.zip(arguments.kw_defaults.iter().rev())
.chain(
arguments
.args
.iter()
.rev()
.chain(arguments.posonlyargs.iter().rev())
.zip(arguments.defaults.iter().rev()),
)
{
match &expr.node {
ExprKind::List { .. }
| ExprKind::Dict { .. }
| ExprKind::Set { .. }
| ExprKind::ListComp { .. }
| ExprKind::DictComp { .. }
| ExprKind::SetComp { .. } => {
checker.add_check(Check::new(
CheckKind::MutableArgumentDefault,
Range::from_located(expr),
));
}
ExprKind::Call { func, .. } => {
if is_mutable_func(func, &checker.from_imports, &checker.import_aliases) {
checker.add_check(Check::new(
CheckKind::MutableArgumentDefault,
Range::from_located(expr),
));
}
}
_ => {}
if is_mutable_expr(default, &checker.from_imports, &checker.import_aliases)
&& arg.node.annotation.as_ref().map_or(true, |expr| {
!is_immutable_annotation(expr, &checker.from_imports, &checker.import_aliases)
})
{
checker.add_check(Check::new(
CheckKind::MutableArgumentDefault,
Range::from_located(default),
));
}
}
}
24 changes: 24 additions & 0 deletions src/snapshots/ruff__linter__tests__B006_B006_B008.py.snap
Original file line number Diff line number Diff line change
Expand Up @@ -90,4 +90,28 @@ expression: checks
row: 170
column: 48
fix: ~
- kind: MutableArgumentDefault
location:
row: 203
column: 26
end_location:
row: 203
column: 28
fix: ~
- kind: MutableArgumentDefault
location:
row: 204
column: 34
end_location:
row: 204
column: 36
fix: ~
- kind: MutableArgumentDefault
location:
row: 205
column: 61
end_location:
row: 205
column: 66
fix: ~