From caa49c85d6c1d45678d1065c689cc4d168e18dfb Mon Sep 17 00:00:00 2001 From: Yoshitomo Nakanishi Date: Thu, 18 Mar 2021 16:03:56 +0900 Subject: [PATCH 1/6] Move absurd_extreme_comparisons to its own module --- .../src/absurd_extreme_comparisons.rs | 173 +++++++++++++++++ clippy_lints/src/lib.rs | 9 +- clippy_lints/src/types/mod.rs | 178 +----------------- 3 files changed, 183 insertions(+), 177 deletions(-) create mode 100644 clippy_lints/src/absurd_extreme_comparisons.rs diff --git a/clippy_lints/src/absurd_extreme_comparisons.rs b/clippy_lints/src/absurd_extreme_comparisons.rs new file mode 100644 index 000000000000..33c720c666e4 --- /dev/null +++ b/clippy_lints/src/absurd_extreme_comparisons.rs @@ -0,0 +1,173 @@ +use rustc_hir::{BinOpKind, Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty; +use rustc_session::{declare_lint_pass, declare_tool_lint}; + +use crate::consts::{constant, Constant}; + +use clippy_utils::comparisons::{normalize_comparison, Rel}; +use clippy_utils::diagnostics::span_lint_and_help; +use clippy_utils::source::snippet; +use clippy_utils::ty::is_isize_or_usize; +use clippy_utils::{clip, int_bits, unsext}; + +declare_clippy_lint! { + /// **What it does:** Checks for comparisons where one side of the relation is + /// either the minimum or maximum value for its type and warns if it involves a + /// case that is always true or always false. Only integer and boolean types are + /// checked. + /// + /// **Why is this bad?** An expression like `min <= x` may misleadingly imply + /// that it is possible for `x` to be less than the minimum. Expressions like + /// `max < x` are probably mistakes. + /// + /// **Known problems:** For `usize` the size of the current compile target will + /// be assumed (e.g., 64 bits on 64 bit systems). This means code that uses such + /// a comparison to detect target pointer width will trigger this lint. One can + /// use `mem::sizeof` and compare its value or conditional compilation + /// attributes + /// like `#[cfg(target_pointer_width = "64")] ..` instead. + /// + /// **Example:** + /// + /// ```rust + /// let vec: Vec = Vec::new(); + /// if vec.len() <= 0 {} + /// if 100 > i32::MAX {} + /// ``` + pub ABSURD_EXTREME_COMPARISONS, + correctness, + "a comparison with a maximum or minimum value that is always true or false" +} + +declare_lint_pass!(AbsurdExtremeComparisons => [ABSURD_EXTREME_COMPARISONS]); + +impl<'tcx> LateLintPass<'tcx> for AbsurdExtremeComparisons { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + if let ExprKind::Binary(ref cmp, ref lhs, ref rhs) = expr.kind { + if let Some((culprit, result)) = detect_absurd_comparison(cx, cmp.node, lhs, rhs) { + if !expr.span.from_expansion() { + let msg = "this comparison involving the minimum or maximum element for this \ + type contains a case that is always true or always false"; + + let conclusion = match result { + AbsurdComparisonResult::AlwaysFalse => "this comparison is always false".to_owned(), + AbsurdComparisonResult::AlwaysTrue => "this comparison is always true".to_owned(), + AbsurdComparisonResult::InequalityImpossible => format!( + "the case where the two sides are not equal never occurs, consider using `{} == {}` \ + instead", + snippet(cx, lhs.span, "lhs"), + snippet(cx, rhs.span, "rhs") + ), + }; + + let help = format!( + "because `{}` is the {} value for this type, {}", + snippet(cx, culprit.expr.span, "x"), + match culprit.which { + ExtremeType::Minimum => "minimum", + ExtremeType::Maximum => "maximum", + }, + conclusion + ); + + span_lint_and_help(cx, ABSURD_EXTREME_COMPARISONS, expr.span, msg, None, &help); + } + } + } + } +} + +enum ExtremeType { + Minimum, + Maximum, +} + +struct ExtremeExpr<'a> { + which: ExtremeType, + expr: &'a Expr<'a>, +} + +enum AbsurdComparisonResult { + AlwaysFalse, + AlwaysTrue, + InequalityImpossible, +} + +fn is_cast_between_fixed_and_target<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { + if let ExprKind::Cast(ref cast_exp, _) = expr.kind { + let precast_ty = cx.typeck_results().expr_ty(cast_exp); + let cast_ty = cx.typeck_results().expr_ty(expr); + + return is_isize_or_usize(precast_ty) != is_isize_or_usize(cast_ty); + } + + false +} + +fn detect_absurd_comparison<'tcx>( + cx: &LateContext<'tcx>, + op: BinOpKind, + lhs: &'tcx Expr<'_>, + rhs: &'tcx Expr<'_>, +) -> Option<(ExtremeExpr<'tcx>, AbsurdComparisonResult)> { + use AbsurdComparisonResult::{AlwaysFalse, AlwaysTrue, InequalityImpossible}; + use ExtremeType::{Maximum, Minimum}; + // absurd comparison only makes sense on primitive types + // primitive types don't implement comparison operators with each other + if cx.typeck_results().expr_ty(lhs) != cx.typeck_results().expr_ty(rhs) { + return None; + } + + // comparisons between fix sized types and target sized types are considered unanalyzable + if is_cast_between_fixed_and_target(cx, lhs) || is_cast_between_fixed_and_target(cx, rhs) { + return None; + } + + let (rel, normalized_lhs, normalized_rhs) = normalize_comparison(op, lhs, rhs)?; + + let lx = detect_extreme_expr(cx, normalized_lhs); + let rx = detect_extreme_expr(cx, normalized_rhs); + + Some(match rel { + Rel::Lt => { + match (lx, rx) { + (Some(l @ ExtremeExpr { which: Maximum, .. }), _) => (l, AlwaysFalse), // max < x + (_, Some(r @ ExtremeExpr { which: Minimum, .. })) => (r, AlwaysFalse), // x < min + _ => return None, + } + }, + Rel::Le => { + match (lx, rx) { + (Some(l @ ExtremeExpr { which: Minimum, .. }), _) => (l, AlwaysTrue), // min <= x + (Some(l @ ExtremeExpr { which: Maximum, .. }), _) => (l, InequalityImpossible), // max <= x + (_, Some(r @ ExtremeExpr { which: Minimum, .. })) => (r, InequalityImpossible), // x <= min + (_, Some(r @ ExtremeExpr { which: Maximum, .. })) => (r, AlwaysTrue), // x <= max + _ => return None, + } + }, + Rel::Ne | Rel::Eq => return None, + }) +} + +fn detect_extreme_expr<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option> { + let ty = cx.typeck_results().expr_ty(expr); + + let cv = constant(cx, cx.typeck_results(), expr)?.0; + + let which = match (ty.kind(), cv) { + (&ty::Bool, Constant::Bool(false)) | (&ty::Uint(_), Constant::Int(0)) => ExtremeType::Minimum, + (&ty::Int(ity), Constant::Int(i)) if i == unsext(cx.tcx, i128::MIN >> (128 - int_bits(cx.tcx, ity)), ity) => { + ExtremeType::Minimum + }, + + (&ty::Bool, Constant::Bool(true)) => ExtremeType::Maximum, + (&ty::Int(ity), Constant::Int(i)) if i == unsext(cx.tcx, i128::MAX >> (128 - int_bits(cx.tcx, ity)), ity) => { + ExtremeType::Maximum + }, + (&ty::Uint(uty), Constant::Int(i)) if clip(cx.tcx, u128::MAX, uty) == i => ExtremeType::Maximum, + + _ => return None, + }; + Some(ExtremeExpr { which, expr }) +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 3a9236d8735b..7b261121f518 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -164,6 +164,7 @@ mod consts; mod utils; // begin lints modules, do not remove this comment, it’s used in `update_lints` +mod absurd_extreme_comparisons; mod approx_const; mod arithmetic; mod as_conversions; @@ -558,6 +559,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &utils::internal_lints::PRODUCE_ICE, #[cfg(feature = "internal-lints")] &utils::internal_lints::UNNECESSARY_SYMBOL_STR, + &absurd_extreme_comparisons::ABSURD_EXTREME_COMPARISONS, &approx_const::APPROX_CONSTANT, &arithmetic::FLOAT_ARITHMETIC, &arithmetic::INTEGER_ARITHMETIC, @@ -955,7 +957,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &transmute::WRONG_TRANSMUTE, &transmuting_null::TRANSMUTING_NULL, &try_err::TRY_ERR, - &types::ABSURD_EXTREME_COMPARISONS, &types::BORROWED_BOX, &types::BOX_VEC, &types::IMPLICIT_HASHER, @@ -1112,7 +1113,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box get_last_with_len::GetLastWithLen); store.register_late_pass(|| box drop_forget_ref::DropForgetRef); store.register_late_pass(|| box empty_enum::EmptyEnum); - store.register_late_pass(|| box types::AbsurdExtremeComparisons); + store.register_late_pass(|| box absurd_extreme_comparisons::AbsurdExtremeComparisons); store.register_late_pass(|| box types::InvalidUpcastComparisons); store.register_late_pass(|| box regex::Regex::default()); store.register_late_pass(|| box copies::CopyAndPaste); @@ -1442,6 +1443,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: ]); store.register_group(true, "clippy::all", Some("clippy"), vec![ + LintId::of(&absurd_extreme_comparisons::ABSURD_EXTREME_COMPARISONS), LintId::of(&approx_const::APPROX_CONSTANT), LintId::of(&assertions_on_constants::ASSERTIONS_ON_CONSTANTS), LintId::of(&assign_ops::ASSIGN_OP_PATTERN), @@ -1701,7 +1703,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&transmute::WRONG_TRANSMUTE), LintId::of(&transmuting_null::TRANSMUTING_NULL), LintId::of(&try_err::TRY_ERR), - LintId::of(&types::ABSURD_EXTREME_COMPARISONS), LintId::of(&types::BORROWED_BOX), LintId::of(&types::BOX_VEC), LintId::of(&types::REDUNDANT_ALLOCATION), @@ -1941,6 +1942,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: ]); store.register_group(true, "clippy::correctness", Some("clippy_correctness"), vec![ + LintId::of(&absurd_extreme_comparisons::ABSURD_EXTREME_COMPARISONS), LintId::of(&approx_const::APPROX_CONSTANT), LintId::of(&async_yields_async::ASYNC_YIELDS_ASYNC), LintId::of(&atomic_ordering::INVALID_ATOMIC_ORDERING), @@ -2002,7 +2004,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&transmute::UNSOUND_COLLECTION_TRANSMUTE), LintId::of(&transmute::WRONG_TRANSMUTE), LintId::of(&transmuting_null::TRANSMUTING_NULL), - LintId::of(&types::ABSURD_EXTREME_COMPARISONS), LintId::of(&undropped_manually_drops::UNDROPPED_MANUALLY_DROPS), LintId::of(&unicode::INVISIBLE_CHARACTERS), LintId::of(&unit_return_expecting_ord::UNIT_RETURN_EXPECTING_ORD), diff --git a/clippy_lints/src/types/mod.rs b/clippy_lints/src/types/mod.rs index c73c1c9d92db..0c50ed8348ec 100644 --- a/clippy_lints/src/types/mod.rs +++ b/clippy_lints/src/types/mod.rs @@ -13,16 +13,16 @@ use std::borrow::Cow; use std::cmp::Ordering; use std::collections::BTreeMap; -use clippy_utils::diagnostics::{multispan_sugg, span_lint, span_lint_and_help, span_lint_and_then}; +use clippy_utils::diagnostics::{multispan_sugg, span_lint, span_lint_and_then}; use clippy_utils::source::{snippet, snippet_opt}; -use clippy_utils::ty::{is_isize_or_usize, is_type_diagnostic_item}; +use clippy_utils::ty::is_type_diagnostic_item; use if_chain::if_chain; use rustc_errors::DiagnosticBuilder; use rustc_hir as hir; use rustc_hir::intravisit::{walk_body, walk_expr, walk_ty, FnKind, NestedVisitorMap, Visitor}; use rustc_hir::{ - BinOpKind, Body, Expr, ExprKind, FnDecl, FnRetTy, FnSig, GenericArg, GenericParamKind, HirId, ImplItem, - ImplItemKind, Item, ItemKind, Local, MutTy, QPath, TraitFn, TraitItem, TraitItemKind, TyKind, + Body, Expr, ExprKind, FnDecl, FnRetTy, FnSig, GenericArg, GenericParamKind, HirId, ImplItem, ImplItemKind, Item, + ItemKind, Local, MutTy, QPath, TraitFn, TraitItem, TraitItemKind, TyKind, }; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::hir::map::Map; @@ -37,7 +37,7 @@ use rustc_typeck::hir_ty_to_ty; use crate::consts::{constant, Constant}; use clippy_utils::paths; -use clippy_utils::{clip, comparisons, differing_macro_contexts, int_bits, match_path, sext, unsext}; +use clippy_utils::{comparisons, differing_macro_contexts, match_path, sext}; declare_clippy_lint! { /// **What it does:** Checks for use of `Box>` anywhere in the code. @@ -552,174 +552,6 @@ impl<'tcx> Visitor<'tcx> for TypeComplexityVisitor { } } -declare_clippy_lint! { - /// **What it does:** Checks for comparisons where one side of the relation is - /// either the minimum or maximum value for its type and warns if it involves a - /// case that is always true or always false. Only integer and boolean types are - /// checked. - /// - /// **Why is this bad?** An expression like `min <= x` may misleadingly imply - /// that it is possible for `x` to be less than the minimum. Expressions like - /// `max < x` are probably mistakes. - /// - /// **Known problems:** For `usize` the size of the current compile target will - /// be assumed (e.g., 64 bits on 64 bit systems). This means code that uses such - /// a comparison to detect target pointer width will trigger this lint. One can - /// use `mem::sizeof` and compare its value or conditional compilation - /// attributes - /// like `#[cfg(target_pointer_width = "64")] ..` instead. - /// - /// **Example:** - /// - /// ```rust - /// let vec: Vec = Vec::new(); - /// if vec.len() <= 0 {} - /// if 100 > i32::MAX {} - /// ``` - pub ABSURD_EXTREME_COMPARISONS, - correctness, - "a comparison with a maximum or minimum value that is always true or false" -} - -declare_lint_pass!(AbsurdExtremeComparisons => [ABSURD_EXTREME_COMPARISONS]); - -enum ExtremeType { - Minimum, - Maximum, -} - -struct ExtremeExpr<'a> { - which: ExtremeType, - expr: &'a Expr<'a>, -} - -enum AbsurdComparisonResult { - AlwaysFalse, - AlwaysTrue, - InequalityImpossible, -} - -fn is_cast_between_fixed_and_target<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { - if let ExprKind::Cast(ref cast_exp, _) = expr.kind { - let precast_ty = cx.typeck_results().expr_ty(cast_exp); - let cast_ty = cx.typeck_results().expr_ty(expr); - - return is_isize_or_usize(precast_ty) != is_isize_or_usize(cast_ty); - } - - false -} - -fn detect_absurd_comparison<'tcx>( - cx: &LateContext<'tcx>, - op: BinOpKind, - lhs: &'tcx Expr<'_>, - rhs: &'tcx Expr<'_>, -) -> Option<(ExtremeExpr<'tcx>, AbsurdComparisonResult)> { - use crate::types::AbsurdComparisonResult::{AlwaysFalse, AlwaysTrue, InequalityImpossible}; - use crate::types::ExtremeType::{Maximum, Minimum}; - use clippy_utils::comparisons::{normalize_comparison, Rel}; - - // absurd comparison only makes sense on primitive types - // primitive types don't implement comparison operators with each other - if cx.typeck_results().expr_ty(lhs) != cx.typeck_results().expr_ty(rhs) { - return None; - } - - // comparisons between fix sized types and target sized types are considered unanalyzable - if is_cast_between_fixed_and_target(cx, lhs) || is_cast_between_fixed_and_target(cx, rhs) { - return None; - } - - let (rel, normalized_lhs, normalized_rhs) = normalize_comparison(op, lhs, rhs)?; - - let lx = detect_extreme_expr(cx, normalized_lhs); - let rx = detect_extreme_expr(cx, normalized_rhs); - - Some(match rel { - Rel::Lt => { - match (lx, rx) { - (Some(l @ ExtremeExpr { which: Maximum, .. }), _) => (l, AlwaysFalse), // max < x - (_, Some(r @ ExtremeExpr { which: Minimum, .. })) => (r, AlwaysFalse), // x < min - _ => return None, - } - }, - Rel::Le => { - match (lx, rx) { - (Some(l @ ExtremeExpr { which: Minimum, .. }), _) => (l, AlwaysTrue), // min <= x - (Some(l @ ExtremeExpr { which: Maximum, .. }), _) => (l, InequalityImpossible), // max <= x - (_, Some(r @ ExtremeExpr { which: Minimum, .. })) => (r, InequalityImpossible), // x <= min - (_, Some(r @ ExtremeExpr { which: Maximum, .. })) => (r, AlwaysTrue), // x <= max - _ => return None, - } - }, - Rel::Ne | Rel::Eq => return None, - }) -} - -fn detect_extreme_expr<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option> { - use crate::types::ExtremeType::{Maximum, Minimum}; - - let ty = cx.typeck_results().expr_ty(expr); - - let cv = constant(cx, cx.typeck_results(), expr)?.0; - - let which = match (ty.kind(), cv) { - (&ty::Bool, Constant::Bool(false)) | (&ty::Uint(_), Constant::Int(0)) => Minimum, - (&ty::Int(ity), Constant::Int(i)) if i == unsext(cx.tcx, i128::MIN >> (128 - int_bits(cx.tcx, ity)), ity) => { - Minimum - }, - - (&ty::Bool, Constant::Bool(true)) => Maximum, - (&ty::Int(ity), Constant::Int(i)) if i == unsext(cx.tcx, i128::MAX >> (128 - int_bits(cx.tcx, ity)), ity) => { - Maximum - }, - (&ty::Uint(uty), Constant::Int(i)) if clip(cx.tcx, u128::MAX, uty) == i => Maximum, - - _ => return None, - }; - Some(ExtremeExpr { which, expr }) -} - -impl<'tcx> LateLintPass<'tcx> for AbsurdExtremeComparisons { - fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - use crate::types::AbsurdComparisonResult::{AlwaysFalse, AlwaysTrue, InequalityImpossible}; - use crate::types::ExtremeType::{Maximum, Minimum}; - - if let ExprKind::Binary(ref cmp, ref lhs, ref rhs) = expr.kind { - if let Some((culprit, result)) = detect_absurd_comparison(cx, cmp.node, lhs, rhs) { - if !expr.span.from_expansion() { - let msg = "this comparison involving the minimum or maximum element for this \ - type contains a case that is always true or always false"; - - let conclusion = match result { - AlwaysFalse => "this comparison is always false".to_owned(), - AlwaysTrue => "this comparison is always true".to_owned(), - InequalityImpossible => format!( - "the case where the two sides are not equal never occurs, consider using `{} == {}` \ - instead", - snippet(cx, lhs.span, "lhs"), - snippet(cx, rhs.span, "rhs") - ), - }; - - let help = format!( - "because `{}` is the {} value for this type, {}", - snippet(cx, culprit.expr.span, "x"), - match culprit.which { - Minimum => "minimum", - Maximum => "maximum", - }, - conclusion - ); - - span_lint_and_help(cx, ABSURD_EXTREME_COMPARISONS, expr.span, msg, None, &help); - } - } - } - } -} - declare_clippy_lint! { /// **What it does:** Checks for comparisons where the relation is always either /// true or false, but where one side has been upcast so that the comparison is From f231b59b9e3fc1ea074a269b006421295d5402d0 Mon Sep 17 00:00:00 2001 From: Yoshitomo Nakanishi Date: Thu, 18 Mar 2021 16:15:19 +0900 Subject: [PATCH 2/6] Move invalid_upcast_comparisons to its own module --- .../src/invalid_upcast_comparisons.rs | 221 ++++++++++++++++++ clippy_lints/src/lib.rs | 7 +- clippy_lints/src/types/mod.rs | 215 +---------------- 3 files changed, 227 insertions(+), 216 deletions(-) create mode 100644 clippy_lints/src/invalid_upcast_comparisons.rs diff --git a/clippy_lints/src/invalid_upcast_comparisons.rs b/clippy_lints/src/invalid_upcast_comparisons.rs new file mode 100644 index 000000000000..d183fc41f315 --- /dev/null +++ b/clippy_lints/src/invalid_upcast_comparisons.rs @@ -0,0 +1,221 @@ +use std::cmp::Ordering; + +use rustc_hir::{Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::{self, IntTy, UintTy}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::Span; +use rustc_target::abi::LayoutOf; + +use crate::consts::{constant, Constant}; + +use clippy_utils::comparisons::Rel; +use clippy_utils::diagnostics::span_lint; +use clippy_utils::source::snippet; +use clippy_utils::{comparisons, sext}; + +declare_clippy_lint! { + /// **What it does:** Checks for comparisons where the relation is always either + /// true or false, but where one side has been upcast so that the comparison is + /// necessary. Only integer types are checked. + /// + /// **Why is this bad?** An expression like `let x : u8 = ...; (x as u32) > 300` + /// will mistakenly imply that it is possible for `x` to be outside the range of + /// `u8`. + /// + /// **Known problems:** + /// https://github.com/rust-lang/rust-clippy/issues/886 + /// + /// **Example:** + /// ```rust + /// let x: u8 = 1; + /// (x as u32) > 300; + /// ``` + pub INVALID_UPCAST_COMPARISONS, + pedantic, + "a comparison involving an upcast which is always true or false" +} + +declare_lint_pass!(InvalidUpcastComparisons => [INVALID_UPCAST_COMPARISONS]); + +#[derive(Copy, Clone, Debug, Eq)] +enum FullInt { + S(i128), + U(u128), +} + +impl FullInt { + #[allow(clippy::cast_sign_loss)] + #[must_use] + fn cmp_s_u(s: i128, u: u128) -> Ordering { + if s < 0 { + Ordering::Less + } else if u > (i128::MAX as u128) { + Ordering::Greater + } else { + (s as u128).cmp(&u) + } + } +} + +impl PartialEq for FullInt { + #[must_use] + fn eq(&self, other: &Self) -> bool { + self.partial_cmp(other).expect("`partial_cmp` only returns `Some(_)`") == Ordering::Equal + } +} + +impl PartialOrd for FullInt { + #[must_use] + fn partial_cmp(&self, other: &Self) -> Option { + Some(match (self, other) { + (&Self::S(s), &Self::S(o)) => s.cmp(&o), + (&Self::U(s), &Self::U(o)) => s.cmp(&o), + (&Self::S(s), &Self::U(o)) => Self::cmp_s_u(s, o), + (&Self::U(s), &Self::S(o)) => Self::cmp_s_u(o, s).reverse(), + }) + } +} + +impl Ord for FullInt { + #[must_use] + fn cmp(&self, other: &Self) -> Ordering { + self.partial_cmp(other) + .expect("`partial_cmp` for FullInt can never return `None`") + } +} + +fn numeric_cast_precast_bounds<'a>(cx: &LateContext<'_>, expr: &'a Expr<'_>) -> Option<(FullInt, FullInt)> { + if let ExprKind::Cast(ref cast_exp, _) = expr.kind { + let pre_cast_ty = cx.typeck_results().expr_ty(cast_exp); + let cast_ty = cx.typeck_results().expr_ty(expr); + // if it's a cast from i32 to u32 wrapping will invalidate all these checks + if cx.layout_of(pre_cast_ty).ok().map(|l| l.size) == cx.layout_of(cast_ty).ok().map(|l| l.size) { + return None; + } + match pre_cast_ty.kind() { + ty::Int(int_ty) => Some(match int_ty { + IntTy::I8 => (FullInt::S(i128::from(i8::MIN)), FullInt::S(i128::from(i8::MAX))), + IntTy::I16 => (FullInt::S(i128::from(i16::MIN)), FullInt::S(i128::from(i16::MAX))), + IntTy::I32 => (FullInt::S(i128::from(i32::MIN)), FullInt::S(i128::from(i32::MAX))), + IntTy::I64 => (FullInt::S(i128::from(i64::MIN)), FullInt::S(i128::from(i64::MAX))), + IntTy::I128 => (FullInt::S(i128::MIN), FullInt::S(i128::MAX)), + IntTy::Isize => (FullInt::S(isize::MIN as i128), FullInt::S(isize::MAX as i128)), + }), + ty::Uint(uint_ty) => Some(match uint_ty { + UintTy::U8 => (FullInt::U(u128::from(u8::MIN)), FullInt::U(u128::from(u8::MAX))), + UintTy::U16 => (FullInt::U(u128::from(u16::MIN)), FullInt::U(u128::from(u16::MAX))), + UintTy::U32 => (FullInt::U(u128::from(u32::MIN)), FullInt::U(u128::from(u32::MAX))), + UintTy::U64 => (FullInt::U(u128::from(u64::MIN)), FullInt::U(u128::from(u64::MAX))), + UintTy::U128 => (FullInt::U(u128::MIN), FullInt::U(u128::MAX)), + UintTy::Usize => (FullInt::U(usize::MIN as u128), FullInt::U(usize::MAX as u128)), + }), + _ => None, + } + } else { + None + } +} + +fn node_as_const_fullint<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option { + let val = constant(cx, cx.typeck_results(), expr)?.0; + if let Constant::Int(const_int) = val { + match *cx.typeck_results().expr_ty(expr).kind() { + ty::Int(ity) => Some(FullInt::S(sext(cx.tcx, const_int, ity))), + ty::Uint(_) => Some(FullInt::U(const_int)), + _ => None, + } + } else { + None + } +} + +fn err_upcast_comparison(cx: &LateContext<'_>, span: Span, expr: &Expr<'_>, always: bool) { + if let ExprKind::Cast(ref cast_val, _) = expr.kind { + span_lint( + cx, + INVALID_UPCAST_COMPARISONS, + span, + &format!( + "because of the numeric bounds on `{}` prior to casting, this expression is always {}", + snippet(cx, cast_val.span, "the expression"), + if always { "true" } else { "false" }, + ), + ); + } +} + +fn upcast_comparison_bounds_err<'tcx>( + cx: &LateContext<'tcx>, + span: Span, + rel: comparisons::Rel, + lhs_bounds: Option<(FullInt, FullInt)>, + lhs: &'tcx Expr<'_>, + rhs: &'tcx Expr<'_>, + invert: bool, +) { + if let Some((lb, ub)) = lhs_bounds { + if let Some(norm_rhs_val) = node_as_const_fullint(cx, rhs) { + if rel == Rel::Eq || rel == Rel::Ne { + if norm_rhs_val < lb || norm_rhs_val > ub { + err_upcast_comparison(cx, span, lhs, rel == Rel::Ne); + } + } else if match rel { + Rel::Lt => { + if invert { + norm_rhs_val < lb + } else { + ub < norm_rhs_val + } + }, + Rel::Le => { + if invert { + norm_rhs_val <= lb + } else { + ub <= norm_rhs_val + } + }, + Rel::Eq | Rel::Ne => unreachable!(), + } { + err_upcast_comparison(cx, span, lhs, true) + } else if match rel { + Rel::Lt => { + if invert { + norm_rhs_val >= ub + } else { + lb >= norm_rhs_val + } + }, + Rel::Le => { + if invert { + norm_rhs_val > ub + } else { + lb > norm_rhs_val + } + }, + Rel::Eq | Rel::Ne => unreachable!(), + } { + err_upcast_comparison(cx, span, lhs, false) + } + } + } +} + +impl<'tcx> LateLintPass<'tcx> for InvalidUpcastComparisons { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + if let ExprKind::Binary(ref cmp, ref lhs, ref rhs) = expr.kind { + let normalized = comparisons::normalize_comparison(cmp.node, lhs, rhs); + let (rel, normalized_lhs, normalized_rhs) = if let Some(val) = normalized { + val + } else { + return; + }; + + let lhs_bounds = numeric_cast_precast_bounds(cx, normalized_lhs); + let rhs_bounds = numeric_cast_precast_bounds(cx, normalized_rhs); + + upcast_comparison_bounds_err(cx, expr.span, rel, lhs_bounds, normalized_lhs, normalized_rhs, false); + upcast_comparison_bounds_err(cx, expr.span, rel, rhs_bounds, normalized_rhs, normalized_lhs, true); + } + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 7b261121f518..c16be6da11a8 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -242,6 +242,7 @@ mod inherent_to_string; mod inline_fn_without_body; mod int_plus_one; mod integer_division; +mod invalid_upcast_comparisons; mod items_after_statements; mod large_const_arrays; mod large_enum_variant; @@ -697,6 +698,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &inline_fn_without_body::INLINE_FN_WITHOUT_BODY, &int_plus_one::INT_PLUS_ONE, &integer_division::INTEGER_DIVISION, + &invalid_upcast_comparisons::INVALID_UPCAST_COMPARISONS, &items_after_statements::ITEMS_AFTER_STATEMENTS, &large_const_arrays::LARGE_CONST_ARRAYS, &large_enum_variant::LARGE_ENUM_VARIANT, @@ -960,7 +962,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &types::BORROWED_BOX, &types::BOX_VEC, &types::IMPLICIT_HASHER, - &types::INVALID_UPCAST_COMPARISONS, &types::LINKEDLIST, &types::OPTION_OPTION, &types::RC_BUFFER, @@ -1114,7 +1115,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box drop_forget_ref::DropForgetRef); store.register_late_pass(|| box empty_enum::EmptyEnum); store.register_late_pass(|| box absurd_extreme_comparisons::AbsurdExtremeComparisons); - store.register_late_pass(|| box types::InvalidUpcastComparisons); + store.register_late_pass(|| box invalid_upcast_comparisons::InvalidUpcastComparisons); store.register_late_pass(|| box regex::Regex::default()); store.register_late_pass(|| box copies::CopyAndPaste); store.register_late_pass(|| box copy_iterator::CopyIterator); @@ -1374,6 +1375,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&if_not_else::IF_NOT_ELSE), LintId::of(&implicit_saturating_sub::IMPLICIT_SATURATING_SUB), LintId::of(&infinite_iter::MAYBE_INFINITE_ITER), + LintId::of(&invalid_upcast_comparisons::INVALID_UPCAST_COMPARISONS), LintId::of(&items_after_statements::ITEMS_AFTER_STATEMENTS), LintId::of(&large_stack_arrays::LARGE_STACK_ARRAYS), LintId::of(&let_underscore::LET_UNDERSCORE_DROP), @@ -1413,7 +1415,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&trait_bounds::TRAIT_DUPLICATION_IN_BOUNDS), LintId::of(&trait_bounds::TYPE_REPETITION_IN_BOUNDS), LintId::of(&types::IMPLICIT_HASHER), - LintId::of(&types::INVALID_UPCAST_COMPARISONS), LintId::of(&types::LINKEDLIST), LintId::of(&types::OPTION_OPTION), LintId::of(&unicode::NON_ASCII_LITERAL), diff --git a/clippy_lints/src/types/mod.rs b/clippy_lints/src/types/mod.rs index 0c50ed8348ec..785f2c511d0f 100644 --- a/clippy_lints/src/types/mod.rs +++ b/clippy_lints/src/types/mod.rs @@ -10,7 +10,6 @@ mod utils; mod vec_box; use std::borrow::Cow; -use std::cmp::Ordering; use std::collections::BTreeMap; use clippy_utils::diagnostics::{multispan_sugg, span_lint, span_lint_and_then}; @@ -27,17 +26,15 @@ use rustc_hir::{ use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::hir::map::Map; use rustc_middle::lint::in_external_macro; -use rustc_middle::ty::{self, IntTy, Ty, TyS, TypeckResults, UintTy}; +use rustc_middle::ty::{Ty, TyS, TypeckResults}; use rustc_session::{declare_lint_pass, declare_tool_lint, impl_lint_pass}; use rustc_span::source_map::Span; use rustc_span::symbol::sym; -use rustc_target::abi::LayoutOf; use rustc_target::spec::abi::Abi; use rustc_typeck::hir_ty_to_ty; -use crate::consts::{constant, Constant}; use clippy_utils::paths; -use clippy_utils::{comparisons, differing_macro_contexts, match_path, sext}; +use clippy_utils::{differing_macro_contexts, match_path}; declare_clippy_lint! { /// **What it does:** Checks for use of `Box>` anywhere in the code. @@ -552,214 +549,6 @@ impl<'tcx> Visitor<'tcx> for TypeComplexityVisitor { } } -declare_clippy_lint! { - /// **What it does:** Checks for comparisons where the relation is always either - /// true or false, but where one side has been upcast so that the comparison is - /// necessary. Only integer types are checked. - /// - /// **Why is this bad?** An expression like `let x : u8 = ...; (x as u32) > 300` - /// will mistakenly imply that it is possible for `x` to be outside the range of - /// `u8`. - /// - /// **Known problems:** - /// https://github.com/rust-lang/rust-clippy/issues/886 - /// - /// **Example:** - /// ```rust - /// let x: u8 = 1; - /// (x as u32) > 300; - /// ``` - pub INVALID_UPCAST_COMPARISONS, - pedantic, - "a comparison involving an upcast which is always true or false" -} - -declare_lint_pass!(InvalidUpcastComparisons => [INVALID_UPCAST_COMPARISONS]); - -#[derive(Copy, Clone, Debug, Eq)] -enum FullInt { - S(i128), - U(u128), -} - -impl FullInt { - #[allow(clippy::cast_sign_loss)] - #[must_use] - fn cmp_s_u(s: i128, u: u128) -> Ordering { - if s < 0 { - Ordering::Less - } else if u > (i128::MAX as u128) { - Ordering::Greater - } else { - (s as u128).cmp(&u) - } - } -} - -impl PartialEq for FullInt { - #[must_use] - fn eq(&self, other: &Self) -> bool { - self.partial_cmp(other).expect("`partial_cmp` only returns `Some(_)`") == Ordering::Equal - } -} - -impl PartialOrd for FullInt { - #[must_use] - fn partial_cmp(&self, other: &Self) -> Option { - Some(match (self, other) { - (&Self::S(s), &Self::S(o)) => s.cmp(&o), - (&Self::U(s), &Self::U(o)) => s.cmp(&o), - (&Self::S(s), &Self::U(o)) => Self::cmp_s_u(s, o), - (&Self::U(s), &Self::S(o)) => Self::cmp_s_u(o, s).reverse(), - }) - } -} - -impl Ord for FullInt { - #[must_use] - fn cmp(&self, other: &Self) -> Ordering { - self.partial_cmp(other) - .expect("`partial_cmp` for FullInt can never return `None`") - } -} - -fn numeric_cast_precast_bounds<'a>(cx: &LateContext<'_>, expr: &'a Expr<'_>) -> Option<(FullInt, FullInt)> { - if let ExprKind::Cast(ref cast_exp, _) = expr.kind { - let pre_cast_ty = cx.typeck_results().expr_ty(cast_exp); - let cast_ty = cx.typeck_results().expr_ty(expr); - // if it's a cast from i32 to u32 wrapping will invalidate all these checks - if cx.layout_of(pre_cast_ty).ok().map(|l| l.size) == cx.layout_of(cast_ty).ok().map(|l| l.size) { - return None; - } - match pre_cast_ty.kind() { - ty::Int(int_ty) => Some(match int_ty { - IntTy::I8 => (FullInt::S(i128::from(i8::MIN)), FullInt::S(i128::from(i8::MAX))), - IntTy::I16 => (FullInt::S(i128::from(i16::MIN)), FullInt::S(i128::from(i16::MAX))), - IntTy::I32 => (FullInt::S(i128::from(i32::MIN)), FullInt::S(i128::from(i32::MAX))), - IntTy::I64 => (FullInt::S(i128::from(i64::MIN)), FullInt::S(i128::from(i64::MAX))), - IntTy::I128 => (FullInt::S(i128::MIN), FullInt::S(i128::MAX)), - IntTy::Isize => (FullInt::S(isize::MIN as i128), FullInt::S(isize::MAX as i128)), - }), - ty::Uint(uint_ty) => Some(match uint_ty { - UintTy::U8 => (FullInt::U(u128::from(u8::MIN)), FullInt::U(u128::from(u8::MAX))), - UintTy::U16 => (FullInt::U(u128::from(u16::MIN)), FullInt::U(u128::from(u16::MAX))), - UintTy::U32 => (FullInt::U(u128::from(u32::MIN)), FullInt::U(u128::from(u32::MAX))), - UintTy::U64 => (FullInt::U(u128::from(u64::MIN)), FullInt::U(u128::from(u64::MAX))), - UintTy::U128 => (FullInt::U(u128::MIN), FullInt::U(u128::MAX)), - UintTy::Usize => (FullInt::U(usize::MIN as u128), FullInt::U(usize::MAX as u128)), - }), - _ => None, - } - } else { - None - } -} - -fn node_as_const_fullint<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option { - let val = constant(cx, cx.typeck_results(), expr)?.0; - if let Constant::Int(const_int) = val { - match *cx.typeck_results().expr_ty(expr).kind() { - ty::Int(ity) => Some(FullInt::S(sext(cx.tcx, const_int, ity))), - ty::Uint(_) => Some(FullInt::U(const_int)), - _ => None, - } - } else { - None - } -} - -fn err_upcast_comparison(cx: &LateContext<'_>, span: Span, expr: &Expr<'_>, always: bool) { - if let ExprKind::Cast(ref cast_val, _) = expr.kind { - span_lint( - cx, - INVALID_UPCAST_COMPARISONS, - span, - &format!( - "because of the numeric bounds on `{}` prior to casting, this expression is always {}", - snippet(cx, cast_val.span, "the expression"), - if always { "true" } else { "false" }, - ), - ); - } -} - -fn upcast_comparison_bounds_err<'tcx>( - cx: &LateContext<'tcx>, - span: Span, - rel: comparisons::Rel, - lhs_bounds: Option<(FullInt, FullInt)>, - lhs: &'tcx Expr<'_>, - rhs: &'tcx Expr<'_>, - invert: bool, -) { - use clippy_utils::comparisons::Rel; - - if let Some((lb, ub)) = lhs_bounds { - if let Some(norm_rhs_val) = node_as_const_fullint(cx, rhs) { - if rel == Rel::Eq || rel == Rel::Ne { - if norm_rhs_val < lb || norm_rhs_val > ub { - err_upcast_comparison(cx, span, lhs, rel == Rel::Ne); - } - } else if match rel { - Rel::Lt => { - if invert { - norm_rhs_val < lb - } else { - ub < norm_rhs_val - } - }, - Rel::Le => { - if invert { - norm_rhs_val <= lb - } else { - ub <= norm_rhs_val - } - }, - Rel::Eq | Rel::Ne => unreachable!(), - } { - err_upcast_comparison(cx, span, lhs, true) - } else if match rel { - Rel::Lt => { - if invert { - norm_rhs_val >= ub - } else { - lb >= norm_rhs_val - } - }, - Rel::Le => { - if invert { - norm_rhs_val > ub - } else { - lb > norm_rhs_val - } - }, - Rel::Eq | Rel::Ne => unreachable!(), - } { - err_upcast_comparison(cx, span, lhs, false) - } - } - } -} - -impl<'tcx> LateLintPass<'tcx> for InvalidUpcastComparisons { - fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if let ExprKind::Binary(ref cmp, ref lhs, ref rhs) = expr.kind { - let normalized = comparisons::normalize_comparison(cmp.node, lhs, rhs); - let (rel, normalized_lhs, normalized_rhs) = if let Some(val) = normalized { - val - } else { - return; - }; - - let lhs_bounds = numeric_cast_precast_bounds(cx, normalized_lhs); - let rhs_bounds = numeric_cast_precast_bounds(cx, normalized_rhs); - - upcast_comparison_bounds_err(cx, expr.span, rel, lhs_bounds, normalized_lhs, normalized_rhs, false); - upcast_comparison_bounds_err(cx, expr.span, rel, rhs_bounds, normalized_rhs, normalized_lhs, true); - } - } -} - declare_clippy_lint! { /// **What it does:** Checks for public `impl` or `fn` missing generalization /// over different hashers and implicitly defaulting to the default hashing From dad39b6613645aefd6671f1378cafa1c8f2d5f65 Mon Sep 17 00:00:00 2001 From: Yoshitomo Nakanishi Date: Thu, 18 Mar 2021 16:34:22 +0900 Subject: [PATCH 3/6] Move implicit_hasher to its own module --- clippy_lints/src/implicit_hasher.rs | 377 +++++++++++++++++++++++++++ clippy_lints/src/lib.rs | 7 +- clippy_lints/src/types/mod.rs | 380 +--------------------------- 3 files changed, 387 insertions(+), 377 deletions(-) create mode 100644 clippy_lints/src/implicit_hasher.rs diff --git a/clippy_lints/src/implicit_hasher.rs b/clippy_lints/src/implicit_hasher.rs new file mode 100644 index 000000000000..0b748b4d72de --- /dev/null +++ b/clippy_lints/src/implicit_hasher.rs @@ -0,0 +1,377 @@ +#![allow(rustc::default_hash_types)] + +use std::borrow::Cow; +use std::collections::BTreeMap; + +use rustc_errors::DiagnosticBuilder; +use rustc_hir as hir; +use rustc_hir::intravisit::{walk_body, walk_expr, walk_ty, NestedVisitorMap, Visitor}; +use rustc_hir::{Body, Expr, ExprKind, GenericArg, Item, ItemKind, QPath, TyKind}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::hir::map::Map; +use rustc_middle::lint::in_external_macro; +use rustc_middle::ty::{Ty, TyS, TypeckResults}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::source_map::Span; +use rustc_span::symbol::sym; +use rustc_typeck::hir_ty_to_ty; + +use if_chain::if_chain; + +use clippy_utils::diagnostics::{multispan_sugg, span_lint_and_then}; +use clippy_utils::paths; +use clippy_utils::source::{snippet, snippet_opt}; +use clippy_utils::ty::is_type_diagnostic_item; +use clippy_utils::{differing_macro_contexts, match_path}; + +declare_clippy_lint! { + /// **What it does:** Checks for public `impl` or `fn` missing generalization + /// over different hashers and implicitly defaulting to the default hashing + /// algorithm (`SipHash`). + /// + /// **Why is this bad?** `HashMap` or `HashSet` with custom hashers cannot be + /// used with them. + /// + /// **Known problems:** Suggestions for replacing constructors can contain + /// false-positives. Also applying suggestions can require modification of other + /// pieces of code, possibly including external crates. + /// + /// **Example:** + /// ```rust + /// # use std::collections::HashMap; + /// # use std::hash::{Hash, BuildHasher}; + /// # trait Serialize {}; + /// impl Serialize for HashMap { } + /// + /// pub fn foo(map: &mut HashMap) { } + /// ``` + /// could be rewritten as + /// ```rust + /// # use std::collections::HashMap; + /// # use std::hash::{Hash, BuildHasher}; + /// # trait Serialize {}; + /// impl Serialize for HashMap { } + /// + /// pub fn foo(map: &mut HashMap) { } + /// ``` + pub IMPLICIT_HASHER, + pedantic, + "missing generalization over different hashers" +} + +declare_lint_pass!(ImplicitHasher => [IMPLICIT_HASHER]); + +impl<'tcx> LateLintPass<'tcx> for ImplicitHasher { + #[allow(clippy::cast_possible_truncation, clippy::too_many_lines)] + fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) { + use rustc_span::BytePos; + + fn suggestion<'tcx>( + cx: &LateContext<'tcx>, + diag: &mut DiagnosticBuilder<'_>, + generics_span: Span, + generics_suggestion_span: Span, + target: &ImplicitHasherType<'_>, + vis: ImplicitHasherConstructorVisitor<'_, '_, '_>, + ) { + let generics_snip = snippet(cx, generics_span, ""); + // trim `<` `>` + let generics_snip = if generics_snip.is_empty() { + "" + } else { + &generics_snip[1..generics_snip.len() - 1] + }; + + multispan_sugg( + diag, + "consider adding a type parameter", + vec![ + ( + generics_suggestion_span, + format!( + "<{}{}S: ::std::hash::BuildHasher{}>", + generics_snip, + if generics_snip.is_empty() { "" } else { ", " }, + if vis.suggestions.is_empty() { + "" + } else { + // request users to add `Default` bound so that generic constructors can be used + " + Default" + }, + ), + ), + ( + target.span(), + format!("{}<{}, S>", target.type_name(), target.type_arguments(),), + ), + ], + ); + + if !vis.suggestions.is_empty() { + multispan_sugg(diag, "...and use generic constructor", vis.suggestions); + } + } + + if !cx.access_levels.is_exported(item.hir_id()) { + return; + } + + match item.kind { + ItemKind::Impl(ref impl_) => { + let mut vis = ImplicitHasherTypeVisitor::new(cx); + vis.visit_ty(impl_.self_ty); + + for target in &vis.found { + if differing_macro_contexts(item.span, target.span()) { + return; + } + + let generics_suggestion_span = impl_.generics.span.substitute_dummy({ + let pos = snippet_opt(cx, item.span.until(target.span())) + .and_then(|snip| Some(item.span.lo() + BytePos(snip.find("impl")? as u32 + 4))); + if let Some(pos) = pos { + Span::new(pos, pos, item.span.data().ctxt) + } else { + return; + } + }); + + let mut ctr_vis = ImplicitHasherConstructorVisitor::new(cx, target); + for item in impl_.items.iter().map(|item| cx.tcx.hir().impl_item(item.id)) { + ctr_vis.visit_impl_item(item); + } + + span_lint_and_then( + cx, + IMPLICIT_HASHER, + target.span(), + &format!( + "impl for `{}` should be generalized over different hashers", + target.type_name() + ), + move |diag| { + suggestion(cx, diag, impl_.generics.span, generics_suggestion_span, target, ctr_vis); + }, + ); + } + }, + ItemKind::Fn(ref sig, ref generics, body_id) => { + let body = cx.tcx.hir().body(body_id); + + for ty in sig.decl.inputs { + let mut vis = ImplicitHasherTypeVisitor::new(cx); + vis.visit_ty(ty); + + for target in &vis.found { + if in_external_macro(cx.sess(), generics.span) { + continue; + } + let generics_suggestion_span = generics.span.substitute_dummy({ + let pos = snippet_opt(cx, item.span.until(body.params[0].pat.span)) + .and_then(|snip| { + let i = snip.find("fn")?; + Some(item.span.lo() + BytePos((i + (&snip[i..]).find('(')?) as u32)) + }) + .expect("failed to create span for type parameters"); + Span::new(pos, pos, item.span.data().ctxt) + }); + + let mut ctr_vis = ImplicitHasherConstructorVisitor::new(cx, target); + ctr_vis.visit_body(body); + + span_lint_and_then( + cx, + IMPLICIT_HASHER, + target.span(), + &format!( + "parameter of type `{}` should be generalized over different hashers", + target.type_name() + ), + move |diag| { + suggestion(cx, diag, generics.span, generics_suggestion_span, target, ctr_vis); + }, + ); + } + } + }, + _ => {}, + } + } +} + +enum ImplicitHasherType<'tcx> { + HashMap(Span, Ty<'tcx>, Cow<'static, str>, Cow<'static, str>), + HashSet(Span, Ty<'tcx>, Cow<'static, str>), +} + +impl<'tcx> ImplicitHasherType<'tcx> { + /// Checks that `ty` is a target type without a `BuildHasher`. + fn new(cx: &LateContext<'tcx>, hir_ty: &hir::Ty<'_>) -> Option { + if let TyKind::Path(QPath::Resolved(None, ref path)) = hir_ty.kind { + let params: Vec<_> = path + .segments + .last() + .as_ref()? + .args + .as_ref()? + .args + .iter() + .filter_map(|arg| match arg { + GenericArg::Type(ty) => Some(ty), + _ => None, + }) + .collect(); + let params_len = params.len(); + + let ty = hir_ty_to_ty(cx.tcx, hir_ty); + + if is_type_diagnostic_item(cx, ty, sym::hashmap_type) && params_len == 2 { + Some(ImplicitHasherType::HashMap( + hir_ty.span, + ty, + snippet(cx, params[0].span, "K"), + snippet(cx, params[1].span, "V"), + )) + } else if is_type_diagnostic_item(cx, ty, sym::hashset_type) && params_len == 1 { + Some(ImplicitHasherType::HashSet( + hir_ty.span, + ty, + snippet(cx, params[0].span, "T"), + )) + } else { + None + } + } else { + None + } + } + + fn type_name(&self) -> &'static str { + match *self { + ImplicitHasherType::HashMap(..) => "HashMap", + ImplicitHasherType::HashSet(..) => "HashSet", + } + } + + fn type_arguments(&self) -> String { + match *self { + ImplicitHasherType::HashMap(.., ref k, ref v) => format!("{}, {}", k, v), + ImplicitHasherType::HashSet(.., ref t) => format!("{}", t), + } + } + + fn ty(&self) -> Ty<'tcx> { + match *self { + ImplicitHasherType::HashMap(_, ty, ..) | ImplicitHasherType::HashSet(_, ty, ..) => ty, + } + } + + fn span(&self) -> Span { + match *self { + ImplicitHasherType::HashMap(span, ..) | ImplicitHasherType::HashSet(span, ..) => span, + } + } +} + +struct ImplicitHasherTypeVisitor<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + found: Vec>, +} + +impl<'a, 'tcx> ImplicitHasherTypeVisitor<'a, 'tcx> { + fn new(cx: &'a LateContext<'tcx>) -> Self { + Self { cx, found: vec![] } + } +} + +impl<'a, 'tcx> Visitor<'tcx> for ImplicitHasherTypeVisitor<'a, 'tcx> { + type Map = Map<'tcx>; + + fn visit_ty(&mut self, t: &'tcx hir::Ty<'_>) { + if let Some(target) = ImplicitHasherType::new(self.cx, t) { + self.found.push(target); + } + + walk_ty(self, t); + } + + fn nested_visit_map(&mut self) -> NestedVisitorMap { + NestedVisitorMap::None + } +} + +/// Looks for default-hasher-dependent constructors like `HashMap::new`. +struct ImplicitHasherConstructorVisitor<'a, 'b, 'tcx> { + cx: &'a LateContext<'tcx>, + maybe_typeck_results: Option<&'tcx TypeckResults<'tcx>>, + target: &'b ImplicitHasherType<'tcx>, + suggestions: BTreeMap, +} + +impl<'a, 'b, 'tcx> ImplicitHasherConstructorVisitor<'a, 'b, 'tcx> { + fn new(cx: &'a LateContext<'tcx>, target: &'b ImplicitHasherType<'tcx>) -> Self { + Self { + cx, + maybe_typeck_results: cx.maybe_typeck_results(), + target, + suggestions: BTreeMap::new(), + } + } +} + +impl<'a, 'b, 'tcx> Visitor<'tcx> for ImplicitHasherConstructorVisitor<'a, 'b, 'tcx> { + type Map = Map<'tcx>; + + fn visit_body(&mut self, body: &'tcx Body<'_>) { + let old_maybe_typeck_results = self.maybe_typeck_results.replace(self.cx.tcx.typeck_body(body.id())); + walk_body(self, body); + self.maybe_typeck_results = old_maybe_typeck_results; + } + + fn visit_expr(&mut self, e: &'tcx Expr<'_>) { + if_chain! { + if let ExprKind::Call(ref fun, ref args) = e.kind; + if let ExprKind::Path(QPath::TypeRelative(ref ty, ref method)) = fun.kind; + if let TyKind::Path(QPath::Resolved(None, ty_path)) = ty.kind; + then { + if !TyS::same_type(self.target.ty(), self.maybe_typeck_results.unwrap().expr_ty(e)) { + return; + } + + if match_path(ty_path, &paths::HASHMAP) { + if method.ident.name == sym::new { + self.suggestions + .insert(e.span, "HashMap::default()".to_string()); + } else if method.ident.name == sym!(with_capacity) { + self.suggestions.insert( + e.span, + format!( + "HashMap::with_capacity_and_hasher({}, Default::default())", + snippet(self.cx, args[0].span, "capacity"), + ), + ); + } + } else if match_path(ty_path, &paths::HASHSET) { + if method.ident.name == sym::new { + self.suggestions + .insert(e.span, "HashSet::default()".to_string()); + } else if method.ident.name == sym!(with_capacity) { + self.suggestions.insert( + e.span, + format!( + "HashSet::with_capacity_and_hasher({}, Default::default())", + snippet(self.cx, args[0].span, "capacity"), + ), + ); + } + } + } + } + + walk_expr(self, e); + } + + fn nested_visit_map(&mut self) -> NestedVisitorMap { + NestedVisitorMap::OnlyBodies(self.cx.tcx.hir()) + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index c16be6da11a8..05165ec9d66d 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -232,6 +232,7 @@ mod if_let_mutex; mod if_let_some_result; mod if_not_else; mod if_then_some_else_none; +mod implicit_hasher; mod implicit_return; mod implicit_saturating_sub; mod inconsistent_struct_constructor; @@ -685,6 +686,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &if_let_some_result::IF_LET_SOME_RESULT, &if_not_else::IF_NOT_ELSE, &if_then_some_else_none::IF_THEN_SOME_ELSE_NONE, + &implicit_hasher::IMPLICIT_HASHER, &implicit_return::IMPLICIT_RETURN, &implicit_saturating_sub::IMPLICIT_SATURATING_SUB, &inconsistent_struct_constructor::INCONSISTENT_STRUCT_CONSTRUCTOR, @@ -961,7 +963,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &try_err::TRY_ERR, &types::BORROWED_BOX, &types::BOX_VEC, - &types::IMPLICIT_HASHER, &types::LINKEDLIST, &types::OPTION_OPTION, &types::RC_BUFFER, @@ -1159,7 +1160,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box infinite_iter::InfiniteIter); store.register_late_pass(|| box inline_fn_without_body::InlineFnWithoutBody); store.register_late_pass(|| box useless_conversion::UselessConversion::default()); - store.register_late_pass(|| box types::ImplicitHasher); + store.register_late_pass(|| box implicit_hasher::ImplicitHasher); store.register_late_pass(|| box fallible_impl_from::FallibleImplFrom); store.register_late_pass(|| box double_comparison::DoubleComparisons); store.register_late_pass(|| box question_mark::QuestionMark); @@ -1373,6 +1374,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&functions::MUST_USE_CANDIDATE), LintId::of(&functions::TOO_MANY_LINES), LintId::of(&if_not_else::IF_NOT_ELSE), + LintId::of(&implicit_hasher::IMPLICIT_HASHER), LintId::of(&implicit_saturating_sub::IMPLICIT_SATURATING_SUB), LintId::of(&infinite_iter::MAYBE_INFINITE_ITER), LintId::of(&invalid_upcast_comparisons::INVALID_UPCAST_COMPARISONS), @@ -1414,7 +1416,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&strings::STRING_ADD_ASSIGN), LintId::of(&trait_bounds::TRAIT_DUPLICATION_IN_BOUNDS), LintId::of(&trait_bounds::TYPE_REPETITION_IN_BOUNDS), - LintId::of(&types::IMPLICIT_HASHER), LintId::of(&types::LINKEDLIST), LintId::of(&types::OPTION_OPTION), LintId::of(&unicode::NON_ASCII_LITERAL), diff --git a/clippy_lints/src/types/mod.rs b/clippy_lints/src/types/mod.rs index 785f2c511d0f..d5f2b3d013ed 100644 --- a/clippy_lints/src/types/mod.rs +++ b/clippy_lints/src/types/mod.rs @@ -1,5 +1,3 @@ -#![allow(rustc::default_hash_types)] - mod borrowed_box; mod box_vec; mod linked_list; @@ -9,32 +7,18 @@ mod redundant_allocation; mod utils; mod vec_box; -use std::borrow::Cow; -use std::collections::BTreeMap; - -use clippy_utils::diagnostics::{multispan_sugg, span_lint, span_lint_and_then}; -use clippy_utils::source::{snippet, snippet_opt}; -use clippy_utils::ty::is_type_diagnostic_item; -use if_chain::if_chain; -use rustc_errors::DiagnosticBuilder; +use clippy_utils::diagnostics::span_lint; use rustc_hir as hir; -use rustc_hir::intravisit::{walk_body, walk_expr, walk_ty, FnKind, NestedVisitorMap, Visitor}; +use rustc_hir::intravisit::{walk_ty, FnKind, NestedVisitorMap, Visitor}; use rustc_hir::{ - Body, Expr, ExprKind, FnDecl, FnRetTy, FnSig, GenericArg, GenericParamKind, HirId, ImplItem, ImplItemKind, Item, - ItemKind, Local, MutTy, QPath, TraitFn, TraitItem, TraitItemKind, TyKind, + Body, FnDecl, FnRetTy, FnSig, GenericArg, GenericParamKind, HirId, ImplItem, ImplItemKind, Item, ItemKind, Local, + MutTy, QPath, TraitFn, TraitItem, TraitItemKind, TyKind, }; -use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::hir::map::Map; -use rustc_middle::lint::in_external_macro; -use rustc_middle::ty::{Ty, TyS, TypeckResults}; -use rustc_session::{declare_lint_pass, declare_tool_lint, impl_lint_pass}; +use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::source_map::Span; -use rustc_span::symbol::sym; use rustc_target::spec::abi::Abi; -use rustc_typeck::hir_ty_to_ty; - -use clippy_utils::paths; -use clippy_utils::{differing_macro_contexts, match_path}; declare_clippy_lint! { /// **What it does:** Checks for use of `Box>` anywhere in the code. @@ -548,355 +532,3 @@ impl<'tcx> Visitor<'tcx> for TypeComplexityVisitor { NestedVisitorMap::None } } - -declare_clippy_lint! { - /// **What it does:** Checks for public `impl` or `fn` missing generalization - /// over different hashers and implicitly defaulting to the default hashing - /// algorithm (`SipHash`). - /// - /// **Why is this bad?** `HashMap` or `HashSet` with custom hashers cannot be - /// used with them. - /// - /// **Known problems:** Suggestions for replacing constructors can contain - /// false-positives. Also applying suggestions can require modification of other - /// pieces of code, possibly including external crates. - /// - /// **Example:** - /// ```rust - /// # use std::collections::HashMap; - /// # use std::hash::{Hash, BuildHasher}; - /// # trait Serialize {}; - /// impl Serialize for HashMap { } - /// - /// pub fn foo(map: &mut HashMap) { } - /// ``` - /// could be rewritten as - /// ```rust - /// # use std::collections::HashMap; - /// # use std::hash::{Hash, BuildHasher}; - /// # trait Serialize {}; - /// impl Serialize for HashMap { } - /// - /// pub fn foo(map: &mut HashMap) { } - /// ``` - pub IMPLICIT_HASHER, - pedantic, - "missing generalization over different hashers" -} - -declare_lint_pass!(ImplicitHasher => [IMPLICIT_HASHER]); - -impl<'tcx> LateLintPass<'tcx> for ImplicitHasher { - #[allow(clippy::cast_possible_truncation, clippy::too_many_lines)] - fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) { - use rustc_span::BytePos; - - fn suggestion<'tcx>( - cx: &LateContext<'tcx>, - diag: &mut DiagnosticBuilder<'_>, - generics_span: Span, - generics_suggestion_span: Span, - target: &ImplicitHasherType<'_>, - vis: ImplicitHasherConstructorVisitor<'_, '_, '_>, - ) { - let generics_snip = snippet(cx, generics_span, ""); - // trim `<` `>` - let generics_snip = if generics_snip.is_empty() { - "" - } else { - &generics_snip[1..generics_snip.len() - 1] - }; - - multispan_sugg( - diag, - "consider adding a type parameter", - vec![ - ( - generics_suggestion_span, - format!( - "<{}{}S: ::std::hash::BuildHasher{}>", - generics_snip, - if generics_snip.is_empty() { "" } else { ", " }, - if vis.suggestions.is_empty() { - "" - } else { - // request users to add `Default` bound so that generic constructors can be used - " + Default" - }, - ), - ), - ( - target.span(), - format!("{}<{}, S>", target.type_name(), target.type_arguments(),), - ), - ], - ); - - if !vis.suggestions.is_empty() { - multispan_sugg(diag, "...and use generic constructor", vis.suggestions); - } - } - - if !cx.access_levels.is_exported(item.hir_id()) { - return; - } - - match item.kind { - ItemKind::Impl(ref impl_) => { - let mut vis = ImplicitHasherTypeVisitor::new(cx); - vis.visit_ty(impl_.self_ty); - - for target in &vis.found { - if differing_macro_contexts(item.span, target.span()) { - return; - } - - let generics_suggestion_span = impl_.generics.span.substitute_dummy({ - let pos = snippet_opt(cx, item.span.until(target.span())) - .and_then(|snip| Some(item.span.lo() + BytePos(snip.find("impl")? as u32 + 4))); - if let Some(pos) = pos { - Span::new(pos, pos, item.span.data().ctxt) - } else { - return; - } - }); - - let mut ctr_vis = ImplicitHasherConstructorVisitor::new(cx, target); - for item in impl_.items.iter().map(|item| cx.tcx.hir().impl_item(item.id)) { - ctr_vis.visit_impl_item(item); - } - - span_lint_and_then( - cx, - IMPLICIT_HASHER, - target.span(), - &format!( - "impl for `{}` should be generalized over different hashers", - target.type_name() - ), - move |diag| { - suggestion(cx, diag, impl_.generics.span, generics_suggestion_span, target, ctr_vis); - }, - ); - } - }, - ItemKind::Fn(ref sig, ref generics, body_id) => { - let body = cx.tcx.hir().body(body_id); - - for ty in sig.decl.inputs { - let mut vis = ImplicitHasherTypeVisitor::new(cx); - vis.visit_ty(ty); - - for target in &vis.found { - if in_external_macro(cx.sess(), generics.span) { - continue; - } - let generics_suggestion_span = generics.span.substitute_dummy({ - let pos = snippet_opt(cx, item.span.until(body.params[0].pat.span)) - .and_then(|snip| { - let i = snip.find("fn")?; - Some(item.span.lo() + BytePos((i + (&snip[i..]).find('(')?) as u32)) - }) - .expect("failed to create span for type parameters"); - Span::new(pos, pos, item.span.data().ctxt) - }); - - let mut ctr_vis = ImplicitHasherConstructorVisitor::new(cx, target); - ctr_vis.visit_body(body); - - span_lint_and_then( - cx, - IMPLICIT_HASHER, - target.span(), - &format!( - "parameter of type `{}` should be generalized over different hashers", - target.type_name() - ), - move |diag| { - suggestion(cx, diag, generics.span, generics_suggestion_span, target, ctr_vis); - }, - ); - } - } - }, - _ => {}, - } - } -} - -enum ImplicitHasherType<'tcx> { - HashMap(Span, Ty<'tcx>, Cow<'static, str>, Cow<'static, str>), - HashSet(Span, Ty<'tcx>, Cow<'static, str>), -} - -impl<'tcx> ImplicitHasherType<'tcx> { - /// Checks that `ty` is a target type without a `BuildHasher`. - fn new(cx: &LateContext<'tcx>, hir_ty: &hir::Ty<'_>) -> Option { - if let TyKind::Path(QPath::Resolved(None, ref path)) = hir_ty.kind { - let params: Vec<_> = path - .segments - .last() - .as_ref()? - .args - .as_ref()? - .args - .iter() - .filter_map(|arg| match arg { - GenericArg::Type(ty) => Some(ty), - _ => None, - }) - .collect(); - let params_len = params.len(); - - let ty = hir_ty_to_ty(cx.tcx, hir_ty); - - if is_type_diagnostic_item(cx, ty, sym::hashmap_type) && params_len == 2 { - Some(ImplicitHasherType::HashMap( - hir_ty.span, - ty, - snippet(cx, params[0].span, "K"), - snippet(cx, params[1].span, "V"), - )) - } else if is_type_diagnostic_item(cx, ty, sym::hashset_type) && params_len == 1 { - Some(ImplicitHasherType::HashSet( - hir_ty.span, - ty, - snippet(cx, params[0].span, "T"), - )) - } else { - None - } - } else { - None - } - } - - fn type_name(&self) -> &'static str { - match *self { - ImplicitHasherType::HashMap(..) => "HashMap", - ImplicitHasherType::HashSet(..) => "HashSet", - } - } - - fn type_arguments(&self) -> String { - match *self { - ImplicitHasherType::HashMap(.., ref k, ref v) => format!("{}, {}", k, v), - ImplicitHasherType::HashSet(.., ref t) => format!("{}", t), - } - } - - fn ty(&self) -> Ty<'tcx> { - match *self { - ImplicitHasherType::HashMap(_, ty, ..) | ImplicitHasherType::HashSet(_, ty, ..) => ty, - } - } - - fn span(&self) -> Span { - match *self { - ImplicitHasherType::HashMap(span, ..) | ImplicitHasherType::HashSet(span, ..) => span, - } - } -} - -struct ImplicitHasherTypeVisitor<'a, 'tcx> { - cx: &'a LateContext<'tcx>, - found: Vec>, -} - -impl<'a, 'tcx> ImplicitHasherTypeVisitor<'a, 'tcx> { - fn new(cx: &'a LateContext<'tcx>) -> Self { - Self { cx, found: vec![] } - } -} - -impl<'a, 'tcx> Visitor<'tcx> for ImplicitHasherTypeVisitor<'a, 'tcx> { - type Map = Map<'tcx>; - - fn visit_ty(&mut self, t: &'tcx hir::Ty<'_>) { - if let Some(target) = ImplicitHasherType::new(self.cx, t) { - self.found.push(target); - } - - walk_ty(self, t); - } - - fn nested_visit_map(&mut self) -> NestedVisitorMap { - NestedVisitorMap::None - } -} - -/// Looks for default-hasher-dependent constructors like `HashMap::new`. -struct ImplicitHasherConstructorVisitor<'a, 'b, 'tcx> { - cx: &'a LateContext<'tcx>, - maybe_typeck_results: Option<&'tcx TypeckResults<'tcx>>, - target: &'b ImplicitHasherType<'tcx>, - suggestions: BTreeMap, -} - -impl<'a, 'b, 'tcx> ImplicitHasherConstructorVisitor<'a, 'b, 'tcx> { - fn new(cx: &'a LateContext<'tcx>, target: &'b ImplicitHasherType<'tcx>) -> Self { - Self { - cx, - maybe_typeck_results: cx.maybe_typeck_results(), - target, - suggestions: BTreeMap::new(), - } - } -} - -impl<'a, 'b, 'tcx> Visitor<'tcx> for ImplicitHasherConstructorVisitor<'a, 'b, 'tcx> { - type Map = Map<'tcx>; - - fn visit_body(&mut self, body: &'tcx Body<'_>) { - let old_maybe_typeck_results = self.maybe_typeck_results.replace(self.cx.tcx.typeck_body(body.id())); - walk_body(self, body); - self.maybe_typeck_results = old_maybe_typeck_results; - } - - fn visit_expr(&mut self, e: &'tcx Expr<'_>) { - if_chain! { - if let ExprKind::Call(ref fun, ref args) = e.kind; - if let ExprKind::Path(QPath::TypeRelative(ref ty, ref method)) = fun.kind; - if let TyKind::Path(QPath::Resolved(None, ty_path)) = ty.kind; - then { - if !TyS::same_type(self.target.ty(), self.maybe_typeck_results.unwrap().expr_ty(e)) { - return; - } - - if match_path(ty_path, &paths::HASHMAP) { - if method.ident.name == sym::new { - self.suggestions - .insert(e.span, "HashMap::default()".to_string()); - } else if method.ident.name == sym!(with_capacity) { - self.suggestions.insert( - e.span, - format!( - "HashMap::with_capacity_and_hasher({}, Default::default())", - snippet(self.cx, args[0].span, "capacity"), - ), - ); - } - } else if match_path(ty_path, &paths::HASHSET) { - if method.ident.name == sym::new { - self.suggestions - .insert(e.span, "HashSet::default()".to_string()); - } else if method.ident.name == sym!(with_capacity) { - self.suggestions.insert( - e.span, - format!( - "HashSet::with_capacity_and_hasher({}, Default::default())", - snippet(self.cx, args[0].span, "capacity"), - ), - ); - } - } - } - } - - walk_expr(self, e); - } - - fn nested_visit_map(&mut self) -> NestedVisitorMap { - NestedVisitorMap::OnlyBodies(self.cx.tcx.hir()) - } -} From 494bc8a30cf154eede2f22178c9a7ebc404302e7 Mon Sep 17 00:00:00 2001 From: Yoshitomo Nakanishi Date: Fri, 19 Mar 2021 18:14:48 +0900 Subject: [PATCH 4/6] Fix FN that types lints don't work with const or static --- clippy_lints/src/types/mod.rs | 8 ++++++++ tests/ui/dlist.rs | 3 +++ tests/ui/dlist.stderr | 32 +++++++++++++++++++++-------- tests/ui/option_option.rs | 3 +++ tests/ui/option_option.stderr | 38 +++++++++++++++++++++++------------ tests/ui/vec_box_sized.fixed | 2 ++ tests/ui/vec_box_sized.rs | 2 ++ tests/ui/vec_box_sized.stderr | 26 +++++++++++++++++------- 8 files changed, 86 insertions(+), 28 deletions(-) diff --git a/clippy_lints/src/types/mod.rs b/clippy_lints/src/types/mod.rs index d5f2b3d013ed..1df964db38fe 100644 --- a/clippy_lints/src/types/mod.rs +++ b/clippy_lints/src/types/mod.rs @@ -249,6 +249,14 @@ impl<'tcx> LateLintPass<'tcx> for Types { self.check_fn_decl(cx, decl); } + fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) { + match item.kind { + ItemKind::Static(ref ty, _, _) | ItemKind::Const(ref ty, _) => self.check_ty(cx, ty, false), + // functions, enums, structs, impls and traits are covered + _ => (), + } + } + fn check_field_def(&mut self, cx: &LateContext<'_>, field: &hir::FieldDef<'_>) { self.check_ty(cx, &field.ty, false); } diff --git a/tests/ui/dlist.rs b/tests/ui/dlist.rs index 2940d2d29011..2c3b25cd45e8 100644 --- a/tests/ui/dlist.rs +++ b/tests/ui/dlist.rs @@ -5,6 +5,9 @@ extern crate alloc; use alloc::collections::linked_list::LinkedList; +const C: LinkedList = LinkedList::new(); +static S: LinkedList = LinkedList::new(); + trait Foo { type Baz = LinkedList; fn foo(_: LinkedList); diff --git a/tests/ui/dlist.stderr b/tests/ui/dlist.stderr index 234db33ba124..425407dc3349 100644 --- a/tests/ui/dlist.stderr +++ b/tests/ui/dlist.stderr @@ -1,14 +1,30 @@ error: you seem to be using a `LinkedList`! Perhaps you meant some other data structure? - --> $DIR/dlist.rs:9:16 + --> $DIR/dlist.rs:8:10 + | +LL | const C: LinkedList = LinkedList::new(); + | ^^^^^^^^^^^^^^^ + | + = note: `-D clippy::linkedlist` implied by `-D warnings` + = help: a `VecDeque` might work + +error: you seem to be using a `LinkedList`! Perhaps you meant some other data structure? + --> $DIR/dlist.rs:9:11 + | +LL | static S: LinkedList = LinkedList::new(); + | ^^^^^^^^^^^^^^^ + | + = help: a `VecDeque` might work + +error: you seem to be using a `LinkedList`! Perhaps you meant some other data structure? + --> $DIR/dlist.rs:12:16 | LL | type Baz = LinkedList; | ^^^^^^^^^^^^^^ | - = note: `-D clippy::linkedlist` implied by `-D warnings` = help: a `VecDeque` might work error: you seem to be using a `LinkedList`! Perhaps you meant some other data structure? - --> $DIR/dlist.rs:10:15 + --> $DIR/dlist.rs:13:15 | LL | fn foo(_: LinkedList); | ^^^^^^^^^^^^^^ @@ -16,7 +32,7 @@ LL | fn foo(_: LinkedList); = help: a `VecDeque` might work error: you seem to be using a `LinkedList`! Perhaps you meant some other data structure? - --> $DIR/dlist.rs:11:23 + --> $DIR/dlist.rs:14:23 | LL | const BAR: Option>; | ^^^^^^^^^^^^^^ @@ -24,7 +40,7 @@ LL | const BAR: Option>; = help: a `VecDeque` might work error: you seem to be using a `LinkedList`! Perhaps you meant some other data structure? - --> $DIR/dlist.rs:22:15 + --> $DIR/dlist.rs:25:15 | LL | fn foo(_: LinkedList) {} | ^^^^^^^^^^^^^^ @@ -32,7 +48,7 @@ LL | fn foo(_: LinkedList) {} = help: a `VecDeque` might work error: you seem to be using a `LinkedList`! Perhaps you meant some other data structure? - --> $DIR/dlist.rs:25:39 + --> $DIR/dlist.rs:28:39 | LL | pub fn test(my_favourite_linked_list: LinkedList) { | ^^^^^^^^^^^^^^ @@ -40,12 +56,12 @@ LL | pub fn test(my_favourite_linked_list: LinkedList) { = help: a `VecDeque` might work error: you seem to be using a `LinkedList`! Perhaps you meant some other data structure? - --> $DIR/dlist.rs:29:29 + --> $DIR/dlist.rs:32:29 | LL | pub fn test_ret() -> Option> { | ^^^^^^^^^^^^^^ | = help: a `VecDeque` might work -error: aborting due to 6 previous errors +error: aborting due to 8 previous errors diff --git a/tests/ui/option_option.rs b/tests/ui/option_option.rs index 6859ba8e5bb8..2faab9e035d9 100644 --- a/tests/ui/option_option.rs +++ b/tests/ui/option_option.rs @@ -1,6 +1,9 @@ #![deny(clippy::option_option)] #![allow(clippy::unnecessary_wraps)] +const C: Option> = None; +static S: Option> = None; + fn input(_: Option>) {} fn output() -> Option> { diff --git a/tests/ui/option_option.stderr b/tests/ui/option_option.stderr index ad7f081c7139..a925bb35b04d 100644 --- a/tests/ui/option_option.stderr +++ b/tests/ui/option_option.stderr @@ -1,8 +1,8 @@ error: consider using `Option` instead of `Option>` or a custom enum if you need to distinguish all 3 cases - --> $DIR/option_option.rs:4:13 + --> $DIR/option_option.rs:4:10 | -LL | fn input(_: Option>) {} - | ^^^^^^^^^^^^^^^^^^ +LL | const C: Option> = None; + | ^^^^^^^^^^^^^^^^^^^ | note: the lint level is defined here --> $DIR/option_option.rs:1:9 @@ -11,58 +11,70 @@ LL | #![deny(clippy::option_option)] | ^^^^^^^^^^^^^^^^^^^^^ error: consider using `Option` instead of `Option>` or a custom enum if you need to distinguish all 3 cases - --> $DIR/option_option.rs:6:16 + --> $DIR/option_option.rs:5:11 + | +LL | static S: Option> = None; + | ^^^^^^^^^^^^^^^^^^^ + +error: consider using `Option` instead of `Option>` or a custom enum if you need to distinguish all 3 cases + --> $DIR/option_option.rs:7:13 + | +LL | fn input(_: Option>) {} + | ^^^^^^^^^^^^^^^^^^ + +error: consider using `Option` instead of `Option>` or a custom enum if you need to distinguish all 3 cases + --> $DIR/option_option.rs:9:16 | LL | fn output() -> Option> { | ^^^^^^^^^^^^^^^^^^ error: consider using `Option` instead of `Option>` or a custom enum if you need to distinguish all 3 cases - --> $DIR/option_option.rs:10:27 + --> $DIR/option_option.rs:13:27 | LL | fn output_nested() -> Vec>> { | ^^^^^^^^^^^^^^^^^^ error: consider using `Option` instead of `Option>` or a custom enum if you need to distinguish all 3 cases - --> $DIR/option_option.rs:15:30 + --> $DIR/option_option.rs:18:30 | LL | fn output_nested_nested() -> Option>> { | ^^^^^^^^^^^^^^^^^^^^^^^^^^ error: consider using `Option` instead of `Option>` or a custom enum if you need to distinguish all 3 cases - --> $DIR/option_option.rs:20:8 + --> $DIR/option_option.rs:23:8 | LL | x: Option>, | ^^^^^^^^^^^^^^^^^^ error: consider using `Option` instead of `Option>` or a custom enum if you need to distinguish all 3 cases - --> $DIR/option_option.rs:24:23 + --> $DIR/option_option.rs:27:23 | LL | fn struct_fn() -> Option> { | ^^^^^^^^^^^^^^^^^^ error: consider using `Option` instead of `Option>` or a custom enum if you need to distinguish all 3 cases - --> $DIR/option_option.rs:30:22 + --> $DIR/option_option.rs:33:22 | LL | fn trait_fn() -> Option>; | ^^^^^^^^^^^^^^^^^^ error: consider using `Option` instead of `Option>` or a custom enum if you need to distinguish all 3 cases - --> $DIR/option_option.rs:34:11 + --> $DIR/option_option.rs:37:11 | LL | Tuple(Option>), | ^^^^^^^^^^^^^^^^^^ error: consider using `Option` instead of `Option>` or a custom enum if you need to distinguish all 3 cases - --> $DIR/option_option.rs:35:17 + --> $DIR/option_option.rs:38:17 | LL | Struct { x: Option> }, | ^^^^^^^^^^^^^^^^^^ error: consider using `Option` instead of `Option>` or a custom enum if you need to distinguish all 3 cases - --> $DIR/option_option.rs:76:14 + --> $DIR/option_option.rs:79:14 | LL | foo: Option>>, | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 10 previous errors +error: aborting due to 12 previous errors diff --git a/tests/ui/vec_box_sized.fixed b/tests/ui/vec_box_sized.fixed index 4fa28b525c3c..a40d91fdb18a 100644 --- a/tests/ui/vec_box_sized.fixed +++ b/tests/ui/vec_box_sized.fixed @@ -9,6 +9,8 @@ struct BigStruct([i32; 10000]); /// The following should trigger the lint mod should_trigger { use super::SizedStruct; + const C: Vec = Vec::new(); + static S: Vec = Vec::new(); struct StructWithVecBox { sized_type: Vec, diff --git a/tests/ui/vec_box_sized.rs b/tests/ui/vec_box_sized.rs index 7dc735cd90be..843bbb64e719 100644 --- a/tests/ui/vec_box_sized.rs +++ b/tests/ui/vec_box_sized.rs @@ -9,6 +9,8 @@ struct BigStruct([i32; 10000]); /// The following should trigger the lint mod should_trigger { use super::SizedStruct; + const C: Vec> = Vec::new(); + static S: Vec> = Vec::new(); struct StructWithVecBox { sized_type: Vec>, diff --git a/tests/ui/vec_box_sized.stderr b/tests/ui/vec_box_sized.stderr index 83435a40aa16..c518267f0418 100644 --- a/tests/ui/vec_box_sized.stderr +++ b/tests/ui/vec_box_sized.stderr @@ -1,28 +1,40 @@ error: `Vec` is already on the heap, the boxing is unnecessary - --> $DIR/vec_box_sized.rs:14:21 + --> $DIR/vec_box_sized.rs:12:14 | -LL | sized_type: Vec>, - | ^^^^^^^^^^^^^^^^^^^^^ help: try: `Vec` +LL | const C: Vec> = Vec::new(); + | ^^^^^^^^^^^^^ help: try: `Vec` | = note: `-D clippy::vec-box` implied by `-D warnings` error: `Vec` is already on the heap, the boxing is unnecessary - --> $DIR/vec_box_sized.rs:17:14 + --> $DIR/vec_box_sized.rs:13:15 + | +LL | static S: Vec> = Vec::new(); + | ^^^^^^^^^^^^^ help: try: `Vec` + +error: `Vec` is already on the heap, the boxing is unnecessary + --> $DIR/vec_box_sized.rs:16:21 + | +LL | sized_type: Vec>, + | ^^^^^^^^^^^^^^^^^^^^^ help: try: `Vec` + +error: `Vec` is already on the heap, the boxing is unnecessary + --> $DIR/vec_box_sized.rs:19:14 | LL | struct A(Vec>); | ^^^^^^^^^^^^^^^^^^^^^ help: try: `Vec` error: `Vec` is already on the heap, the boxing is unnecessary - --> $DIR/vec_box_sized.rs:18:18 + --> $DIR/vec_box_sized.rs:20:18 | LL | struct B(Vec>>); | ^^^^^^^^^^^^^^^ help: try: `Vec` error: `Vec` is already on the heap, the boxing is unnecessary - --> $DIR/vec_box_sized.rs:46:23 + --> $DIR/vec_box_sized.rs:48:23 | LL | pub fn f() -> Vec> { | ^^^^^^^^^^^ help: try: `Vec` -error: aborting due to 4 previous errors +error: aborting due to 6 previous errors From bd1201a2635d757244dbdab09026d53ad52da6a1 Mon Sep 17 00:00:00 2001 From: Yoshitomo Nakanishi Date: Fri, 19 Mar 2021 18:21:33 +0900 Subject: [PATCH 5/6] Fix inconsistent test names --- tests/ui/{dlist.rs => linkedlist.rs} | 0 tests/ui/{dlist.stderr => linkedlist.stderr} | 16 +++++----- .../{complex_types.rs => type_complexity.rs} | 0 ...ex_types.stderr => type_complexity.stderr} | 30 +++++++++---------- 4 files changed, 23 insertions(+), 23 deletions(-) rename tests/ui/{dlist.rs => linkedlist.rs} (100%) rename tests/ui/{dlist.stderr => linkedlist.stderr} (88%) rename tests/ui/{complex_types.rs => type_complexity.rs} (100%) rename tests/ui/{complex_types.stderr => type_complexity.stderr} (85%) diff --git a/tests/ui/dlist.rs b/tests/ui/linkedlist.rs similarity index 100% rename from tests/ui/dlist.rs rename to tests/ui/linkedlist.rs diff --git a/tests/ui/dlist.stderr b/tests/ui/linkedlist.stderr similarity index 88% rename from tests/ui/dlist.stderr rename to tests/ui/linkedlist.stderr index 425407dc3349..38ae71714d66 100644 --- a/tests/ui/dlist.stderr +++ b/tests/ui/linkedlist.stderr @@ -1,5 +1,5 @@ error: you seem to be using a `LinkedList`! Perhaps you meant some other data structure? - --> $DIR/dlist.rs:8:10 + --> $DIR/linkedlist.rs:8:10 | LL | const C: LinkedList = LinkedList::new(); | ^^^^^^^^^^^^^^^ @@ -8,7 +8,7 @@ LL | const C: LinkedList = LinkedList::new(); = help: a `VecDeque` might work error: you seem to be using a `LinkedList`! Perhaps you meant some other data structure? - --> $DIR/dlist.rs:9:11 + --> $DIR/linkedlist.rs:9:11 | LL | static S: LinkedList = LinkedList::new(); | ^^^^^^^^^^^^^^^ @@ -16,7 +16,7 @@ LL | static S: LinkedList = LinkedList::new(); = help: a `VecDeque` might work error: you seem to be using a `LinkedList`! Perhaps you meant some other data structure? - --> $DIR/dlist.rs:12:16 + --> $DIR/linkedlist.rs:12:16 | LL | type Baz = LinkedList; | ^^^^^^^^^^^^^^ @@ -24,7 +24,7 @@ LL | type Baz = LinkedList; = help: a `VecDeque` might work error: you seem to be using a `LinkedList`! Perhaps you meant some other data structure? - --> $DIR/dlist.rs:13:15 + --> $DIR/linkedlist.rs:13:15 | LL | fn foo(_: LinkedList); | ^^^^^^^^^^^^^^ @@ -32,7 +32,7 @@ LL | fn foo(_: LinkedList); = help: a `VecDeque` might work error: you seem to be using a `LinkedList`! Perhaps you meant some other data structure? - --> $DIR/dlist.rs:14:23 + --> $DIR/linkedlist.rs:14:23 | LL | const BAR: Option>; | ^^^^^^^^^^^^^^ @@ -40,7 +40,7 @@ LL | const BAR: Option>; = help: a `VecDeque` might work error: you seem to be using a `LinkedList`! Perhaps you meant some other data structure? - --> $DIR/dlist.rs:25:15 + --> $DIR/linkedlist.rs:25:15 | LL | fn foo(_: LinkedList) {} | ^^^^^^^^^^^^^^ @@ -48,7 +48,7 @@ LL | fn foo(_: LinkedList) {} = help: a `VecDeque` might work error: you seem to be using a `LinkedList`! Perhaps you meant some other data structure? - --> $DIR/dlist.rs:28:39 + --> $DIR/linkedlist.rs:28:39 | LL | pub fn test(my_favourite_linked_list: LinkedList) { | ^^^^^^^^^^^^^^ @@ -56,7 +56,7 @@ LL | pub fn test(my_favourite_linked_list: LinkedList) { = help: a `VecDeque` might work error: you seem to be using a `LinkedList`! Perhaps you meant some other data structure? - --> $DIR/dlist.rs:32:29 + --> $DIR/linkedlist.rs:32:29 | LL | pub fn test_ret() -> Option> { | ^^^^^^^^^^^^^^ diff --git a/tests/ui/complex_types.rs b/tests/ui/type_complexity.rs similarity index 100% rename from tests/ui/complex_types.rs rename to tests/ui/type_complexity.rs diff --git a/tests/ui/complex_types.stderr b/tests/ui/type_complexity.stderr similarity index 85% rename from tests/ui/complex_types.stderr rename to tests/ui/type_complexity.stderr index 7fcbb4bce883..7879233fdf28 100644 --- a/tests/ui/complex_types.stderr +++ b/tests/ui/type_complexity.stderr @@ -1,5 +1,5 @@ error: very complex type used. Consider factoring parts into `type` definitions - --> $DIR/complex_types.rs:7:12 + --> $DIR/type_complexity.rs:7:12 | LL | const CST: (u32, (u32, (u32, (u32, u32)))) = (0, (0, (0, (0, 0)))); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -7,85 +7,85 @@ LL | const CST: (u32, (u32, (u32, (u32, u32)))) = (0, (0, (0, (0, 0)))); = note: `-D clippy::type-complexity` implied by `-D warnings` error: very complex type used. Consider factoring parts into `type` definitions - --> $DIR/complex_types.rs:8:12 + --> $DIR/type_complexity.rs:8:12 | LL | static ST: (u32, (u32, (u32, (u32, u32)))) = (0, (0, (0, (0, 0)))); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: very complex type used. Consider factoring parts into `type` definitions - --> $DIR/complex_types.rs:11:8 + --> $DIR/type_complexity.rs:11:8 | LL | f: Vec>>, | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: very complex type used. Consider factoring parts into `type` definitions - --> $DIR/complex_types.rs:14:11 + --> $DIR/type_complexity.rs:14:11 | LL | struct Ts(Vec>>); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: very complex type used. Consider factoring parts into `type` definitions - --> $DIR/complex_types.rs:17:11 + --> $DIR/type_complexity.rs:17:11 | LL | Tuple(Vec>>), | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: very complex type used. Consider factoring parts into `type` definitions - --> $DIR/complex_types.rs:18:17 + --> $DIR/type_complexity.rs:18:17 | LL | Struct { f: Vec>> }, | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: very complex type used. Consider factoring parts into `type` definitions - --> $DIR/complex_types.rs:22:14 + --> $DIR/type_complexity.rs:22:14 | LL | const A: (u32, (u32, (u32, (u32, u32)))) = (0, (0, (0, (0, 0)))); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: very complex type used. Consider factoring parts into `type` definitions - --> $DIR/complex_types.rs:23:30 + --> $DIR/type_complexity.rs:23:30 | LL | fn impl_method(&self, p: Vec>>) {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: very complex type used. Consider factoring parts into `type` definitions - --> $DIR/complex_types.rs:27:14 + --> $DIR/type_complexity.rs:27:14 | LL | const A: Vec>>; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: very complex type used. Consider factoring parts into `type` definitions - --> $DIR/complex_types.rs:28:14 + --> $DIR/type_complexity.rs:28:14 | LL | type B = Vec>>; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: very complex type used. Consider factoring parts into `type` definitions - --> $DIR/complex_types.rs:29:25 + --> $DIR/type_complexity.rs:29:25 | LL | fn method(&self, p: Vec>>); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: very complex type used. Consider factoring parts into `type` definitions - --> $DIR/complex_types.rs:30:29 + --> $DIR/type_complexity.rs:30:29 | LL | fn def_method(&self, p: Vec>>) {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: very complex type used. Consider factoring parts into `type` definitions - --> $DIR/complex_types.rs:33:15 + --> $DIR/type_complexity.rs:33:15 | LL | fn test1() -> Vec>> { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: very complex type used. Consider factoring parts into `type` definitions - --> $DIR/complex_types.rs:37:14 + --> $DIR/type_complexity.rs:37:14 | LL | fn test2(_x: Vec>>) {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: very complex type used. Consider factoring parts into `type` definitions - --> $DIR/complex_types.rs:40:13 + --> $DIR/type_complexity.rs:40:13 | LL | let _y: Vec>> = vec![]; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ From 818f8320a34c60ec72c2cce0eb24a4a26c0d2b7a Mon Sep 17 00:00:00 2001 From: Yoshitomo Nakanishi Date: Sat, 20 Mar 2021 15:32:19 +0900 Subject: [PATCH 6/6] Merge type_complexity pass into types pass --- clippy_lints/src/lib.rs | 5 +- clippy_lints/src/types/mod.rs | 308 ++++++++-------------- clippy_lints/src/types/type_complexity.rs | 79 ++++++ 3 files changed, 192 insertions(+), 200 deletions(-) create mode 100644 clippy_lints/src/types/type_complexity.rs diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 05165ec9d66d..402fecd478cc 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -1031,7 +1031,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box await_holding_invalid::AwaitHolding); store.register_late_pass(|| box serde_api::SerdeApi); let vec_box_size_threshold = conf.vec_box_size_threshold; - store.register_late_pass(move || box types::Types::new(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)); store.register_late_pass(|| box booleans::NonminimalBool); store.register_late_pass(|| box eq_op::EqOp); store.register_late_pass(|| box enum_clike::UnportableVariant); @@ -1092,8 +1093,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box main_recursion::MainRecursion::default()); store.register_late_pass(|| box lifetimes::Lifetimes); store.register_late_pass(|| box entry::HashMapPass); - let type_complexity_threshold = conf.type_complexity_threshold; - store.register_late_pass(move || box types::TypeComplexity::new(type_complexity_threshold)); store.register_late_pass(|| box minmax::MinMaxPass); store.register_late_pass(|| box open_options::OpenOptions); store.register_late_pass(|| box zero_div_zero::ZeroDiv); diff --git a/clippy_lints/src/types/mod.rs b/clippy_lints/src/types/mod.rs index 1df964db38fe..12e1eba2ca66 100644 --- a/clippy_lints/src/types/mod.rs +++ b/clippy_lints/src/types/mod.rs @@ -4,21 +4,19 @@ mod linked_list; mod option_option; mod rc_buffer; mod redundant_allocation; +mod type_complexity; mod utils; mod vec_box; -use clippy_utils::diagnostics::span_lint; use rustc_hir as hir; -use rustc_hir::intravisit::{walk_ty, FnKind, NestedVisitorMap, Visitor}; +use rustc_hir::intravisit::FnKind; use rustc_hir::{ - Body, FnDecl, FnRetTy, FnSig, GenericArg, GenericParamKind, HirId, ImplItem, ImplItemKind, Item, ItemKind, Local, - MutTy, QPath, TraitFn, TraitItem, TraitItemKind, TyKind, + Body, FnDecl, FnRetTy, GenericArg, HirId, ImplItem, ImplItemKind, Item, ItemKind, Local, MutTy, QPath, TraitItem, + TraitItemKind, TyKind, }; use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::hir::map::Map; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::source_map::Span; -use rustc_target::spec::abi::Abi; declare_clippy_lint! { /// **What it does:** Checks for use of `Box>` anywhere in the code. @@ -231,63 +229,121 @@ declare_clippy_lint! { "shared ownership of a buffer type" } +declare_clippy_lint! { + /// **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 + /// using a `type` definition to simplify them. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// ```rust + /// # use std::rc::Rc; + /// struct Foo { + /// inner: Rc>>>, + /// } + /// ``` + pub TYPE_COMPLEXITY, + complexity, + "usage of very complex types that might be better factored into `type` definitions" +} + pub struct Types { vec_box_size_threshold: u64, + type_complexity_threshold: u64, } -impl_lint_pass!(Types => [BOX_VEC, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX, REDUNDANT_ALLOCATION, RC_BUFFER]); +impl_lint_pass!(Types => [BOX_VEC, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX, REDUNDANT_ALLOCATION, RC_BUFFER, TYPE_COMPLEXITY]); impl<'tcx> LateLintPass<'tcx> for Types { fn check_fn(&mut self, cx: &LateContext<'_>, _: FnKind<'_>, decl: &FnDecl<'_>, _: &Body<'_>, _: Span, id: HirId) { - // Skip trait implementations; see issue #605. - if let Some(hir::Node::Item(item)) = cx.tcx.hir().find(cx.tcx.hir().get_parent_item(id)) { - if let ItemKind::Impl(hir::Impl { of_trait: Some(_), .. }) = item.kind { - return; - } - } + let is_in_trait_impl = if let Some(hir::Node::Item(item)) = cx.tcx.hir().find(cx.tcx.hir().get_parent_item(id)) + { + matches!(item.kind, ItemKind::Impl(hir::Impl { of_trait: Some(_), .. })) + } else { + false + }; - self.check_fn_decl(cx, decl); + self.check_fn_decl( + cx, + decl, + CheckTyContext { + is_in_trait_impl, + ..CheckTyContext::default() + }, + ); } fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) { match item.kind { - ItemKind::Static(ref ty, _, _) | ItemKind::Const(ref ty, _) => self.check_ty(cx, ty, false), + ItemKind::Static(ref ty, _, _) | ItemKind::Const(ref ty, _) => { + self.check_ty(cx, ty, CheckTyContext::default()) + }, // functions, enums, structs, impls and traits are covered _ => (), } } + fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx ImplItem<'_>) { + match item.kind { + ImplItemKind::Const(ref ty, _) | ImplItemKind::TyAlias(ref ty) => self.check_ty( + cx, + ty, + CheckTyContext { + is_in_trait_impl: true, + ..CheckTyContext::default() + }, + ), + // methods are covered by check_fn + ImplItemKind::Fn(..) => (), + } + } + fn check_field_def(&mut self, cx: &LateContext<'_>, field: &hir::FieldDef<'_>) { - self.check_ty(cx, &field.ty, false); + self.check_ty(cx, &field.ty, CheckTyContext::default()); } fn check_trait_item(&mut self, cx: &LateContext<'_>, item: &TraitItem<'_>) { match item.kind { - TraitItemKind::Const(ref ty, _) | TraitItemKind::Type(_, Some(ref ty)) => self.check_ty(cx, ty, false), - TraitItemKind::Fn(ref sig, _) => self.check_fn_decl(cx, &sig.decl), + TraitItemKind::Const(ref ty, _) | TraitItemKind::Type(_, Some(ref ty)) => { + self.check_ty(cx, ty, CheckTyContext::default()) + }, + TraitItemKind::Fn(ref sig, _) => self.check_fn_decl(cx, &sig.decl, CheckTyContext::default()), TraitItemKind::Type(..) => (), } } fn check_local(&mut self, cx: &LateContext<'_>, local: &Local<'_>) { if let Some(ref ty) = local.ty { - self.check_ty(cx, ty, true); + self.check_ty( + cx, + ty, + CheckTyContext { + is_local: true, + ..CheckTyContext::default() + }, + ); } } } impl Types { - pub fn new(vec_box_size_threshold: u64) -> Self { - Self { vec_box_size_threshold } + pub fn new(vec_box_size_threshold: u64, type_complexity_threshold: u64) -> Self { + Self { + vec_box_size_threshold, + type_complexity_threshold, + } } - fn check_fn_decl(&mut self, cx: &LateContext<'_>, decl: &FnDecl<'_>) { + fn check_fn_decl(&mut self, cx: &LateContext<'_>, decl: &FnDecl<'_>, context: CheckTyContext) { for input in decl.inputs { - self.check_ty(cx, input, false); + self.check_ty(cx, input, context); } if let FnRetTy::Return(ref ty) = decl.output { - self.check_ty(cx, ty, false); + self.check_ty(cx, ty, context); } } @@ -295,12 +351,22 @@ impl Types { /// lint found. /// /// The parameter `is_local` distinguishes the context of the type. - fn check_ty(&mut self, cx: &LateContext<'_>, hir_ty: &hir::Ty<'_>, is_local: bool) { + fn check_ty(&mut self, cx: &LateContext<'_>, hir_ty: &hir::Ty<'_>, mut context: CheckTyContext) { if hir_ty.span.from_expansion() { return; } + + if !context.is_nested_call && type_complexity::check(cx, hir_ty, self.type_complexity_threshold) { + return; + } + + // Skip trait implementations; see issue #605. + if context.is_in_trait_impl { + return; + } + match hir_ty.kind { - TyKind::Path(ref qpath) if !is_local => { + TyKind::Path(ref qpath) if !context.is_local => { let hir_id = hir_ty.hir_id; let res = cx.qpath_res(qpath, hir_id); if let Some(def_id) = res.opt_def_id() { @@ -318,7 +384,8 @@ impl Types { } match *qpath { QPath::Resolved(Some(ref ty), ref p) => { - self.check_ty(cx, ty, is_local); + context.is_nested_call = true; + self.check_ty(cx, ty, context); for ty in p.segments.iter().flat_map(|seg| { seg.args .as_ref() @@ -328,10 +395,11 @@ impl Types { _ => None, }) }) { - self.check_ty(cx, ty, is_local); + self.check_ty(cx, ty, context); } }, QPath::Resolved(None, ref p) => { + context.is_nested_call = true; for ty in p.segments.iter().flat_map(|seg| { seg.args .as_ref() @@ -341,17 +409,18 @@ impl Types { _ => None, }) }) { - self.check_ty(cx, ty, is_local); + self.check_ty(cx, ty, context); } }, QPath::TypeRelative(ref ty, ref seg) => { - self.check_ty(cx, ty, is_local); + context.is_nested_call = true; + self.check_ty(cx, ty, context); 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); + self.check_ty(cx, ty, context); } } }, @@ -359,16 +428,19 @@ impl Types { } }, TyKind::Rptr(ref lt, ref mut_ty) => { + context.is_nested_call = true; if !borrowed_box::check(cx, hir_ty, lt, mut_ty) { - self.check_ty(cx, &mut_ty.ty, is_local); + self.check_ty(cx, &mut_ty.ty, context); } }, TyKind::Slice(ref ty) | TyKind::Array(ref ty, _) | TyKind::Ptr(MutTy { ref ty, .. }) => { - self.check_ty(cx, ty, is_local) + context.is_nested_call = true; + self.check_ty(cx, ty, context) }, TyKind::Tup(tys) => { + context.is_nested_call = true; for ty in tys { - self.check_ty(cx, ty, is_local); + self.check_ty(cx, ty, context); } }, _ => {}, @@ -376,167 +448,9 @@ impl Types { } } -declare_clippy_lint! { - /// **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 - /// using a `type` definition to simplify them. - /// - /// **Known problems:** None. - /// - /// **Example:** - /// ```rust - /// # use std::rc::Rc; - /// struct Foo { - /// inner: Rc>>>, - /// } - /// ``` - pub TYPE_COMPLEXITY, - complexity, - "usage of very complex types that might be better factored into `type` definitions" -} - -pub struct TypeComplexity { - threshold: u64, -} - -impl TypeComplexity { - #[must_use] - pub fn new(threshold: u64) -> Self { - Self { threshold } - } -} - -impl_lint_pass!(TypeComplexity => [TYPE_COMPLEXITY]); - -impl<'tcx> LateLintPass<'tcx> for TypeComplexity { - fn check_fn( - &mut self, - cx: &LateContext<'tcx>, - _: FnKind<'tcx>, - decl: &'tcx FnDecl<'_>, - _: &'tcx Body<'_>, - _: Span, - _: HirId, - ) { - self.check_fndecl(cx, decl); - } - - fn check_field_def(&mut self, cx: &LateContext<'tcx>, field: &'tcx hir::FieldDef<'_>) { - // enum variants are also struct fields now - self.check_type(cx, &field.ty); - } - - fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) { - match item.kind { - ItemKind::Static(ref ty, _, _) | ItemKind::Const(ref ty, _) => self.check_type(cx, ty), - // functions, enums, structs, impls and traits are covered - _ => (), - } - } - - fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx TraitItem<'_>) { - match item.kind { - TraitItemKind::Const(ref ty, _) | TraitItemKind::Type(_, Some(ref ty)) => self.check_type(cx, ty), - TraitItemKind::Fn(FnSig { ref decl, .. }, TraitFn::Required(_)) => self.check_fndecl(cx, decl), - // methods with default impl are covered by check_fn - TraitItemKind::Type(..) | TraitItemKind::Fn(_, TraitFn::Provided(_)) => (), - } - } - - fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx ImplItem<'_>) { - match item.kind { - ImplItemKind::Const(ref ty, _) | ImplItemKind::TyAlias(ref ty) => self.check_type(cx, ty), - // methods are covered by check_fn - ImplItemKind::Fn(..) => (), - } - } - - fn check_local(&mut self, cx: &LateContext<'tcx>, local: &'tcx Local<'_>) { - if let Some(ref ty) = local.ty { - self.check_type(cx, ty); - } - } -} - -impl<'tcx> TypeComplexity { - fn check_fndecl(&self, cx: &LateContext<'tcx>, decl: &'tcx FnDecl<'_>) { - for arg in decl.inputs { - self.check_type(cx, arg); - } - if let FnRetTy::Return(ref ty) = decl.output { - self.check_type(cx, ty); - } - } - - fn check_type(&self, cx: &LateContext<'_>, ty: &hir::Ty<'_>) { - if ty.span.from_expansion() { - return; - } - let score = { - let mut visitor = TypeComplexityVisitor { score: 0, nest: 1 }; - visitor.visit_ty(ty); - visitor.score - }; - - if score > self.threshold { - span_lint( - cx, - TYPE_COMPLEXITY, - ty.span, - "very complex type used. Consider factoring parts into `type` definitions", - ); - } - } -} - -/// Walks a type and assigns a complexity score to it. -struct TypeComplexityVisitor { - /// total complexity score of the type - score: u64, - /// current nesting level - nest: u64, -} - -impl<'tcx> Visitor<'tcx> for TypeComplexityVisitor { - type Map = Map<'tcx>; - - fn visit_ty(&mut self, ty: &'tcx hir::Ty<'_>) { - let (add_score, sub_nest) = match ty.kind { - // _, &x and *x have only small overhead; don't mess with nesting level - TyKind::Infer | TyKind::Ptr(..) | TyKind::Rptr(..) => (1, 0), - - // the "normal" components of a type: named types, arrays/tuples - TyKind::Path(..) | TyKind::Slice(..) | TyKind::Tup(..) | TyKind::Array(..) => (10 * self.nest, 1), - - // function types bring a lot of overhead - TyKind::BareFn(ref bare) if bare.abi == Abi::Rust => (50 * self.nest, 1), - - TyKind::TraitObject(ref param_bounds, _, _) => { - let has_lifetime_parameters = param_bounds.iter().any(|bound| { - bound - .bound_generic_params - .iter() - .any(|gen| matches!(gen.kind, GenericParamKind::Lifetime { .. })) - }); - if has_lifetime_parameters { - // complex trait bounds like A<'a, 'b> - (50 * self.nest, 1) - } else { - // simple trait bounds like A + B - (20 * self.nest, 0) - } - }, - - _ => (0, 0), - }; - self.score += add_score; - self.nest += sub_nest; - walk_ty(self, ty); - self.nest -= sub_nest; - } - fn nested_visit_map(&mut self) -> NestedVisitorMap { - NestedVisitorMap::None - } +#[derive(Clone, Copy, Default)] +struct CheckTyContext { + is_in_trait_impl: bool, + is_local: bool, + is_nested_call: bool, } diff --git a/clippy_lints/src/types/type_complexity.rs b/clippy_lints/src/types/type_complexity.rs new file mode 100644 index 000000000000..9a4e9da3e2be --- /dev/null +++ b/clippy_lints/src/types/type_complexity.rs @@ -0,0 +1,79 @@ +use clippy_utils::diagnostics::span_lint; +use rustc_hir as hir; +use rustc_hir::intravisit::{walk_ty, NestedVisitorMap, Visitor}; +use rustc_hir::{GenericParamKind, TyKind}; +use rustc_lint::LateContext; +use rustc_middle::hir::map::Map; +use rustc_target::spec::abi::Abi; + +use super::TYPE_COMPLEXITY; + +pub(super) fn check(cx: &LateContext<'_>, ty: &hir::Ty<'_>, type_complexity_threshold: u64) -> bool { + let score = { + let mut visitor = TypeComplexityVisitor { score: 0, nest: 1 }; + visitor.visit_ty(ty); + visitor.score + }; + + if score > type_complexity_threshold { + span_lint( + cx, + TYPE_COMPLEXITY, + ty.span, + "very complex type used. Consider factoring parts into `type` definitions", + ); + true + } else { + false + } +} + +/// Walks a type and assigns a complexity score to it. +struct TypeComplexityVisitor { + /// total complexity score of the type + score: u64, + /// current nesting level + nest: u64, +} + +impl<'tcx> Visitor<'tcx> for TypeComplexityVisitor { + type Map = Map<'tcx>; + + fn visit_ty(&mut self, ty: &'tcx hir::Ty<'_>) { + let (add_score, sub_nest) = match ty.kind { + // _, &x and *x have only small overhead; don't mess with nesting level + TyKind::Infer | TyKind::Ptr(..) | TyKind::Rptr(..) => (1, 0), + + // the "normal" components of a type: named types, arrays/tuples + TyKind::Path(..) | TyKind::Slice(..) | TyKind::Tup(..) | TyKind::Array(..) => (10 * self.nest, 1), + + // function types bring a lot of overhead + TyKind::BareFn(ref bare) if bare.abi == Abi::Rust => (50 * self.nest, 1), + + TyKind::TraitObject(ref param_bounds, _, _) => { + let has_lifetime_parameters = param_bounds.iter().any(|bound| { + bound + .bound_generic_params + .iter() + .any(|gen| matches!(gen.kind, GenericParamKind::Lifetime { .. })) + }); + if has_lifetime_parameters { + // complex trait bounds like A<'a, 'b> + (50 * self.nest, 1) + } else { + // simple trait bounds like A + B + (20 * self.nest, 0) + } + }, + + _ => (0, 0), + }; + self.score += add_score; + self.nest += sub_nest; + walk_ty(self, ty); + self.nest -= sub_nest; + } + fn nested_visit_map(&mut self) -> NestedVisitorMap { + NestedVisitorMap::None + } +}