diff options
| author | flip1995 <philipp.krones@embecosm.com> | 2020-11-23 13:51:04 +0100 |
|---|---|---|
| committer | flip1995 <philipp.krones@embecosm.com> | 2020-11-23 13:51:04 +0100 |
| commit | dfac9be0827e7b3a6cea641926e217d2497236d7 (patch) | |
| tree | d24a235a1cd051ebd78f637cad1558c43bf9c506 /src/tools/clippy/clippy_lints | |
| parent | 068320b39e3e4839d832b3aa71fa910ba170673b (diff) | |
| parent | 3e7c6dec244539970b593824334876f8b6ed0b18 (diff) | |
| download | rust-dfac9be0827e7b3a6cea641926e217d2497236d7.tar.gz rust-dfac9be0827e7b3a6cea641926e217d2497236d7.zip | |
Merge commit '3e7c6dec244539970b593824334876f8b6ed0b18' into clippyup
Diffstat (limited to 'src/tools/clippy/clippy_lints')
33 files changed, 830 insertions, 324 deletions
diff --git a/src/tools/clippy/clippy_lints/src/assertions_on_constants.rs b/src/tools/clippy/clippy_lints/src/assertions_on_constants.rs index a52f0997d43..62c73dbac48 100644 --- a/src/tools/clippy/clippy_lints/src/assertions_on_constants.rs +++ b/src/tools/clippy/clippy_lints/src/assertions_on_constants.rs @@ -129,7 +129,7 @@ fn match_assert_with_message<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) if let ExprKind::Block(ref block, _) = arms[0].body.kind; if block.stmts.is_empty(); if let Some(block_expr) = &block.expr; - // inner block is optional. unwarp it if it exists, or use the expression as is otherwise. + // inner block is optional. unwrap it if it exists, or use the expression as is otherwise. if let Some(begin_panic_call) = match block_expr.kind { ExprKind::Block(ref inner_block, _) => &inner_block.expr, _ => &block.expr, diff --git a/src/tools/clippy/clippy_lints/src/await_holding_invalid.rs b/src/tools/clippy/clippy_lints/src/await_holding_invalid.rs index fcebb54c6c2..58892024ce2 100644 --- a/src/tools/clippy/clippy_lints/src/await_holding_invalid.rs +++ b/src/tools/clippy/clippy_lints/src/await_holding_invalid.rs @@ -45,7 +45,7 @@ declare_clippy_lint! { /// } /// ``` pub AWAIT_HOLDING_LOCK, - correctness, + pedantic, "Inside an async function, holding a MutexGuard while calling await" } @@ -65,8 +65,8 @@ declare_clippy_lint! { /// use std::cell::RefCell; /// /// async fn foo(x: &RefCell<u32>) { - /// let b = x.borrow_mut()(); - /// *ref += 1; + /// let mut y = x.borrow_mut(); + /// *y += 1; /// bar.await; /// } /// ``` @@ -77,14 +77,14 @@ declare_clippy_lint! { /// /// async fn foo(x: &RefCell<u32>) { /// { - /// let b = x.borrow_mut(); - /// *ref += 1; + /// let mut y = x.borrow_mut(); + /// *y += 1; /// } /// bar.await; /// } /// ``` pub AWAIT_HOLDING_REFCELL_REF, - correctness, + pedantic, "Inside an async function, holding a RefCell ref while calling await" } diff --git a/src/tools/clippy/clippy_lints/src/cargo_common_metadata.rs b/src/tools/clippy/clippy_lints/src/cargo_common_metadata.rs index 76a000157df..0d294761af5 100644 --- a/src/tools/clippy/clippy_lints/src/cargo_common_metadata.rs +++ b/src/tools/clippy/clippy_lints/src/cargo_common_metadata.rs @@ -23,6 +23,21 @@ declare_clippy_lint! { /// [package] /// name = "clippy" /// version = "0.0.212" + /// description = "A bunch of helpful lints to avoid common pitfalls in Rust" + /// repository = "https://github.com/rust-lang/rust-clippy" + /// readme = "README.md" + /// license = "MIT OR Apache-2.0" + /// keywords = ["clippy", "lint", "plugin"] + /// categories = ["development-tools", "development-tools::cargo-plugins"] + /// ``` + /// + /// Should include an authors field like: + /// + /// ```toml + /// # This `Cargo.toml` includes all common metadata + /// [package] + /// name = "clippy" + /// version = "0.0.212" /// authors = ["Someone <someone@rust-lang.org>"] /// description = "A bunch of helpful lints to avoid common pitfalls in Rust" /// repository = "https://github.com/rust-lang/rust-clippy" diff --git a/src/tools/clippy/clippy_lints/src/deprecated_lints.rs b/src/tools/clippy/clippy_lints/src/deprecated_lints.rs index 461c6e31d3e..1c3285ed701 100644 --- a/src/tools/clippy/clippy_lints/src/deprecated_lints.rs +++ b/src/tools/clippy/clippy_lints/src/deprecated_lints.rs @@ -181,3 +181,8 @@ declare_deprecated_lint! { pub TEMPORARY_CSTRING_AS_PTR, "this lint has been uplifted to rustc and is now called `temporary_cstring_as_ptr`" } + +declare_deprecated_lint! { + pub PANIC_PARAMS, + "this lint has been uplifted to rustc and is now called `panic_fmt`" +} diff --git a/src/tools/clippy/clippy_lints/src/len_zero.rs b/src/tools/clippy/clippy_lints/src/len_zero.rs index 8e2f03d6e4e..8842901d90b 100644 --- a/src/tools/clippy/clippy_lints/src/len_zero.rs +++ b/src/tools/clippy/clippy_lints/src/len_zero.rs @@ -69,7 +69,7 @@ declare_clippy_lint! { } declare_clippy_lint! { - /// **What it does:** Checks for comparing to an empty slice such as "" or [],` + /// **What it does:** Checks for comparing to an empty slice such as `""` or `[]`, /// and suggests using `.is_empty()` where applicable. /// /// **Why is this bad?** Some structures can answer `.is_empty()` much faster diff --git a/src/tools/clippy/clippy_lints/src/let_underscore.rs b/src/tools/clippy/clippy_lints/src/let_underscore.rs index ae2f6131b5b..6a5a77f8690 100644 --- a/src/tools/clippy/clippy_lints/src/let_underscore.rs +++ b/src/tools/clippy/clippy_lints/src/let_underscore.rs @@ -5,7 +5,7 @@ use rustc_middle::lint::in_external_macro; use rustc_middle::ty::subst::GenericArgKind; use rustc_session::{declare_lint_pass, declare_tool_lint}; -use crate::utils::{is_must_use_func_call, is_must_use_ty, match_type, paths, span_lint_and_help}; +use crate::utils::{implements_trait, is_must_use_func_call, is_must_use_ty, match_type, paths, span_lint_and_help}; declare_clippy_lint! { /// **What it does:** Checks for `let _ = <expr>` @@ -58,7 +58,48 @@ declare_clippy_lint! { "non-binding let on a synchronization lock" } -declare_lint_pass!(LetUnderscore => [LET_UNDERSCORE_MUST_USE, LET_UNDERSCORE_LOCK]); +declare_clippy_lint! { + /// **What it does:** Checks for `let _ = <expr>` + /// where expr has a type that implements `Drop` + /// + /// **Why is this bad?** This statement immediately drops the initializer + /// expression instead of extending its lifetime to the end of the scope, which + /// is often not intended. To extend the expression's lifetime to the end of the + /// scope, use an underscore-prefixed name instead (i.e. _var). If you want to + /// explicitly drop the expression, `std::mem::drop` conveys your intention + /// better and is less error-prone. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// Bad: + /// ```rust,ignore + /// struct Droppable; + /// impl Drop for Droppable { + /// fn drop(&mut self) {} + /// } + /// { + /// let _ = Droppable; + /// // ^ dropped here + /// /* more code */ + /// } + /// ``` + /// + /// Good: + /// ```rust,ignore + /// { + /// let _droppable = Droppable; + /// /* more code */ + /// // dropped at end of scope + /// } + /// ``` + pub LET_UNDERSCORE_DROP, + pedantic, + "non-binding let on a type that implements `Drop`" +} + +declare_lint_pass!(LetUnderscore => [LET_UNDERSCORE_MUST_USE, LET_UNDERSCORE_LOCK, LET_UNDERSCORE_DROP]); const SYNC_GUARD_PATHS: [&[&str]; 3] = [ &paths::MUTEX_GUARD, @@ -84,6 +125,15 @@ impl<'tcx> LateLintPass<'tcx> for LetUnderscore { GenericArgKind::Lifetime(_) | GenericArgKind::Const(_) => false, }); + let implements_drop = cx.tcx.lang_items().drop_trait().map_or(false, |drop_trait| + init_ty.walk().any(|inner| match inner.unpack() { + GenericArgKind::Type(inner_ty) => { + implements_trait(cx, inner_ty, drop_trait, &[]) + }, + + GenericArgKind::Lifetime(_) | GenericArgKind::Const(_) => false, + }) + ); if contains_sync_guard { span_lint_and_help( cx, @@ -94,6 +144,16 @@ impl<'tcx> LateLintPass<'tcx> for LetUnderscore { "consider using an underscore-prefixed named \ binding or dropping explicitly with `std::mem::drop`" ) + } else if implements_drop { + span_lint_and_help( + cx, + LET_UNDERSCORE_DROP, + local.span, + "non-binding `let` on a type that implements `Drop`", + None, + "consider using an underscore-prefixed named \ + binding or dropping explicitly with `std::mem::drop`" + ) } else if is_must_use_ty(cx, cx.typeck_results().expr_ty(init)) { span_lint_and_help( cx, diff --git a/src/tools/clippy/clippy_lints/src/lib.rs b/src/tools/clippy/clippy_lints/src/lib.rs index 19bf67d80c4..7e8cbd00c22 100644 --- a/src/tools/clippy/clippy_lints/src/lib.rs +++ b/src/tools/clippy/clippy_lints/src/lib.rs @@ -323,6 +323,7 @@ mod unicode; mod unit_return_expecting_ord; mod unnamed_address; mod unnecessary_sort_by; +mod unnecessary_wraps; mod unnested_or_patterns; mod unsafe_removed_from_name; mod unused_io_amount; @@ -495,6 +496,10 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: "clippy::temporary_cstring_as_ptr", "this lint has been uplifted to rustc and is now called `temporary_cstring_as_ptr`", ); + store.register_removed( + "clippy::panic_params", + "this lint has been uplifted to rustc and is now called `panic_fmt`", + ); // end deprecated lints, do not remove this comment, it’s used in `update_lints` // begin register lints, do not remove this comment, it’s used in `update_lints` @@ -622,6 +627,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &len_zero::LEN_WITHOUT_IS_EMPTY, &len_zero::LEN_ZERO, &let_if_seq::USELESS_LET_IF_SEQ, + &let_underscore::LET_UNDERSCORE_DROP, &let_underscore::LET_UNDERSCORE_LOCK, &let_underscore::LET_UNDERSCORE_MUST_USE, &lifetimes::EXTRA_UNUSED_LIFETIMES, @@ -831,6 +837,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &stable_sort_primitive::STABLE_SORT_PRIMITIVE, &strings::STRING_ADD, &strings::STRING_ADD_ASSIGN, + &strings::STRING_FROM_UTF8_AS_BYTES, &strings::STRING_LIT_AS_BYTES, &suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL, &suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL, @@ -889,6 +896,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &unnamed_address::FN_ADDRESS_COMPARISONS, &unnamed_address::VTABLE_ADDRESS_COMPARISONS, &unnecessary_sort_by::UNNECESSARY_SORT_BY, + &unnecessary_wraps::UNNECESSARY_WRAPS, &unnested_or_patterns::UNNESTED_OR_PATTERNS, &unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME, &unused_io_amount::UNUSED_IO_AMOUNT, @@ -1061,6 +1069,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box redundant_clone::RedundantClone); store.register_late_pass(|| box slow_vector_initialization::SlowVectorInit); store.register_late_pass(|| box unnecessary_sort_by::UnnecessarySortBy); + store.register_late_pass(|| box unnecessary_wraps::UnnecessaryWraps); store.register_late_pass(|| box types::RefToMut); store.register_late_pass(|| box assertions_on_constants::AssertionsOnConstants); store.register_late_pass(|| box missing_const_for_fn::MissingConstForFn); @@ -1215,6 +1224,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_group(true, "clippy::pedantic", Some("clippy_pedantic"), vec![ LintId::of(&attrs::INLINE_ALWAYS), + LintId::of(&await_holding_invalid::AWAIT_HOLDING_LOCK), + LintId::of(&await_holding_invalid::AWAIT_HOLDING_REFCELL_REF), LintId::of(&bit_mask::VERBOSE_BIT_MASK), LintId::of(&checked_conversions::CHECKED_CONVERSIONS), LintId::of(&copies::SAME_FUNCTIONS_IN_IF_CONDITION), @@ -1238,6 +1249,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&infinite_iter::MAYBE_INFINITE_ITER), LintId::of(&items_after_statements::ITEMS_AFTER_STATEMENTS), LintId::of(&large_stack_arrays::LARGE_STACK_ARRAYS), + LintId::of(&let_underscore::LET_UNDERSCORE_DROP), LintId::of(&literal_representation::LARGE_DIGIT_GROUPS), LintId::of(&literal_representation::UNREADABLE_LITERAL), LintId::of(&loops::EXPLICIT_INTO_ITER_LOOP), @@ -1317,8 +1329,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&attrs::MISMATCHED_TARGET_OS), LintId::of(&attrs::UNKNOWN_CLIPPY_LINTS), LintId::of(&attrs::USELESS_ATTRIBUTE), - LintId::of(&await_holding_invalid::AWAIT_HOLDING_LOCK), - LintId::of(&await_holding_invalid::AWAIT_HOLDING_REFCELL_REF), LintId::of(&bit_mask::BAD_BIT_MASK), LintId::of(&bit_mask::INEFFECTIVE_BIT_MASK), LintId::of(&blacklisted_name::BLACKLISTED_NAME), @@ -1525,6 +1535,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS), LintId::of(&slow_vector_initialization::SLOW_VECTOR_INITIALIZATION), LintId::of(&stable_sort_primitive::STABLE_SORT_PRIMITIVE), + LintId::of(&strings::STRING_FROM_UTF8_AS_BYTES), LintId::of(&suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL), LintId::of(&suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL), LintId::of(&swap::ALMOST_SWAPPED), @@ -1565,6 +1576,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&unnamed_address::FN_ADDRESS_COMPARISONS), LintId::of(&unnamed_address::VTABLE_ADDRESS_COMPARISONS), LintId::of(&unnecessary_sort_by::UNNECESSARY_SORT_BY), + LintId::of(&unnecessary_wraps::UNNECESSARY_WRAPS), LintId::of(&unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME), LintId::of(&unused_io_amount::UNUSED_IO_AMOUNT), LintId::of(&unused_unit::UNUSED_UNIT), @@ -1749,6 +1761,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&reference::DEREF_ADDROF), LintId::of(&reference::REF_IN_DEREF), LintId::of(&repeat_once::REPEAT_ONCE), + LintId::of(&strings::STRING_FROM_UTF8_AS_BYTES), LintId::of(&swap::MANUAL_SWAP), LintId::of(&temporary_assignment::TEMPORARY_ASSIGNMENT), LintId::of(&transmute::CROSSPOINTER_TRANSMUTE), @@ -1767,6 +1780,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&types::UNNECESSARY_CAST), LintId::of(&types::VEC_BOX), LintId::of(&unnecessary_sort_by::UNNECESSARY_SORT_BY), + LintId::of(&unnecessary_wraps::UNNECESSARY_WRAPS), LintId::of(&unwrap::UNNECESSARY_UNWRAP), LintId::of(&useless_conversion::USELESS_CONVERSION), LintId::of(&zero_div_zero::ZERO_DIVIDED_BY_ZERO), @@ -1779,8 +1793,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&attrs::DEPRECATED_SEMVER), LintId::of(&attrs::MISMATCHED_TARGET_OS), LintId::of(&attrs::USELESS_ATTRIBUTE), - LintId::of(&await_holding_invalid::AWAIT_HOLDING_LOCK), - LintId::of(&await_holding_invalid::AWAIT_HOLDING_REFCELL_REF), LintId::of(&bit_mask::BAD_BIT_MASK), LintId::of(&bit_mask::INEFFECTIVE_BIT_MASK), LintId::of(&booleans::LOGIC_BUG), diff --git a/src/tools/clippy/clippy_lints/src/loops.rs b/src/tools/clippy/clippy_lints/src/loops.rs index 5a4264f9b5c..0d31e9cfc3d 100644 --- a/src/tools/clippy/clippy_lints/src/loops.rs +++ b/src/tools/clippy/clippy_lints/src/loops.rs @@ -4,10 +4,10 @@ use crate::utils::sugg::Sugg; use crate::utils::usage::{is_unused, mutated_variables}; use crate::utils::{ contains_name, get_enclosing_block, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait, - indent_of, is_integer_const, is_no_std_crate, is_refutable, is_type_diagnostic_item, last_path_segment, - match_trait_method, match_type, match_var, multispan_sugg, qpath_res, single_segment_path, snippet, - snippet_with_applicability, snippet_with_macro_callsite, span_lint, span_lint_and_help, span_lint_and_sugg, - span_lint_and_then, sugg, SpanlessEq, + indent_of, is_in_panic_handler, is_integer_const, is_no_std_crate, is_refutable, is_type_diagnostic_item, + last_path_segment, match_trait_method, match_type, match_var, multispan_sugg, qpath_res, single_segment_path, + snippet, snippet_with_applicability, snippet_with_macro_callsite, span_lint, span_lint_and_help, + span_lint_and_sugg, span_lint_and_then, sugg, SpanlessEq, }; use if_chain::if_chain; use rustc_ast::ast; @@ -543,17 +543,15 @@ impl<'tcx> LateLintPass<'tcx> for Loops { // (also matches an explicit "match" instead of "if let") // (even if the "match" or "if let" is used for declaration) if let ExprKind::Loop(ref block, _, LoopSource::Loop) = expr.kind { - // also check for empty `loop {}` statements - // TODO(issue #6161): Enable for no_std crates (outside of #[panic_handler]) - if block.stmts.is_empty() && block.expr.is_none() && !is_no_std_crate(cx.tcx.hir().krate()) { - span_lint_and_help( - cx, - EMPTY_LOOP, - expr.span, - "empty `loop {}` wastes CPU cycles", - None, - "You should either use `panic!()` or add `std::thread::sleep(..);` to the loop body.", - ); + // also check for empty `loop {}` statements, skipping those in #[panic_handler] + if block.stmts.is_empty() && block.expr.is_none() && !is_in_panic_handler(cx, expr) { + let msg = "empty `loop {}` wastes CPU cycles"; + let help = if is_no_std_crate(cx.tcx.hir().krate()) { + "you should either use `panic!()` or add a call pausing or sleeping the thread to the loop body" + } else { + "you should either use `panic!()` or add `std::thread::sleep(..);` to the loop body" + }; + span_lint_and_help(cx, EMPTY_LOOP, expr.span, msg, None, help); } // extract the expression from the first statement (if any) in a block diff --git a/src/tools/clippy/clippy_lints/src/manual_async_fn.rs b/src/tools/clippy/clippy_lints/src/manual_async_fn.rs index 864d1ea87f5..7b3b450ef93 100644 --- a/src/tools/clippy/clippy_lints/src/manual_async_fn.rs +++ b/src/tools/clippy/clippy_lints/src/manual_async_fn.rs @@ -1,5 +1,5 @@ use crate::utils::paths::FUTURE_FROM_GENERATOR; -use crate::utils::{match_function_call, snippet_block, snippet_opt, span_lint_and_then}; +use crate::utils::{match_function_call, position_before_rarrow, snippet_block, snippet_opt, span_lint_and_then}; use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir::intravisit::FnKind; @@ -69,7 +69,7 @@ impl<'tcx> LateLintPass<'tcx> for ManualAsyncFn { |diag| { if_chain! { if let Some(header_snip) = snippet_opt(cx, header_span); - if let Some(ret_pos) = header_snip.rfind("->"); + if let Some(ret_pos) = position_before_rarrow(header_snip.clone()); if let Some((ret_sugg, ret_snip)) = suggested_ret(cx, output); then { let help = format!("make the function `async` and {}", ret_sugg); @@ -194,7 +194,7 @@ fn suggested_ret(cx: &LateContext<'_>, output: &Ty<'_>) -> Option<(&'static str, }, _ => { let sugg = "return the output of the future directly"; - snippet_opt(cx, output.span).map(|snip| (sugg, format!("-> {}", snip))) + snippet_opt(cx, output.span).map(|snip| (sugg, format!(" -> {}", snip))) }, } } diff --git a/src/tools/clippy/clippy_lints/src/map_clone.rs b/src/tools/clippy/clippy_lints/src/map_clone.rs index 034cd99a9be..220240acb7a 100644 --- a/src/tools/clippy/clippy_lints/src/map_clone.rs +++ b/src/tools/clippy/clippy_lints/src/map_clone.rs @@ -8,13 +8,15 @@ use rustc_hir as hir; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::mir::Mutability; use rustc_middle::ty; +use rustc_middle::ty::adjustment::Adjust; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::symbol::Ident; use rustc_span::{sym, Span}; declare_clippy_lint! { - /// **What it does:** Checks for usage of `iterator.map(|x| x.clone())` and suggests - /// `iterator.cloned()` instead + /// **What it does:** Checks for usage of `map(|x| x.clone())` or + /// dereferencing closures for `Copy` types, on `Iterator` or `Option`, + /// and suggests `cloned()` or `copied()` instead /// /// **Why is this bad?** Readability, this can be written more concisely /// @@ -75,14 +77,19 @@ impl<'tcx> LateLintPass<'tcx> for MapClone { } } }, - hir::ExprKind::MethodCall(ref method, _, ref obj, _) => { - if ident_eq(name, &obj[0]) && method.ident.as_str() == "clone" - && match_trait_method(cx, closure_expr, &paths::CLONE_TRAIT) { - - let obj_ty = cx.typeck_results().expr_ty(&obj[0]); - if let ty::Ref(_, ty, _) = obj_ty.kind() { - let copy = is_copy(cx, ty); - lint(cx, e.span, args[0].span, copy); + hir::ExprKind::MethodCall(ref method, _, [obj], _) => if_chain! { + if ident_eq(name, obj) && method.ident.name == sym::clone; + if match_trait_method(cx, closure_expr, &paths::CLONE_TRAIT); + // no autoderefs + if !cx.typeck_results().expr_adjustments(obj).iter() + .any(|a| matches!(a.kind, Adjust::Deref(Some(..)))); + then { + let obj_ty = cx.typeck_results().expr_ty(obj); + if let ty::Ref(_, ty, mutability) = obj_ty.kind() { + if matches!(mutability, Mutability::Not) { + let copy = is_copy(cx, ty); + lint(cx, e.span, args[0].span, copy); + } } else { lint_needless_cloning(cx, e.span, args[0].span); } diff --git a/src/tools/clippy/clippy_lints/src/methods/bind_instead_of_map.rs b/src/tools/clippy/clippy_lints/src/methods/bind_instead_of_map.rs index ae37942e55a..540a1484a85 100644 --- a/src/tools/clippy/clippy_lints/src/methods/bind_instead_of_map.rs +++ b/src/tools/clippy/clippy_lints/src/methods/bind_instead_of_map.rs @@ -1,14 +1,12 @@ use super::{contains_return, BIND_INSTEAD_OF_MAP}; use crate::utils::{ in_macro, match_qpath, match_type, method_calls, multispan_sugg_with_applicability, paths, remove_blocks, snippet, - snippet_with_macro_callsite, span_lint_and_sugg, span_lint_and_then, + snippet_with_macro_callsite, span_lint_and_sugg, span_lint_and_then, visitors::find_all_ret_expressions, }; use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir as hir; -use rustc_hir::intravisit::{self, Visitor}; use rustc_lint::LateContext; -use rustc_middle::hir::map::Map; use rustc_span::Span; pub(crate) struct OptionAndThenSome; @@ -193,124 +191,3 @@ pub(crate) trait BindInsteadOfMap { } } } - -/// returns `true` if expr contains match expr desugared from try -fn contains_try(expr: &hir::Expr<'_>) -> bool { - struct TryFinder { - found: bool, - } - - impl<'hir> intravisit::Visitor<'hir> for TryFinder { - type Map = Map<'hir>; - - fn nested_visit_map(&mut self) -> intravisit::NestedVisitorMap<Self::Map> { - intravisit::NestedVisitorMap::None - } - - fn visit_expr(&mut self, expr: &'hir hir::Expr<'hir>) { - if self.found { - return; - } - match expr.kind { - hir::ExprKind::Match(_, _, hir::MatchSource::TryDesugar) => self.found = true, - _ => intravisit::walk_expr(self, expr), - } - } - } - - let mut visitor = TryFinder { found: false }; - visitor.visit_expr(expr); - visitor.found -} - -fn find_all_ret_expressions<'hir, F>(_cx: &LateContext<'_>, expr: &'hir hir::Expr<'hir>, callback: F) -> bool -where - F: FnMut(&'hir hir::Expr<'hir>) -> bool, -{ - struct RetFinder<F> { - in_stmt: bool, - failed: bool, - cb: F, - } - - struct WithStmtGuarg<'a, F> { - val: &'a mut RetFinder<F>, - prev_in_stmt: bool, - } - - impl<F> RetFinder<F> { - fn inside_stmt(&mut self, in_stmt: bool) -> WithStmtGuarg<'_, F> { - let prev_in_stmt = std::mem::replace(&mut self.in_stmt, in_stmt); - WithStmtGuarg { - val: self, - prev_in_stmt, - } - } - } - - impl<F> std::ops::Deref for WithStmtGuarg<'_, F> { - type Target = RetFinder<F>; - - fn deref(&self) -> &Self::Target { - self.val - } - } - - impl<F> std::ops::DerefMut for WithStmtGuarg<'_, F> { - fn deref_mut(&mut self) -> &mut Self::Target { - self.val - } - } - - impl<F> Drop for WithStmtGuarg<'_, F> { - fn drop(&mut self) { - self.val.in_stmt = self.prev_in_stmt; - } - } - - impl<'hir, F: FnMut(&'hir hir::Expr<'hir>) -> bool> intravisit::Visitor<'hir> for RetFinder<F> { - type Map = Map<'hir>; - - fn nested_visit_map(&mut self) -> intravisit::NestedVisitorMap<Self::Map> { - intravisit::NestedVisitorMap::None - } - - fn visit_stmt(&mut self, stmt: &'hir hir::Stmt<'_>) { - intravisit::walk_stmt(&mut *self.inside_stmt(true), stmt) - } - - fn visit_expr(&mut self, expr: &'hir hir::Expr<'_>) { - if self.failed { - return; - } - if self.in_stmt { - match expr.kind { - hir::ExprKind::Ret(Some(expr)) => self.inside_stmt(false).visit_expr(expr), - _ => intravisit::walk_expr(self, expr), - } - } else { - match expr.kind { - hir::ExprKind::Match(cond, arms, _) => { - self.inside_stmt(true).visit_expr(cond); - for arm in arms { - self.visit_expr(arm.body); - } - }, - hir::ExprKind::Block(..) => intravisit::walk_expr(self, expr), - hir::ExprKind::Ret(Some(expr)) => self.visit_expr(expr), - _ => self.failed |= !(self.cb)(expr), - } - } - } - } - - !contains_try(expr) && { - let mut ret_finder = RetFinder { - in_stmt: false, - failed: false, - cb: callback, - }; - ret_finder.visit_expr(expr); - !ret_finder.failed - } -} diff --git a/src/tools/clippy/clippy_lints/src/methods/mod.rs b/src/tools/clippy/clippy_lints/src/methods/mod.rs index 30f4c0c56d2..fa174404365 100644 --- a/src/tools/clippy/clippy_lints/src/methods/mod.rs +++ b/src/tools/clippy/clippy_lints/src/methods/mod.rs @@ -515,11 +515,11 @@ declare_clippy_lint! { } declare_clippy_lint! { - /// **What it does:** Checks for an iterator search (such as `find()`, + /// **What it does:** Checks for an iterator or string search (such as `find()`, /// `position()`, or `rposition()`) followed by a call to `is_some()`. /// /// **Why is this bad?** Readability, this can be written more concisely as - /// `_.any(_)`. + /// `_.any(_)` or `_.contains(_)`. /// /// **Known problems:** None. /// @@ -535,7 +535,7 @@ declare_clippy_lint! { /// ``` pub SEARCH_IS_SOME, complexity, - "using an iterator search followed by `is_some()`, which is more succinctly expressed as a call to `any()`" + "using an iterator or string search followed by `is_some()`, which is more succinctly expressed as a call to `any()` or `contains()`" } declare_clippy_lint! { @@ -1351,7 +1351,7 @@ declare_clippy_lint! { } declare_clippy_lint! { - /// **What it does:** Checks for usage of `_.map(_).collect::<Result<(),_>()`. + /// **What it does:** Checks for usage of `_.map(_).collect::<Result<(), _>()`. /// /// **Why is this bad?** Using `try_for_each` instead is more readable and idiomatic. /// @@ -1797,12 +1797,20 @@ fn lint_or_fun_call<'tcx>( cx: &LateContext<'tcx>, name: &str, method_span: Span, - fun_span: Span, self_expr: &hir::Expr<'_>, arg: &'tcx hir::Expr<'_>, - or_has_args: bool, span: Span, + // None if lambda is required + fun_span: Option<Span>, ) { + // (path, fn_has_argument, methods, suffix) + static KNOW_TYPES: [(&[&str], bool, &[&str], &str); 4] = [ + (&paths::BTREEMAP_ENTRY, false, &["or_insert"], "with"), + (&paths::HASHMAP_ENTRY, false, &["or_insert"], "with"), + (&paths::OPTION, false, &["map_or", "ok_or", "or", "unwrap_or"], "else"), + (&paths::RESULT, true, &["or", "unwrap_or"], "else"), + ]; + if let hir::ExprKind::MethodCall(ref path, _, ref args, _) = &arg.kind { if path.ident.as_str() == "len" { let ty = cx.typeck_results().expr_ty(&args[0]).peel_refs(); @@ -1818,16 +1826,8 @@ fn lint_or_fun_call<'tcx>( } } - // (path, fn_has_argument, methods, suffix) - let know_types: &[(&[_], _, &[_], _)] = &[ - (&paths::BTREEMAP_ENTRY, false, &["or_insert"], "with"), - (&paths::HASHMAP_ENTRY, false, &["or_insert"], "with"), - (&paths::OPTION, false, &["map_or", "ok_or", "or", "unwrap_or"], "else"), - (&paths::RESULT, true, &["or", "unwrap_or"], "else"), - ]; - if_chain! { - if know_types.iter().any(|k| k.2.contains(&name)); + if KNOW_TYPES.iter().any(|k| k.2.contains(&name)); if is_lazyness_candidate(cx, arg); if !contains_return(&arg); @@ -1835,15 +1835,23 @@ fn lint_or_fun_call<'tcx>( let self_ty = cx.typeck_results().expr_ty(self_expr); if let Some(&(_, fn_has_arguments, poss, suffix)) = - know_types.iter().find(|&&i| match_type(cx, self_ty, i.0)); + KNOW_TYPES.iter().find(|&&i| match_type(cx, self_ty, i.0)); if poss.contains(&name); then { - let sugg: Cow<'_, _> = match (fn_has_arguments, !or_has_args) { - (true, _) => format!("|_| {}", snippet_with_macro_callsite(cx, arg.span, "..")).into(), - (false, false) => format!("|| {}", snippet_with_macro_callsite(cx, arg.span, "..")).into(), - (false, true) => snippet_with_macro_callsite(cx, fun_span, ".."), + let sugg: Cow<'_, str> = { + let (snippet_span, use_lambda) = match (fn_has_arguments, fun_span) { + (false, Some(fun_span)) => (fun_span, false), + _ => (arg.span, true), + }; + let snippet = snippet_with_macro_callsite(cx, snippet_span, ".."); + if use_lambda { + let l_arg = if fn_has_arguments { "_" } else { "" }; + format!("|{}| {}", l_arg, snippet).into() + } else { + snippet + } }; let span_replace_word = method_span.with_hi(span.hi()); span_lint_and_sugg( @@ -1864,28 +1872,13 @@ fn lint_or_fun_call<'tcx>( hir::ExprKind::Call(ref fun, ref or_args) => { let or_has_args = !or_args.is_empty(); if !check_unwrap_or_default(cx, name, fun, &args[0], &args[1], or_has_args, expr.span) { - check_general_case( - cx, - name, - method_span, - fun.span, - &args[0], - &args[1], - or_has_args, - expr.span, - ); + let fun_span = if or_has_args { None } else { Some(fun.span) }; + check_general_case(cx, name, method_span, &args[0], &args[1], expr.span, fun_span); } }, - hir::ExprKind::MethodCall(_, span, ref or_args, _) => check_general_case( - cx, - name, - method_span, - span, - &args[0], - &args[1], - !or_args.is_empty(), - expr.span, - ), + hir::ExprKind::Index(..) | hir::ExprKind::MethodCall(..) => { + check_general_case(cx, name, method_span, &args[0], &args[1], expr.span, None); + }, _ => {}, } } @@ -3048,6 +3041,7 @@ fn lint_flat_map_identity<'tcx>( } /// lint searching an Iterator followed by `is_some()` +/// or calling `find()` on a string followed by `is_some()` fn lint_search_is_some<'tcx>( cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, @@ -3059,10 +3053,10 @@ fn lint_search_is_some<'tcx>( // lint if caller of search is an Iterator if match_trait_method(cx, &is_some_args[0], &paths::ITERATOR) { let msg = format!( - "called `is_some()` after searching an `Iterator` with {}. This is more succinctly \ - expressed by calling `any()`.", + "called `is_some()` after searching an `Iterator` with `{}`", search_method ); + let hint = "this is more succinctly expressed by calling `any()`"; let search_snippet = snippet(cx, search_args[1].span, ".."); if search_snippet.lines().count() <= 1 { // suggest `any(|x| ..)` instead of `any(|&x| ..)` for `find(|&x| ..).is_some()` @@ -3090,7 +3084,7 @@ fn lint_search_is_some<'tcx>( SEARCH_IS_SOME, method_span.with_hi(expr.span.hi()), &msg, - "try this", + "use `any()` instead", format!( "any({})", any_search_snippet.as_ref().map_or(&*search_snippet, String::as_str) @@ -3098,7 +3092,36 @@ fn lint_search_is_some<'tcx>( Applicability::MachineApplicable, ); } else { - span_lint(cx, SEARCH_IS_SOME, expr.span, &msg); + span_lint_and_help(cx, SEARCH_IS_SOME, expr.span, &msg, None, hint); + } + } + // lint if `find()` is called by `String` or `&str` + else if search_method == "find" { + let is_string_or_str_slice = |e| { + let self_ty = cx.typeck_results().expr_ty(e).peel_refs(); + if is_type_diagnostic_item(cx, self_ty, sym!(string_type)) { + true + } else { + *self_ty.kind() == ty::Str + } + }; + if_chain! { + if is_string_or_str_slice(&search_args[0]); + if is_string_or_str_slice(&search_args[1]); + then { + let msg = "called `is_some()` after calling `find()` on a string"; + let mut applicability = Applicability::MachineApplicable; + let find_arg = snippet_with_applicability(cx, search_args[1].span, "..", &mut applicability); + span_lint_and_sugg( + cx, + SEARCH_IS_SOME, + method_span.with_hi(expr.span.hi()), + msg, + "use `contains()` instead", + format!("contains({})", find_arg), + applicability, + ); + } } } } @@ -3901,21 +3924,24 @@ fn lint_from_iter(cx: &LateContext<'_>, expr: &hir::Expr<'_>, args: &[hir::Expr< let ty = cx.typeck_results().expr_ty(expr); let arg_ty = cx.typeck_results().expr_ty(&args[0]); - let from_iter_id = get_trait_def_id(cx, &paths::FROM_ITERATOR).unwrap(); - let iter_id = get_trait_def_id(cx, &paths::ITERATOR).unwrap(); + if_chain! { + if let Some(from_iter_id) = get_trait_def_id(cx, &paths::FROM_ITERATOR); + if let Some(iter_id) = get_trait_def_id(cx, &paths::ITERATOR); - if implements_trait(cx, ty, from_iter_id, &[]) && implements_trait(cx, arg_ty, iter_id, &[]) { - // `expr` implements `FromIterator` trait - let iter_expr = snippet(cx, args[0].span, ".."); - span_lint_and_sugg( - cx, - FROM_ITER_INSTEAD_OF_COLLECT, - expr.span, - "usage of `FromIterator::from_iter`", - "use `.collect()` instead of `::from_iter()`", - format!("{}.collect()", iter_expr), - Applicability::MaybeIncorrect, - ); + if implements_trait(cx, ty, from_iter_id, &[]) && implements_trait(cx, arg_ty, iter_id, &[]); + then { + // `expr` implements `FromIterator` trait + let iter_expr = snippet(cx, args[0].span, ".."); + span_lint_and_sugg( + cx, + FROM_ITER_INSTEAD_OF_COLLECT, + expr.span, + "usage of `FromIterator::from_iter`", + "use `.collect()` instead of `::from_iter()`", + format!("{}.collect()", iter_expr), + Applicability::MaybeIncorrect, + ); + } } } diff --git a/src/tools/clippy/clippy_lints/src/methods/unnecessary_lazy_eval.rs b/src/tools/clippy/clippy_lints/src/methods/unnecessary_lazy_eval.rs index cde89983a26..a867bdb326d 100644 --- a/src/tools/clippy/clippy_lints/src/methods/unnecessary_lazy_eval.rs +++ b/src/tools/clippy/clippy_lints/src/methods/unnecessary_lazy_eval.rs @@ -33,6 +33,17 @@ pub(super) fn lint<'tcx>( } else { "unnecessary closure used to substitute value for `Result::Err`" }; + let applicability = if body + .params + .iter() + // bindings are checked to be unused above + .all(|param| matches!(param.pat.kind, hir::PatKind::Binding(..) | hir::PatKind::Wild)) + { + Applicability::MachineApplicable + } else { + // replacing the lambda may break type inference + Applicability::MaybeIncorrect + }; span_lint_and_sugg( cx, @@ -46,7 +57,7 @@ pub(super) fn lint<'tcx>( simplify_using, snippet(cx, body_expr.span, ".."), ), - Applicability::MachineApplicable, + applicability, ); } } diff --git a/src/tools/clippy/clippy_lints/src/mut_key.rs b/src/tools/clippy/clippy_lints/src/mut_key.rs index 9cceecc785a..11044e0c2fb 100644 --- a/src/tools/clippy/clippy_lints/src/mut_key.rs +++ b/src/tools/clippy/clippy_lints/src/mut_key.rs @@ -89,11 +89,7 @@ fn check_sig<'tcx>(cx: &LateContext<'tcx>, item_hir_id: hir::HirId, decl: &hir:: for (hir_ty, ty) in decl.inputs.iter().zip(fn_sig.inputs().skip_binder().iter()) { check_ty(cx, hir_ty.span, ty); } - check_ty( - cx, - decl.output.span(), - cx.tcx.erase_late_bound_regions(fn_sig.output()), - ); + check_ty(cx, decl.output.span(), cx.tcx.erase_late_bound_regions(fn_sig.output())); } // We want to lint 1. sets or maps with 2. not immutable key types and 3. no unerased diff --git a/src/tools/clippy/clippy_lints/src/non_copy_const.rs b/src/tools/clippy/clippy_lints/src/non_copy_const.rs index 7b662eae775..6b0d198edcf 100644 --- a/src/tools/clippy/clippy_lints/src/non_copy_const.rs +++ b/src/tools/clippy/clippy_lints/src/non_copy_const.rs @@ -5,11 +5,15 @@ use std::ptr; use rustc_hir::def::{DefKind, Res}; -use rustc_hir::{Expr, ExprKind, ImplItem, ImplItemKind, Item, ItemKind, Node, TraitItem, TraitItemKind, UnOp}; +use rustc_hir::def_id::DefId; +use rustc_hir::{ + BodyId, Expr, ExprKind, HirId, ImplItem, ImplItemKind, Item, ItemKind, Node, TraitItem, TraitItemKind, UnOp, +}; use rustc_infer::traits::specialization_graph; use rustc_lint::{LateContext, LateLintPass, Lint}; +use rustc_middle::mir::interpret::{ConstValue, ErrorHandled}; use rustc_middle::ty::adjustment::Adjust; -use rustc_middle::ty::{AssocKind, Ty}; +use rustc_middle::ty::{self, AssocKind, Const, Ty}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::{InnerSpan, Span, DUMMY_SP}; use rustc_typeck::hir_ty_to_ty; @@ -36,14 +40,17 @@ declare_clippy_lint! { /// `std::sync::ONCE_INIT` constant). In this case the use of `const` is legit, /// and this lint should be suppressed. /// - /// When an enum has variants with interior mutability, use of its non interior mutable - /// variants can generate false positives. See issue - /// [#3962](https://github.com/rust-lang/rust-clippy/issues/3962) + /// Even though the lint avoids triggering on a constant whose type has enums that have variants + /// with interior mutability, and its value uses non interior mutable variants (see + /// [#3962](https://github.com/rust-lang/rust-clippy/issues/3962) and + /// [#3825](https://github.com/rust-lang/rust-clippy/issues/3825) for examples); + /// it complains about associated constants without default values only based on its types; + /// which might not be preferable. + /// There're other enums plus associated constants cases that the lint cannot handle. /// /// Types that have underlying or potential interior mutability trigger the lint whether /// the interior mutable field is used or not. See issues /// [#5812](https://github.com/rust-lang/rust-clippy/issues/5812) and - /// [#3825](https://github.com/rust-lang/rust-clippy/issues/3825) /// /// **Example:** /// ```rust @@ -105,6 +112,79 @@ declare_clippy_lint! { "referencing `const` with interior mutability" } +fn is_unfrozen<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool { + // Ignore types whose layout is unknown since `is_freeze` reports every generic types as `!Freeze`, + // making it indistinguishable from `UnsafeCell`. i.e. it isn't a tool to prove a type is + // 'unfrozen'. However, this code causes a false negative in which + // a type contains a layout-unknown type, but also a unsafe cell like `const CELL: Cell<T>`. + // Yet, it's better than `ty.has_type_flags(TypeFlags::HAS_TY_PARAM | TypeFlags::HAS_PROJECTION)` + // since it works when a pointer indirection involves (`Cell<*const T>`). + // Making up a `ParamEnv` where every generic params and assoc types are `Freeze`is another option; + // but I'm not sure whether it's a decent way, if possible. + cx.tcx.layout_of(cx.param_env.and(ty)).is_ok() && !ty.is_freeze(cx.tcx.at(DUMMY_SP), cx.param_env) +} + +fn is_value_unfrozen_raw<'tcx>( + cx: &LateContext<'tcx>, + result: Result<ConstValue<'tcx>, ErrorHandled>, + ty: Ty<'tcx>, +) -> bool { + fn inner<'tcx>(cx: &LateContext<'tcx>, val: &'tcx Const<'tcx>) -> bool { + match val.ty.kind() { + // the fact that we have to dig into every structs to search enums + // leads us to the point checking `UnsafeCell` directly is the only option. + ty::Adt(ty_def, ..) if Some(ty_def.did) == cx.tcx.lang_items().unsafe_cell_type() => true, + ty::Array(..) | ty::Adt(..) | ty::Tuple(..) => { + let val = cx.tcx.destructure_const(cx.param_env.and(val)); + val.fields.iter().any(|field| inner(cx, field)) + }, + _ => false, + } + } + + result.map_or_else( + |err| { + // Consider `TooGeneric` cases as being unfrozen. + // This causes a false positive where an assoc const whose type is unfrozen + // have a value that is a frozen variant with a generic param (an example is + // `declare_interior_mutable_const::enums::BothOfCellAndGeneric::GENERIC_VARIANT`). + // However, it prevents a number of false negatives that is, I think, important: + // 1. assoc consts in trait defs referring to consts of themselves + // (an example is `declare_interior_mutable_const::traits::ConcreteTypes::ANOTHER_ATOMIC`). + // 2. a path expr referring to assoc consts whose type is doesn't have + // any frozen variants in trait defs (i.e. without substitute for `Self`). + // (e.g. borrowing `borrow_interior_mutable_const::trait::ConcreteTypes::ATOMIC`) + // 3. similar to the false positive above; + // but the value is an unfrozen variant, or the type has no enums. (An example is + // `declare_interior_mutable_const::enums::BothOfCellAndGeneric::UNFROZEN_VARIANT` + // and `declare_interior_mutable_const::enums::BothOfCellAndGeneric::NO_ENUM`). + // One might be able to prevent these FNs correctly, and replace this with `false`; + // e.g. implementing `has_frozen_variant` described above, and not running this function + // when the type doesn't have any frozen variants would be the 'correct' way for the 2nd + // case (that actually removes another suboptimal behavior (I won't say 'false positive') where, + // similar to 2., but with the a frozen variant) (e.g. borrowing + // `borrow_interior_mutable_const::enums::AssocConsts::TO_BE_FROZEN_VARIANT`). + // I chose this way because unfrozen enums as assoc consts are rare (or, hopefully, none). + err == ErrorHandled::TooGeneric + }, + |val| inner(cx, Const::from_value(cx.tcx, val, ty)), + ) +} + +fn is_value_unfrozen_poly<'tcx>(cx: &LateContext<'tcx>, body_id: BodyId, ty: Ty<'tcx>) -> bool { + let result = cx.tcx.const_eval_poly(body_id.hir_id.owner.to_def_id()); + is_value_unfrozen_raw(cx, result, ty) +} + +fn is_value_unfrozen_expr<'tcx>(cx: &LateContext<'tcx>, hir_id: HirId, def_id: DefId, ty: Ty<'tcx>) -> bool { + let substs = cx.typeck_results().node_substs(hir_id); + + let result = cx + .tcx + .const_eval_resolve(cx.param_env, ty::WithOptConstParam::unknown(def_id), substs, None, None); + is_value_unfrozen_raw(cx, result, ty) +} + #[derive(Copy, Clone)] enum Source { Item { item: Span }, @@ -130,19 +210,7 @@ impl Source { } } -fn verify_ty_bound<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, source: Source) { - // Ignore types whose layout is unknown since `is_freeze` reports every generic types as `!Freeze`, - // making it indistinguishable from `UnsafeCell`. i.e. it isn't a tool to prove a type is - // 'unfrozen'. However, this code causes a false negative in which - // a type contains a layout-unknown type, but also a unsafe cell like `const CELL: Cell<T>`. - // Yet, it's better than `ty.has_type_flags(TypeFlags::HAS_TY_PARAM | TypeFlags::HAS_PROJECTION)` - // since it works when a pointer indirection involves (`Cell<*const T>`). - // Making up a `ParamEnv` where every generic params and assoc types are `Freeze`is another option; - // but I'm not sure whether it's a decent way, if possible. - if cx.tcx.layout_of(cx.param_env.and(ty)).is_err() || ty.is_freeze(cx.tcx.at(DUMMY_SP), cx.param_env) { - return; - } - +fn lint(cx: &LateContext<'_>, source: Source) { let (lint, msg, span) = source.lint(); span_lint_and_then(cx, lint, span, msg, |diag| { if span.from_expansion() { @@ -165,24 +233,44 @@ declare_lint_pass!(NonCopyConst => [DECLARE_INTERIOR_MUTABLE_CONST, BORROW_INTER impl<'tcx> LateLintPass<'tcx> for NonCopyConst { fn check_item(&mut self, cx: &LateContext<'tcx>, it: &'tcx Item<'_>) { - if let ItemKind::Const(hir_ty, ..) = &it.kind { + if let ItemKind::Const(hir_ty, body_id) = it.kind { let ty = hir_ty_to_ty(cx.tcx, hir_ty); - verify_ty_bound(cx, ty, Source::Item { item: it.span }); + + if is_unfrozen(cx, ty) && is_value_unfrozen_poly(cx, body_id, ty) { + lint(cx, Source::Item { item: it.span }); + } } } fn check_trait_item(&mut self, cx: &LateContext<'tcx>, trait_item: &'tcx TraitItem<'_>) { - if let TraitItemKind::Const(hir_ty, ..) = &trait_item.kind { + if let TraitItemKind::Const(hir_ty, body_id_opt) = &trait_item.kind { let ty = hir_ty_to_ty(cx.tcx, hir_ty); + // Normalize assoc types because ones originated from generic params // bounded other traits could have their bound. let normalized = cx.tcx.normalize_erasing_regions(cx.param_env, ty); - verify_ty_bound(cx, normalized, Source::Assoc { item: trait_item.span }); + if is_unfrozen(cx, normalized) + // When there's no default value, lint it only according to its type; + // in other words, lint consts whose value *could* be unfrozen, not definitely is. + // This feels inconsistent with how the lint treats generic types, + // which avoids linting types which potentially become unfrozen. + // One could check whether a unfrozen type have a *frozen variant* + // (like `body_id_opt.map_or_else(|| !has_frozen_variant(...), ...)`), + // and do the same as the case of generic types at impl items. + // Note that it isn't sufficient to check if it has an enum + // since all of that enum's variants can be unfrozen: + // i.e. having an enum doesn't necessary mean a type has a frozen variant. + // And, implementing it isn't a trivial task; it'll probably end up + // re-implementing the trait predicate evaluation specific to `Freeze`. + && body_id_opt.map_or(true, |body_id| is_value_unfrozen_poly(cx, body_id, normalized)) + { + lint(cx, Source::Assoc { item: trait_item.span }); + } } } fn check_impl_item(&mut self, cx: &LateContext<'tcx>, impl_item: &'tcx ImplItem<'_>) { - if let ImplItemKind::Const(hir_ty, ..) = &impl_item.kind { + if let ImplItemKind::Const(hir_ty, body_id) = &impl_item.kind { let item_hir_id = cx.tcx.hir().get_parent_node(impl_item.hir_id); let item = cx.tcx.hir().expect_item(item_hir_id); @@ -209,16 +297,23 @@ impl<'tcx> LateLintPass<'tcx> for NonCopyConst { ), )) .is_err(); + // If there were a function like `has_frozen_variant` described above, + // we should use here as a frozen variant is a potential to be frozen + // similar to unknown layouts. + // e.g. `layout_of(...).is_err() || has_frozen_variant(...);` then { let ty = hir_ty_to_ty(cx.tcx, hir_ty); let normalized = cx.tcx.normalize_erasing_regions(cx.param_env, ty); - verify_ty_bound( - cx, - normalized, - Source::Assoc { - item: impl_item.span, - }, - ); + if is_unfrozen(cx, normalized) + && is_value_unfrozen_poly(cx, *body_id, normalized) + { + lint( + cx, + Source::Assoc { + item: impl_item.span, + }, + ); + } } } }, @@ -226,7 +321,10 @@ impl<'tcx> LateLintPass<'tcx> for NonCopyConst { let ty = hir_ty_to_ty(cx.tcx, hir_ty); // Normalize assoc types originated from generic params. let normalized = cx.tcx.normalize_erasing_regions(cx.param_env, ty); - verify_ty_bound(cx, normalized, Source::Assoc { item: impl_item.span }); + + if is_unfrozen(cx, ty) && is_value_unfrozen_poly(cx, *body_id, normalized) { + lint(cx, Source::Assoc { item: impl_item.span }); + } }, _ => (), } @@ -241,8 +339,8 @@ impl<'tcx> LateLintPass<'tcx> for NonCopyConst { } // Make sure it is a const item. - match qpath_res(cx, qpath, expr.hir_id) { - Res::Def(DefKind::Const | DefKind::AssocConst, _) => {}, + let item_def_id = match qpath_res(cx, qpath, expr.hir_id) { + Res::Def(DefKind::Const | DefKind::AssocConst, did) => did, _ => return, }; @@ -319,7 +417,9 @@ impl<'tcx> LateLintPass<'tcx> for NonCopyConst { cx.typeck_results().expr_ty(dereferenced_expr) }; - verify_ty_bound(cx, ty, Source::Expr { expr: expr.span }); + if is_unfrozen(cx, ty) && is_value_unfrozen_expr(cx, expr.hir_id, item_def_id, ty) { + lint(cx, Source::Expr { expr: expr.span }); + } } } } diff --git a/src/tools/clippy/clippy_lints/src/non_expressive_names.rs b/src/tools/clippy/clippy_lints/src/non_expressive_names.rs index 6b175490cc8..5b42b61fcde 100644 --- a/src/tools/clippy/clippy_lints/src/non_expressive_names.rs +++ b/src/tools/clippy/clippy_lints/src/non_expressive_names.rs @@ -1,7 +1,5 @@ use crate::utils::{span_lint, span_lint_and_then}; -use rustc_ast::ast::{ - Arm, AssocItem, AssocItemKind, Attribute, Block, FnDecl, Item, ItemKind, Local, Pat, PatKind, -}; +use rustc_ast::ast::{Arm, AssocItem, AssocItemKind, Attribute, Block, FnDecl, Item, ItemKind, Local, Pat, PatKind}; use rustc_ast::visit::{walk_block, walk_expr, walk_pat, Visitor}; use rustc_lint::{EarlyContext, EarlyLintPass}; use rustc_middle::lint::in_external_macro; diff --git a/src/tools/clippy/clippy_lints/src/panic_unimplemented.rs b/src/tools/clippy/clippy_lints/src/panic_unimplemented.rs index 8b10d071647..31b03ecd101 100644 --- a/src/tools/clippy/clippy_lints/src/panic_unimplemented.rs +++ b/src/tools/clippy/clippy_lints/src/panic_unimplemented.rs @@ -73,7 +73,7 @@ declare_lint_pass!(PanicUnimplemented => [UNIMPLEMENTED, UNREACHABLE, TODO, PANI impl<'tcx> LateLintPass<'tcx> for PanicUnimplemented { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if let Some(_) = match_panic_call(cx, expr) { + if match_panic_call(cx, expr).is_some() { let span = get_outer_span(expr); if is_expn_of(expr.span, "unimplemented").is_some() { span_lint( diff --git a/src/tools/clippy/clippy_lints/src/ranges.rs b/src/tools/clippy/clippy_lints/src/ranges.rs index 79e9a56af9a..4b514bbd42c 100644 --- a/src/tools/clippy/clippy_lints/src/ranges.rs +++ b/src/tools/clippy/clippy_lints/src/ranges.rs @@ -222,13 +222,14 @@ fn check_possible_range_contains(cx: &LateContext<'_>, op: BinOpKind, l: &Expr<' let name = snippet_with_applicability(cx, name_span, "_", &mut applicability); let lo = snippet_with_applicability(cx, l_span, "_", &mut applicability); let hi = snippet_with_applicability(cx, u_span, "_", &mut applicability); + let space = if lo.ends_with('.') { " " } else { "" }; span_lint_and_sugg( cx, MANUAL_RANGE_CONTAINS, span, &format!("manual `{}::contains` implementation", range_type), "use", - format!("({}{}{}).contains(&{})", lo, range_op, hi, name), + format!("({}{}{}{}).contains(&{})", lo, space, range_op, hi, name), applicability, ); } else if !combine_and && ord == Some(lord) { @@ -251,13 +252,14 @@ fn check_possible_range_contains(cx: &LateContext<'_>, op: BinOpKind, l: &Expr<' let name = snippet_with_applicability(cx, name_span, "_", &mut applicability); let lo = snippet_with_applicability(cx, l_span, "_", &mut applicability); let hi = snippet_with_applicability(cx, u_span, "_", &mut applicability); + let space = if lo.ends_with('.') { " " } else { "" }; span_lint_and_sugg( cx, MANUAL_RANGE_CONTAINS, span, &format!("manual `!{}::contains` implementation", range_type), "use", - format!("!({}{}{}).contains(&{})", lo, range_op, hi, name), + format!("!({}{}{}{}).contains(&{})", lo, space, range_op, hi, name), applicability, ); } diff --git a/src/tools/clippy/clippy_lints/src/redundant_clone.rs b/src/tools/clippy/clippy_lints/src/redundant_clone.rs index 602facbe062..f0e507105a6 100644 --- a/src/tools/clippy/clippy_lints/src/redundant_clone.rs +++ b/src/tools/clippy/clippy_lints/src/redundant_clone.rs @@ -320,11 +320,11 @@ fn find_stmt_assigns_to<'tcx>( match (by_ref, &*rvalue) { (true, mir::Rvalue::Ref(_, _, place)) | (false, mir::Rvalue::Use(mir::Operand::Copy(place))) => { - base_local_and_movability(cx, mir, *place) + Some(base_local_and_movability(cx, mir, *place)) }, (false, mir::Rvalue::Ref(_, _, place)) => { if let [mir::ProjectionElem::Deref] = place.as_ref().projection { - base_local_and_movability(cx, mir, *place) + Some(base_local_and_movability(cx, mir, *place)) } else { None } @@ -341,7 +341,7 @@ fn base_local_and_movability<'tcx>( cx: &LateContext<'tcx>, mir: &mir::Body<'tcx>, place: mir::Place<'tcx>, -) -> Option<(mir::Local, CannotMoveOut)> { +) -> (mir::Local, CannotMoveOut) { use rustc_middle::mir::PlaceRef; // Dereference. You cannot move things out from a borrowed value. @@ -362,7 +362,7 @@ fn base_local_and_movability<'tcx>( && !is_copy(cx, mir::Place::ty_from(local, projection, &mir.local_decls, cx.tcx).ty); } - Some((local, deref || field || slice)) + (local, deref || field || slice) } struct LocalUseVisitor { diff --git a/src/tools/clippy/clippy_lints/src/reference.rs b/src/tools/clippy/clippy_lints/src/reference.rs index 3fda00403c6..efe3237990d 100644 --- a/src/tools/clippy/clippy_lints/src/reference.rs +++ b/src/tools/clippy/clippy_lints/src/reference.rs @@ -1,9 +1,10 @@ -use crate::utils::{in_macro, snippet_with_applicability, span_lint_and_sugg}; +use crate::utils::{in_macro, snippet_opt, snippet_with_applicability, span_lint_and_sugg}; use if_chain::if_chain; -use rustc_ast::ast::{Expr, ExprKind, UnOp}; +use rustc_ast::ast::{Expr, ExprKind, Mutability, UnOp}; use rustc_errors::Applicability; use rustc_lint::{EarlyContext, EarlyLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::BytePos; declare_clippy_lint! { /// **What it does:** Checks for usage of `*&` and `*&mut` in expressions. @@ -42,19 +43,55 @@ impl EarlyLintPass for DerefAddrOf { fn check_expr(&mut self, cx: &EarlyContext<'_>, e: &Expr) { if_chain! { if let ExprKind::Unary(UnOp::Deref, ref deref_target) = e.kind; - if let ExprKind::AddrOf(_, _, ref addrof_target) = without_parens(deref_target).kind; + if let ExprKind::AddrOf(_, ref mutability, ref addrof_target) = without_parens(deref_target).kind; if !in_macro(addrof_target.span); then { let mut applicability = Applicability::MachineApplicable; - span_lint_and_sugg( - cx, - DEREF_ADDROF, - e.span, - "immediately dereferencing a reference", - "try this", - format!("{}", snippet_with_applicability(cx, addrof_target.span, "_", &mut applicability)), - applicability, - ); + let sugg = if e.span.from_expansion() { + if let Ok(macro_source) = cx.sess.source_map().span_to_snippet(e.span) { + // Remove leading whitespace from the given span + // e.g: ` $visitor` turns into `$visitor` + let trim_leading_whitespaces = |span| { + snippet_opt(cx, span).and_then(|snip| { + #[allow(clippy::cast_possible_truncation)] + snip.find(|c: char| !c.is_whitespace()).map(|pos| { + span.lo() + BytePos(pos as u32) + }) + }).map_or(span, |start_no_whitespace| e.span.with_lo(start_no_whitespace)) + }; + + let mut generate_snippet = |pattern: &str| { + #[allow(clippy::cast_possible_truncation)] + macro_source.rfind(pattern).map(|pattern_pos| { + let rpos = pattern_pos + pattern.len(); + let span_after_ref = e.span.with_lo(BytePos(e.span.lo().0 + rpos as u32)); + let span = trim_leading_whitespaces(span_after_ref); + snippet_with_applicability(cx, span, "_", &mut applicability) + }) + }; + + if *mutability == Mutability::Mut { + generate_snippet("mut") + } else { + generate_snippet("&") + } + } else { + Some(snippet_with_applicability(cx, e.span, "_", &mut applicability)) + } + } else { + Some(snippet_with_applicability(cx, addrof_target.span, "_", &mut applicability)) + }; + if let Some(sugg) = sugg { + span_lint_and_sugg( + cx, + DEREF_ADDROF, + e.span, + "immediately dereferencing a reference", + "try this", + sugg.to_string(), + applicability, + ); + } } } } diff --git a/src/tools/clippy/clippy_lints/src/regex.rs b/src/tools/clippy/clippy_lints/src/regex.rs index 95594e38c9e..d06ab143482 100644 --- a/src/tools/clippy/clippy_lints/src/regex.rs +++ b/src/tools/clippy/clippy_lints/src/regex.rs @@ -11,7 +11,7 @@ use std::convert::TryFrom; declare_clippy_lint! { /// **What it does:** Checks [regex](https://crates.io/crates/regex) creation - /// (with `Regex::new`,`RegexBuilder::new` or `RegexSet::new`) for correct + /// (with `Regex::new`, `RegexBuilder::new`, or `RegexSet::new`) for correct /// regex syntax. /// /// **Why is this bad?** This will lead to a runtime panic. @@ -29,7 +29,7 @@ declare_clippy_lint! { declare_clippy_lint! { /// **What it does:** Checks for trivial [regex](https://crates.io/crates/regex) - /// creation (with `Regex::new`, `RegexBuilder::new` or `RegexSet::new`). + /// creation (with `Regex::new`, `RegexBuilder::new`, or `RegexSet::new`). /// /// **Why is this bad?** Matching the regex can likely be replaced by `==` or /// `str::starts_with`, `str::ends_with` or `std::contains` or other `str` diff --git a/src/tools/clippy/clippy_lints/src/strings.rs b/src/tools/clippy/clippy_lints/src/strings.rs index 0dd2da949c4..ede37624f71 100644 --- a/src/tools/clippy/clippy_lints/src/strings.rs +++ b/src/tools/clippy/clippy_lints/src/strings.rs @@ -1,5 +1,5 @@ use rustc_errors::Applicability; -use rustc_hir::{BinOpKind, Expr, ExprKind}; +use rustc_hir::{BinOpKind, BorrowKind, Expr, ExprKind, LangItem, QPath}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; use rustc_session::{declare_lint_pass, declare_tool_lint}; @@ -9,7 +9,10 @@ use rustc_span::sym; use if_chain::if_chain; use crate::utils::SpanlessEq; -use crate::utils::{get_parent_expr, is_allowed, is_type_diagnostic_item, span_lint, span_lint_and_sugg}; +use crate::utils::{ + get_parent_expr, is_allowed, is_type_diagnostic_item, match_function_call, method_calls, paths, span_lint, + span_lint_and_sugg, +}; declare_clippy_lint! { /// **What it does:** Checks for string appends of the form `x = x + y` (without @@ -174,10 +177,30 @@ fn is_add(cx: &LateContext<'_>, src: &Expr<'_>, target: &Expr<'_>) -> bool { } } +declare_clippy_lint! { + /// **What it does:** Check if the string is transformed to byte array and casted back to string. + /// + /// **Why is this bad?** It's unnecessary, the string can be used directly. + /// + /// **Known problems:** None + /// + /// **Example:** + /// ```rust + /// let _ = std::str::from_utf8(&"Hello World!".as_bytes()[6..11]).unwrap(); + /// ``` + /// could be written as + /// ```rust + /// let _ = &"Hello World!"[6..11]; + /// ``` + pub STRING_FROM_UTF8_AS_BYTES, + complexity, + "casting string slices to byte slices and back" +} + // Max length a b"foo" string can take const MAX_LENGTH_BYTE_STRING_LIT: usize = 32; -declare_lint_pass!(StringLitAsBytes => [STRING_LIT_AS_BYTES]); +declare_lint_pass!(StringLitAsBytes => [STRING_LIT_AS_BYTES, STRING_FROM_UTF8_AS_BYTES]); impl<'tcx> LateLintPass<'tcx> for StringLitAsBytes { fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { @@ -185,6 +208,45 @@ impl<'tcx> LateLintPass<'tcx> for StringLitAsBytes { use rustc_ast::LitKind; if_chain! { + // Find std::str::converts::from_utf8 + if let Some(args) = match_function_call(cx, e, &paths::STR_FROM_UTF8); + + // Find string::as_bytes + if let ExprKind::AddrOf(BorrowKind::Ref, _, ref args) = args[0].kind; + if let ExprKind::Index(ref left, ref right) = args.kind; + let (method_names, expressions, _) = method_calls(left, 1); + if method_names.len() == 1; + if expressions.len() == 1; + if expressions[0].len() == 1; + if method_names[0] == sym!(as_bytes); + + // Check for slicer + if let ExprKind::Struct(ref path, _, _) = right.kind; + if let QPath::LangItem(LangItem::Range, _) = path; + + then { + let mut applicability = Applicability::MachineApplicable; + let string_expression = &expressions[0][0]; + + let snippet_app = snippet_with_applicability( + cx, + string_expression.span, "..", + &mut applicability, + ); + + span_lint_and_sugg( + cx, + STRING_FROM_UTF8_AS_BYTES, + e.span, + "calling a slice of `as_bytes()` with `from_utf8` should be not necessary", + "try", + format!("Some(&{}[{}])", snippet_app, snippet(cx, right.span, "..")), + applicability + ) + } + } + + if_chain! { if let ExprKind::MethodCall(path, _, args, _) = &e.kind; if path.ident.name == sym!(as_bytes); if let ExprKind::Lit(lit) = &args[0].kind; diff --git a/src/tools/clippy/clippy_lints/src/try_err.rs b/src/tools/clippy/clippy_lints/src/try_err.rs index 6f6b6999bf0..73e3a04aec9 100644 --- a/src/tools/clippy/clippy_lints/src/try_err.rs +++ b/src/tools/clippy/clippy_lints/src/try_err.rs @@ -1,6 +1,6 @@ use crate::utils::{ - is_type_diagnostic_item, match_def_path, match_qpath, paths, snippet, snippet_with_macro_callsite, - span_lint_and_sugg, + differing_macro_contexts, in_macro, is_type_diagnostic_item, match_def_path, match_qpath, paths, snippet, + snippet_with_macro_callsite, span_lint_and_sugg, }; use if_chain::if_chain; use rustc_errors::Applicability; @@ -92,8 +92,11 @@ impl<'tcx> LateLintPass<'tcx> for TryErr { }; let expr_err_ty = cx.typeck_results().expr_ty(err_arg); + let differing_contexts = differing_macro_contexts(expr.span, err_arg.span); - let origin_snippet = if err_arg.span.from_expansion() { + let origin_snippet = if in_macro(expr.span) && in_macro(err_arg.span) && differing_contexts { + snippet(cx, err_arg.span.ctxt().outer_expn_data().call_site, "_") + } else if err_arg.span.from_expansion() && !in_macro(expr.span) { snippet_with_macro_callsite(cx, err_arg.span, "_") } else { snippet(cx, err_arg.span, "_") diff --git a/src/tools/clippy/clippy_lints/src/types.rs b/src/tools/clippy/clippy_lints/src/types.rs index c7d82da3b8b..f0e10e374e1 100644 --- a/src/tools/clippy/clippy_lints/src/types.rs +++ b/src/tools/clippy/clippy_lints/src/types.rs @@ -553,7 +553,7 @@ impl Types { hir_ty.span, "`Vec<T>` is already on the heap, the boxing is unnecessary.", "try", - format!("Vec<{}>", ty_ty), + format!("Vec<{}>", snippet(cx, boxed_ty.span, "..")), Applicability::MachineApplicable, ); return; // don't recurse into the type diff --git a/src/tools/clippy/clippy_lints/src/unit_return_expecting_ord.rs b/src/tools/clippy/clippy_lints/src/unit_return_expecting_ord.rs index ade5fff5ffc..2501635e7ef 100644 --- a/src/tools/clippy/clippy_lints/src/unit_return_expecting_ord.rs +++ b/src/tools/clippy/clippy_lints/src/unit_return_expecting_ord.rs @@ -24,7 +24,7 @@ declare_clippy_lint! { /// **Example:** /// /// ```rust - /// let mut twins = vec!((1,1), (2,2)); + /// let mut twins = vec!((1, 1), (2, 2)); /// twins.sort_by_key(|x| { x.1; }); /// ``` pub UNIT_RETURN_EXPECTING_ORD, diff --git a/src/tools/clippy/clippy_lints/src/unnecessary_wraps.rs b/src/tools/clippy/clippy_lints/src/unnecessary_wraps.rs new file mode 100644 index 00000000000..25ecc7a82f1 --- /dev/null +++ b/src/tools/clippy/clippy_lints/src/unnecessary_wraps.rs @@ -0,0 +1,143 @@ +use crate::utils::{ + in_macro, is_type_diagnostic_item, match_qpath, paths, return_ty, snippet, span_lint_and_then, + visitors::find_all_ret_expressions, +}; +use if_chain::if_chain; +use rustc_errors::Applicability; +use rustc_hir::intravisit::FnKind; +use rustc_hir::{Body, ExprKind, FnDecl, HirId, ItemKind, Node}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::subst::GenericArgKind; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::Span; + +declare_clippy_lint! { + /// **What it does:** Checks for private functions that only return `Ok` or `Some`. + /// + /// **Why is this bad?** It is not meaningful to wrap values when no `None` or `Err` is returned. + /// + /// **Known problems:** Since this lint changes function type signature, you may need to + /// adjust some code at callee side. + /// + /// **Example:** + /// + /// ```rust + /// fn get_cool_number(a: bool, b: bool) -> Option<i32> { + /// if a && b { + /// return Some(50); + /// } + /// if a { + /// Some(0) + /// } else { + /// Some(10) + /// } + /// } + /// ``` + /// Use instead: + /// ```rust + /// fn get_cool_number(a: bool, b: bool) -> i32 { + /// if a && b { + /// return 50; + /// } + /// if a { + /// 0 + /// } else { + /// 10 + /// } + /// } + /// ``` + pub UNNECESSARY_WRAPS, + complexity, + "functions that only return `Ok` or `Some`" +} + +declare_lint_pass!(UnnecessaryWraps => [UNNECESSARY_WRAPS]); + +impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps { + fn check_fn( + &mut self, + cx: &LateContext<'tcx>, + fn_kind: FnKind<'tcx>, + fn_decl: &FnDecl<'tcx>, + body: &Body<'tcx>, + span: Span, + hir_id: HirId, + ) { + match fn_kind { + FnKind::ItemFn(.., visibility, _) | FnKind::Method(.., Some(visibility), _) => { + if visibility.node.is_pub() { + return; + } + }, + FnKind::Closure(..) => return, + _ => (), + } + + if let Some(Node::Item(item)) = cx.tcx.hir().find(cx.tcx.hir().get_parent_node(hir_id)) { + if matches!(item.kind, ItemKind::Impl{ of_trait: Some(_), ..} | ItemKind::Trait(..)) { + return; + } + } + + let (return_type, path) = if is_type_diagnostic_item(cx, return_ty(cx, hir_id), sym!(option_type)) { + ("Option", &paths::OPTION_SOME) + } else if is_type_diagnostic_item(cx, return_ty(cx, hir_id), sym!(result_type)) { + ("Result", &paths::RESULT_OK) + } else { + return; + }; + + let mut suggs = Vec::new(); + let can_sugg = find_all_ret_expressions(cx, &body.value, |ret_expr| { + if_chain! { + if !in_macro(ret_expr.span); + if let ExprKind::Call(ref func, ref args) = ret_expr.kind; + if let ExprKind::Path(ref qpath) = func.kind; + if match_qpath(qpath, path); + if args.len() == 1; + then { + suggs.push((ret_expr.span, snippet(cx, args[0].span.source_callsite(), "..").to_string())); + true + } else { + false + } + } + }); + + if can_sugg && !suggs.is_empty() { + span_lint_and_then( + cx, + UNNECESSARY_WRAPS, + span, + format!( + "this function's return value is unnecessarily wrapped by `{}`", + return_type + ) + .as_str(), + |diag| { + let inner_ty = return_ty(cx, hir_id) + .walk() + .skip(1) // skip `std::option::Option` or `std::result::Result` + .take(1) // take the first outermost inner type + .filter_map(|inner| match inner.unpack() { + GenericArgKind::Type(inner_ty) => Some(inner_ty.to_string()), + _ => None, + }); + inner_ty.for_each(|inner_ty| { + diag.span_suggestion( + fn_decl.output.span(), + format!("remove `{}` from the return type...", return_type).as_str(), + inner_ty, + Applicability::MaybeIncorrect, + ); + }); + diag.multipart_suggestion( + "...and change the returning expressions", + suggs, + Applicability::MachineApplicable, + ); + }, + ); + } + } +} diff --git a/src/tools/clippy/clippy_lints/src/unused_unit.rs b/src/tools/clippy/clippy_lints/src/unused_unit.rs index b1339c3d639..f61fd2ecd73 100644 --- a/src/tools/clippy/clippy_lints/src/unused_unit.rs +++ b/src/tools/clippy/clippy_lints/src/unused_unit.rs @@ -7,7 +7,7 @@ use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::source_map::Span; use rustc_span::BytePos; -use crate::utils::span_lint_and_sugg; +use crate::utils::{position_before_rarrow, span_lint_and_sugg}; declare_clippy_lint! { /// **What it does:** Checks for unit (`()`) expressions that can be removed. @@ -120,26 +120,13 @@ fn is_unit_expr(expr: &ast::Expr) -> bool { fn lint_unneeded_unit_return(cx: &EarlyContext<'_>, ty: &ast::Ty, span: Span) { let (ret_span, appl) = if let Ok(fn_source) = cx.sess().source_map().span_to_snippet(span.with_hi(ty.span.hi())) { - fn_source - .rfind("->") - .map_or((ty.span, Applicability::MaybeIncorrect), |rpos| { - let mut rpos = rpos; - let chars: Vec<char> = fn_source.chars().collect(); - while rpos > 1 { - if let Some(c) = chars.get(rpos - 1) { - if c.is_whitespace() { - rpos -= 1; - continue; - } - } - break; - } - ( - #[allow(clippy::cast_possible_truncation)] - ty.span.with_lo(BytePos(span.lo().0 + rpos as u32)), - Applicability::MachineApplicable, - ) - }) + position_before_rarrow(fn_source).map_or((ty.span, Applicability::MaybeIncorrect), |rpos| { + ( + #[allow(clippy::cast_possible_truncation)] + ty.span.with_lo(BytePos(span.lo().0 + rpos as u32)), + Applicability::MachineApplicable, + ) + }) } else { (ty.span, Applicability::MaybeIncorrect) }; diff --git a/src/tools/clippy/clippy_lints/src/useless_conversion.rs b/src/tools/clippy/clippy_lints/src/useless_conversion.rs index c6194b0c6de..efa9c3fab4a 100644 --- a/src/tools/clippy/clippy_lints/src/useless_conversion.rs +++ b/src/tools/clippy/clippy_lints/src/useless_conversion.rs @@ -12,8 +12,8 @@ use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::sym; declare_clippy_lint! { - /// **What it does:** Checks for `Into`, `TryInto`, `From`, `TryFrom`,`IntoIter` calls - /// that useless converts to the same type as caller. + /// **What it does:** Checks for `Into`, `TryInto`, `From`, `TryFrom`, or `IntoIter` calls + /// which uselessly convert to the same type. /// /// **Why is this bad?** Redundant code. /// @@ -31,7 +31,7 @@ declare_clippy_lint! { /// ``` pub USELESS_CONVERSION, complexity, - "calls to `Into`, `TryInto`, `From`, `TryFrom`, `IntoIter` that performs useless conversions to the same type" + "calls to `Into`, `TryInto`, `From`, `TryFrom`, or `IntoIter` which perform useless conversions to the same type" } #[derive(Default)] diff --git a/src/tools/clippy/clippy_lints/src/utils/ast_utils.rs b/src/tools/clippy/clippy_lints/src/utils/ast_utils.rs index 9050b9b2d9a..fcf7a4b1367 100644 --- a/src/tools/clippy/clippy_lints/src/utils/ast_utils.rs +++ b/src/tools/clippy/clippy_lints/src/utils/ast_utils.rs @@ -110,8 +110,7 @@ pub fn eq_expr_opt(l: &Option<P<Expr>>, r: &Option<P<Expr>>) -> bool { pub fn eq_struct_rest(l: &StructRest, r: &StructRest) -> bool { match (l, r) { (StructRest::Base(lb), StructRest::Base(rb)) => eq_expr(lb, rb), - (StructRest::Rest(_), StructRest::Rest(_)) => true, - (StructRest::None, StructRest::None) => true, + (StructRest::Rest(_), StructRest::Rest(_)) | (StructRest::None, StructRest::None) => true, _ => false, } } diff --git a/src/tools/clippy/clippy_lints/src/utils/eager_or_lazy.rs b/src/tools/clippy/clippy_lints/src/utils/eager_or_lazy.rs index 4ceea13df37..8fe5ddee1ca 100644 --- a/src/tools/clippy/clippy_lints/src/utils/eager_or_lazy.rs +++ b/src/tools/clippy/clippy_lints/src/utils/eager_or_lazy.rs @@ -9,7 +9,7 @@ //! - or-fun-call //! - option-if-let-else -use crate::utils::is_ctor_or_promotable_const_function; +use crate::utils::{is_ctor_or_promotable_const_function, is_type_diagnostic_item, match_type, paths}; use rustc_hir::def::{DefKind, Res}; use rustc_hir::intravisit; @@ -96,6 +96,11 @@ fn identify_some_potentially_expensive_patterns<'tcx>(cx: &LateContext<'tcx>, ex let call_found = match &expr.kind { // ignore enum and struct constructors ExprKind::Call(..) => !is_ctor_or_promotable_const_function(self.cx, expr), + ExprKind::Index(obj, _) => { + let ty = self.cx.typeck_results().expr_ty(obj); + is_type_diagnostic_item(self.cx, ty, sym!(hashmap_type)) + || match_type(self.cx, ty, &paths::BTREEMAP) + }, ExprKind::MethodCall(..) => true, _ => false, }; diff --git a/src/tools/clippy/clippy_lints/src/utils/mod.rs b/src/tools/clippy/clippy_lints/src/utils/mod.rs index 270fdc9bf46..5bd64dcb541 100644 --- a/src/tools/clippy/clippy_lints/src/utils/mod.rs +++ b/src/tools/clippy/clippy_lints/src/utils/mod.rs @@ -21,6 +21,7 @@ pub mod ptr; pub mod qualify_min_const_fn; pub mod sugg; pub mod usage; +pub mod visitors; pub use self::attrs::*; pub use self::diagnostics::*; @@ -468,6 +469,13 @@ pub fn is_entrypoint_fn(cx: &LateContext<'_>, def_id: DefId) -> bool { .map_or(false, |(entry_fn_def_id, _)| def_id == entry_fn_def_id.to_def_id()) } +/// Returns `true` if the expression is in the program's `#[panic_handler]`. +pub fn is_in_panic_handler(cx: &LateContext<'_>, e: &Expr<'_>) -> bool { + let parent = cx.tcx.hir().get_parent_item(e.hir_id); + let def_id = cx.tcx.hir().local_def_id(parent).to_def_id(); + Some(def_id) == cx.tcx.lang_items().panic_impl() +} + /// Gets the name of the item the expression is in, if available. pub fn get_item_name(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<Symbol> { let parent_id = cx.tcx.hir().get_parent_item(expr.hir_id); @@ -659,6 +667,35 @@ pub fn indent_of<T: LintContext>(cx: &T, span: Span) -> Option<usize> { snippet_opt(cx, line_span(cx, span)).and_then(|snip| snip.find(|c: char| !c.is_whitespace())) } +/// Returns the positon just before rarrow +/// +/// ```rust,ignore +/// fn into(self) -> () {} +/// ^ +/// // in case of unformatted code +/// fn into2(self)-> () {} +/// ^ +/// fn into3(self) -> () {} +/// ^ +/// ``` +#[allow(clippy::needless_pass_by_value)] +pub fn position_before_rarrow(s: String) -> Option<usize> { + s.rfind("->").map(|rpos| { + let mut rpos = rpos; + let chars: Vec<char> = s.chars().collect(); + while rpos > 1 { + if let Some(c) = chars.get(rpos - 1) { + if c.is_whitespace() { + rpos -= 1; + continue; + } + } + break; + } + rpos + }) +} + /// Extends the span to the beginning of the spans line, incl. whitespaces. /// /// ```rust,ignore diff --git a/src/tools/clippy/clippy_lints/src/utils/paths.rs b/src/tools/clippy/clippy_lints/src/utils/paths.rs index 8f5fbfd9f84..137f5d18b66 100644 --- a/src/tools/clippy/clippy_lints/src/utils/paths.rs +++ b/src/tools/clippy/clippy_lints/src/utils/paths.rs @@ -126,6 +126,7 @@ pub const STRING: [&str; 3] = ["alloc", "string", "String"]; pub const STRING_AS_MUT_STR: [&str; 4] = ["alloc", "string", "String", "as_mut_str"]; pub const STRING_AS_STR: [&str; 4] = ["alloc", "string", "String", "as_str"]; pub const STR_ENDS_WITH: [&str; 4] = ["core", "str", "<impl str>", "ends_with"]; +pub const STR_FROM_UTF8: [&str; 4] = ["core", "str", "converts", "from_utf8"]; pub const STR_LEN: [&str; 4] = ["core", "str", "<impl str>", "len"]; pub const STR_STARTS_WITH: [&str; 4] = ["core", "str", "<impl str>", "starts_with"]; pub const SYNTAX_CONTEXT: [&str; 3] = ["rustc_span", "hygiene", "SyntaxContext"]; diff --git a/src/tools/clippy/clippy_lints/src/utils/visitors.rs b/src/tools/clippy/clippy_lints/src/utils/visitors.rs new file mode 100644 index 00000000000..b0837b6c43e --- /dev/null +++ b/src/tools/clippy/clippy_lints/src/utils/visitors.rs @@ -0,0 +1,125 @@ +use rustc_hir as hir; +use rustc_hir::intravisit::{self, Visitor}; +use rustc_lint::LateContext; +use rustc_middle::hir::map::Map; + +/// returns `true` if expr contains match expr desugared from try +fn contains_try(expr: &hir::Expr<'_>) -> bool { + struct TryFinder { + found: bool, + } + + impl<'hir> intravisit::Visitor<'hir> for TryFinder { + type Map = Map<'hir>; + + fn nested_visit_map(&mut self) -> intravisit::NestedVisitorMap<Self::Map> { + intravisit::NestedVisitorMap::None + } + + fn visit_expr(&mut self, expr: &'hir hir::Expr<'hir>) { + if self.found { + return; + } + match expr.kind { + hir::ExprKind::Match(_, _, hir::MatchSource::TryDesugar) => self.found = true, + _ => intravisit::walk_expr(self, expr), + } + } + } + + let mut visitor = TryFinder { found: false }; + visitor.visit_expr(expr); + visitor.found +} + +pub fn find_all_ret_expressions<'hir, F>(_cx: &LateContext<'_>, expr: &'hir hir::Expr<'hir>, callback: F) -> bool +where + F: FnMut(&'hir hir::Expr<'hir>) -> bool, +{ + struct RetFinder<F> { + in_stmt: bool, + failed: bool, + cb: F, + } + + struct WithStmtGuarg<'a, F> { + val: &'a mut RetFinder<F>, + prev_in_stmt: bool, + } + + impl<F> RetFinder<F> { + fn inside_stmt(&mut self, in_stmt: bool) -> WithStmtGuarg<'_, F> { + let prev_in_stmt = std::mem::replace(&mut self.in_stmt, in_stmt); + WithStmtGuarg { + val: self, + prev_in_stmt, + } + } + } + + impl<F> std::ops::Deref for WithStmtGuarg<'_, F> { + type Target = RetFinder<F>; + + fn deref(&self) -> &Self::Target { + self.val + } + } + + impl<F> std::ops::DerefMut for WithStmtGuarg<'_, F> { + fn deref_mut(&mut self) -> &mut Self::Target { + self.val + } + } + + impl<F> Drop for WithStmtGuarg<'_, F> { + fn drop(&mut self) { + self.val.in_stmt = self.prev_in_stmt; + } + } + + impl<'hir, F: FnMut(&'hir hir::Expr<'hir>) -> bool> intravisit::Visitor<'hir> for RetFinder<F> { + type Map = Map<'hir>; + + fn nested_visit_map(&mut self) -> intravisit::NestedVisitorMap<Self::Map> { + intravisit::NestedVisitorMap::None + } + + fn visit_stmt(&mut self, stmt: &'hir hir::Stmt<'_>) { + intravisit::walk_stmt(&mut *self.inside_stmt(true), stmt) + } + + fn visit_expr(&mut self, expr: &'hir hir::Expr<'_>) { + if self.failed { + return; + } + if self.in_stmt { + match expr.kind { + hir::ExprKind::Ret(Some(expr)) => self.inside_stmt(false).visit_expr(expr), + _ => intravisit::walk_expr(self, expr), + } + } else { + match expr.kind { + hir::ExprKind::Match(cond, arms, _) => { + self.inside_stmt(true).visit_expr(cond); + for arm in arms { + self.visit_expr(arm.body); + } + }, + hir::ExprKind::Block(..) => intravisit::walk_expr(self, expr), + hir::ExprKind::Ret(Some(expr)) => self.visit_expr(expr), + _ => self.failed |= !(self.cb)(expr), + } + } + } + } + + !contains_try(expr) && { + let mut ret_finder = RetFinder { + in_stmt: false, + failed: false, + cb: callback, + }; + ret_finder.visit_expr(expr); + !ret_finder.failed + } +} |