diff options
| author | Philipp Krones <hello@philkrones.com> | 2024-12-26 15:01:07 +0100 |
|---|---|---|
| committer | Philipp Krones <hello@philkrones.com> | 2024-12-26 15:15:54 +0100 |
| commit | 1cc50519d103c84ec68f466af17c728e4584087e (patch) | |
| tree | d06a6f44149ab7f0ce6b7064ea64d7a5bee6c2d7 /clippy_lints/src | |
| parent | b5fe6ec47b8bc543481bb6efe4fc2b2daaee3d6b (diff) | |
Merge commit '609cd310be44677ae31d452a17b0f8207e1abfe1' into clippy-subtree-update
Diffstat (limited to 'clippy_lints/src')
65 files changed, 1188 insertions, 402 deletions
diff --git a/clippy_lints/src/arbitrary_source_item_ordering.rs b/clippy_lints/src/arbitrary_source_item_ordering.rs index 8719f61a890..cf33e1444e4 100644 --- a/clippy_lints/src/arbitrary_source_item_ordering.rs +++ b/clippy_lints/src/arbitrary_source_item_ordering.rs @@ -126,7 +126,7 @@ declare_clippy_lint! { /// /// [cargo-pgo]: https://github.com/Kobzol/cargo-pgo/blob/main/README.md /// - #[clippy::version = "1.82.0"] + #[clippy::version = "1.84.0"] pub ARBITRARY_SOURCE_ITEM_ORDERING, restriction, "arbitrary source item ordering" diff --git a/clippy_lints/src/attrs/mod.rs b/clippy_lints/src/attrs/mod.rs index a9766597d50..92efd1a4ddc 100644 --- a/clippy_lints/src/attrs/mod.rs +++ b/clippy_lints/src/attrs/mod.rs @@ -7,6 +7,7 @@ mod duplicated_attributes; mod inline_always; mod mixed_attributes_style; mod non_minimal_cfg; +mod repr_attributes; mod should_panic_without_expect; mod unnecessary_clippy_cfg; mod useless_attribute; @@ -274,6 +275,44 @@ declare_clippy_lint! { declare_clippy_lint! { /// ### What it does + /// Checks for items with `#[repr(packed)]`-attribute without ABI qualification + /// + /// ### Why is this bad? + /// Without qualification, `repr(packed)` implies `repr(Rust)`. The Rust-ABI is inherently unstable. + /// While this is fine as long as the type is accessed correctly within Rust-code, most uses + /// of `#[repr(packed)]` involve FFI and/or data structures specified by network-protocols or + /// other external specifications. In such situations, the unstable Rust-ABI implied in + /// `#[repr(packed)]` may lead to future bugs should the Rust-ABI change. + /// + /// In case you are relying on a well defined and stable memory layout, qualify the type's + /// representation using the `C`-ABI. Otherwise, if the type in question is only ever + /// accessed from Rust-code according to Rust's rules, use the `Rust`-ABI explicitly. + /// + /// ### Example + /// ```no_run + /// #[repr(packed)] + /// struct NetworkPacketHeader { + /// header_length: u8, + /// header_version: u16 + /// } + /// ``` + /// + /// Use instead: + /// ```no_run + /// #[repr(C, packed)] + /// struct NetworkPacketHeader { + /// header_length: u8, + /// header_version: u16 + /// } + /// ``` + #[clippy::version = "1.84.0"] + pub REPR_PACKED_WITHOUT_ABI, + suspicious, + "ensures that `repr(packed)` always comes with a qualified ABI" +} + +declare_clippy_lint! { + /// ### What it does /// Checks for `any` and `all` combinators in `cfg` with only one condition. /// /// ### Why is this bad? @@ -415,6 +454,7 @@ pub struct Attributes { impl_lint_pass!(Attributes => [ INLINE_ALWAYS, + REPR_PACKED_WITHOUT_ABI, ]); impl Attributes { @@ -431,6 +471,7 @@ impl<'tcx> LateLintPass<'tcx> for Attributes { if is_relevant_item(cx, item) { inline_always::check(cx, item.span, item.ident.name, attrs); } + repr_attributes::check(cx, item.span, attrs, &self.msrv); } fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx ImplItem<'_>) { diff --git a/clippy_lints/src/attrs/repr_attributes.rs b/clippy_lints/src/attrs/repr_attributes.rs new file mode 100644 index 00000000000..6d1ab46aa0c --- /dev/null +++ b/clippy_lints/src/attrs/repr_attributes.rs @@ -0,0 +1,43 @@ +use rustc_hir::Attribute; +use rustc_lint::LateContext; +use rustc_span::{Span, sym}; + +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::msrvs; + +use super::REPR_PACKED_WITHOUT_ABI; + +pub(super) fn check(cx: &LateContext<'_>, item_span: Span, attrs: &[Attribute], msrv: &msrvs::Msrv) { + if msrv.meets(msrvs::REPR_RUST) { + check_packed(cx, item_span, attrs); + } +} + +fn check_packed(cx: &LateContext<'_>, item_span: Span, attrs: &[Attribute]) { + if let Some(items) = attrs.iter().find_map(|attr| { + if attr.ident().is_some_and(|ident| matches!(ident.name, sym::repr)) { + attr.meta_item_list() + } else { + None + } + }) && let Some(packed) = items + .iter() + .find(|item| item.ident().is_some_and(|ident| matches!(ident.name, sym::packed))) + && !items.iter().any(|item| { + item.ident() + .is_some_and(|ident| matches!(ident.name, sym::C | sym::Rust)) + }) + { + span_lint_and_then( + cx, + REPR_PACKED_WITHOUT_ABI, + item_span, + "item uses `packed` representation without ABI-qualification", + |diag| { + diag.warn("unqualified `#[repr(packed)]` defaults to `#[repr(Rust, packed)]`, which has no stable ABI") + .help("qualify the desired ABI explicity via `#[repr(C, packed)]` or `#[repr(Rust, packed)]`") + .span_label(packed.span(), "`packed` representation set here"); + }, + ); + } +} diff --git a/clippy_lints/src/attrs/useless_attribute.rs b/clippy_lints/src/attrs/useless_attribute.rs index e21853598c3..e7158a6a6b6 100644 --- a/clippy_lints/src/attrs/useless_attribute.rs +++ b/clippy_lints/src/attrs/useless_attribute.rs @@ -1,8 +1,8 @@ use super::USELESS_ATTRIBUTE; -use super::utils::{extract_clippy_lint, is_lint_level, is_word}; +use super::utils::{is_lint_level, is_word, namespace_and_lint}; use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::source::{SpanRangeExt, first_line_of_span}; -use rustc_ast::{Attribute, Item, ItemKind, MetaItemInner}; +use rustc_ast::{Attribute, Item, ItemKind}; use rustc_errors::Applicability; use rustc_lint::{EarlyContext, LintContext}; use rustc_middle::lint::in_external_macro; @@ -20,11 +20,13 @@ pub(super) fn check(cx: &EarlyContext<'_>, item: &Item, attrs: &[Attribute]) { for lint in lint_list { match item.kind { ItemKind::Use(..) => { - if let MetaItemInner::MetaItem(meta_item) = lint - && meta_item.is_word() - && let Some(ident) = meta_item.ident() + let (namespace @ (Some(sym::clippy) | None), Some(name)) = namespace_and_lint(lint) else { + return; + }; + + if namespace.is_none() && matches!( - ident.name.as_str(), + name.as_str(), "ambiguous_glob_reexports" | "dead_code" | "deprecated" @@ -39,9 +41,9 @@ pub(super) fn check(cx: &EarlyContext<'_>, item: &Item, attrs: &[Attribute]) { return; } - if extract_clippy_lint(lint).is_some_and(|symbol| { - matches!( - symbol.as_str(), + if namespace == Some(sym::clippy) + && matches!( + name.as_str(), "wildcard_imports" | "enum_glob_use" | "redundant_pub_crate" @@ -52,7 +54,7 @@ pub(super) fn check(cx: &EarlyContext<'_>, item: &Item, attrs: &[Attribute]) { | "disallowed_types" | "unused_trait_names" ) - }) { + { return; } }, diff --git a/clippy_lints/src/attrs/utils.rs b/clippy_lints/src/attrs/utils.rs index 3bb02688bf2..96de0642904 100644 --- a/clippy_lints/src/attrs/utils.rs +++ b/clippy_lints/src/attrs/utils.rs @@ -75,13 +75,18 @@ fn is_relevant_expr(cx: &LateContext<'_>, typeck_results: &ty::TypeckResults<'_> /// Returns the lint name if it is clippy lint. pub(super) fn extract_clippy_lint(lint: &MetaItemInner) -> Option<Symbol> { - if let Some(meta_item) = lint.meta_item() - && meta_item.path.segments.len() > 1 - && let tool_name = meta_item.path.segments[0].ident - && tool_name.name == sym::clippy - { - let lint_name = meta_item.path.segments.last().unwrap().ident.name; - return Some(lint_name); + match namespace_and_lint(lint) { + (Some(sym::clippy), name) => name, + _ => None, + } +} + +/// Returns the lint namespace, if any, as well as the lint name. (`None`, `None`) means +/// the lint had less than 1 or more than 2 segments. +pub(super) fn namespace_and_lint(lint: &MetaItemInner) -> (Option<Symbol>, Option<Symbol>) { + match lint.meta_item().map(|m| m.path.segments.as_slice()).unwrap_or_default() { + [name] => (None, Some(name.ident.name)), + [namespace, name] => (Some(namespace.ident.name), Some(name.ident.name)), + _ => (None, None), } - None } diff --git a/clippy_lints/src/booleans.rs b/clippy_lints/src/booleans.rs index f68a7a89b39..f8c30d1c881 100644 --- a/clippy_lints/src/booleans.rs +++ b/clippy_lints/src/booleans.rs @@ -5,11 +5,11 @@ use clippy_utils::msrvs::{self, Msrv}; use clippy_utils::source::SpanRangeExt; use clippy_utils::ty::{implements_trait, is_type_diagnostic_item}; use rustc_ast::ast::LitKind; +use rustc_attr_parsing::RustcVersion; use rustc_errors::Applicability; use rustc_hir::intravisit::{FnKind, Visitor, walk_expr}; use rustc_hir::{BinOpKind, Body, Expr, ExprKind, FnDecl, UnOp}; use rustc_lint::{LateContext, LateLintPass, Level}; -use rustc_attr_parsing::RustcVersion; use rustc_session::impl_lint_pass; use rustc_span::def_id::LocalDefId; use rustc_span::{Span, sym}; diff --git a/clippy_lints/src/casts/as_pointer_underscore.rs b/clippy_lints/src/casts/as_pointer_underscore.rs new file mode 100644 index 00000000000..536126fd02b --- /dev/null +++ b/clippy_lints/src/casts/as_pointer_underscore.rs @@ -0,0 +1,19 @@ +use rustc_errors::Applicability; +use rustc_lint::LateContext; +use rustc_middle::ty::Ty; + +pub fn check<'tcx>(cx: &LateContext<'tcx>, ty_into: Ty<'_>, cast_to_hir: &'tcx rustc_hir::Ty<'tcx>) { + if let rustc_hir::TyKind::Ptr(rustc_hir::MutTy { ty, .. }) = cast_to_hir.kind + && matches!(ty.kind, rustc_hir::TyKind::Infer) + { + clippy_utils::diagnostics::span_lint_and_sugg( + cx, + super::AS_POINTER_UNDERSCORE, + cast_to_hir.span, + "using inferred pointer cast", + "use explicit type", + ty_into.to_string(), + Applicability::MachineApplicable, + ); + } +} diff --git a/clippy_lints/src/casts/borrow_as_ptr.rs b/clippy_lints/src/casts/borrow_as_ptr.rs index 4dd51dcbc9a..67aa33ca06c 100644 --- a/clippy_lints/src/casts/borrow_as_ptr.rs +++ b/clippy_lints/src/casts/borrow_as_ptr.rs @@ -1,6 +1,7 @@ use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::msrvs::Msrv; use clippy_utils::source::snippet_with_context; -use clippy_utils::std_or_core; +use clippy_utils::{is_lint_allowed, msrvs, std_or_core}; use rustc_errors::Applicability; use rustc_hir::{BorrowKind, Expr, ExprKind, Mutability, Ty, TyKind}; use rustc_lint::LateContext; @@ -13,15 +14,12 @@ pub(super) fn check<'tcx>( expr: &'tcx Expr<'_>, cast_expr: &'tcx Expr<'_>, cast_to: &'tcx Ty<'_>, -) { + msrv: &Msrv, +) -> bool { if matches!(cast_to.kind, TyKind::Ptr(_)) && let ExprKind::AddrOf(BorrowKind::Ref, mutability, e) = cast_expr.kind - && let Some(std_or_core) = std_or_core(cx) + && !is_lint_allowed(cx, BORROW_AS_PTR, expr.hir_id) { - let macro_name = match mutability { - Mutability::Not => "addr_of", - Mutability::Mut => "addr_of_mut", - }; let mut app = Applicability::MachineApplicable; let snip = snippet_with_context(cx, e.span, cast_expr.span.ctxt(), "..", &mut app).0; // Fix #9884 @@ -31,17 +29,36 @@ pub(super) fn check<'tcx>( .get(base.hir_id) .is_some_and(|x| x.iter().any(|adj| matches!(adj.kind, Adjust::Deref(_)))) }) { - return; + return false; } + let suggestion = if msrv.meets(msrvs::RAW_REF_OP) { + let operator_kind = match mutability { + Mutability::Not => "const", + Mutability::Mut => "mut", + }; + format!("&raw {operator_kind} {snip}") + } else { + let Some(std_or_core) = std_or_core(cx) else { + return false; + }; + let macro_name = match mutability { + Mutability::Not => "addr_of", + Mutability::Mut => "addr_of_mut", + }; + format!("{std_or_core}::ptr::{macro_name}!({snip})") + }; + span_lint_and_sugg( cx, BORROW_AS_PTR, expr.span, "borrow as raw pointer", "try", - format!("{std_or_core}::ptr::{macro_name}!({snip})"), + suggestion, Applicability::MachineApplicable, ); + return true; } + false } diff --git a/clippy_lints/src/casts/mod.rs b/clippy_lints/src/casts/mod.rs index 8b884399f92..c64c0e15144 100644 --- a/clippy_lints/src/casts/mod.rs +++ b/clippy_lints/src/casts/mod.rs @@ -1,3 +1,4 @@ +mod as_pointer_underscore; mod as_ptr_cast_mut; mod as_underscore; mod borrow_as_ptr; @@ -574,13 +575,13 @@ declare_clippy_lint! { declare_clippy_lint! { /// ### What it does /// Checks for the usage of `&expr as *const T` or - /// `&mut expr as *mut T`, and suggest using `ptr::addr_of` or - /// `ptr::addr_of_mut` instead. + /// `&mut expr as *mut T`, and suggest using `&raw const` or + /// `&raw mut` instead. /// /// ### Why is this bad? /// This would improve readability and avoid creating a reference /// that points to an uninitialized value or unaligned place. - /// Read the `ptr::addr_of` docs for more information. + /// Read the `&raw` explanation in the Reference for more information. /// /// ### Example /// ```no_run @@ -593,10 +594,10 @@ declare_clippy_lint! { /// Use instead: /// ```no_run /// let val = 1; - /// let p = std::ptr::addr_of!(val); + /// let p = &raw const val; /// /// let mut val_mut = 1; - /// let p_mut = std::ptr::addr_of_mut!(val_mut); + /// let p_mut = &raw mut val_mut; /// ``` #[clippy::version = "1.60.0"] pub BORROW_AS_PTR, @@ -726,6 +727,33 @@ declare_clippy_lint! { "using `as` to cast a reference to pointer" } +declare_clippy_lint! { + /// ### What it does + /// Checks for the usage of `as *const _` or `as *mut _` conversion using inferred type. + /// + /// ### Why restrict this? + /// The conversion might include a dangerous cast that might go undetected due to the type being inferred. + /// + /// ### Example + /// ```no_run + /// fn as_usize<T>(t: &T) -> usize { + /// // BUG: `t` is already a reference, so we will here + /// // return a dangling pointer to a temporary value instead + /// &t as *const _ as usize + /// } + /// ``` + /// Use instead: + /// ```no_run + /// fn as_usize<T>(t: &T) -> usize { + /// t as *const T as usize + /// } + /// ``` + #[clippy::version = "1.81.0"] + pub AS_POINTER_UNDERSCORE, + restriction, + "detects `as *mut _` and `as *const _` conversion" +} + pub struct Casts { msrv: Msrv, } @@ -763,6 +791,7 @@ impl_lint_pass!(Casts => [ CAST_NAN_TO_INT, ZERO_PTR, REF_AS_PTR, + AS_POINTER_UNDERSCORE, ]); impl<'tcx> LateLintPass<'tcx> for Casts { @@ -805,11 +834,15 @@ impl<'tcx> LateLintPass<'tcx> for Casts { } as_underscore::check(cx, expr, cast_to_hir); - - if self.msrv.meets(msrvs::PTR_FROM_REF) { + as_pointer_underscore::check(cx, cast_to, cast_to_hir); + + let was_borrow_as_ptr_emitted = if self.msrv.meets(msrvs::BORROW_AS_PTR) { + borrow_as_ptr::check(cx, expr, cast_from_expr, cast_to_hir, &self.msrv) + } else { + false + }; + if self.msrv.meets(msrvs::PTR_FROM_REF) && !was_borrow_as_ptr_emitted { ref_as_ptr::check(cx, expr, cast_from_expr, cast_to_hir); - } else if self.msrv.meets(msrvs::BORROW_AS_PTR) { - borrow_as_ptr::check(cx, expr, cast_from_expr, cast_to_hir); } } diff --git a/clippy_lints/src/comparison_chain.rs b/clippy_lints/src/comparison_chain.rs index c85e3500ebd..61c92d441d0 100644 --- a/clippy_lints/src/comparison_chain.rs +++ b/clippy_lints/src/comparison_chain.rs @@ -1,6 +1,8 @@ -use clippy_utils::diagnostics::span_lint_and_help; +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::sugg::Sugg; use clippy_utils::ty::implements_trait; use clippy_utils::{SpanlessEq, if_sequence, is_else_clause, is_in_const_context}; +use rustc_errors::Applicability; use rustc_hir::{BinOpKind, Expr, ExprKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::declare_lint_pass; @@ -120,13 +122,19 @@ impl<'tcx> LateLintPass<'tcx> for ComparisonChain { return; } } - span_lint_and_help( + let ExprKind::Binary(_, lhs, rhs) = conds[0].kind else { + unreachable!(); + }; + let lhs = Sugg::hir(cx, lhs, "..").maybe_par(); + let rhs = Sugg::hir(cx, rhs, "..").addr(); + span_lint_and_sugg( cx, COMPARISON_CHAIN, expr.span, "`if` chain can be rewritten with `match`", - None, - "consider rewriting the `if` chain to use `cmp` and `match`", + "consider rewriting the `if` chain with `match`", + format!("match {lhs}.cmp({rhs}) {{...}}"), + Applicability::HasPlaceholders, ); } } diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 022ed180ed8..7451fb909ef 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -55,6 +55,7 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::attrs::INLINE_ALWAYS_INFO, crate::attrs::MIXED_ATTRIBUTES_STYLE_INFO, crate::attrs::NON_MINIMAL_CFG_INFO, + crate::attrs::REPR_PACKED_WITHOUT_ABI_INFO, crate::attrs::SHOULD_PANIC_WITHOUT_EXPECT_INFO, crate::attrs::UNNECESSARY_CLIPPY_CFG_INFO, crate::attrs::USELESS_ATTRIBUTE_INFO, @@ -75,6 +76,7 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::cargo::NEGATIVE_FEATURE_NAMES_INFO, crate::cargo::REDUNDANT_FEATURE_NAMES_INFO, crate::cargo::WILDCARD_DEPENDENCIES_INFO, + crate::casts::AS_POINTER_UNDERSCORE_INFO, crate::casts::AS_PTR_CAST_MUT_INFO, crate::casts::AS_UNDERSCORE_INFO, crate::casts::BORROW_AS_PTR_INFO, @@ -139,6 +141,7 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::doc::DOC_LAZY_CONTINUATION_INFO, crate::doc::DOC_LINK_WITH_QUOTES_INFO, crate::doc::DOC_MARKDOWN_INFO, + crate::doc::DOC_NESTED_REFDEFS_INFO, crate::doc::EMPTY_DOCS_INFO, crate::doc::EMPTY_LINE_AFTER_DOC_COMMENTS_INFO, crate::doc::EMPTY_LINE_AFTER_OUTER_ATTR_INFO, @@ -277,6 +280,7 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::literal_representation::MISTYPED_LITERAL_SUFFIXES_INFO, crate::literal_representation::UNREADABLE_LITERAL_INFO, crate::literal_representation::UNUSUAL_BYTE_GROUPINGS_INFO, + crate::literal_string_with_formatting_args::LITERAL_STRING_WITH_FORMATTING_ARGS_INFO, crate::loops::EMPTY_LOOP_INFO, crate::loops::EXPLICIT_COUNTER_LOOP_INFO, crate::loops::EXPLICIT_INTO_ITER_LOOP_INFO, diff --git a/clippy_lints/src/default_numeric_fallback.rs b/clippy_lints/src/default_numeric_fallback.rs index 6819ad547f8..3b3a78cb115 100644 --- a/clippy_lints/src/default_numeric_fallback.rs +++ b/clippy_lints/src/default_numeric_fallback.rs @@ -26,7 +26,8 @@ declare_clippy_lint! { /// To ensure that every numeric type is chosen explicitly rather than implicitly. /// /// ### Known problems - /// This lint can only be allowed at the function level or above. + /// This lint is implemented using a custom algorithm independent of rustc's inference, + /// which results in many false positives and false negatives. /// /// ### Example /// ```no_run @@ -36,8 +37,8 @@ declare_clippy_lint! { /// /// Use instead: /// ```no_run - /// let i = 10i32; - /// let f = 1.23f64; + /// let i = 10_i32; + /// let f = 1.23_f64; /// ``` #[clippy::version = "1.52.0"] pub DEFAULT_NUMERIC_FALLBACK, diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index 821312a9e40..653726872c6 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -1004,7 +1004,10 @@ fn report<'tcx>( let needs_paren = match cx.tcx.parent_hir_node(data.first_expr.hir_id) { Node::Expr(e) => match e.kind { ExprKind::Call(callee, _) if callee.hir_id != data.first_expr.hir_id => false, - ExprKind::Call(..) => expr.precedence() < ExprPrecedence::Unambiguous || matches!(expr.kind, ExprKind::Field(..)), + ExprKind::Call(..) => { + expr.precedence() < ExprPrecedence::Unambiguous + || matches!(expr.kind, ExprKind::Field(..)) + }, _ => expr.precedence() < e.precedence(), }, _ => false, @@ -1017,11 +1020,7 @@ fn report<'tcx>( }) ); - let sugg = if !snip_is_macro - && needs_paren - && !has_enclosing_paren(&snip) - && !is_in_tuple - { + let sugg = if !snip_is_macro && needs_paren && !has_enclosing_paren(&snip) && !is_in_tuple { format!("({snip})") } else { snip.into() diff --git a/clippy_lints/src/derive.rs b/clippy_lints/src/derive.rs index f65edd36253..7c2f5efd8dd 100644 --- a/clippy_lints/src/derive.rs +++ b/clippy_lints/src/derive.rs @@ -6,9 +6,7 @@ use clippy_utils::{has_non_exhaustive_attr, is_lint_allowed, match_def_path, pat use rustc_errors::Applicability; use rustc_hir::def_id::DefId; use rustc_hir::intravisit::{FnKind, Visitor, walk_expr, walk_fn, walk_item}; -use rustc_hir::{ - self as hir, BlockCheckMode, BodyId, Expr, ExprKind, FnDecl, Impl, Item, ItemKind, UnsafeSource, -}; +use rustc_hir::{self as hir, BlockCheckMode, BodyId, Expr, ExprKind, FnDecl, Impl, Item, ItemKind, UnsafeSource}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::hir::nested_filter; use rustc_middle::ty::{ @@ -453,7 +451,7 @@ fn check_partial_eq_without_eq<'tcx>(cx: &LateContext<'tcx>, span: Span, trait_r && cx.tcx.is_diagnostic_item(sym::PartialEq, def_id) && !has_non_exhaustive_attr(cx.tcx, *adt) && !ty_implements_eq_trait(cx.tcx, ty, eq_trait_def_id) - && let typing_env = typing_env_env_for_derived_eq(cx.tcx, adt.did(), eq_trait_def_id) + && let typing_env = typing_env_for_derived_eq(cx.tcx, adt.did(), eq_trait_def_id) && let Some(local_def_id) = adt.did().as_local() // If all of our fields implement `Eq`, we can implement `Eq` too && adt @@ -484,7 +482,7 @@ fn ty_implements_eq_trait<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>, eq_trait_id: De } /// Creates the `ParamEnv` used for the give type's derived `Eq` impl. -fn typing_env_env_for_derived_eq(tcx: TyCtxt<'_>, did: DefId, eq_trait_id: DefId) -> ty::TypingEnv<'_> { +fn typing_env_for_derived_eq(tcx: TyCtxt<'_>, did: DefId, eq_trait_id: DefId) -> ty::TypingEnv<'_> { // Initial map from generic index to param def. // Vec<(param_def, needs_eq)> let mut params = tcx diff --git a/clippy_lints/src/disallowed_macros.rs b/clippy_lints/src/disallowed_macros.rs index a0cb36f88dc..a78c392e208 100644 --- a/clippy_lints/src/disallowed_macros.rs +++ b/clippy_lints/src/disallowed_macros.rs @@ -43,7 +43,6 @@ declare_clippy_lint! { /// ```no_run /// use serde::Serialize; /// - /// // Example code where clippy issues a warning /// println!("warns"); /// /// // The diagnostic will contain the message "no serializing" diff --git a/clippy_lints/src/disallowed_methods.rs b/clippy_lints/src/disallowed_methods.rs index 1e660b1957a..c4ed118b7c9 100644 --- a/clippy_lints/src/disallowed_methods.rs +++ b/clippy_lints/src/disallowed_methods.rs @@ -35,7 +35,6 @@ declare_clippy_lint! { /// ``` /// /// ```rust,ignore - /// // Example code where clippy issues a warning /// let xs = vec![1, 2, 3, 4]; /// xs.leak(); // Vec::leak is disallowed in the config. /// // The diagnostic contains the message "no leaking memory". @@ -47,7 +46,6 @@ declare_clippy_lint! { /// /// Use instead: /// ```rust,ignore - /// // Example code which does not raise clippy warning /// let mut xs = Vec::new(); // Vec::new is _not_ disallowed in the config. /// xs.push(123); // Vec::push is _not_ disallowed in the config. /// ``` diff --git a/clippy_lints/src/doc/include_in_doc_without_cfg.rs b/clippy_lints/src/doc/include_in_doc_without_cfg.rs index 0bb16a0c77d..4b40fc0b1ee 100644 --- a/clippy_lints/src/doc/include_in_doc_without_cfg.rs +++ b/clippy_lints/src/doc/include_in_doc_without_cfg.rs @@ -1,9 +1,9 @@ use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::snippet_opt; -use rustc_ast::{AttrStyle}; +use rustc_ast::AttrStyle; use rustc_errors::Applicability; +use rustc_hir::{AttrArgs, AttrKind, Attribute}; use rustc_lint::LateContext; -use rustc_hir::{Attribute, AttrKind, AttrArgs}; use super::DOC_INCLUDE_WITHOUT_CFG; diff --git a/clippy_lints/src/doc/mod.rs b/clippy_lints/src/doc/mod.rs index f65acd7978a..b2135fe18bd 100644 --- a/clippy_lints/src/doc/mod.rs +++ b/clippy_lints/src/doc/mod.rs @@ -5,7 +5,7 @@ mod too_long_first_doc_paragraph; use clippy_config::Conf; use clippy_utils::attrs::is_doc_hidden; -use clippy_utils::diagnostics::{span_lint, span_lint_and_help}; +use clippy_utils::diagnostics::{span_lint, span_lint_and_help, span_lint_and_then}; use clippy_utils::macros::{is_panic, root_macro_call_first_node}; use clippy_utils::ty::is_type_diagnostic_item; use clippy_utils::visitors::Visitable; @@ -17,6 +17,7 @@ use pulldown_cmark::Event::{ use pulldown_cmark::Tag::{BlockQuote, CodeBlock, FootnoteDefinition, Heading, Item, Link, Paragraph}; use pulldown_cmark::{BrokenLink, CodeBlockKind, CowStr, Options, TagEnd}; use rustc_data_structures::fx::FxHashSet; +use rustc_errors::Applicability; use rustc_hir::intravisit::{self, Visitor}; use rustc_hir::{AnonConst, Attribute, Expr, ImplItemKind, ItemKind, Node, Safety, TraitItemKind}; use rustc_lint::{LateContext, LateLintPass, LintContext}; @@ -563,6 +564,32 @@ declare_clippy_lint! { "check if files included in documentation are behind `cfg(doc)`" } +declare_clippy_lint! { + /// ### What it does + /// Warns if a link reference definition appears at the start of a + /// list item or quote. + /// + /// ### Why is this bad? + /// This is probably intended as an intra-doc link. If it is really + /// supposed to be a reference definition, it can be written outside + /// of the list item or quote. + /// + /// ### Example + /// ```no_run + /// //! - [link]: description + /// ``` + /// Use instead: + /// ```no_run + /// //! - [link][]: description (for intra-doc link) + /// //! + /// //! [link]: destination (for link reference definition) + /// ``` + #[clippy::version = "1.84.0"] + pub DOC_NESTED_REFDEFS, + suspicious, + "link reference defined in list item or quote" +} + pub struct Documentation { valid_idents: FxHashSet<String>, check_private_items: bool, @@ -580,6 +607,7 @@ impl Documentation { impl_lint_pass!(Documentation => [ DOC_LINK_WITH_QUOTES, DOC_MARKDOWN, + DOC_NESTED_REFDEFS, MISSING_SAFETY_DOC, MISSING_ERRORS_DOC, MISSING_PANICS_DOC, @@ -831,6 +859,31 @@ fn check_doc<'a, Events: Iterator<Item = (pulldown_cmark::Event<'a>, Range<usize Start(BlockQuote(_)) => { blockquote_level += 1; containers.push(Container::Blockquote); + if let Some((next_event, next_range)) = events.peek() { + let next_start = match next_event { + End(TagEnd::BlockQuote) => next_range.end, + _ => next_range.start, + }; + if let Some(refdefrange) = looks_like_refdef(doc, range.start..next_start) && + let Some(refdefspan) = fragments.span(cx, refdefrange.clone()) + { + span_lint_and_then( + cx, + DOC_NESTED_REFDEFS, + refdefspan, + "link reference defined in quote", + |diag| { + diag.span_suggestion_short( + refdefspan.shrink_to_hi(), + "for an intra-doc link, add `[]` between the label and the colon", + "[]", + Applicability::MaybeIncorrect, + ); + diag.help("link definitions are not shown in rendered documentation"); + } + ); + } + } }, End(TagEnd::BlockQuote) => { blockquote_level -= 1; @@ -869,11 +922,42 @@ fn check_doc<'a, Events: Iterator<Item = (pulldown_cmark::Event<'a>, Range<usize in_heading = true; } if let Start(Item) = event { - if let Some((_next_event, next_range)) = events.peek() { - containers.push(Container::List(next_range.start - range.start)); + let indent = if let Some((next_event, next_range)) = events.peek() { + let next_start = match next_event { + End(TagEnd::Item) => next_range.end, + _ => next_range.start, + }; + if let Some(refdefrange) = looks_like_refdef(doc, range.start..next_start) && + let Some(refdefspan) = fragments.span(cx, refdefrange.clone()) + { + span_lint_and_then( + cx, + DOC_NESTED_REFDEFS, + refdefspan, + "link reference defined in list item", + |diag| { + diag.span_suggestion_short( + refdefspan.shrink_to_hi(), + "for an intra-doc link, add `[]` between the label and the colon", + "[]", + Applicability::MaybeIncorrect, + ); + diag.help("link definitions are not shown in rendered documentation"); + } + ); + refdefrange.start - range.start + } else { + let mut start = next_range.start; + if start > 0 && doc.as_bytes().get(start - 1) == Some(&b'\\') { + // backslashes aren't in the event stream... + start -= 1; + } + start - range.start + } } else { - containers.push(Container::List(0)); - } + 0 + }; + containers.push(Container::List(indent)); } ticks_unbalanced = false; paragraph_range = range; @@ -1045,3 +1129,25 @@ impl<'tcx> Visitor<'tcx> for FindPanicUnwrap<'_, 'tcx> { self.cx.tcx.hir() } } + +#[expect(clippy::range_plus_one)] // inclusive ranges aren't the same type +fn looks_like_refdef(doc: &str, range: Range<usize>) -> Option<Range<usize>> { + let offset = range.start; + let mut iterator = doc.as_bytes()[range].iter().copied().enumerate(); + let mut start = None; + while let Some((i, byte)) = iterator.next() { + match byte { + b'\\' => { + iterator.next(); + }, + b'[' => { + start = Some(i + offset); + }, + b']' if let Some(start) = start => { + return Some(start..i + offset + 1); + }, + _ => {}, + } + } + None +} diff --git a/clippy_lints/src/explicit_write.rs b/clippy_lints/src/explicit_write.rs index 0550c22761a..a5a4e05b3a6 100644 --- a/clippy_lints/src/explicit_write.rs +++ b/clippy_lints/src/explicit_write.rs @@ -58,7 +58,7 @@ impl<'tcx> LateLintPass<'tcx> for ExplicitWrite { // match call to write_fmt && let ExprKind::MethodCall(write_fun, write_recv, [write_arg], _) = *look_in_block(cx, &write_call.kind) && let ExprKind::Call(write_recv_path, []) = write_recv.kind - && write_fun.ident.name.as_str() == "write_fmt" + && write_fun.ident.name == sym::write_fmt && let Some(def_id) = path_def_id(cx, write_recv_path) { // match calls to std::io::stdout() / std::io::stderr () diff --git a/clippy_lints/src/functions/mod.rs b/clippy_lints/src/functions/mod.rs index be3d0f7ad63..243eb5cbfd4 100644 --- a/clippy_lints/src/functions/mod.rs +++ b/clippy_lints/src/functions/mod.rs @@ -10,6 +10,7 @@ mod too_many_lines; use clippy_config::Conf; use clippy_utils::def_path_def_ids; +use clippy_utils::msrvs::Msrv; use rustc_hir as hir; use rustc_hir::intravisit; use rustc_lint::{LateContext, LateLintPass}; @@ -455,6 +456,7 @@ pub struct Functions { /// A set of resolved `def_id` of traits that are configured to allow /// function params renaming. trait_ids: DefIdSet, + msrv: Msrv, } impl Functions { @@ -469,6 +471,7 @@ impl Functions { .iter() .flat_map(|p| def_path_def_ids(tcx, &p.split("::").collect::<Vec<_>>())) .collect(), + msrv: conf.msrv.clone(), } } } @@ -518,12 +521,12 @@ impl<'tcx> LateLintPass<'tcx> for Functions { fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) { must_use::check_item(cx, item); - result::check_item(cx, item, self.large_error_threshold); + result::check_item(cx, item, self.large_error_threshold, &self.msrv); } fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::ImplItem<'_>) { must_use::check_impl_item(cx, item); - result::check_impl_item(cx, item, self.large_error_threshold); + result::check_impl_item(cx, item, self.large_error_threshold, &self.msrv); impl_trait_in_params::check_impl_item(cx, item); renamed_function_params::check_impl_item(cx, item, &self.trait_ids); } @@ -532,8 +535,10 @@ impl<'tcx> LateLintPass<'tcx> for Functions { too_many_arguments::check_trait_item(cx, item, self.too_many_arguments_threshold); not_unsafe_ptr_arg_deref::check_trait_item(cx, item); must_use::check_trait_item(cx, item); - result::check_trait_item(cx, item, self.large_error_threshold); + result::check_trait_item(cx, item, self.large_error_threshold, &self.msrv); impl_trait_in_params::check_trait_item(cx, item, self.avoid_breaking_exported_api); ref_option::check_trait_item(cx, item, self.avoid_breaking_exported_api); } + + extract_msrv_attr!(LateContext); } diff --git a/clippy_lints/src/functions/must_use.rs b/clippy_lints/src/functions/must_use.rs index 2b26285429a..afdb5d5306a 100644 --- a/clippy_lints/src/functions/must_use.rs +++ b/clippy_lints/src/functions/must_use.rs @@ -28,7 +28,7 @@ pub(super) fn check_item<'tcx>(cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_> let is_public = cx.effective_visibilities.is_exported(item.owner_id.def_id); let fn_header_span = item.span.with_hi(sig.decl.output.span().hi()); if let Some(attr) = attr { - check_needless_must_use(cx, sig.decl, item.owner_id, item.span, fn_header_span, attr, sig); + check_needless_must_use(cx, sig.decl, item.owner_id, item.span, fn_header_span, attr, attrs, sig); } else if is_public && !is_proc_macro(attrs) && !attrs.iter().any(|a| a.has_name(sym::no_mangle)) { check_must_use_candidate( cx, @@ -50,7 +50,7 @@ pub(super) fn check_impl_item<'tcx>(cx: &LateContext<'tcx>, item: &'tcx hir::Imp let attrs = cx.tcx.hir().attrs(item.hir_id()); let attr = cx.tcx.get_attr(item.owner_id, sym::must_use); if let Some(attr) = attr { - check_needless_must_use(cx, sig.decl, item.owner_id, item.span, fn_header_span, attr, sig); + check_needless_must_use(cx, sig.decl, item.owner_id, item.span, fn_header_span, attr, attrs, sig); } else if is_public && !is_proc_macro(attrs) && trait_ref_of_method(cx, item.owner_id.def_id).is_none() { check_must_use_candidate( cx, @@ -73,7 +73,7 @@ pub(super) fn check_trait_item<'tcx>(cx: &LateContext<'tcx>, item: &'tcx hir::Tr let attrs = cx.tcx.hir().attrs(item.hir_id()); let attr = cx.tcx.get_attr(item.owner_id, sym::must_use); if let Some(attr) = attr { - check_needless_must_use(cx, sig.decl, item.owner_id, item.span, fn_header_span, attr, sig); + check_needless_must_use(cx, sig.decl, item.owner_id, item.span, fn_header_span, attr, attrs, sig); } else if let hir::TraitFn::Provided(eid) = *eid { let body = cx.tcx.hir().body(eid); if attr.is_none() && is_public && !is_proc_macro(attrs) { @@ -91,6 +91,7 @@ pub(super) fn check_trait_item<'tcx>(cx: &LateContext<'tcx>, item: &'tcx hir::Tr } } +#[allow(clippy::too_many_arguments)] fn check_needless_must_use( cx: &LateContext<'_>, decl: &hir::FnDecl<'_>, @@ -98,21 +99,54 @@ fn check_needless_must_use( item_span: Span, fn_header_span: Span, attr: &Attribute, + attrs: &[Attribute], sig: &FnSig<'_>, ) { if in_external_macro(cx.sess(), item_span) { return; } if returns_unit(decl) { - span_lint_and_then( - cx, - MUST_USE_UNIT, - fn_header_span, - "this unit-returning function has a `#[must_use]` attribute", - |diag| { - diag.span_suggestion(attr.span, "remove the attribute", "", Applicability::MachineApplicable); - }, - ); + if attrs.len() == 1 { + span_lint_and_then( + cx, + MUST_USE_UNIT, + fn_header_span, + "this unit-returning function has a `#[must_use]` attribute", + |diag| { + diag.span_suggestion(attr.span, "remove the attribute", "", Applicability::MachineApplicable); + }, + ); + } else { + // When there are multiple attributes, it is not sufficient to simply make `must_use` empty, see + // issue #12320. + span_lint_and_then( + cx, + MUST_USE_UNIT, + fn_header_span, + "this unit-returning function has a `#[must_use]` attribute", + |diag| { + let mut attrs_without_must_use = attrs.to_vec(); + attrs_without_must_use.retain(|a| a.id != attr.id); + let sugg_str = attrs_without_must_use + .iter() + .map(|a| { + if a.value_str().is_none() { + return a.name_or_empty().to_string(); + } + format!("{} = \"{}\"", a.name_or_empty(), a.value_str().unwrap()) + }) + .collect::<Vec<_>>() + .join(", "); + + diag.span_suggestion( + attrs[0].span.with_hi(attrs[attrs.len() - 1].span.hi()), + "change these attributes to", + sugg_str, + Applicability::MachineApplicable, + ); + }, + ); + } } else if attr.value_str().is_none() && is_must_use_ty(cx, return_ty(cx, item_id)) { // Ignore async functions unless Future::Output type is a must_use type if sig.header.is_async() { diff --git a/clippy_lints/src/functions/result.rs b/clippy_lints/src/functions/result.rs index d4eaa166320..674d78eaae7 100644 --- a/clippy_lints/src/functions/result.rs +++ b/clippy_lints/src/functions/result.rs @@ -1,3 +1,4 @@ +use clippy_utils::msrvs::{self, Msrv}; use rustc_errors::Diag; use rustc_hir as hir; use rustc_lint::{LateContext, LintContext}; @@ -6,8 +7,8 @@ use rustc_middle::ty::{self, Ty}; use rustc_span::{Span, sym}; use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_then}; -use clippy_utils::trait_ref_of_method; use clippy_utils::ty::{AdtVariantInfo, approx_ty_size, is_type_diagnostic_item}; +use clippy_utils::{is_no_std_crate, trait_ref_of_method}; use super::{RESULT_LARGE_ERR, RESULT_UNIT_ERR}; @@ -34,19 +35,24 @@ fn result_err_ty<'tcx>( } } -pub(super) fn check_item<'tcx>(cx: &LateContext<'tcx>, item: &hir::Item<'tcx>, large_err_threshold: u64) { +pub(super) fn check_item<'tcx>(cx: &LateContext<'tcx>, item: &hir::Item<'tcx>, large_err_threshold: u64, msrv: &Msrv) { if let hir::ItemKind::Fn(ref sig, _generics, _) = item.kind && let Some((hir_ty, err_ty)) = result_err_ty(cx, sig.decl, item.owner_id.def_id, item.span) { if cx.effective_visibilities.is_exported(item.owner_id.def_id) { let fn_header_span = item.span.with_hi(sig.decl.output.span().hi()); - check_result_unit_err(cx, err_ty, fn_header_span); + check_result_unit_err(cx, err_ty, fn_header_span, msrv); } check_result_large_err(cx, err_ty, hir_ty.span, large_err_threshold); } } -pub(super) fn check_impl_item<'tcx>(cx: &LateContext<'tcx>, item: &hir::ImplItem<'tcx>, large_err_threshold: u64) { +pub(super) fn check_impl_item<'tcx>( + cx: &LateContext<'tcx>, + item: &hir::ImplItem<'tcx>, + large_err_threshold: u64, + msrv: &Msrv, +) { // Don't lint if method is a trait's implementation, we can't do anything about those if let hir::ImplItemKind::Fn(ref sig, _) = item.kind && let Some((hir_ty, err_ty)) = result_err_ty(cx, sig.decl, item.owner_id.def_id, item.span) @@ -54,26 +60,31 @@ pub(super) fn check_impl_item<'tcx>(cx: &LateContext<'tcx>, item: &hir::ImplItem { if cx.effective_visibilities.is_exported(item.owner_id.def_id) { let fn_header_span = item.span.with_hi(sig.decl.output.span().hi()); - check_result_unit_err(cx, err_ty, fn_header_span); + check_result_unit_err(cx, err_ty, fn_header_span, msrv); } check_result_large_err(cx, err_ty, hir_ty.span, large_err_threshold); } } -pub(super) fn check_trait_item<'tcx>(cx: &LateContext<'tcx>, item: &hir::TraitItem<'tcx>, large_err_threshold: u64) { +pub(super) fn check_trait_item<'tcx>( + cx: &LateContext<'tcx>, + item: &hir::TraitItem<'tcx>, + large_err_threshold: u64, + msrv: &Msrv, +) { if let hir::TraitItemKind::Fn(ref sig, _) = item.kind { let fn_header_span = item.span.with_hi(sig.decl.output.span().hi()); if let Some((hir_ty, err_ty)) = result_err_ty(cx, sig.decl, item.owner_id.def_id, item.span) { if cx.effective_visibilities.is_exported(item.owner_id.def_id) { - check_result_unit_err(cx, err_ty, fn_header_span); + check_result_unit_err(cx, err_ty, fn_header_span, msrv); } check_result_large_err(cx, err_ty, hir_ty.span, large_err_threshold); } } } -fn check_result_unit_err(cx: &LateContext<'_>, err_ty: Ty<'_>, fn_header_span: Span) { - if err_ty.is_unit() { +fn check_result_unit_err(cx: &LateContext<'_>, err_ty: Ty<'_>, fn_header_span: Span, msrv: &Msrv) { + if err_ty.is_unit() && (!is_no_std_crate(cx) || msrv.meets(msrvs::ERROR_IN_CORE)) { span_lint_and_help( cx, RESULT_UNIT_ERR, diff --git a/clippy_lints/src/if_not_else.rs b/clippy_lints/src/if_not_else.rs index 120c5396a1c..2806d4d0e5d 100644 --- a/clippy_lints/src/if_not_else.rs +++ b/clippy_lints/src/if_not_else.rs @@ -1,9 +1,13 @@ use clippy_utils::consts::{ConstEvalCtxt, Constant}; -use clippy_utils::diagnostics::span_lint_and_help; +use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg}; use clippy_utils::is_else_clause; +use clippy_utils::source::{HasSession, indent_of, reindent_multiline, snippet}; +use rustc_errors::Applicability; use rustc_hir::{BinOpKind, Expr, ExprKind, UnOp}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::declare_lint_pass; +use rustc_span::Span; +use std::borrow::Cow; declare_clippy_lint! { /// ### What it does @@ -54,7 +58,7 @@ fn is_zero_const(expr: &Expr<'_>, cx: &LateContext<'_>) -> bool { impl LateLintPass<'_> for IfNotElse { fn check_expr(&mut self, cx: &LateContext<'_>, e: &Expr<'_>) { - if let ExprKind::If(cond, _, Some(els)) = e.kind + if let ExprKind::If(cond, cond_inner, Some(els)) = e.kind && let ExprKind::DropTemps(cond) = cond.kind && let ExprKind::Block(..) = els.kind { @@ -79,8 +83,52 @@ impl LateLintPass<'_> for IfNotElse { // } // ``` if !e.span.from_expansion() && !is_else_clause(cx.tcx, e) { - span_lint_and_help(cx, IF_NOT_ELSE, e.span, msg, None, help); + match cond.kind { + ExprKind::Unary(UnOp::Not, _) | ExprKind::Binary(_, _, _) => span_lint_and_sugg( + cx, + IF_NOT_ELSE, + e.span, + msg, + "try", + make_sugg(cx, &cond.kind, cond_inner.span, els.span, "..", Some(e.span)).to_string(), + Applicability::MachineApplicable, + ), + _ => span_lint_and_help(cx, IF_NOT_ELSE, e.span, msg, None, help), + } } } } } + +fn make_sugg<'a>( + sess: &impl HasSession, + cond_kind: &'a ExprKind<'a>, + cond_inner: Span, + els_span: Span, + default: &'a str, + indent_relative_to: Option<Span>, +) -> Cow<'a, str> { + let cond_inner_snip = snippet(sess, cond_inner, default); + let els_snip = snippet(sess, els_span, default); + let indent = indent_relative_to.and_then(|s| indent_of(sess, s)); + + let suggestion = match cond_kind { + ExprKind::Unary(UnOp::Not, cond_rest) => { + format!( + "if {} {} else {}", + snippet(sess, cond_rest.span, default), + els_snip, + cond_inner_snip + ) + }, + ExprKind::Binary(_, lhs, rhs) => { + let lhs_snip = snippet(sess, lhs.span, default); + let rhs_snip = snippet(sess, rhs.span, default); + + format!("if {lhs_snip} == {rhs_snip} {els_snip} else {cond_inner_snip}") + }, + _ => String::new(), + }; + + reindent_multiline(suggestion.into(), true, indent) +} diff --git a/clippy_lints/src/incompatible_msrv.rs b/clippy_lints/src/incompatible_msrv.rs index 6cee7cfaca2..b10206dcd05 100644 --- a/clippy_lints/src/incompatible_msrv.rs +++ b/clippy_lints/src/incompatible_msrv.rs @@ -2,7 +2,7 @@ use clippy_config::Conf; use clippy_utils::diagnostics::span_lint; use clippy_utils::is_in_test; use clippy_utils::msrvs::Msrv; -use rustc_attr_parsing::{StabilityLevel, StableSince, RustcVersion}; +use rustc_attr_parsing::{RustcVersion, StabilityLevel, StableSince}; use rustc_data_structures::fx::FxHashMap; use rustc_hir::{Expr, ExprKind, HirId}; use rustc_lint::{LateContext, LateLintPass}; diff --git a/clippy_lints/src/index_refutable_slice.rs b/clippy_lints/src/index_refutable_slice.rs index c2030a5ab09..15650c4f732 100644 --- a/clippy_lints/src/index_refutable_slice.rs +++ b/clippy_lints/src/index_refutable_slice.rs @@ -135,7 +135,7 @@ fn lint_slice(cx: &LateContext<'_>, slice: &SliceLintInformation) { .map(|(index, _)| *index) .collect::<FxIndexSet<_>>(); - let value_name = |index| format!("{}_{index}", slice.ident.name); + let value_name = |index| format!("{}_{}", slice.ident.name, index); if let Some(max_index) = used_indices.iter().max() { let opt_ref = if slice.needs_ref { "ref " } else { "" }; @@ -150,6 +150,18 @@ fn lint_slice(cx: &LateContext<'_>, slice: &SliceLintInformation) { .collect::<Vec<_>>(); let pat_sugg = format!("[{}, ..]", pat_sugg_idents.join(", ")); + let mut suggestions = Vec::new(); + + // Add the binding pattern suggestion + if !slice.pattern_spans.is_empty() { + suggestions.extend(slice.pattern_spans.iter().map(|span| (*span, pat_sugg.clone()))); + } + + // Add the index replacement suggestions + if !slice.index_use.is_empty() { + suggestions.extend(slice.index_use.iter().map(|(index, span)| (*span, value_name(*index)))); + } + span_lint_and_then( cx, INDEX_REFUTABLE_SLICE, @@ -157,28 +169,10 @@ fn lint_slice(cx: &LateContext<'_>, slice: &SliceLintInformation) { "this binding can be a slice pattern to avoid indexing", |diag| { diag.multipart_suggestion( - "try using a slice pattern here", - slice - .pattern_spans - .iter() - .map(|span| (*span, pat_sugg.clone())) - .collect(), + "replace the binding and indexed access with a slice pattern", + suggestions, Applicability::MaybeIncorrect, ); - - diag.multipart_suggestion( - "and replace the index expressions here", - slice - .index_use - .iter() - .map(|(index, span)| (*span, value_name(*index))) - .collect(), - Applicability::MaybeIncorrect, - ); - - // The lint message doesn't contain a warning about the removed index expression, - // since `filter_lintable_slices` will only return slices where all access indices - // are known at compile time. Therefore, they can be removed without side effects. }, ); } diff --git a/clippy_lints/src/indexing_slicing.rs b/clippy_lints/src/indexing_slicing.rs index ae2c3e0491f..f666ed0a440 100644 --- a/clippy_lints/src/indexing_slicing.rs +++ b/clippy_lints/src/indexing_slicing.rs @@ -2,7 +2,7 @@ use clippy_config::Conf; use clippy_utils::consts::{ConstEvalCtxt, Constant}; use clippy_utils::diagnostics::{span_lint, span_lint_and_then}; use clippy_utils::ty::{deref_chain, get_adt_inherent_method}; -use clippy_utils::{higher, is_from_proc_macro}; +use clippy_utils::{higher, is_from_proc_macro, is_in_test}; use rustc_ast::ast::RangeLimits; use rustc_hir::{Expr, ExprKind}; use rustc_lint::{LateContext, LateLintPass}; @@ -42,39 +42,50 @@ declare_clippy_lint! { declare_clippy_lint! { /// ### What it does - /// Checks for usage of indexing or slicing. Arrays are special cases, this lint - /// does report on arrays if we can tell that slicing operations are in bounds and does not - /// lint on constant `usize` indexing on arrays because that is handled by rustc's `const_err` lint. + /// Checks for usage of indexing or slicing that may panic at runtime. + /// + /// This lint does not report on indexing or slicing operations + /// that always panic, clippy's `out_of_bound_indexing` already + /// handles those cases. /// /// ### Why restrict this? /// To avoid implicit panics from indexing and slicing. + /// /// There are “checked” alternatives which do not panic, and can be used with `unwrap()` to make /// an explicit panic when it is desired. /// + /// ### Limitations + /// This lint does not check for the usage of indexing or slicing on strings. These are covered + /// by the more specific `string_slice` lint. + /// /// ### Example /// ```rust,no_run /// // Vector - /// let x = vec![0; 5]; + /// let x = vec![0, 1, 2, 3]; /// /// x[2]; + /// x[100]; /// &x[2..100]; /// /// // Array /// let y = [0, 1, 2, 3]; /// - /// &y[10..100]; - /// &y[10..]; + /// let i = 10; // Could be a runtime value + /// let j = 20; + /// &y[i..j]; /// ``` /// /// Use instead: /// ```no_run - /// # let x = vec![0; 5]; - /// # let y = [0, 1, 2, 3]; + /// # let x = vec![0, 1, 2, 3]; /// x.get(2); + /// x.get(100); /// x.get(2..100); /// - /// y.get(10); - /// y.get(10..100); + /// # let y = [0, 1, 2, 3]; + /// let i = 10; + /// let j = 20; + /// y.get(i..j); /// ``` #[clippy::version = "pre 1.29.0"] pub INDEXING_SLICING, @@ -85,12 +96,14 @@ declare_clippy_lint! { impl_lint_pass!(IndexingSlicing => [INDEXING_SLICING, OUT_OF_BOUNDS_INDEXING]); pub struct IndexingSlicing { + allow_indexing_slicing_in_tests: bool, suppress_restriction_lint_in_const: bool, } impl IndexingSlicing { pub fn new(conf: &'static Conf) -> Self { Self { + allow_indexing_slicing_in_tests: conf.allow_indexing_slicing_in_tests, suppress_restriction_lint_in_const: conf.suppress_restriction_lint_in_const, } } @@ -111,6 +124,7 @@ impl<'tcx> LateLintPass<'tcx> for IndexingSlicing { { let note = "the suggestion might not be applicable in constant blocks"; let ty = cx.typeck_results().expr_ty(array).peel_refs(); + let allowed_in_tests = self.allow_indexing_slicing_in_tests && is_in_test(cx.tcx, expr.hir_id); if let Some(range) = higher::Range::hir(index) { // Ranged indexes, i.e., &x[n..m], &x[n..], &x[..n] and &x[..] if let ty::Array(_, s) = ty.kind() { @@ -160,6 +174,10 @@ impl<'tcx> LateLintPass<'tcx> for IndexingSlicing { (None, None) => return, // [..] is ok. }; + if allowed_in_tests { + return; + } + span_lint_and_then(cx, INDEXING_SLICING, expr.span, "slicing may panic", |diag| { diag.help(help_msg); @@ -198,6 +216,10 @@ impl<'tcx> LateLintPass<'tcx> for IndexingSlicing { } } + if allowed_in_tests { + return; + } + span_lint_and_then(cx, INDEXING_SLICING, expr.span, "indexing may panic", |diag| { diag.help("consider using `.get(n)` or `.get_mut(n)` instead"); diff --git a/clippy_lints/src/large_include_file.rs b/clippy_lints/src/large_include_file.rs index 66d4c40ab5e..f3d62b513e8 100644 --- a/clippy_lints/src/large_include_file.rs +++ b/clippy_lints/src/large_include_file.rs @@ -2,8 +2,8 @@ use clippy_config::Conf; use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::macros::root_macro_call_first_node; use clippy_utils::source::snippet_opt; -use rustc_ast::{LitKind}; -use rustc_hir::{Expr, ExprKind, Attribute, AttrArgs, AttrKind}; +use rustc_ast::LitKind; +use rustc_hir::{AttrArgs, AttrKind, Attribute, Expr, ExprKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::impl_lint_pass; use rustc_span::{Span, sym}; diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 3e8315588cc..d33013ba663 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -9,6 +9,7 @@ #![feature(iter_partition_in_place)] #![feature(let_chains)] #![feature(never_type)] +#![feature(round_char_boundary)] #![feature(rustc_private)] #![feature(stmt_expr_attributes)] #![feature(unwrap_infallible)] @@ -17,7 +18,8 @@ clippy::missing_docs_in_private_items, clippy::must_use_candidate, rustc::diagnostic_outside_of_impl, - rustc::untranslatable_diagnostic + rustc::untranslatable_diagnostic, + clippy::literal_string_with_formatting_args )] #![warn( trivial_casts, @@ -49,6 +51,7 @@ extern crate rustc_lexer; extern crate rustc_lint; extern crate rustc_middle; extern crate rustc_parse; +extern crate rustc_parse_format; extern crate rustc_resolve; extern crate rustc_session; extern crate rustc_span; @@ -196,6 +199,7 @@ mod let_with_type_underscore; mod lifetimes; mod lines_filter_map_ok; mod literal_representation; +mod literal_string_with_formatting_args; mod loops; mod macro_metavars_in_unsafe; mod macro_use; @@ -957,6 +961,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { store.register_late_pass(move |_| Box::new(manual_div_ceil::ManualDivCeil::new(conf))); store.register_late_pass(|_| Box::new(manual_is_power_of_two::ManualIsPowerOfTwo)); store.register_late_pass(|_| Box::new(non_zero_suggestions::NonZeroSuggestions)); + store.register_late_pass(|_| Box::new(literal_string_with_formatting_args::LiteralStringWithFormattingArg)); store.register_late_pass(move |_| Box::new(unused_trait_names::UnusedTraitNames::new(conf))); store.register_late_pass(|_| Box::new(manual_ignore_case_cmp::ManualIgnoreCaseCmp)); store.register_late_pass(|_| Box::new(unnecessary_literal_bound::UnnecessaryLiteralBound)); diff --git a/clippy_lints/src/lifetimes.rs b/clippy_lints/src/lifetimes.rs index 35b14776e59..8b2eee34a97 100644 --- a/clippy_lints/src/lifetimes.rs +++ b/clippy_lints/src/lifetimes.rs @@ -643,8 +643,7 @@ fn report_extra_impl_lifetimes<'tcx>(cx: &LateContext<'tcx>, impl_: &'tcx Impl<' // An `impl` lifetime is elidable if it satisfies the following conditions: // - It is used exactly once. -// - That single use is not in a bounded type or `GenericArgs` in a `WherePredicate`. (Note that -// `GenericArgs` are different from `GenericParam`s.) +// - That single use is not in a `WherePredicate`. fn report_elidable_impl_lifetimes<'tcx>( cx: &LateContext<'tcx>, impl_: &'tcx Impl<'_>, @@ -658,12 +657,6 @@ fn report_elidable_impl_lifetimes<'tcx>( lifetime, in_where_predicate: false, .. - } - | Usage { - lifetime, - in_bounded_ty: false, - in_generics_arg: false, - .. }, ] = usages.as_slice() { diff --git a/clippy_lints/src/literal_string_with_formatting_args.rs b/clippy_lints/src/literal_string_with_formatting_args.rs new file mode 100644 index 00000000000..49353a1b76b --- /dev/null +++ b/clippy_lints/src/literal_string_with_formatting_args.rs @@ -0,0 +1,167 @@ +use rustc_ast::{LitKind, StrStyle}; +use rustc_hir::{Expr, ExprKind}; +use rustc_lexer::is_ident; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_parse_format::{ParseMode, Parser, Piece}; +use rustc_session::declare_lint_pass; +use rustc_span::{BytePos, Span}; + +use clippy_utils::diagnostics::span_lint; +use clippy_utils::mir::enclosing_mir; + +declare_clippy_lint! { + /// ### What it does + /// Checks if string literals have formatting arguments outside of macros + /// using them (like `format!`). + /// + /// ### Why is this bad? + /// It will likely not generate the expected content. + /// + /// ### Example + /// ```no_run + /// let x: Option<usize> = None; + /// let y = "hello"; + /// x.expect("{y:?}"); + /// ``` + /// Use instead: + /// ```no_run + /// let x: Option<usize> = None; + /// let y = "hello"; + /// x.expect(&format!("{y:?}")); + /// ``` + #[clippy::version = "1.83.0"] + pub LITERAL_STRING_WITH_FORMATTING_ARGS, + suspicious, + "Checks if string literals have formatting arguments" +} + +declare_lint_pass!(LiteralStringWithFormattingArg => [LITERAL_STRING_WITH_FORMATTING_ARGS]); + +fn emit_lint(cx: &LateContext<'_>, expr: &Expr<'_>, spans: &[(Span, Option<String>)]) { + if !spans.is_empty() + && let Some(mir) = enclosing_mir(cx.tcx, expr.hir_id) + { + let spans = spans + .iter() + .filter_map(|(span, name)| { + if let Some(name) = name { + // We need to check that the name is a local. + if !mir + .var_debug_info + .iter() + .any(|local| !local.source_info.span.from_expansion() && local.name.as_str() == name) + { + return None; + } + } + Some(*span) + }) + .collect::<Vec<_>>(); + match spans.len() { + 0 => {}, + 1 => { + span_lint( + cx, + LITERAL_STRING_WITH_FORMATTING_ARGS, + spans, + "this looks like a formatting argument but it is not part of a formatting macro", + ); + }, + _ => { + span_lint( + cx, + LITERAL_STRING_WITH_FORMATTING_ARGS, + spans, + "these look like formatting arguments but are not part of a formatting macro", + ); + }, + } + } +} + +impl LateLintPass<'_> for LiteralStringWithFormattingArg { + fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { + if expr.span.from_expansion() { + return; + } + if let ExprKind::Lit(lit) = expr.kind { + let (add, symbol) = match lit.node { + LitKind::Str(symbol, style) => { + let add = match style { + StrStyle::Cooked => 1, + StrStyle::Raw(nb) => nb as usize + 2, + }; + (add, symbol) + }, + _ => return, + }; + let fmt_str = symbol.as_str(); + let lo = expr.span.lo(); + let mut current = fmt_str; + let mut diff_len = 0; + + let mut parser = Parser::new(current, None, None, false, ParseMode::Format); + let mut spans = Vec::new(); + while let Some(piece) = parser.next() { + if let Some(error) = parser.errors.last() { + // We simply ignore the errors and move after them. + if error.span.end >= current.len() { + break; + } + // We find the closest char to where the error location ends. + let pos = current.floor_char_boundary(error.span.end); + // We get the next character. + current = if let Some((next_char_pos, _)) = current[pos..].char_indices().nth(1) { + // We make the parser start from this new location. + ¤t[pos + next_char_pos..] + } else { + break; + }; + diff_len = fmt_str.len() - current.len(); + parser = Parser::new(current, None, None, false, ParseMode::Format); + } else if let Piece::NextArgument(arg) = piece { + let mut pos = arg.position_span; + pos.start += diff_len; + pos.end += diff_len; + + let start = fmt_str[..pos.start].rfind('{').unwrap_or(pos.start); + // If this is a unicode character escape, we don't want to lint. + if start > 1 && fmt_str[..start].ends_with("\\u") { + continue; + } + + if fmt_str[start + 1..].trim_start().starts_with('}') { + // We ignore `{}`. + continue; + } + + let end = fmt_str[start + 1..] + .find('}') + .map_or(pos.end, |found| start + 1 + found) + + 1; + let ident_start = start + 1; + let colon_pos = fmt_str[ident_start..end].find(':'); + let ident_end = colon_pos.unwrap_or(end - 1); + let mut name = None; + if ident_start < ident_end + && let arg = &fmt_str[ident_start..ident_end] + && !arg.is_empty() + && is_ident(arg) + { + name = Some(arg.to_string()); + } else if colon_pos.is_none() { + // Not a `{:?}`. + continue; + } + spans.push(( + expr.span + .with_hi(lo + BytePos((start + add).try_into().unwrap())) + .with_lo(lo + BytePos((end + add).try_into().unwrap())), + name, + )); + } + } + emit_lint(cx, expr, &spans); + } + } +} diff --git a/clippy_lints/src/manual_async_fn.rs b/clippy_lints/src/manual_async_fn.rs index 3c77db84a40..9f3b0957eab 100644 --- a/clippy_lints/src/manual_async_fn.rs +++ b/clippy_lints/src/manual_async_fn.rs @@ -74,7 +74,7 @@ impl<'tcx> LateLintPass<'tcx> for ManualAsyncFn { if let Some(vis_snip) = vis_span.get_source_text(cx) && let Some(header_snip) = header_span.get_source_text(cx) && let Some(ret_pos) = position_before_rarrow(&header_snip) - && let Some((ret_sugg, ret_snip)) = suggested_ret(cx, output) + && let Some((_, ret_snip)) = suggested_ret(cx, output) { let header_snip = if vis_snip.is_empty() { format!("async {}", &header_snip[..ret_pos]) @@ -82,19 +82,14 @@ impl<'tcx> LateLintPass<'tcx> for ManualAsyncFn { format!("{} async {}", vis_snip, &header_snip[vis_snip.len() + 1..ret_pos]) }; - let help = format!("make the function `async` and {ret_sugg}"); - diag.span_suggestion( - header_span, - help, - format!("{header_snip}{ret_snip}"), - Applicability::MachineApplicable, - ); + let body_snip = snippet_block(cx, closure_body.value.span, "..", Some(block.span)).to_string(); - let body_snip = snippet_block(cx, closure_body.value.span, "..", Some(block.span)); - diag.span_suggestion( - block.span, - "move the body of the async block to the enclosing function", - body_snip, + diag.multipart_suggestion( + "make the function `async` and return the output of the future directly", + vec![ + (header_span, format!("{header_snip}{ret_snip}")), + (block.span, body_snip), + ], Applicability::MachineApplicable, ); } diff --git a/clippy_lints/src/manual_float_methods.rs b/clippy_lints/src/manual_float_methods.rs index 0e08e2eb83d..a1951b9da44 100644 --- a/clippy_lints/src/manual_float_methods.rs +++ b/clippy_lints/src/manual_float_methods.rs @@ -5,9 +5,9 @@ use clippy_utils::msrvs::{self, Msrv}; use clippy_utils::source::SpanRangeExt; use clippy_utils::{is_from_proc_macro, path_to_local}; use rustc_errors::Applicability; -use rustc_hir::{BinOpKind, Constness, Expr, ExprKind}; use rustc_hir::def::DefKind; use rustc_hir::def_id::DefId; +use rustc_hir::{BinOpKind, Constness, Expr, ExprKind}; use rustc_lint::{LateContext, LateLintPass, Lint, LintContext}; use rustc_middle::lint::in_external_macro; use rustc_middle::ty::TyCtxt; @@ -129,9 +129,7 @@ fn is_not_const(tcx: TyCtxt<'_>, def_id: DefId) -> bool { | DefKind::Ctor(..) | DefKind::AssocConst => false, - DefKind::Fn - | DefKind::AssocFn - | DefKind::Closure => tcx.constness(def_id) == Constness::NotConst, + DefKind::Fn | DefKind::AssocFn | DefKind::Closure => tcx.constness(def_id) == Constness::NotConst, } } diff --git a/clippy_lints/src/manual_is_power_of_two.rs b/clippy_lints/src/manual_is_power_of_two.rs index 4fee3bf7aa9..841adfec462 100644 --- a/clippy_lints/src/manual_is_power_of_two.rs +++ b/clippy_lints/src/manual_is_power_of_two.rs @@ -43,21 +43,21 @@ impl LateLintPass<'_> for ManualIsPowerOfTwo { && bin_op.node == BinOpKind::Eq { // a.count_ones() == 1 - if let ExprKind::MethodCall(method_name, reciever, [], _) = left.kind + if let ExprKind::MethodCall(method_name, receiver, [], _) = left.kind && method_name.ident.as_str() == "count_ones" - && let &Uint(_) = cx.typeck_results().expr_ty(reciever).kind() + && let &Uint(_) = cx.typeck_results().expr_ty(receiver).kind() && check_lit(right, 1) { - build_sugg(cx, expr, reciever, &mut applicability); + build_sugg(cx, expr, receiver, &mut applicability); } // 1 == a.count_ones() - if let ExprKind::MethodCall(method_name, reciever, [], _) = right.kind + if let ExprKind::MethodCall(method_name, receiver, [], _) = right.kind && method_name.ident.as_str() == "count_ones" - && let &Uint(_) = cx.typeck_results().expr_ty(reciever).kind() + && let &Uint(_) = cx.typeck_results().expr_ty(receiver).kind() && check_lit(left, 1) { - build_sugg(cx, expr, reciever, &mut applicability); + build_sugg(cx, expr, receiver, &mut applicability); } // a & (a - 1) == 0 @@ -115,8 +115,8 @@ impl LateLintPass<'_> for ManualIsPowerOfTwo { } } -fn build_sugg(cx: &LateContext<'_>, expr: &Expr<'_>, reciever: &Expr<'_>, applicability: &mut Applicability) { - let snippet = snippet_with_applicability(cx, reciever.span, "..", applicability); +fn build_sugg(cx: &LateContext<'_>, expr: &Expr<'_>, receiver: &Expr<'_>, applicability: &mut Applicability) { + let snippet = snippet_with_applicability(cx, receiver.span, "..", applicability); span_lint_and_sugg( cx, diff --git a/clippy_lints/src/matches/match_same_arms.rs b/clippy_lints/src/matches/match_same_arms.rs index 20984bc40ca..b72a61a4384 100644 --- a/clippy_lints/src/matches/match_same_arms.rs +++ b/clippy_lints/src/matches/match_same_arms.rs @@ -74,8 +74,8 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) { // 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) + // 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) @@ -149,16 +149,12 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) { let move_pat_snip = snippet_with_applicability(cx, move_arm.pat.span, "<pat2>", &mut appl); let keep_pat_snip = snippet_with_applicability(cx, keep_arm.pat.span, "<pat1>", &mut appl); - diag.span_suggestion( - keep_arm.pat.span, - "or try merging the arm patterns", - format!("{keep_pat_snip} | {move_pat_snip}"), - appl, - ) - .span_suggestion( - adjusted_arm_span(cx, move_arm.span), - "and remove this obsolete arm", - "", + diag.multipart_suggestion( + "or try merging the arm patterns and removing the obsolete arm", + vec![ + (keep_arm.pat.span, format!("{keep_pat_snip} | {move_pat_snip}")), + (adjusted_arm_span(cx, move_arm.span), String::new()), + ], appl, ) .help("try changing either arm body"); diff --git a/clippy_lints/src/matches/mod.rs b/clippy_lints/src/matches/mod.rs index 64969271764..1fd2ebcb54a 100644 --- a/clippy_lints/src/matches/mod.rs +++ b/clippy_lints/src/matches/mod.rs @@ -27,7 +27,9 @@ mod wild_in_or_pats; use clippy_config::Conf; use clippy_utils::msrvs::{self, Msrv}; use clippy_utils::source::walk_span_to_context; -use clippy_utils::{higher, is_direct_expn_of, is_in_const_context, is_span_match, span_contains_cfg}; +use clippy_utils::{ + higher, is_direct_expn_of, is_in_const_context, is_span_match, span_contains_cfg, span_extract_comments, +}; use rustc_hir::{Arm, Expr, ExprKind, LetStmt, MatchSource, Pat, PatKind}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; @@ -1059,7 +1061,28 @@ impl<'tcx> LateLintPass<'tcx> for Matches { } redundant_pattern_match::check_match(cx, expr, ex, arms); - single_match::check(cx, ex, arms, expr); + let source_map = cx.tcx.sess.source_map(); + let mut match_comments = span_extract_comments(source_map, expr.span); + // We remove comments from inside arms block. + if !match_comments.is_empty() { + for arm in arms { + for comment in span_extract_comments(source_map, arm.body.span) { + if let Some(index) = match_comments + .iter() + .enumerate() + .find(|(_, cm)| **cm == comment) + .map(|(index, _)| index) + { + match_comments.remove(index); + } + } + } + } + // If there are still comments, it means they are outside of the arms, therefore + // we should not lint. + if match_comments.is_empty() { + single_match::check(cx, ex, arms, expr); + } match_bool::check(cx, ex, arms, expr); overlapping_arms::check(cx, ex, arms); match_wild_enum::check(cx, ex, arms); diff --git a/clippy_lints/src/matches/needless_match.rs b/clippy_lints/src/matches/needless_match.rs index 6f7d6902640..73822314b4b 100644 --- a/clippy_lints/src/matches/needless_match.rs +++ b/clippy_lints/src/matches/needless_match.rs @@ -3,7 +3,7 @@ use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::snippet_with_applicability; use clippy_utils::ty::{is_type_diagnostic_item, same_type_and_consts}; use clippy_utils::{ - eq_expr_value, get_parent_expr_for_hir, higher, is_else_clause, is_res_lang_ctor, over, path_res, + SpanlessEq, eq_expr_value, get_parent_expr_for_hir, higher, is_else_clause, is_res_lang_ctor, over, path_res, peel_blocks_with_stmt, }; use rustc_errors::Applicability; @@ -90,7 +90,9 @@ fn check_if_let_inner(cx: &LateContext<'_>, if_let: &higher::IfLet<'_>) -> bool } // Recursively check for each `else if let` phrase, - if let Some(ref nested_if_let) = higher::IfLet::hir(cx, if_else) { + if let Some(ref nested_if_let) = higher::IfLet::hir(cx, if_else) + && SpanlessEq::new(cx).eq_expr(nested_if_let.let_expr, if_let.let_expr) + { return check_if_let_inner(cx, nested_if_let); } diff --git a/clippy_lints/src/matches/single_match.rs b/clippy_lints/src/matches/single_match.rs index 95a4bf6f60d..3ca20479f8e 100644 --- a/clippy_lints/src/matches/single_match.rs +++ b/clippy_lints/src/matches/single_match.rs @@ -129,7 +129,7 @@ fn report_single_pattern(cx: &LateContext<'_>, ex: &Expr<'_>, arm: &Arm<'_>, exp PatKind::Lit(Expr { kind: ExprKind::Lit(lit), .. - }) if lit.node.is_str() => pat_ref_count + 1, + }) if lit.node.is_str() || lit.node.is_bytestr() => pat_ref_count + 1, _ => pat_ref_count, }; // References are only implicitly added to the pattern, so no overflow here. diff --git a/clippy_lints/src/methods/filter_map_identity.rs b/clippy_lints/src/methods/filter_map_identity.rs index cf7f276dabb..b04d761d486 100644 --- a/clippy_lints/src/methods/filter_map_identity.rs +++ b/clippy_lints/src/methods/filter_map_identity.rs @@ -2,6 +2,7 @@ use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::{is_expr_identity_function, is_expr_untyped_identity_function, is_trait_method}; use rustc_errors::Applicability; use rustc_hir as hir; +use rustc_hir::ExprKind; use rustc_lint::LateContext; use rustc_span::{Span, sym}; @@ -21,6 +22,15 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, filter_map_arg: if is_trait_method(cx, expr, sym::Iterator) && let Some(applicability) = is_identity(cx, filter_map_arg) { + // check if the iterator is from an empty array, see issue #12653 + if let ExprKind::MethodCall(_, recv, ..) = expr.kind + && let ExprKind::MethodCall(_, recv2, ..) = recv.kind + && let ExprKind::Array(arr) = recv2.kind + && arr.is_empty() + { + return; + } + span_lint_and_sugg( cx, FILTER_MAP_IDENTITY, diff --git a/clippy_lints/src/methods/map_with_unused_argument_over_ranges.rs b/clippy_lints/src/methods/map_with_unused_argument_over_ranges.rs index 80703618a11..1ebb71e251a 100644 --- a/clippy_lints/src/methods/map_with_unused_argument_over_ranges.rs +++ b/clippy_lints/src/methods/map_with_unused_argument_over_ranges.rs @@ -19,7 +19,7 @@ fn extract_count_with_applicability( ) -> Option<String> { let start = range.start?; let end = range.end?; - // TODO: This doens't handle if either the start or end are negative literals, or if the start is + // TODO: This doesn't handle if either the start or end are negative literals, or if the start is // not a literal. In the first case, we need to be careful about how we handle computing the // count to avoid overflows. In the second, we may need to add parenthesis to make the // suggestion correct. diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 7d1d5d69c99..810287fa541 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -1864,7 +1864,6 @@ declare_clippy_lint! { /// /// ### Example /// ```no_run - /// // example code where clippy issues a warning /// let opt: Option<u32> = None; /// /// opt.unwrap_or_else(|| 42); @@ -3839,13 +3838,11 @@ declare_clippy_lint! { /// /// ### Example /// ```no_run - /// // example code where clippy issues a warning /// vec![Some(1)].into_iter().filter(Option::is_some); /// /// ``` /// Use instead: /// ```no_run - /// // example code which does not raise clippy warning /// vec![Some(1)].into_iter().flatten(); /// ``` #[clippy::version = "1.77.0"] @@ -3865,13 +3862,11 @@ declare_clippy_lint! { /// /// ### Example /// ```no_run - /// // example code where clippy issues a warning /// vec![Ok::<i32, String>(1)].into_iter().filter(Result::is_ok); /// /// ``` /// Use instead: /// ```no_run - /// // example code which does not raise clippy warning /// vec![Ok::<i32, String>(1)].into_iter().flatten(); /// ``` #[clippy::version = "1.77.0"] @@ -3969,7 +3964,7 @@ declare_clippy_lint! { /// /// ### Why is this bad? /// - /// In the aformentioned cases it is not necessary to call `min()` or `max()` + /// In the aforementioned cases it is not necessary to call `min()` or `max()` /// to compare values, it may even cause confusion. /// /// ### Example @@ -4982,6 +4977,10 @@ impl Methods { } map_identity::check(cx, expr, recv, m_arg, name, span); manual_inspect::check(cx, expr, m_arg, name, span, &self.msrv); + crate::useless_conversion::check_function_application(cx, expr, recv, m_arg); + }, + ("map_break" | "map_continue", [m_arg]) => { + crate::useless_conversion::check_function_application(cx, expr, recv, m_arg); }, ("map_or", [def, map]) => { option_map_or_none::check(cx, expr, recv, def, map); diff --git a/clippy_lints/src/methods/needless_option_take.rs b/clippy_lints/src/methods/needless_option_take.rs index 829c118d291..c41ce2481d7 100644 --- a/clippy_lints/src/methods/needless_option_take.rs +++ b/clippy_lints/src/methods/needless_option_take.rs @@ -1,9 +1,6 @@ -use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::match_def_path; -use clippy_utils::source::snippet_with_applicability; +use clippy_utils::diagnostics::span_lint_and_note; use clippy_utils::ty::is_type_diagnostic_item; -use rustc_errors::Applicability; -use rustc_hir::Expr; +use rustc_hir::{Expr, ExprKind, QPath}; use rustc_lint::LateContext; use rustc_span::sym; @@ -11,20 +8,17 @@ use super::NEEDLESS_OPTION_TAKE; pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, recv: &'tcx Expr<'_>) { // Checks if expression type is equal to sym::Option and if the expr is not a syntactic place - if !recv.is_syntactic_place_expr() && is_expr_option(cx, recv) && has_expr_as_ref_path(cx, recv) { - let mut applicability = Applicability::MachineApplicable; - span_lint_and_sugg( - cx, - NEEDLESS_OPTION_TAKE, - expr.span, - "called `Option::take()` on a temporary value", - "try", - format!( - "{}", - snippet_with_applicability(cx, recv.span, "..", &mut applicability) - ), - applicability, - ); + if !recv.is_syntactic_place_expr() && is_expr_option(cx, recv) { + if let Some(function_name) = source_of_temporary_value(recv) { + span_lint_and_note( + cx, + NEEDLESS_OPTION_TAKE, + expr.span, + "called `Option::take()` on a temporary value", + None, + format!("`{function_name}` creates a temporary value, so calling take() has no effect"), + ); + } } } @@ -33,9 +27,24 @@ fn is_expr_option(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { is_type_diagnostic_item(cx, expr_type, sym::Option) } -fn has_expr_as_ref_path(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { - if let Some(ref_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) { - return match_def_path(cx, ref_id, &["core", "option", "Option", "as_ref"]); +/// Returns the string of the function call that creates the temporary. +/// When this function is called, we are reasonably certain that the `ExprKind` is either +/// `Call` or `MethodCall` because we already checked that the expression is not +/// `is_syntactic_place_expr()`. +fn source_of_temporary_value<'a>(expr: &'a Expr<'_>) -> Option<&'a str> { + match expr.peel_borrows().kind { + ExprKind::Call(function, _) => { + if let ExprKind::Path(QPath::Resolved(_, func_path)) = function.kind { + if !func_path.segments.is_empty() { + return Some(func_path.segments[0].ident.name.as_str()); + } + } + if let ExprKind::Path(QPath::TypeRelative(_, func_path_segment)) = function.kind { + return Some(func_path_segment.ident.name.as_str()); + } + None + }, + ExprKind::MethodCall(path_segment, ..) => Some(path_segment.ident.name.as_str()), + _ => None, } - false } diff --git a/clippy_lints/src/methods/str_splitn.rs b/clippy_lints/src/methods/str_splitn.rs index c91be33b1cd..c6d4ef5911e 100644 --- a/clippy_lints/src/methods/str_splitn.rs +++ b/clippy_lints/src/methods/str_splitn.rs @@ -129,7 +129,7 @@ fn check_manual_split_once_indirect( let ctxt = expr.span.ctxt(); let mut parents = cx.tcx.hir().parent_iter(expr.hir_id); if let (_, Node::LetStmt(local)) = parents.next()? - && let PatKind::Binding(BindingMode::MUT, iter_binding_id, iter_ident, None) = local.pat.kind + && let PatKind::Binding(BindingMode::MUT, iter_binding_id, _, None) = local.pat.kind && let (iter_stmt_id, Node::Stmt(_)) = parents.next()? && let (_, Node::Block(enclosing_block)) = parents.next()? && let mut stmts = enclosing_block @@ -162,16 +162,20 @@ fn check_manual_split_once_indirect( UnwrapKind::Unwrap => ".unwrap()", UnwrapKind::QuestionMark => "?", }; - diag.span_suggestion_verbose( - local.span, - format!("try `{r}split_once`"), - format!("let ({lhs}, {rhs}) = {self_snip}.{r}split_once({pat_snip}){unwrap};"), + + // Add a multipart suggestion + diag.multipart_suggestion( + format!("replace with `{r}split_once`"), + vec![ + ( + local.span, + format!("let ({lhs}, {rhs}) = {self_snip}.{r}split_once({pat_snip}){unwrap};"), + ), + (first.span, String::new()), // Remove the first usage + (second.span, String::new()), // Remove the second usage + ], app, ); - - let remove_msg = format!("remove the `{iter_ident}` usages"); - diag.span_suggestion(first.span, remove_msg.clone(), "", app); - diag.span_suggestion(second.span, remove_msg, "", app); }); } diff --git a/clippy_lints/src/methods/unnecessary_iter_cloned.rs b/clippy_lints/src/methods/unnecessary_iter_cloned.rs index 029704882dd..671c189a98e 100644 --- a/clippy_lints/src/methods/unnecessary_iter_cloned.rs +++ b/clippy_lints/src/methods/unnecessary_iter_cloned.rs @@ -6,6 +6,7 @@ use clippy_utils::ty::{get_iterator_item_ty, implements_trait}; use clippy_utils::visitors::for_each_expr_without_closures; use clippy_utils::{can_mut_borrow_both, fn_def_id, get_parent_expr, path_to_local}; use core::ops::ControlFlow; +use itertools::Itertools; use rustc_errors::Applicability; use rustc_hir::def_id::DefId; use rustc_hir::{BindingMode, Expr, ExprKind, Node, PatKind}; @@ -122,14 +123,13 @@ pub fn check_for_loop_iter( } else { Applicability::MachineApplicable }; - diag.span_suggestion(expr.span, "use", snippet.to_owned(), applicability); - if !references_to_binding.is_empty() { - diag.multipart_suggestion( - "remove any references to the binding", - references_to_binding, - applicability, - ); - } + + let combined = references_to_binding + .into_iter() + .chain(vec![(expr.span, snippet.to_owned())]) + .collect_vec(); + + diag.multipart_suggestion("remove any references to the binding", combined, applicability); }, ); return true; diff --git a/clippy_lints/src/methods/unnecessary_sort_by.rs b/clippy_lints/src/methods/unnecessary_sort_by.rs index 603916e06c9..9a45b04d1a6 100644 --- a/clippy_lints/src/methods/unnecessary_sort_by.rs +++ b/clippy_lints/src/methods/unnecessary_sort_by.rs @@ -1,7 +1,7 @@ use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::is_trait_method; use clippy_utils::sugg::Sugg; use clippy_utils::ty::implements_trait; +use clippy_utils::{is_trait_method, std_or_core}; use rustc_errors::Applicability; use rustc_hir::{Closure, Expr, ExprKind, Mutability, Param, Pat, PatKind, Path, PathSegment, QPath}; use rustc_lint::LateContext; @@ -211,8 +211,10 @@ pub(super) fn check<'tcx>( trigger.vec_name, if is_unstable { "_unstable" } else { "" }, trigger.closure_arg, - if trigger.reverse { - format!("std::cmp::Reverse({})", trigger.closure_body) + if let Some(std_or_core) = std_or_core(cx) + && trigger.reverse + { + format!("{}::cmp::Reverse({})", std_or_core, trigger.closure_body) } else { trigger.closure_body.to_string() }, diff --git a/clippy_lints/src/missing_const_for_thread_local.rs b/clippy_lints/src/missing_const_for_thread_local.rs index 9a44a3c980c..e2ca4458eda 100644 --- a/clippy_lints/src/missing_const_for_thread_local.rs +++ b/clippy_lints/src/missing_const_for_thread_local.rs @@ -27,14 +27,12 @@ declare_clippy_lint! { /// /// ### Example /// ```no_run - /// // example code where clippy issues a warning /// thread_local! { /// static BUF: String = String::new(); /// } /// ``` /// Use instead: /// ```no_run - /// // example code which does not raise clippy warning /// thread_local! { /// static BUF: String = const { String::new() }; /// } diff --git a/clippy_lints/src/no_effect.rs b/clippy_lints/src/no_effect.rs index 9e44bb02c56..7feff7e4d3f 100644 --- a/clippy_lints/src/no_effect.rs +++ b/clippy_lints/src/no_effect.rs @@ -8,7 +8,7 @@ use rustc_errors::Applicability; use rustc_hir::def::{DefKind, Res}; use rustc_hir::{ BinOpKind, BlockCheckMode, Expr, ExprKind, HirId, HirIdMap, ItemKind, LocalSource, Node, PatKind, Stmt, StmtKind, - UnsafeSource, StructTailExpr, is_range_literal, + StructTailExpr, UnsafeSource, is_range_literal, }; use rustc_infer::infer::TyCtxtInferExt as _; use rustc_lint::{LateContext, LateLintPass, LintContext}; diff --git a/clippy_lints/src/operators/numeric_arithmetic.rs b/clippy_lints/src/operators/numeric_arithmetic.rs index d369978b8be..2083f2bf628 100644 --- a/clippy_lints/src/operators/numeric_arithmetic.rs +++ b/clippy_lints/src/operators/numeric_arithmetic.rs @@ -43,8 +43,8 @@ impl Context { _ => (), } - let (_, r_ty) = (cx.typeck_results().expr_ty(l), cx.typeck_results().expr_ty(r)); - if r_ty.peel_refs().is_floating_point() && r_ty.peel_refs().is_floating_point() { + let (l_ty, r_ty) = (cx.typeck_results().expr_ty(l), cx.typeck_results().expr_ty(r)); + if l_ty.peel_refs().is_floating_point() && r_ty.peel_refs().is_floating_point() { span_lint(cx, FLOAT_ARITHMETIC, expr.span, "floating-point arithmetic detected"); self.expr_id = Some(expr.hir_id); } diff --git a/clippy_lints/src/precedence.rs b/clippy_lints/src/precedence.rs index 37f5dd5583b..031f0931059 100644 --- a/clippy_lints/src/precedence.rs +++ b/clippy_lints/src/precedence.rs @@ -1,5 +1,6 @@ use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::snippet_with_applicability; +use rustc_ast::ast::BinOpKind::{Add, BitAnd, BitOr, BitXor, Div, Mul, Rem, Shl, Shr, Sub}; use rustc_ast::ast::{BinOpKind, Expr, ExprKind}; use rustc_errors::Applicability; use rustc_lint::{EarlyContext, EarlyLintPass}; @@ -12,6 +13,7 @@ declare_clippy_lint! { /// and suggests to add parentheses. Currently it catches the following: /// * mixed usage of arithmetic and bit shifting/combining operators without /// parentheses + /// * mixed usage of bitmasking and bit shifting operators without parentheses /// /// ### Why is this bad? /// Not everyone knows the precedence of those operators by @@ -20,6 +22,7 @@ declare_clippy_lint! { /// /// ### Example /// * `1 << 2 + 3` equals 32, while `(1 << 2) + 3` equals 7 + /// * `0x2345 & 0xF000 >> 12` equals 5, while `(0x2345 & 0xF000) >> 12` equals 2 #[clippy::version = "pre 1.29.0"] pub PRECEDENCE, complexity, @@ -51,8 +54,13 @@ impl EarlyLintPass for Precedence { return; } let mut applicability = Applicability::MachineApplicable; - match (is_arith_expr(left), is_arith_expr(right)) { - (true, true) => { + match (op, get_bin_opt(left), get_bin_opt(right)) { + ( + BitAnd | BitOr | BitXor, + Some(Shl | Shr | Add | Div | Mul | Rem | Sub), + Some(Shl | Shr | Add | Div | Mul | Rem | Sub), + ) + | (Shl | Shr, Some(Add | Div | Mul | Rem | Sub), Some(Add | Div | Mul | Rem | Sub)) => { let sugg = format!( "({}) {} ({})", snippet_with_applicability(cx, left.span, "..", &mut applicability), @@ -61,7 +69,8 @@ impl EarlyLintPass for Precedence { ); span_sugg(expr, sugg, applicability); }, - (true, false) => { + (BitAnd | BitOr | BitXor, Some(Shl | Shr | Add | Div | Mul | Rem | Sub), _) + | (Shl | Shr, Some(Add | Div | Mul | Rem | Sub), _) => { let sugg = format!( "({}) {} {}", snippet_with_applicability(cx, left.span, "..", &mut applicability), @@ -70,7 +79,8 @@ impl EarlyLintPass for Precedence { ); span_sugg(expr, sugg, applicability); }, - (false, true) => { + (BitAnd | BitOr | BitXor, _, Some(Shl | Shr | Add | Div | Mul | Rem | Sub)) + | (Shl | Shr, _, Some(Add | Div | Mul | Rem | Sub)) => { let sugg = format!( "{} {} ({})", snippet_with_applicability(cx, left.span, "..", &mut applicability), @@ -79,27 +89,20 @@ impl EarlyLintPass for Precedence { ); span_sugg(expr, sugg, applicability); }, - (false, false) => (), + _ => (), } } } } -fn is_arith_expr(expr: &Expr) -> bool { +fn get_bin_opt(expr: &Expr) -> Option<BinOpKind> { match expr.kind { - ExprKind::Binary(Spanned { node: op, .. }, _, _) => is_arith_op(op), - _ => false, + ExprKind::Binary(Spanned { node: op, .. }, _, _) => Some(op), + _ => None, } } #[must_use] fn is_bit_op(op: BinOpKind) -> bool { - use rustc_ast::ast::BinOpKind::{BitAnd, BitOr, BitXor, Shl, Shr}; matches!(op, BitXor | BitAnd | BitOr | Shl | Shr) } - -#[must_use] -fn is_arith_op(op: BinOpKind) -> bool { - use rustc_ast::ast::BinOpKind::{Add, Div, Mul, Rem, Sub}; - matches!(op, Add | Sub | Mul | Div | Rem) -} diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index 77abe7151f0..ffc3b86c502 100644 --- a/clippy_lints/src/question_mark.rs +++ b/clippy_lints/src/question_mark.rs @@ -59,7 +59,7 @@ pub struct QuestionMark { /// As for why we need this in the first place: <https://github.com/rust-lang/rust-clippy/issues/8628> try_block_depth_stack: Vec<u32>, /// Keeps track of the number of inferred return type closures we are inside, to avoid problems - /// with the `Err(x.into())` expansion being ambiguious. + /// with the `Err(x.into())` expansion being ambiguous. inferred_ret_closure_stack: u16, } diff --git a/clippy_lints/src/redundant_slicing.rs b/clippy_lints/src/redundant_slicing.rs index 8e3472b1b5a..7038b19d275 100644 --- a/clippy_lints/src/redundant_slicing.rs +++ b/clippy_lints/src/redundant_slicing.rs @@ -85,7 +85,8 @@ impl<'tcx> LateLintPass<'tcx> for RedundantSlicing { let (expr_ty, expr_ref_count) = peel_middle_ty_refs(cx.typeck_results().expr_ty(expr)); let (indexed_ty, indexed_ref_count) = peel_middle_ty_refs(cx.typeck_results().expr_ty(indexed)); let parent_expr = get_parent_expr(cx, expr); - let needs_parens_for_prefix = parent_expr.is_some_and(|parent| parent.precedence() > ExprPrecedence::Prefix); + let needs_parens_for_prefix = + parent_expr.is_some_and(|parent| parent.precedence() > ExprPrecedence::Prefix); if expr_ty == indexed_ty { if expr_ref_count > indexed_ref_count { diff --git a/clippy_lints/src/shadow.rs b/clippy_lints/src/shadow.rs index 7ae0310b6d9..83199ba0f70 100644 --- a/clippy_lints/src/shadow.rs +++ b/clippy_lints/src/shadow.rs @@ -1,6 +1,9 @@ +use std::ops::ControlFlow; + use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::path_to_local_id; use clippy_utils::source::snippet; -use clippy_utils::visitors::is_local_used; +use clippy_utils::visitors::{Descend, Visitable, for_each_expr}; use rustc_data_structures::fx::FxHashMap; use rustc_hir::def::Res; use rustc_hir::def_id::LocalDefId; @@ -175,9 +178,31 @@ fn is_shadow(cx: &LateContext<'_>, owner: LocalDefId, first: ItemLocalId, second false } +/// Checks if the given local is used, except for in child expression of `except`. +/// +/// This is a version of [`is_local_used`](clippy_utils::visitors::is_local_used), used to +/// implement the fix for <https://github.com/rust-lang/rust-clippy/issues/10780>. +pub fn is_local_used_except<'tcx>( + cx: &LateContext<'tcx>, + visitable: impl Visitable<'tcx>, + id: HirId, + except: Option<HirId>, +) -> bool { + for_each_expr(cx, visitable, |e| { + if except.is_some_and(|it| it == e.hir_id) { + ControlFlow::Continue(Descend::No) + } else if path_to_local_id(e, id) { + ControlFlow::Break(()) + } else { + ControlFlow::Continue(Descend::Yes) + } + }) + .is_some() +} + fn lint_shadow(cx: &LateContext<'_>, pat: &Pat<'_>, shadowed: HirId, span: Span) { let (lint, msg) = match find_init(cx, pat.hir_id) { - Some(expr) if is_self_shadow(cx, pat, expr, shadowed) => { + Some((expr, _)) if is_self_shadow(cx, pat, expr, shadowed) => { let msg = format!( "`{}` is shadowed by itself in `{}`", snippet(cx, pat.span, "_"), @@ -185,7 +210,7 @@ fn lint_shadow(cx: &LateContext<'_>, pat: &Pat<'_>, shadowed: HirId, span: Span) ); (SHADOW_SAME, msg) }, - Some(expr) if is_local_used(cx, expr, shadowed) => { + Some((expr, except)) if is_local_used_except(cx, expr, shadowed, except) => { let msg = format!("`{}` is shadowed", snippet(cx, pat.span, "_")); (SHADOW_REUSE, msg) }, @@ -232,15 +257,32 @@ fn is_self_shadow(cx: &LateContext<'_>, pat: &Pat<'_>, mut expr: &Expr<'_>, hir_ /// Finds the "init" expression for a pattern: `let <pat> = <init>;` (or `if let`) or /// `match <init> { .., <pat> => .., .. }` -fn find_init<'tcx>(cx: &LateContext<'tcx>, hir_id: HirId) -> Option<&'tcx Expr<'tcx>> { - for (_, node) in cx.tcx.hir().parent_iter(hir_id) { +/// +/// For closure arguments passed to a method call, returns the method call, and the `HirId` of the +/// closure (which will later be skipped). This is for <https://github.com/rust-lang/rust-clippy/issues/10780> +fn find_init<'tcx>(cx: &LateContext<'tcx>, hir_id: HirId) -> Option<(&'tcx Expr<'tcx>, Option<HirId>)> { + for (hir_id, node) in cx.tcx.hir().parent_iter(hir_id) { let init = match node { - Node::Arm(_) | Node::Pat(_) => continue, + Node::Arm(_) | Node::Pat(_) | Node::PatField(_) | Node::Param(_) => continue, Node::Expr(expr) => match expr.kind { - ExprKind::Match(e, _, _) | ExprKind::Let(&LetExpr { init: e, .. }) => Some(e), + ExprKind::Match(e, _, _) | ExprKind::Let(&LetExpr { init: e, .. }) => Some((e, None)), + // If we're a closure argument, then a parent call is also an associated item. + ExprKind::Closure(_) => { + if let Some((_, node)) = cx.tcx.hir().parent_iter(hir_id).next() { + match node { + Node::Expr(expr) => match expr.kind { + ExprKind::MethodCall(_, _, _, _) | ExprKind::Call(_, _) => Some((expr, Some(hir_id))), + _ => None, + }, + _ => None, + } + } else { + None + } + }, _ => None, }, - Node::LetStmt(local) => local.init, + Node::LetStmt(local) => local.init.map(|init| (init, None)), _ => None, }; return init; diff --git a/clippy_lints/src/significant_drop_tightening.rs b/clippy_lints/src/significant_drop_tightening.rs index 1a5b958e6a6..c690696aefc 100644 --- a/clippy_lints/src/significant_drop_tightening.rs +++ b/clippy_lints/src/significant_drop_tightening.rs @@ -99,16 +99,10 @@ impl<'tcx> LateLintPass<'tcx> for SignificantDropTightening<'tcx> { snippet(cx, apa.last_bind_ident.span, ".."), ) }; - diag.span_suggestion_verbose( - apa.first_stmt_span, + + diag.multipart_suggestion_verbose( "merge the temporary construction with its single usage", - stmt, - Applicability::MaybeIncorrect, - ); - diag.span_suggestion( - apa.last_stmt_span, - "remove separated single usage", - "", + vec![(apa.first_stmt_span, stmt), (apa.last_stmt_span, String::new())], Applicability::MaybeIncorrect, ); }, diff --git a/clippy_lints/src/single_range_in_vec_init.rs b/clippy_lints/src/single_range_in_vec_init.rs index bb11daecc07..9737b84cdb9 100644 --- a/clippy_lints/src/single_range_in_vec_init.rs +++ b/clippy_lints/src/single_range_in_vec_init.rs @@ -86,7 +86,8 @@ impl LateLintPass<'_> for SingleRangeInVecInit { return; }; - let ExprKind::Struct(QPath::LangItem(lang_item, ..), [start, end], StructTailExpr::None) = inner_expr.kind else { + let ExprKind::Struct(QPath::LangItem(lang_item, ..), [start, end], StructTailExpr::None) = inner_expr.kind + else { return; }; diff --git a/clippy_lints/src/strings.rs b/clippy_lints/src/strings.rs index e09c0706006..2925f355d0b 100644 --- a/clippy_lints/src/strings.rs +++ b/clippy_lints/src/strings.rs @@ -370,12 +370,10 @@ declare_clippy_lint! { /// /// ### Example /// ```no_run - /// // example code where clippy issues a warning /// let _ = "str".to_string(); /// ``` /// Use instead: /// ```no_run - /// // example code which does not raise clippy warning /// let _ = "str".to_owned(); /// ``` #[clippy::version = "pre 1.29.0"] @@ -424,13 +422,11 @@ declare_clippy_lint! { /// /// ### Example /// ```no_run - /// // example code where clippy issues a warning /// let msg = String::from("Hello World"); /// let _ = msg.to_string(); /// ``` /// Use instead: /// ```no_run - /// // example code which does not raise clippy warning /// let msg = String::from("Hello World"); /// let _ = msg.clone(); /// ``` diff --git a/clippy_lints/src/unit_types/let_unit_value.rs b/clippy_lints/src/unit_types/let_unit_value.rs index 0702f6d1e74..d2727968c0c 100644 --- a/clippy_lints/src/unit_types/let_unit_value.rs +++ b/clippy_lints/src/unit_types/let_unit_value.rs @@ -1,6 +1,6 @@ use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::source::snippet_with_context; -use clippy_utils::visitors::{for_each_local_assignment, for_each_value_source, is_local_used}; +use clippy_utils::visitors::{for_each_local_assignment, for_each_value_source}; use core::ops::ControlFlow; use rustc_errors::Applicability; use rustc_hir::def::{DefKind, Res}; @@ -71,25 +71,38 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, local: &'tcx LetStmt<'_>) { local.span, "this let-binding has unit value", |diag| { + let mut suggestions = Vec::new(); + + // Suggest omitting the `let` binding if let Some(expr) = &local.init { let mut app = Applicability::MachineApplicable; let snip = snippet_with_context(cx, expr.span, local.span.ctxt(), "()", &mut app).0; - diag.span_suggestion(local.span, "omit the `let` binding", format!("{snip};"), app); + suggestions.push((local.span, format!("{snip};"))); } - if let PatKind::Binding(_, binding_hir_id, ident, ..) = local.pat.kind + // If this is a binding pattern, we need to add suggestions to remove any usages + // of the variable + if let PatKind::Binding(_, binding_hir_id, ..) = local.pat.kind && let Some(body_id) = cx.enclosing_body.as_ref() - && let body = cx.tcx.hir().body(*body_id) - && is_local_used(cx, body, binding_hir_id) { - let identifier = ident.as_str(); + let body = cx.tcx.hir().body(*body_id); + + // Collect variable usages let mut visitor = UnitVariableCollector::new(binding_hir_id); walk_body(&mut visitor, body); - visitor.spans.into_iter().for_each(|span| { - let msg = - format!("variable `{identifier}` of type `()` can be replaced with explicit `()`"); - diag.span_suggestion(span, msg, "()", Applicability::MachineApplicable); - }); + + // Add suggestions for replacing variable usages + suggestions.extend(visitor.spans.into_iter().map(|span| (span, "()".to_string()))); + } + + // Emit appropriate diagnostic based on whether there are usages of the let binding + if !suggestions.is_empty() { + let message = if suggestions.len() == 1 { + "omit the `let` binding" + } else { + "omit the `let` binding and replace variable usages with `()`" + }; + diag.multipart_suggestion(message, suggestions, Applicability::MachineApplicable); } }, ); diff --git a/clippy_lints/src/unit_types/unit_arg.rs b/clippy_lints/src/unit_types/unit_arg.rs index 0f4bd286145..47d6fe7db76 100644 --- a/clippy_lints/src/unit_types/unit_arg.rs +++ b/clippy_lints/src/unit_types/unit_arg.rs @@ -25,13 +25,13 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { return; } - let (reciever, args) = match expr.kind { + let (receiver, args) = match expr.kind { ExprKind::Call(_, args) => (None, args), ExprKind::MethodCall(_, receiver, args, _) => (Some(receiver), args), _ => return, }; - let args_to_recover = reciever + let args_to_recover = receiver .into_iter() .chain(args) .filter(|arg| { diff --git a/clippy_lints/src/unnecessary_literal_bound.rs b/clippy_lints/src/unnecessary_literal_bound.rs index 80ce6711126..8165a45bc5b 100644 --- a/clippy_lints/src/unnecessary_literal_bound.rs +++ b/clippy_lints/src/unnecessary_literal_bound.rs @@ -17,7 +17,7 @@ declare_clippy_lint! { /// /// ### Why is this bad? /// - /// This leaves the caller unable to use the `&str` as `&'static str`, causing unneccessary allocations or confusion. + /// This leaves the caller unable to use the `&str` as `&'static str`, causing unnecessary allocations or confusion. /// This is also most likely what you meant to write. /// /// ### Example diff --git a/clippy_lints/src/unnecessary_struct_initialization.rs b/clippy_lints/src/unnecessary_struct_initialization.rs index 0a90d31db7e..1df229c330e 100644 --- a/clippy_lints/src/unnecessary_struct_initialization.rs +++ b/clippy_lints/src/unnecessary_struct_initialization.rs @@ -2,7 +2,7 @@ use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::snippet; use clippy_utils::ty::is_copy; use clippy_utils::{get_parent_expr, path_to_local}; -use rustc_hir::{BindingMode, Expr, ExprField, ExprKind, Node, PatKind, Path, QPath, UnOp, StructTailExpr}; +use rustc_hir::{BindingMode, Expr, ExprField, ExprKind, Node, PatKind, Path, QPath, StructTailExpr, UnOp}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::declare_lint_pass; @@ -63,7 +63,9 @@ impl LateLintPass<'_> for UnnecessaryStruct { // all fields match, no base given path.span }, - (Some(path), StructTailExpr::Base(base)) if base_is_suitable(cx, expr, base) && path_matches_base(path, base) => { + (Some(path), StructTailExpr::Base(base)) + if base_is_suitable(cx, expr, base) && path_matches_base(path, base) => + { // all fields match, has base: ensure that the path of the base matches base.span }, diff --git a/clippy_lints/src/useless_conversion.rs b/clippy_lints/src/useless_conversion.rs index 3b05abc546f..7ffab81a544 100644 --- a/clippy_lints/src/useless_conversion.rs +++ b/clippy_lints/src/useless_conversion.rs @@ -1,8 +1,10 @@ use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg, span_lint_and_then}; use clippy_utils::source::{snippet, snippet_with_applicability, snippet_with_context}; -use clippy_utils::sugg::Sugg; +use clippy_utils::sugg::{DiagExt as _, Sugg}; use clippy_utils::ty::{is_copy, is_type_diagnostic_item, same_type_and_consts}; -use clippy_utils::{get_parent_expr, is_trait_method, is_ty_alias, path_to_local}; +use clippy_utils::{ + get_parent_expr, is_inherent_method_call, is_trait_item, is_trait_method, is_ty_alias, path_to_local, +}; use rustc_errors::Applicability; use rustc_hir::def_id::DefId; use rustc_hir::{BindingMode, Expr, ExprKind, HirId, MatchSource, Node, PatKind}; @@ -10,7 +12,7 @@ use rustc_infer::infer::TyCtxtInferExt; use rustc_infer::traits::Obligation; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::traits::ObligationCause; -use rustc_middle::ty::{self, EarlyBinder, GenericArg, GenericArgsRef, Ty, TypeVisitableExt}; +use rustc_middle::ty::{self, AdtDef, EarlyBinder, GenericArg, GenericArgsRef, Ty, TypeVisitableExt}; use rustc_session::impl_lint_pass; use rustc_span::{Span, sym}; use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt; @@ -382,3 +384,50 @@ impl<'tcx> LateLintPass<'tcx> for UselessConversion { } } } + +/// Check if `arg` is a `Into::into` or `From::from` applied to `receiver` to give `expr`, through a +/// higher-order mapping function. +pub fn check_function_application(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, arg: &Expr<'_>) { + if has_eligible_receiver(cx, recv, expr) + && (is_trait_item(cx, arg, sym::Into) || is_trait_item(cx, arg, sym::From)) + && let ty::FnDef(_, args) = cx.typeck_results().expr_ty(arg).kind() + && let &[from_ty, to_ty] = args.into_type_list(cx.tcx).as_slice() + && same_type_and_consts(from_ty, to_ty) + { + span_lint_and_then( + cx, + USELESS_CONVERSION, + expr.span.with_lo(recv.span.hi()), + format!("useless conversion to the same type: `{from_ty}`"), + |diag| { + diag.suggest_remove_item( + cx, + expr.span.with_lo(recv.span.hi()), + "consider removing", + Applicability::MachineApplicable, + ); + }, + ); + } +} + +fn has_eligible_receiver(cx: &LateContext<'_>, recv: &Expr<'_>, expr: &Expr<'_>) -> bool { + let recv_ty = cx.typeck_results().expr_ty(recv); + if is_inherent_method_call(cx, expr) + && let Some(recv_ty_defid) = recv_ty.ty_adt_def().map(AdtDef::did) + { + if let Some(diag_name) = cx.tcx.get_diagnostic_name(recv_ty_defid) + && matches!(diag_name, sym::Option | sym::Result) + { + return true; + } + + if cx.tcx.is_diagnostic_item(sym::ControlFlow, recv_ty_defid) { + return true; + } + } + if is_trait_method(cx, expr, sym::Iterator) { + return true; + } + false +} diff --git a/clippy_lints/src/utils/author.rs b/clippy_lints/src/utils/author.rs index 521bf6a5fed..d2970c93f8e 100644 --- a/clippy_lints/src/utils/author.rs +++ b/clippy_lints/src/utils/author.rs @@ -3,8 +3,8 @@ use rustc_ast::LitIntType; use rustc_ast::ast::{LitFloatType, LitKind}; use rustc_data_structures::fx::FxHashMap; use rustc_hir::{ - self as hir, BindingMode, CaptureBy, Closure, ClosureKind, ConstArg, ConstArgKind, CoroutineKind, - ExprKind, FnRetTy, HirId, Lit, PatKind, QPath, StmtKind, TyKind, StructTailExpr, + self as hir, BindingMode, CaptureBy, Closure, ClosureKind, ConstArg, ConstArgKind, CoroutineKind, ExprKind, + FnRetTy, HirId, Lit, PatKind, QPath, StmtKind, StructTailExpr, TyKind, }; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_session::declare_lint_pass; @@ -625,7 +625,7 @@ impl<'a, 'tcx> PrintVisitor<'a, 'tcx> { }, ExprKind::UnsafeBinderCast(..) => { unimplemented!("unsafe binders are not implemented yet"); - } + }, } } diff --git a/clippy_lints/src/utils/internal_lints/almost_standard_lint_formulation.rs b/clippy_lints/src/utils/internal_lints/almost_standard_lint_formulation.rs index 5483e80f932..bfcce81c498 100644 --- a/clippy_lints/src/utils/internal_lints/almost_standard_lint_formulation.rs +++ b/clippy_lints/src/utils/internal_lints/almost_standard_lint_formulation.rs @@ -1,8 +1,7 @@ use crate::utils::internal_lints::lint_without_lint_pass::is_lint_ref_type; use clippy_utils::diagnostics::span_lint_and_help; use regex::Regex; -use rustc_ast as ast; -use rustc_hir::{Item, ItemKind, Mutability}; +use rustc_hir::{Attribute, Item, ItemKind, Mutability}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::impl_lint_pass; @@ -51,7 +50,7 @@ impl<'tcx> LateLintPass<'tcx> for AlmostStandardFormulation { .hir() .attrs(item.hir_id()) .iter() - .filter_map(|attr| ast::Attribute::doc_str(attr).map(|sym| (sym, attr))); + .filter_map(|attr| Attribute::doc_str(attr).map(|sym| (sym, attr))); if is_lint_ref_type(cx, ty) { for (line, attr) in lines { let cur_line = line.as_str().trim(); diff --git a/clippy_lints/src/utils/internal_lints/interning_defined_symbol.rs b/clippy_lints/src/utils/internal_lints/interning_defined_symbol.rs index 9e400d2391f..e454427adde 100644 --- a/clippy_lints/src/utils/internal_lints/interning_defined_symbol.rs +++ b/clippy_lints/src/utils/internal_lints/interning_defined_symbol.rs @@ -92,7 +92,7 @@ impl<'tcx> LateLintPass<'tcx> for InterningDefinedSymbol { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { if let ExprKind::Call(func, [arg]) = &expr.kind && let ty::FnDef(def_id, _) = cx.typeck_results().expr_ty(func).kind() - && match_def_path(cx, *def_id, &paths::SYMBOL_INTERN) + && cx.tcx.is_diagnostic_item(sym::SymbolIntern, *def_id) && let Some(Constant::Str(arg)) = ConstEvalCtxt::new(cx).eval_simple(arg) && let value = Symbol::intern(&arg).as_u32() && let Some(&def_id) = self.symbol_map.get(&value) diff --git a/clippy_lints/src/utils/internal_lints/lint_without_lint_pass.rs b/clippy_lints/src/utils/internal_lints/lint_without_lint_pass.rs index 496343d82c8..dac1951489c 100644 --- a/clippy_lints/src/utils/internal_lints/lint_without_lint_pass.rs +++ b/clippy_lints/src/utils/internal_lints/lint_without_lint_pass.rs @@ -3,6 +3,7 @@ use clippy_utils::macros::root_macro_call_first_node; use clippy_utils::{is_lint_allowed, match_def_path, paths}; use rustc_ast::ast::LitKind; use rustc_data_structures::fx::{FxIndexMap, FxIndexSet}; +use rustc_hir as hir; use rustc_hir::def::{DefKind, Res}; use rustc_hir::hir_id::CRATE_HIR_ID; use rustc_hir::intravisit::Visitor; @@ -13,7 +14,6 @@ use rustc_session::impl_lint_pass; use rustc_span::source_map::Spanned; use rustc_span::symbol::Symbol; use rustc_span::{Span, sym}; -use {rustc_ast as ast, rustc_hir as hir}; declare_clippy_lint! { /// ### What it does @@ -249,11 +249,11 @@ fn check_invalid_clippy_version_attribute(cx: &LateContext<'_>, item: &'_ Item<' pub(super) fn extract_clippy_version_value(cx: &LateContext<'_>, item: &'_ Item<'_>) -> Option<Symbol> { let attrs = cx.tcx.hir().attrs(item.hir_id()); attrs.iter().find_map(|attr| { - if let ast::AttrKind::Normal(attr_kind) = &attr.kind + if let hir::AttrKind::Normal(attr_kind) = &attr.kind // Identify attribute - && let [tool_name, attr_name] = &attr_kind.item.path.segments[..] - && tool_name.ident.name == sym::clippy - && attr_name.ident.name == sym::version + && let [tool_name, attr_name] = &attr_kind.path.segments[..] + && tool_name.name == sym::clippy + && attr_name.name == sym::version && let Some(version) = attr.value_str() { Some(version) diff --git a/clippy_lints/src/utils/internal_lints/slow_symbol_comparisons.rs b/clippy_lints/src/utils/internal_lints/slow_symbol_comparisons.rs index 3742be0e103..49aad881994 100644 --- a/clippy_lints/src/utils/internal_lints/slow_symbol_comparisons.rs +++ b/clippy_lints/src/utils/internal_lints/slow_symbol_comparisons.rs @@ -1,29 +1,29 @@ use clippy_utils::consts::{ConstEvalCtxt, Constant}; use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::paths; use clippy_utils::source::snippet_with_applicability; use clippy_utils::ty::match_type; -use clippy_utils::{match_function_call, paths}; use rustc_errors::Applicability; use rustc_hir::{BinOpKind, Expr, ExprKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::declare_lint_pass; -use rustc_span::Span; +use rustc_span::{Span, sym}; declare_clippy_lint! { /// ### What it does /// - /// Detects symbol comparision using `Symbol::intern`. + /// Detects symbol comparison using `Symbol::intern`. /// /// ### Why is this bad? /// - /// Comparision via `Symbol::as_str()` is faster if the interned symbols are not reused. + /// Comparison via `Symbol::as_str()` is faster if the interned symbols are not reused. /// /// ### Example /// /// None, see suggestion. pub SLOW_SYMBOL_COMPARISONS, internal, - "detects slow comparisions of symbol" + "detects slow comparisons of symbol" } declare_lint_pass!(SlowSymbolComparisons => [SLOW_SYMBOL_COMPARISONS]); @@ -34,7 +34,12 @@ fn check_slow_comparison<'tcx>( op2: &'tcx Expr<'tcx>, ) -> Option<(Span, String)> { if match_type(cx, cx.typeck_results().expr_ty(op1), &paths::SYMBOL) - && let Some([symbol_name_expr]) = match_function_call(cx, op2, &paths::SYMBOL_INTERN) + && let ExprKind::Call(fun, args) = op2.kind + && let ExprKind::Path(ref qpath) = fun.kind + && cx + .tcx + .is_diagnostic_item(sym::SymbolIntern, cx.qpath_res(qpath, fun.hir_id).opt_def_id()?) + && let [symbol_name_expr] = args && let Some(Constant::Str(symbol_name)) = ConstEvalCtxt::new(cx).eval_simple(symbol_name_expr) { Some((op1.span, symbol_name)) diff --git a/clippy_lints/src/zombie_processes.rs b/clippy_lints/src/zombie_processes.rs index 4a13c10166f..a702e0785a9 100644 --- a/clippy_lints/src/zombie_processes.rs +++ b/clippy_lints/src/zombie_processes.rs @@ -2,13 +2,14 @@ use ControlFlow::{Break, Continue}; use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::{fn_def_id, get_enclosing_block, match_any_def_paths, match_def_path, path_to_local_id, paths}; use rustc_ast::Mutability; +use rustc_ast::visit::visit_opt; use rustc_errors::Applicability; use rustc_hir::intravisit::{Visitor, walk_block, walk_expr, walk_local}; use rustc_hir::{Expr, ExprKind, HirId, LetStmt, Node, PatKind, Stmt, StmtKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::hir::nested_filter; use rustc_session::declare_lint_pass; -use rustc_span::sym; +use rustc_span::{Span, sym}; use std::ops::ControlFlow; declare_clippy_lint! { @@ -22,6 +23,17 @@ declare_clippy_lint! { /// which can eventually lead to resource exhaustion, so it's recommended to call `wait()` in long-running applications. /// Such processes are called "zombie processes". /// + /// To reduce the rate of false positives, if the spawned process is assigned to a binding, the lint actually works the other way around; it + /// conservatively checks that all uses of a variable definitely don't call `wait()` and only then emits a warning. + /// For that reason, a seemingly unrelated use can get called out as calling `wait()` in help messages. + /// + /// ### Control flow + /// If a `wait()` call exists in an if/then block but not in the else block (or there is no else block), + /// then this still gets linted as not calling `wait()` in all code paths. + /// Likewise, when early-returning from the function, `wait()` calls that appear after the return expression + /// are also not accepted. + /// In other words, the `wait()` call must be unconditionally reachable after the spawn expression. + /// /// ### Example /// ```rust /// use std::process::Command; @@ -53,26 +65,47 @@ impl<'tcx> LateLintPass<'tcx> for ZombieProcesses { if let PatKind::Binding(_, local_id, ..) = local.pat.kind && let Some(enclosing_block) = get_enclosing_block(cx, expr.hir_id) => { - let mut vis = WaitFinder::WalkUpTo(cx, local_id); - - // If it does have a `wait()` call, we're done. Don't lint. - if let Break(BreakReason::WaitFound) = walk_block(&mut vis, enclosing_block) { - return; - } + let mut vis = WaitFinder { + cx, + local_id, + state: VisitorState::WalkUpToLocal, + early_return: None, + missing_wait_branch: None, + }; + + let res = ( + walk_block(&mut vis, enclosing_block), + vis.missing_wait_branch, + vis.early_return, + ); + + let cause = match res { + (Break(MaybeWait(wait_span)), _, Some(return_span)) => { + Cause::EarlyReturn { wait_span, return_span } + }, + (Break(MaybeWait(_)), _, None) => return, + (Continue(()), None, _) => Cause::NeverWait, + (Continue(()), Some(MissingWaitBranch::MissingElse { if_span, wait_span }), _) => { + Cause::MissingElse { wait_span, if_span } + }, + (Continue(()), Some(MissingWaitBranch::MissingWaitInBranch { branch_span, wait_span }), _) => { + Cause::MissingWaitInBranch { wait_span, branch_span } + }, + }; // Don't emit a suggestion since the binding is used later - check(cx, expr, false); + check(cx, expr, cause, false); }, Node::LetStmt(&LetStmt { pat, .. }) if let PatKind::Wild = pat.kind => { // `let _ = child;`, also dropped immediately without `wait()`ing - check(cx, expr, true); + check(cx, expr, Cause::NeverWait, true); }, Node::Stmt(&Stmt { kind: StmtKind::Semi(_), .. }) => { // Immediately dropped. E.g. `std::process::Command::new("echo").spawn().unwrap();` - check(cx, expr, true); + check(cx, expr, Cause::NeverWait, true); }, _ => {}, } @@ -80,21 +113,10 @@ impl<'tcx> LateLintPass<'tcx> for ZombieProcesses { } } -enum BreakReason { - WaitFound, - EarlyReturn, -} +struct MaybeWait(Span); /// A visitor responsible for finding a `wait()` call on a local variable. /// -/// Conditional `wait()` calls are assumed to not call wait: -/// ```ignore -/// let mut c = Command::new("").spawn().unwrap(); -/// if true { -/// c.wait(); -/// } -/// ``` -/// /// Note that this visitor does NOT explicitly look for `wait()` calls directly, but rather does the /// inverse -- checking if all uses of the local are either: /// - a field access (`child.{stderr,stdin,stdout}`) @@ -104,43 +126,50 @@ enum BreakReason { /// /// None of these are sufficient to prevent zombie processes. /// Doing it like this means more FNs, but FNs are better than FPs. -/// -/// `return` expressions, conditional or not, short-circuit the visitor because -/// if a `wait()` call hadn't been found at that point, it might never reach one at a later point: -/// ```ignore -/// let mut c = Command::new("").spawn().unwrap(); -/// if true { -/// return; // Break(BreakReason::EarlyReturn) -/// } -/// c.wait(); // this might not be reachable -/// ``` -enum WaitFinder<'a, 'tcx> { - WalkUpTo(&'a LateContext<'tcx>, HirId), - Found(&'a LateContext<'tcx>, HirId), +struct WaitFinder<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + local_id: HirId, + state: VisitorState, + early_return: Option<Span>, + // When joining two if branches where one of them doesn't call `wait()`, stores its span for more targetted help + // messages + missing_wait_branch: Option<MissingWaitBranch>, +} + +#[derive(PartialEq)] +enum VisitorState { + WalkUpToLocal, + LocalFound, +} + +#[derive(Copy, Clone)] +enum MissingWaitBranch { + MissingElse { if_span: Span, wait_span: Span }, + MissingWaitInBranch { branch_span: Span, wait_span: Span }, } impl<'tcx> Visitor<'tcx> for WaitFinder<'_, 'tcx> { type NestedFilter = nested_filter::OnlyBodies; - type Result = ControlFlow<BreakReason>; + type Result = ControlFlow<MaybeWait>; fn visit_local(&mut self, l: &'tcx LetStmt<'tcx>) -> Self::Result { - if let Self::WalkUpTo(cx, local_id) = *self + if self.state == VisitorState::WalkUpToLocal && let PatKind::Binding(_, pat_id, ..) = l.pat.kind - && local_id == pat_id + && self.local_id == pat_id { - *self = Self::Found(cx, local_id); + self.state = VisitorState::LocalFound; } walk_local(self, l) } fn visit_expr(&mut self, ex: &'tcx Expr<'tcx>) -> Self::Result { - let Self::Found(cx, local_id) = *self else { + if self.state != VisitorState::LocalFound { return walk_expr(self, ex); - }; + } - if path_to_local_id(ex, local_id) { - match cx.tcx.parent_hir_node(ex.hir_id) { + if path_to_local_id(ex, self.local_id) { + match self.cx.tcx.parent_hir_node(ex.hir_id) { Node::Stmt(Stmt { kind: StmtKind::Semi(_), .. @@ -148,29 +177,33 @@ impl<'tcx> Visitor<'tcx> for WaitFinder<'_, 'tcx> { Node::Expr(expr) if let ExprKind::Field(..) = expr.kind => {}, Node::Expr(expr) if let ExprKind::AddrOf(_, Mutability::Not, _) = expr.kind => {}, Node::Expr(expr) - if let Some(fn_did) = fn_def_id(cx, expr) - && match_any_def_paths(cx, fn_did, &[&paths::CHILD_ID, &paths::CHILD_KILL]).is_some() => {}, + if let Some(fn_did) = fn_def_id(self.cx, expr) + && match_any_def_paths(self.cx, fn_did, &[&paths::CHILD_ID, &paths::CHILD_KILL]).is_some() => { + }, // Conservatively assume that all other kinds of nodes call `.wait()` somehow. - _ => return Break(BreakReason::WaitFound), + _ => return Break(MaybeWait(ex.span)), } } else { match ex.kind { - ExprKind::Ret(..) => return Break(BreakReason::EarlyReturn), + ExprKind::Ret(e) => { + visit_opt!(self, visit_expr, e); + if self.early_return.is_none() { + self.early_return = Some(ex.span); + } + + return Continue(()); + }, ExprKind::If(cond, then, None) => { walk_expr(self, cond)?; - // A `wait()` call in an if expression with no else is not enough: - // - // let c = spawn(); - // if true { - // c.wait(); - // } - // - // This might not call wait(). However, early returns are propagated, - // because they might lead to a later wait() not being called. - if let Break(BreakReason::EarlyReturn) = walk_expr(self, then) { - return Break(BreakReason::EarlyReturn); + if let Break(MaybeWait(wait_span)) = walk_expr(self, then) + && self.missing_wait_branch.is_none() + { + self.missing_wait_branch = Some(MissingWaitBranch::MissingElse { + if_span: ex.span, + wait_span, + }); } return Continue(()); @@ -179,22 +212,31 @@ impl<'tcx> Visitor<'tcx> for WaitFinder<'_, 'tcx> { ExprKind::If(cond, then, Some(else_)) => { walk_expr(self, cond)?; - #[expect(clippy::unnested_or_patterns)] match (walk_expr(self, then), walk_expr(self, else_)) { - (Continue(()), Continue(())) + (Continue(()), Continue(())) => {}, // `wait()` in one branch but nothing in the other does not count - | (Continue(()), Break(BreakReason::WaitFound)) - | (Break(BreakReason::WaitFound), Continue(())) => {}, - - // `wait()` in both branches is ok - (Break(BreakReason::WaitFound), Break(BreakReason::WaitFound)) => { - return Break(BreakReason::WaitFound); + (Continue(()), Break(MaybeWait(wait_span))) => { + if self.missing_wait_branch.is_none() { + self.missing_wait_branch = Some(MissingWaitBranch::MissingWaitInBranch { + branch_span: ex.span.shrink_to_lo().to(then.span), + wait_span, + }); + } + }, + (Break(MaybeWait(wait_span)), Continue(())) => { + if self.missing_wait_branch.is_none() { + self.missing_wait_branch = Some(MissingWaitBranch::MissingWaitInBranch { + branch_span: then.span.shrink_to_hi().to(else_.span), + wait_span, + }); + } }, - // Propagate early returns in either branch - (Break(BreakReason::EarlyReturn), _) | (_, Break(BreakReason::EarlyReturn)) => { - return Break(BreakReason::EarlyReturn); + // `wait()` in both branches is ok + (Break(MaybeWait(wait_span)), Break(MaybeWait(_))) => { + self.missing_wait_branch = None; + return Break(MaybeWait(wait_span)); }, } @@ -208,8 +250,40 @@ impl<'tcx> Visitor<'tcx> for WaitFinder<'_, 'tcx> { } fn nested_visit_map(&mut self) -> Self::Map { - let (Self::Found(cx, _) | Self::WalkUpTo(cx, _)) = self; - cx.tcx.hir() + self.cx.tcx.hir() + } +} + +#[derive(Copy, Clone)] +enum Cause { + /// No call to `wait()` at all + NeverWait, + /// `wait()` call exists, but not all code paths definitely lead to one due to + /// an early return + EarlyReturn { wait_span: Span, return_span: Span }, + /// `wait()` call exists in some if branches but not this one + MissingWaitInBranch { wait_span: Span, branch_span: Span }, + /// `wait()` call exists in an if/then branch but it is missing an else block + MissingElse { wait_span: Span, if_span: Span }, +} + +impl Cause { + fn message(self) -> &'static str { + match self { + Cause::NeverWait => "spawned process is never `wait()`ed on", + Cause::EarlyReturn { .. } | Cause::MissingWaitInBranch { .. } | Cause::MissingElse { .. } => { + "spawned process is not `wait()`ed on in all code paths" + }, + } + } + + fn fallback_help(self) -> &'static str { + match self { + Cause::NeverWait => "consider calling `.wait()`", + Cause::EarlyReturn { .. } | Cause::MissingWaitInBranch { .. } | Cause::MissingElse { .. } => { + "consider calling `.wait()` in all code paths" + }, + } } } @@ -220,7 +294,7 @@ impl<'tcx> Visitor<'tcx> for WaitFinder<'_, 'tcx> { /// `let _ = <expr that spawns child>;`. /// /// This checks if the program doesn't unconditionally exit after the spawn expression. -fn check<'tcx>(cx: &LateContext<'tcx>, spawn_expr: &'tcx Expr<'tcx>, emit_suggestion: bool) { +fn check<'tcx>(cx: &LateContext<'tcx>, spawn_expr: &'tcx Expr<'tcx>, cause: Cause, emit_suggestion: bool) { let Some(block) = get_enclosing_block(cx, spawn_expr.hir_id) else { return; }; @@ -234,27 +308,46 @@ fn check<'tcx>(cx: &LateContext<'tcx>, spawn_expr: &'tcx Expr<'tcx>, emit_sugges return; } - span_lint_and_then( - cx, - ZOMBIE_PROCESSES, - spawn_expr.span, - "spawned process is never `wait()`ed on", - |diag| { - if emit_suggestion { - diag.span_suggestion( - spawn_expr.span.shrink_to_hi(), - "try", - ".wait()", - Applicability::MaybeIncorrect, + span_lint_and_then(cx, ZOMBIE_PROCESSES, spawn_expr.span, cause.message(), |diag| { + match cause { + Cause::EarlyReturn { wait_span, return_span } => { + diag.span_note( + return_span, + "no `wait()` call exists on the code path to this early return", ); - } else { - diag.note("consider calling `.wait()`"); - } + diag.span_note( + wait_span, + "`wait()` call exists, but it is unreachable due to the early return", + ); + }, + Cause::MissingWaitInBranch { wait_span, branch_span } => { + diag.span_note(branch_span, "`wait()` is not called in this if branch"); + diag.span_note(wait_span, "`wait()` is called in the other branch"); + }, + Cause::MissingElse { if_span, wait_span } => { + diag.span_note( + if_span, + "this if expression has a `wait()` call, but it is missing an else block", + ); + diag.span_note(wait_span, "`wait()` called here"); + }, + Cause::NeverWait => {}, + } + + if emit_suggestion { + diag.span_suggestion( + spawn_expr.span.shrink_to_hi(), + "try", + ".wait()", + Applicability::MaybeIncorrect, + ); + } else { + diag.help(cause.fallback_help()); + } - diag.note("not doing so might leave behind zombie processes") - .note("see https://doc.rust-lang.org/stable/std/process/struct.Child.html#warning"); - }, - ); + diag.note("not doing so might leave behind zombie processes") + .note("see https://doc.rust-lang.org/stable/std/process/struct.Child.html#warning"); + }); } /// Checks if the given expression exits the process. |