diff options
Diffstat (limited to 'clippy_lints')
56 files changed, 1560 insertions, 974 deletions
diff --git a/clippy_lints/src/asm_syntax.rs b/clippy_lints/src/asm_syntax.rs index feb6437ee26..c2fa56e1360 100644 --- a/clippy_lints/src/asm_syntax.rs +++ b/clippy_lints/src/asm_syntax.rs @@ -2,8 +2,11 @@ use std::fmt; use clippy_utils::diagnostics::span_lint_and_help; use rustc_ast::ast::{Expr, ExprKind, InlineAsmOptions}; -use rustc_lint::{EarlyContext, EarlyLintPass, Lint}; +use rustc_ast::{InlineAsm, Item, ItemKind}; +use rustc_lint::{EarlyContext, EarlyLintPass, Lint, LintContext}; use rustc_session::declare_lint_pass; +use rustc_span::Span; +use rustc_target::asm::InlineAsmArch; #[derive(Clone, Copy, PartialEq, Eq)] enum AsmStyle { @@ -31,8 +34,14 @@ impl std::ops::Not for AsmStyle { } } -fn check_expr_asm_syntax(lint: &'static Lint, cx: &EarlyContext<'_>, expr: &Expr, check_for: AsmStyle) { - if let ExprKind::InlineAsm(ref inline_asm) = expr.kind { +fn check_asm_syntax( + lint: &'static Lint, + cx: &EarlyContext<'_>, + inline_asm: &InlineAsm, + span: Span, + check_for: AsmStyle, +) { + if matches!(cx.sess().asm_arch, Some(InlineAsmArch::X86 | InlineAsmArch::X86_64)) { let style = if inline_asm.options.contains(InlineAsmOptions::ATT_SYNTAX) { AsmStyle::Att } else { @@ -43,7 +52,7 @@ fn check_expr_asm_syntax(lint: &'static Lint, cx: &EarlyContext<'_>, expr: &Expr span_lint_and_help( cx, lint, - expr.span, + span, &format!("{style} x86 assembly syntax used"), None, &format!("use {} x86 assembly syntax", !style), @@ -89,7 +98,15 @@ declare_lint_pass!(InlineAsmX86IntelSyntax => [INLINE_ASM_X86_INTEL_SYNTAX]); impl EarlyLintPass for InlineAsmX86IntelSyntax { fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) { - check_expr_asm_syntax(Self::get_lints()[0], cx, expr, AsmStyle::Intel); + if let ExprKind::InlineAsm(inline_asm) = &expr.kind { + check_asm_syntax(Self::get_lints()[0], cx, inline_asm, expr.span, AsmStyle::Intel); + } + } + + fn check_item(&mut self, cx: &EarlyContext<'_>, item: &Item) { + if let ItemKind::GlobalAsm(inline_asm) = &item.kind { + check_asm_syntax(Self::get_lints()[0], cx, inline_asm, item.span, AsmStyle::Intel); + } } } @@ -130,6 +147,14 @@ declare_lint_pass!(InlineAsmX86AttSyntax => [INLINE_ASM_X86_ATT_SYNTAX]); impl EarlyLintPass for InlineAsmX86AttSyntax { fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) { - check_expr_asm_syntax(Self::get_lints()[0], cx, expr, AsmStyle::Att); + if let ExprKind::InlineAsm(inline_asm) = &expr.kind { + check_asm_syntax(Self::get_lints()[0], cx, inline_asm, expr.span, AsmStyle::Att); + } + } + + fn check_item(&mut self, cx: &EarlyContext<'_>, item: &Item) { + if let ItemKind::GlobalAsm(inline_asm) = &item.kind { + check_asm_syntax(Self::get_lints()[0], cx, inline_asm, item.span, AsmStyle::Att); + } } } diff --git a/clippy_lints/src/attrs.rs b/clippy_lints/src/attrs.rs index da38422874b..f90a7dbbfe5 100644 --- a/clippy_lints/src/attrs.rs +++ b/clippy_lints/src/attrs.rs @@ -1,7 +1,9 @@ //! checks for attributes use clippy_config::msrvs::{self, Msrv}; -use clippy_utils::diagnostics::{span_lint, span_lint_and_help, span_lint_and_sugg, span_lint_and_then}; +use clippy_utils::diagnostics::{ + span_lint, span_lint_and_help, span_lint_and_note, span_lint_and_sugg, span_lint_and_then, +}; use clippy_utils::is_from_proc_macro; use clippy_utils::macros::{is_panic, macro_backtrace}; use clippy_utils::source::{first_line_of_span, is_present_in_source, snippet_opt, without_block_comments}; @@ -433,6 +435,56 @@ declare_clippy_lint! { "prevent from misusing the wrong attr name" } +declare_clippy_lint! { + /// ### What it does + /// Checks for `#[cfg_attr(feature = "cargo-clippy", ...)]` and for + /// `#[cfg(feature = "cargo-clippy")]` and suggests to replace it with + /// `#[cfg_attr(clippy, ...)]` or `#[cfg(clippy)]`. + /// + /// ### Why is this bad? + /// This feature has been deprecated for years and shouldn't be used anymore. + /// + /// ### Example + /// ```no_run + /// #[cfg(feature = "cargo-clippy")] + /// struct Bar; + /// ``` + /// + /// Use instead: + /// ```no_run + /// #[cfg(clippy)] + /// struct Bar; + /// ``` + #[clippy::version = "1.78.0"] + pub DEPRECATED_CLIPPY_CFG_ATTR, + suspicious, + "usage of `cfg(feature = \"cargo-clippy\")` instead of `cfg(clippy)`" +} + +declare_clippy_lint! { + /// ### What it does + /// Checks for `#[cfg_attr(clippy, allow(clippy::lint))]` + /// and suggests to replace it with `#[allow(clippy::lint)]`. + /// + /// ### Why is this bad? + /// There is no reason to put clippy attributes behind a clippy `cfg` as they are not + /// run by anything else than clippy. + /// + /// ### Example + /// ```no_run + /// #![cfg_attr(clippy, allow(clippy::deprecated_cfg_attr))] + /// ``` + /// + /// Use instead: + /// ```no_run + /// #![allow(clippy::deprecated_cfg_attr)] + /// ``` + #[clippy::version = "1.78.0"] + pub UNNECESSARY_CLIPPY_CFG, + suspicious, + "usage of `cfg_attr(clippy, allow(clippy::lint))` instead of `allow(clippy::lint)`" +} + declare_lint_pass!(Attributes => [ ALLOW_ATTRIBUTES_WITHOUT_REASON, INLINE_ALWAYS, @@ -794,6 +846,8 @@ impl_lint_pass!(EarlyAttributes => [ EMPTY_LINE_AFTER_DOC_COMMENTS, NON_MINIMAL_CFG, MAYBE_MISUSED_CFG, + DEPRECATED_CLIPPY_CFG_ATTR, + UNNECESSARY_CLIPPY_CFG, ]); impl EarlyLintPass for EarlyAttributes { @@ -803,6 +857,7 @@ impl EarlyLintPass for EarlyAttributes { fn check_attribute(&mut self, cx: &EarlyContext<'_>, attr: &Attribute) { check_deprecated_cfg_attr(cx, attr, &self.msrv); + check_deprecated_cfg(cx, attr); check_mismatched_target_os(cx, attr); check_minimal_cfg_condition(cx, attr); check_misused_cfg(cx, attr); @@ -857,39 +912,148 @@ fn check_empty_line_after_outer_attr(cx: &EarlyContext<'_>, item: &rustc_ast::It } } +fn check_cargo_clippy_attr(cx: &EarlyContext<'_>, item: &rustc_ast::MetaItem) { + if item.has_name(sym::feature) && item.value_str().is_some_and(|v| v.as_str() == "cargo-clippy") { + span_lint_and_sugg( + cx, + DEPRECATED_CLIPPY_CFG_ATTR, + item.span, + "`feature = \"cargo-clippy\"` was replaced by `clippy`", + "replace with", + "clippy".to_string(), + Applicability::MachineApplicable, + ); + } +} + +fn check_deprecated_cfg_recursively(cx: &EarlyContext<'_>, attr: &rustc_ast::MetaItem) { + if let Some(ident) = attr.ident() { + if ["any", "all", "not"].contains(&ident.name.as_str()) { + let Some(list) = attr.meta_item_list() else { return }; + for item in list.iter().filter_map(|item| item.meta_item()) { + check_deprecated_cfg_recursively(cx, item); + } + } else { + check_cargo_clippy_attr(cx, attr); + } + } +} + +fn check_deprecated_cfg(cx: &EarlyContext<'_>, attr: &Attribute) { + if attr.has_name(sym::cfg) + && let Some(list) = attr.meta_item_list() + { + for item in list.iter().filter_map(|item| item.meta_item()) { + check_deprecated_cfg_recursively(cx, item); + } + } +} + fn check_deprecated_cfg_attr(cx: &EarlyContext<'_>, attr: &Attribute, msrv: &Msrv) { - if msrv.meets(msrvs::TOOL_ATTRIBUTES) - // check cfg_attr - && attr.has_name(sym::cfg_attr) + // check cfg_attr + if attr.has_name(sym::cfg_attr) && let Some(items) = attr.meta_item_list() && items.len() == 2 - // check for `rustfmt` && let Some(feature_item) = items[0].meta_item() - && feature_item.has_name(sym::rustfmt) - // check for `rustfmt_skip` and `rustfmt::skip` - && let Some(skip_item) = &items[1].meta_item() - && (skip_item.has_name(sym!(rustfmt_skip)) - || skip_item - .path - .segments - .last() - .expect("empty path in attribute") - .ident - .name - == sym::skip) - // Only lint outer attributes, because custom inner attributes are unstable - // Tracking issue: https://github.com/rust-lang/rust/issues/54726 - && attr.style == AttrStyle::Outer { - span_lint_and_sugg( - cx, - DEPRECATED_CFG_ATTR, - attr.span, - "`cfg_attr` is deprecated for rustfmt and got replaced by tool attributes", - "use", - "#[rustfmt::skip]".to_string(), - Applicability::MachineApplicable, - ); + // check for `rustfmt` + if feature_item.has_name(sym::rustfmt) + && msrv.meets(msrvs::TOOL_ATTRIBUTES) + // check for `rustfmt_skip` and `rustfmt::skip` + && let Some(skip_item) = &items[1].meta_item() + && (skip_item.has_name(sym!(rustfmt_skip)) + || skip_item + .path + .segments + .last() + .expect("empty path in attribute") + .ident + .name + == sym::skip) + // Only lint outer attributes, because custom inner attributes are unstable + // Tracking issue: https://github.com/rust-lang/rust/issues/54726 + && attr.style == AttrStyle::Outer + { + span_lint_and_sugg( + cx, + DEPRECATED_CFG_ATTR, + attr.span, + "`cfg_attr` is deprecated for rustfmt and got replaced by tool attributes", + "use", + "#[rustfmt::skip]".to_string(), + Applicability::MachineApplicable, + ); + } else { + check_deprecated_cfg_recursively(cx, feature_item); + if let Some(behind_cfg_attr) = items[1].meta_item() { + check_clippy_cfg_attr(cx, feature_item, behind_cfg_attr, attr); + } + } + } +} + +fn check_clippy_cfg_attr( + cx: &EarlyContext<'_>, + cfg_attr: &rustc_ast::MetaItem, + behind_cfg_attr: &rustc_ast::MetaItem, + attr: &Attribute, +) { + if cfg_attr.has_name(sym::clippy) + && let Some(ident) = behind_cfg_attr.ident() + // FIXME: replace with `from_symbol` once https://github.com/rust-lang/rust/pull/121230 + // is merged. + && Level::from_str(ident.name.as_str()).is_some() + && let Some(items) = behind_cfg_attr.meta_item_list() + { + let nb_items = items.len(); + let mut clippy_lints = Vec::with_capacity(items.len()); + for item in items { + if let Some(meta_item) = item.meta_item() + && let [part1, _] = meta_item.path.segments.as_slice() + && part1.ident.name == sym::clippy + { + clippy_lints.push(item.span()); + } + } + if clippy_lints.is_empty() { + return; + } + if nb_items == clippy_lints.len() { + if let Some(snippet) = snippet_opt(cx, behind_cfg_attr.span) { + span_lint_and_sugg( + cx, + UNNECESSARY_CLIPPY_CFG, + attr.span, + "no need to put clippy lints behind a `clippy` cfg", + "replace with", + format!( + "#{}[{}]", + if attr.style == AttrStyle::Inner { "!" } else { "" }, + snippet + ), + Applicability::MachineApplicable, + ); + } + } else { + let snippet = clippy_lints + .iter() + .filter_map(|sp| snippet_opt(cx, *sp)) + .collect::<Vec<_>>() + .join(","); + span_lint_and_note( + cx, + UNNECESSARY_CLIPPY_CFG, + clippy_lints, + "no need to put clippy lints behind a `clippy` cfg", + None, + &format!( + "write instead: `#{}[{}({})]`", + if attr.style == AttrStyle::Inner { "!" } else { "" }, + ident.name, + snippet + ), + ); + } } } diff --git a/clippy_lints/src/blocks_in_conditions.rs b/clippy_lints/src/blocks_in_conditions.rs index ff4dffd0607..2eb0dac9742 100644 --- a/clippy_lints/src/blocks_in_conditions.rs +++ b/clippy_lints/src/blocks_in_conditions.rs @@ -2,7 +2,7 @@ use clippy_utils::diagnostics::{span_lint, span_lint_and_sugg}; use clippy_utils::source::snippet_block_with_applicability; use clippy_utils::ty::implements_trait; use clippy_utils::visitors::{for_each_expr, Descend}; -use clippy_utils::{get_parent_expr, higher}; +use clippy_utils::{get_parent_expr, higher, is_from_proc_macro}; use core::ops::ControlFlow; use rustc_errors::Applicability; use rustc_hir::{BlockCheckMode, Expr, ExprKind, MatchSource}; @@ -13,7 +13,7 @@ use rustc_span::sym; declare_clippy_lint! { /// ### What it does - /// Checks for `if` conditions that use blocks containing an + /// Checks for `if` and `match` conditions that use blocks containing an /// expression, statements or conditions that use closures with blocks. /// /// ### Why is this bad? @@ -25,6 +25,11 @@ declare_clippy_lint! { /// if { true } { /* ... */ } /// /// if { let x = somefunc(); x } { /* ... */ } + /// + /// match { let e = somefunc(); e } { + /// // ... + /// # _ => {} + /// } /// ``` /// /// Use instead: @@ -34,6 +39,12 @@ declare_clippy_lint! { /// /// let res = { let x = somefunc(); x }; /// if res { /* ... */ } + /// + /// let res = { let e = somefunc(); e }; + /// match res { + /// // ... + /// # _ => {} + /// } /// ``` #[clippy::version = "1.45.0"] pub BLOCKS_IN_CONDITIONS, @@ -94,7 +105,7 @@ impl<'tcx> LateLintPass<'tcx> for BlocksInConditions { } } else { let span = block.expr.as_ref().map_or_else(|| block.stmts[0].span, |e| e.span); - if span.from_expansion() || expr.span.from_expansion() { + if span.from_expansion() || expr.span.from_expansion() || is_from_proc_macro(cx, cond) { return; } // move block higher diff --git a/clippy_lints/src/booleans.rs b/clippy_lints/src/booleans.rs index 2d1c250ace9..0d66f2d644d 100644 --- a/clippy_lints/src/booleans.rs +++ b/clippy_lints/src/booleans.rs @@ -85,7 +85,117 @@ impl<'tcx> LateLintPass<'tcx> for NonminimalBool { ) { NonminimalBoolVisitor { cx }.visit_body(body); } + + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + match expr.kind { + ExprKind::Unary(UnOp::Not, sub) => check_inverted_condition(cx, expr.span, sub), + // This check the case where an element in a boolean comparison is inverted, like: + // + // ``` + // let a = true; + // !a == false; + // ``` + ExprKind::Binary(op, left, right) if matches!(op.node, BinOpKind::Eq | BinOpKind::Ne) => { + check_inverted_bool_in_condition(cx, expr.span, op.node, left, right); + }, + _ => {}, + } + } +} + +fn inverted_bin_op_eq_str(op: BinOpKind) -> Option<&'static str> { + match op { + BinOpKind::Eq => Some("!="), + BinOpKind::Ne => Some("=="), + _ => None, + } +} + +fn bin_op_eq_str(op: BinOpKind) -> Option<&'static str> { + match op { + BinOpKind::Eq => Some("=="), + BinOpKind::Ne => Some("!="), + _ => None, + } +} + +fn check_inverted_condition(cx: &LateContext<'_>, expr_span: Span, sub_expr: &Expr<'_>) { + if !expr_span.from_expansion() + && let ExprKind::Binary(op, left, right) = sub_expr.kind + && let Some(left) = snippet_opt(cx, left.span) + && let Some(right) = snippet_opt(cx, right.span) + { + let Some(op) = inverted_bin_op_eq_str(op.node) else { + return; + }; + span_lint_and_sugg( + cx, + NONMINIMAL_BOOL, + expr_span, + "this boolean expression can be simplified", + "try", + format!("{left} {op} {right}",), + Applicability::MachineApplicable, + ); + } +} + +fn check_inverted_bool_in_condition( + cx: &LateContext<'_>, + expr_span: Span, + op: BinOpKind, + left: &Expr<'_>, + right: &Expr<'_>, +) { + if expr_span.from_expansion() + && (!cx.typeck_results().node_types()[left.hir_id].is_bool() + || !cx.typeck_results().node_types()[right.hir_id].is_bool()) + { + return; + } + + let suggestion = match (left.kind, right.kind) { + (ExprKind::Unary(UnOp::Not, left_sub), ExprKind::Unary(UnOp::Not, right_sub)) => { + let Some(left) = snippet_opt(cx, left_sub.span) else { + return; + }; + let Some(right) = snippet_opt(cx, right_sub.span) else { + return; + }; + let Some(op) = bin_op_eq_str(op) else { return }; + format!("{left} {op} {right}") + }, + (ExprKind::Unary(UnOp::Not, left_sub), _) => { + let Some(left) = snippet_opt(cx, left_sub.span) else { + return; + }; + let Some(right) = snippet_opt(cx, right.span) else { + return; + }; + let Some(op) = inverted_bin_op_eq_str(op) else { return }; + format!("{left} {op} {right}") + }, + (_, ExprKind::Unary(UnOp::Not, right_sub)) => { + let Some(left) = snippet_opt(cx, left.span) else { return }; + let Some(right) = snippet_opt(cx, right_sub.span) else { + return; + }; + let Some(op) = inverted_bin_op_eq_str(op) else { return }; + format!("{left} {op} {right}") + }, + _ => return, + }; + span_lint_and_sugg( + cx, + NONMINIMAL_BOOL, + expr_span, + "this boolean expression can be simplified", + "try", + suggestion, + Applicability::MachineApplicable, + ); } + struct NonminimalBoolVisitor<'a, 'tcx> { cx: &'a LateContext<'tcx>, } diff --git a/clippy_lints/src/box_default.rs b/clippy_lints/src/box_default.rs index ef12fe344e4..d71cca6aa4f 100644 --- a/clippy_lints/src/box_default.rs +++ b/clippy_lints/src/box_default.rs @@ -101,25 +101,22 @@ impl<'tcx> Visitor<'tcx> for InferVisitor { fn given_type(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { match get_parent_node(cx.tcx, expr.hir_id) { - Some(Node::Local(Local { ty: Some(ty), .. })) => { + Node::Local(Local { ty: Some(ty), .. }) => { let mut v = InferVisitor::default(); v.visit_ty(ty); !v.0 }, - Some( - Node::Expr(Expr { + Node::Expr(Expr { + kind: ExprKind::Call(path, args), + .. + }) + | Node::Block(Block { + expr: Some(Expr { kind: ExprKind::Call(path, args), .. - }) - | Node::Block(Block { - expr: - Some(Expr { - kind: ExprKind::Call(path, args), - .. - }), - .. }), - ) => { + .. + }) => { if let Some(index) = args.iter().position(|arg| arg.hir_id == expr.hir_id) && let Some(sig) = expr_sig(cx, path) && let Some(input) = sig.input(index) diff --git a/clippy_lints/src/casts/cast_possible_truncation.rs b/clippy_lints/src/casts/cast_possible_truncation.rs index 1543ae80399..ab89bb2f5f1 100644 --- a/clippy_lints/src/casts/cast_possible_truncation.rs +++ b/clippy_lints/src/casts/cast_possible_truncation.rs @@ -131,8 +131,7 @@ pub(super) fn check( let cast_from_ptr_size = def.repr().int.map_or(true, |ty| matches!(ty, IntegerType::Pointer(_),)); let suffix = match (cast_from_ptr_size, is_isize_or_usize(cast_to)) { - (false, false) if from_nbits > to_nbits => "", - (true, false) if from_nbits > to_nbits => "", + (_, false) if from_nbits > to_nbits => "", (false, true) if from_nbits > 64 => "", (false, true) if from_nbits > 32 => " on targets with 32-bit wide pointers", _ => return, diff --git a/clippy_lints/src/casts/unnecessary_cast.rs b/clippy_lints/src/casts/unnecessary_cast.rs index b4a23d0d4db..3800d32eedc 100644 --- a/clippy_lints/src/casts/unnecessary_cast.rs +++ b/clippy_lints/src/casts/unnecessary_cast.rs @@ -264,8 +264,7 @@ fn is_cast_from_ty_alias<'tcx>(cx: &LateContext<'tcx>, expr: impl Visitable<'tcx } // Local usage } else if let Res::Local(hir_id) = res - && let Some(parent) = get_parent_node(cx.tcx, hir_id) - && let Node::Local(l) = parent + && let Node::Local(l) = get_parent_node(cx.tcx, hir_id) { if let Some(e) = l.init && is_cast_from_ty_alias(cx, e, cast_from) diff --git a/clippy_lints/src/collection_is_never_read.rs b/clippy_lints/src/collection_is_never_read.rs index d0c989cfff3..5ab52312843 100644 --- a/clippy_lints/src/collection_is_never_read.rs +++ b/clippy_lints/src/collection_is_never_read.rs @@ -94,7 +94,7 @@ fn has_no_read_access<'tcx>(cx: &LateContext<'tcx>, id: HirId, block: &'tcx Bloc // `id` appearing in the left-hand side of an assignment is not a read access: // // id = ...; // Not reading `id`. - if let Some(Node::Expr(parent)) = get_parent_node(cx.tcx, expr.hir_id) + if let Node::Expr(parent) = get_parent_node(cx.tcx, expr.hir_id) && let ExprKind::Assign(lhs, ..) = parent.kind && path_to_local_id(lhs, id) { @@ -108,7 +108,7 @@ fn has_no_read_access<'tcx>(cx: &LateContext<'tcx>, id: HirId, block: &'tcx Bloc // Only assuming this for "official" methods defined on the type. For methods defined in extension // traits (identified as local, based on the orphan rule), pessimistically assume that they might // have side effects, so consider them a read. - if let Some(Node::Expr(parent)) = get_parent_node(cx.tcx, expr.hir_id) + if let Node::Expr(parent) = get_parent_node(cx.tcx, expr.hir_id) && let ExprKind::MethodCall(_, receiver, _, _) = parent.kind && path_to_local_id(receiver, id) && let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(parent.hir_id) @@ -117,7 +117,7 @@ fn has_no_read_access<'tcx>(cx: &LateContext<'tcx>, id: HirId, block: &'tcx Bloc // The method call is a statement, so the return value is not used. That's not a read access: // // id.foo(args); - if let Some(Node::Stmt(..)) = get_parent_node(cx.tcx, parent.hir_id) { + if let Node::Stmt(..) = get_parent_node(cx.tcx, parent.hir_id) { return ControlFlow::Continue(()); } diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 0a5baabd973..76595e2e26e 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -51,6 +51,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::attrs::ALLOW_ATTRIBUTES_WITHOUT_REASON_INFO, crate::attrs::BLANKET_CLIPPY_RESTRICTION_LINTS_INFO, crate::attrs::DEPRECATED_CFG_ATTR_INFO, + crate::attrs::DEPRECATED_CLIPPY_CFG_ATTR_INFO, crate::attrs::DEPRECATED_SEMVER_INFO, crate::attrs::EMPTY_LINE_AFTER_DOC_COMMENTS_INFO, crate::attrs::EMPTY_LINE_AFTER_OUTER_ATTR_INFO, @@ -59,6 +60,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::attrs::MISMATCHED_TARGET_OS_INFO, crate::attrs::NON_MINIMAL_CFG_INFO, crate::attrs::SHOULD_PANIC_WITHOUT_EXPECT_INFO, + crate::attrs::UNNECESSARY_CLIPPY_CFG_INFO, crate::attrs::USELESS_ATTRIBUTE_INFO, crate::await_holding_invalid::AWAIT_HOLDING_INVALID_TYPE_INFO, crate::await_holding_invalid::AWAIT_HOLDING_LOCK_INFO, diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index 0ddfeaa0ae0..e6eb5088c2b 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -1009,7 +1009,7 @@ fn report<'tcx>( state.msg, |diag| { let (precedence, calls_field) = match get_parent_node(cx.tcx, data.first_expr.hir_id) { - Some(Node::Expr(e)) => match e.kind { + Node::Expr(e) => match e.kind { ExprKind::Call(callee, _) if callee.hir_id != data.first_expr.hir_id => (0, false), ExprKind::Call(..) => (PREC_POSTFIX, matches!(expr.kind, ExprKind::Field(..))), _ => (e.precedence().order(), false), diff --git a/clippy_lints/src/disallowed_macros.rs b/clippy_lints/src/disallowed_macros.rs index 656b3d9bfaf..75379cb4e54 100644 --- a/clippy_lints/src/disallowed_macros.rs +++ b/clippy_lints/src/disallowed_macros.rs @@ -1,13 +1,16 @@ use clippy_config::types::DisallowedPath; -use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::diagnostics::{span_lint_and_then, span_lint_hir_and_then}; use clippy_utils::macros::macro_backtrace; use rustc_ast::Attribute; use rustc_data_structures::fx::FxHashSet; +use rustc_errors::DiagnosticBuilder; use rustc_hir::def_id::DefIdMap; -use rustc_hir::{Expr, ExprKind, ForeignItem, HirId, ImplItem, Item, Pat, Path, Stmt, TraitItem, Ty}; +use rustc_hir::{ + Expr, ExprKind, ForeignItem, HirId, ImplItem, Item, ItemKind, OwnerId, Pat, Path, Stmt, TraitItem, Ty, +}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::impl_lint_pass; -use rustc_span::{ExpnId, Span}; +use rustc_span::{ExpnId, MacroKind, Span}; declare_clippy_lint! { /// ### What it does @@ -57,6 +60,10 @@ pub struct DisallowedMacros { conf_disallowed: Vec<DisallowedPath>, disallowed: DefIdMap<usize>, seen: FxHashSet<ExpnId>, + + // Track the most recently seen node that can have a `derive` attribute. + // Needed to use the correct lint level. + derive_src: Option<OwnerId>, } impl DisallowedMacros { @@ -65,10 +72,11 @@ impl DisallowedMacros { conf_disallowed, disallowed: DefIdMap::default(), seen: FxHashSet::default(), + derive_src: None, } } - fn check(&mut self, cx: &LateContext<'_>, span: Span) { + fn check(&mut self, cx: &LateContext<'_>, span: Span, derive_src: Option<OwnerId>) { if self.conf_disallowed.is_empty() { return; } @@ -80,18 +88,26 @@ impl DisallowedMacros { if let Some(&index) = self.disallowed.get(&mac.def_id) { let conf = &self.conf_disallowed[index]; - - span_lint_and_then( - cx, - DISALLOWED_MACROS, - mac.span, - &format!("use of a disallowed macro `{}`", conf.path()), - |diag| { - if let Some(reason) = conf.reason() { - diag.note(reason); - } - }, - ); + let msg = format!("use of a disallowed macro `{}`", conf.path()); + let add_note = |diag: &mut DiagnosticBuilder<'_, _>| { + if let Some(reason) = conf.reason() { + diag.note(reason); + } + }; + if matches!(mac.kind, MacroKind::Derive) + && let Some(derive_src) = derive_src + { + span_lint_hir_and_then( + cx, + DISALLOWED_MACROS, + cx.tcx.local_def_id_to_hir_id(derive_src.def_id), + mac.span, + &msg, + add_note, + ); + } else { + span_lint_and_then(cx, DISALLOWED_MACROS, mac.span, &msg, add_note); + } } } } @@ -110,49 +126,57 @@ impl LateLintPass<'_> for DisallowedMacros { } fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { - self.check(cx, expr.span); + self.check(cx, expr.span, None); // `$t + $t` can have the context of $t, check also the span of the binary operator if let ExprKind::Binary(op, ..) = expr.kind { - self.check(cx, op.span); + self.check(cx, op.span, None); } } fn check_stmt(&mut self, cx: &LateContext<'_>, stmt: &Stmt<'_>) { - self.check(cx, stmt.span); + self.check(cx, stmt.span, None); } fn check_ty(&mut self, cx: &LateContext<'_>, ty: &Ty<'_>) { - self.check(cx, ty.span); + self.check(cx, ty.span, None); } fn check_pat(&mut self, cx: &LateContext<'_>, pat: &Pat<'_>) { - self.check(cx, pat.span); + self.check(cx, pat.span, None); } fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { - self.check(cx, item.span); - self.check(cx, item.vis_span); + self.check(cx, item.span, self.derive_src); + self.check(cx, item.vis_span, None); + + if matches!( + item.kind, + ItemKind::Struct(..) | ItemKind::Enum(..) | ItemKind::Union(..) + ) && macro_backtrace(item.span).all(|m| !matches!(m.kind, MacroKind::Derive)) + { + self.derive_src = Some(item.owner_id); + } } fn check_foreign_item(&mut self, cx: &LateContext<'_>, item: &ForeignItem<'_>) { - self.check(cx, item.span); - self.check(cx, item.vis_span); + self.check(cx, item.span, None); + self.check(cx, item.vis_span, None); } fn check_impl_item(&mut self, cx: &LateContext<'_>, item: &ImplItem<'_>) { - self.check(cx, item.span); - self.check(cx, item.vis_span); + self.check(cx, item.span, None); + self.check(cx, item.vis_span, None); } fn check_trait_item(&mut self, cx: &LateContext<'_>, item: &TraitItem<'_>) { - self.check(cx, item.span); + self.check(cx, item.span, None); } fn check_path(&mut self, cx: &LateContext<'_>, path: &Path<'_>, _: HirId) { - self.check(cx, path.span); + self.check(cx, path.span, None); } fn check_attribute(&mut self, cx: &LateContext<'_>, attr: &Attribute) { - self.check(cx, attr.span); + self.check(cx, attr.span, self.derive_src); } } diff --git a/clippy_lints/src/drop_forget_ref.rs b/clippy_lints/src/drop_forget_ref.rs index 124d78fc4ff..da1dcba7d33 100644 --- a/clippy_lints/src/drop_forget_ref.rs +++ b/clippy_lints/src/drop_forget_ref.rs @@ -144,8 +144,7 @@ impl<'tcx> LateLintPass<'tcx> for DropForgetRef { // } fn is_single_call_in_arm<'tcx>(cx: &LateContext<'tcx>, arg: &'tcx Expr<'_>, drop_expr: &'tcx Expr<'_>) -> bool { if matches!(arg.kind, ExprKind::Call(..) | ExprKind::MethodCall(..)) { - let parent_node = get_parent_node(cx.tcx, drop_expr.hir_id); - if let Some(Node::Arm(Arm { body, .. })) = &parent_node { + if let Node::Arm(Arm { body, .. }) = get_parent_node(cx.tcx, drop_expr.hir_id) { return body.hir_id == drop_expr.hir_id; } } diff --git a/clippy_lints/src/format_args.rs b/clippy_lints/src/format_args.rs index 8af321e4d55..61f550ce0be 100644 --- a/clippy_lints/src/format_args.rs +++ b/clippy_lints/src/format_args.rs @@ -4,7 +4,7 @@ use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then}; use clippy_utils::is_diag_trait_item; use clippy_utils::macros::{ find_format_arg_expr, find_format_args, format_arg_removal_span, format_placeholder_format_span, is_assert_macro, - is_format_macro, is_panic, root_macro_call, root_macro_call_first_node, FormatParamUsage, + is_format_macro, is_panic, root_macro_call, root_macro_call_first_node, FormatParamUsage, MacroCall, }; use clippy_utils::source::snippet_opt; use clippy_utils::ty::{implements_trait, is_type_lang_item}; @@ -20,7 +20,6 @@ use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::ty::adjustment::{Adjust, Adjustment}; use rustc_middle::ty::Ty; use rustc_session::impl_lint_pass; -use rustc_span::def_id::DefId; use rustc_span::edition::Edition::Edition2021; use rustc_span::{sym, Span, Symbol}; @@ -189,32 +188,18 @@ impl<'tcx> LateLintPass<'tcx> for FormatArgs { && is_format_macro(cx, macro_call.def_id) && let Some(format_args) = find_format_args(cx, expr, macro_call.expn) { - for piece in &format_args.template { - if let FormatArgsPiece::Placeholder(placeholder) = piece - && let Ok(index) = placeholder.argument.index - && let Some(arg) = format_args.arguments.all_args().get(index) - { - let arg_expr = find_format_arg_expr(expr, arg); - - check_unused_format_specifier(cx, placeholder, arg_expr); - - if placeholder.format_trait != FormatTrait::Display - || placeholder.format_options != FormatOptions::default() - || is_aliased(&format_args, index) - { - continue; - } + let linter = FormatArgsExpr { + cx, + expr, + macro_call: ¯o_call, + format_args: &format_args, + ignore_mixed: self.ignore_mixed, + }; - if let Ok(arg_hir_expr) = arg_expr { - let name = cx.tcx.item_name(macro_call.def_id); - check_format_in_format_args(cx, macro_call.span, name, arg_hir_expr); - check_to_string_in_format_args(cx, name, arg_hir_expr); - } - } - } + linter.check_templates(); if self.msrv.meets(msrvs::FORMAT_ARGS_CAPTURE) { - check_uninlined_args(cx, &format_args, macro_call.span, macro_call.def_id, self.ignore_mixed); + linter.check_uninlined_args(); } } } @@ -222,255 +207,279 @@ impl<'tcx> LateLintPass<'tcx> for FormatArgs { extract_msrv_attr!(LateContext); } -fn check_unused_format_specifier( - cx: &LateContext<'_>, - placeholder: &FormatPlaceholder, - arg_expr: Result<&Expr<'_>, &rustc_ast::Expr>, -) { - let ty_or_ast_expr = arg_expr.map(|expr| cx.typeck_results().expr_ty(expr).peel_refs()); - - let is_format_args = match ty_or_ast_expr { - Ok(ty) => is_type_lang_item(cx, ty, LangItem::FormatArguments), - Err(expr) => matches!(expr.peel_parens_and_refs().kind, rustc_ast::ExprKind::FormatArgs(_)), - }; - - let options = &placeholder.format_options; - - let arg_span = match arg_expr { - Ok(expr) => expr.span, - Err(expr) => expr.span, - }; - - if let Some(placeholder_span) = placeholder.span - && is_format_args - && *options != FormatOptions::default() - { - span_lint_and_then( - cx, - UNUSED_FORMAT_SPECS, - placeholder_span, - "format specifiers have no effect on `format_args!()`", - |diag| { - let mut suggest_format = |spec| { - let message = format!("for the {spec} to apply consider using `format!()`"); - - if let Some(mac_call) = root_macro_call(arg_span) - && cx.tcx.is_diagnostic_item(sym::format_args_macro, mac_call.def_id) - { - diag.span_suggestion( - cx.sess().source_map().span_until_char(mac_call.span, '!'), - message, - "format", - Applicability::MaybeIncorrect, - ); - } else { - diag.help(message); - } - }; +struct FormatArgsExpr<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + expr: &'tcx Expr<'tcx>, + macro_call: &'a MacroCall, + format_args: &'a rustc_ast::FormatArgs, + ignore_mixed: bool, +} - if options.width.is_some() { - suggest_format("width"); +impl<'a, 'tcx> FormatArgsExpr<'a, 'tcx> { + fn check_templates(&self) { + for piece in &self.format_args.template { + if let FormatArgsPiece::Placeholder(placeholder) = piece + && let Ok(index) = placeholder.argument.index + && let Some(arg) = self.format_args.arguments.all_args().get(index) + { + let arg_expr = find_format_arg_expr(self.expr, arg); + + self.check_unused_format_specifier(placeholder, arg_expr); + + if let Ok(arg_expr) = arg_expr + && placeholder.format_trait == FormatTrait::Display + && placeholder.format_options == FormatOptions::default() + && !self.is_aliased(index) + { + let name = self.cx.tcx.item_name(self.macro_call.def_id); + self.check_format_in_format_args(name, arg_expr); + self.check_to_string_in_format_args(name, arg_expr); } + } + } + } - if options.precision.is_some() { - suggest_format("precision"); - } + fn check_unused_format_specifier( + &self, + placeholder: &FormatPlaceholder, + arg_expr: Result<&Expr<'_>, &rustc_ast::Expr>, + ) { + let ty_or_ast_expr = arg_expr.map(|expr| self.cx.typeck_results().expr_ty(expr).peel_refs()); - if let Some(format_span) = format_placeholder_format_span(placeholder) { - diag.span_suggestion_verbose( - format_span, - "if the current behavior is intentional, remove the format specifiers", - "", - Applicability::MaybeIncorrect, - ); - } - }, - ); - } -} + let is_format_args = match ty_or_ast_expr { + Ok(ty) => is_type_lang_item(self.cx, ty, LangItem::FormatArguments), + Err(expr) => matches!(expr.peel_parens_and_refs().kind, rustc_ast::ExprKind::FormatArgs(_)), + }; -fn check_uninlined_args( - cx: &LateContext<'_>, - args: &rustc_ast::FormatArgs, - call_site: Span, - def_id: DefId, - ignore_mixed: bool, -) { - if args.span.from_expansion() { - return; - } - if call_site.edition() < Edition2021 && (is_panic(cx, def_id) || is_assert_macro(cx, def_id)) { - // panic!, assert!, and debug_assert! before 2021 edition considers a single string argument as - // non-format - return; + let options = &placeholder.format_options; + + let arg_span = match arg_expr { + Ok(expr) => expr.span, + Err(expr) => expr.span, + }; + + if let Some(placeholder_span) = placeholder.span + && is_format_args + && *options != FormatOptions::default() + { + span_lint_and_then( + self.cx, + UNUSED_FORMAT_SPECS, + placeholder_span, + "format specifiers have no effect on `format_args!()`", + |diag| { + let mut suggest_format = |spec| { + let message = format!("for the {spec} to apply consider using `format!()`"); + + if let Some(mac_call) = root_macro_call(arg_span) + && self.cx.tcx.is_diagnostic_item(sym::format_args_macro, mac_call.def_id) + { + diag.span_suggestion( + self.cx.sess().source_map().span_until_char(mac_call.span, '!'), + message, + "format", + Applicability::MaybeIncorrect, + ); + } else { + diag.help(message); + } + }; + + if options.width.is_some() { + suggest_format("width"); + } + + if options.precision.is_some() { + suggest_format("precision"); + } + + if let Some(format_span) = format_placeholder_format_span(placeholder) { + diag.span_suggestion_verbose( + format_span, + "if the current behavior is intentional, remove the format specifiers", + "", + Applicability::MaybeIncorrect, + ); + } + }, + ); + } } - let mut fixes = Vec::new(); - // If any of the arguments are referenced by an index number, - // and that argument is not a simple variable and cannot be inlined, - // we cannot remove any other arguments in the format string, - // because the index numbers might be wrong after inlining. - // Example of an un-inlinable format: print!("{}{1}", foo, 2) - for (pos, usage) in format_arg_positions(args) { - if !check_one_arg(args, pos, usage, &mut fixes, ignore_mixed) { + fn check_uninlined_args(&self) { + if self.format_args.span.from_expansion() { + return; + } + if self.macro_call.span.edition() < Edition2021 + && (is_panic(self.cx, self.macro_call.def_id) || is_assert_macro(self.cx, self.macro_call.def_id)) + { + // panic!, assert!, and debug_assert! before 2021 edition considers a single string argument as + // non-format return; } - } - if fixes.is_empty() { - return; - } + let mut fixes = Vec::new(); + // If any of the arguments are referenced by an index number, + // and that argument is not a simple variable and cannot be inlined, + // we cannot remove any other arguments in the format string, + // because the index numbers might be wrong after inlining. + // Example of an un-inlinable format: print!("{}{1}", foo, 2) + for (pos, usage) in self.format_arg_positions() { + if !self.check_one_arg(pos, usage, &mut fixes) { + return; + } + } - // multiline span display suggestion is sometimes broken: https://github.com/rust-lang/rust/pull/102729#discussion_r988704308 - // in those cases, make the code suggestion hidden - let multiline_fix = fixes.iter().any(|(span, _)| cx.sess().source_map().is_multiline(*span)); - - // Suggest removing each argument only once, for example in `format!("{0} {0}", arg)`. - fixes.sort_unstable_by_key(|(span, _)| *span); - fixes.dedup_by_key(|(span, _)| *span); - - span_lint_and_then( - cx, - UNINLINED_FORMAT_ARGS, - call_site, - "variables can be used directly in the `format!` string", - |diag| { - diag.multipart_suggestion_with_style( - "change this to", - fixes, - Applicability::MachineApplicable, - if multiline_fix { CompletelyHidden } else { ShowCode }, - ); - }, - ); -} + if fixes.is_empty() { + return; + } -fn check_one_arg( - args: &rustc_ast::FormatArgs, - pos: &FormatArgPosition, - usage: FormatParamUsage, - fixes: &mut Vec<(Span, String)>, - ignore_mixed: bool, -) -> bool { - let index = pos.index.unwrap(); - let arg = &args.arguments.all_args()[index]; - - if !matches!(arg.kind, FormatArgumentKind::Captured(_)) - && let rustc_ast::ExprKind::Path(None, path) = &arg.expr.kind - && let [segment] = path.segments.as_slice() - && segment.args.is_none() - && let Some(arg_span) = format_arg_removal_span(args, index) - && let Some(pos_span) = pos.span - { - let replacement = match usage { - FormatParamUsage::Argument => segment.ident.name.to_string(), - FormatParamUsage::Width => format!("{}$", segment.ident.name), - FormatParamUsage::Precision => format!(".{}$", segment.ident.name), - }; - fixes.push((pos_span, replacement)); - fixes.push((arg_span, String::new())); - true // successful inlining, continue checking - } else { - // Do not continue inlining (return false) in case - // * if we can't inline a numbered argument, e.g. `print!("{0} ...", foo.bar, ...)` - // * if allow_mixed_uninlined_format_args is false and this arg hasn't been inlined already - pos.kind != FormatArgPositionKind::Number - && (!ignore_mixed || matches!(arg.kind, FormatArgumentKind::Captured(_))) - } -} + // multiline span display suggestion is sometimes broken: https://github.com/rust-lang/rust/pull/102729#discussion_r988704308 + // in those cases, make the code suggestion hidden + let multiline_fix = fixes + .iter() + .any(|(span, _)| self.cx.sess().source_map().is_multiline(*span)); -fn check_format_in_format_args(cx: &LateContext<'_>, call_site: Span, name: Symbol, arg: &Expr<'_>) { - let expn_data = arg.span.ctxt().outer_expn_data(); - if expn_data.call_site.from_expansion() { - return; - } - let Some(mac_id) = expn_data.macro_def_id else { return }; - if !cx.tcx.is_diagnostic_item(sym::format_macro, mac_id) { - return; + // Suggest removing each argument only once, for example in `format!("{0} {0}", arg)`. + fixes.sort_unstable_by_key(|(span, _)| *span); + fixes.dedup_by_key(|(span, _)| *span); + + span_lint_and_then( + self.cx, + UNINLINED_FORMAT_ARGS, + self.macro_call.span, + "variables can be used directly in the `format!` string", + |diag| { + diag.multipart_suggestion_with_style( + "change this to", + fixes, + Applicability::MachineApplicable, + if multiline_fix { CompletelyHidden } else { ShowCode }, + ); + }, + ); } - span_lint_and_then( - cx, - FORMAT_IN_FORMAT_ARGS, - call_site, - &format!("`format!` in `{name}!` args"), - |diag| { - diag.help(format!( - "combine the `format!(..)` arguments with the outer `{name}!(..)` call" - )); - diag.help("or consider changing `format!` to `format_args!`"); - }, - ); -} -fn check_to_string_in_format_args(cx: &LateContext<'_>, name: Symbol, value: &Expr<'_>) { - if !value.span.from_expansion() - && let ExprKind::MethodCall(_, receiver, [], to_string_span) = value.kind - && let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(value.hir_id) - && is_diag_trait_item(cx, method_def_id, sym::ToString) - && let receiver_ty = cx.typeck_results().expr_ty(receiver) - && let Some(display_trait_id) = cx.tcx.get_diagnostic_item(sym::Display) - && let (n_needed_derefs, target) = - count_needed_derefs(receiver_ty, cx.typeck_results().expr_adjustments(receiver).iter()) - && implements_trait(cx, target, display_trait_id, &[]) - && let Some(sized_trait_id) = cx.tcx.lang_items().sized_trait() - && let Some(receiver_snippet) = snippet_opt(cx, receiver.span) - { - let needs_ref = !implements_trait(cx, receiver_ty, sized_trait_id, &[]); - if n_needed_derefs == 0 && !needs_ref { - span_lint_and_sugg( - cx, - TO_STRING_IN_FORMAT_ARGS, - to_string_span.with_lo(receiver.span.hi()), - &format!("`to_string` applied to a type that implements `Display` in `{name}!` args"), - "remove this", - String::new(), - Applicability::MachineApplicable, - ); + fn check_one_arg(&self, pos: &FormatArgPosition, usage: FormatParamUsage, fixes: &mut Vec<(Span, String)>) -> bool { + let index = pos.index.unwrap(); + let arg = &self.format_args.arguments.all_args()[index]; + + if !matches!(arg.kind, FormatArgumentKind::Captured(_)) + && let rustc_ast::ExprKind::Path(None, path) = &arg.expr.kind + && let [segment] = path.segments.as_slice() + && segment.args.is_none() + && let Some(arg_span) = format_arg_removal_span(self.format_args, index) + && let Some(pos_span) = pos.span + { + let replacement = match usage { + FormatParamUsage::Argument => segment.ident.name.to_string(), + FormatParamUsage::Width => format!("{}$", segment.ident.name), + FormatParamUsage::Precision => format!(".{}$", segment.ident.name), + }; + fixes.push((pos_span, replacement)); + fixes.push((arg_span, String::new())); + true // successful inlining, continue checking } else { - span_lint_and_sugg( - cx, - TO_STRING_IN_FORMAT_ARGS, - value.span, - &format!("`to_string` applied to a type that implements `Display` in `{name}!` args"), - "use this", - format!( - "{}{:*>n_needed_derefs$}{receiver_snippet}", - if needs_ref { "&" } else { "" }, - "" - ), - Applicability::MachineApplicable, - ); + // Do not continue inlining (return false) in case + // * if we can't inline a numbered argument, e.g. `print!("{0} ...", foo.bar, ...)` + // * if allow_mixed_uninlined_format_args is false and this arg hasn't been inlined already + pos.kind != FormatArgPositionKind::Number + && (!self.ignore_mixed || matches!(arg.kind, FormatArgumentKind::Captured(_))) } } -} -fn format_arg_positions( - format_args: &rustc_ast::FormatArgs, -) -> impl Iterator<Item = (&FormatArgPosition, FormatParamUsage)> { - format_args.template.iter().flat_map(|piece| match piece { - FormatArgsPiece::Placeholder(placeholder) => { - let mut positions = ArrayVec::<_, 3>::new(); + fn check_format_in_format_args(&self, name: Symbol, arg: &Expr<'_>) { + let expn_data = arg.span.ctxt().outer_expn_data(); + if expn_data.call_site.from_expansion() { + return; + } + let Some(mac_id) = expn_data.macro_def_id else { return }; + if !self.cx.tcx.is_diagnostic_item(sym::format_macro, mac_id) { + return; + } + span_lint_and_then( + self.cx, + FORMAT_IN_FORMAT_ARGS, + self.macro_call.span, + &format!("`format!` in `{name}!` args"), + |diag| { + diag.help(format!( + "combine the `format!(..)` arguments with the outer `{name}!(..)` call" + )); + diag.help("or consider changing `format!` to `format_args!`"); + }, + ); + } - positions.push((&placeholder.argument, FormatParamUsage::Argument)); - if let Some(FormatCount::Argument(position)) = &placeholder.format_options.width { - positions.push((position, FormatParamUsage::Width)); - } - if let Some(FormatCount::Argument(position)) = &placeholder.format_options.precision { - positions.push((position, FormatParamUsage::Precision)); + fn check_to_string_in_format_args(&self, name: Symbol, value: &Expr<'_>) { + let cx = self.cx; + if !value.span.from_expansion() + && let ExprKind::MethodCall(_, receiver, [], to_string_span) = value.kind + && let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(value.hir_id) + && is_diag_trait_item(cx, method_def_id, sym::ToString) + && let receiver_ty = cx.typeck_results().expr_ty(receiver) + && let Some(display_trait_id) = cx.tcx.get_diagnostic_item(sym::Display) + && let (n_needed_derefs, target) = + count_needed_derefs(receiver_ty, cx.typeck_results().expr_adjustments(receiver).iter()) + && implements_trait(cx, target, display_trait_id, &[]) + && let Some(sized_trait_id) = cx.tcx.lang_items().sized_trait() + && let Some(receiver_snippet) = snippet_opt(cx, receiver.span) + { + let needs_ref = !implements_trait(cx, receiver_ty, sized_trait_id, &[]); + if n_needed_derefs == 0 && !needs_ref { + span_lint_and_sugg( + cx, + TO_STRING_IN_FORMAT_ARGS, + to_string_span.with_lo(receiver.span.hi()), + &format!("`to_string` applied to a type that implements `Display` in `{name}!` args"), + "remove this", + String::new(), + Applicability::MachineApplicable, + ); + } else { + span_lint_and_sugg( + cx, + TO_STRING_IN_FORMAT_ARGS, + value.span, + &format!("`to_string` applied to a type that implements `Display` in `{name}!` args"), + "use this", + format!( + "{}{:*>n_needed_derefs$}{receiver_snippet}", + if needs_ref { "&" } else { "" }, + "" + ), + Applicability::MachineApplicable, + ); } + } + } - positions - }, - FormatArgsPiece::Literal(_) => ArrayVec::new(), - }) -} + fn format_arg_positions(&self) -> impl Iterator<Item = (&FormatArgPosition, FormatParamUsage)> { + self.format_args.template.iter().flat_map(|piece| match piece { + FormatArgsPiece::Placeholder(placeholder) => { + let mut positions = ArrayVec::<_, 3>::new(); -/// Returns true if the format argument at `index` is referred to by multiple format params -fn is_aliased(format_args: &rustc_ast::FormatArgs, index: usize) -> bool { - format_arg_positions(format_args) - .filter(|(position, _)| position.index == Ok(index)) - .at_most_one() - .is_err() + positions.push((&placeholder.argument, FormatParamUsage::Argument)); + if let Some(FormatCount::Argument(position)) = &placeholder.format_options.width { + positions.push((position, FormatParamUsage::Width)); + } + if let Some(FormatCount::Argument(position)) = &placeholder.format_options.precision { + positions.push((position, FormatParamUsage::Precision)); + } + + positions + }, + FormatArgsPiece::Literal(_) => ArrayVec::new(), + }) + } + + /// Returns true if the format argument at `index` is referred to by multiple format params + fn is_aliased(&self, index: usize) -> bool { + self.format_arg_positions() + .filter(|(position, _)| position.index == Ok(index)) + .at_most_one() + .is_err() + } } fn count_needed_derefs<'tcx, I>(mut ty: Ty<'tcx>, mut iter: I) -> (usize, Ty<'tcx>) diff --git a/clippy_lints/src/format_impl.rs b/clippy_lints/src/format_impl.rs index 9360eb1fa91..93517076cda 100644 --- a/clippy_lints/src/format_impl.rs +++ b/clippy_lints/src/format_impl.rs @@ -7,7 +7,7 @@ use rustc_hir::{Expr, ExprKind, Impl, ImplItem, ImplItemKind, QPath}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::impl_lint_pass; use rustc_span::symbol::kw; -use rustc_span::{sym, Span, Symbol}; +use rustc_span::{sym, Symbol}; declare_clippy_lint! { /// ### What it does @@ -119,123 +119,132 @@ impl<'tcx> LateLintPass<'tcx> for FormatImpl { } fn check_impl_item_post(&mut self, cx: &LateContext<'_>, impl_item: &ImplItem<'_>) { - // Assume no nested Impl of Debug and Display within eachother + // Assume no nested Impl of Debug and Display within each other if is_format_trait_impl(cx, impl_item).is_some() { self.format_trait_impl = None; } } fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - let Some(format_trait_impl) = self.format_trait_impl else { - return; - }; - - if format_trait_impl.name == sym::Display { - check_to_string_in_display(cx, expr); + if let Some(format_trait_impl) = self.format_trait_impl { + let linter = FormatImplExpr { + cx, + expr, + format_trait_impl, + }; + linter.check_to_string_in_display(); + linter.check_self_in_format_args(); + linter.check_print_in_format_impl(); } - - check_self_in_format_args(cx, expr, format_trait_impl); - check_print_in_format_impl(cx, expr, format_trait_impl); } } -fn check_to_string_in_display(cx: &LateContext<'_>, expr: &Expr<'_>) { - if let ExprKind::MethodCall(path, self_arg, ..) = expr.kind - // Get the hir_id of the object we are calling the method on - // Is the method to_string() ? - && path.ident.name == sym::to_string - // Is the method a part of the ToString trait? (i.e. not to_string() implemented - // separately) - && let Some(expr_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) - && is_diag_trait_item(cx, expr_def_id, sym::ToString) - // Is the method is called on self - && let ExprKind::Path(QPath::Resolved(_, path)) = self_arg.kind - && let [segment] = path.segments - && segment.ident.name == kw::SelfLower - { - span_lint( - cx, - RECURSIVE_FORMAT_IMPL, - expr.span, - "using `self.to_string` in `fmt::Display` implementation will cause infinite recursion", - ); - } +struct FormatImplExpr<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + expr: &'tcx Expr<'tcx>, + format_trait_impl: FormatTraitNames, } -fn check_self_in_format_args<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, impl_trait: FormatTraitNames) { - // Check each arg in format calls - do we ever use Display on self (directly or via deref)? - if let Some(outer_macro) = root_macro_call_first_node(cx, expr) - && let macro_def_id = outer_macro.def_id - && is_format_macro(cx, macro_def_id) - && let Some(format_args) = find_format_args(cx, expr, outer_macro.expn) - { - for piece in &format_args.template { - if let FormatArgsPiece::Placeholder(placeholder) = piece - && let trait_name = match placeholder.format_trait { - FormatTrait::Display => sym::Display, - FormatTrait::Debug => sym::Debug, - FormatTrait::LowerExp => sym!(LowerExp), - FormatTrait::UpperExp => sym!(UpperExp), - FormatTrait::Octal => sym!(Octal), - FormatTrait::Pointer => sym::Pointer, - FormatTrait::Binary => sym!(Binary), - FormatTrait::LowerHex => sym!(LowerHex), - FormatTrait::UpperHex => sym!(UpperHex), +impl<'a, 'tcx> FormatImplExpr<'a, 'tcx> { + fn check_to_string_in_display(&self) { + if self.format_trait_impl.name == sym::Display + && let ExprKind::MethodCall(path, self_arg, ..) = self.expr.kind + // Get the hir_id of the object we are calling the method on + // Is the method to_string() ? + && path.ident.name == sym::to_string + // Is the method a part of the ToString trait? (i.e. not to_string() implemented + // separately) + && let Some(expr_def_id) = self.cx.typeck_results().type_dependent_def_id(self.expr.hir_id) + && is_diag_trait_item(self.cx, expr_def_id, sym::ToString) + // Is the method is called on self + && let ExprKind::Path(QPath::Resolved(_, path)) = self_arg.kind + && let [segment] = path.segments + && segment.ident.name == kw::SelfLower + { + span_lint( + self.cx, + RECURSIVE_FORMAT_IMPL, + self.expr.span, + "using `self.to_string` in `fmt::Display` implementation will cause infinite recursion", + ); + } + } + + fn check_self_in_format_args(&self) { + // Check each arg in format calls - do we ever use Display on self (directly or via deref)? + if let Some(outer_macro) = root_macro_call_first_node(self.cx, self.expr) + && let macro_def_id = outer_macro.def_id + && is_format_macro(self.cx, macro_def_id) + && let Some(format_args) = find_format_args(self.cx, self.expr, outer_macro.expn) + { + for piece in &format_args.template { + if let FormatArgsPiece::Placeholder(placeholder) = piece + && let trait_name = match placeholder.format_trait { + FormatTrait::Display => sym::Display, + FormatTrait::Debug => sym::Debug, + FormatTrait::LowerExp => sym!(LowerExp), + FormatTrait::UpperExp => sym!(UpperExp), + FormatTrait::Octal => sym!(Octal), + FormatTrait::Pointer => sym::Pointer, + FormatTrait::Binary => sym!(Binary), + FormatTrait::LowerHex => sym!(LowerHex), + FormatTrait::UpperHex => sym!(UpperHex), + } + && trait_name == self.format_trait_impl.name + && let Ok(index) = placeholder.argument.index + && let Some(arg) = format_args.arguments.all_args().get(index) + && let Ok(arg_expr) = find_format_arg_expr(self.expr, arg) + { + self.check_format_arg_self(arg_expr); } - && trait_name == impl_trait.name - && let Ok(index) = placeholder.argument.index - && let Some(arg) = format_args.arguments.all_args().get(index) - && let Ok(arg_expr) = find_format_arg_expr(expr, arg) - { - check_format_arg_self(cx, expr.span, arg_expr, impl_trait); } } } -} -fn check_format_arg_self(cx: &LateContext<'_>, span: Span, arg: &Expr<'_>, impl_trait: FormatTraitNames) { - // Handle multiple dereferencing of references e.g. &&self - // Handle dereference of &self -> self that is equivalent (i.e. via *self in fmt() impl) - // Since the argument to fmt is itself a reference: &self - let reference = peel_ref_operators(cx, arg); - let map = cx.tcx.hir(); - // Is the reference self? - if path_to_local(reference).map(|x| map.name(x)) == Some(kw::SelfLower) { - let FormatTraitNames { name, .. } = impl_trait; - span_lint( - cx, - RECURSIVE_FORMAT_IMPL, - span, - &format!("using `self` as `{name}` in `impl {name}` will cause infinite recursion"), - ); + fn check_format_arg_self(&self, arg: &Expr<'_>) { + // Handle multiple dereferencing of references e.g. &&self + // Handle dereference of &self -> self that is equivalent (i.e. via *self in fmt() impl) + // Since the argument to fmt is itself a reference: &self + let reference = peel_ref_operators(self.cx, arg); + let map = self.cx.tcx.hir(); + // Is the reference self? + if path_to_local(reference).map(|x| map.name(x)) == Some(kw::SelfLower) { + let FormatTraitNames { name, .. } = self.format_trait_impl; + span_lint( + self.cx, + RECURSIVE_FORMAT_IMPL, + self.expr.span, + &format!("using `self` as `{name}` in `impl {name}` will cause infinite recursion"), + ); + } } -} -fn check_print_in_format_impl(cx: &LateContext<'_>, expr: &Expr<'_>, impl_trait: FormatTraitNames) { - if let Some(macro_call) = root_macro_call_first_node(cx, expr) - && let Some(name) = cx.tcx.get_diagnostic_name(macro_call.def_id) - { - let replacement = match name { - sym::print_macro | sym::eprint_macro => "write", - sym::println_macro | sym::eprintln_macro => "writeln", - _ => return, - }; + fn check_print_in_format_impl(&self) { + if let Some(macro_call) = root_macro_call_first_node(self.cx, self.expr) + && let Some(name) = self.cx.tcx.get_diagnostic_name(macro_call.def_id) + { + let replacement = match name { + sym::print_macro | sym::eprint_macro => "write", + sym::println_macro | sym::eprintln_macro => "writeln", + _ => return, + }; - let name = name.as_str().strip_suffix("_macro").unwrap(); + let name = name.as_str().strip_suffix("_macro").unwrap(); - span_lint_and_sugg( - cx, - PRINT_IN_FORMAT_IMPL, - macro_call.span, - &format!("use of `{name}!` in `{}` impl", impl_trait.name), - "replace with", - if let Some(formatter_name) = impl_trait.formatter_name { - format!("{replacement}!({formatter_name}, ..)") - } else { - format!("{replacement}!(..)") - }, - Applicability::HasPlaceholders, - ); + span_lint_and_sugg( + self.cx, + PRINT_IN_FORMAT_IMPL, + macro_call.span, + &format!("use of `{name}!` in `{}` impl", self.format_trait_impl.name), + "replace with", + if let Some(formatter_name) = self.format_trait_impl.formatter_name { + format!("{replacement}!({formatter_name}, ..)") + } else { + format!("{replacement}!(..)") + }, + Applicability::HasPlaceholders, + ); + } } } diff --git a/clippy_lints/src/from_over_into.rs b/clippy_lints/src/from_over_into.rs index 1933a00891b..93527bcdf5c 100644 --- a/clippy_lints/src/from_over_into.rs +++ b/clippy_lints/src/from_over_into.rs @@ -181,6 +181,9 @@ fn convert_to_from( let from = snippet_opt(cx, self_ty.span)?; let into = snippet_opt(cx, target_ty.span)?; + let return_type = matches!(sig.decl.output, FnRetTy::Return(_)) + .then_some(String::from("Self")) + .unwrap_or_default(); let mut suggestions = vec![ // impl Into<T> for U -> impl From<T> for U // ~~~~ ~~~~ @@ -197,13 +200,10 @@ fn convert_to_from( // fn into([mut] self) -> T -> fn into([mut] v: T) -> T // ~~~~ ~~~~ (self_ident.span, format!("val: {from}")), - ]; - - if let FnRetTy::Return(_) = sig.decl.output { // fn into(self) -> T -> fn into(self) -> Self // ~ ~~~~ - suggestions.push((sig.decl.output.span(), String::from("Self"))); - } + (sig.decl.output.span(), return_type), + ]; let mut finder = SelfFinder { cx, diff --git a/clippy_lints/src/implied_bounds_in_impls.rs b/clippy_lints/src/implied_bounds_in_impls.rs index fab8ffedb94..174db30a7cd 100644 --- a/clippy_lints/src/implied_bounds_in_impls.rs +++ b/clippy_lints/src/implied_bounds_in_impls.rs @@ -1,16 +1,17 @@ use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::source::snippet; use rustc_errors::{Applicability, SuggestionStyle}; -use rustc_hir::def_id::{DefId, LocalDefId}; +use rustc_hir::def_id::DefId; use rustc_hir::intravisit::FnKind; use rustc_hir::{ - Body, FnDecl, FnRetTy, GenericArg, GenericBound, ImplItem, ImplItemKind, ItemKind, TraitBoundModifier, TraitItem, - TraitItemKind, TyKind, + Body, FnDecl, FnRetTy, GenericArg, GenericBound, ImplItem, ImplItemKind, ItemKind, OpaqueTy, TraitBoundModifier, + TraitItem, TraitItemKind, TyKind, TypeBinding, }; use rustc_hir_analysis::hir_ty_to_ty; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::{self, ClauseKind, Generics, Ty, TyCtxt}; use rustc_session::declare_lint_pass; +use rustc_span::def_id::LocalDefId; use rustc_span::Span; declare_clippy_lint! { @@ -50,20 +51,17 @@ declare_clippy_lint! { } declare_lint_pass!(ImpliedBoundsInImpls => [IMPLIED_BOUNDS_IN_IMPLS]); -#[allow(clippy::too_many_arguments)] fn emit_lint( cx: &LateContext<'_>, poly_trait: &rustc_hir::PolyTraitRef<'_>, opaque_ty: &rustc_hir::OpaqueTy<'_>, index: usize, - // The bindings that were implied + // The bindings that were implied, used for suggestion purposes since removing a bound with associated types + // means we might need to then move it to a different bound implied_bindings: &[rustc_hir::TypeBinding<'_>], - // The original bindings that `implied_bindings` are implied from - implied_by_bindings: &[rustc_hir::TypeBinding<'_>], - implied_by_args: &[GenericArg<'_>], - implied_by_span: Span, + bound: &ImplTraitBound<'_>, ) { - let implied_by = snippet(cx, implied_by_span, ".."); + let implied_by = snippet(cx, bound.span, ".."); span_lint_and_then( cx, @@ -93,17 +91,17 @@ fn emit_lint( // If we're going to suggest removing `Deref<..>`, we'll need to put `<Target = u8>` on `DerefMut` let omitted_assoc_tys: Vec<_> = implied_bindings .iter() - .filter(|binding| !implied_by_bindings.iter().any(|b| b.ident == binding.ident)) + .filter(|binding| !bound.bindings.iter().any(|b| b.ident == binding.ident)) .collect(); if !omitted_assoc_tys.is_empty() { // `<>` needs to be added if there aren't yet any generic arguments or bindings - let needs_angle_brackets = implied_by_args.is_empty() && implied_by_bindings.is_empty(); - let insert_span = match (implied_by_args, implied_by_bindings) { + let needs_angle_brackets = bound.args.is_empty() && bound.bindings.is_empty(); + let insert_span = match (bound.args, bound.bindings) { ([.., arg], [.., binding]) => arg.span().max(binding.span).shrink_to_hi(), ([.., arg], []) => arg.span().shrink_to_hi(), ([], [.., binding]) => binding.span.shrink_to_hi(), - ([], []) => implied_by_span.shrink_to_hi(), + ([], []) => bound.span.shrink_to_hi(), }; let mut associated_tys_sugg = if needs_angle_brackets { @@ -223,42 +221,93 @@ fn is_same_generics<'tcx>( }) } +struct ImplTraitBound<'tcx> { + /// The span of the bound in the `impl Trait` type + span: Span, + /// The predicates defined in the trait referenced by this bound. This also contains the actual + /// supertrait bounds + predicates: &'tcx [(ty::Clause<'tcx>, Span)], + /// The `DefId` of the trait being referenced by this bound + trait_def_id: DefId, + /// The generic arguments on the `impl Trait` bound + args: &'tcx [GenericArg<'tcx>], + /// The associated types on this bound + bindings: &'tcx [TypeBinding<'tcx>], +} + +/// Given an `impl Trait` type, gets all the supertraits from each bound ("implied bounds"). +/// +/// For `impl Deref + DerefMut + Eq` this returns `[Deref, PartialEq]`. +/// The `Deref` comes from `DerefMut` because `trait DerefMut: Deref {}`, and `PartialEq` comes from +/// `Eq`. +fn collect_supertrait_bounds<'tcx>(cx: &LateContext<'tcx>, opaque_ty: &OpaqueTy<'tcx>) -> Vec<ImplTraitBound<'tcx>> { + opaque_ty + .bounds + .iter() + .filter_map(|bound| { + if let GenericBound::Trait(poly_trait, TraitBoundModifier::None) = bound + && let [.., path] = poly_trait.trait_ref.path.segments + && poly_trait.bound_generic_params.is_empty() + && let Some(trait_def_id) = path.res.opt_def_id() + && let predicates = cx.tcx.super_predicates_of(trait_def_id).predicates + // If the trait has no supertrait, there is no need to collect anything from that bound + && !predicates.is_empty() + { + Some(ImplTraitBound { + predicates, + args: path.args.map_or([].as_slice(), |p| p.args), + bindings: path.args.map_or([].as_slice(), |p| p.bindings), + trait_def_id, + span: bound.span(), + }) + } else { + None + } + }) + .collect() +} + +/// Given a bound in an `impl Trait` type, looks for a trait in the set of supertraits (previously +/// collected in [`collect_supertrait_bounds`]) that matches (same trait and generic arguments). +fn find_bound_in_supertraits<'a, 'tcx>( + cx: &LateContext<'tcx>, + trait_def_id: DefId, + args: &'tcx [GenericArg<'tcx>], + bounds: &'a [ImplTraitBound<'tcx>], +) -> Option<&'a ImplTraitBound<'tcx>> { + bounds.iter().find(|bound| { + bound.predicates.iter().any(|(clause, _)| { + if let ClauseKind::Trait(tr) = clause.kind().skip_binder() + && tr.def_id() == trait_def_id + { + is_same_generics( + cx.tcx, + tr.trait_ref.args, + bound.args, + args, + bound.trait_def_id, + trait_def_id, + ) + } else { + false + } + }) + }) +} + fn check(cx: &LateContext<'_>, decl: &FnDecl<'_>) { if let FnRetTy::Return(ty) = decl.output - &&let TyKind::OpaqueDef(item_id, ..) = ty.kind + && let TyKind::OpaqueDef(item_id, ..) = ty.kind && let item = cx.tcx.hir().item(item_id) && let ItemKind::OpaqueTy(opaque_ty) = item.kind // Very often there is only a single bound, e.g. `impl Deref<..>`, in which case // we can avoid doing a bunch of stuff unnecessarily. && opaque_ty.bounds.len() > 1 { - // Get all the (implied) trait predicates in the bounds. - // For `impl Deref + DerefMut` this will contain [`Deref`]. - // The implied `Deref` comes from `DerefMut` because `trait DerefMut: Deref {}`. - // N.B. (G)ATs are fine to disregard, because they must be the same for all of its supertraits. - // Example: - // `impl Deref<Target = i32> + DerefMut<Target = u32>` is not allowed. - // `DerefMut::Target` needs to match `Deref::Target`. - let implied_bounds: Vec<_> = opaque_ty - .bounds - .iter() - .filter_map(|bound| { - if let GenericBound::Trait(poly_trait, TraitBoundModifier::None) = bound - && let [.., path] = poly_trait.trait_ref.path.segments - && poly_trait.bound_generic_params.is_empty() - && let Some(trait_def_id) = path.res.opt_def_id() - && let predicates = cx.tcx.super_predicates_of(trait_def_id).predicates - && !predicates.is_empty() - // If the trait has no supertrait, there is nothing to add. - { - Some((bound.span(), path, predicates, trait_def_id)) - } else { - None - } - }) - .collect(); + let supertraits = collect_supertrait_bounds(cx, opaque_ty); - // Lint all bounds in the `impl Trait` type that are also in the `implied_bounds` vec. + // Lint all bounds in the `impl Trait` type that we've previously also seen in the set of + // supertraits of each of the bounds. // This involves some extra logic when generic arguments are present, since // simply comparing trait `DefId`s won't be enough. We also need to compare the generics. for (index, bound) in opaque_ty.bounds.iter().enumerate() { @@ -267,42 +316,18 @@ fn check(cx: &LateContext<'_>, decl: &FnDecl<'_>) { && let implied_args = path.args.map_or([].as_slice(), |a| a.args) && let implied_bindings = path.args.map_or([].as_slice(), |a| a.bindings) && let Some(def_id) = poly_trait.trait_ref.path.res.opt_def_id() - && let Some((implied_by_span, implied_by_args, implied_by_bindings)) = - implied_bounds - .iter() - .find_map(|&(span, implied_by_path, preds, implied_by_def_id)| { - let implied_by_args = implied_by_path.args.map_or([].as_slice(), |a| a.args); - let implied_by_bindings = implied_by_path.args.map_or([].as_slice(), |a| a.bindings); - - preds.iter().find_map(|(clause, _)| { - if let ClauseKind::Trait(tr) = clause.kind().skip_binder() - && tr.def_id() == def_id - && is_same_generics( - cx.tcx, - tr.trait_ref.args, - implied_by_args, - implied_args, - implied_by_def_id, - def_id, - ) - { - Some((span, implied_by_args, implied_by_bindings)) - } else { - None - } - }) - }) + && let Some(bound) = find_bound_in_supertraits(cx, def_id, implied_args, &supertraits) + // If the implied bound has a type binding that also exists in the implied-by trait, + // then we shouldn't lint. See #11880 for an example. + && let assocs = cx.tcx.associated_items(bound.trait_def_id) + && !implied_bindings.iter().any(|binding| { + assocs + .filter_by_name_unhygienic(binding.ident.name) + .next() + .is_some_and(|assoc| assoc.kind == ty::AssocKind::Type) + }) { - emit_lint( - cx, - poly_trait, - opaque_ty, - index, - implied_bindings, - implied_by_bindings, - implied_by_args, - implied_by_span, - ); + emit_lint(cx, poly_trait, opaque_ty, index, implied_bindings, bound); } } } diff --git a/clippy_lints/src/incompatible_msrv.rs b/clippy_lints/src/incompatible_msrv.rs index f2f0e7d4266..cd000fcd184 100644 --- a/clippy_lints/src/incompatible_msrv.rs +++ b/clippy_lints/src/incompatible_msrv.rs @@ -1,14 +1,15 @@ use clippy_config::msrvs::Msrv; use clippy_utils::diagnostics::span_lint; +use clippy_utils::is_in_test_function; use rustc_attr::{StabilityLevel, StableSince}; use rustc_data_structures::fx::FxHashMap; -use rustc_hir::{Expr, ExprKind}; +use rustc_hir::{Expr, ExprKind, HirId}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::TyCtxt; use rustc_semver::RustcVersion; use rustc_session::impl_lint_pass; use rustc_span::def_id::DefId; -use rustc_span::Span; +use rustc_span::{ExpnKind, Span}; declare_clippy_lint! { /// ### What it does @@ -81,13 +82,18 @@ impl IncompatibleMsrv { version } - fn emit_lint_if_under_msrv(&mut self, cx: &LateContext<'_>, def_id: DefId, span: Span) { + fn emit_lint_if_under_msrv(&mut self, cx: &LateContext<'_>, def_id: DefId, node: HirId, span: Span) { if def_id.is_local() { // We don't check local items since their MSRV is supposed to always be valid. return; } let version = self.get_def_id_version(cx.tcx, def_id); - if self.msrv.meets(version) { + if self.msrv.meets(version) || is_in_test_function(cx.tcx, node) { + return; + } + if let ExpnKind::AstPass(_) | ExpnKind::Desugaring(_) = span.ctxt().outer_expn_data().kind { + // Desugared expressions get to cheat and stability is ignored. + // Intentionally not using `.from_expansion()`, since we do still care about macro expansions return; } self.emit_lint_for(cx, span, version); @@ -117,14 +123,14 @@ impl<'tcx> LateLintPass<'tcx> for IncompatibleMsrv { match expr.kind { ExprKind::MethodCall(_, _, _, span) => { if let Some(method_did) = cx.typeck_results().type_dependent_def_id(expr.hir_id) { - self.emit_lint_if_under_msrv(cx, method_did, span); + self.emit_lint_if_under_msrv(cx, method_did, expr.hir_id, span); } }, ExprKind::Call(call, [_]) => { if let ExprKind::Path(qpath) = call.kind && let Some(path_def_id) = cx.qpath_res(&qpath, call.hir_id).opt_def_id() { - self.emit_lint_if_under_msrv(cx, path_def_id, call.span); + self.emit_lint_if_under_msrv(cx, path_def_id, expr.hir_id, call.span); } }, _ => {}, diff --git a/clippy_lints/src/index_refutable_slice.rs b/clippy_lints/src/index_refutable_slice.rs index 41e9d5b1c2e..5b5eb355f86 100644 --- a/clippy_lints/src/index_refutable_slice.rs +++ b/clippy_lints/src/index_refutable_slice.rs @@ -255,7 +255,9 @@ impl<'a, 'tcx> Visitor<'tcx> for SliceIndexLintingVisitor<'a, 'tcx> { && let hir::Node::Expr(maybe_addrof_expr) = cx.tcx.parent_hir_node(parent_id) && let hir::ExprKind::AddrOf(_kind, hir::Mutability::Not, _inner_expr) = maybe_addrof_expr.kind { - use_info.index_use.push((index_value, cx.tcx.hir().span(parent_expr.hir_id))); + use_info + .index_use + .push((index_value, cx.tcx.hir().span(parent_expr.hir_id))); return; } diff --git a/clippy_lints/src/indexing_slicing.rs b/clippy_lints/src/indexing_slicing.rs index 391db0b0df7..35fcd8cdd35 100644 --- a/clippy_lints/src/indexing_slicing.rs +++ b/clippy_lints/src/indexing_slicing.rs @@ -174,6 +174,7 @@ impl<'tcx> LateLintPass<'tcx> for IndexingSlicing { // only `usize` index is legal in rust array index // leave other type to rustc if let Constant::Int(off) = constant + && off <= usize::MAX as u128 && let ty::Uint(utype) = cx.typeck_results().expr_ty(index).kind() && *utype == ty::UintTy::Usize && let ty::Array(_, s) = ty.kind() diff --git a/clippy_lints/src/item_name_repetitions.rs b/clippy_lints/src/item_name_repetitions.rs index 276c1abb60c..0b4c416d94d 100644 --- a/clippy_lints/src/item_name_repetitions.rs +++ b/clippy_lints/src/item_name_repetitions.rs @@ -385,7 +385,6 @@ impl LateLintPass<'_> for ItemNameRepetitions { assert!(last.is_some()); } - #[expect(clippy::similar_names)] fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { let item_name = item.ident.name.as_str(); let item_camel = to_camel_case(item_name); diff --git a/clippy_lints/src/iter_not_returning_iterator.rs b/clippy_lints/src/iter_not_returning_iterator.rs index b9fad726511..2344f040522 100644 --- a/clippy_lints/src/iter_not_returning_iterator.rs +++ b/clippy_lints/src/iter_not_returning_iterator.rs @@ -57,7 +57,7 @@ impl<'tcx> LateLintPass<'tcx> for IterNotReturningIterator { if matches!(name, "iter" | "iter_mut") && !matches!( get_parent_node(cx.tcx, item.hir_id()), - Some(Node::Item(Item { kind: ItemKind::Impl(i), .. })) if i.of_trait.is_some() + Node::Item(Item { kind: ItemKind::Impl(i), .. }) if i.of_trait.is_some() ) { if let ImplItemKind::Fn(fn_sig, _) = &item.kind { diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 14bd82f9c97..1bca1f2bd6f 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -14,7 +14,7 @@ clippy::missing_docs_in_private_items, clippy::must_use_candidate, rustc::diagnostic_outside_of_impl, - rustc::untranslatable_diagnostic, + rustc::untranslatable_diagnostic )] #![warn(trivial_casts, trivial_numeric_casts)] // warn on lints, that are included in `rust-lang/rust`s bootstrap diff --git a/clippy_lints/src/loops/infinite_loop.rs b/clippy_lints/src/loops/infinite_loop.rs index 5e099f1e76f..5b5bb88c179 100644 --- a/clippy_lints/src/loops/infinite_loop.rs +++ b/clippy_lints/src/loops/infinite_loop.rs @@ -1,17 +1,18 @@ use clippy_utils::diagnostics::span_lint_and_then; -use clippy_utils::{fn_def_id, is_lint_allowed}; +use clippy_utils::{fn_def_id, is_from_proc_macro, is_lint_allowed}; use hir::intravisit::{walk_expr, Visitor}; use hir::{Expr, ExprKind, FnRetTy, FnSig, Node}; use rustc_ast::Label; use rustc_errors::Applicability; use rustc_hir as hir; -use rustc_lint::LateContext; +use rustc_lint::{LateContext, LintContext}; +use rustc_middle::lint::in_external_macro; use super::INFINITE_LOOP; pub(super) fn check<'tcx>( cx: &LateContext<'tcx>, - expr: &Expr<'_>, + expr: &Expr<'tcx>, loop_block: &'tcx hir::Block<'_>, label: Option<Label>, ) { @@ -34,6 +35,10 @@ pub(super) fn check<'tcx>( return; } + if in_external_macro(cx.sess(), expr.span) || is_from_proc_macro(cx, expr) { + return; + } + let mut loop_visitor = LoopVisitor { cx, label, diff --git a/clippy_lints/src/macro_use.rs b/clippy_lints/src/macro_use.rs index 8d3e7520a54..aea973bd249 100644 --- a/clippy_lints/src/macro_use.rs +++ b/clippy_lints/src/macro_use.rs @@ -36,7 +36,8 @@ struct PathAndSpan { span: Span, } -/// `MacroRefData` includes the name of the macro. +/// `MacroRefData` includes the name of the macro +/// and the path from `SourceMap::span_to_filename`. #[derive(Debug, Clone)] pub struct MacroRefData { name: String, diff --git a/clippy_lints/src/matches/match_same_arms.rs b/clippy_lints/src/matches/match_same_arms.rs index b8f7d966820..c7c453b7f6e 100644 --- a/clippy_lints/src/matches/match_same_arms.rs +++ b/clippy_lints/src/matches/match_same_arms.rs @@ -64,38 +64,50 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) { let min_index = usize::min(lindex, rindex); let max_index = usize::max(lindex, rindex); - let mut local_map: HirIdMap<HirId> = HirIdMap::default(); - let eq_fallback = |a: &Expr<'_>, b: &Expr<'_>| { - if let Some(a_id) = path_to_local(a) - && let Some(b_id) = path_to_local(b) - && let entry = match local_map.entry(a_id) { - HirIdMapEntry::Vacant(entry) => entry, - // check if using the same bindings as before - HirIdMapEntry::Occupied(entry) => return *entry.get() == b_id, - } + let check_eq_with_pat = |expr_a: &Expr<'_>, expr_b: &Expr<'_>| { + let mut local_map: HirIdMap<HirId> = HirIdMap::default(); + let eq_fallback = |a: &Expr<'_>, b: &Expr<'_>| { + if let Some(a_id) = path_to_local(a) + && let Some(b_id) = path_to_local(b) + && let entry = match local_map.entry(a_id) { + HirIdMapEntry::Vacant(entry) => entry, + // check if using the same bindings as before + HirIdMapEntry::Occupied(entry) => return *entry.get() == b_id, + } // the names technically don't have to match; this makes the lint more conservative && cx.tcx.hir().name(a_id) == cx.tcx.hir().name(b_id) - && cx.typeck_results().expr_ty(a) == cx.typeck_results().expr_ty(b) - && pat_contains_local(lhs.pat, a_id) - && pat_contains_local(rhs.pat, b_id) - { - entry.insert(b_id); - true - } else { - false - } - }; - // Arms with a guard are ignored, those can’t always be merged together - // If both arms overlap with an arm in between then these can't be merged either. - !(backwards_blocking_idxs[max_index] > min_index && forwards_blocking_idxs[min_index] < max_index) - && lhs.guard.is_none() - && rhs.guard.is_none() - && SpanlessEq::new(cx) - .expr_fallback(eq_fallback) - .eq_expr(lhs.body, rhs.body) + && cx.typeck_results().expr_ty(a) == cx.typeck_results().expr_ty(b) + && pat_contains_local(lhs.pat, a_id) + && pat_contains_local(rhs.pat, b_id) + { + entry.insert(b_id); + true + } else { + false + } + }; + + SpanlessEq::new(cx) + .expr_fallback(eq_fallback) + .eq_expr(expr_a, expr_b) // these checks could be removed to allow unused bindings && bindings_eq(lhs.pat, local_map.keys().copied().collect()) && bindings_eq(rhs.pat, local_map.values().copied().collect()) + }; + + let check_same_guard = || match (&lhs.guard, &rhs.guard) { + (None, None) => true, + (Some(lhs_guard), Some(rhs_guard)) => check_eq_with_pat(lhs_guard, rhs_guard), + _ => false, + }; + + let check_same_body = || check_eq_with_pat(lhs.body, rhs.body); + + // Arms with different guard are ignored, those can’t always be merged together + // If both arms overlap with an arm in between then these can't be merged either. + !(backwards_blocking_idxs[max_index] > min_index && forwards_blocking_idxs[min_index] < max_index) + && check_same_guard() + && check_same_body() }; let indexed_arms: Vec<(usize, &Arm<'_>)> = arms.iter().enumerate().collect(); diff --git a/clippy_lints/src/matches/needless_match.rs b/clippy_lints/src/matches/needless_match.rs index cc482f15a91..2ee7bce9962 100644 --- a/clippy_lints/src/matches/needless_match.rs +++ b/clippy_lints/src/matches/needless_match.rs @@ -123,37 +123,35 @@ fn strip_return<'hir>(expr: &'hir Expr<'hir>) -> &'hir Expr<'hir> { /// Manually check for coercion casting by checking if the type of the match operand or let expr /// differs with the assigned local variable or the function return type. fn expr_ty_matches_p_ty(cx: &LateContext<'_>, expr: &Expr<'_>, p_expr: &Expr<'_>) -> bool { - if let Some(p_node) = get_parent_node(cx.tcx, p_expr.hir_id) { - match p_node { - // Compare match_expr ty with local in `let local = match match_expr {..}` - Node::Local(local) => { - let results = cx.typeck_results(); - return same_type_and_consts(results.node_type(local.hir_id), results.expr_ty(expr)); - }, - // compare match_expr ty with RetTy in `fn foo() -> RetTy` - Node::Item(item) => { - if let ItemKind::Fn(..) = item.kind { - let output = cx - .tcx - .fn_sig(item.owner_id) - .instantiate_identity() - .output() - .skip_binder(); - return same_type_and_consts(output, cx.typeck_results().expr_ty(expr)); - } - }, - // check the parent expr for this whole block `{ match match_expr {..} }` - Node::Block(block) => { - if let Some(block_parent_expr) = get_parent_expr_for_hir(cx, block.hir_id) { - return expr_ty_matches_p_ty(cx, expr, block_parent_expr); - } - }, - // recursively call on `if xxx {..}` etc. - Node::Expr(p_expr) => { - return expr_ty_matches_p_ty(cx, expr, p_expr); - }, - _ => {}, - } + match get_parent_node(cx.tcx, p_expr.hir_id) { + // Compare match_expr ty with local in `let local = match match_expr {..}` + Node::Local(local) => { + let results = cx.typeck_results(); + return same_type_and_consts(results.node_type(local.hir_id), results.expr_ty(expr)); + }, + // compare match_expr ty with RetTy in `fn foo() -> RetTy` + Node::Item(item) => { + if let ItemKind::Fn(..) = item.kind { + let output = cx + .tcx + .fn_sig(item.owner_id) + .instantiate_identity() + .output() + .skip_binder(); + return same_type_and_consts(output, cx.typeck_results().expr_ty(expr)); + } + }, + // check the parent expr for this whole block `{ match match_expr {..} }` + Node::Block(block) => { + if let Some(block_parent_expr) = get_parent_expr_for_hir(cx, block.hir_id) { + return expr_ty_matches_p_ty(cx, expr, block_parent_expr); + } + }, + // recursively call on `if xxx {..}` etc. + Node::Expr(p_expr) => { + return expr_ty_matches_p_ty(cx, expr, p_expr); + }, + _ => {}, } false } diff --git a/clippy_lints/src/matches/redundant_guards.rs b/clippy_lints/src/matches/redundant_guards.rs index a1b82679f2e..d3986cc7c22 100644 --- a/clippy_lints/src/matches/redundant_guards.rs +++ b/clippy_lints/src/matches/redundant_guards.rs @@ -5,7 +5,7 @@ use clippy_utils::visitors::{for_each_expr, is_local_used}; use rustc_ast::{BorrowKind, LitKind}; use rustc_errors::Applicability; use rustc_hir::def::{DefKind, Res}; -use rustc_hir::{Arm, BinOpKind, Expr, ExprKind, MatchSource, Node, Pat, PatKind}; +use rustc_hir::{Arm, BinOpKind, Expr, ExprKind, MatchSource, Node, Pat, PatKind, UnOp}; use rustc_lint::LateContext; use rustc_span::symbol::Ident; use rustc_span::{Span, Symbol}; @@ -269,7 +269,11 @@ fn expr_can_be_pat(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { Res::Def(DefKind::Struct | DefKind::Enum | DefKind::Ctor(..), ..), ) }, - ExprKind::AddrOf(..) | ExprKind::Array(..) | ExprKind::Tup(..) | ExprKind::Struct(..) => true, + ExprKind::AddrOf(..) + | ExprKind::Array(..) + | ExprKind::Tup(..) + | ExprKind::Struct(..) + | ExprKind::Unary(UnOp::Neg, _) => true, ExprKind::Lit(lit) if !matches!(lit.node, LitKind::Float(..)) => true, _ => false, } { diff --git a/clippy_lints/src/matches/significant_drop_in_scrutinee.rs b/clippy_lints/src/matches/significant_drop_in_scrutinee.rs index a7e42fd2405..78107c0cd23 100644 --- a/clippy_lints/src/matches/significant_drop_in_scrutinee.rs +++ b/clippy_lints/src/matches/significant_drop_in_scrutinee.rs @@ -37,7 +37,12 @@ pub(super) fn check<'tcx>( } } -fn set_diagnostic<'tcx>(diag: &mut DiagnosticBuilder<'_, ()>, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, found: FoundSigDrop) { +fn set_diagnostic<'tcx>( + diag: &mut DiagnosticBuilder<'_, ()>, + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'tcx>, + found: FoundSigDrop, +) { if found.lint_suggestion == LintSuggestion::MoveAndClone { // If our suggestion is to move and clone, then we want to leave it to the user to // decide how to address this lint, since it may be that cloning is inappropriate. diff --git a/clippy_lints/src/mem_replace.rs b/clippy_lints/src/mem_replace.rs index fa4f4a47e38..1cdb7921f81 100644 --- a/clippy_lints/src/mem_replace.rs +++ b/clippy_lints/src/mem_replace.rs @@ -3,7 +3,9 @@ use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg, span_lin use clippy_utils::source::{snippet, snippet_with_applicability}; use clippy_utils::sugg::Sugg; use clippy_utils::ty::is_non_aggregate_primitive_type; -use clippy_utils::{is_default_equivalent, is_res_lang_ctor, path_res, peel_ref_operators, std_or_core}; +use clippy_utils::{ + is_default_equivalent, is_expr_used_or_unified, is_res_lang_ctor, path_res, peel_ref_operators, std_or_core, +}; use rustc_errors::Applicability; use rustc_hir::LangItem::OptionNone; use rustc_hir::{Expr, ExprKind}; @@ -232,7 +234,7 @@ impl<'tcx> LateLintPass<'tcx> for MemReplace { // Check that second argument is `Option::None` if is_res_lang_ctor(cx, path_res(cx, src), OptionNone) { check_replace_option_with_none(cx, dest, expr.span); - } else if self.msrv.meets(msrvs::MEM_TAKE) { + } else if self.msrv.meets(msrvs::MEM_TAKE) && is_expr_used_or_unified(cx.tcx, expr) { check_replace_with_default(cx, src, dest, expr.span); } check_replace_with_uninit(cx, src, dest, expr.span); diff --git a/clippy_lints/src/methods/case_sensitive_file_extension_comparisons.rs b/clippy_lints/src/methods/case_sensitive_file_extension_comparisons.rs index a37087d0abf..a5df863d800 100644 --- a/clippy_lints/src/methods/case_sensitive_file_extension_comparisons.rs +++ b/clippy_lints/src/methods/case_sensitive_file_extension_comparisons.rs @@ -38,6 +38,7 @@ pub(super) fn check<'tcx>( && ext_str.starts_with('.') && (ext_str.chars().skip(1).all(|c| c.is_uppercase() || c.is_ascii_digit()) || ext_str.chars().skip(1).all(|c| c.is_lowercase() || c.is_ascii_digit())) + && !ext_str.chars().skip(1).all(|c| c.is_ascii_digit()) && let recv_ty = cx.typeck_results().expr_ty(recv).peel_refs() && (recv_ty.is_str() || is_type_lang_item(cx, recv_ty, LangItem::String)) { diff --git a/clippy_lints/src/methods/clone_on_copy.rs b/clippy_lints/src/methods/clone_on_copy.rs index 532bbbeaf03..8736341f6fe 100644 --- a/clippy_lints/src/methods/clone_on_copy.rs +++ b/clippy_lints/src/methods/clone_on_copy.rs @@ -49,7 +49,7 @@ pub(super) fn check( if is_copy(cx, ty) { let parent_is_suffix_expr = match get_parent_node(cx.tcx, expr.hir_id) { - Some(Node::Expr(parent)) => match parent.kind { + Node::Expr(parent) => match parent.kind { // &*x is a nop, &x.clone() is not ExprKind::AddrOf(..) => return, // (*x).func() is useless, x.clone().func() can work in case func borrows self @@ -70,7 +70,7 @@ pub(super) fn check( _ => false, }, // local binding capturing a reference - Some(Node::Local(l)) if matches!(l.pat.kind, PatKind::Binding(BindingAnnotation(ByRef::Yes, _), ..)) => { + Node::Local(l) if matches!(l.pat.kind, PatKind::Binding(BindingAnnotation(ByRef::Yes, _), ..)) => { return; }, _ => false, diff --git a/clippy_lints/src/methods/needless_collect.rs b/clippy_lints/src/methods/needless_collect.rs index 293b4981c55..8363b57169f 100644 --- a/clippy_lints/src/methods/needless_collect.rs +++ b/clippy_lints/src/methods/needless_collect.rs @@ -28,104 +28,102 @@ pub(super) fn check<'tcx>( iter_expr: &'tcx Expr<'tcx>, call_span: Span, ) { - if let Some(parent) = get_parent_node(cx.tcx, collect_expr.hir_id) { - match parent { - Node::Expr(parent) => { - check_collect_into_intoiterator(cx, parent, collect_expr, call_span, iter_expr); + match get_parent_node(cx.tcx, collect_expr.hir_id) { + Node::Expr(parent) => { + check_collect_into_intoiterator(cx, parent, collect_expr, call_span, iter_expr); - if let ExprKind::MethodCall(name, _, args @ ([] | [_]), _) = parent.kind { - let mut app = Applicability::MachineApplicable; - let name = name.ident.as_str(); - let collect_ty = cx.typeck_results().expr_ty(collect_expr); + if let ExprKind::MethodCall(name, _, args @ ([] | [_]), _) = parent.kind { + let mut app = Applicability::MachineApplicable; + let name = name.ident.as_str(); + let collect_ty = cx.typeck_results().expr_ty(collect_expr); - let sugg: String = match name { - "len" => { - if let Some(adt) = collect_ty.ty_adt_def() - && matches!( - cx.tcx.get_diagnostic_name(adt.did()), - Some(sym::Vec | sym::VecDeque | sym::LinkedList | sym::BinaryHeap) - ) - { - "count()".into() - } else { - return; - } - }, - "is_empty" - if is_is_empty_sig(cx, parent.hir_id) - && iterates_same_ty(cx, cx.typeck_results().expr_ty(iter_expr), collect_ty) => + let sugg: String = match name { + "len" => { + if let Some(adt) = collect_ty.ty_adt_def() + && matches!( + cx.tcx.get_diagnostic_name(adt.did()), + Some(sym::Vec | sym::VecDeque | sym::LinkedList | sym::BinaryHeap) + ) { - "next().is_none()".into() - }, - "contains" => { - if is_contains_sig(cx, parent.hir_id, iter_expr) - && let Some(arg) = args.first() - { - let (span, prefix) = if let ExprKind::AddrOf(_, _, arg) = arg.kind { - (arg.span, "") - } else { - (arg.span, "*") - }; - let snip = snippet_with_applicability(cx, span, "??", &mut app); - format!("any(|x| x == {prefix}{snip})") + "count()".into() + } else { + return; + } + }, + "is_empty" + if is_is_empty_sig(cx, parent.hir_id) + && iterates_same_ty(cx, cx.typeck_results().expr_ty(iter_expr), collect_ty) => + { + "next().is_none()".into() + }, + "contains" => { + if is_contains_sig(cx, parent.hir_id, iter_expr) + && let Some(arg) = args.first() + { + let (span, prefix) = if let ExprKind::AddrOf(_, _, arg) = arg.kind { + (arg.span, "") } else { - return; - } - }, - _ => return, - }; + (arg.span, "*") + }; + let snip = snippet_with_applicability(cx, span, "??", &mut app); + format!("any(|x| x == {prefix}{snip})") + } else { + return; + } + }, + _ => return, + }; - span_lint_and_sugg( - cx, - NEEDLESS_COLLECT, - call_span.with_hi(parent.span.hi()), - NEEDLESS_COLLECT_MSG, - "replace with", - sugg, - app, - ); + span_lint_and_sugg( + cx, + NEEDLESS_COLLECT, + call_span.with_hi(parent.span.hi()), + NEEDLESS_COLLECT_MSG, + "replace with", + sugg, + app, + ); + } + }, + Node::Local(l) => { + if let PatKind::Binding(BindingAnnotation::NONE | BindingAnnotation::MUT, id, _, None) = l.pat.kind + && let ty = cx.typeck_results().expr_ty(collect_expr) + && [sym::Vec, sym::VecDeque, sym::BinaryHeap, sym::LinkedList] + .into_iter() + .any(|item| is_type_diagnostic_item(cx, ty, item)) + && let iter_ty = cx.typeck_results().expr_ty(iter_expr) + && let Some(block) = get_enclosing_block(cx, l.hir_id) + && let Some(iter_calls) = detect_iter_and_into_iters(block, id, cx, get_captured_ids(cx, iter_ty)) + && let [iter_call] = &*iter_calls + { + let mut used_count_visitor = UsedCountVisitor { cx, id, count: 0 }; + walk_block(&mut used_count_visitor, block); + if used_count_visitor.count > 1 { + return; } - }, - Node::Local(l) => { - if let PatKind::Binding(BindingAnnotation::NONE | BindingAnnotation::MUT, id, _, None) = l.pat.kind - && let ty = cx.typeck_results().expr_ty(collect_expr) - && [sym::Vec, sym::VecDeque, sym::BinaryHeap, sym::LinkedList] - .into_iter() - .any(|item| is_type_diagnostic_item(cx, ty, item)) - && let iter_ty = cx.typeck_results().expr_ty(iter_expr) - && let Some(block) = get_enclosing_block(cx, l.hir_id) - && let Some(iter_calls) = detect_iter_and_into_iters(block, id, cx, get_captured_ids(cx, iter_ty)) - && let [iter_call] = &*iter_calls - { - let mut used_count_visitor = UsedCountVisitor { cx, id, count: 0 }; - walk_block(&mut used_count_visitor, block); - if used_count_visitor.count > 1 { - return; - } - // Suggest replacing iter_call with iter_replacement, and removing stmt - let mut span = MultiSpan::from_span(name_span); - span.push_span_label(iter_call.span, "the iterator could be used here instead"); - span_lint_hir_and_then( - cx, - super::NEEDLESS_COLLECT, - collect_expr.hir_id, - span, - NEEDLESS_COLLECT_MSG, - |diag| { - let iter_replacement = - format!("{}{}", Sugg::hir(cx, iter_expr, ".."), iter_call.get_iter_method(cx)); - diag.multipart_suggestion( - iter_call.get_suggestion_text(), - vec![(l.span, String::new()), (iter_call.span, iter_replacement)], - Applicability::MaybeIncorrect, - ); - }, - ); - } - }, - _ => (), - } + // Suggest replacing iter_call with iter_replacement, and removing stmt + let mut span = MultiSpan::from_span(name_span); + span.push_span_label(iter_call.span, "the iterator could be used here instead"); + span_lint_hir_and_then( + cx, + super::NEEDLESS_COLLECT, + collect_expr.hir_id, + span, + NEEDLESS_COLLECT_MSG, + |diag| { + let iter_replacement = + format!("{}{}", Sugg::hir(cx, iter_expr, ".."), iter_call.get_iter_method(cx)); + diag.multipart_suggestion( + iter_call.get_suggestion_text(), + vec![(l.span, String::new()), (iter_call.span, iter_replacement)], + Applicability::MaybeIncorrect, + ); + }, + ); + } + }, + _ => (), } } diff --git a/clippy_lints/src/methods/unnecessary_fallible_conversions.rs b/clippy_lints/src/methods/unnecessary_fallible_conversions.rs index 89cf20c14fc..d46b584f8f4 100644 --- a/clippy_lints/src/methods/unnecessary_fallible_conversions.rs +++ b/clippy_lints/src/methods/unnecessary_fallible_conversions.rs @@ -2,7 +2,7 @@ use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::get_parent_expr; use clippy_utils::ty::implements_trait; use rustc_errors::Applicability; -use rustc_hir::{Expr, ExprKind}; +use rustc_hir::{Expr, ExprKind, QPath}; use rustc_lint::LateContext; use rustc_middle::ty; use rustc_middle::ty::print::with_forced_trimmed_paths; @@ -10,17 +10,71 @@ use rustc_span::{sym, Span}; use super::UNNECESSARY_FALLIBLE_CONVERSIONS; +#[derive(Copy, Clone)] +enum SpansKind { + TraitFn { trait_span: Span, fn_span: Span }, + Fn { fn_span: Span }, +} + /// What function is being called and whether that call is written as a method call or a function /// call #[derive(Copy, Clone)] #[expect(clippy::enum_variant_names)] enum FunctionKind { /// `T::try_from(U)` - TryFromFunction, + TryFromFunction(Option<SpansKind>), /// `t.try_into()` TryIntoMethod, /// `U::try_into(t)` - TryIntoFunction, + TryIntoFunction(Option<SpansKind>), +} + +impl FunctionKind { + fn appl_sugg(&self, parent_unwrap_call: Option<Span>, primary_span: Span) -> (Applicability, Vec<(Span, String)>) { + let Some(unwrap_span) = parent_unwrap_call else { + return (Applicability::Unspecified, self.default_sugg(primary_span)); + }; + + match &self { + FunctionKind::TryFromFunction(None) | FunctionKind::TryIntoFunction(None) => { + (Applicability::Unspecified, self.default_sugg(primary_span)) + }, + _ => ( + Applicability::MachineApplicable, + self.machine_applicable_sugg(primary_span, unwrap_span), + ), + } + } + + fn default_sugg(&self, primary_span: Span) -> Vec<(Span, String)> { + let replacement = match *self { + FunctionKind::TryFromFunction(_) => "From::from", + FunctionKind::TryIntoFunction(_) => "Into::into", + FunctionKind::TryIntoMethod => "into", + }; + + vec![(primary_span, String::from(replacement))] + } + + fn machine_applicable_sugg(&self, primary_span: Span, unwrap_span: Span) -> Vec<(Span, String)> { + let (trait_name, fn_name) = match self { + FunctionKind::TryFromFunction(_) => ("From".to_owned(), "from".to_owned()), + FunctionKind::TryIntoFunction(_) | FunctionKind::TryIntoMethod => ("Into".to_owned(), "into".to_owned()), + }; + + let mut sugg = match *self { + FunctionKind::TryFromFunction(Some(spans)) | FunctionKind::TryIntoFunction(Some(spans)) => match spans { + SpansKind::TraitFn { trait_span, fn_span } => vec![(trait_span, trait_name), (fn_span, fn_name)], + SpansKind::Fn { fn_span } => vec![(fn_span, fn_name)], + }, + FunctionKind::TryIntoMethod => vec![(primary_span, fn_name)], + // Or the suggestion is not machine-applicable + _ => unreachable!(), + }; + + sugg.push((unwrap_span, String::new())); + sugg + } } fn check<'tcx>( @@ -35,8 +89,8 @@ fn check<'tcx>( && self_ty != other_ty && let Some(self_ty) = self_ty.as_type() && let Some(from_into_trait) = cx.tcx.get_diagnostic_item(match kind { - FunctionKind::TryFromFunction => sym::From, - FunctionKind::TryIntoMethod | FunctionKind::TryIntoFunction => sym::Into, + FunctionKind::TryFromFunction(_) => sym::From, + FunctionKind::TryIntoMethod | FunctionKind::TryIntoFunction(_) => sym::Into, }) // If `T: TryFrom<U>` and `T: From<U>` both exist, then that means that the `TryFrom` // _must_ be from the blanket impl and cannot have been manually implemented @@ -45,49 +99,37 @@ fn check<'tcx>( && implements_trait(cx, self_ty, from_into_trait, &[other_ty]) && let Some(other_ty) = other_ty.as_type() { + // Extend the span to include the unwrap/expect call: + // `foo.try_into().expect("..")` + // ^^^^^^^^^^^^^^^^^^^^^^^ + // + // `try_into().unwrap()` specifically can be trivially replaced with just `into()`, + // so that can be machine-applicable let parent_unwrap_call = get_parent_expr(cx, expr).and_then(|parent| { if let ExprKind::MethodCall(path, .., span) = parent.kind && let sym::unwrap | sym::expect = path.ident.name { - Some(span) + // include `.` before `unwrap`/`expect` + Some(span.with_lo(expr.span.hi())) } else { None } }); - let (source_ty, target_ty, sugg, span, applicability) = match kind { - FunctionKind::TryIntoMethod if let Some(unwrap_span) = parent_unwrap_call => { - // Extend the span to include the unwrap/expect call: - // `foo.try_into().expect("..")` - // ^^^^^^^^^^^^^^^^^^^^^^^ - // - // `try_into().unwrap()` specifically can be trivially replaced with just `into()`, - // so that can be machine-applicable - - ( - self_ty, - other_ty, - "into()", - primary_span.with_hi(unwrap_span.hi()), - Applicability::MachineApplicable, - ) - }, - FunctionKind::TryFromFunction => ( - other_ty, - self_ty, - "From::from", - primary_span, - Applicability::Unspecified, - ), - FunctionKind::TryIntoFunction => ( - self_ty, - other_ty, - "Into::into", - primary_span, - Applicability::Unspecified, - ), - FunctionKind::TryIntoMethod => (self_ty, other_ty, "into", primary_span, Applicability::Unspecified), + + // If there is an unwrap/expect call, extend the span to include the call + let span = if let Some(unwrap_call) = parent_unwrap_call { + primary_span.with_hi(unwrap_call.hi()) + } else { + primary_span + }; + + let (source_ty, target_ty) = match kind { + FunctionKind::TryIntoMethod | FunctionKind::TryIntoFunction(_) => (self_ty, other_ty), + FunctionKind::TryFromFunction(_) => (other_ty, self_ty), }; + let (applicability, sugg) = kind.appl_sugg(parent_unwrap_call, primary_span); + span_lint_and_then( cx, UNNECESSARY_FALLIBLE_CONVERSIONS, @@ -97,7 +139,7 @@ fn check<'tcx>( with_forced_trimmed_paths!({ diag.note(format!("converting `{source_ty}` to `{target_ty}` cannot fail")); }); - diag.span_suggestion(span, "use", sugg, applicability); + diag.multipart_suggestion("use", sugg, applicability); }, ); } @@ -125,13 +167,30 @@ pub(super) fn check_function(cx: &LateContext<'_>, expr: &Expr<'_>, callee: &Exp && let Some(item_def_id) = cx.qpath_res(qpath, callee.hir_id).opt_def_id() && let Some(trait_def_id) = cx.tcx.trait_of_item(item_def_id) { + let qpath_spans = match qpath { + QPath::Resolved(_, path) => { + if let [trait_seg, fn_seg] = path.segments { + Some(SpansKind::TraitFn { + trait_span: trait_seg.ident.span, + fn_span: fn_seg.ident.span, + }) + } else { + None + } + }, + QPath::TypeRelative(_, seg) => Some(SpansKind::Fn { + fn_span: seg.ident.span, + }), + QPath::LangItem(_, _) => unreachable!("`TryFrom` and `TryInto` are not lang items"), + }; + check( cx, expr, cx.typeck_results().node_args(callee.hir_id), match cx.tcx.get_diagnostic_name(trait_def_id) { - Some(sym::TryFrom) => FunctionKind::TryFromFunction, - Some(sym::TryInto) => FunctionKind::TryIntoFunction, + Some(sym::TryFrom) => FunctionKind::TryFromFunction(qpath_spans), + Some(sym::TryInto) => FunctionKind::TryIntoFunction(qpath_spans), _ => return, }, callee.span, diff --git a/clippy_lints/src/methods/unnecessary_sort_by.rs b/clippy_lints/src/methods/unnecessary_sort_by.rs index 6911da69b94..a92fe9e55a4 100644 --- a/clippy_lints/src/methods/unnecessary_sort_by.rs +++ b/clippy_lints/src/methods/unnecessary_sort_by.rs @@ -5,7 +5,8 @@ use clippy_utils::ty::implements_trait; use rustc_errors::Applicability; use rustc_hir::{Closure, Expr, ExprKind, Mutability, Param, Pat, PatKind, Path, PathSegment, QPath}; use rustc_lint::LateContext; -use rustc_middle::ty::{self, GenericArgKind}; +use rustc_middle::ty; +use rustc_middle::ty::GenericArgKind; use rustc_span::sym; use rustc_span::symbol::Ident; use std::iter; diff --git a/clippy_lints/src/methods/unnecessary_to_owned.rs b/clippy_lints/src/methods/unnecessary_to_owned.rs index 64fcd9f8f45..c234e4f9b11 100644 --- a/clippy_lints/src/methods/unnecessary_to_owned.rs +++ b/clippy_lints/src/methods/unnecessary_to_owned.rs @@ -3,7 +3,9 @@ use super::unnecessary_iter_cloned::{self, is_into_iter}; use clippy_config::msrvs::{self, Msrv}; use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::snippet_opt; -use clippy_utils::ty::{get_iterator_item_ty, implements_trait, is_copy, is_type_lang_item, peel_mid_ty_refs}; +use clippy_utils::ty::{ + get_iterator_item_ty, implements_trait, is_copy, is_type_diagnostic_item, is_type_lang_item, peel_mid_ty_refs, +}; use clippy_utils::visitors::find_all_ret_expressions; use clippy_utils::{fn_def_id, get_parent_expr, is_diag_item_method, is_diag_trait_item, return_ty}; use rustc_errors::Applicability; @@ -16,7 +18,8 @@ use rustc_lint::LateContext; use rustc_middle::mir::Mutability; use rustc_middle::ty::adjustment::{Adjust, Adjustment, OverloadedDeref}; use rustc_middle::ty::{ - self, ClauseKind, GenericArg, GenericArgKind, GenericArgsRef, ParamTy, ProjectionPredicate, TraitPredicate, Ty, + self, ClauseKind, GenericArg, GenericArgKind, GenericArgsRef, ImplPolarity, ParamTy, ProjectionPredicate, + TraitPredicate, Ty, }; use rustc_span::{sym, Symbol}; use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt as _; @@ -53,6 +56,8 @@ pub fn check<'tcx>( } check_other_call_arg(cx, expr, method_name, receiver); } + } else { + check_borrow_predicate(cx, expr); } } @@ -590,3 +595,92 @@ fn is_to_string_on_string_like<'a>( false } } + +fn is_a_std_map_type(cx: &LateContext<'_>, ty: Ty<'_>) -> bool { + is_type_diagnostic_item(cx, ty, sym::HashSet) + || is_type_diagnostic_item(cx, ty, sym::HashMap) + || is_type_diagnostic_item(cx, ty, sym::BTreeMap) + || is_type_diagnostic_item(cx, ty, sym::BTreeSet) +} + +fn is_str_and_string(cx: &LateContext<'_>, arg_ty: Ty<'_>, original_arg_ty: Ty<'_>) -> bool { + original_arg_ty.is_str() && is_type_lang_item(cx, arg_ty, LangItem::String) +} + +fn is_slice_and_vec(cx: &LateContext<'_>, arg_ty: Ty<'_>, original_arg_ty: Ty<'_>) -> bool { + (original_arg_ty.is_slice() || original_arg_ty.is_array() || original_arg_ty.is_array_slice()) + && is_type_diagnostic_item(cx, arg_ty, sym::Vec) +} + +// This function will check the following: +// 1. The argument is a non-mutable reference. +// 2. It calls `to_owned()`, `to_string()` or `to_vec()`. +// 3. That the method is called on `String` or on `Vec` (only types supported for the moment). +fn check_if_applicable_to_argument<'tcx>(cx: &LateContext<'tcx>, arg: &Expr<'tcx>) { + if let ExprKind::AddrOf(BorrowKind::Ref, Mutability::Not, expr) = arg.kind + && let ExprKind::MethodCall(method_path, caller, &[], _) = expr.kind + && let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) + && let method_name = method_path.ident.name.as_str() + && match method_name { + "to_owned" => cx.tcx.is_diagnostic_item(sym::to_owned_method, method_def_id), + "to_string" => cx.tcx.is_diagnostic_item(sym::to_string_method, method_def_id), + "to_vec" => cx + .tcx + .impl_of_method(method_def_id) + .filter(|&impl_did| cx.tcx.type_of(impl_did).instantiate_identity().is_slice()) + .is_some(), + _ => false, + } + && let original_arg_ty = cx.typeck_results().node_type(caller.hir_id).peel_refs() + && let arg_ty = cx.typeck_results().expr_ty(arg) + && let ty::Ref(_, arg_ty, Mutability::Not) = arg_ty.kind() + // FIXME: try to fix `can_change_type` to make it work in this case. + // && can_change_type(cx, caller, *arg_ty) + && let arg_ty = arg_ty.peel_refs() + // For now we limit this lint to `String` and `Vec`. + && (is_str_and_string(cx, arg_ty, original_arg_ty) || is_slice_and_vec(cx, arg_ty, original_arg_ty)) + && let Some(snippet) = snippet_opt(cx, caller.span) + { + span_lint_and_sugg( + cx, + UNNECESSARY_TO_OWNED, + arg.span, + &format!("unnecessary use of `{method_name}`"), + "replace it with", + if original_arg_ty.is_array() { + format!("{snippet}.as_slice()") + } else { + snippet + }, + Applicability::MaybeIncorrect, + ); + } +} + +// In std "map types", the getters all expect a `Borrow<Key>` generic argument. So in here, we +// check that: +// 1. This is a method with only one argument that doesn't come from a trait. +// 2. That it has `Borrow` in its generic predicates. +// 3. `Self` is a std "map type" (ie `HashSet`, `HashMap`, BTreeSet`, `BTreeMap`). +fn check_borrow_predicate<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) { + if let ExprKind::MethodCall(_, caller, &[arg], _) = expr.kind + && let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) + && cx.tcx.trait_of_item(method_def_id).is_none() + && let Some(borrow_id) = cx.tcx.get_diagnostic_item(sym::Borrow) + && cx.tcx.predicates_of(method_def_id).predicates.iter().any(|(pred, _)| { + if let ClauseKind::Trait(trait_pred) = pred.kind().skip_binder() + && trait_pred.polarity == ImplPolarity::Positive + && trait_pred.trait_ref.def_id == borrow_id + { + true + } else { + false + } + }) + && let caller_ty = cx.typeck_results().expr_ty(caller) + // For now we limit it to "map types". + && is_a_std_map_type(cx, caller_ty) + { + check_if_applicable_to_argument(cx, &arg); + } +} diff --git a/clippy_lints/src/min_ident_chars.rs b/clippy_lints/src/min_ident_chars.rs index 70cc43e266c..0016fb33517 100644 --- a/clippy_lints/src/min_ident_chars.rs +++ b/clippy_lints/src/min_ident_chars.rs @@ -2,8 +2,8 @@ use clippy_utils::diagnostics::span_lint; use clippy_utils::is_from_proc_macro; use rustc_data_structures::fx::FxHashSet; use rustc_hir::def::{DefKind, Res}; -use rustc_hir::intravisit::{walk_item, Visitor}; -use rustc_hir::{GenericParamKind, HirId, Item, ItemKind, ItemLocalId, Node, Pat, PatKind}; +use rustc_hir::intravisit::{walk_item, walk_trait_item, Visitor}; +use rustc_hir::{GenericParamKind, HirId, Item, ItemKind, ItemLocalId, Node, Pat, PatKind, TraitItem, UsePath}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; use rustc_session::impl_lint_pass; @@ -53,7 +53,7 @@ impl MinIdentChars { && str.len() <= self.min_ident_chars_threshold as usize && !str.starts_with('_') && !str.is_empty() - && self.allowed_idents_below_min_chars.get(&str.to_owned()).is_none() + && !self.allowed_idents_below_min_chars.contains(str) } } @@ -66,6 +66,14 @@ impl LateLintPass<'_> for MinIdentChars { walk_item(&mut IdentVisitor { conf: self, cx }, item); } + fn check_trait_item(&mut self, cx: &LateContext<'_>, item: &TraitItem<'_>) { + if self.min_ident_chars_threshold == 0 { + return; + } + + walk_trait_item(&mut IdentVisitor { conf: self, cx }, item); + } + // This is necessary as `Node::Pat`s are not visited in `visit_id`. :/ fn check_pat(&mut self, cx: &LateContext<'_>, pat: &Pat<'_>) { if let PatKind::Binding(_, _, ident, ..) = pat.kind @@ -105,11 +113,26 @@ impl Visitor<'_> for IdentVisitor<'_, '_> { let str = ident.as_str(); if conf.is_ident_too_short(cx, str, ident.span) { - if let Node::Item(item) = node - && let ItemKind::Use(..) = item.kind + // Check whether the node is part of a `use` statement. We don't want to emit a warning if the user + // has no control over the type. + let usenode = opt_as_use_node(node).or_else(|| { + cx.tcx + .hir() + .parent_iter(hir_id) + .find_map(|(_, node)| opt_as_use_node(node)) + }); + + // If the name of the identifier is the same as the one of the imported item, this means that we + // found a `use foo::bar`. We can early-return to not emit the warning. + // If however the identifier is different, this means it is an alias (`use foo::bar as baz`). In + // this case, we need to emit the warning for `baz`. + if let Some(imported_item_path) = usenode + && let Some(Res::Def(_, imported_item_defid)) = imported_item_path.res.first() + && cx.tcx.item_name(*imported_item_defid).as_str() == str { return; } + // `struct Awa<T>(T)` // ^ if let Node::PathSegment(path) = node { @@ -160,3 +183,16 @@ fn emit_min_ident_chars(conf: &MinIdentChars, cx: &impl LintContext, ident: &str }; span_lint(cx, MIN_IDENT_CHARS, span, &help); } + +/// Attempt to convert the node to an [`ItemKind::Use`] node. +/// +/// If it is, return the [`UsePath`] contained within. +fn opt_as_use_node(node: Node<'_>) -> Option<&'_ UsePath<'_>> { + if let Node::Item(item) = node + && let ItemKind::Use(path, _) = item.kind + { + Some(path) + } else { + None + } +} diff --git a/clippy_lints/src/mutable_debug_assertion.rs b/clippy_lints/src/mutable_debug_assertion.rs index 96cd81ecdf3..2d7ce7b52ae 100644 --- a/clippy_lints/src/mutable_debug_assertion.rs +++ b/clippy_lints/src/mutable_debug_assertion.rs @@ -96,10 +96,6 @@ impl<'a, 'tcx> Visitor<'tcx> for MutArgVisitor<'a, 'tcx> { self.found = true; return; }, - ExprKind::If(..) => { - self.found = true; - return; - }, ExprKind::Path(_) => { if let Some(adj) = self.cx.typeck_results().adjustments().get(expr.hir_id) { if adj diff --git a/clippy_lints/src/needless_bool.rs b/clippy_lints/src/needless_bool.rs index bf27adb45af..99917213bc3 100644 --- a/clippy_lints/src/needless_bool.rs +++ b/clippy_lints/src/needless_bool.rs @@ -139,7 +139,7 @@ fn condition_needs_parentheses(e: &Expr<'_>) -> bool { fn is_parent_stmt(cx: &LateContext<'_>, id: HirId) -> bool { matches!( get_parent_node(cx.tcx, id), - Some(Node::Stmt(..) | Node::Block(Block { stmts: &[], .. })) + Node::Stmt(..) | Node::Block(Block { stmts: &[], .. }) ) } diff --git a/clippy_lints/src/needless_pass_by_ref_mut.rs b/clippy_lints/src/needless_pass_by_ref_mut.rs index 710ecd745f8..f8b321c31a8 100644 --- a/clippy_lints/src/needless_pass_by_ref_mut.rs +++ b/clippy_lints/src/needless_pass_by_ref_mut.rs @@ -236,11 +236,10 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut<'tcx> { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { // #11182; do not lint if mutability is required elsewhere if let ExprKind::Path(..) = expr.kind - && let Some(parent) = get_parent_node(cx.tcx, expr.hir_id) && let ty::FnDef(def_id, _) = cx.typeck_results().expr_ty(expr).kind() && let Some(def_id) = def_id.as_local() { - if let Node::Expr(e) = parent + if let Node::Expr(e) = get_parent_node(cx.tcx, expr.hir_id) && let ExprKind::Call(call, _) = e.kind && call.hir_id == expr.hir_id { diff --git a/clippy_lints/src/new_without_default.rs b/clippy_lints/src/new_without_default.rs index 9de6ad42137..b3b8a5e9963 100644 --- a/clippy_lints/src/new_without_default.rs +++ b/clippy_lints/src/new_without_default.rs @@ -75,10 +75,6 @@ impl<'tcx> LateLintPass<'tcx> for NewWithoutDefault { if let hir::ImplItemKind::Fn(ref sig, _) = impl_item.kind { let name = impl_item.ident.name; let id = impl_item.owner_id; - if sig.header.constness == hir::Constness::Const { - // can't be implemented by default - return; - } if sig.header.unsafety == hir::Unsafety::Unsafe { // can't be implemented for unsafe new return; diff --git a/clippy_lints/src/no_effect.rs b/clippy_lints/src/no_effect.rs index 6a5555ca383..5014445843f 100644 --- a/clippy_lints/src/no_effect.rs +++ b/clippy_lints/src/no_effect.rs @@ -5,8 +5,8 @@ use clippy_utils::{any_parent_is_automatically_derived, get_parent_node, is_lint use rustc_errors::Applicability; use rustc_hir::def::{DefKind, Res}; use rustc_hir::{ - is_range_literal, BinOpKind, BlockCheckMode, Expr, ExprKind, HirId, HirIdMap, ItemKind, Node, PatKind, Stmt, - StmtKind, UnsafeSource, + is_range_literal, BinOpKind, BlockCheckMode, Expr, ExprKind, HirId, HirIdMap, ItemKind, LocalSource, Node, PatKind, + Stmt, StmtKind, UnsafeSource, }; use rustc_infer::infer::TyCtxtInferExt as _; use rustc_lint::{LateContext, LateLintPass, LintContext}; @@ -43,10 +43,6 @@ declare_clippy_lint! { /// executed. However, as they have no effect and shouldn't be used further on, all they /// do is make the code less readable. /// - /// ### Known problems - /// Further usage of this variable is not checked, which can lead to false positives if it is - /// used later in the code. - /// /// ### Example /// ```rust,ignore /// let _i_serve_no_purpose = 1; @@ -144,7 +140,7 @@ impl NoEffect { for parent in cx.tcx.hir().parent_iter(stmt.hir_id) { if let Node::Item(item) = parent.1 && let ItemKind::Fn(..) = item.kind - && let Some(Node::Block(block)) = get_parent_node(cx.tcx, stmt.hir_id) + && let Node::Block(block) = get_parent_node(cx.tcx, stmt.hir_id) && let [.., final_stmt] = block.stmts && final_stmt.hir_id == stmt.hir_id { @@ -180,6 +176,7 @@ impl NoEffect { } } else if let StmtKind::Local(local) = stmt.kind { if !is_lint_allowed(cx, NO_EFFECT_UNDERSCORE_BINDING, local.hir_id) + && !matches!(local.source, LocalSource::AsyncFn) && let Some(init) = local.init && local.els.is_none() && !local.pat.span.from_expansion() diff --git a/clippy_lints/src/non_canonical_impls.rs b/clippy_lints/src/non_canonical_impls.rs index 63050080ac6..35c43a5b649 100644 --- a/clippy_lints/src/non_canonical_impls.rs +++ b/clippy_lints/src/non_canonical_impls.rs @@ -112,7 +112,7 @@ declare_lint_pass!(NonCanonicalImpls => [NON_CANONICAL_CLONE_IMPL, NON_CANONICAL impl LateLintPass<'_> for NonCanonicalImpls { #[expect(clippy::too_many_lines)] fn check_impl_item(&mut self, cx: &LateContext<'_>, impl_item: &ImplItem<'_>) { - let Some(Node::Item(item)) = get_parent_node(cx.tcx, impl_item.hir_id()) else { + let Node::Item(item) = get_parent_node(cx.tcx, impl_item.hir_id()) else { return; }; let Some(trait_impl) = cx.tcx.impl_trait_ref(item.owner_id).map(EarlyBinder::skip_binder) else { diff --git a/clippy_lints/src/non_expressive_names.rs b/clippy_lints/src/non_expressive_names.rs index ba9230dab7a..b8c3c7fa65a 100644 --- a/clippy_lints/src/non_expressive_names.rs +++ b/clippy_lints/src/non_expressive_names.rs @@ -119,7 +119,6 @@ impl<'a, 'tcx> SimilarNamesLocalVisitor<'a, 'tcx> { // this list contains lists of names that are allowed to be similar // the assumption is that no name is ever contained in multiple lists. -#[rustfmt::skip] const ALLOWED_TO_BE_SIMILAR: &[&[&str]] = &[ &["parsed", "parser"], &["lhs", "rhs"], @@ -132,6 +131,14 @@ const ALLOWED_TO_BE_SIMILAR: &[&[&str]] = &[ &["iter", "item"], ]; +/// Characters that look visually similar +const SIMILAR_CHARS: &[(char, char)] = &[('l', 'i'), ('l', '1'), ('i', '1'), ('u', 'v')]; + +/// Return true if two characters are visually similar +fn chars_are_similar(a: char, b: char) -> bool { + a == b || SIMILAR_CHARS.contains(&(a, b)) || SIMILAR_CHARS.contains(&(b, a)) +} + struct SimilarNamesNameVisitor<'a, 'tcx, 'b>(&'b mut SimilarNamesLocalVisitor<'a, 'tcx>); impl<'a, 'tcx, 'b> Visitor<'tcx> for SimilarNamesNameVisitor<'a, 'tcx, 'b> { @@ -189,7 +196,6 @@ impl<'a, 'tcx, 'b> SimilarNamesNameVisitor<'a, 'tcx, 'b> { } } - #[expect(clippy::too_many_lines)] fn check_ident(&mut self, ident: Ident) { let interned_name = ident.name.as_str(); if interned_name.chars().any(char::is_uppercase) { @@ -219,71 +225,28 @@ impl<'a, 'tcx, 'b> SimilarNamesNameVisitor<'a, 'tcx, 'b> { if allowed_to_be_similar(interned_name, existing_name.exemptions) { continue; } - match existing_name.len.cmp(&count) { - Ordering::Greater => { - if existing_name.len - count != 1 - || levenstein_not_1(interned_name, existing_name.interned.as_str()) - { - continue; - } - }, - Ordering::Less => { - if count - existing_name.len != 1 - || levenstein_not_1(existing_name.interned.as_str(), interned_name) - { - continue; - } - }, - Ordering::Equal => { - let mut interned_chars = interned_name.chars(); - let interned_str = existing_name.interned.as_str(); - let mut existing_chars = interned_str.chars(); - let first_i = interned_chars.next().expect("we know we have at least one char"); - let first_e = existing_chars.next().expect("we know we have at least one char"); - let eq_or_numeric = |(a, b): (char, char)| a == b || a.is_numeric() && b.is_numeric(); - - if eq_or_numeric((first_i, first_e)) { - let last_i = interned_chars.next_back().expect("we know we have at least two chars"); - let last_e = existing_chars.next_back().expect("we know we have at least two chars"); - if eq_or_numeric((last_i, last_e)) { - if interned_chars - .zip(existing_chars) - .filter(|&ie| !eq_or_numeric(ie)) - .count() - != 1 - { - continue; - } - } else { - let second_last_i = interned_chars - .next_back() - .expect("we know we have at least three chars"); - let second_last_e = existing_chars - .next_back() - .expect("we know we have at least three chars"); - if !eq_or_numeric((second_last_i, second_last_e)) - || second_last_i == '_' - || !interned_chars.zip(existing_chars).all(eq_or_numeric) - { - // allowed similarity foo_x, foo_y - // or too many chars differ (foo_x, boo_y) or (foox, booy) - continue; - } - } - } else { - let second_i = interned_chars.next().expect("we know we have at least two chars"); - let second_e = existing_chars.next().expect("we know we have at least two chars"); - if !eq_or_numeric((second_i, second_e)) - || second_i == '_' - || !interned_chars.zip(existing_chars).all(eq_or_numeric) - { - // allowed similarity x_foo, y_foo - // or too many chars differ (x_foo, y_boo) or (xfoo, yboo) - continue; - } - } - }, + + let existing_str = existing_name.interned.as_str(); + + // The first char being different is usually enough to set identifiers apart, as long + // as the characters aren't too similar. + if !chars_are_similar( + interned_name.chars().next().expect("len >= 1"), + existing_str.chars().next().expect("len >= 1"), + ) { + continue; } + + let dissimilar = match existing_name.len.cmp(&count) { + Ordering::Greater => existing_name.len - count != 1 || levenstein_not_1(interned_name, existing_str), + Ordering::Less => count - existing_name.len != 1 || levenstein_not_1(existing_str, interned_name), + Ordering::Equal => Self::equal_length_strs_not_similar(interned_name, existing_str), + }; + + if dissimilar { + continue; + } + span_lint_and_then( self.0.cx, SIMILAR_NAMES, @@ -302,6 +265,57 @@ impl<'a, 'tcx, 'b> SimilarNamesNameVisitor<'a, 'tcx, 'b> { len: count, }); } + + fn equal_length_strs_not_similar(interned_name: &str, existing_name: &str) -> bool { + let mut interned_chars = interned_name.chars(); + let mut existing_chars = existing_name.chars(); + let first_i = interned_chars.next().expect("we know we have at least one char"); + let first_e = existing_chars.next().expect("we know we have at least one char"); + let eq_or_numeric = |(a, b): (char, char)| a == b || a.is_numeric() && b.is_numeric(); + + if eq_or_numeric((first_i, first_e)) { + let last_i = interned_chars.next_back().expect("we know we have at least two chars"); + let last_e = existing_chars.next_back().expect("we know we have at least two chars"); + if eq_or_numeric((last_i, last_e)) { + if interned_chars + .zip(existing_chars) + .filter(|&ie| !eq_or_numeric(ie)) + .count() + != 1 + { + return true; + } + } else { + let second_last_i = interned_chars + .next_back() + .expect("we know we have at least three chars"); + let second_last_e = existing_chars + .next_back() + .expect("we know we have at least three chars"); + if !eq_or_numeric((second_last_i, second_last_e)) + || second_last_i == '_' + || !interned_chars.zip(existing_chars).all(eq_or_numeric) + { + // allowed similarity foo_x, foo_y + // or too many chars differ (foo_x, boo_y) or (foox, booy) + return true; + } + } + } else { + let second_i = interned_chars.next().expect("we know we have at least two chars"); + let second_e = existing_chars.next().expect("we know we have at least two chars"); + if !eq_or_numeric((second_i, second_e)) + || second_i == '_' + || !interned_chars.zip(existing_chars).all(eq_or_numeric) + { + // allowed similarity x_foo, y_foo + // or too many chars differ (x_foo, y_boo) or (xfoo, yboo) + return true; + } + } + + false + } } impl<'a, 'b> SimilarNamesLocalVisitor<'a, 'b> { diff --git a/clippy_lints/src/only_used_in_recursion.rs b/clippy_lints/src/only_used_in_recursion.rs index a568392ecc4..9fa0edb10c5 100644 --- a/clippy_lints/src/only_used_in_recursion.rs +++ b/clippy_lints/src/only_used_in_recursion.rs @@ -228,23 +228,23 @@ impl<'tcx> LateLintPass<'tcx> for OnlyUsedInRecursion { // `skip_params` is either `0` or `1` to skip the `self` parameter in trait functions. // It can't be renamed, and it can't be removed without removing it from multiple functions. let (fn_id, fn_kind, skip_params) = match get_parent_node(cx.tcx, body.value.hir_id) { - Some(Node::Item(i)) => (i.owner_id.to_def_id(), FnKind::Fn, 0), - Some(Node::TraitItem(&TraitItem { + Node::Item(i) => (i.owner_id.to_def_id(), FnKind::Fn, 0), + Node::TraitItem(&TraitItem { kind: TraitItemKind::Fn(ref sig, _), owner_id, .. - })) => ( + }) => ( owner_id.to_def_id(), FnKind::TraitFn, usize::from(sig.decl.implicit_self.has_implicit_self()), ), - Some(Node::ImplItem(&ImplItem { + Node::ImplItem(&ImplItem { kind: ImplItemKind::Fn(ref sig, _), owner_id, .. - })) => { + }) => { #[allow(trivial_casts)] - if let Some(Node::Item(item)) = get_parent_node(cx.tcx, owner_id.into()) + if let Node::Item(item) = get_parent_node(cx.tcx, owner_id.into()) && let Some(trait_ref) = cx .tcx .impl_trait_ref(item.owner_id) diff --git a/clippy_lints/src/operators/double_comparison.rs b/clippy_lints/src/operators/double_comparison.rs index d48e8286f2c..d72a2fc3b1a 100644 --- a/clippy_lints/src/operators/double_comparison.rs +++ b/clippy_lints/src/operators/double_comparison.rs @@ -8,7 +8,6 @@ use rustc_span::Span; use super::DOUBLE_COMPARISONS; -#[expect(clippy::similar_names)] pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, op: BinOpKind, lhs: &'tcx Expr<'_>, rhs: &'tcx Expr<'_>, span: Span) { let (lkind, llhs, lrhs, rkind, rlhs, rrhs) = match (&lhs.kind, &rhs.kind) { (ExprKind::Binary(lb, llhs, lrhs), ExprKind::Binary(rb, rlhs, rrhs)) => { diff --git a/clippy_lints/src/operators/op_ref.rs b/clippy_lints/src/operators/op_ref.rs index 7d8aa3f56fe..c2b27c9b229 100644 --- a/clippy_lints/src/operators/op_ref.rs +++ b/clippy_lints/src/operators/op_ref.rs @@ -11,7 +11,7 @@ use rustc_middle::ty::{self, Ty}; use super::OP_REF; -#[expect(clippy::similar_names, clippy::too_many_lines)] +#[expect(clippy::too_many_lines)] pub(crate) fn check<'tcx>( cx: &LateContext<'tcx>, e: &'tcx Expr<'_>, diff --git a/clippy_lints/src/pub_underscore_fields.rs b/clippy_lints/src/pub_underscore_fields.rs index 88b5a6cfe2a..d20d4a605a2 100644 --- a/clippy_lints/src/pub_underscore_fields.rs +++ b/clippy_lints/src/pub_underscore_fields.rs @@ -1,6 +1,6 @@ use clippy_config::types::PubUnderscoreFieldsBehaviour; use clippy_utils::attrs::is_doc_hidden; -use clippy_utils::diagnostics::span_lint_and_help; +use clippy_utils::diagnostics::span_lint_hir_and_then; use clippy_utils::is_path_lang_item; use rustc_hir::{FieldDef, Item, ItemKind, LangItem}; use rustc_lint::{LateContext, LateLintPass}; @@ -69,13 +69,15 @@ impl<'tcx> LateLintPass<'tcx> for PubUnderscoreFields { // We ignore fields that are `PhantomData`. && !is_path_lang_item(cx, field.ty, LangItem::PhantomData) { - span_lint_and_help( + span_lint_hir_and_then( cx, PUB_UNDERSCORE_FIELDS, + field.hir_id, field.vis_span.to(field.ident.span), "field marked as public but also inferred as unused because it's prefixed with `_`", - None, - "consider removing the underscore, or making the field private", + |diag| { + diag.help("consider removing the underscore, or making the field private"); + }, ); } } diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index bd3128a0f97..b5da27d7993 100644 --- a/clippy_lints/src/question_mark.rs +++ b/clippy_lints/src/question_mark.rs @@ -289,7 +289,7 @@ impl QuestionMark { { let mut applicability = Applicability::MachineApplicable; let receiver_str = snippet_with_applicability(cx, let_expr.span, "..", &mut applicability); - let requires_semi = matches!(get_parent_node(cx.tcx, expr.hir_id), Some(Node::Stmt(_))); + let requires_semi = matches!(get_parent_node(cx.tcx, expr.hir_id), Node::Stmt(_)); let sugg = format!( "{receiver_str}{}?{}", if by_ref == ByRef::Yes { ".as_ref()" } else { "" }, diff --git a/clippy_lints/src/redundant_closure_call.rs b/clippy_lints/src/redundant_closure_call.rs index 915da18aafe..8d5b3ca21dc 100644 --- a/clippy_lints/src/redundant_closure_call.rs +++ b/clippy_lints/src/redundant_closure_call.rs @@ -1,7 +1,7 @@ use crate::rustc_lint::LintContext; use clippy_utils::diagnostics::{span_lint, span_lint_and_then}; -use clippy_utils::get_parent_expr; use clippy_utils::sugg::Sugg; +use clippy_utils::{get_parent_expr, get_parent_node}; use hir::Param; use rustc_errors::Applicability; use rustc_hir as hir; @@ -200,11 +200,11 @@ impl<'tcx> LateLintPass<'tcx> for RedundantClosureCall { hint = hint.asyncify(); } - let is_in_fn_call_arg = - clippy_utils::get_parent_node(cx.tcx, expr.hir_id).is_some_and(|x| match x { - Node::Expr(expr) => matches!(expr.kind, hir::ExprKind::Call(_, _)), - _ => false, - }); + let is_in_fn_call_arg = if let Node::Expr(expr) = get_parent_node(cx.tcx, expr.hir_id) { + matches!(expr.kind, hir::ExprKind::Call(_, _)) + } else { + false + }; // avoid clippy::double_parens if !is_in_fn_call_arg { diff --git a/clippy_lints/src/same_name_method.rs b/clippy_lints/src/same_name_method.rs index 07806b182f2..7e27f70bcf9 100644 --- a/clippy_lints/src/same_name_method.rs +++ b/clippy_lints/src/same_name_method.rs @@ -47,7 +47,6 @@ struct ExistingName { } impl<'tcx> LateLintPass<'tcx> for SameNameMethod { - #[expect(clippy::too_many_lines)] fn check_crate_post(&mut self, cx: &LateContext<'tcx>) { let mut map = FxHashMap::<Res, ExistingName>::default(); @@ -75,24 +74,23 @@ impl<'tcx> LateLintPass<'tcx> for SameNameMethod { match of_trait { Some(trait_ref) => { - let mut methods_in_trait: BTreeSet<Symbol> = - if let Node::TraitRef(TraitRef { path, .. }) = - cx.tcx.hir_node(trait_ref.hir_ref_id) - && let Res::Def(DefKind::Trait, did) = path.res - { - // FIXME: if - // `rustc_middle::ty::assoc::AssocItems::items` is public, - // we can iterate its keys instead of `in_definition_order`, - // which's more efficient - cx.tcx - .associated_items(did) - .in_definition_order() - .filter(|assoc_item| matches!(assoc_item.kind, AssocKind::Fn)) - .map(|assoc_item| assoc_item.name) - .collect() - } else { - BTreeSet::new() - }; + let mut methods_in_trait: BTreeSet<Symbol> = if let Node::TraitRef(TraitRef { path, .. }) = + cx.tcx.hir_node(trait_ref.hir_ref_id) + && let Res::Def(DefKind::Trait, did) = path.res + { + // FIXME: if + // `rustc_middle::ty::assoc::AssocItems::items` is public, + // we can iterate its keys instead of `in_definition_order`, + // which's more efficient + cx.tcx + .associated_items(did) + .in_definition_order() + .filter(|assoc_item| matches!(assoc_item.kind, AssocKind::Fn)) + .map(|assoc_item| assoc_item.name) + .collect() + } else { + BTreeSet::new() + }; let mut check_trait_method = |method_name: Symbol, trait_method_span: Span| { if let Some((impl_span, hir_id)) = existing_name.impl_methods.get(&method_name) { diff --git a/clippy_lints/src/strlen_on_c_strings.rs b/clippy_lints/src/strlen_on_c_strings.rs index 8cf4715eeb8..8cb59caa573 100644 --- a/clippy_lints/src/strlen_on_c_strings.rs +++ b/clippy_lints/src/strlen_on_c_strings.rs @@ -51,11 +51,11 @@ impl<'tcx> LateLintPass<'tcx> for StrlenOnCStrings { { let ctxt = expr.span.ctxt(); let span = match get_parent_node(cx.tcx, expr.hir_id) { - Some(Node::Block(&Block { + Node::Block(&Block { rules: BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided), span, .. - })) if span.ctxt() == ctxt && !is_expr_unsafe(cx, self_arg) => span, + }) if span.ctxt() == ctxt && !is_expr_unsafe(cx, self_arg) => span, _ => expr.span, }; diff --git a/clippy_lints/src/to_string_trait_impl.rs b/clippy_lints/src/to_string_trait_impl.rs index e1cea99085f..59ae185c9de 100644 --- a/clippy_lints/src/to_string_trait_impl.rs +++ b/clippy_lints/src/to_string_trait_impl.rs @@ -1,4 +1,5 @@ use clippy_utils::diagnostics::span_lint_and_help; +use clippy_utils::ty::implements_trait; use rustc_hir::{Impl, Item, ItemKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::declare_lint_pass; @@ -53,6 +54,8 @@ impl<'tcx> LateLintPass<'tcx> for ToStringTraitImpl { }) = it.kind && let Some(trait_did) = trait_ref.trait_def_id() && cx.tcx.is_diagnostic_item(sym::ToString, trait_did) + && let Some(display_did) = cx.tcx.get_diagnostic_item(sym::Display) + && !implements_trait(cx, cx.tcx.type_of(it.owner_id).instantiate_identity(), display_did, &[]) { span_lint_and_help( cx, diff --git a/clippy_lints/src/tuple_array_conversions.rs b/clippy_lints/src/tuple_array_conversions.rs index c11504cd2d4..0d84a9ab395 100644 --- a/clippy_lints/src/tuple_array_conversions.rs +++ b/clippy_lints/src/tuple_array_conversions.rs @@ -153,10 +153,7 @@ fn all_bindings_are_for_conv<'tcx>( let Some(locals) = locals.iter().map(|e| path_to_local(e)).collect::<Option<Vec<_>>>() else { return false; }; - let local_parents = locals - .iter() - .map(|l| cx.tcx.parent_hir_node(*l)) - .collect::<Vec<_>>(); + let local_parents = locals.iter().map(|l| cx.tcx.parent_hir_node(*l)).collect::<Vec<_>>(); local_parents .iter() diff --git a/clippy_lints/src/undocumented_unsafe_blocks.rs b/clippy_lints/src/undocumented_unsafe_blocks.rs index add4b3e5637..86882cc374b 100644 --- a/clippy_lints/src/undocumented_unsafe_blocks.rs +++ b/clippy_lints/src/undocumented_unsafe_blocks.rs @@ -340,47 +340,43 @@ fn block_parents_have_safety_comment( cx: &LateContext<'_>, id: hir::HirId, ) -> bool { - if let Some(node) = get_parent_node(cx.tcx, id) { - let (span, hir_id) = match node { - Node::Expr(expr) => match get_parent_node(cx.tcx, expr.hir_id) { - Some(Node::Local(hir::Local { span, hir_id, .. })) => (*span, *hir_id), - Some(Node::Item(hir::Item { - kind: hir::ItemKind::Const(..) | ItemKind::Static(..), - span, - owner_id, - .. - })) => (*span, cx.tcx.local_def_id_to_hir_id(owner_id.def_id)), - _ => { - if is_branchy(expr) { - return false; - } - (expr.span, expr.hir_id) - }, - }, - Node::Stmt(hir::Stmt { - kind: - hir::StmtKind::Local(hir::Local { span, hir_id, .. }) - | hir::StmtKind::Expr(hir::Expr { span, hir_id, .. }) - | hir::StmtKind::Semi(hir::Expr { span, hir_id, .. }), - .. - }) - | Node::Local(hir::Local { span, hir_id, .. }) => (*span, *hir_id), + let (span, hir_id) = match get_parent_node(cx.tcx, id) { + Node::Expr(expr) => match get_parent_node(cx.tcx, expr.hir_id) { + Node::Local(hir::Local { span, hir_id, .. }) => (*span, *hir_id), Node::Item(hir::Item { kind: hir::ItemKind::Const(..) | ItemKind::Static(..), span, owner_id, .. }) => (*span, cx.tcx.local_def_id_to_hir_id(owner_id.def_id)), - _ => return false, - }; - // if unsafe block is part of a let/const/static statement, - // and accept_comment_above_statement is set to true - // we accept the safety comment in the line the precedes this statement. - accept_comment_above_statement - && span_with_attrs_has_safety_comment(cx, span, hir_id, accept_comment_above_attributes) - } else { - false - } + _ => { + if is_branchy(expr) { + return false; + } + (expr.span, expr.hir_id) + }, + }, + Node::Stmt(hir::Stmt { + kind: + hir::StmtKind::Local(hir::Local { span, hir_id, .. }) + | hir::StmtKind::Expr(hir::Expr { span, hir_id, .. }) + | hir::StmtKind::Semi(hir::Expr { span, hir_id, .. }), + .. + }) + | Node::Local(hir::Local { span, hir_id, .. }) => (*span, *hir_id), + Node::Item(hir::Item { + kind: hir::ItemKind::Const(..) | ItemKind::Static(..), + span, + owner_id, + .. + }) => (*span, cx.tcx.local_def_id_to_hir_id(owner_id.def_id)), + _ => return false, + }; + // if unsafe block is part of a let/const/static statement, + // and accept_comment_above_statement is set to true + // we accept the safety comment in the line the precedes this statement. + accept_comment_above_statement + && span_with_attrs_has_safety_comment(cx, span, hir_id, accept_comment_above_attributes) } /// Extends `span` to also include its attributes, then checks if that span has a safety comment. @@ -449,53 +445,49 @@ fn item_has_safety_comment(cx: &LateContext<'_>, item: &hir::Item<'_>) -> HasSaf if item.span.ctxt() != SyntaxContext::root() { return HasSafetyComment::No; } - if let Some(parent_node) = get_parent_node(cx.tcx, item.hir_id()) { - let comment_start = match parent_node { - Node::Crate(parent_mod) => { - comment_start_before_item_in_mod(cx, parent_mod, parent_mod.spans.inner_span, item) - }, - Node::Item(parent_item) => { - if let ItemKind::Mod(parent_mod) = &parent_item.kind { - comment_start_before_item_in_mod(cx, parent_mod, parent_item.span, item) - } else { - // Doesn't support impls in this position. Pretend a comment was found. - return HasSafetyComment::Maybe; - } - }, - Node::Stmt(stmt) => { - if let Some(Node::Block(block)) = get_parent_node(cx.tcx, stmt.hir_id) { - walk_span_to_context(block.span, SyntaxContext::root()).map(Span::lo) - } else { - // Problem getting the parent node. Pretend a comment was found. - return HasSafetyComment::Maybe; - } - }, - _ => { + let comment_start = match get_parent_node(cx.tcx, item.hir_id()) { + Node::Crate(parent_mod) => comment_start_before_item_in_mod(cx, parent_mod, parent_mod.spans.inner_span, item), + Node::Item(parent_item) => { + if let ItemKind::Mod(parent_mod) = &parent_item.kind { + comment_start_before_item_in_mod(cx, parent_mod, parent_item.span, item) + } else { // Doesn't support impls in this position. Pretend a comment was found. return HasSafetyComment::Maybe; - }, - }; - - let source_map = cx.sess().source_map(); - if let Some(comment_start) = comment_start - && let Ok(unsafe_line) = source_map.lookup_line(item.span.lo()) - && let Ok(comment_start_line) = source_map.lookup_line(comment_start) - && Lrc::ptr_eq(&unsafe_line.sf, &comment_start_line.sf) - && let Some(src) = unsafe_line.sf.src.as_deref() - { - return if comment_start_line.line >= unsafe_line.line { - HasSafetyComment::No + } + }, + Node::Stmt(stmt) => { + if let Node::Block(block) = get_parent_node(cx.tcx, stmt.hir_id) { + walk_span_to_context(block.span, SyntaxContext::root()).map(Span::lo) } else { - match text_has_safety_comment( - src, - &unsafe_line.sf.lines()[comment_start_line.line + 1..=unsafe_line.line], - unsafe_line.sf.start_pos, - ) { - Some(b) => HasSafetyComment::Yes(b), - None => HasSafetyComment::No, - } - }; - } + // Problem getting the parent node. Pretend a comment was found. + return HasSafetyComment::Maybe; + } + }, + _ => { + // Doesn't support impls in this position. Pretend a comment was found. + return HasSafetyComment::Maybe; + }, + }; + + let source_map = cx.sess().source_map(); + if let Some(comment_start) = comment_start + && let Ok(unsafe_line) = source_map.lookup_line(item.span.lo()) + && let Ok(comment_start_line) = source_map.lookup_line(comment_start) + && Lrc::ptr_eq(&unsafe_line.sf, &comment_start_line.sf) + && let Some(src) = unsafe_line.sf.src.as_deref() + { + return if comment_start_line.line >= unsafe_line.line { + HasSafetyComment::No + } else { + match text_has_safety_comment( + src, + &unsafe_line.sf.lines()[comment_start_line.line + 1..=unsafe_line.line], + unsafe_line.sf.start_pos, + ) { + Some(b) => HasSafetyComment::Yes(b), + None => HasSafetyComment::No, + } + }; } HasSafetyComment::Maybe } @@ -512,32 +504,30 @@ fn stmt_has_safety_comment(cx: &LateContext<'_>, span: Span, hir_id: HirId) -> H return HasSafetyComment::No; } - if let Some(parent_node) = get_parent_node(cx.tcx, hir_id) { - let comment_start = match parent_node { - Node::Block(block) => walk_span_to_context(block.span, SyntaxContext::root()).map(Span::lo), - _ => return HasSafetyComment::Maybe, - }; + let comment_start = match get_parent_node(cx.tcx, hir_id) { + Node::Block(block) => walk_span_to_context(block.span, SyntaxContext::root()).map(Span::lo), + _ => return HasSafetyComment::Maybe, + }; - let source_map = cx.sess().source_map(); - if let Some(comment_start) = comment_start - && let Ok(unsafe_line) = source_map.lookup_line(span.lo()) - && let Ok(comment_start_line) = source_map.lookup_line(comment_start) - && Lrc::ptr_eq(&unsafe_line.sf, &comment_start_line.sf) - && let Some(src) = unsafe_line.sf.src.as_deref() - { - return if comment_start_line.line >= unsafe_line.line { - HasSafetyComment::No - } else { - match text_has_safety_comment( - src, - &unsafe_line.sf.lines()[comment_start_line.line + 1..=unsafe_line.line], - unsafe_line.sf.start_pos, - ) { - Some(b) => HasSafetyComment::Yes(b), - None => HasSafetyComment::No, - } - }; - } + let source_map = cx.sess().source_map(); + if let Some(comment_start) = comment_start + && let Ok(unsafe_line) = source_map.lookup_line(span.lo()) + && let Ok(comment_start_line) = source_map.lookup_line(comment_start) + && Lrc::ptr_eq(&unsafe_line.sf, &comment_start_line.sf) + && let Some(src) = unsafe_line.sf.src.as_deref() + { + return if comment_start_line.line >= unsafe_line.line { + HasSafetyComment::No + } else { + match text_has_safety_comment( + src, + &unsafe_line.sf.lines()[comment_start_line.line + 1..=unsafe_line.line], + unsafe_line.sf.start_pos, + ) { + Some(b) => HasSafetyComment::Yes(b), + None => HasSafetyComment::No, + } + }; } HasSafetyComment::Maybe } diff --git a/clippy_lints/src/unit_types/let_unit_value.rs b/clippy_lints/src/unit_types/let_unit_value.rs index ed0958197f8..1b10169a3f2 100644 --- a/clippy_lints/src/unit_types/let_unit_value.rs +++ b/clippy_lints/src/unit_types/let_unit_value.rs @@ -103,7 +103,7 @@ fn expr_needs_inferred_result<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) - return false; } while let Some(id) = locals_to_check.pop() { - if let Some(Node::Local(l)) = get_parent_node(cx.tcx, id) { + if let Node::Local(l) = get_parent_node(cx.tcx, id) { if !l.ty.map_or(true, |ty| matches!(ty.kind, TyKind::Infer)) { return false; } diff --git a/clippy_lints/src/utils/internal_lints/metadata_collector.rs b/clippy_lints/src/utils/internal_lints/metadata_collector.rs index 349e0e3e077..6fceb5656a6 100644 --- a/clippy_lints/src/utils/internal_lints/metadata_collector.rs +++ b/clippy_lints/src/utils/internal_lints/metadata_collector.rs @@ -1005,8 +1005,6 @@ fn get_parent_local<'hir>(cx: &LateContext<'hir>, expr: &'hir hir::Expr<'hir>) - } fn get_parent_local_hir_id<'hir>(cx: &LateContext<'hir>, hir_id: hir::HirId) -> Option<&'hir hir::Local<'hir>> { - let map = cx.tcx.hir(); - match cx.tcx.parent_hir_node(hir_id) { hir::Node::Local(local) => Some(local), hir::Node::Pat(pattern) => get_parent_local_hir_id(cx, pattern.hir_id), |