Mirrors/rust-clippy
2
0
Fork 0
mirror of https://github.com/rust-lang/rust-clippy synced 2024-09-20 22:51:58 +00:00
Code Issues Projects Releases Packages Wiki Activity

Auto merge of #5081 - Areredify:vec_box_threshold, r=flip1995

Browse source 
add size parameter for `vec_box`  lint

changelog: add size threshold for the `vec_box` lint, currently 4096 bytes (one page) (subject to change). Closes #3547.

diff is a little bit confusing due to some refactoring (moving free functions to lint struct functions), relevant portion is [this](https://github.com/rust-lang/rust-clippy/compare/master...Areredify:vec_box_threshold?expand=1#diff-1096120ca9143af89dcc9175ea92b54aR294-R298). In hindsight should've been different commits, but oh well.
This commit is contained in:
bors 2020-01-25 16:11:43 +00:00
commit 6b5419412e
10 changed files with 254 additions and 191 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
clippy_lints/src/lib.rs
View file

@ -819,7 +819,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
store.register_late_pass(|| box utils::internal_lints::OuterExpnDataPass);
store.register_late_pass(|| box utils::inspector::DeepCodeInspector);
store.register_late_pass(|| box utils::author::Author);
store.register_late_pass(|| box types::Types);
let vec_box_size_threshold = conf.vec_box_size_threshold;
store.register_late_pass(move || box types::Types::new(vec_box_size_threshold));
store.register_late_pass(|| box booleans::NonminimalBool);
store.register_late_pass(|| box eq_op::EqOp);
store.register_late_pass(|| box enum_glob_use::EnumGlobUse);

386
clippy_lints/src/types.rs
View file

@ -167,7 +167,11 @@ declare_clippy_lint! {
"a borrow of a boxed type"
}
declare_lint_pass!(Types => [BOX_VEC, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX]);
pub struct Types {
vec_box_size_threshold: u64,
}
impl_lint_pass!(Types => [BOX_VEC, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX]);
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Types {
fn check_fn(
@ -186,38 +190,28 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Types {
}
}
check_fn_decl(cx, decl);
self.check_fn_decl(cx, decl);
}
fn check_struct_field(&mut self, cx: &LateContext<'_, '_>, field: &hir::StructField<'_>) {
check_ty(cx, &field.ty, false);
self.check_ty(cx, &field.ty, false);
}
fn check_trait_item(&mut self, cx: &LateContext<'_, '_>, item: &TraitItem<'_>) {
match item.kind {
TraitItemKind::Const(ref ty, _) | TraitItemKind::Type(_, Some(ref ty)) => check_ty(cx, ty, false),
TraitItemKind::Method(ref sig, _) => check_fn_decl(cx, &sig.decl),
TraitItemKind::Const(ref ty, _) | TraitItemKind::Type(_, Some(ref ty)) => self.check_ty(cx, ty, false),
TraitItemKind::Method(ref sig, _) => self.check_fn_decl(cx, &sig.decl),
_ => (),
}
}
fn check_local(&mut self, cx: &LateContext<'_, '_>, local: &Local<'_>) {
if let Some(ref ty) = local.ty {
check_ty(cx, ty, true);
self.check_ty(cx, ty, true);
}
}
}
fn check_fn_decl(cx: &LateContext<'_, '_>, decl: &FnDecl<'_>) {
for input in decl.inputs {
check_ty(cx, input, false);
}
if let FunctionRetTy::Return(ref ty) = decl.output {
check_ty(cx, ty, false);
}
}
/// Checks if `qpath` has last segment with type parameter matching `path`
fn match_type_parameter(cx: &LateContext<'_, '_>, qpath: &QPath<'_>, path: &[&str]) -> bool {
let last = last_path_segment(qpath);
@ -238,54 +232,71 @@ fn match_type_parameter(cx: &LateContext<'_, '_>, qpath: &QPath<'_>, path: &[&st
false
}
/// Recursively check for `TypePass` lints in the given type. Stop at the first
/// lint found.
///
/// The parameter `is_local` distinguishes the context of the type; types from
/// local bindings should only be checked for the `BORROWED_BOX` lint.
#[allow(clippy::too_many_lines)]
fn check_ty(cx: &LateContext<'_, '_>, hir_ty: &hir::Ty<'_>, is_local: bool) {
if hir_ty.span.from_expansion() {
return;
impl Types {
pub fn new(vec_box_size_threshold: u64) -> Self {
Self { vec_box_size_threshold }
}
match hir_ty.kind {
TyKind::Path(ref qpath) if !is_local => {
let hir_id = hir_ty.hir_id;
let res = qpath_res(cx, qpath, hir_id);
if let Some(def_id) = res.opt_def_id() {
if Some(def_id) == cx.tcx.lang_items().owned_box() {
if match_type_parameter(cx, qpath, &paths::VEC) {
span_help_and_lint(
cx,
BOX_VEC,
hir_ty.span,
"you seem to be trying to use `Box<Vec<T>>`. Consider using just `Vec<T>`",
"`Vec<T>` is already on the heap, `Box<Vec<T>>` makes an extra allocation.",
);
return; // don't recurse into the type
}
} else if cx.tcx.is_diagnostic_item(Symbol::intern("vec_type"), def_id) {
if_chain! {
// Get the _ part of Vec<_>
if let Some(ref last) = last_path_segment(qpath).args;
if let Some(ty) = last.args.iter().find_map(|arg| match arg {
GenericArg::Type(ty) => Some(ty),
_ => None,
});
// ty is now _ at this point
if let TyKind::Path(ref ty_qpath) = ty.kind;
let res = qpath_res(cx, ty_qpath, ty.hir_id);
if let Some(def_id) = res.opt_def_id();
if Some(def_id) == cx.tcx.lang_items().owned_box();
// At this point, we know ty is Box<T>, now get T
if let Some(ref last) = last_path_segment(ty_qpath).args;
if let Some(boxed_ty) = last.args.iter().find_map(|arg| match arg {
GenericArg::Type(ty) => Some(ty),
_ => None,
});
then {
fn check_fn_decl(&mut self, cx: &LateContext<'_, '_>, decl: &FnDecl<'_>) {
for input in decl.inputs {
self.check_ty(cx, input, false);
}
if let FunctionRetTy::Return(ref ty) = decl.output {
self.check_ty(cx, ty, false);
}
}
/// Recursively check for `TypePass` lints in the given type. Stop at the first
/// lint found.
///
/// The parameter `is_local` distinguishes the context of the type; types from
/// local bindings should only be checked for the `BORROWED_BOX` lint.
#[allow(clippy::too_many_lines)]
fn check_ty(&mut self, cx: &LateContext<'_, '_>, hir_ty: &hir::Ty<'_>, is_local: bool) {
if hir_ty.span.from_expansion() {
return;
}
match hir_ty.kind {
TyKind::Path(ref qpath) if !is_local => {
let hir_id = hir_ty.hir_id;
let res = qpath_res(cx, qpath, hir_id);
if let Some(def_id) = res.opt_def_id() {
if Some(def_id) == cx.tcx.lang_items().owned_box() {
if match_type_parameter(cx, qpath, &paths::VEC) {
span_help_and_lint(
cx,
BOX_VEC,
hir_ty.span,
"you seem to be trying to use `Box<Vec<T>>`. Consider using just `Vec<T>`",
"`Vec<T>` is already on the heap, `Box<Vec<T>>` makes an extra allocation.",
);
return; // don't recurse into the type
}
} else if cx.tcx.is_diagnostic_item(Symbol::intern("vec_type"), def_id) {
if_chain! {
// Get the _ part of Vec<_>
if let Some(ref last) = last_path_segment(qpath).args;
if let Some(ty) = last.args.iter().find_map(|arg| match arg {
GenericArg::Type(ty) => Some(ty),
_ => None,
});
// ty is now _ at this point
if let TyKind::Path(ref ty_qpath) = ty.kind;
let res = qpath_res(cx, ty_qpath, ty.hir_id);
if let Some(def_id) = res.opt_def_id();
if Some(def_id) == cx.tcx.lang_items().owned_box();
// At this point, we know ty is Box<T>, now get T
if let Some(ref last) = last_path_segment(ty_qpath).args;
if let Some(boxed_ty) = last.args.iter().find_map(|arg| match arg {
GenericArg::Type(ty) => Some(ty),
_ => None,
});
let ty_ty = hir_ty_to_ty(cx.tcx, boxed_ty);
if ty_ty.is_sized(cx.tcx.at(ty.span), cx.param_env) {
if ty_ty.is_sized(cx.tcx.at(ty.span), cx.param_env);
if let Ok(ty_ty_size) = cx.layout_of(ty_ty).map(|l| l.size.bytes());
if ty_ty_size <= self.vec_box_size_threshold;
then {
span_lint_and_sugg(
cx,
VEC_BOX,
@ -298,137 +309,144 @@ fn check_ty(cx: &LateContext<'_, '_>, hir_ty: &hir::Ty<'_>, is_local: bool) {
return; // don't recurse into the type
}
}
}
} else if match_def_path(cx, def_id, &paths::OPTION) {
if match_type_parameter(cx, qpath, &paths::OPTION) {
span_lint(
} else if match_def_path(cx, def_id, &paths::OPTION) {
if match_type_parameter(cx, qpath, &paths::OPTION) {
span_lint(
cx,
OPTION_OPTION,
hir_ty.span,
"consider using `Option<T>` instead of `Option<Option<T>>` or a custom \
enum if you need to distinguish all 3 cases",
);
return; // don't recurse into the type
}
} else if match_def_path(cx, def_id, &paths::LINKED_LIST) {
span_help_and_lint(
cx,
OPTION_OPTION,
LINKEDLIST,
hir_ty.span,
"consider using `Option<T>` instead of `Option<Option<T>>` or a custom \
enum if you need to distinguish all 3 cases",
"I see you're using a LinkedList! Perhaps you meant some other data structure?",
"a `VecDeque` might work",
);
return; // don't recurse into the type
}
} else if match_def_path(cx, def_id, &paths::LINKED_LIST) {
span_help_and_lint(
cx,
LINKEDLIST,
hir_ty.span,
"I see you're using a LinkedList! Perhaps you meant some other data structure?",
"a `VecDeque` might work",
);
return; // don't recurse into the type
}
}
match *qpath {
QPath::Resolved(Some(ref ty), ref p) => {
check_ty(cx, ty, is_local);
for ty in p.segments.iter().flat_map(|seg| {
seg.args
.as_ref()
.map_or_else(|| [].iter(), |params| params.args.iter())
.filter_map(|arg| match arg {
GenericArg::Type(ty) => Some(ty),
_ => None,
})
}) {
check_ty(cx, ty, is_local);
}
},
QPath::Resolved(None, ref p) => {
for ty in p.segments.iter().flat_map(|seg| {
seg.args
.as_ref()
.map_or_else(|| [].iter(), |params| params.args.iter())
.filter_map(|arg| match arg {
GenericArg::Type(ty) => Some(ty),
_ => None,
})
}) {
check_ty(cx, ty, is_local);
}
},
QPath::TypeRelative(ref ty, ref seg) => {
check_ty(cx, ty, is_local);
if let Some(ref params) = seg.args {
for ty in params.args.iter().filter_map(|arg| match arg {
GenericArg::Type(ty) => Some(ty),
_ => None,
match *qpath {
QPath::Resolved(Some(ref ty), ref p) => {
self.check_ty(cx, ty, is_local);
for ty in p.segments.iter().flat_map(|seg| {
seg.args
.as_ref()
.map_or_else(|| [].iter(), |params| params.args.iter())
.filter_map(|arg| match arg {
GenericArg::Type(ty) => Some(ty),
_ => None,
})
}) {
check_ty(cx, ty, is_local);
self.check_ty(cx, ty, is_local);
}
}
},
}
},
TyKind::Rptr(ref lt, ref mut_ty) => check_ty_rptr(cx, hir_ty, is_local, lt, mut_ty),
// recurse
TyKind::Slice(ref ty) | TyKind::Array(ref ty, _) | TyKind::Ptr(MutTy { ref ty, .. }) => {
check_ty(cx, ty, is_local)
},
TyKind::Tup(tys) => {
for ty in tys {
check_ty(cx, ty, is_local);
}
},
_ => {},
}
}
fn check_ty_rptr(cx: &LateContext<'_, '_>, hir_ty: &hir::Ty<'_>, is_local: bool, lt: &Lifetime, mut_ty: &MutTy<'_>) {
match mut_ty.ty.kind {
TyKind::Path(ref qpath) => {
let hir_id = mut_ty.ty.hir_id;
let def = qpath_res(cx, qpath, hir_id);
if_chain! {
if let Some(def_id) = def.opt_def_id();
if Some(def_id) == cx.tcx.lang_items().owned_box();
if let QPath::Resolved(None, ref path) = *qpath;
if let [ref bx] = *path.segments;
if let Some(ref params) = bx.args;
if !params.parenthesized;
if let Some(inner) = params.args.iter().find_map(|arg| match arg {
GenericArg::Type(ty) => Some(ty),
_ => None,
});
then {
if is_any_trait(inner) {
// Ignore `Box<Any>` types; see issue #1884 for details.
return;
}
let ltopt = if lt.is_elided() {
String::new()
} else {
format!("{} ", lt.name.ident().as_str())
};
let mutopt = if mut_ty.mutbl == Mutability::Mut {
"mut "
} else {
""
};
let mut applicability = Applicability::MachineApplicable;
span_lint_and_sugg(
cx,
BORROWED_BOX,
hir_ty.span,
"you seem to be trying to use `&Box<T>`. Consider using just `&T`",
"try",
format!(
"&{}{}{}",
ltopt,
mutopt,
&snippet_with_applicability(cx, inner.span, "..", &mut applicability)
),
Applicability::Unspecified,
);
return; // don't recurse into the type
},
QPath::Resolved(None, ref p) => {
for ty in p.segments.iter().flat_map(|seg| {
seg.args
.as_ref()
.map_or_else(|| [].iter(), |params| params.args.iter())
.filter_map(|arg| match arg {
GenericArg::Type(ty) => Some(ty),
_ => None,
})
}) {
self.check_ty(cx, ty, is_local);
}
},
QPath::TypeRelative(ref ty, ref seg) => {
self.check_ty(cx, ty, is_local);
if let Some(ref params) = seg.args {
for ty in params.args.iter().filter_map(|arg| match arg {
GenericArg::Type(ty) => Some(ty),
_ => None,
}) {
self.check_ty(cx, ty, is_local);
}
}
},
}
};
check_ty(cx, &mut_ty.ty, is_local);
},
_ => check_ty(cx, &mut_ty.ty, is_local),
},
TyKind::Rptr(ref lt, ref mut_ty) => self.check_ty_rptr(cx, hir_ty, is_local, lt, mut_ty),
// recurse
TyKind::Slice(ref ty) | TyKind::Array(ref ty, _) | TyKind::Ptr(MutTy { ref ty, .. }) => {
self.check_ty(cx, ty, is_local)
},
TyKind::Tup(tys) => {
for ty in tys {
self.check_ty(cx, ty, is_local);
}
},
_ => {},
}
}
fn check_ty_rptr(
&mut self,
cx: &LateContext<'_, '_>,
hir_ty: &hir::Ty<'_>,
is_local: bool,
lt: &Lifetime,
mut_ty: &MutTy<'_>,
) {
match mut_ty.ty.kind {
TyKind::Path(ref qpath) => {
let hir_id = mut_ty.ty.hir_id;
let def = qpath_res(cx, qpath, hir_id);
if_chain! {
if let Some(def_id) = def.opt_def_id();
if Some(def_id) == cx.tcx.lang_items().owned_box();
if let QPath::Resolved(None, ref path) = *qpath;
if let [ref bx] = *path.segments;
if let Some(ref params) = bx.args;
if !params.parenthesized;
if let Some(inner) = params.args.iter().find_map(|arg| match arg {
GenericArg::Type(ty) => Some(ty),
_ => None,
});
then {
if is_any_trait(inner) {
// Ignore `Box<Any>` types; see issue #1884 for details.
return;
}
let ltopt = if lt.is_elided() {
String::new()
} else {
format!("{} ", lt.name.ident().as_str())
};
let mutopt = if mut_ty.mutbl == Mutability::Mut {
"mut "
} else {
""
};
let mut applicability = Applicability::MachineApplicable;
span_lint_and_sugg(
cx,
BORROWED_BOX,
hir_ty.span,
"you seem to be trying to use `&Box<T>`. Consider using just `&T`",
"try",
format!(
"&{}{}{}",
ltopt,
mutopt,
&snippet_with_applicability(cx, inner.span, "..", &mut applicability)
),
Applicability::Unspecified,
);
return; // don't recurse into the type
}
};
self.check_ty(cx, &mut_ty.ty, is_local);
},
_ => self.check_ty(cx, &mut_ty.ty, is_local),
}
}
}

2
clippy_lints/src/utils/conf.rs
View file

@ -152,6 +152,8 @@ define_Conf! {
(too_many_lines_threshold, "too_many_lines_threshold", 100 => u64),
/// Lint: LARGE_STACK_ARRAYS. The maximum allowed size for arrays on the stack
(array_size_threshold, "array_size_threshold", 512_000 => u64),
/// Lint: VEC_BOX. The size of the boxed type in bytes, where boxing in a `Vec` is allowed
(vec_box_size_threshold, "vec_box_size_threshold", 4096 => u64),
}
impl Default for Conf {

2
tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr
View file

@ -1,4 +1,4 @@
error: error reading Clippy's configuration file `$DIR/clippy.toml`: unknown field `foobar`, expected one of `blacklisted-names`, `cognitive-complexity-threshold`, `cyclomatic-complexity-threshold`, `doc-valid-idents`, `too-many-arguments-threshold`, `type-complexity-threshold`, `single-char-binding-names-threshold`, `too-large-for-stack`, `enum-variant-name-threshold`, `enum-variant-size-threshold`, `verbose-bit-mask-threshold`, `literal-representation-threshold`, `trivial-copy-size-limit`, `too-many-lines-threshold`, `array-size-threshold`, `third-party` at line 5 column 1
error: error reading Clippy's configuration file `$DIR/clippy.toml`: unknown field `foobar`, expected one of `blacklisted-names`, `cognitive-complexity-threshold`, `cyclomatic-complexity-threshold`, `doc-valid-idents`, `too-many-arguments-threshold`, `type-complexity-threshold`, `single-char-binding-names-threshold`, `too-large-for-stack`, `enum-variant-name-threshold`, `enum-variant-size-threshold`, `verbose-bit-mask-threshold`, `literal-representation-threshold`, `trivial-copy-size-limit`, `too-many-lines-threshold`, `array-size-threshold`, `vec-box-size-threshold`, `third-party` at line 5 column 1
error: aborting due to previous error

1
tests/ui-toml/vec_box_sized/clippy.toml Normal file
View file

@ -0,0 +1 @@
vec-box-size-threshold = 4

15
tests/ui-toml/vec_box_sized/test.rs Normal file
View file

@ -0,0 +1,15 @@
struct S {
x: u64,
}
struct C {
y: u16,
}
struct Foo(Vec<Box<u8>>);
struct Bar(Vec<Box<u32>>);
struct Baz(Vec<Box<(u32, u32)>>);
struct BarBaz(Vec<Box<S>>);
struct FooBarBaz(Vec<Box<C>>);
fn main() {}

22
tests/ui-toml/vec_box_sized/test.stderr Normal file
View file

@ -0,0 +1,22 @@
error: `Vec<T>` is already on the heap, the boxing is unnecessary.
--> $DIR/test.rs:9:12
|
LL | struct Foo(Vec<Box<u8>>);
| ^^^^^^^^^^^^ help: try: `Vec<u8>`
|
= note: `-D clippy::vec-box` implied by `-D warnings`
error: `Vec<T>` is already on the heap, the boxing is unnecessary.
--> $DIR/test.rs:10:12
|
LL | struct Bar(Vec<Box<u32>>);
| ^^^^^^^^^^^^^ help: try: `Vec<u32>`
error: `Vec<T>` is already on the heap, the boxing is unnecessary.
--> $DIR/test.rs:13:18
|
LL | struct FooBarBaz(Vec<Box<C>>);
| ^^^^^^^^^^^ help: try: `Vec<C>`
error: aborting due to 3 previous errors

4
tests/ui/vec_box_sized.fixed
View file

@ -4,6 +4,7 @@
struct SizedStruct(i32);
struct UnsizedStruct([i32]);
struct BigStruct([i32; 10000]);
/// The following should trigger the lint
mod should_trigger {
@ -19,9 +20,10 @@ mod should_trigger {
/// The following should not trigger the lint
mod should_not_trigger {
use super::UnsizedStruct;
use super::{BigStruct, UnsizedStruct};
struct C(Vec<Box<UnsizedStruct>>);
struct D(Vec<Box<BigStruct>>);
struct StructWithVecBoxButItsUnsized {
unsized_type: Vec<Box<UnsizedStruct>>,

4
tests/ui/vec_box_sized.rs
View file

@ -4,6 +4,7 @@
struct SizedStruct(i32);
struct UnsizedStruct([i32]);
struct BigStruct([i32; 10000]);
/// The following should trigger the lint
mod should_trigger {
@ -19,9 +20,10 @@ mod should_trigger {
/// The following should not trigger the lint
mod should_not_trigger {
use super::UnsizedStruct;
use super::{BigStruct, UnsizedStruct};
struct C(Vec<Box<UnsizedStruct>>);
struct D(Vec<Box<BigStruct>>);
struct StructWithVecBoxButItsUnsized {
unsized_type: Vec<Box<UnsizedStruct>>,

6
tests/ui/vec_box_sized.stderr
View file

@ -1,5 +1,5 @@
error: `Vec<T>` is already on the heap, the boxing is unnecessary.
--> $DIR/vec_box_sized.rs:13:21
--> $DIR/vec_box_sized.rs:14:21
|
LL | sized_type: Vec<Box<SizedStruct>>,
| ^^^^^^^^^^^^^^^^^^^^^ help: try: `Vec<SizedStruct>`
@ -7,13 +7,13 @@ LL | sized_type: Vec<Box<SizedStruct>>,
= note: `-D clippy::vec-box` implied by `-D warnings`
error: `Vec<T>` is already on the heap, the boxing is unnecessary.
--> $DIR/vec_box_sized.rs:16:14
--> $DIR/vec_box_sized.rs:17:14
|
LL | struct A(Vec<Box<SizedStruct>>);
| ^^^^^^^^^^^^^^^^^^^^^ help: try: `Vec<SizedStruct>`
error: `Vec<T>` is already on the heap, the boxing is unnecessary.
--> $DIR/vec_box_sized.rs:17:18
--> $DIR/vec_box_sized.rs:18:18
|
LL | struct B(Vec<Vec<Box<(u32)>>>);
| ^^^^^^^^^^^^^^^ help: try: `Vec<u32>`