From 206741bf57361efcd04243b9e2e6275701c15c76 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Thu, 12 Aug 2021 12:05:02 +0200 Subject: [PATCH] Use `avoid_breaking_exported_api` for `types` module lints Addressed PR reviews regarding code style --- clippy_lints/src/lib.rs | 7 ++- clippy_lints/src/types/mod.rs | 79 +++++++++++++++++++++++++++------- clippy_lints/src/utils/conf.rs | 2 +- 3 files changed, 70 insertions(+), 18 deletions(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index dbdb4251b3be..18600498e1c4 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -1840,7 +1840,12 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box serde_api::SerdeApi); let vec_box_size_threshold = conf.vec_box_size_threshold; let type_complexity_threshold = conf.type_complexity_threshold; - store.register_late_pass(move || box types::Types::new(vec_box_size_threshold, type_complexity_threshold)); + let avoid_breaking_exported_api = conf.avoid_breaking_exported_api; + store.register_late_pass(move || box types::Types::new( + vec_box_size_threshold, + type_complexity_threshold, + avoid_breaking_exported_api, + )); store.register_late_pass(|| box booleans::NonminimalBool); store.register_late_pass(|| box needless_bitwise_bool::NeedlessBitwiseBool); store.register_late_pass(|| box eq_op::EqOp); diff --git a/clippy_lints/src/types/mod.rs b/clippy_lints/src/types/mod.rs index 371bb8b445a7..9588de8459cf 100644 --- a/clippy_lints/src/types/mod.rs +++ b/clippy_lints/src/types/mod.rs @@ -295,6 +295,7 @@ declare_clippy_lint! { 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, RC_MUTEX, TYPE_COMPLEXITY]); @@ -308,19 +309,31 @@ impl<'tcx> LateLintPass<'tcx> for Types { 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 _ => (), } @@ -342,15 +355,31 @@ impl<'tcx> LateLintPass<'tcx> for Types { } 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(..) => (), } } @@ -370,10 +399,11 @@ impl<'tcx> LateLintPass<'tcx> for Types { } 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, } } @@ -410,17 +440,24 @@ impl Types { 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); - triggered |= rc_mutex::check(cx, hir_ty, qpath, def_id); + 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` - if triggered { - return; + 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); + triggered |= rc_mutex::check(cx, hir_ty, qpath, def_id); + + if triggered { + return; + } } } match *qpath { @@ -487,11 +524,21 @@ impl Types { _ => {}, } } + + /// 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, } diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs index a28b1d78f7d4..9ee2e3024520 100644 --- a/clippy_lints/src/utils/conf.rs +++ b/clippy_lints/src/utils/conf.rs @@ -132,7 +132,7 @@ macro_rules! define_Conf { // N.B., this macro is parsed by util/lintlib.py define_Conf! { - /// Lint: ENUM_VARIANT_NAMES, LARGE_TYPES_PASSED_BY_VALUE, TRIVIALLY_COPY_PASS_BY_REF, UNNECESSARY_WRAPS, UPPER_CASE_ACRONYMS, WRONG_SELF_CONVENTION. + /// Lint: ENUM_VARIANT_NAMES, LARGE_TYPES_PASSED_BY_VALUE, TRIVIALLY_COPY_PASS_BY_REF, UNNECESSARY_WRAPS, UPPER_CASE_ACRONYMS, WRONG_SELF_CONVENTION, BOX_VEC, REDUNDANT_ALLOCATION, RC_BUFFER, VEC_BOX, OPTION_OPTION, LINKEDLIST, RC_MUTEX. /// /// Suppress lints whenever the suggested change would cause breakage for other crates. (avoid_breaking_exported_api: bool = true),