X-Git-Url: https://git.lizzy.rs/?a=blobdiff_plain;f=clippy_lints%2Fsrc%2Ftypes%2Fmod.rs;h=69cd49d884cc00ae91674369eeae38cf86c5ac5d;hb=063f8aa094a740b98b0dab49d8361441c8f1d0c4;hp=d9b47a699dc3cc4150b3bb85eb2e32153527183e;hpb=f0ceb28ba16c9f7c138a4311cabcf44bacc923ce;p=rust.git diff --git a/clippy_lints/src/types/mod.rs b/clippy_lints/src/types/mod.rs index d9b47a699dc..69cd49d884c 100644 --- a/clippy_lints/src/types/mod.rs +++ b/clippy_lints/src/types/mod.rs @@ -1,8 +1,9 @@ mod borrowed_box; -mod box_vec; +mod box_collection; mod linked_list; mod option_option; mod rc_buffer; +mod rc_mutex; mod redundant_allocation; mod type_complexity; mod utils; @@ -19,16 +20,16 @@ use rustc_span::source_map::Span; declare_clippy_lint! { - /// **What it does:** Checks for use of `Box>` anywhere in the code. + /// ### What it does + /// Checks for use of `Box` where T is a collection such as Vec anywhere in the code. /// Check the [Box documentation](https://doc.rust-lang.org/std/boxed/index.html) for more information. /// - /// **Why is this bad?** `Vec` already keeps its contents in a separate area on - /// the heap. So if you `Box` it, you just add another level of indirection + /// ### Why is this bad? + /// Collections already keeps their contents in a separate area on + /// the heap. So if you `Box` them, you just add another level of indirection /// without any benefit whatsoever. /// - /// **Known problems:** None. - /// - /// **Example:** + /// ### Example /// ```rust,ignore /// struct X { /// values: Box>, @@ -42,22 +43,26 @@ /// values: Vec, /// } /// ``` - pub BOX_VEC, + #[clippy::version = "1.57.0"] + pub BOX_COLLECTION, perf, "usage of `Box>`, vector elements are already on the heap" } declare_clippy_lint! { - /// **What it does:** Checks for use of `Vec>` where T: Sized anywhere in the code. + /// ### What it does + /// Checks for use of `Vec>` where T: Sized anywhere in the code. /// Check the [Box documentation](https://doc.rust-lang.org/std/boxed/index.html) for more information. /// - /// **Why is this bad?** `Vec` already keeps its contents in a separate area on + /// ### Why is this bad? + /// `Vec` already keeps its contents in a separate area on /// the heap. So if you `Box` its contents, you just add another level of indirection. /// - /// **Known problems:** Vec> makes sense if T is a large type (see [#3530](https://github.com/rust-lang/rust-clippy/issues/3530), + /// ### Known problems + /// Vec> makes sense if T is a large type (see [#3530](https://github.com/rust-lang/rust-clippy/issues/3530), /// 1st comment). /// - /// **Example:** + /// ### Example /// ```rust /// struct X { /// values: Vec>, @@ -71,25 +76,26 @@ /// values: Vec, /// } /// ``` + #[clippy::version = "1.33.0"] pub VEC_BOX, complexity, "usage of `Vec>` where T: Sized, vector elements are already on the heap" } declare_clippy_lint! { - /// **What it does:** Checks for use of `Option>` in function signatures and type + /// ### What it does + /// Checks for use of `Option>` in function signatures and type /// definitions /// - /// **Why is this bad?** `Option<_>` represents an optional value. `Option>` + /// ### Why is this bad? + /// `Option<_>` represents an optional value. `Option>` /// represents an optional optional value which is logically the same thing as an optional /// value but has an unneeded extra level of wrapping. /// /// If you have a case where `Some(Some(_))`, `Some(None)` and `None` are distinct cases, /// consider a custom `enum` instead, with clear names for each case. /// - /// **Known problems:** None. - /// - /// **Example** + /// ### Example /// ```rust /// fn get_data() -> Option> { /// None @@ -109,16 +115,19 @@ /// Contents::None /// } /// ``` + #[clippy::version = "pre 1.29.0"] pub OPTION_OPTION, pedantic, "usage of `Option>`" } declare_clippy_lint! { - /// **What it does:** Checks for usage of any `LinkedList`, suggesting to use a + /// ### What it does + /// Checks for usage of any `LinkedList`, suggesting to use a /// `Vec` or a `VecDeque` (formerly called `RingBuf`). /// - /// **Why is this bad?** Gankro says: + /// ### Why is this bad? + /// Gankro says: /// /// > The TL;DR of `LinkedList` is that it's built on a massive amount of /// pointers and indirection. @@ -137,29 +146,31 @@ /// can still be better /// > because of how expensive it is to seek to the middle of a `LinkedList`. /// - /// **Known problems:** False positives – the instances where using a + /// ### Known problems + /// False positives – the instances where using a /// `LinkedList` makes sense are few and far between, but they can still happen. /// - /// **Example:** + /// ### Example /// ```rust /// # use std::collections::LinkedList; /// let x: LinkedList = LinkedList::new(); /// ``` + #[clippy::version = "pre 1.29.0"] pub LINKEDLIST, pedantic, "usage of LinkedList, usually a vector is faster, or a more specialized data structure like a `VecDeque`" } declare_clippy_lint! { - /// **What it does:** Checks for use of `&Box` anywhere in the code. + /// ### What it does + /// Checks for use of `&Box` anywhere in the code. /// Check the [Box documentation](https://doc.rust-lang.org/std/boxed/index.html) for more information. /// - /// **Why is this bad?** Any `&Box` can also be a `&T`, which is more + /// ### Why is this bad? + /// Any `&Box` can also be a `&T`, which is more /// general. /// - /// **Known problems:** None. - /// - /// **Example:** + /// ### Example /// ```rust,ignore /// fn foo(bar: &Box) { ... } /// ``` @@ -169,20 +180,21 @@ /// ```rust,ignore /// fn foo(bar: &T) { ... } /// ``` + #[clippy::version = "pre 1.29.0"] pub BORROWED_BOX, complexity, "a borrow of a boxed type" } declare_clippy_lint! { - /// **What it does:** Checks for use of redundant allocations anywhere in the code. + /// ### What it does + /// Checks for use of redundant allocations anywhere in the code. /// - /// **Why is this bad?** Expressions such as `Rc<&T>`, `Rc>`, `Rc>`, `Box<&T>` - /// add an unnecessary level of indirection. + /// ### Why is this bad? + /// Expressions such as `Rc<&T>`, `Rc>`, `Rc>`, `Rc>`, `Arc<&T>`, `Arc>`, + /// `Arc>`, `Arc>`, `Box<&T>`, `Box>`, `Box>`, `Box>`, add an unnecessary level of indirection. /// - /// **Known problems:** None. - /// - /// **Example:** + /// ### Example /// ```rust /// # use std::rc::Rc; /// fn foo(bar: Rc<&usize>) {} @@ -193,15 +205,18 @@ /// ```rust /// fn foo(bar: &usize) {} /// ``` + #[clippy::version = "1.44.0"] pub REDUNDANT_ALLOCATION, perf, "redundant allocation" } declare_clippy_lint! { - /// **What it does:** Checks for `Rc` and `Arc` when `T` is a mutable buffer type such as `String` or `Vec`. + /// ### What it does + /// Checks for `Rc` and `Arc` when `T` is a mutable buffer type such as `String` or `Vec`. /// - /// **Why is this bad?** Expressions such as `Rc` usually have no advantage over `Rc`, since + /// ### Why is this bad? + /// Expressions such as `Rc` usually have no advantage over `Rc`, since /// it is larger and involves an extra level of indirection, and doesn't implement `Borrow`. /// /// While mutating a buffer type would still be possible with `Rc::get_mut()`, it only @@ -210,10 +225,11 @@ /// type with an interior mutable container (such as `RefCell` or `Mutex`) would normally /// be used. /// - /// **Known problems:** This pattern can be desirable to avoid the overhead of a `RefCell` or `Mutex` for + /// ### Known problems + /// This pattern can be desirable to avoid the overhead of a `RefCell` or `Mutex` for /// cases where mutation only happens before there are any additional references. /// - /// **Example:** + /// ### Example /// ```rust,ignore /// # use std::rc::Rc; /// fn foo(interned: Rc) { ... } @@ -224,38 +240,74 @@ /// ```rust,ignore /// fn foo(interned: Rc) { ... } /// ``` + #[clippy::version = "1.48.0"] pub RC_BUFFER, restriction, "shared ownership of a buffer type" } declare_clippy_lint! { - /// **What it does:** Checks for types used in structs, parameters and `let` + /// ### What it does + /// Checks for types used in structs, parameters and `let` /// declarations above a certain complexity threshold. /// - /// **Why is this bad?** Too complex types make the code less readable. Consider + /// ### Why is this bad? + /// Too complex types make the code less readable. Consider /// using a `type` definition to simplify them. /// - /// **Known problems:** None. - /// - /// **Example:** + /// ### Example /// ```rust /// # use std::rc::Rc; /// struct Foo { /// inner: Rc>>>, /// } /// ``` + #[clippy::version = "pre 1.29.0"] pub TYPE_COMPLEXITY, complexity, "usage of very complex types that might be better factored into `type` definitions" } +declare_clippy_lint! { + /// ### What it does + /// Checks for `Rc>`. + /// + /// ### Why is this bad? + /// `Rc` is used in single thread and `Mutex` is used in multi thread. + /// Consider using `Rc>` in single thread or `Arc>` in multi thread. + /// + /// ### Known problems + /// Sometimes combining generic types can lead to the requirement that a + /// type use Rc in conjunction with Mutex. We must consider those cases false positives, but + /// alas they are quite hard to rule out. Luckily they are also rare. + /// + /// ### Example + /// ```rust,ignore + /// use std::rc::Rc; + /// use std::sync::Mutex; + /// fn foo(interned: Rc>) { ... } + /// ``` + /// + /// Better: + /// + /// ```rust,ignore + /// use std::rc::Rc; + /// use std::cell::RefCell + /// fn foo(interned: Rc>) { ... } + /// ``` + #[clippy::version = "1.55.0"] + pub RC_MUTEX, + restriction, + "usage of `Rc>`" +} + pub struct Types { vec_box_size_threshold: u64, type_complexity_threshold: u64, + avoid_breaking_exported_api: bool, } -impl_lint_pass!(Types => [BOX_VEC, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX, REDUNDANT_ALLOCATION, RC_BUFFER, TYPE_COMPLEXITY]); +impl_lint_pass!(Types => [BOX_COLLECTION, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX, REDUNDANT_ALLOCATION, RC_BUFFER, RC_MUTEX, TYPE_COMPLEXITY]); impl<'tcx> LateLintPass<'tcx> for Types { fn check_fn(&mut self, cx: &LateContext<'_>, _: FnKind<'_>, decl: &FnDecl<'_>, _: &Body<'_>, _: Span, id: HirId) { @@ -266,19 +318,31 @@ fn check_fn(&mut self, cx: &LateContext<'_>, _: FnKind<'_>, decl: &FnDecl<'_>, _ false }; + let is_exported = cx.access_levels.is_exported(cx.tcx.hir().local_def_id(id)); + self.check_fn_decl( cx, decl, CheckTyContext { is_in_trait_impl, + is_exported, ..CheckTyContext::default() }, ); } fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) { + let is_exported = cx.access_levels.is_exported(item.def_id); + match item.kind { - ItemKind::Static(ty, _, _) | ItemKind::Const(ty, _) => self.check_ty(cx, ty, CheckTyContext::default()), + ItemKind::Static(ty, _, _) | ItemKind::Const(ty, _) => self.check_ty( + cx, + ty, + CheckTyContext { + is_exported, + ..CheckTyContext::default() + }, + ), // functions, enums, structs, impls and traits are covered _ => (), } @@ -286,7 +350,7 @@ fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) { fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx ImplItem<'_>) { match item.kind { - ImplItemKind::Const(ty, _) | ImplItemKind::TyAlias(ty) => self.check_ty( + ImplItemKind::Const(ty, _) => self.check_ty( cx, ty, CheckTyContext { @@ -294,21 +358,39 @@ fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx ImplItem<'_>) ..CheckTyContext::default() }, ), - // methods are covered by check_fn - ImplItemKind::Fn(..) => (), + // Methods are covered by check_fn. + // Type aliases are ignored because oftentimes it's impossible to + // make type alias declaration in trait simpler, see #1013 + ImplItemKind::Fn(..) | ImplItemKind::TyAlias(..) => (), } } fn check_field_def(&mut self, cx: &LateContext<'_>, field: &hir::FieldDef<'_>) { - self.check_ty(cx, field.ty, CheckTyContext::default()); + let is_exported = cx.access_levels.is_exported(cx.tcx.hir().local_def_id(field.hir_id)); + + self.check_ty( + cx, + field.ty, + CheckTyContext { + is_exported, + ..CheckTyContext::default() + }, + ); } - fn check_trait_item(&mut self, cx: &LateContext<'_>, item: &TraitItem<'_>) { + fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &TraitItem<'_>) { + let is_exported = cx.access_levels.is_exported(item.def_id); + + let context = CheckTyContext { + is_exported, + ..CheckTyContext::default() + }; + match item.kind { TraitItemKind::Const(ty, _) | TraitItemKind::Type(_, Some(ty)) => { - self.check_ty(cx, ty, CheckTyContext::default()) + self.check_ty(cx, ty, context); }, - TraitItemKind::Fn(ref sig, _) => self.check_fn_decl(cx, sig.decl, CheckTyContext::default()), + TraitItemKind::Fn(ref sig, _) => self.check_fn_decl(cx, sig.decl, context), TraitItemKind::Type(..) => (), } } @@ -328,10 +410,11 @@ fn check_local(&mut self, cx: &LateContext<'_>, local: &Local<'_>) { } impl Types { - pub fn new(vec_box_size_threshold: u64, type_complexity_threshold: u64) -> Self { + pub fn new(vec_box_size_threshold: u64, type_complexity_threshold: u64, avoid_breaking_exported_api: bool) -> Self { Self { vec_box_size_threshold, type_complexity_threshold, + avoid_breaking_exported_api, } } @@ -368,16 +451,24 @@ fn check_ty(&mut self, cx: &LateContext<'_>, hir_ty: &hir::Ty<'_>, mut context: let hir_id = hir_ty.hir_id; let res = cx.qpath_res(qpath, hir_id); if let Some(def_id) = res.opt_def_id() { - let mut triggered = false; - triggered |= box_vec::check(cx, hir_ty, qpath, def_id); - triggered |= redundant_allocation::check(cx, hir_ty, qpath, def_id); - triggered |= rc_buffer::check(cx, hir_ty, qpath, def_id); - triggered |= vec_box::check(cx, hir_ty, qpath, def_id, self.vec_box_size_threshold); - triggered |= option_option::check(cx, hir_ty, qpath, def_id); - triggered |= linked_list::check(cx, hir_ty, def_id); - - if triggered { - return; + if self.is_type_change_allowed(context) { + // All lints that are being checked in this block are guarded by + // the `avoid_breaking_exported_api` configuration. When adding a + // new lint, please also add the name to the configuration documentation + // in `clippy_lints::utils::conf.rs` + + let mut triggered = false; + triggered |= box_collection::check(cx, hir_ty, qpath, def_id); + triggered |= redundant_allocation::check(cx, hir_ty, qpath, def_id); + triggered |= rc_buffer::check(cx, hir_ty, qpath, def_id); + triggered |= vec_box::check(cx, hir_ty, qpath, def_id, self.vec_box_size_threshold); + triggered |= option_option::check(cx, hir_ty, qpath, def_id); + triggered |= linked_list::check(cx, hir_ty, def_id); + triggered |= rc_mutex::check(cx, hir_ty, qpath, def_id); + + if triggered { + return; + } } } match *qpath { @@ -433,7 +524,7 @@ fn check_ty(&mut self, cx: &LateContext<'_>, hir_ty: &hir::Ty<'_>, mut context: }, TyKind::Slice(ty) | TyKind::Array(ty, _) | TyKind::Ptr(MutTy { ty, .. }) => { context.is_nested_call = true; - self.check_ty(cx, ty, context) + self.check_ty(cx, ty, context); }, TyKind::Tup(tys) => { context.is_nested_call = true; @@ -444,11 +535,21 @@ fn check_ty(&mut self, cx: &LateContext<'_>, hir_ty: &hir::Ty<'_>, mut context: _ => {}, } } + + /// This function checks if the type is allowed to change in the current context + /// based on the `avoid_breaking_exported_api` configuration + fn is_type_change_allowed(&self, context: CheckTyContext) -> bool { + !(context.is_exported && self.avoid_breaking_exported_api) + } } +#[allow(clippy::struct_excessive_bools)] #[derive(Clone, Copy, Default)] struct CheckTyContext { is_in_trait_impl: bool, + /// `true` for types on local variables. is_local: bool, + /// `true` for types that are part of the public API. + is_exported: bool, is_nested_call: bool, }