diff --git a/.github/ISSUE_TEMPLATE/blank_issue.md b/.github/ISSUE_TEMPLATE/blank_issue.md index 9aef3ebe637..5d7dfa36f37 100644 --- a/.github/ISSUE_TEMPLATE/blank_issue.md +++ b/.github/ISSUE_TEMPLATE/blank_issue.md @@ -2,3 +2,16 @@ name: Blank Issue about: Create a blank issue. --- + + + diff --git a/.github/ISSUE_TEMPLATE/bug_report.md b/.github/ISSUE_TEMPLATE/bug_report.md index 2bc87db123d..41e6abea1c6 100644 --- a/.github/ISSUE_TEMPLATE/bug_report.md +++ b/.github/ISSUE_TEMPLATE/bug_report.md @@ -20,28 +20,23 @@ Instead, this happened: *explanation* ### Meta -- `cargo clippy -V`: e.g. clippy 0.0.212 (f455e46 2020-06-20) -- `rustc -Vv`: - ``` - rustc 1.46.0-nightly (f455e46ea 2020-06-20) - binary: rustc - commit-hash: f455e46eae1a227d735091091144601b467e1565 - commit-date: 2020-06-20 - host: x86_64-unknown-linux-gnu - release: 1.46.0-nightly - LLVM version: 10.0 - ``` +**Rust version (`rustc -Vv`):** + +``` +rustc 1.46.0-nightly (f455e46ea 2020-06-20) +binary: rustc +commit-hash: f455e46eae1a227d735091091144601b467e1565 +commit-date: 2020-06-20 +host: x86_64-unknown-linux-gnu +release: 1.46.0-nightly +LLVM version: 10.0 +``` -
Backtrace -

- - ``` - - ``` - -

-
diff --git a/.github/ISSUE_TEMPLATE/false_negative.md b/.github/ISSUE_TEMPLATE/false_negative.md index 53341c7a928..d9ea2db34ed 100644 --- a/.github/ISSUE_TEMPLATE/false_negative.md +++ b/.github/ISSUE_TEMPLATE/false_negative.md @@ -22,14 +22,14 @@ Instead, this happened: *explanation* ### Meta -- `cargo clippy -V`: e.g. clippy 0.0.212 (f455e46 2020-06-20) -- `rustc -Vv`: - ``` - rustc 1.46.0-nightly (f455e46ea 2020-06-20) - binary: rustc - commit-hash: f455e46eae1a227d735091091144601b467e1565 - commit-date: 2020-06-20 - host: x86_64-unknown-linux-gnu - release: 1.46.0-nightly - LLVM version: 10.0 - ``` +**Rust version (`rustc -Vv`):** + +``` +rustc 1.46.0-nightly (f455e46ea 2020-06-20) +binary: rustc +commit-hash: f455e46eae1a227d735091091144601b467e1565 +commit-date: 2020-06-20 +host: x86_64-unknown-linux-gnu +release: 1.46.0-nightly +LLVM version: 10.0 +``` diff --git a/.github/ISSUE_TEMPLATE/false_positive.md b/.github/ISSUE_TEMPLATE/false_positive.md index 34fd6f4bdb7..edf9e04e3b1 100644 --- a/.github/ISSUE_TEMPLATE/false_positive.md +++ b/.github/ISSUE_TEMPLATE/false_positive.md @@ -22,14 +22,22 @@ Instead, this happened: *explanation* ### Meta -- `cargo clippy -V`: e.g. clippy 0.0.212 (f455e46 2020-06-20) -- `rustc -Vv`: - ``` - rustc 1.46.0-nightly (f455e46ea 2020-06-20) - binary: rustc - commit-hash: f455e46eae1a227d735091091144601b467e1565 - commit-date: 2020-06-20 - host: x86_64-unknown-linux-gnu - release: 1.46.0-nightly - LLVM version: 10.0 - ``` +**Rust version (`rustc -Vv`):** +``` +rustc 1.46.0-nightly (f455e46ea 2020-06-20) +binary: rustc +commit-hash: f455e46eae1a227d735091091144601b467e1565 +commit-date: 2020-06-20 +host: x86_64-unknown-linux-gnu +release: 1.46.0-nightly +LLVM version: 10.0 +``` + + diff --git a/.github/ISSUE_TEMPLATE/ice.md b/.github/ISSUE_TEMPLATE/ice.md index 0b7cd1ed0fb..6c1bed663c6 100644 --- a/.github/ISSUE_TEMPLATE/ice.md +++ b/.github/ISSUE_TEMPLATE/ice.md @@ -20,17 +20,16 @@ http://blog.pnkfx.org/blog/2019/11/18/rust-bug-minimization-patterns/ ### Meta -- `cargo clippy -V`: e.g. clippy 0.0.212 (f455e46 2020-06-20) -- `rustc -Vv`: - ``` - rustc 1.46.0-nightly (f455e46ea 2020-06-20) - binary: rustc - commit-hash: f455e46eae1a227d735091091144601b467e1565 - commit-date: 2020-06-20 - host: x86_64-unknown-linux-gnu - release: 1.46.0-nightly - LLVM version: 10.0 - ``` +**Rust version (`rustc -Vv`):** +``` +rustc 1.46.0-nightly (f455e46ea 2020-06-20) +binary: rustc +commit-hash: f455e46eae1a227d735091091144601b467e1565 +commit-date: 2020-06-20 +host: x86_64-unknown-linux-gnu +release: 1.46.0-nightly +LLVM version: 10.0 +``` ### Error output diff --git a/.github/workflows/clippy_dev.yml b/.github/workflows/clippy_dev.yml index 95da775b7bc..9a5416153ab 100644 --- a/.github/workflows/clippy_dev.yml +++ b/.github/workflows/clippy_dev.yml @@ -42,9 +42,6 @@ jobs: run: cargo build --features deny-warnings working-directory: clippy_dev - - name: Test limit_stderr_length - run: cargo dev limit_stderr_length - - name: Test update_lints run: cargo dev update_lints --check diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b89170073b..57088370343 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2712,7 +2712,6 @@ Released 2018-09-13 [`integer_arithmetic`]: https://rust-lang.github.io/rust-clippy/master/index.html#integer_arithmetic [`integer_division`]: https://rust-lang.github.io/rust-clippy/master/index.html#integer_division [`into_iter_on_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#into_iter_on_ref -[`invalid_atomic_ordering`]: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_atomic_ordering [`invalid_null_ptr_usage`]: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_null_ptr_usage [`invalid_regex`]: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_regex [`invalid_upcast_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_upcast_comparisons @@ -2754,6 +2753,7 @@ Released 2018-09-13 [`manual_ok_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ok_or [`manual_range_contains`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains [`manual_saturating_arithmetic`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_saturating_arithmetic +[`manual_split_once`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_split_once [`manual_str_repeat`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_str_repeat [`manual_strip`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_strip [`manual_swap`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_swap @@ -2795,6 +2795,7 @@ Released 2018-09-13 [`missing_safety_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_safety_doc [`mistyped_literal_suffixes`]: https://rust-lang.github.io/rust-clippy/master/index.html#mistyped_literal_suffixes [`mixed_case_hex_literals`]: https://rust-lang.github.io/rust-clippy/master/index.html#mixed_case_hex_literals +[`mod_module_files`]: https://rust-lang.github.io/rust-clippy/master/index.html#mod_module_files [`module_inception`]: https://rust-lang.github.io/rust-clippy/master/index.html#module_inception [`module_name_repetitions`]: https://rust-lang.github.io/rust-clippy/master/index.html#module_name_repetitions [`modulo_arithmetic`]: https://rust-lang.github.io/rust-clippy/master/index.html#modulo_arithmetic @@ -2828,6 +2829,7 @@ Released 2018-09-13 [`needless_update`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_update [`neg_cmp_op_on_partial_ord`]: https://rust-lang.github.io/rust-clippy/master/index.html#neg_cmp_op_on_partial_ord [`neg_multiply`]: https://rust-lang.github.io/rust-clippy/master/index.html#neg_multiply +[`negative_feature_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#negative_feature_names [`never_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#never_loop [`new_ret_no_self`]: https://rust-lang.github.io/rust-clippy/master/index.html#new_ret_no_self [`new_without_default`]: https://rust-lang.github.io/rust-clippy/master/index.html#new_without_default @@ -2881,6 +2883,7 @@ Released 2018-09-13 [`redundant_closure_call`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure_call [`redundant_closure_for_method_calls`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure_for_method_calls [`redundant_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_else +[`redundant_feature_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_feature_names [`redundant_field_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_field_names [`redundant_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_pattern [`redundant_pattern_matching`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_pattern_matching @@ -2903,6 +2906,7 @@ Released 2018-09-13 [`search_is_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#search_is_some [`self_assignment`]: https://rust-lang.github.io/rust-clippy/master/index.html#self_assignment [`self_named_constructors`]: https://rust-lang.github.io/rust-clippy/master/index.html#self_named_constructors +[`self_named_module_files`]: https://rust-lang.github.io/rust-clippy/master/index.html#self_named_module_files [`semicolon_if_nothing_returned`]: https://rust-lang.github.io/rust-clippy/master/index.html#semicolon_if_nothing_returned [`serde_api_misuse`]: https://rust-lang.github.io/rust-clippy/master/index.html#serde_api_misuse [`shadow_reuse`]: https://rust-lang.github.io/rust-clippy/master/index.html#shadow_reuse diff --git a/README.md b/README.md index e1c968273cd..aaf404eadea 100644 --- a/README.md +++ b/README.md @@ -45,13 +45,13 @@ or in Travis CI. One way to use Clippy is by installing Clippy through rustup as a cargo subcommand. -#### Step 1: Install rustup +#### Step 1: Install Rustup -You can install [rustup](https://rustup.rs/) on supported platforms. This will help +You can install [Rustup](https://rustup.rs/) on supported platforms. This will help us install Clippy and its dependencies. -If you already have rustup installed, update to ensure you have the latest -rustup and compiler: +If you already have Rustup installed, update to ensure you have the latest +Rustup and compiler: ```terminal rustup update diff --git a/clippy_dev/src/lib.rs b/clippy_dev/src/lib.rs index 72bdaf8d592..e05db7af586 100644 --- a/clippy_dev/src/lib.rs +++ b/clippy_dev/src/lib.rs @@ -17,7 +17,6 @@ pub mod new_lint; pub mod serve; pub mod setup; -pub mod stderr_length_check; pub mod update_lints; static DEC_CLIPPY_LINT_RE: SyncLazy = SyncLazy::new(|| { diff --git a/clippy_dev/src/main.rs b/clippy_dev/src/main.rs index ff324ff6ee6..8fdeba9842a 100644 --- a/clippy_dev/src/main.rs +++ b/clippy_dev/src/main.rs @@ -3,7 +3,7 @@ #![warn(rust_2018_idioms, unused_lifetimes)] use clap::{App, AppSettings, Arg, ArgMatches, SubCommand}; -use clippy_dev::{bless, fmt, new_lint, serve, setup, stderr_length_check, update_lints}; +use clippy_dev::{bless, fmt, new_lint, serve, setup, update_lints}; fn main() { let matches = get_clap_config(); @@ -33,9 +33,6 @@ fn main() { Err(e) => eprintln!("Unable to create lint: {}", e), } }, - ("limit_stderr_length", _) => { - stderr_length_check::check(); - }, ("setup", Some(sub_command)) => match sub_command.subcommand() { ("intellij", Some(matches)) => setup::intellij::setup_rustc_src( matches @@ -152,10 +149,6 @@ fn get_clap_config<'a>() -> ArgMatches<'a> { .takes_value(true), ), ) - .subcommand( - SubCommand::with_name("limit_stderr_length") - .about("Ensures that stderr files do not grow longer than a certain amount of lines."), - ) .subcommand( SubCommand::with_name("setup") .about("Support for setting up your personal development environment") diff --git a/clippy_dev/src/stderr_length_check.rs b/clippy_dev/src/stderr_length_check.rs deleted file mode 100644 index e02b6f7da5f..00000000000 --- a/clippy_dev/src/stderr_length_check.rs +++ /dev/null @@ -1,51 +0,0 @@ -use crate::clippy_project_root; -use std::ffi::OsStr; -use std::fs; -use std::path::{Path, PathBuf}; -use walkdir::WalkDir; - -// The maximum length allowed for stderr files. -// -// We limit this because small files are easier to deal with than bigger files. -const LENGTH_LIMIT: usize = 200; - -pub fn check() { - let exceeding_files: Vec<_> = exceeding_stderr_files(); - - if !exceeding_files.is_empty() { - eprintln!("Error: stderr files exceeding limit of {} lines:", LENGTH_LIMIT); - for (path, count) in exceeding_files { - println!("{}: {}", path.display(), count); - } - std::process::exit(1); - } -} - -fn exceeding_stderr_files() -> Vec<(PathBuf, usize)> { - // We use `WalkDir` instead of `fs::read_dir` here in order to recurse into subdirectories. - WalkDir::new(clippy_project_root().join("tests/ui")) - .into_iter() - .filter_map(Result::ok) - .filter(|f| !f.file_type().is_dir()) - .filter_map(|e| { - let p = e.into_path(); - let count = count_linenumbers(&p); - if p.extension() == Some(OsStr::new("stderr")) && count > LENGTH_LIMIT { - Some((p, count)) - } else { - None - } - }) - .collect() -} - -#[must_use] -fn count_linenumbers(filepath: &Path) -> usize { - match fs::read(filepath) { - Ok(content) => bytecount::count(&content, b'\n'), - Err(e) => { - eprintln!("Failed to read file: {}", e); - 0 - }, - } -} diff --git a/clippy_lints/src/bool_assert_comparison.rs b/clippy_lints/src/bool_assert_comparison.rs index 8d3f68565b2..cdc192a47e4 100644 --- a/clippy_lints/src/bool_assert_comparison.rs +++ b/clippy_lints/src/bool_assert_comparison.rs @@ -1,9 +1,11 @@ -use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::{ast_utils, is_direct_expn_of}; -use rustc_ast::ast::{Expr, ExprKind, Lit, LitKind}; +use clippy_utils::{diagnostics::span_lint_and_sugg, higher, is_direct_expn_of, ty::implements_trait}; +use rustc_ast::ast::LitKind; use rustc_errors::Applicability; -use rustc_lint::{EarlyContext, EarlyLintPass}; +use rustc_hir::{Expr, ExprKind, Lit}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty; use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::symbol::Ident; declare_clippy_lint! { /// ### What it does @@ -28,45 +30,77 @@ declare_lint_pass!(BoolAssertComparison => [BOOL_ASSERT_COMPARISON]); -fn is_bool_lit(e: &Expr) -> bool { +fn is_bool_lit(e: &Expr<'_>) -> bool { matches!( e.kind, ExprKind::Lit(Lit { - kind: LitKind::Bool(_), + node: LitKind::Bool(_), .. }) ) && !e.span.from_expansion() } -impl EarlyLintPass for BoolAssertComparison { - fn check_expr(&mut self, cx: &EarlyContext<'_>, e: &Expr) { +fn is_impl_not_trait_with_bool_out(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> bool { + let ty = cx.typeck_results().expr_ty(e); + + cx.tcx + .lang_items() + .not_trait() + .filter(|trait_id| implements_trait(cx, ty, *trait_id, &[])) + .and_then(|trait_id| { + cx.tcx.associated_items(trait_id).find_by_name_and_kind( + cx.tcx, + Ident::from_str("Output"), + ty::AssocKind::Type, + trait_id, + ) + }) + .map_or(false, |assoc_item| { + let proj = cx.tcx.mk_projection(assoc_item.def_id, cx.tcx.mk_substs_trait(ty, &[])); + let nty = cx.tcx.normalize_erasing_regions(cx.param_env, proj); + + nty.is_bool() + }) +} + +impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { let macros = ["assert_eq", "debug_assert_eq"]; let inverted_macros = ["assert_ne", "debug_assert_ne"]; for mac in macros.iter().chain(inverted_macros.iter()) { - if let Some(span) = is_direct_expn_of(e.span, mac) { - if let Some([a, b]) = ast_utils::extract_assert_macro_args(e) { - let nb_bool_args = is_bool_lit(a) as usize + is_bool_lit(b) as usize; + if let Some(span) = is_direct_expn_of(expr.span, mac) { + if let Some(args) = higher::extract_assert_macro_args(expr) { + if let [a, b, ..] = args[..] { + let nb_bool_args = is_bool_lit(a) as usize + is_bool_lit(b) as usize; - if nb_bool_args != 1 { - // If there are two boolean arguments, we definitely don't understand - // what's going on, so better leave things as is... - // - // Or there is simply no boolean and then we can leave things as is! + if nb_bool_args != 1 { + // If there are two boolean arguments, we definitely don't understand + // what's going on, so better leave things as is... + // + // Or there is simply no boolean and then we can leave things as is! + return; + } + + if !is_impl_not_trait_with_bool_out(cx, a) || !is_impl_not_trait_with_bool_out(cx, b) { + // At this point the expression which is not a boolean + // literal does not implement Not trait with a bool output, + // so we cannot suggest to rewrite our code + return; + } + + let non_eq_mac = &mac[..mac.len() - 3]; + span_lint_and_sugg( + cx, + BOOL_ASSERT_COMPARISON, + span, + &format!("used `{}!` with a literal bool", mac), + "replace it with", + format!("{}!(..)", non_eq_mac), + Applicability::MaybeIncorrect, + ); return; } - - let non_eq_mac = &mac[..mac.len() - 3]; - span_lint_and_sugg( - cx, - BOOL_ASSERT_COMPARISON, - span, - &format!("used `{}!` with a literal bool", mac), - "replace it with", - format!("{}!(..)", non_eq_mac), - Applicability::MaybeIncorrect, - ); - return; } } } diff --git a/clippy_lints/src/bytecount.rs b/clippy_lints/src/bytecount.rs index c444984bc13..a07cd5e5f4e 100644 --- a/clippy_lints/src/bytecount.rs +++ b/clippy_lints/src/bytecount.rs @@ -1,7 +1,7 @@ use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::snippet_with_applicability; use clippy_utils::ty::match_type; -use clippy_utils::visitors::LocalUsedVisitor; +use clippy_utils::visitors::is_local_used; use clippy_utils::{path_to_local_id, paths, peel_ref_operators, remove_blocks, strip_pat_refs}; use if_chain::if_chain; use rustc_errors::Applicability; @@ -65,7 +65,7 @@ fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { return; }; if ty::Uint(UintTy::U8) == *cx.typeck_results().expr_ty(needle).peel_refs().kind(); - if !LocalUsedVisitor::new(cx, arg_id).check_expr(needle); + if !is_local_used(cx, needle, arg_id); then { let haystack = if let ExprKind::MethodCall(path, _, args, _) = filter_recv.kind { diff --git a/clippy_lints/src/collapsible_match.rs b/clippy_lints/src/collapsible_match.rs index a42eee53459..f5683856889 100644 --- a/clippy_lints/src/collapsible_match.rs +++ b/clippy_lints/src/collapsible_match.rs @@ -1,5 +1,5 @@ use clippy_utils::diagnostics::span_lint_and_then; -use clippy_utils::visitors::LocalUsedVisitor; +use clippy_utils::visitors::is_local_used; use clippy_utils::{higher, is_lang_ctor, is_unit_expr, path_to_local, peel_ref_operators, SpanlessEq}; use if_chain::if_chain; use rustc_hir::LangItem::OptionNone; @@ -56,11 +56,11 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) { check_arm(cx, true, arm.pat, arm.body, arm.guard.as_ref(), Some(els_arm.body)); } } - } + }, Some(IfLetOrMatch::IfLet(_, pat, body, els)) => { check_arm(cx, false, pat, body, None, els); - } - None => {} + }, + None => {}, } } } @@ -71,7 +71,7 @@ fn check_arm<'tcx>( outer_pat: &'tcx Pat<'tcx>, outer_then_body: &'tcx Expr<'tcx>, outer_guard: Option<&'tcx Guard<'tcx>>, - outer_else_body: Option<&'tcx Expr<'tcx>> + outer_else_body: Option<&'tcx Expr<'tcx>>, ) { let inner_expr = strip_singleton_blocks(outer_then_body); if_chain! { @@ -106,14 +106,12 @@ fn check_arm<'tcx>( (Some(a), Some(b)) => SpanlessEq::new(cx).eq_expr(a, b), }; // the binding must not be used in the if guard - let mut used_visitor = LocalUsedVisitor::new(cx, binding_id); - if outer_guard.map_or(true, |(Guard::If(e) | Guard::IfLet(_, e))| !used_visitor.check_expr(e)); - // ...or anywhere in the inner expression + if outer_guard.map_or(true, |(Guard::If(e) | Guard::IfLet(_, e))| !is_local_used(cx, *e, binding_id)); // ...or anywhere in the inner expression if match inner { IfLetOrMatch::IfLet(_, _, body, els) => { - !used_visitor.check_expr(body) && els.map_or(true, |e| !used_visitor.check_expr(e)) + !is_local_used(cx, body, binding_id) && els.map_or(true, |e| !is_local_used(cx, e, binding_id)) }, - IfLetOrMatch::Match(_, arms, ..) => !arms.iter().any(|arm| used_visitor.check_arm(arm)), + IfLetOrMatch::Match(_, arms, ..) => !arms.iter().any(|arm| is_local_used(cx, arm, binding_id)), }; then { let msg = format!( @@ -154,16 +152,26 @@ fn strip_singleton_blocks<'hir>(mut expr: &'hir Expr<'hir>) -> &'hir Expr<'hir> enum IfLetOrMatch<'hir> { Match(&'hir Expr<'hir>, &'hir [Arm<'hir>], MatchSource), /// scrutinee, pattern, then block, else block - IfLet(&'hir Expr<'hir>, &'hir Pat<'hir>, &'hir Expr<'hir>, Option<&'hir Expr<'hir>>), + IfLet( + &'hir Expr<'hir>, + &'hir Pat<'hir>, + &'hir Expr<'hir>, + Option<&'hir Expr<'hir>>, + ), } impl<'hir> IfLetOrMatch<'hir> { fn parse(cx: &LateContext<'_>, expr: &Expr<'hir>) -> Option { match expr.kind { ExprKind::Match(expr, arms, source) => Some(Self::Match(expr, arms, source)), - _ => higher::IfLet::hir(cx, expr).map(|higher::IfLet { let_expr, let_pat, if_then, if_else }| { - Self::IfLet(let_expr, let_pat, if_then, if_else) - }) + _ => higher::IfLet::hir(cx, expr).map( + |higher::IfLet { + let_expr, + let_pat, + if_then, + if_else, + }| { Self::IfLet(let_expr, let_pat, if_then, if_else) }, + ), } } } diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index 5eb99cfe24f..d58e4949120 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -148,7 +148,7 @@ /// }; /// ``` pub BRANCHES_SHARING_CODE, - complexity, + nursery, "`if` statement with shared code in all blocks" } diff --git a/clippy_lints/src/derive.rs b/clippy_lints/src/derive.rs index dcfa5253f83..8416b8440df 100644 --- a/clippy_lints/src/derive.rs +++ b/clippy_lints/src/derive.rs @@ -105,9 +105,6 @@ /// nothing more than copy the object, which is what `#[derive(Copy, Clone)]` /// gets you. /// - /// ### Known problems - /// Bounds of generic types are sometimes wrong: https://github.com/rust-lang/rust/issues/26925 - /// /// ### Example /// ```rust,ignore /// #[derive(Copy)] diff --git a/clippy_lints/src/entry.rs b/clippy_lints/src/entry.rs index 627f746ec99..ac6824672f6 100644 --- a/clippy_lints/src/entry.rs +++ b/clippy_lints/src/entry.rs @@ -9,8 +9,9 @@ use core::fmt::Write; use rustc_errors::Applicability; use rustc_hir::{ + hir_id::HirIdSet, intravisit::{walk_expr, ErasedMap, NestedVisitorMap, Visitor}, - Block, Expr, ExprKind, Guard, HirId, Local, Stmt, StmtKind, UnOp, + Block, Expr, ExprKind, Guard, HirId, Pat, Stmt, StmtKind, UnOp, }; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; @@ -338,6 +339,8 @@ struct InsertSearcher<'cx, 'tcx> { edits: Vec>, /// A stack of loops the visitor is currently in. loops: Vec, + /// Local variables created in the expression. These don't need to be captured. + locals: HirIdSet, } impl<'tcx> InsertSearcher<'_, 'tcx> { /// Visit the expression as a branch in control flow. Multiple insert calls can be used, but @@ -385,13 +388,16 @@ fn visit_stmt(&mut self, stmt: &'tcx Stmt<'_>) { } }, StmtKind::Expr(e) => self.visit_expr(e), - StmtKind::Local(Local { init: Some(e), .. }) => { - self.allow_insert_closure &= !self.in_tail_pos; - self.in_tail_pos = false; - self.is_single_insert = false; - self.visit_expr(e); + StmtKind::Local(l) => { + self.visit_pat(l.pat); + if let Some(e) = l.init { + self.allow_insert_closure &= !self.in_tail_pos; + self.in_tail_pos = false; + self.is_single_insert = false; + self.visit_expr(e); + } }, - _ => { + StmtKind::Item(_) => { self.allow_insert_closure &= !self.in_tail_pos; self.is_single_insert = false; }, @@ -473,6 +479,7 @@ fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { // Each branch may contain it's own insert expression. let mut is_map_used = self.is_map_used; for arm in arms { + self.visit_pat(arm.pat); if let Some(Guard::If(guard) | Guard::IfLet(_, guard)) = arm.guard { self.visit_non_tail_expr(guard); } @@ -498,7 +505,8 @@ fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { }, _ => { self.allow_insert_closure &= !self.in_tail_pos; - self.allow_insert_closure &= can_move_expr_to_closure_no_visit(self.cx, expr, &self.loops); + self.allow_insert_closure &= + can_move_expr_to_closure_no_visit(self.cx, expr, &self.loops, &self.locals); // Sub expressions are no longer in the tail position. self.is_single_insert = false; self.in_tail_pos = false; @@ -507,6 +515,12 @@ fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { }, } } + + fn visit_pat(&mut self, p: &'tcx Pat<'tcx>) { + p.each_binding_or_first(&mut |_, id, _, _| { + self.locals.insert(id); + }); + } } struct InsertSearchResults<'tcx> { @@ -632,6 +646,7 @@ fn find_insert_calls( in_tail_pos: true, is_single_insert: true, loops: Vec::new(), + locals: HirIdSet::default(), }; s.visit_expr(expr); let allow_insert_closure = s.allow_insert_closure; diff --git a/clippy_lints/src/escape.rs b/clippy_lints/src/escape.rs index 685dbf26250..ae66edc2a9e 100644 --- a/clippy_lints/src/escape.rs +++ b/clippy_lints/src/escape.rs @@ -171,7 +171,8 @@ fn mutate(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId) { // skip if there is a `self` parameter binding to a type // that contains `Self` (i.e.: `self: Box`), see #4804 if let Some(trait_self_ty) = self.trait_self_ty { - if map.name(cmt.hir_id) == kw::SelfLower && contains_ty(self.cx.tcx, cmt.place.ty(), trait_self_ty) { + if map.name(cmt.hir_id) == kw::SelfLower && contains_ty(self.cx.tcx, cmt.place.ty(), trait_self_ty) + { return; } } diff --git a/clippy_lints/src/feature_name.rs b/clippy_lints/src/feature_name.rs new file mode 100644 index 00000000000..eef1407a80c --- /dev/null +++ b/clippy_lints/src/feature_name.rs @@ -0,0 +1,164 @@ +use clippy_utils::diagnostics::span_lint_and_help; +use clippy_utils::{diagnostics::span_lint, is_lint_allowed}; +use rustc_hir::{Crate, CRATE_HIR_ID}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::source_map::DUMMY_SP; + +declare_clippy_lint! { + /// ### What it does + /// Checks for feature names with prefix `use-`, `with-` or suffix `-support` + /// + /// ### Why is this bad? + /// These prefixes and suffixes have no significant meaning. + /// + /// ### Example + /// ```toml + /// # The `Cargo.toml` with feature name redundancy + /// [features] + /// default = ["use-abc", "with-def", "ghi-support"] + /// use-abc = [] // redundant + /// with-def = [] // redundant + /// ghi-support = [] // redundant + /// ``` + /// + /// Use instead: + /// ```toml + /// [features] + /// default = ["abc", "def", "ghi"] + /// abc = [] + /// def = [] + /// ghi = [] + /// ``` + /// + pub REDUNDANT_FEATURE_NAMES, + cargo, + "usage of a redundant feature name" +} + +declare_clippy_lint! { + /// ### What it does + /// Checks for negative feature names with prefix `no-` or `not-` + /// + /// ### Why is this bad? + /// Features are supposed to be additive, and negatively-named features violate it. + /// + /// ### Example + /// ```toml + /// # The `Cargo.toml` with negative feature names + /// [features] + /// default = [] + /// no-abc = [] + /// not-def = [] + /// + /// ``` + /// Use instead: + /// ```toml + /// [features] + /// default = ["abc", "def"] + /// abc = [] + /// def = [] + /// + /// ``` + pub NEGATIVE_FEATURE_NAMES, + cargo, + "usage of a negative feature name" +} + +declare_lint_pass!(FeatureName => [REDUNDANT_FEATURE_NAMES, NEGATIVE_FEATURE_NAMES]); + +static PREFIXES: [&str; 8] = ["no-", "no_", "not-", "not_", "use-", "use_", "with-", "with_"]; +static SUFFIXES: [&str; 2] = ["-support", "_support"]; + +fn is_negative_prefix(s: &str) -> bool { + s.starts_with("no") +} + +fn lint(cx: &LateContext<'_>, feature: &str, substring: &str, is_prefix: bool) { + let is_negative = is_prefix && is_negative_prefix(substring); + span_lint_and_help( + cx, + if is_negative { + NEGATIVE_FEATURE_NAMES + } else { + REDUNDANT_FEATURE_NAMES + }, + DUMMY_SP, + &format!( + "the \"{}\" {} in the feature name \"{}\" is {}", + substring, + if is_prefix { "prefix" } else { "suffix" }, + feature, + if is_negative { "negative" } else { "redundant" } + ), + None, + &format!( + "consider renaming the feature to \"{}\"{}", + if is_prefix { + feature.strip_prefix(substring) + } else { + feature.strip_suffix(substring) + } + .unwrap(), + if is_negative { + ", but make sure the feature adds functionality" + } else { + "" + } + ), + ); +} + +impl LateLintPass<'_> for FeatureName { + fn check_crate(&mut self, cx: &LateContext<'_>, _: &Crate<'_>) { + if is_lint_allowed(cx, REDUNDANT_FEATURE_NAMES, CRATE_HIR_ID) + && is_lint_allowed(cx, NEGATIVE_FEATURE_NAMES, CRATE_HIR_ID) + { + return; + } + + let metadata = unwrap_cargo_metadata!(cx, REDUNDANT_FEATURE_NAMES, false); + + for package in metadata.packages { + let mut features: Vec<&String> = package.features.keys().collect(); + features.sort(); + for feature in features { + let prefix_opt = { + let i = PREFIXES.partition_point(|prefix| prefix < &feature.as_str()); + if i > 0 && feature.starts_with(PREFIXES[i - 1]) { + Some(PREFIXES[i - 1]) + } else { + None + } + }; + if let Some(prefix) = prefix_opt { + lint(cx, feature, prefix, true); + } + + let suffix_opt: Option<&str> = { + let i = SUFFIXES.partition_point(|suffix| { + suffix.bytes().rev().cmp(feature.bytes().rev()) == std::cmp::Ordering::Less + }); + if i > 0 && feature.ends_with(SUFFIXES[i - 1]) { + Some(SUFFIXES[i - 1]) + } else { + None + } + }; + if let Some(suffix) = suffix_opt { + lint(cx, feature, suffix, false); + } + } + } + } +} + +#[test] +fn test_prefixes_sorted() { + let mut sorted_prefixes = PREFIXES; + sorted_prefixes.sort_unstable(); + assert_eq!(PREFIXES, sorted_prefixes); + let mut sorted_suffixes = SUFFIXES; + sorted_suffixes.sort_by(|a, b| a.bytes().rev().cmp(b.bytes().rev())); + assert_eq!(SUFFIXES, sorted_suffixes); +} diff --git a/clippy_lints/src/let_if_seq.rs b/clippy_lints/src/let_if_seq.rs index 0594b73dd38..7f2c7b707f0 100644 --- a/clippy_lints/src/let_if_seq.rs +++ b/clippy_lints/src/let_if_seq.rs @@ -1,6 +1,6 @@ use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::source::snippet; -use clippy_utils::{path_to_local_id, visitors::LocalUsedVisitor}; +use clippy_utils::{path_to_local_id, visitors::is_local_used}; use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir as hir; @@ -65,11 +65,10 @@ fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx hir::Block<'_>) { if let hir::PatKind::Binding(mode, canonical_id, ident, None) = local.pat.kind; if let hir::StmtKind::Expr(if_) = expr.kind; if let hir::ExprKind::If(hir::Expr { kind: hir::ExprKind::DropTemps(cond), ..}, then, else_) = if_.kind; - let mut used_visitor = LocalUsedVisitor::new(cx, canonical_id); - if !used_visitor.check_expr(cond); + if !is_local_used(cx, *cond, canonical_id); if let hir::ExprKind::Block(then, _) = then.kind; if let Some(value) = check_assign(cx, canonical_id, &*then); - if !used_visitor.check_expr(value); + if !is_local_used(cx, value, canonical_id); then { let span = stmt.span.to(if_.span); @@ -148,15 +147,13 @@ fn check_assign<'tcx>( if let hir::ExprKind::Assign(var, value, _) = expr.kind; if path_to_local_id(var, decl); then { - let mut v = LocalUsedVisitor::new(cx, decl); - - if block.stmts.iter().take(block.stmts.len()-1).any(|stmt| v.check_stmt(stmt)) { - return None; + if block.stmts.iter().take(block.stmts.len()-1).any(|stmt| is_local_used(cx, stmt, decl)) { + None + } else { + Some(value) } - - return Some(value); + } else { + None } } - - None } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 19719502870..5ce5f809f46 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -211,6 +211,7 @@ macro_rules! declare_clippy_lint { mod exit; mod explicit_write; mod fallible_impl_from; +mod feature_name; mod float_equality_without_abs; mod float_literal; mod floating_point_arithmetic; @@ -272,6 +273,7 @@ macro_rules! declare_clippy_lint { mod missing_doc; mod missing_enforced_import_rename; mod missing_inline; +mod module_style; mod modulo_arithmetic; mod multiple_crate_versions; mod mut_key; @@ -625,6 +627,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: exit::EXIT, explicit_write::EXPLICIT_WRITE, fallible_impl_from::FALLIBLE_IMPL_FROM, + feature_name::NEGATIVE_FEATURE_NAMES, + feature_name::REDUNDANT_FEATURE_NAMES, float_equality_without_abs::FLOAT_EQUALITY_WITHOUT_ABS, float_literal::EXCESSIVE_PRECISION, float_literal::LOSSY_FLOAT_LITERAL, @@ -770,6 +774,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: methods::MANUAL_FILTER_MAP, methods::MANUAL_FIND_MAP, methods::MANUAL_SATURATING_ARITHMETIC, + methods::MANUAL_SPLIT_ONCE, methods::MANUAL_STR_REPEAT, methods::MAP_COLLECT_RESULT_UNIT, methods::MAP_FLATTEN, @@ -822,6 +827,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: missing_doc::MISSING_DOCS_IN_PRIVATE_ITEMS, missing_enforced_import_rename::MISSING_ENFORCED_IMPORT_RENAMES, missing_inline::MISSING_INLINE_IN_PUBLIC_ITEMS, + module_style::MOD_MODULE_FILES, + module_style::SELF_NAMED_MODULE_FILES, modulo_arithmetic::MODULO_ARITHMETIC, multiple_crate_versions::MULTIPLE_CRATE_VERSIONS, mut_key::MUTABLE_KEY_TYPE, @@ -1031,6 +1038,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(missing_doc::MISSING_DOCS_IN_PRIVATE_ITEMS), LintId::of(missing_enforced_import_rename::MISSING_ENFORCED_IMPORT_RENAMES), LintId::of(missing_inline::MISSING_INLINE_IN_PUBLIC_ITEMS), + LintId::of(module_style::MOD_MODULE_FILES), + LintId::of(module_style::SELF_NAMED_MODULE_FILES), LintId::of(modulo_arithmetic::MODULO_ARITHMETIC), LintId::of(panic_in_result_fn::PANIC_IN_RESULT_FN), LintId::of(panic_unimplemented::PANIC), @@ -1122,7 +1131,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(needless_for_each::NEEDLESS_FOR_EACH), LintId::of(needless_pass_by_value::NEEDLESS_PASS_BY_VALUE), LintId::of(non_expressive_names::SIMILAR_NAMES), - LintId::of(option_if_let_else::OPTION_IF_LET_ELSE), LintId::of(pass_by_ref_or_value::LARGE_TYPES_PASSED_BY_VALUE), LintId::of(pass_by_ref_or_value::TRIVIALLY_COPY_PASS_BY_REF), LintId::of(ranges::RANGE_MINUS_ONE), @@ -1193,7 +1201,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(collapsible_if::COLLAPSIBLE_IF), LintId::of(collapsible_match::COLLAPSIBLE_MATCH), LintId::of(comparison_chain::COMPARISON_CHAIN), - LintId::of(copies::BRANCHES_SHARING_CODE), LintId::of(copies::IFS_SAME_COND), LintId::of(copies::IF_SAME_THEN_ELSE), LintId::of(default::FIELD_REASSIGN_WITH_DEFAULT), @@ -1316,6 +1323,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(methods::MANUAL_FILTER_MAP), LintId::of(methods::MANUAL_FIND_MAP), LintId::of(methods::MANUAL_SATURATING_ARITHMETIC), + LintId::of(methods::MANUAL_SPLIT_ONCE), LintId::of(methods::MANUAL_STR_REPEAT), LintId::of(methods::MAP_COLLECT_RESULT_UNIT), LintId::of(methods::MAP_IDENTITY), @@ -1581,7 +1589,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(booleans::NONMINIMAL_BOOL), LintId::of(casts::CHAR_LIT_AS_U8), LintId::of(casts::UNNECESSARY_CAST), - LintId::of(copies::BRANCHES_SHARING_CODE), LintId::of(double_comparison::DOUBLE_COMPARISONS), LintId::of(double_parens::DOUBLE_PARENS), LintId::of(duration_subsec::DURATION_SUBSEC), @@ -1614,6 +1621,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(methods::ITER_COUNT), LintId::of(methods::MANUAL_FILTER_MAP), LintId::of(methods::MANUAL_FIND_MAP), + LintId::of(methods::MANUAL_SPLIT_ONCE), LintId::of(methods::MAP_IDENTITY), LintId::of(methods::OPTION_AS_REF_DEREF), LintId::of(methods::OPTION_FILTER_MAP), @@ -1779,6 +1787,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_group(true, "clippy::cargo", Some("clippy_cargo"), vec![ LintId::of(cargo_common_metadata::CARGO_COMMON_METADATA), + LintId::of(feature_name::NEGATIVE_FEATURE_NAMES), + LintId::of(feature_name::REDUNDANT_FEATURE_NAMES), LintId::of(multiple_crate_versions::MULTIPLE_CRATE_VERSIONS), LintId::of(wildcard_dependencies::WILDCARD_DEPENDENCIES), ]); @@ -1786,6 +1796,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_group(true, "clippy::nursery", Some("clippy_nursery"), vec![ LintId::of(attrs::EMPTY_LINE_AFTER_OUTER_ATTR), LintId::of(cognitive_complexity::COGNITIVE_COMPLEXITY), + LintId::of(copies::BRANCHES_SHARING_CODE), LintId::of(disallowed_method::DISALLOWED_METHOD), LintId::of(disallowed_type::DISALLOWED_TYPE), LintId::of(fallible_impl_from::FALLIBLE_IMPL_FROM), @@ -1797,6 +1808,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(mutable_debug_assertion::DEBUG_ASSERT_WITH_MUT_CALL), LintId::of(mutex_atomic::MUTEX_INTEGER), LintId::of(nonstandard_macro_braces::NONSTANDARD_MACRO_BRACES), + LintId::of(option_if_let_else::OPTION_IF_LET_ELSE), LintId::of(path_buf_push_overwrite::PATH_BUF_PUSH_OVERWRITE), LintId::of(redundant_pub_crate::REDUNDANT_PUB_CRATE), LintId::of(regex::TRIVIAL_REGEX), @@ -1835,7 +1847,12 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| Box::new(serde_api::SerdeApi)); let vec_box_size_threshold = conf.vec_box_size_threshold; let type_complexity_threshold = conf.type_complexity_threshold; - store.register_late_pass(move || Box::new(types::Types::new(vec_box_size_threshold, type_complexity_threshold))); + let avoid_breaking_exported_api = conf.avoid_breaking_exported_api; + store.register_late_pass(move || Box::new(types::Types::new( + vec_box_size_threshold, + type_complexity_threshold, + avoid_breaking_exported_api, + ))); store.register_late_pass(|| Box::new(booleans::NonminimalBool)); store.register_late_pass(|| Box::new(needless_bitwise_bool::NeedlessBitwiseBool)); store.register_late_pass(|| Box::new(eq_op::EqOp)); @@ -2092,7 +2109,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| Box::new(from_str_radix_10::FromStrRadix10)); store.register_late_pass(|| Box::new(manual_map::ManualMap)); store.register_late_pass(move || Box::new(if_then_some_else_none::IfThenSomeElseNone::new(msrv))); - store.register_early_pass(|| Box::new(bool_assert_comparison::BoolAssertComparison)); + store.register_late_pass(|| Box::new(bool_assert_comparison::BoolAssertComparison)); + store.register_early_pass(move || Box::new(module_style::ModStyle)); store.register_late_pass(|| Box::new(unused_async::UnusedAsync)); let disallowed_types = conf.disallowed_types.iter().cloned().collect::>(); store.register_late_pass(move || Box::new(disallowed_type::DisallowedType::new(&disallowed_types))); @@ -2102,6 +2120,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_early_pass(move || Box::new(disallowed_script_idents::DisallowedScriptIdents::new(&scripts))); store.register_late_pass(|| Box::new(strlen_on_c_strings::StrlenOnCStrings)); store.register_late_pass(move || Box::new(self_named_constructors::SelfNamedConstructors)); + store.register_late_pass(move || Box::new(feature_name::FeatureName)); } #[rustfmt::skip] @@ -2180,7 +2199,6 @@ pub fn register_renamed(ls: &mut rustc_lint::LintStore) { ls.register_renamed("clippy::temporary_cstring_as_ptr", "temporary_cstring_as_ptr"); ls.register_renamed("clippy::panic_params", "non_fmt_panics"); ls.register_renamed("clippy::unknown_clippy_lints", "unknown_lints"); - ls.register_renamed("clippy::invalid_atomic_ordering", "invalid_atomic_ordering"); } // only exists to let the dogfood integration test works. diff --git a/clippy_lints/src/loops/for_kv_map.rs b/clippy_lints/src/loops/for_kv_map.rs index 82bf49f5b49..68bef2f4c8b 100644 --- a/clippy_lints/src/loops/for_kv_map.rs +++ b/clippy_lints/src/loops/for_kv_map.rs @@ -3,7 +3,7 @@ use clippy_utils::source::snippet; use clippy_utils::sugg; use clippy_utils::ty::is_type_diagnostic_item; -use clippy_utils::visitors::LocalUsedVisitor; +use clippy_utils::visitors::is_local_used; use rustc_hir::{BorrowKind, Expr, ExprKind, Mutability, Pat, PatKind}; use rustc_lint::LateContext; use rustc_middle::ty; @@ -66,9 +66,7 @@ pub(super) fn check<'tcx>( fn pat_is_wild<'tcx>(cx: &LateContext<'tcx>, pat: &'tcx PatKind<'_>, body: &'tcx Expr<'_>) -> bool { match *pat { PatKind::Wild => true, - PatKind::Binding(_, id, ident, None) if ident.as_str().starts_with('_') => { - !LocalUsedVisitor::new(cx, id).check_expr(body) - }, + PatKind::Binding(_, id, ident, None) if ident.as_str().starts_with('_') => !is_local_used(cx, body, id), _ => false, } } diff --git a/clippy_lints/src/loops/manual_flatten.rs b/clippy_lints/src/loops/manual_flatten.rs index 5852674da57..5b6e27085d5 100644 --- a/clippy_lints/src/loops/manual_flatten.rs +++ b/clippy_lints/src/loops/manual_flatten.rs @@ -2,6 +2,7 @@ use super::MANUAL_FLATTEN; use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::higher; +use clippy_utils::visitors::is_local_used; use clippy_utils::{is_lang_ctor, path_to_local_id}; use if_chain::if_chain; use rustc_errors::Applicability; @@ -37,7 +38,8 @@ pub(super) fn check<'tcx>( if_chain! { if let Some(inner_expr) = inner_expr; - if let Some(higher::IfLet { let_pat, let_expr, if_else: None, .. }) = higher::IfLet::hir(cx, inner_expr); + if let Some(higher::IfLet { let_pat, let_expr, if_then, if_else: None }) + = higher::IfLet::hir(cx, inner_expr); // Ensure match_expr in `if let` statement is the same as the pat from the for-loop if let PatKind::Binding(_, pat_hir_id, _, _) = pat.kind; if path_to_local_id(let_expr, pat_hir_id); @@ -46,6 +48,8 @@ pub(super) fn check<'tcx>( let some_ctor = is_lang_ctor(cx, qpath, OptionSome); let ok_ctor = is_lang_ctor(cx, qpath, ResultOk); if some_ctor || ok_ctor; + // Ensure epxr in `if let` is not used afterwards + if !is_local_used(cx, if_then, pat_hir_id); then { let if_let_type = if some_ctor { "Some" } else { "Ok" }; // Prepare the error message diff --git a/clippy_lints/src/loops/manual_memcpy.rs b/clippy_lints/src/loops/manual_memcpy.rs index 2296842e86f..df848e68802 100644 --- a/clippy_lints/src/loops/manual_memcpy.rs +++ b/clippy_lints/src/loops/manual_memcpy.rs @@ -204,11 +204,8 @@ fn print_offset(offset: MinifyingSugg<'static>) -> MinifyingSugg<'static> { impl<'a> MinifyingSugg<'a> { fn as_str(&self) -> &str { - // HACK: Don't sync to Clippy! Required because something with the `or_patterns` feature - // changed and this would now require parentheses. - match &self.0 { - Sugg::NonParen(s) | Sugg::MaybeParen(s) | Sugg::BinOp(_, s) => s.as_ref(), - } + let (Sugg::NonParen(s) | Sugg::MaybeParen(s) | Sugg::BinOp(_, s)) = &self.0; + s.as_ref() } fn into_sugg(self) -> Sugg<'a> { diff --git a/clippy_lints/src/loops/needless_collect.rs b/clippy_lints/src/loops/needless_collect.rs index 51d7def137e..f90ed7397e1 100644 --- a/clippy_lints/src/loops/needless_collect.rs +++ b/clippy_lints/src/loops/needless_collect.rs @@ -26,7 +26,7 @@ fn check_needless_collect_direct_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateCont if chain_method.ident.name == sym!(collect) && is_trait_method(cx, &args[0], sym::Iterator); then { let ty = cx.typeck_results().expr_ty(&args[0]); - let mut applicability = Applicability::MachineApplicable; + let mut applicability = Applicability::MaybeIncorrect; let is_empty_sugg = "next().is_none()".to_string(); let method_name = &*method.ident.name.as_str(); let sugg = if is_type_diagnostic_item(cx, ty, sym::vec_type) || @@ -113,7 +113,7 @@ fn check_needless_collect_indirect_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateCo (stmt.span, String::new()), (iter_call.span, iter_replacement) ], - Applicability::MachineApplicable,// MaybeIncorrect, + Applicability::MaybeIncorrect, ); }, ); diff --git a/clippy_lints/src/loops/needless_range_loop.rs b/clippy_lints/src/loops/needless_range_loop.rs index 3f77e7af927..f17e76d9682 100644 --- a/clippy_lints/src/loops/needless_range_loop.rs +++ b/clippy_lints/src/loops/needless_range_loop.rs @@ -2,10 +2,8 @@ use clippy_utils::diagnostics::{multispan_sugg, span_lint_and_then}; use clippy_utils::source::snippet; use clippy_utils::ty::has_iter_method; -use clippy_utils::visitors::LocalUsedVisitor; -use clippy_utils::{ - contains_name, higher, is_integer_const, match_trait_method, path_to_local_id, paths, sugg, SpanlessEq, -}; +use clippy_utils::visitors::is_local_used; +use clippy_utils::{contains_name, higher, is_integer_const, match_trait_method, paths, sugg, SpanlessEq}; use if_chain::if_chain; use rustc_ast::ast; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; @@ -256,43 +254,36 @@ fn check(&mut self, idx: &'tcx Expr<'_>, seqexpr: &'tcx Expr<'_>, expr: &'tcx Ex if let ExprKind::Path(ref seqpath) = seqexpr.kind; if let QPath::Resolved(None, seqvar) = *seqpath; if seqvar.segments.len() == 1; - let index_used_directly = path_to_local_id(idx, self.var); - let indexed_indirectly = { - let mut used_visitor = LocalUsedVisitor::new(self.cx, self.var); - walk_expr(&mut used_visitor, idx); - used_visitor.used - }; - if indexed_indirectly || index_used_directly; + if is_local_used(self.cx, idx, self.var); then { if self.prefer_mutable { self.indexed_mut.insert(seqvar.segments[0].ident.name); } + let index_used_directly = matches!(idx.kind, ExprKind::Path(_)); let res = self.cx.qpath_res(seqpath, seqexpr.hir_id); match res { Res::Local(hir_id) => { let parent_id = self.cx.tcx.hir().get_parent_item(expr.hir_id); let parent_def_id = self.cx.tcx.hir().local_def_id(parent_id); let extent = self.cx.tcx.region_scope_tree(parent_def_id).var_scope(hir_id.local_id); - if indexed_indirectly { - self.indexed_indirectly.insert(seqvar.segments[0].ident.name, Some(extent)); - } if index_used_directly { self.indexed_directly.insert( seqvar.segments[0].ident.name, (Some(extent), self.cx.typeck_results().node_type(seqexpr.hir_id)), ); + } else { + self.indexed_indirectly.insert(seqvar.segments[0].ident.name, Some(extent)); } return false; // no need to walk further *on the variable* } Res::Def(DefKind::Static | DefKind::Const, ..) => { - if indexed_indirectly { - self.indexed_indirectly.insert(seqvar.segments[0].ident.name, None); - } if index_used_directly { self.indexed_directly.insert( seqvar.segments[0].ident.name, (None, self.cx.typeck_results().node_type(seqexpr.hir_id)), ); + } else { + self.indexed_indirectly.insert(seqvar.segments[0].ident.name, None); } return false; // no need to walk further *on the variable* } diff --git a/clippy_lints/src/manual_map.rs b/clippy_lints/src/manual_map.rs index 161d8841490..385a3f546b9 100644 --- a/clippy_lints/src/manual_map.rs +++ b/clippy_lints/src/manual_map.rs @@ -5,12 +5,14 @@ use clippy_utils::ty::{is_type_diagnostic_item, peel_mid_ty_refs_is_mutable}; use clippy_utils::{ can_move_expr_to_closure, in_constant, is_else_clause, is_lang_ctor, is_lint_allowed, path_to_local_id, - peel_hir_expr_refs, + peel_hir_expr_refs, peel_hir_expr_while, CaptureKind, }; use rustc_ast::util::parser::PREC_POSTFIX; use rustc_errors::Applicability; use rustc_hir::LangItem::{OptionNone, OptionSome}; -use rustc_hir::{Arm, BindingAnnotation, Block, Expr, ExprKind, HirId, Mutability, Pat, PatKind}; +use rustc_hir::{ + def::Res, Arm, BindingAnnotation, Block, Expr, ExprKind, HirId, Mutability, Pat, PatKind, Path, QPath, +}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; use rustc_session::{declare_lint_pass, declare_tool_lint}; @@ -119,7 +121,9 @@ fn manage_lint<'tcx>( None => return, }; - if cx.typeck_results().expr_ty(some_expr) == cx.tcx.types.unit && !is_lint_allowed(cx, OPTION_MAP_UNIT_FN, expr.hir_id) { + if cx.typeck_results().expr_ty(some_expr) == cx.tcx.types.unit + && !is_lint_allowed(cx, OPTION_MAP_UNIT_FN, expr.hir_id) + { return; } @@ -128,10 +132,6 @@ fn manage_lint<'tcx>( return; } - if !can_move_expr_to_closure(cx, some_expr) { - return; - } - // Determine which binding mode to use. let explicit_ref = some_pat.contains_explicit_ref_binding(); let binding_ref = explicit_ref.or_else(|| (ty_ref_count != pat_ref_count).then(|| ty_mutability)); @@ -142,6 +142,30 @@ fn manage_lint<'tcx>( None => "", }; + match can_move_expr_to_closure(cx, some_expr) { + Some(captures) => { + // Check if captures the closure will need conflict with borrows made in the scrutinee. + // TODO: check all the references made in the scrutinee expression. This will require interacting + // with the borrow checker. Currently only `[.]*` is checked for. + if let Some(binding_ref_mutability) = binding_ref { + let e = peel_hir_expr_while(scrut, |e| match e.kind { + ExprKind::Field(e, _) | ExprKind::AddrOf(_, _, e) => Some(e), + _ => None, + }); + if let ExprKind::Path(QPath::Resolved(None, Path { res: Res::Local(l), .. })) = e.kind { + match captures.get(l) { + Some(CaptureKind::Value | CaptureKind::Ref(Mutability::Mut)) => return, + Some(CaptureKind::Ref(Mutability::Not)) if binding_ref_mutability == Mutability::Mut => { + return; + }, + Some(CaptureKind::Ref(Mutability::Not)) | None => (), + } + } + } + }, + None => return, + }; + let mut app = Applicability::MachineApplicable; // Remove address-of expressions from the scrutinee. Either `as_ref` will be called, or @@ -213,7 +237,7 @@ fn manage_lint<'tcx>( fn can_pass_as_func(cx: &LateContext<'tcx>, binding: HirId, expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> { match expr.kind { ExprKind::Call(func, [arg]) - if path_to_local_id (arg, binding) && cx.typeck_results().expr_adjustments(arg).is_empty() => + if path_to_local_id(arg, binding) && cx.typeck_results().expr_adjustments(arg).is_empty() => { Some(func) }, diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index 3d0da472ddc..131ed1d8925 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -6,7 +6,7 @@ use clippy_utils::source::{expr_block, indent_of, snippet, snippet_block, snippet_opt, snippet_with_applicability}; use clippy_utils::sugg::Sugg; use clippy_utils::ty::{implements_trait, is_type_diagnostic_item, match_type, peel_mid_ty_refs}; -use clippy_utils::visitors::LocalUsedVisitor; +use clippy_utils::visitors::is_local_used; use clippy_utils::{ get_parent_expr, in_macro, is_expn_of, is_lang_ctor, is_lint_allowed, is_refutable, is_unit_expr, is_wild, meets_msrv, msrvs, path_to_local, path_to_local_id, peel_hir_pat_refs, peel_n_hir_expr_refs, recurse_or_patterns, @@ -959,9 +959,7 @@ fn check_wild_err_arm<'tcx>(cx: &LateContext<'tcx>, ex: &Expr<'tcx>, arms: &[Arm // Looking for unused bindings (i.e.: `_e`) for pat in inner.iter() { if let PatKind::Binding(_, id, ident, None) = pat.kind { - if ident.as_str().starts_with('_') - && !LocalUsedVisitor::new(cx, id).check_expr(arm.body) - { + if ident.as_str().starts_with('_') && !is_local_used(cx, arm.body, id) { ident_bind_name = (&ident.name.as_str()).to_string(); matching_wild = true; } diff --git a/clippy_lints/src/methods/filter_next.rs b/clippy_lints/src/methods/filter_next.rs index 172714f6b01..bcf8d93b602 100644 --- a/clippy_lints/src/methods/filter_next.rs +++ b/clippy_lints/src/methods/filter_next.rs @@ -1,6 +1,6 @@ use clippy_utils::diagnostics::{span_lint, span_lint_and_sugg}; -use clippy_utils::is_trait_method; use clippy_utils::source::snippet; +use clippy_utils::ty::implements_trait; use rustc_errors::Applicability; use rustc_hir as hir; use rustc_lint::LateContext; @@ -16,7 +16,10 @@ pub(super) fn check<'tcx>( filter_arg: &'tcx hir::Expr<'_>, ) { // lint if caller of `.filter().next()` is an Iterator - if is_trait_method(cx, expr, sym::Iterator) { + let recv_impls_iterator = cx.tcx.get_diagnostic_item(sym::Iterator).map_or(false, |id| { + implements_trait(cx, cx.typeck_results().expr_ty(recv), id, &[]) + }); + if recv_impls_iterator { let msg = "called `filter(..).next()` on an `Iterator`. This is more succinctly expressed by calling \ `.find(..)` instead"; let filter_snippet = snippet(cx, filter_arg.span, ".."); diff --git a/clippy_lints/src/methods/manual_split_once.rs b/clippy_lints/src/methods/manual_split_once.rs new file mode 100644 index 00000000000..e273186d051 --- /dev/null +++ b/clippy_lints/src/methods/manual_split_once.rs @@ -0,0 +1,213 @@ +use clippy_utils::consts::{constant, Constant}; +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::source::snippet_with_context; +use clippy_utils::{is_diag_item_method, match_def_path, paths}; +use if_chain::if_chain; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind, HirId, LangItem, Node, QPath}; +use rustc_lint::LateContext; +use rustc_middle::ty::{self, adjustment::Adjust}; +use rustc_span::{symbol::sym, Span, SyntaxContext}; + +use super::MANUAL_SPLIT_ONCE; + +pub(super) fn check(cx: &LateContext<'_>, method_name: &str, expr: &Expr<'_>, self_arg: &Expr<'_>, pat_arg: &Expr<'_>) { + if !cx.typeck_results().expr_ty_adjusted(self_arg).peel_refs().is_str() { + return; + } + + let ctxt = expr.span.ctxt(); + let usage = match parse_iter_usage(cx, ctxt, cx.tcx.hir().parent_iter(expr.hir_id)) { + Some(x) => x, + None => return, + }; + let (method_name, msg) = if method_name == "splitn" { + ("split_once", "manual implementation of `split_once`") + } else { + ("rsplit_once", "manual implementation of `rsplit_once`") + }; + + let mut app = Applicability::MachineApplicable; + let self_snip = snippet_with_context(cx, self_arg.span, ctxt, "..", &mut app).0; + let pat_snip = snippet_with_context(cx, pat_arg.span, ctxt, "..", &mut app).0; + + match usage.kind { + IterUsageKind::NextTuple => { + span_lint_and_sugg( + cx, + MANUAL_SPLIT_ONCE, + usage.span, + msg, + "try this", + format!("{}.{}({})", self_snip, method_name, pat_snip), + app, + ); + }, + IterUsageKind::Next => { + let self_deref = { + let adjust = cx.typeck_results().expr_adjustments(self_arg); + if adjust.is_empty() { + String::new() + } else if cx.typeck_results().expr_ty(self_arg).is_box() + || adjust + .iter() + .any(|a| matches!(a.kind, Adjust::Deref(Some(_))) || a.target.is_box()) + { + format!("&{}", "*".repeat(adjust.len() - 1)) + } else { + "*".repeat(adjust.len() - 2) + } + }; + let sugg = if usage.unwrap_kind.is_some() { + format!( + "{}.{}({}).map_or({}{}, |x| x.0)", + &self_snip, method_name, pat_snip, self_deref, &self_snip + ) + } else { + format!( + "Some({}.{}({}).map_or({}{}, |x| x.0))", + &self_snip, method_name, pat_snip, self_deref, &self_snip + ) + }; + + span_lint_and_sugg(cx, MANUAL_SPLIT_ONCE, usage.span, msg, "try this", sugg, app); + }, + IterUsageKind::Second => { + let access_str = match usage.unwrap_kind { + Some(UnwrapKind::Unwrap) => ".unwrap().1", + Some(UnwrapKind::QuestionMark) => "?.1", + None => ".map(|x| x.1)", + }; + span_lint_and_sugg( + cx, + MANUAL_SPLIT_ONCE, + usage.span, + msg, + "try this", + format!("{}.{}({}){}", self_snip, method_name, pat_snip, access_str), + app, + ); + }, + } +} + +enum IterUsageKind { + Next, + Second, + NextTuple, +} + +enum UnwrapKind { + Unwrap, + QuestionMark, +} + +struct IterUsage { + kind: IterUsageKind, + unwrap_kind: Option, + span: Span, +} + +fn parse_iter_usage( + cx: &LateContext<'tcx>, + ctxt: SyntaxContext, + mut iter: impl Iterator)>, +) -> Option { + let (kind, span) = match iter.next() { + Some((_, Node::Expr(e))) if e.span.ctxt() == ctxt => { + let (name, args) = if let ExprKind::MethodCall(name, _, [_, args @ ..], _) = e.kind { + (name, args) + } else { + return None; + }; + let did = cx.typeck_results().type_dependent_def_id(e.hir_id)?; + let iter_id = cx.tcx.get_diagnostic_item(sym::Iterator)?; + + match (&*name.ident.as_str(), args) { + ("next", []) if cx.tcx.trait_of_item(did) == Some(iter_id) => (IterUsageKind::Next, e.span), + ("next_tuple", []) => { + if_chain! { + if match_def_path(cx, did, &paths::ITERTOOLS_NEXT_TUPLE); + if let ty::Adt(adt_def, subs) = cx.typeck_results().expr_ty(e).kind(); + if cx.tcx.is_diagnostic_item(sym::option_type, adt_def.did); + if let ty::Tuple(subs) = subs.type_at(0).kind(); + if subs.len() == 2; + then { + return Some(IterUsage { kind: IterUsageKind::NextTuple, span: e.span, unwrap_kind: None }); + } else { + return None; + } + } + }, + ("nth" | "skip", [idx_expr]) if cx.tcx.trait_of_item(did) == Some(iter_id) => { + if let Some((Constant::Int(idx), _)) = constant(cx, cx.typeck_results(), idx_expr) { + let span = if name.ident.as_str() == "nth" { + e.span + } else { + if_chain! { + if let Some((_, Node::Expr(next_expr))) = iter.next(); + if let ExprKind::MethodCall(next_name, _, [_], _) = next_expr.kind; + if next_name.ident.name == sym::next; + if next_expr.span.ctxt() == ctxt; + if let Some(next_id) = cx.typeck_results().type_dependent_def_id(next_expr.hir_id); + if cx.tcx.trait_of_item(next_id) == Some(iter_id); + then { + next_expr.span + } else { + return None; + } + } + }; + match idx { + 0 => (IterUsageKind::Next, span), + 1 => (IterUsageKind::Second, span), + _ => return None, + } + } else { + return None; + } + }, + _ => return None, + } + }, + _ => return None, + }; + + let (unwrap_kind, span) = if let Some((_, Node::Expr(e))) = iter.next() { + match e.kind { + ExprKind::Call( + Expr { + kind: ExprKind::Path(QPath::LangItem(LangItem::TryTraitBranch, _)), + .. + }, + _, + ) => { + let parent_span = e.span.parent().unwrap(); + if parent_span.ctxt() == ctxt { + (Some(UnwrapKind::QuestionMark), parent_span) + } else { + (None, span) + } + }, + _ if e.span.ctxt() != ctxt => (None, span), + ExprKind::MethodCall(name, _, [_], _) + if name.ident.name == sym::unwrap + && cx + .typeck_results() + .type_dependent_def_id(e.hir_id) + .map_or(false, |id| is_diag_item_method(cx, id, sym::option_type)) => + { + (Some(UnwrapKind::Unwrap), e.span) + }, + _ => (None, span), + } + } else { + (None, span) + }; + + Some(IterUsage { + kind, + unwrap_kind, + span, + }) +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 9626cf79dc1..e89b2d295b9 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -33,6 +33,7 @@ mod iter_skip_next; mod iterator_step_by_zero; mod manual_saturating_arithmetic; +mod manual_split_once; mod manual_str_repeat; mod map_collect_result_unit; mod map_flatten; @@ -64,6 +65,7 @@ mod zst_offset; use bind_instead_of_map::BindInsteadOfMap; +use clippy_utils::consts::{constant, Constant}; use clippy_utils::diagnostics::{span_lint, span_lint_and_help}; use clippy_utils::ty::{contains_adt_constructor, contains_ty, implements_trait, is_copy, is_type_diagnostic_item}; use clippy_utils::{contains_return, get_trait_def_id, in_macro, iter_input_pats, meets_msrv, msrvs, paths, return_ty}; @@ -1771,6 +1773,29 @@ "manual implementation of `str::repeat`" } +declare_clippy_lint! { + /// **What it does:** Checks for usages of `str::splitn(2, _)` + /// + /// **Why is this bad?** `split_once` is both clearer in intent and slightly more efficient. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust,ignore + /// // Bad + /// let (key, value) = _.splitn(2, '=').next_tuple()?; + /// let value = _.splitn(2, '=').nth(1)?; + /// + /// // Good + /// let (key, value) = _.split_once('=')?; + /// let value = _.split_once('=')?.1; + /// ``` + pub MANUAL_SPLIT_ONCE, + complexity, + "replace `.splitn(2, pat)` with `.split_once(pat)`" +} + pub struct Methods { avoid_breaking_exported_api: bool, msrv: Option, @@ -1848,7 +1873,8 @@ pub fn new(avoid_breaking_exported_api: bool, msrv: Option) -> Sel IMPLICIT_CLONE, SUSPICIOUS_SPLITN, MANUAL_STR_REPEAT, - EXTEND_WITH_DRAIN + EXTEND_WITH_DRAIN, + MANUAL_SPLIT_ONCE ]); /// Extracts a method call name, args, and `Span` of the method name. @@ -2176,8 +2202,18 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio unnecessary_lazy_eval::check(cx, expr, recv, arg, "or"); } }, - ("splitn" | "splitn_mut" | "rsplitn" | "rsplitn_mut", [count_arg, _]) => { - suspicious_splitn::check(cx, name, expr, recv, count_arg); + ("splitn" | "rsplitn", [count_arg, pat_arg]) => { + if let Some((Constant::Int(count), _)) = constant(cx, cx.typeck_results(), count_arg) { + suspicious_splitn::check(cx, name, expr, recv, count); + if count == 2 && meets_msrv(msrv, &msrvs::STR_SPLIT_ONCE) { + manual_split_once::check(cx, name, expr, recv, pat_arg); + } + } + }, + ("splitn_mut" | "rsplitn_mut", [count_arg, _]) => { + if let Some((Constant::Int(count), _)) = constant(cx, cx.typeck_results(), count_arg) { + suspicious_splitn::check(cx, name, expr, recv, count); + } }, ("step_by", [arg]) => iterator_step_by_zero::check(cx, expr, arg), ("to_os_string" | "to_owned" | "to_path_buf" | "to_vec", []) => { diff --git a/clippy_lints/src/methods/suspicious_splitn.rs b/clippy_lints/src/methods/suspicious_splitn.rs index a271df60572..1c546a15bf6 100644 --- a/clippy_lints/src/methods/suspicious_splitn.rs +++ b/clippy_lints/src/methods/suspicious_splitn.rs @@ -1,4 +1,3 @@ -use clippy_utils::consts::{constant, Constant}; use clippy_utils::diagnostics::span_lint_and_note; use if_chain::if_chain; use rustc_ast::LitKind; @@ -8,15 +7,8 @@ use super::SUSPICIOUS_SPLITN; -pub(super) fn check( - cx: &LateContext<'_>, - method_name: &str, - expr: &Expr<'_>, - self_arg: &Expr<'_>, - count_arg: &Expr<'_>, -) { +pub(super) fn check(cx: &LateContext<'_>, method_name: &str, expr: &Expr<'_>, self_arg: &Expr<'_>, count: u128) { if_chain! { - if let Some((Constant::Int(count), _)) = constant(cx, cx.typeck_results(), count_arg); if count <= 1; if let Some(call_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id); if let Some(impl_id) = cx.tcx.impl_of_method(call_id); @@ -24,9 +16,9 @@ pub(super) fn check( if lang_items.slice_impl() == Some(impl_id) || lang_items.str_impl() == Some(impl_id); then { // Ignore empty slice and string literals when used with a literal count. - if (matches!(self_arg.kind, ExprKind::Array([])) + if matches!(self_arg.kind, ExprKind::Array([])) || matches!(self_arg.kind, ExprKind::Lit(Spanned { node: LitKind::Str(s, _), .. }) if s.is_empty()) - ) && matches!(count_arg.kind, ExprKind::Lit(_)) + { return; } diff --git a/clippy_lints/src/missing_doc.rs b/clippy_lints/src/missing_doc.rs index 940eee7a788..55d2e29e8bc 100644 --- a/clippy_lints/src/missing_doc.rs +++ b/clippy_lints/src/missing_doc.rs @@ -7,7 +7,8 @@ use clippy_utils::attrs::is_doc_hidden; use clippy_utils::diagnostics::span_lint; -use rustc_ast::ast; +use if_chain::if_chain; +use rustc_ast::ast::{self, MetaItem, MetaItemKind}; use rustc_hir as hir; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::ty; @@ -55,6 +56,20 @@ fn doc_hidden(&self) -> bool { *self.doc_hidden_stack.last().expect("empty doc_hidden_stack") } + fn has_include(meta: Option) -> bool { + if_chain! { + if let Some(meta) = meta; + if let MetaItemKind::List(list) = meta.kind; + if let Some(meta) = list.get(0); + if let Some(name) = meta.ident(); + then { + name.name == sym::include + } else { + false + } + } + } + fn check_missing_docs_attrs( &self, cx: &LateContext<'_>, @@ -80,7 +95,7 @@ fn check_missing_docs_attrs( let has_doc = attrs .iter() - .any(|a| a.doc_str().is_some()); + .any(|a| a.doc_str().is_some() || Self::has_include(a.meta())); if !has_doc { span_lint( cx, diff --git a/clippy_lints/src/module_style.rs b/clippy_lints/src/module_style.rs new file mode 100644 index 00000000000..80a930d0c54 --- /dev/null +++ b/clippy_lints/src/module_style.rs @@ -0,0 +1,178 @@ +use std::{ + ffi::OsString, + path::{Component, Path}, +}; + +use rustc_ast::ast; +use rustc_data_structures::fx::{FxHashMap, FxHashSet}; +use rustc_lint::{EarlyContext, EarlyLintPass, Level, LintContext}; +use rustc_session::{declare_tool_lint, impl_lint_pass}; +use rustc_span::{FileName, RealFileName, SourceFile, Span, SyntaxContext}; + +declare_clippy_lint! { + /// ### What it does + /// Checks that module layout uses only self named module files, bans mod.rs files. + /// + /// ### Why is this bad? + /// Having multiple module layout styles in a project can be confusing. + /// + /// ### Example + /// ```text + /// src/ + /// stuff/ + /// stuff_files.rs + /// mod.rs + /// lib.rs + /// ``` + /// Use instead: + /// ```text + /// src/ + /// stuff/ + /// stuff_files.rs + /// stuff.rs + /// lib.rs + /// ``` + pub MOD_MODULE_FILES, + restriction, + "checks that module layout is consistent" +} + +declare_clippy_lint! { + /// ### What it does + /// Checks that module layout uses only mod.rs files. + /// + /// ### Why is this bad? + /// Having multiple module layout styles in a project can be confusing. + /// + /// ### Example + /// ```text + /// src/ + /// stuff/ + /// stuff_files.rs + /// stuff.rs + /// lib.rs + /// ``` + /// Use instead: + /// ```text + /// src/ + /// stuff/ + /// stuff_files.rs + /// mod.rs + /// lib.rs + /// ``` + + pub SELF_NAMED_MODULE_FILES, + restriction, + "checks that module layout is consistent" +} + +pub struct ModStyle; + +impl_lint_pass!(ModStyle => [MOD_MODULE_FILES, SELF_NAMED_MODULE_FILES]); + +impl EarlyLintPass for ModStyle { + fn check_crate(&mut self, cx: &EarlyContext<'_>, _: &ast::Crate) { + if cx.builder.lint_level(MOD_MODULE_FILES).0 == Level::Allow + && cx.builder.lint_level(SELF_NAMED_MODULE_FILES).0 == Level::Allow + { + return; + } + + let files = cx.sess.source_map().files(); + + let trim_to_src = if let RealFileName::LocalPath(p) = &cx.sess.opts.working_dir { + p.to_string_lossy() + } else { + return; + }; + + // `folder_segments` is all unique folder path segments `path/to/foo.rs` gives + // `[path, to]` but not foo + let mut folder_segments = FxHashSet::default(); + // `mod_folders` is all the unique folder names that contain a mod.rs file + let mut mod_folders = FxHashSet::default(); + // `file_map` maps file names to the full path including the file name + // `{ foo => path/to/foo.rs, .. } + let mut file_map = FxHashMap::default(); + for file in files.iter() { + match &file.name { + FileName::Real(RealFileName::LocalPath(lp)) + if lp.to_string_lossy().starts_with(trim_to_src.as_ref()) => + { + let p = lp.to_string_lossy(); + let path = Path::new(p.trim_start_matches(trim_to_src.as_ref())); + if let Some(stem) = path.file_stem() { + file_map.insert(stem.to_os_string(), (file, path.to_owned())); + } + process_paths_for_mod_files(path, &mut folder_segments, &mut mod_folders); + check_self_named_mod_exists(cx, path, file); + } + _ => {}, + } + } + + for folder in &folder_segments { + if !mod_folders.contains(folder) { + if let Some((file, path)) = file_map.get(folder) { + let mut correct = path.clone(); + correct.pop(); + correct.push(folder); + correct.push("mod.rs"); + cx.struct_span_lint( + SELF_NAMED_MODULE_FILES, + Span::new(file.start_pos, file.start_pos, SyntaxContext::root()), + |build| { + let mut lint = + build.build(&format!("`mod.rs` files are required, found `{}`", path.display())); + lint.help(&format!("move `{}` to `{}`", path.display(), correct.display(),)); + lint.emit(); + }, + ); + } + } + } + } +} + +/// For each `path` we add each folder component to `folder_segments` and if the file name +/// is `mod.rs` we add it's parent folder to `mod_folders`. +fn process_paths_for_mod_files( + path: &Path, + folder_segments: &mut FxHashSet, + mod_folders: &mut FxHashSet, +) { + let mut comp = path.components().rev().peekable(); + let _ = comp.next(); + if path.ends_with("mod.rs") { + mod_folders.insert(comp.peek().map(|c| c.as_os_str().to_owned()).unwrap_or_default()); + } + let folders = comp + .filter_map(|c| { + if let Component::Normal(s) = c { + Some(s.to_os_string()) + } else { + None + } + }) + .collect::>(); + folder_segments.extend(folders); +} + +/// Checks every path for the presence of `mod.rs` files and emits the lint if found. +fn check_self_named_mod_exists(cx: &EarlyContext<'_>, path: &Path, file: &SourceFile) { + if path.ends_with("mod.rs") { + let mut mod_file = path.to_path_buf(); + mod_file.pop(); + mod_file.set_extension("rs"); + + cx.struct_span_lint( + MOD_MODULE_FILES, + Span::new(file.start_pos, file.start_pos, SyntaxContext::root()), + |build| { + let mut lint = build.build(&format!("`mod.rs` files are not allowed, found `{}`", path.display())); + lint.help(&format!("move `{}` to `{}`", path.display(), mod_file.display(),)); + lint.emit(); + }, + ); + } +} diff --git a/clippy_lints/src/mutable_debug_assertion.rs b/clippy_lints/src/mutable_debug_assertion.rs index 9b44cf9d43a..ee50891cc31 100644 --- a/clippy_lints/src/mutable_debug_assertion.rs +++ b/clippy_lints/src/mutable_debug_assertion.rs @@ -87,10 +87,6 @@ fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { self.found = true; return; }, - ExprKind::If(..) => { - self.found = true; - return; - }, ExprKind::Path(_) => { if let Some(adj) = self.cx.typeck_results().adjustments().get(expr.hir_id) { if adj diff --git a/clippy_lints/src/no_effect.rs b/clippy_lints/src/no_effect.rs index 28e9e6f438e..c5a5cde4b11 100644 --- a/clippy_lints/src/no_effect.rs +++ b/clippy_lints/src/no_effect.rs @@ -96,28 +96,63 @@ fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) { if has_no_effect(cx, expr) { span_lint_hir(cx, NO_EFFECT, expr.hir_id, stmt.span, "statement with no effect"); } else if let Some(reduced) = reduce_expression(cx, expr) { - let mut snippet = String::new(); - for e in reduced { + for e in &reduced { if e.span.from_expansion() { return; } - if let Some(snip) = snippet_opt(cx, e.span) { - snippet.push_str(&snip); - snippet.push(';'); - } else { - return; - } } - span_lint_hir_and_then( - cx, - UNNECESSARY_OPERATION, - expr.hir_id, - stmt.span, - "statement can be reduced", - |diag| { - diag.span_suggestion(stmt.span, "replace it with", snippet, Applicability::MachineApplicable); - }, - ); + if let ExprKind::Index(..) = &expr.kind { + let snippet; + if_chain! { + if let Some(arr) = snippet_opt(cx, reduced[0].span); + if let Some(func) = snippet_opt(cx, reduced[1].span); + then { + snippet = format!("assert!({}.len() > {});", &arr, &func); + } else { + return; + } + } + span_lint_hir_and_then( + cx, + UNNECESSARY_OPERATION, + expr.hir_id, + stmt.span, + "unnecessary operation", + |diag| { + diag.span_suggestion( + stmt.span, + "statement can be written as", + snippet, + Applicability::MaybeIncorrect, + ); + }, + ); + } else { + let mut snippet = String::new(); + for e in reduced { + if let Some(snip) = snippet_opt(cx, e.span) { + snippet.push_str(&snip); + snippet.push(';'); + } else { + return; + } + } + span_lint_hir_and_then( + cx, + UNNECESSARY_OPERATION, + expr.hir_id, + stmt.span, + "unnecessary operation", + |diag| { + diag.span_suggestion( + stmt.span, + "statement can be reduced to", + snippet, + Applicability::MachineApplicable, + ); + }, + ); + } } } } diff --git a/clippy_lints/src/option_if_let_else.rs b/clippy_lints/src/option_if_let_else.rs index eff3d3abff8..80b4f544b1a 100644 --- a/clippy_lints/src/option_if_let_else.rs +++ b/clippy_lints/src/option_if_let_else.rs @@ -2,12 +2,13 @@ use clippy_utils::higher; use clippy_utils::sugg::Sugg; use clippy_utils::ty::is_type_diagnostic_item; -use clippy_utils::usage::contains_return_break_continue_macro; -use clippy_utils::{eager_or_lazy, in_macro, is_else_clause, is_lang_ctor}; +use clippy_utils::{ + can_move_expr_to_closure, eager_or_lazy, in_macro, is_else_clause, is_lang_ctor, peel_hir_expr_while, CaptureKind, +}; use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir::LangItem::OptionSome; -use rustc_hir::{BindingAnnotation, Block, Expr, ExprKind, Mutability, PatKind, UnOp}; +use rustc_hir::{def::Res, BindingAnnotation, Block, Expr, ExprKind, Mutability, PatKind, Path, QPath, UnOp}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::sym; @@ -58,7 +59,7 @@ /// }, |foo| foo); /// ``` pub OPTION_IF_LET_ELSE, - pedantic, + nursery, "reimplementation of Option::map_or" } @@ -125,20 +126,29 @@ fn format_option_in_sugg(cx: &LateContext<'_>, cond_expr: &Expr<'_>, as_ref: boo fn detect_option_if_let_else<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> Option { if_chain! { if !in_macro(expr.span); // Don't lint macros, because it behaves weirdly - if let Some(higher::IfLet { let_pat, let_expr, if_then, if_else: Some(if_else) }) = higher::IfLet::hir(cx, expr); + if let Some(higher::IfLet { let_pat, let_expr, if_then, if_else: Some(if_else) }) + = higher::IfLet::hir(cx, expr); if !is_else_clause(cx.tcx, expr); if !is_result_ok(cx, let_expr); // Don't lint on Result::ok because a different lint does it already if let PatKind::TupleStruct(struct_qpath, [inner_pat], _) = &let_pat.kind; if is_lang_ctor(cx, struct_qpath, OptionSome); if let PatKind::Binding(bind_annotation, _, id, _) = &inner_pat.kind; - if !contains_return_break_continue_macro(if_then); - if !contains_return_break_continue_macro(if_else); + if let Some(some_captures) = can_move_expr_to_closure(cx, if_then); + if let Some(none_captures) = can_move_expr_to_closure(cx, if_else); + if some_captures + .iter() + .filter_map(|(id, &c)| none_captures.get(id).map(|&c2| (c, c2))) + .all(|(x, y)| x.is_imm_ref() && y.is_imm_ref()); then { let capture_mut = if bind_annotation == &BindingAnnotation::Mutable { "mut " } else { "" }; let some_body = extract_body_from_expr(if_then)?; let none_body = extract_body_from_expr(if_else)?; - let method_sugg = if eager_or_lazy::is_eagerness_candidate(cx, none_body) { "map_or" } else { "map_or_else" }; + let method_sugg = if eager_or_lazy::is_eagerness_candidate(cx, none_body) { + "map_or" + } else { + "map_or_else" + }; let capture_name = id.name.to_ident_string(); let (as_ref, as_mut) = match &let_expr.kind { ExprKind::AddrOf(_, Mutability::Not, _) => (true, false), @@ -150,6 +160,24 @@ fn detect_option_if_let_else<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> ExprKind::Unary(UnOp::Deref, expr) | ExprKind::AddrOf(_, _, expr) => expr, _ => let_expr, }; + // Check if captures the closure will need conflict with borrows made in the scrutinee. + // TODO: check all the references made in the scrutinee expression. This will require interacting + // with the borrow checker. Currently only `[.]*` is checked for. + if as_ref || as_mut { + let e = peel_hir_expr_while(cond_expr, |e| match e.kind { + ExprKind::Field(e, _) | ExprKind::AddrOf(_, _, e) => Some(e), + _ => None, + }); + if let ExprKind::Path(QPath::Resolved(None, Path { res: Res::Local(local_id), .. })) = e.kind { + match some_captures.get(local_id) + .or_else(|| (method_sugg == "map_or_else").then(|| ()).and_then(|_| none_captures.get(local_id))) + { + Some(CaptureKind::Value | CaptureKind::Ref(Mutability::Mut)) => return None, + Some(CaptureKind::Ref(Mutability::Not)) if as_mut => return None, + Some(CaptureKind::Ref(Mutability::Not)) | None => (), + } + } + } Some(OptionIfLetElseOccurence { option: format_option_in_sugg(cx, cond_expr, as_ref, as_mut), method_sugg: method_sugg.to_string(), diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index 7b6a0894e6d..17c9e3e7541 100644 --- a/clippy_lints/src/question_mark.rs +++ b/clippy_lints/src/question_mark.rs @@ -97,7 +97,8 @@ fn check_is_none_and_early_return_none(cx: &LateContext<'_>, expr: &Expr<'_>) { fn check_if_let_some_and_early_return_none(cx: &LateContext<'_>, expr: &Expr<'_>) { if_chain! { - if let Some(higher::IfLet { let_pat, let_expr, if_then, if_else: Some(if_else) }) = higher::IfLet::hir(cx, expr); + if let Some(higher::IfLet { let_pat, let_expr, if_then, if_else: Some(if_else) }) + = higher::IfLet::hir(cx, expr); if Self::is_option(cx, let_expr); if let PatKind::TupleStruct(ref path1, fields, None) = let_pat.kind; diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index f5e43264a5c..cfa12ef3a32 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -625,7 +625,10 @@ fn visit_terminator(&mut self, terminator: &mir::Terminator<'_>, _loc: mir::Loca .flat_map(HybridBitSet::iter) .collect(); - if ContainsRegion(self.cx.tcx).visit_ty(self.body.local_decls[*dest].ty).is_break() { + if ContainsRegion(self.cx.tcx) + .visit_ty(self.body.local_decls[*dest].ty) + .is_break() + { mutable_variables.push(*dest); } diff --git a/clippy_lints/src/types/mod.rs b/clippy_lints/src/types/mod.rs index 371bb8b445a..9588de8459c 100644 --- a/clippy_lints/src/types/mod.rs +++ b/clippy_lints/src/types/mod.rs @@ -295,6 +295,7 @@ pub struct Types { vec_box_size_threshold: u64, type_complexity_threshold: u64, + avoid_breaking_exported_api: bool, } impl_lint_pass!(Types => [BOX_VEC, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX, REDUNDANT_ALLOCATION, RC_BUFFER, RC_MUTEX, TYPE_COMPLEXITY]); @@ -308,19 +309,31 @@ fn check_fn(&mut self, cx: &LateContext<'_>, _: FnKind<'_>, decl: &FnDecl<'_>, _ false }; + let is_exported = cx.access_levels.is_exported(cx.tcx.hir().local_def_id(id)); + self.check_fn_decl( cx, decl, CheckTyContext { is_in_trait_impl, + is_exported, ..CheckTyContext::default() }, ); } fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) { + let is_exported = cx.access_levels.is_exported(item.def_id); + match item.kind { - ItemKind::Static(ty, _, _) | ItemKind::Const(ty, _) => self.check_ty(cx, ty, CheckTyContext::default()), + ItemKind::Static(ty, _, _) | ItemKind::Const(ty, _) => self.check_ty( + cx, + ty, + CheckTyContext { + is_exported, + ..CheckTyContext::default() + }, + ), // functions, enums, structs, impls and traits are covered _ => (), } @@ -342,15 +355,31 @@ fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx ImplItem<'_>) } fn check_field_def(&mut self, cx: &LateContext<'_>, field: &hir::FieldDef<'_>) { - self.check_ty(cx, field.ty, CheckTyContext::default()); + let is_exported = cx.access_levels.is_exported(cx.tcx.hir().local_def_id(field.hir_id)); + + self.check_ty( + cx, + field.ty, + CheckTyContext { + is_exported, + ..CheckTyContext::default() + }, + ); } - fn check_trait_item(&mut self, cx: &LateContext<'_>, item: &TraitItem<'_>) { + fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &TraitItem<'_>) { + let is_exported = cx.access_levels.is_exported(item.def_id); + + let context = CheckTyContext { + is_exported, + ..CheckTyContext::default() + }; + match item.kind { TraitItemKind::Const(ty, _) | TraitItemKind::Type(_, Some(ty)) => { - self.check_ty(cx, ty, CheckTyContext::default()); + self.check_ty(cx, ty, context); }, - TraitItemKind::Fn(ref sig, _) => self.check_fn_decl(cx, sig.decl, CheckTyContext::default()), + TraitItemKind::Fn(ref sig, _) => self.check_fn_decl(cx, sig.decl, context), TraitItemKind::Type(..) => (), } } @@ -370,10 +399,11 @@ fn check_local(&mut self, cx: &LateContext<'_>, local: &Local<'_>) { } impl Types { - pub fn new(vec_box_size_threshold: u64, type_complexity_threshold: u64) -> Self { + pub fn new(vec_box_size_threshold: u64, type_complexity_threshold: u64, avoid_breaking_exported_api: bool) -> Self { Self { vec_box_size_threshold, type_complexity_threshold, + avoid_breaking_exported_api, } } @@ -410,17 +440,24 @@ fn check_ty(&mut self, cx: &LateContext<'_>, hir_ty: &hir::Ty<'_>, mut context: let hir_id = hir_ty.hir_id; let res = cx.qpath_res(qpath, hir_id); if let Some(def_id) = res.opt_def_id() { - let mut triggered = false; - triggered |= box_vec::check(cx, hir_ty, qpath, def_id); - triggered |= redundant_allocation::check(cx, hir_ty, qpath, def_id); - triggered |= rc_buffer::check(cx, hir_ty, qpath, def_id); - triggered |= vec_box::check(cx, hir_ty, qpath, def_id, self.vec_box_size_threshold); - triggered |= option_option::check(cx, hir_ty, qpath, def_id); - triggered |= linked_list::check(cx, hir_ty, def_id); - triggered |= rc_mutex::check(cx, hir_ty, qpath, def_id); + if self.is_type_change_allowed(context) { + // All lints that are being checked in this block are guarded by + // the `avoid_breaking_exported_api` configuration. When adding a + // new lint, please also add the name to the configuration documentation + // in `clippy_lints::utils::conf.rs` - if triggered { - return; + let mut triggered = false; + triggered |= box_vec::check(cx, hir_ty, qpath, def_id); + triggered |= redundant_allocation::check(cx, hir_ty, qpath, def_id); + triggered |= rc_buffer::check(cx, hir_ty, qpath, def_id); + triggered |= vec_box::check(cx, hir_ty, qpath, def_id, self.vec_box_size_threshold); + triggered |= option_option::check(cx, hir_ty, qpath, def_id); + triggered |= linked_list::check(cx, hir_ty, def_id); + triggered |= rc_mutex::check(cx, hir_ty, qpath, def_id); + + if triggered { + return; + } } } match *qpath { @@ -487,11 +524,21 @@ fn check_ty(&mut self, cx: &LateContext<'_>, hir_ty: &hir::Ty<'_>, mut context: _ => {}, } } + + /// This function checks if the type is allowed to change in the current context + /// based on the `avoid_breaking_exported_api` configuration + fn is_type_change_allowed(&self, context: CheckTyContext) -> bool { + !(context.is_exported && self.avoid_breaking_exported_api) + } } +#[allow(clippy::struct_excessive_bools)] #[derive(Clone, Copy, Default)] struct CheckTyContext { is_in_trait_impl: bool, + /// `true` for types on local variables. is_local: bool, + /// `true` for types that are part of the public API. + is_exported: bool, is_nested_call: bool, } diff --git a/clippy_lints/src/types/rc_mutex.rs b/clippy_lints/src/types/rc_mutex.rs index bd7a0ee6408..12db7afb81c 100644 --- a/clippy_lints/src/types/rc_mutex.rs +++ b/clippy_lints/src/types/rc_mutex.rs @@ -1,4 +1,4 @@ -use clippy_utils::diagnostics::span_lint; +use clippy_utils::diagnostics::span_lint_and_help; use clippy_utils::is_ty_param_diagnostic_item; use if_chain::if_chain; use rustc_hir::{self as hir, def_id::DefId, QPath}; @@ -11,13 +11,14 @@ pub(super) fn check(cx: &LateContext<'_>, hir_ty: &hir::Ty<'_>, qpath: &QPath<'_ if_chain! { if cx.tcx.is_diagnostic_item(sym::Rc, def_id) ; if let Some(_) = is_ty_param_diagnostic_item(cx, qpath, sym!(mutex_type)) ; - - then{ - span_lint( + then { + span_lint_and_help( cx, RC_MUTEX, hir_ty.span, - "found `Rc>`. Consider using `Rc>` or `Arc>` instead", + "usage of `Rc>`", + None, + "consider using `Rc>` or `Arc>` instead", ); return true; } diff --git a/clippy_lints/src/types/redundant_allocation.rs b/clippy_lints/src/types/redundant_allocation.rs index 8e83dcbf704..ac7bdd6a1eb 100644 --- a/clippy_lints/src/types/redundant_allocation.rs +++ b/clippy_lints/src/types/redundant_allocation.rs @@ -54,7 +54,13 @@ pub(super) fn check(cx: &LateContext<'_>, hir_ty: &hir::Ty<'_>, qpath: &QPath<'_ _ => return false, }; let inner_span = match get_qpath_generic_tys(inner_qpath).next() { - Some(ty) => ty.span, + Some(ty) => { + // Box> is smaller than Box because of wide pointers + if matches!(ty.kind, TyKind::TraitObject(..)) { + return false; + } + ty.span + }, None => return false, }; if inner_sym == outer_sym { diff --git a/clippy_lints/src/unnecessary_sort_by.rs b/clippy_lints/src/unnecessary_sort_by.rs index 97b1b2dae3c..61670fe124e 100644 --- a/clippy_lints/src/unnecessary_sort_by.rs +++ b/clippy_lints/src/unnecessary_sort_by.rs @@ -218,7 +218,10 @@ fn detect_lint(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option { fn expr_borrows(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { let ty = cx.typeck_results().expr_ty(expr); - matches!(ty.kind(), ty::Ref(..)) || ty.walk(cx.tcx).any(|arg| matches!(arg.unpack(), GenericArgKind::Lifetime(_))) + matches!(ty.kind(), ty::Ref(..)) + || ty + .walk(cx.tcx) + .any(|arg| matches!(arg.unpack(), GenericArgKind::Lifetime(_))) } impl LateLintPass<'_> for UnnecessarySortBy { diff --git a/clippy_lints/src/unused_self.rs b/clippy_lints/src/unused_self.rs index 658ac81f6ea..e7e249c79a2 100644 --- a/clippy_lints/src/unused_self.rs +++ b/clippy_lints/src/unused_self.rs @@ -1,5 +1,5 @@ use clippy_utils::diagnostics::span_lint_and_help; -use clippy_utils::visitors::LocalUsedVisitor; +use clippy_utils::visitors::is_local_used; use if_chain::if_chain; use rustc_hir::{Impl, ImplItem, ImplItemKind, ItemKind}; use rustc_lint::{LateContext, LateLintPass}; @@ -50,8 +50,7 @@ fn check_impl_item(&mut self, cx: &LateContext<'tcx>, impl_item: &ImplItem<'_>) if let ImplItemKind::Fn(.., body_id) = &impl_item.kind; let body = cx.tcx.hir().body(*body_id); if let [self_param, ..] = body.params; - let self_hir_id = self_param.pat.hir_id; - if !LocalUsedVisitor::new(cx, self_hir_id).check_body(body); + if !is_local_used(cx, body, self_param.pat.hir_id); then { span_lint_and_help( cx, diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs index c192f9094a8..881dc321930 100644 --- a/clippy_lints/src/utils/conf.rs +++ b/clippy_lints/src/utils/conf.rs @@ -31,9 +31,6 @@ fn from_error(error: impl Error) -> Self { } } -/// Note that the configuration parsing currently doesn't support documentation that will -/// that spans over several lines. This will be possible with the new implementation -/// See (rust-clippy#7172) macro_rules! define_Conf { ($( $(#[doc = $doc:literal])+ @@ -130,13 +127,12 @@ pub(crate) fn get_configuration_metadata() -> Vec { }; } -// N.B., this macro is parsed by util/lintlib.py define_Conf! { - /// Lint: ENUM_VARIANT_NAMES, LARGE_TYPES_PASSED_BY_VALUE, TRIVIALLY_COPY_PASS_BY_REF, UNNECESSARY_WRAPS, UPPER_CASE_ACRONYMS, WRONG_SELF_CONVENTION. + /// Lint: ENUM_VARIANT_NAMES, LARGE_TYPES_PASSED_BY_VALUE, TRIVIALLY_COPY_PASS_BY_REF, UNNECESSARY_WRAPS, UPPER_CASE_ACRONYMS, WRONG_SELF_CONVENTION, BOX_VEC, REDUNDANT_ALLOCATION, RC_BUFFER, VEC_BOX, OPTION_OPTION, LINKEDLIST, RC_MUTEX. /// /// Suppress lints whenever the suggested change would cause breakage for other crates. (avoid_breaking_exported_api: bool = true), - /// Lint: MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE. + /// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE. /// /// The minimum rust version that the project supports (msrv: Option = None), diff --git a/clippy_lints/src/utils/internal_lints/metadata_collector.rs b/clippy_lints/src/utils/internal_lints/metadata_collector.rs index a48a5385083..91533695eb3 100644 --- a/clippy_lints/src/utils/internal_lints/metadata_collector.rs +++ b/clippy_lints/src/utils/internal_lints/metadata_collector.rs @@ -82,7 +82,7 @@ macro_rules! CONFIGURATION_SECTION_TEMPLATE { /// `default` macro_rules! CONFIGURATION_VALUE_TEMPLATE { () => { - "* {name}: `{ty}`: {doc} (defaults to `{default}`)\n" + "* `{name}`: `{ty}`: {doc} (defaults to `{default}`)\n" }; } diff --git a/clippy_utils/src/diagnostics.rs b/clippy_utils/src/diagnostics.rs index 71cfa196fc3..9302e5c21fa 100644 --- a/clippy_utils/src/diagnostics.rs +++ b/clippy_utils/src/diagnostics.rs @@ -21,7 +21,7 @@ fn docs_link(diag: &mut DiagnosticBuilder<'_>, lint: &'static Lint) { "for further information visit https://rust-lang.github.io/rust-clippy/{}/index.html#{}", &option_env!("RUST_RELEASE_NUM").map_or("master".to_string(), |n| { // extract just major + minor version and ignore patch versions - format!("rust-{}", n.rsplitn(2, '.').nth(1).unwrap()) + format!("rust-{}", n.rsplit_once('.').unwrap().1) }), lint )); diff --git a/clippy_utils/src/higher.rs b/clippy_utils/src/higher.rs index c06b894a738..d20739a9f74 100644 --- a/clippy_utils/src/higher.rs +++ b/clippy_utils/src/higher.rs @@ -92,7 +92,14 @@ pub fn hir(cx: &LateContext<'_>, expr: &Expr<'hir>) -> Option { let hir = cx.tcx.hir(); let mut iter = hir.parent_iter(expr.hir_id); if let Some((_, Node::Block(Block { stmts: [], .. }))) = iter.next() { - if let Some((_, Node::Expr(Expr { kind: ExprKind::Loop(_, _, LoopSource::While, _), .. }))) = iter.next() { + if let Some(( + _, + Node::Expr(Expr { + kind: ExprKind::Loop(_, _, LoopSource::While, _), + .. + }), + )) = iter.next() + { // while loop desugar return None; } diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index ddff1686ba2..7e03cf56e8a 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -2,6 +2,7 @@ #![feature(in_band_lifetimes)] #![feature(iter_zip)] #![feature(rustc_private)] +#![feature(control_flow_enum)] #![recursion_limit = "512"] #![cfg_attr(feature = "deny-warnings", deny(warnings))] #![allow(clippy::missing_errors_doc, clippy::missing_panics_doc, clippy::must_use_candidate)] @@ -62,23 +63,27 @@ use std::hash::BuildHasherDefault; use if_chain::if_chain; -use rustc_ast::ast::{self, Attribute, BorrowKind, LitKind}; +use rustc_ast::ast::{self, Attribute, LitKind}; use rustc_data_structures::unhash::UnhashMap; use rustc_hir as hir; use rustc_hir::def::{DefKind, Res}; use rustc_hir::def_id::DefId; +use rustc_hir::hir_id::{HirIdMap, HirIdSet}; use rustc_hir::intravisit::{self, walk_expr, ErasedMap, FnKind, NestedVisitorMap, Visitor}; use rustc_hir::LangItem::{ResultErr, ResultOk}; use rustc_hir::{ def, Arm, BindingAnnotation, Block, Body, Constness, Destination, Expr, ExprKind, FnDecl, GenericArgs, HirId, Impl, - ImplItem, ImplItemKind, IsAsync, Item, ItemKind, LangItem, Local, MatchSource, Node, Param, Pat, PatKind, Path, - PathSegment, PrimTy, QPath, Stmt, StmtKind, TraitItem, TraitItemKind, TraitRef, TyKind, UnOp, + ImplItem, ImplItemKind, IsAsync, Item, ItemKind, LangItem, Local, MatchSource, Mutability, Node, Param, Pat, + PatKind, Path, PathSegment, PrimTy, QPath, Stmt, StmtKind, TraitItem, TraitItemKind, TraitRef, TyKind, UnOp, }; use rustc_lint::{LateContext, Level, Lint, LintContext}; use rustc_middle::hir::exports::Export; use rustc_middle::hir::map::Map; +use rustc_middle::hir::place::PlaceBase; use rustc_middle::ty as rustc_ty; -use rustc_middle::ty::{layout::IntegerExt, DefIdTree, Ty, TyCtxt, TypeFoldable}; +use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow}; +use rustc_middle::ty::binding::BindingMode; +use rustc_middle::ty::{layout::IntegerExt, BorrowKind, DefIdTree, Ty, TyCtxt, TypeAndMut, TypeFoldable, UpvarCapture}; use rustc_semver::RustcVersion; use rustc_session::Session; use rustc_span::hygiene::{ExpnKind, MacroKind}; @@ -89,7 +94,7 @@ use rustc_target::abi::Integer; use crate::consts::{constant, Constant}; -use crate::ty::{can_partially_move_ty, is_recursively_primitive_type}; +use crate::ty::{can_partially_move_ty, is_copy, is_recursively_primitive_type}; pub fn parse_msrv(msrv: &str, sess: Option<&Session>, span: Option) -> Option { if let Ok(version) = RustcVersion::parse(msrv) { @@ -255,7 +260,17 @@ pub fn in_macro(span: Span) -> bool { } pub fn is_unit_expr(expr: &Expr<'_>) -> bool { - matches!(expr.kind, ExprKind::Block(Block { stmts: [], expr: None, .. }, _) | ExprKind::Tup([])) + matches!( + expr.kind, + ExprKind::Block( + Block { + stmts: [], + expr: None, + .. + }, + _ + ) | ExprKind::Tup([]) + ) } /// Checks if given pattern is a wildcard (`_`) @@ -630,11 +645,46 @@ pub fn can_mut_borrow_both(cx: &LateContext<'_>, e1: &Expr<'_>, e2: &Expr<'_>) - } /// Checks if the top level expression can be moved into a closure as is. -pub fn can_move_expr_to_closure_no_visit(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, jump_targets: &[HirId]) -> bool { +/// Currently checks for: +/// * Break/Continue outside the given loop HIR ids. +/// * Yield/Return statments. +/// * Inline assembly. +/// * Usages of a field of a local where the type of the local can be partially moved. +/// +/// For example, given the following function: +/// +/// ``` +/// fn f<'a>(iter: &mut impl Iterator) { +/// for item in iter { +/// let s = item.1; +/// if item.0 > 10 { +/// continue; +/// } else { +/// s.clear(); +/// } +/// } +/// } +/// ``` +/// +/// When called on the expression `item.0` this will return false unless the local `item` is in the +/// `ignore_locals` set. The type `(usize, &mut String)` can have the second element moved, so it +/// isn't always safe to move into a closure when only a single field is needed. +/// +/// When called on the `continue` expression this will return false unless the outer loop expression +/// is in the `loop_ids` set. +/// +/// Note that this check is not recursive, so passing the `if` expression will always return true +/// even though sub-expressions might return false. +pub fn can_move_expr_to_closure_no_visit( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'_>, + loop_ids: &[HirId], + ignore_locals: &HirIdSet, +) -> bool { match expr.kind { ExprKind::Break(Destination { target_id: Ok(id), .. }, _) | ExprKind::Continue(Destination { target_id: Ok(id), .. }) - if jump_targets.contains(&id) => + if loop_ids.contains(&id) => { true }, @@ -646,25 +696,163 @@ pub fn can_move_expr_to_closure_no_visit(cx: &LateContext<'tcx>, expr: &'tcx Exp | ExprKind::LlvmInlineAsm(_) => false, // Accessing a field of a local value can only be done if the type isn't // partially moved. - ExprKind::Field(base_expr, _) - if matches!( - base_expr.kind, - ExprKind::Path(QPath::Resolved(_, Path { res: Res::Local(_), .. })) - ) && can_partially_move_ty(cx, cx.typeck_results().expr_ty(base_expr)) => - { + ExprKind::Field( + &Expr { + hir_id, + kind: + ExprKind::Path(QPath::Resolved( + _, + Path { + res: Res::Local(local_id), + .. + }, + )), + .. + }, + _, + ) if !ignore_locals.contains(local_id) && can_partially_move_ty(cx, cx.typeck_results().node_type(hir_id)) => { // TODO: check if the local has been partially moved. Assume it has for now. false - } + }, _ => true, } } -/// Checks if the expression can be moved into a closure as is. -pub fn can_move_expr_to_closure(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool { +/// How a local is captured by a closure +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum CaptureKind { + Value, + Ref(Mutability), +} +impl CaptureKind { + pub fn is_imm_ref(self) -> bool { + self == Self::Ref(Mutability::Not) + } +} +impl std::ops::BitOr for CaptureKind { + type Output = Self; + fn bitor(self, rhs: Self) -> Self::Output { + match (self, rhs) { + (CaptureKind::Value, _) | (_, CaptureKind::Value) => CaptureKind::Value, + (CaptureKind::Ref(Mutability::Mut), CaptureKind::Ref(_)) + | (CaptureKind::Ref(_), CaptureKind::Ref(Mutability::Mut)) => CaptureKind::Ref(Mutability::Mut), + (CaptureKind::Ref(Mutability::Not), CaptureKind::Ref(Mutability::Not)) => CaptureKind::Ref(Mutability::Not), + } + } +} +impl std::ops::BitOrAssign for CaptureKind { + fn bitor_assign(&mut self, rhs: Self) { + *self = *self | rhs; + } +} + +/// Given an expression referencing a local, determines how it would be captured in a closure. +/// Note as this will walk up to parent expressions until the capture can be determined it should +/// only be used while making a closure somewhere a value is consumed. e.g. a block, match arm, or +/// function argument (other than a receiver). +pub fn capture_local_usage(cx: &LateContext<'tcx>, e: &Expr<'_>) -> CaptureKind { + fn pat_capture_kind(cx: &LateContext<'_>, pat: &Pat<'_>) -> CaptureKind { + let mut capture = CaptureKind::Ref(Mutability::Not); + pat.each_binding_or_first(&mut |_, id, span, _| match cx + .typeck_results() + .extract_binding_mode(cx.sess(), id, span) + .unwrap() + { + BindingMode::BindByValue(_) if !is_copy(cx, cx.typeck_results().node_type(id)) => { + capture = CaptureKind::Value; + }, + BindingMode::BindByReference(Mutability::Mut) if capture != CaptureKind::Value => { + capture = CaptureKind::Ref(Mutability::Mut); + }, + _ => (), + }); + capture + } + + debug_assert!(matches!( + e.kind, + ExprKind::Path(QPath::Resolved(None, Path { res: Res::Local(_), .. })) + )); + + let map = cx.tcx.hir(); + let mut child_id = e.hir_id; + let mut capture = CaptureKind::Value; + let mut capture_expr_ty = e; + + for (parent_id, parent) in map.parent_iter(e.hir_id) { + if let [Adjustment { + kind: Adjust::Deref(_) | Adjust::Borrow(AutoBorrow::Ref(..)), + target, + }, ref adjust @ ..] = *cx + .typeck_results() + .adjustments() + .get(child_id) + .map_or(&[][..], |x| &**x) + { + if let rustc_ty::RawPtr(TypeAndMut { mutbl: mutability, .. }) | rustc_ty::Ref(_, _, mutability) = + *adjust.last().map_or(target, |a| a.target).kind() + { + return CaptureKind::Ref(mutability); + } + } + + match parent { + Node::Expr(e) => match e.kind { + ExprKind::AddrOf(_, mutability, _) => return CaptureKind::Ref(mutability), + ExprKind::Index(..) | ExprKind::Unary(UnOp::Deref, _) => capture = CaptureKind::Ref(Mutability::Not), + ExprKind::Assign(lhs, ..) | ExprKind::Assign(_, lhs, _) if lhs.hir_id == child_id => { + return CaptureKind::Ref(Mutability::Mut); + }, + ExprKind::Field(..) => { + if capture == CaptureKind::Value { + capture_expr_ty = e; + } + }, + ExprKind::Match(_, arms, _) => { + let mut mutability = Mutability::Not; + for capture in arms.iter().map(|arm| pat_capture_kind(cx, arm.pat)) { + match capture { + CaptureKind::Value => break, + CaptureKind::Ref(Mutability::Mut) => mutability = Mutability::Mut, + CaptureKind::Ref(Mutability::Not) => (), + } + } + return CaptureKind::Ref(mutability); + }, + _ => break, + }, + Node::Local(l) => match pat_capture_kind(cx, l.pat) { + CaptureKind::Value => break, + capture @ CaptureKind::Ref(_) => return capture, + }, + _ => break, + } + + child_id = parent_id; + } + + if capture == CaptureKind::Value && is_copy(cx, cx.typeck_results().expr_ty(capture_expr_ty)) { + // Copy types are never automatically captured by value. + CaptureKind::Ref(Mutability::Not) + } else { + capture + } +} + +/// Checks if the expression can be moved into a closure as is. This will return a list of captures +/// if so, otherwise, `None`. +pub fn can_move_expr_to_closure(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option> { struct V<'cx, 'tcx> { cx: &'cx LateContext<'tcx>, + // Stack of potential break targets contained in the expression. loops: Vec, + /// Local variables created in the expression. These don't need to be captured. + locals: HirIdSet, + /// Whether this expression can be turned into a closure. allow_closure: bool, + /// Locals which need to be captured, and whether they need to be by value, reference, or + /// mutable reference. + captures: HirIdMap, } impl Visitor<'tcx> for V<'_, 'tcx> { type Map = ErasedMap<'tcx>; @@ -676,24 +864,67 @@ fn visit_expr(&mut self, e: &'tcx Expr<'_>) { if !self.allow_closure { return; } - if let ExprKind::Loop(b, ..) = e.kind { - self.loops.push(e.hir_id); - self.visit_block(b); - self.loops.pop(); - } else { - self.allow_closure &= can_move_expr_to_closure_no_visit(self.cx, e, &self.loops); - walk_expr(self, e); + + match e.kind { + ExprKind::Path(QPath::Resolved(None, &Path { res: Res::Local(l), .. })) => { + if !self.locals.contains(&l) { + let cap = capture_local_usage(self.cx, e); + self.captures.entry(l).and_modify(|e| *e |= cap).or_insert(cap); + } + }, + ExprKind::Closure(..) => { + let closure_id = self.cx.tcx.hir().local_def_id(e.hir_id).to_def_id(); + for capture in self.cx.typeck_results().closure_min_captures_flattened(closure_id) { + let local_id = match capture.place.base { + PlaceBase::Local(id) => id, + PlaceBase::Upvar(var) => var.var_path.hir_id, + _ => continue, + }; + if !self.locals.contains(&local_id) { + let capture = match capture.info.capture_kind { + UpvarCapture::ByValue(_) => CaptureKind::Value, + UpvarCapture::ByRef(borrow) => match borrow.kind { + BorrowKind::ImmBorrow => CaptureKind::Ref(Mutability::Not), + BorrowKind::UniqueImmBorrow | BorrowKind::MutBorrow => { + CaptureKind::Ref(Mutability::Mut) + }, + }, + }; + self.captures + .entry(local_id) + .and_modify(|e| *e |= capture) + .or_insert(capture); + } + } + }, + ExprKind::Loop(b, ..) => { + self.loops.push(e.hir_id); + self.visit_block(b); + self.loops.pop(); + }, + _ => { + self.allow_closure &= can_move_expr_to_closure_no_visit(self.cx, e, &self.loops, &self.locals); + walk_expr(self, e); + }, } } + + fn visit_pat(&mut self, p: &'tcx Pat<'tcx>) { + p.each_binding_or_first(&mut |_, id, _, _| { + self.locals.insert(id); + }); + } } let mut v = V { cx, allow_closure: true, loops: Vec::new(), + locals: HirIdSet::default(), + captures: HirIdMap::default(), }; v.visit_expr(expr); - v.allow_closure + v.allow_closure.then(|| v.captures) } /// Returns the method names and argument list of nested method call expressions that make up @@ -1708,7 +1939,7 @@ pub fn peel_hir_expr_while<'tcx>( pub fn peel_n_hir_expr_refs(expr: &'a Expr<'a>, count: usize) -> (&'a Expr<'a>, usize) { let mut remaining = count; let e = peel_hir_expr_while(expr, |e| match e.kind { - ExprKind::AddrOf(BorrowKind::Ref, _, e) if remaining != 0 => { + ExprKind::AddrOf(ast::BorrowKind::Ref, _, e) if remaining != 0 => { remaining -= 1; Some(e) }, @@ -1722,7 +1953,7 @@ pub fn peel_n_hir_expr_refs(expr: &'a Expr<'a>, count: usize) -> (&'a Expr<'a>, pub fn peel_hir_expr_refs(expr: &'a Expr<'a>) -> (&'a Expr<'a>, usize) { let mut count = 0; let e = peel_hir_expr_while(expr, |e| match e.kind { - ExprKind::AddrOf(BorrowKind::Ref, _, e) => { + ExprKind::AddrOf(ast::BorrowKind::Ref, _, e) => { count += 1; Some(e) }, diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs index 4a9c4fd0276..14234d9c9cb 100644 --- a/clippy_utils/src/msrvs.rs +++ b/clippy_utils/src/msrvs.rs @@ -13,6 +13,7 @@ macro_rules! msrv_aliases { // names may refer to stabilized feature flags or library items msrv_aliases! { 1,53,0 { OR_PATTERNS } + 1,52,0 { STR_SPLIT_ONCE } 1,50,0 { BOOL_THEN } 1,46,0 { CONST_IF_MATCH } 1,45,0 { STR_STRIP_PREFIX } diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index b0c3fe1e5a7..d7e46c2d3eb 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -68,6 +68,7 @@ pub const IPADDR_V4: [&str; 5] = ["std", "net", "ip", "IpAddr", "V4"]; pub const IPADDR_V6: [&str; 5] = ["std", "net", "ip", "IpAddr", "V6"]; pub const ITER_REPEAT: [&str; 5] = ["core", "iter", "sources", "repeat", "repeat"]; +pub const ITERTOOLS_NEXT_TUPLE: [&str; 3] = ["itertools", "Itertools", "next_tuple"]; #[cfg(feature = "internal-lints")] pub const KW_MODULE: [&str; 3] = ["rustc_span", "symbol", "kw"]; #[cfg(feature = "internal-lints")] diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs index 3cd8ed5aa2c..7f7fbdc9f21 100644 --- a/clippy_utils/src/ty.rs +++ b/clippy_utils/src/ty.rs @@ -10,7 +10,7 @@ use rustc_infer::infer::TyCtxtInferExt; use rustc_lint::LateContext; use rustc_middle::ty::subst::{GenericArg, GenericArgKind}; -use rustc_middle::ty::{self, TyCtxt, AdtDef, IntTy, Ty, TypeFoldable, UintTy}; +use rustc_middle::ty::{self, AdtDef, IntTy, Ty, TyCtxt, TypeFoldable, UintTy}; use rustc_span::sym; use rustc_span::symbol::{Ident, Symbol}; use rustc_span::DUMMY_SP; diff --git a/clippy_utils/src/visitors.rs b/clippy_utils/src/visitors.rs index ce00106dd4d..503effbdad5 100644 --- a/clippy_utils/src/visitors.rs +++ b/clippy_utils/src/visitors.rs @@ -4,6 +4,7 @@ use rustc_hir::{def::Res, Arm, Block, Body, BodyId, Destination, Expr, ExprKind, HirId, Stmt}; use rustc_lint::LateContext; use rustc_middle::hir::map::Map; +use std::ops::ControlFlow; /// returns `true` if expr contains match expr desugared from try fn contains_try(expr: &hir::Expr<'_>) -> bool { @@ -133,62 +134,6 @@ fn visit_expr(&mut self, expr: &'hir hir::Expr<'_>) { } } -pub struct LocalUsedVisitor<'hir> { - hir: Map<'hir>, - pub local_hir_id: HirId, - pub used: bool, -} - -impl<'hir> LocalUsedVisitor<'hir> { - pub fn new(cx: &LateContext<'hir>, local_hir_id: HirId) -> Self { - Self { - hir: cx.tcx.hir(), - local_hir_id, - used: false, - } - } - - fn check(&mut self, t: T, visit: fn(&mut Self, T)) -> bool { - visit(self, t); - std::mem::replace(&mut self.used, false) - } - - pub fn check_arm(&mut self, arm: &'hir Arm<'_>) -> bool { - self.check(arm, Self::visit_arm) - } - - pub fn check_body(&mut self, body: &'hir Body<'_>) -> bool { - self.check(body, Self::visit_body) - } - - pub fn check_expr(&mut self, expr: &'hir Expr<'_>) -> bool { - self.check(expr, Self::visit_expr) - } - - pub fn check_stmt(&mut self, stmt: &'hir Stmt<'_>) -> bool { - self.check(stmt, Self::visit_stmt) - } -} - -impl<'v> Visitor<'v> for LocalUsedVisitor<'v> { - type Map = Map<'v>; - - fn visit_expr(&mut self, expr: &'v Expr<'v>) { - if self.used { - return; - } - if path_to_local_id(expr, self.local_hir_id) { - self.used = true; - } else { - walk_expr(self, expr); - } - } - - fn nested_visit_map(&mut self) -> NestedVisitorMap { - NestedVisitorMap::OnlyBodies(self.hir) - } -} - /// A type which can be visited. pub trait Visitable<'tcx> { /// Calls the corresponding `visit_*` function on the visitor. @@ -203,7 +148,22 @@ fn visit>(self, visitor: &mut V) { } }; } +visitable_ref!(Arm, visit_arm); visitable_ref!(Block, visit_block); +visitable_ref!(Body, visit_body); +visitable_ref!(Expr, visit_expr); +visitable_ref!(Stmt, visit_stmt); + +// impl<'tcx, I: IntoIterator> Visitable<'tcx> for I +// where +// I::Item: Visitable<'tcx>, +// { +// fn visit>(self, visitor: &mut V) { +// for x in self { +// x.visit(visitor); +// } +// } +// } /// Calls the given function for each break expression. pub fn visit_break_exprs<'tcx>( @@ -260,3 +220,48 @@ fn visit_expr(&mut self, e: &'tcx Expr<'_>) { v.visit_expr(&cx.tcx.hir().body(body).value); v.found } + +/// Calls the given function for each usage of the given local. +pub fn for_each_local_usage<'tcx, B>( + cx: &LateContext<'tcx>, + visitable: impl Visitable<'tcx>, + id: HirId, + f: impl FnMut(&'tcx Expr<'tcx>) -> ControlFlow, +) -> ControlFlow { + struct V<'tcx, B, F> { + map: Map<'tcx>, + id: HirId, + f: F, + res: ControlFlow, + } + impl<'tcx, B, F: FnMut(&'tcx Expr<'tcx>) -> ControlFlow> Visitor<'tcx> for V<'tcx, B, F> { + type Map = Map<'tcx>; + fn nested_visit_map(&mut self) -> NestedVisitorMap { + NestedVisitorMap::OnlyBodies(self.map) + } + + fn visit_expr(&mut self, e: &'tcx Expr<'_>) { + if self.res.is_continue() { + if path_to_local_id(e, self.id) { + self.res = (self.f)(e); + } else { + walk_expr(self, e); + } + } + } + } + + let mut v = V { + map: cx.tcx.hir(), + id, + f, + res: ControlFlow::CONTINUE, + }; + visitable.visit(&mut v); + v.res +} + +/// Checks if the given local is used. +pub fn is_local_used(cx: &LateContext<'tcx>, visitable: impl Visitable<'tcx>, id: HirId) -> bool { + for_each_local_usage(cx, visitable, id, |_| ControlFlow::BREAK).is_break() +} diff --git a/doc/adding_lints.md b/doc/adding_lints.md index f2260c3d1a2..392b70ee3a9 100644 --- a/doc/adding_lints.md +++ b/doc/adding_lints.md @@ -559,11 +559,12 @@ in the following steps: 1. Adding a new configuration entry to [clippy_utils::conf](/clippy_utils/src/conf.rs) like this: ```rust - /// Lint: LINT_NAME. + /// Lint: LINT_NAME. + /// + /// (configuration_ident: Type = DefaultValue), ``` - The configuration value and identifier should usually be the same. The doc comment will be - automatically added to the lint documentation. + The doc comment will be automatically added to the lint documentation. 2. Adding the configuration value to the lint impl struct: 1. This first requires the definition of a lint impl struct. Lint impl structs are usually generated with the `declare_lint_pass!` macro. This struct needs to be defined manually diff --git a/doc/common_tools_writing_lints.md b/doc/common_tools_writing_lints.md index 0a85f650011..1a6b7c8cb47 100644 --- a/doc/common_tools_writing_lints.md +++ b/doc/common_tools_writing_lints.md @@ -11,6 +11,7 @@ You may need following tooltips to catch up with common operations. Useful Rustc dev guide links: - [Stages of compilation](https://rustc-dev-guide.rust-lang.org/compiler-src.html#the-main-stages-of-compilation) +- [Diagnostic items](https://rustc-dev-guide.rust-lang.org/diagnostics/diagnostic-items.html) - [Type checking](https://rustc-dev-guide.rust-lang.org/type-checking.html) - [Ty module](https://rustc-dev-guide.rust-lang.org/ty.html) @@ -75,20 +76,21 @@ impl LateLintPass<'_> for MyStructLint { # Checking if a type implements a specific trait -There are two ways to do this, depending if the target trait is part of lang items. +There are three ways to do this, depending on if the target trait has a diagnostic item, lang item or neither. ```rust -use clippy_utils::{implements_trait, match_trait_method, paths}; +use clippy_utils::{implements_trait, is_trait_method, match_trait_method, paths}; +use rustc_span::symbol::sym; impl LateLintPass<'_> for MyStructLint { fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { - // 1. Using expression and Clippy's convenient method - // we use `match_trait_method` function from Clippy's toolbox - if match_trait_method(cx, expr, &paths::INTO) { - // `expr` implements `Into` trait + // 1. Using diagnostic items with the expression + // we use `is_trait_method` function from Clippy's utils + if is_trait_method(cx, expr, sym::Iterator) { + // method call in `expr` belongs to `Iterator` trait } - // 2. Using type context `TyCtxt` + // 2. Using lang items with the expression type let ty = cx.typeck_results().expr_ty(expr); if cx.tcx.lang_items() // we are looking for the `DefId` of `Drop` trait in lang items @@ -97,15 +99,20 @@ impl LateLintPass<'_> for MyStructLint { .map_or(false, |id| implements_trait(cx, ty, id, &[])) { // `expr` implements `Drop` trait } + + // 3. Using the type path with the expression + // we use `match_trait_method` function from Clippy's utils + if match_trait_method(cx, expr, &paths::INTO) { + // `expr` implements `Into` trait + } } } ``` -> Prefer using lang items, if the target trait is available there. - -A list of defined paths for Clippy can be found in [paths.rs][paths] +> Prefer using diagnostic and lang items, if the target trait has one. We access lang items through the type context `tcx`. `tcx` is of type [`TyCtxt`][TyCtxt] and is defined in the `rustc_middle` crate. +A list of defined paths for Clippy can be found in [paths.rs][paths] # Checking if a type defines a specific method diff --git a/lintcheck/Cargo.toml b/lintcheck/Cargo.toml index 8c33fa372ec..ada033de6e3 100644 --- a/lintcheck/Cargo.toml +++ b/lintcheck/Cargo.toml @@ -19,6 +19,7 @@ serde_json = {version = "1.0"} tar = {version = "0.4.30"} toml = {version = "0.5"} ureq = {version = "2.0.0-rc3"} +walkdir = {version = "2.3.2"} [features] deny-warnings = [] diff --git a/lintcheck/src/main.rs b/lintcheck/src/main.rs index 7d2f3173fb0..f1e03ba4296 100644 --- a/lintcheck/src/main.rs +++ b/lintcheck/src/main.rs @@ -21,6 +21,7 @@ use rayon::prelude::*; use serde::{Deserialize, Serialize}; use serde_json::Value; +use walkdir::{DirEntry, WalkDir}; #[cfg(not(windows))] const CLIPPY_DRIVER_PATH: &str = "target/debug/clippy-driver"; @@ -193,32 +194,41 @@ fn download_and_extract(&self) -> Crate { } }, CrateSource::Path { name, path, options } => { - use fs_extra::dir; + // copy path into the dest_crate_root but skip directories that contain a CACHEDIR.TAG file. + // The target/ directory contains a CACHEDIR.TAG file so it is the most commonly skipped directory + // as a result of this filter. + let dest_crate_root = PathBuf::from(LINTCHECK_SOURCES).join(name); + if dest_crate_root.exists() { + println!("Deleting existing directory at {:?}", dest_crate_root); + std::fs::remove_dir_all(&dest_crate_root).unwrap(); + } - // simply copy the entire directory into our target dir - let copy_dest = PathBuf::from(format!("{}/", LINTCHECK_SOURCES)); + println!("Copying {:?} to {:?}", path, dest_crate_root); - // the source path of the crate we copied, ${copy_dest}/crate_name - let crate_root = copy_dest.join(name); // .../crates/local_crate + fn is_cache_dir(entry: &DirEntry) -> bool { + std::fs::read(entry.path().join("CACHEDIR.TAG")) + .map(|x| x.starts_with(b"Signature: 8a477f597d28d172789f06886806bc55")) + .unwrap_or(false) + } - if crate_root.exists() { - println!( - "Not copying {} to {}, destination already exists", - path.display(), - crate_root.display() - ); - } else { - println!("Copying {} to {}", path.display(), copy_dest.display()); + for entry in WalkDir::new(path).into_iter().filter_entry(|e| !is_cache_dir(e)) { + let entry = entry.unwrap(); + let entry_path = entry.path(); + let relative_entry_path = entry_path.strip_prefix(path).unwrap(); + let dest_path = dest_crate_root.join(relative_entry_path); + let metadata = entry_path.symlink_metadata().unwrap(); - dir::copy(path, ©_dest, &dir::CopyOptions::new()).unwrap_or_else(|_| { - panic!("Failed to copy from {}, to {}", path.display(), crate_root.display()) - }); + if metadata.is_dir() { + std::fs::create_dir(dest_path).unwrap(); + } else if metadata.is_file() { + std::fs::copy(entry_path, dest_path).unwrap(); + } } Crate { version: String::from("local"), name: name.clone(), - path: crate_root, + path: dest_crate_root, options: options.clone(), } }, diff --git a/tests/compile-test.rs b/tests/compile-test.rs index 0a82377a10e..6116acffe07 100644 --- a/tests/compile-test.rs +++ b/tests/compile-test.rs @@ -39,6 +39,7 @@ fn third_party_crates() -> String { "clippy_lints", "clippy_utils", "if_chain", + "itertools", "quote", "regex", "serde", diff --git a/tests/ui-cargo/feature_name/fail/Cargo.toml b/tests/ui-cargo/feature_name/fail/Cargo.toml new file mode 100644 index 00000000000..97d51462a94 --- /dev/null +++ b/tests/ui-cargo/feature_name/fail/Cargo.toml @@ -0,0 +1,21 @@ + +# Content that triggers the lint goes here + +[package] +name = "feature_name" +version = "0.1.0" +publish = false + +[workspace] + +[features] +use-qwq = [] +use_qwq = [] +with-owo = [] +with_owo = [] +qvq-support = [] +qvq_support = [] +no-qaq = [] +no_qaq = [] +not-orz = [] +not_orz = [] diff --git a/tests/ui-cargo/feature_name/fail/src/main.rs b/tests/ui-cargo/feature_name/fail/src/main.rs new file mode 100644 index 00000000000..64f01a98c90 --- /dev/null +++ b/tests/ui-cargo/feature_name/fail/src/main.rs @@ -0,0 +1,7 @@ +// compile-flags: --crate-name=feature_name +#![warn(clippy::redundant_feature_names)] +#![warn(clippy::negative_feature_names)] + +fn main() { + // test code goes here +} diff --git a/tests/ui-cargo/feature_name/fail/src/main.stderr b/tests/ui-cargo/feature_name/fail/src/main.stderr new file mode 100644 index 00000000000..b9e6cb49bc9 --- /dev/null +++ b/tests/ui-cargo/feature_name/fail/src/main.stderr @@ -0,0 +1,44 @@ +error: the "no-" prefix in the feature name "no-qaq" is negative + | + = note: `-D clippy::negative-feature-names` implied by `-D warnings` + = help: consider renaming the feature to "qaq", but make sure the feature adds functionality + +error: the "no_" prefix in the feature name "no_qaq" is negative + | + = help: consider renaming the feature to "qaq", but make sure the feature adds functionality + +error: the "not-" prefix in the feature name "not-orz" is negative + | + = help: consider renaming the feature to "orz", but make sure the feature adds functionality + +error: the "not_" prefix in the feature name "not_orz" is negative + | + = help: consider renaming the feature to "orz", but make sure the feature adds functionality + +error: the "-support" suffix in the feature name "qvq-support" is redundant + | + = note: `-D clippy::redundant-feature-names` implied by `-D warnings` + = help: consider renaming the feature to "qvq" + +error: the "_support" suffix in the feature name "qvq_support" is redundant + | + = help: consider renaming the feature to "qvq" + +error: the "use-" prefix in the feature name "use-qwq" is redundant + | + = help: consider renaming the feature to "qwq" + +error: the "use_" prefix in the feature name "use_qwq" is redundant + | + = help: consider renaming the feature to "qwq" + +error: the "with-" prefix in the feature name "with-owo" is redundant + | + = help: consider renaming the feature to "owo" + +error: the "with_" prefix in the feature name "with_owo" is redundant + | + = help: consider renaming the feature to "owo" + +error: aborting due to 10 previous errors + diff --git a/tests/ui-cargo/feature_name/pass/Cargo.toml b/tests/ui-cargo/feature_name/pass/Cargo.toml new file mode 100644 index 00000000000..cf947312bd4 --- /dev/null +++ b/tests/ui-cargo/feature_name/pass/Cargo.toml @@ -0,0 +1,9 @@ + +# This file should not trigger the lint + +[package] +name = "feature_name" +version = "0.1.0" +publish = false + +[workspace] diff --git a/tests/ui-cargo/feature_name/pass/src/main.rs b/tests/ui-cargo/feature_name/pass/src/main.rs new file mode 100644 index 00000000000..64f01a98c90 --- /dev/null +++ b/tests/ui-cargo/feature_name/pass/src/main.rs @@ -0,0 +1,7 @@ +// compile-flags: --crate-name=feature_name +#![warn(clippy::redundant_feature_names)] +#![warn(clippy::negative_feature_names)] + +fn main() { + // test code goes here +} diff --git a/tests/ui-cargo/module_style/fail_mod/Cargo.toml b/tests/ui-cargo/module_style/fail_mod/Cargo.toml new file mode 100644 index 00000000000..27b61c09fb4 --- /dev/null +++ b/tests/ui-cargo/module_style/fail_mod/Cargo.toml @@ -0,0 +1,8 @@ +[package] +name = "fail" +version = "0.1.0" +edition = "2018" + +# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html + +[dependencies] diff --git a/tests/ui-cargo/module_style/fail_mod/src/bad/inner.rs b/tests/ui-cargo/module_style/fail_mod/src/bad/inner.rs new file mode 100644 index 00000000000..91cd540a28f --- /dev/null +++ b/tests/ui-cargo/module_style/fail_mod/src/bad/inner.rs @@ -0,0 +1 @@ +pub mod stuff; diff --git a/tests/ui-cargo/module_style/fail_mod/src/bad/inner/stuff.rs b/tests/ui-cargo/module_style/fail_mod/src/bad/inner/stuff.rs new file mode 100644 index 00000000000..7713fa9d35c --- /dev/null +++ b/tests/ui-cargo/module_style/fail_mod/src/bad/inner/stuff.rs @@ -0,0 +1,3 @@ +pub mod most; + +pub struct Inner; diff --git a/tests/ui-cargo/module_style/fail_mod/src/bad/inner/stuff/most.rs b/tests/ui-cargo/module_style/fail_mod/src/bad/inner/stuff/most.rs new file mode 100644 index 00000000000..5a5eaf9670f --- /dev/null +++ b/tests/ui-cargo/module_style/fail_mod/src/bad/inner/stuff/most.rs @@ -0,0 +1 @@ +pub struct Snarks; diff --git a/tests/ui-cargo/module_style/fail_mod/src/bad/mod.rs b/tests/ui-cargo/module_style/fail_mod/src/bad/mod.rs new file mode 100644 index 00000000000..a12734db7cb --- /dev/null +++ b/tests/ui-cargo/module_style/fail_mod/src/bad/mod.rs @@ -0,0 +1,3 @@ +pub mod inner; + +pub struct Thing; diff --git a/tests/ui-cargo/module_style/fail_mod/src/main.rs b/tests/ui-cargo/module_style/fail_mod/src/main.rs new file mode 100644 index 00000000000..3e985d4e904 --- /dev/null +++ b/tests/ui-cargo/module_style/fail_mod/src/main.rs @@ -0,0 +1,9 @@ +#![warn(clippy::self_named_module_files)] + +mod bad; + +fn main() { + let _ = bad::Thing; + let _ = bad::inner::stuff::Inner; + let _ = bad::inner::stuff::most::Snarks; +} diff --git a/tests/ui-cargo/module_style/fail_mod/src/main.stderr b/tests/ui-cargo/module_style/fail_mod/src/main.stderr new file mode 100644 index 00000000000..af4c298b310 --- /dev/null +++ b/tests/ui-cargo/module_style/fail_mod/src/main.stderr @@ -0,0 +1,19 @@ +error: `mod.rs` files are required, found `/bad/inner.rs` + --> $DIR/bad/inner.rs:1:1 + | +LL | pub mod stuff; + | ^ + | + = note: `-D clippy::self-named-module-files` implied by `-D warnings` + = help: move `/bad/inner.rs` to `/bad/inner/mod.rs` + +error: `mod.rs` files are required, found `/bad/inner/stuff.rs` + --> $DIR/bad/inner/stuff.rs:1:1 + | +LL | pub mod most; + | ^ + | + = help: move `/bad/inner/stuff.rs` to `/bad/inner/stuff/mod.rs` + +error: aborting due to 2 previous errors + diff --git a/tests/ui-cargo/module_style/fail_no_mod/Cargo.toml b/tests/ui-cargo/module_style/fail_no_mod/Cargo.toml new file mode 100644 index 00000000000..27b61c09fb4 --- /dev/null +++ b/tests/ui-cargo/module_style/fail_no_mod/Cargo.toml @@ -0,0 +1,8 @@ +[package] +name = "fail" +version = "0.1.0" +edition = "2018" + +# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html + +[dependencies] diff --git a/tests/ui-cargo/module_style/fail_no_mod/src/bad/mod.rs b/tests/ui-cargo/module_style/fail_no_mod/src/bad/mod.rs new file mode 100644 index 00000000000..f19ab10d5fb --- /dev/null +++ b/tests/ui-cargo/module_style/fail_no_mod/src/bad/mod.rs @@ -0,0 +1 @@ +pub struct Thing; diff --git a/tests/ui-cargo/module_style/fail_no_mod/src/main.rs b/tests/ui-cargo/module_style/fail_no_mod/src/main.rs new file mode 100644 index 00000000000..c6e9045b8dc --- /dev/null +++ b/tests/ui-cargo/module_style/fail_no_mod/src/main.rs @@ -0,0 +1,7 @@ +#![warn(clippy::mod_module_files)] + +mod bad; + +fn main() { + let _ = bad::Thing; +} diff --git a/tests/ui-cargo/module_style/fail_no_mod/src/main.stderr b/tests/ui-cargo/module_style/fail_no_mod/src/main.stderr new file mode 100644 index 00000000000..11e15db7fb9 --- /dev/null +++ b/tests/ui-cargo/module_style/fail_no_mod/src/main.stderr @@ -0,0 +1,11 @@ +error: `mod.rs` files are not allowed, found `/bad/mod.rs` + --> $DIR/bad/mod.rs:1:1 + | +LL | pub struct Thing; + | ^ + | + = note: `-D clippy::mod-module-files` implied by `-D warnings` + = help: move `/bad/mod.rs` to `/bad.rs` + +error: aborting due to previous error + diff --git a/tests/ui-cargo/module_style/pass_mod/Cargo.toml b/tests/ui-cargo/module_style/pass_mod/Cargo.toml new file mode 100644 index 00000000000..27b61c09fb4 --- /dev/null +++ b/tests/ui-cargo/module_style/pass_mod/Cargo.toml @@ -0,0 +1,8 @@ +[package] +name = "fail" +version = "0.1.0" +edition = "2018" + +# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html + +[dependencies] diff --git a/tests/ui-cargo/module_style/pass_mod/src/bad/mod.rs b/tests/ui-cargo/module_style/pass_mod/src/bad/mod.rs new file mode 100644 index 00000000000..f19ab10d5fb --- /dev/null +++ b/tests/ui-cargo/module_style/pass_mod/src/bad/mod.rs @@ -0,0 +1 @@ +pub struct Thing; diff --git a/tests/ui-cargo/module_style/pass_mod/src/main.rs b/tests/ui-cargo/module_style/pass_mod/src/main.rs new file mode 100644 index 00000000000..9e08715fc05 --- /dev/null +++ b/tests/ui-cargo/module_style/pass_mod/src/main.rs @@ -0,0 +1,10 @@ +#![warn(clippy::self_named_module_files)] + +mod bad; +mod more; + +fn main() { + let _ = bad::Thing; + let _ = more::foo::Foo; + let _ = more::inner::Inner; +} diff --git a/tests/ui-cargo/module_style/pass_mod/src/more/foo.rs b/tests/ui-cargo/module_style/pass_mod/src/more/foo.rs new file mode 100644 index 00000000000..4a835673a59 --- /dev/null +++ b/tests/ui-cargo/module_style/pass_mod/src/more/foo.rs @@ -0,0 +1 @@ +pub struct Foo; diff --git a/tests/ui-cargo/module_style/pass_mod/src/more/inner/mod.rs b/tests/ui-cargo/module_style/pass_mod/src/more/inner/mod.rs new file mode 100644 index 00000000000..aa84f78cc2c --- /dev/null +++ b/tests/ui-cargo/module_style/pass_mod/src/more/inner/mod.rs @@ -0,0 +1 @@ +pub struct Inner; diff --git a/tests/ui-cargo/module_style/pass_mod/src/more/mod.rs b/tests/ui-cargo/module_style/pass_mod/src/more/mod.rs new file mode 100644 index 00000000000..d79569f78ff --- /dev/null +++ b/tests/ui-cargo/module_style/pass_mod/src/more/mod.rs @@ -0,0 +1,2 @@ +pub mod foo; +pub mod inner; diff --git a/tests/ui-cargo/module_style/pass_no_mod/Cargo.toml b/tests/ui-cargo/module_style/pass_no_mod/Cargo.toml new file mode 100644 index 00000000000..3c0896dd2cd --- /dev/null +++ b/tests/ui-cargo/module_style/pass_no_mod/Cargo.toml @@ -0,0 +1,8 @@ +[package] +name = "pass" +version = "0.1.0" +edition = "2018" + +# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html + +[dependencies] diff --git a/tests/ui-cargo/module_style/pass_no_mod/src/good.rs b/tests/ui-cargo/module_style/pass_no_mod/src/good.rs new file mode 100644 index 00000000000..f19ab10d5fb --- /dev/null +++ b/tests/ui-cargo/module_style/pass_no_mod/src/good.rs @@ -0,0 +1 @@ +pub struct Thing; diff --git a/tests/ui-cargo/module_style/pass_no_mod/src/main.rs b/tests/ui-cargo/module_style/pass_no_mod/src/main.rs new file mode 100644 index 00000000000..50211a340b9 --- /dev/null +++ b/tests/ui-cargo/module_style/pass_no_mod/src/main.rs @@ -0,0 +1,7 @@ +#![warn(clippy::mod_module_files)] + +mod good; + +fn main() { + let _ = good::Thing; +} diff --git a/tests/ui/auxiliary/option_helpers.rs b/tests/ui/auxiliary/option_helpers.rs index 7dc3f4ebd4d..86a637ce309 100644 --- a/tests/ui/auxiliary/option_helpers.rs +++ b/tests/ui/auxiliary/option_helpers.rs @@ -53,3 +53,12 @@ pub fn count(self) -> usize { self.foo as usize } } + +#[derive(Copy, Clone)] +pub struct IteratorMethodFalsePositives; + +impl IteratorMethodFalsePositives { + pub fn filter(&self, _s: i32) -> std::vec::IntoIter { + unimplemented!(); + } +} diff --git a/tests/ui/bool_assert_comparison.rs b/tests/ui/bool_assert_comparison.rs index 2de402fae8c..ec4d6f3ff84 100644 --- a/tests/ui/bool_assert_comparison.rs +++ b/tests/ui/bool_assert_comparison.rs @@ -1,5 +1,7 @@ #![warn(clippy::bool_assert_comparison)] +use std::ops::Not; + macro_rules! a { () => { true @@ -11,7 +13,58 @@ macro_rules! b { }; } +// Implements the Not trait but with an output type +// that's not bool. Should not suggest a rewrite +#[derive(Debug)] +enum ImplNotTraitWithoutBool { + VariantX(bool), + VariantY(u32), +} + +impl PartialEq for ImplNotTraitWithoutBool { + fn eq(&self, other: &bool) -> bool { + match *self { + ImplNotTraitWithoutBool::VariantX(b) => b == *other, + _ => false, + } + } +} + +impl Not for ImplNotTraitWithoutBool { + type Output = Self; + + fn not(self) -> Self::Output { + match self { + ImplNotTraitWithoutBool::VariantX(b) => ImplNotTraitWithoutBool::VariantX(!b), + ImplNotTraitWithoutBool::VariantY(0) => ImplNotTraitWithoutBool::VariantY(1), + ImplNotTraitWithoutBool::VariantY(_) => ImplNotTraitWithoutBool::VariantY(0), + } + } +} + +// This type implements the Not trait with an Output of +// type bool. Using assert!(..) must be suggested +#[derive(Debug)] +struct ImplNotTraitWithBool; + +impl PartialEq for ImplNotTraitWithBool { + fn eq(&self, other: &bool) -> bool { + false + } +} + +impl Not for ImplNotTraitWithBool { + type Output = bool; + + fn not(self) -> Self::Output { + true + } +} + fn main() { + let a = ImplNotTraitWithoutBool::VariantX(true); + let b = ImplNotTraitWithBool; + assert_eq!("a".len(), 1); assert_eq!("a".is_empty(), false); assert_eq!("".is_empty(), true); @@ -19,6 +72,8 @@ fn main() { assert_eq!(a!(), b!()); assert_eq!(a!(), "".is_empty()); assert_eq!("".is_empty(), b!()); + assert_eq!(a, true); + assert_eq!(b, true); assert_ne!("a".len(), 1); assert_ne!("a".is_empty(), false); @@ -27,6 +82,8 @@ fn main() { assert_ne!(a!(), b!()); assert_ne!(a!(), "".is_empty()); assert_ne!("".is_empty(), b!()); + assert_ne!(a, true); + assert_ne!(b, true); debug_assert_eq!("a".len(), 1); debug_assert_eq!("a".is_empty(), false); @@ -35,6 +92,8 @@ fn main() { debug_assert_eq!(a!(), b!()); debug_assert_eq!(a!(), "".is_empty()); debug_assert_eq!("".is_empty(), b!()); + debug_assert_eq!(a, true); + debug_assert_eq!(b, true); debug_assert_ne!("a".len(), 1); debug_assert_ne!("a".is_empty(), false); @@ -43,6 +102,8 @@ fn main() { debug_assert_ne!(a!(), b!()); debug_assert_ne!(a!(), "".is_empty()); debug_assert_ne!("".is_empty(), b!()); + debug_assert_ne!(a, true); + debug_assert_ne!(b, true); // assert with error messages assert_eq!("a".len(), 1, "tadam {}", 1); @@ -50,10 +111,12 @@ fn main() { assert_eq!("a".is_empty(), false, "tadam {}", 1); assert_eq!("a".is_empty(), false, "tadam {}", true); assert_eq!(false, "a".is_empty(), "tadam {}", true); + assert_eq!(a, true, "tadam {}", false); debug_assert_eq!("a".len(), 1, "tadam {}", 1); debug_assert_eq!("a".len(), 1, "tadam {}", true); debug_assert_eq!("a".is_empty(), false, "tadam {}", 1); debug_assert_eq!("a".is_empty(), false, "tadam {}", true); debug_assert_eq!(false, "a".is_empty(), "tadam {}", true); + debug_assert_eq!(a, true, "tadam {}", false); } diff --git a/tests/ui/bool_assert_comparison.stderr b/tests/ui/bool_assert_comparison.stderr index f57acf520d5..da9b56aa779 100644 --- a/tests/ui/bool_assert_comparison.stderr +++ b/tests/ui/bool_assert_comparison.stderr @@ -1,5 +1,5 @@ error: used `assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:16:5 + --> $DIR/bool_assert_comparison.rs:69:5 | LL | assert_eq!("a".is_empty(), false); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` @@ -7,106 +7,130 @@ LL | assert_eq!("a".is_empty(), false); = note: `-D clippy::bool-assert-comparison` implied by `-D warnings` error: used `assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:17:5 + --> $DIR/bool_assert_comparison.rs:70:5 | LL | assert_eq!("".is_empty(), true); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` error: used `assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:18:5 + --> $DIR/bool_assert_comparison.rs:71:5 | LL | assert_eq!(true, "".is_empty()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` +error: used `assert_eq!` with a literal bool + --> $DIR/bool_assert_comparison.rs:76:5 + | +LL | assert_eq!(b, true); + | ^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` + error: used `assert_ne!` with a literal bool - --> $DIR/bool_assert_comparison.rs:24:5 + --> $DIR/bool_assert_comparison.rs:79:5 | LL | assert_ne!("a".is_empty(), false); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` error: used `assert_ne!` with a literal bool - --> $DIR/bool_assert_comparison.rs:25:5 + --> $DIR/bool_assert_comparison.rs:80:5 | LL | assert_ne!("".is_empty(), true); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` error: used `assert_ne!` with a literal bool - --> $DIR/bool_assert_comparison.rs:26:5 + --> $DIR/bool_assert_comparison.rs:81:5 | LL | assert_ne!(true, "".is_empty()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` +error: used `assert_ne!` with a literal bool + --> $DIR/bool_assert_comparison.rs:86:5 + | +LL | assert_ne!(b, true); + | ^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` + error: used `debug_assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:32:5 + --> $DIR/bool_assert_comparison.rs:89:5 | LL | debug_assert_eq!("a".is_empty(), false); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` error: used `debug_assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:33:5 + --> $DIR/bool_assert_comparison.rs:90:5 | LL | debug_assert_eq!("".is_empty(), true); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` error: used `debug_assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:34:5 + --> $DIR/bool_assert_comparison.rs:91:5 | LL | debug_assert_eq!(true, "".is_empty()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` +error: used `debug_assert_eq!` with a literal bool + --> $DIR/bool_assert_comparison.rs:96:5 + | +LL | debug_assert_eq!(b, true); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` + error: used `debug_assert_ne!` with a literal bool - --> $DIR/bool_assert_comparison.rs:40:5 + --> $DIR/bool_assert_comparison.rs:99:5 | LL | debug_assert_ne!("a".is_empty(), false); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` error: used `debug_assert_ne!` with a literal bool - --> $DIR/bool_assert_comparison.rs:41:5 + --> $DIR/bool_assert_comparison.rs:100:5 | LL | debug_assert_ne!("".is_empty(), true); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` error: used `debug_assert_ne!` with a literal bool - --> $DIR/bool_assert_comparison.rs:42:5 + --> $DIR/bool_assert_comparison.rs:101:5 | LL | debug_assert_ne!(true, "".is_empty()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` +error: used `debug_assert_ne!` with a literal bool + --> $DIR/bool_assert_comparison.rs:106:5 + | +LL | debug_assert_ne!(b, true); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` + error: used `assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:50:5 + --> $DIR/bool_assert_comparison.rs:111:5 | LL | assert_eq!("a".is_empty(), false, "tadam {}", 1); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` error: used `assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:51:5 + --> $DIR/bool_assert_comparison.rs:112:5 | LL | assert_eq!("a".is_empty(), false, "tadam {}", true); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` error: used `assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:52:5 + --> $DIR/bool_assert_comparison.rs:113:5 | LL | assert_eq!(false, "a".is_empty(), "tadam {}", true); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` error: used `debug_assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:56:5 + --> $DIR/bool_assert_comparison.rs:118:5 | LL | debug_assert_eq!("a".is_empty(), false, "tadam {}", 1); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` error: used `debug_assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:57:5 + --> $DIR/bool_assert_comparison.rs:119:5 | LL | debug_assert_eq!("a".is_empty(), false, "tadam {}", true); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` error: used `debug_assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:58:5 + --> $DIR/bool_assert_comparison.rs:120:5 | LL | debug_assert_eq!(false, "a".is_empty(), "tadam {}", true); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` -error: aborting due to 18 previous errors +error: aborting due to 22 previous errors diff --git a/tests/ui/box_vec.rs b/tests/ui/box_vec.rs index 87b67c23704..1d6366972da 100644 --- a/tests/ui/box_vec.rs +++ b/tests/ui/box_vec.rs @@ -1,6 +1,10 @@ #![warn(clippy::all)] -#![allow(clippy::boxed_local, clippy::needless_pass_by_value)] -#![allow(clippy::blacklisted_name)] +#![allow( + clippy::boxed_local, + clippy::needless_pass_by_value, + clippy::blacklisted_name, + unused +)] macro_rules! boxit { ($init:expr, $x:ty) => { @@ -11,22 +15,22 @@ macro_rules! boxit { fn test_macro() { boxit!(Vec::new(), Vec); } -pub fn test(foo: Box>) { - println!("{:?}", foo.get(0)) -} +fn test(foo: Box>) {} -pub fn test2(foo: Box)>) { +fn test2(foo: Box)>) { // pass if #31 is fixed foo(vec![1, 2, 3]) } -pub fn test_local_not_linted() { +fn test_local_not_linted() { let _: Box>; } -fn main() { - test(Box::new(Vec::new())); - test2(Box::new(|v| println!("{:?}", v))); - test_macro(); - test_local_not_linted(); +// All of these test should be allowed because they are part of the +// public api and `avoid_breaking_exported_api` is `false` by default. +pub fn pub_test(foo: Box>) {} +pub fn pub_test_ret() -> Box> { + Box::new(Vec::new()) } + +fn main() {} diff --git a/tests/ui/box_vec.stderr b/tests/ui/box_vec.stderr index 9b789334bae..58c1f13fb87 100644 --- a/tests/ui/box_vec.stderr +++ b/tests/ui/box_vec.stderr @@ -1,8 +1,8 @@ error: you seem to be trying to use `Box>`. Consider using just `Vec` - --> $DIR/box_vec.rs:14:18 + --> $DIR/box_vec.rs:18:14 | -LL | pub fn test(foo: Box>) { - | ^^^^^^^^^^^^^^ +LL | fn test(foo: Box>) {} + | ^^^^^^^^^^^^^^ | = note: `-D clippy::box-vec` implied by `-D warnings` = help: `Vec` is already on the heap, `Box>` makes an extra allocation diff --git a/tests/ui/entry.fixed b/tests/ui/entry.fixed index cfad3090ba3..8a36ec833d7 100644 --- a/tests/ui/entry.fixed +++ b/tests/ui/entry.fixed @@ -4,7 +4,7 @@ #![warn(clippy::map_entry)] #![feature(asm)] -use std::collections::{BTreeMap, HashMap}; +use std::collections::HashMap; use std::hash::Hash; macro_rules! m { @@ -142,14 +142,13 @@ fn hash_map(m: &mut HashMap, m2: &mut HashMa if !m.contains_key(&k) { insert!(m, k, v); } -} -fn btree_map(m: &mut BTreeMap, k: K, v: V, v2: V) { - // insert then do something, use if let - if let std::collections::btree_map::Entry::Vacant(e) = m.entry(k) { - e.insert(v); - foo(); - } + // or_insert_with. Partial move of a local declared in the closure is ok. + m.entry(k).or_insert_with(|| { + let x = (String::new(), String::new()); + let _ = x.0; + v + }); } fn main() {} diff --git a/tests/ui/entry.rs b/tests/ui/entry.rs index fa9280b58de..d972a201ad7 100644 --- a/tests/ui/entry.rs +++ b/tests/ui/entry.rs @@ -4,7 +4,7 @@ #![warn(clippy::map_entry)] #![feature(asm)] -use std::collections::{BTreeMap, HashMap}; +use std::collections::HashMap; use std::hash::Hash; macro_rules! m { @@ -146,13 +146,12 @@ fn hash_map(m: &mut HashMap, m2: &mut HashMa if !m.contains_key(&k) { insert!(m, k, v); } -} -fn btree_map(m: &mut BTreeMap, k: K, v: V, v2: V) { - // insert then do something, use if let + // or_insert_with. Partial move of a local declared in the closure is ok. if !m.contains_key(&k) { + let x = (String::new(), String::new()); + let _ = x.0; m.insert(k, v); - foo(); } } diff --git a/tests/ui/entry.stderr b/tests/ui/entry.stderr index 8f2e383d675..1076500498d 100644 --- a/tests/ui/entry.stderr +++ b/tests/ui/entry.stderr @@ -165,21 +165,23 @@ LL | | m.insert(m!(k), m!(v)); LL | | } | |_____^ help: try this: `m.entry(m!(k)).or_insert_with(|| m!(v));` -error: usage of `contains_key` followed by `insert` on a `BTreeMap` - --> $DIR/entry.rs:153:5 +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> $DIR/entry.rs:151:5 | LL | / if !m.contains_key(&k) { +LL | | let x = (String::new(), String::new()); +LL | | let _ = x.0; LL | | m.insert(k, v); -LL | | foo(); LL | | } | |_____^ | help: try this | -LL ~ if let std::collections::btree_map::Entry::Vacant(e) = m.entry(k) { -LL + e.insert(v); -LL + foo(); -LL + } +LL ~ m.entry(k).or_insert_with(|| { +LL + let x = (String::new(), String::new()); +LL + let _ = x.0; +LL + v +LL + }); | error: aborting due to 10 previous errors diff --git a/tests/ui/entry_btree.fixed b/tests/ui/entry_btree.fixed new file mode 100644 index 00000000000..94979104556 --- /dev/null +++ b/tests/ui/entry_btree.fixed @@ -0,0 +1,18 @@ +// run-rustfix + +#![warn(clippy::map_entry)] +#![allow(dead_code)] + +use std::collections::BTreeMap; + +fn foo() {} + +fn btree_map(m: &mut BTreeMap, k: K, v: V) { + // insert then do something, use if let + if let std::collections::btree_map::Entry::Vacant(e) = m.entry(k) { + e.insert(v); + foo(); + } +} + +fn main() {} diff --git a/tests/ui/entry_btree.rs b/tests/ui/entry_btree.rs new file mode 100644 index 00000000000..080c1d959e8 --- /dev/null +++ b/tests/ui/entry_btree.rs @@ -0,0 +1,18 @@ +// run-rustfix + +#![warn(clippy::map_entry)] +#![allow(dead_code)] + +use std::collections::BTreeMap; + +fn foo() {} + +fn btree_map(m: &mut BTreeMap, k: K, v: V) { + // insert then do something, use if let + if !m.contains_key(&k) { + m.insert(k, v); + foo(); + } +} + +fn main() {} diff --git a/tests/ui/entry_btree.stderr b/tests/ui/entry_btree.stderr new file mode 100644 index 00000000000..5c6fcdf1a28 --- /dev/null +++ b/tests/ui/entry_btree.stderr @@ -0,0 +1,20 @@ +error: usage of `contains_key` followed by `insert` on a `BTreeMap` + --> $DIR/entry_btree.rs:12:5 + | +LL | / if !m.contains_key(&k) { +LL | | m.insert(k, v); +LL | | foo(); +LL | | } + | |_____^ + | + = note: `-D clippy::map-entry` implied by `-D warnings` +help: try this + | +LL ~ if let std::collections::btree_map::Entry::Vacant(e) = m.entry(k) { +LL + e.insert(v); +LL + foo(); +LL + } + | + +error: aborting due to previous error + diff --git a/tests/ui/linkedlist.rs b/tests/ui/linkedlist.rs index 2c3b25cd45e..690ea810a62 100644 --- a/tests/ui/linkedlist.rs +++ b/tests/ui/linkedlist.rs @@ -1,6 +1,6 @@ #![feature(associated_type_defaults)] #![warn(clippy::linkedlist)] -#![allow(dead_code, clippy::needless_pass_by_value)] +#![allow(unused, dead_code, clippy::needless_pass_by_value)] extern crate alloc; use alloc::collections::linked_list::LinkedList; @@ -20,24 +20,29 @@ fn foo(_: LinkedList) {} const BAR: Option> = None; } -struct Bar; +pub struct Bar { + priv_linked_list_field: LinkedList, + pub pub_linked_list_field: LinkedList, +} impl Bar { fn foo(_: LinkedList) {} } -pub fn test(my_favourite_linked_list: LinkedList) { - println!("{:?}", my_favourite_linked_list) +// All of these test should be trigger the lint because they are not +// part of the public api +fn test(my_favorite_linked_list: LinkedList) {} +fn test_ret() -> Option> { + None } - -pub fn test_ret() -> Option> { - unimplemented!(); -} - -pub fn test_local_not_linted() { +fn test_local_not_linted() { let _: LinkedList; } -fn main() { - test(LinkedList::new()); - test_local_not_linted(); +// All of these test should be allowed because they are part of the +// public api and `avoid_breaking_exported_api` is `false` by default. +pub fn pub_test(the_most_awesome_linked_list: LinkedList) {} +pub fn pub_test_ret() -> Option> { + None } + +fn main() {} diff --git a/tests/ui/linkedlist.stderr b/tests/ui/linkedlist.stderr index 38ae71714d6..51327df1321 100644 --- a/tests/ui/linkedlist.stderr +++ b/tests/ui/linkedlist.stderr @@ -40,7 +40,15 @@ LL | const BAR: Option>; = help: a `VecDeque` might work error: you seem to be using a `LinkedList`! Perhaps you meant some other data structure? - --> $DIR/linkedlist.rs:25:15 + --> $DIR/linkedlist.rs:24:29 + | +LL | priv_linked_list_field: LinkedList, + | ^^^^^^^^^^^^^^ + | + = help: a `VecDeque` might work + +error: you seem to be using a `LinkedList`! Perhaps you meant some other data structure? + --> $DIR/linkedlist.rs:28:15 | LL | fn foo(_: LinkedList) {} | ^^^^^^^^^^^^^^ @@ -48,20 +56,20 @@ LL | fn foo(_: LinkedList) {} = help: a `VecDeque` might work error: you seem to be using a `LinkedList`! Perhaps you meant some other data structure? - --> $DIR/linkedlist.rs:28:39 + --> $DIR/linkedlist.rs:33:34 | -LL | pub fn test(my_favourite_linked_list: LinkedList) { - | ^^^^^^^^^^^^^^ +LL | fn test(my_favorite_linked_list: LinkedList) {} + | ^^^^^^^^^^^^^^ | = help: a `VecDeque` might work error: you seem to be using a `LinkedList`! Perhaps you meant some other data structure? - --> $DIR/linkedlist.rs:32:29 + --> $DIR/linkedlist.rs:34:25 | -LL | pub fn test_ret() -> Option> { - | ^^^^^^^^^^^^^^ +LL | fn test_ret() -> Option> { + | ^^^^^^^^^^^^^^ | = help: a `VecDeque` might work -error: aborting due to 8 previous errors +error: aborting due to 9 previous errors diff --git a/tests/ui/manual_flatten.rs b/tests/ui/manual_flatten.rs index b5bd35a6878..7db6b730963 100644 --- a/tests/ui/manual_flatten.rs +++ b/tests/ui/manual_flatten.rs @@ -91,6 +91,19 @@ fn main() { } } + struct Test { + a: usize, + } + + let mut vec_of_struct = [Some(Test { a: 1 }), None]; + + // Usage of `if let` expression should not trigger lint + for n in vec_of_struct.iter_mut() { + if let Some(z) = n { + *n = None; + } + } + // Using manual flatten should not trigger the lint for n in vec![Some(1), Some(2), Some(3)].iter().flatten() { println!("{}", n); diff --git a/tests/ui/manual_map_option_2.fixed b/tests/ui/manual_map_option_2.fixed new file mode 100644 index 00000000000..8cc12149403 --- /dev/null +++ b/tests/ui/manual_map_option_2.fixed @@ -0,0 +1,50 @@ +// run-rustfix + +#![warn(clippy::manual_map)] +#![allow(clippy::toplevel_ref_arg)] + +fn main() { + // Lint. `y` is declared within the arm, so it isn't captured by the map closure + let _ = Some(0).map(|x| { + let y = (String::new(), String::new()); + (x, y.0) + }); + + // Don't lint. `s` is borrowed until partway through the arm, but needs to be captured by the map + // closure + let s = Some(String::new()); + let _ = match &s { + Some(x) => Some((x.clone(), s)), + None => None, + }; + + // Don't lint. `s` is borrowed until partway through the arm, but needs to be captured by the map + // closure + let s = Some(String::new()); + let _ = match &s { + Some(x) => Some({ + let clone = x.clone(); + let s = || s; + (clone, s()) + }), + None => None, + }; + + // Don't lint. `s` is borrowed until partway through the arm, but needs to be captured as a mutable + // reference by the map closure + let mut s = Some(String::new()); + let _ = match &s { + Some(x) => Some({ + let clone = x.clone(); + let ref mut s = s; + (clone, s) + }), + None => None, + }; + + // Lint. `s` is captured by reference, so no lifetime issues. + let s = Some(String::new()); + let _ = s.as_ref().map(|x| { + if let Some(ref s) = s { (x.clone(), s) } else { panic!() } + }); +} diff --git a/tests/ui/manual_map_option_2.rs b/tests/ui/manual_map_option_2.rs new file mode 100644 index 00000000000..0862b201ead --- /dev/null +++ b/tests/ui/manual_map_option_2.rs @@ -0,0 +1,56 @@ +// run-rustfix + +#![warn(clippy::manual_map)] +#![allow(clippy::toplevel_ref_arg)] + +fn main() { + // Lint. `y` is declared within the arm, so it isn't captured by the map closure + let _ = match Some(0) { + Some(x) => Some({ + let y = (String::new(), String::new()); + (x, y.0) + }), + None => None, + }; + + // Don't lint. `s` is borrowed until partway through the arm, but needs to be captured by the map + // closure + let s = Some(String::new()); + let _ = match &s { + Some(x) => Some((x.clone(), s)), + None => None, + }; + + // Don't lint. `s` is borrowed until partway through the arm, but needs to be captured by the map + // closure + let s = Some(String::new()); + let _ = match &s { + Some(x) => Some({ + let clone = x.clone(); + let s = || s; + (clone, s()) + }), + None => None, + }; + + // Don't lint. `s` is borrowed until partway through the arm, but needs to be captured as a mutable + // reference by the map closure + let mut s = Some(String::new()); + let _ = match &s { + Some(x) => Some({ + let clone = x.clone(); + let ref mut s = s; + (clone, s) + }), + None => None, + }; + + // Lint. `s` is captured by reference, so no lifetime issues. + let s = Some(String::new()); + let _ = match &s { + Some(x) => Some({ + if let Some(ref s) = s { (x.clone(), s) } else { panic!() } + }), + None => None, + }; +} diff --git a/tests/ui/manual_map_option_2.stderr b/tests/ui/manual_map_option_2.stderr new file mode 100644 index 00000000000..711ff6c4a4b --- /dev/null +++ b/tests/ui/manual_map_option_2.stderr @@ -0,0 +1,43 @@ +error: manual implementation of `Option::map` + --> $DIR/manual_map_option_2.rs:8:13 + | +LL | let _ = match Some(0) { + | _____________^ +LL | | Some(x) => Some({ +LL | | let y = (String::new(), String::new()); +LL | | (x, y.0) +LL | | }), +LL | | None => None, +LL | | }; + | |_____^ + | + = note: `-D clippy::manual-map` implied by `-D warnings` +help: try this + | +LL ~ let _ = Some(0).map(|x| { +LL + let y = (String::new(), String::new()); +LL + (x, y.0) +LL ~ }); + | + +error: manual implementation of `Option::map` + --> $DIR/manual_map_option_2.rs:50:13 + | +LL | let _ = match &s { + | _____________^ +LL | | Some(x) => Some({ +LL | | if let Some(ref s) = s { (x.clone(), s) } else { panic!() } +LL | | }), +LL | | None => None, +LL | | }; + | |_____^ + | +help: try this + | +LL ~ let _ = s.as_ref().map(|x| { +LL + if let Some(ref s) = s { (x.clone(), s) } else { panic!() } +LL ~ }); + | + +error: aborting due to 2 previous errors + diff --git a/tests/ui/manual_split_once.fixed b/tests/ui/manual_split_once.fixed new file mode 100644 index 00000000000..3a0332939d4 --- /dev/null +++ b/tests/ui/manual_split_once.fixed @@ -0,0 +1,50 @@ +// run-rustfix + +#![feature(custom_inner_attributes)] +#![warn(clippy::manual_split_once)] +#![allow(clippy::iter_skip_next, clippy::iter_nth_zero)] + +extern crate itertools; + +#[allow(unused_imports)] +use itertools::Itertools; + +fn main() { + let _ = Some("key=value".split_once('=').map_or("key=value", |x| x.0)); + let _ = "key=value".splitn(2, '=').nth(2); + let _ = "key=value".split_once('=').map_or("key=value", |x| x.0); + let _ = "key=value".split_once('=').map_or("key=value", |x| x.0); + let _ = "key=value".split_once('=').unwrap().1; + let _ = "key=value".split_once('=').unwrap().1; + let (_, _) = "key=value".split_once('=').unwrap(); + + let s = String::from("key=value"); + let _ = s.split_once('=').map_or(&*s, |x| x.0); + + let s = Box::::from("key=value"); + let _ = s.split_once('=').map_or(&*s, |x| x.0); + + let s = &"key=value"; + let _ = s.split_once('=').map_or(*s, |x| x.0); + + fn _f(s: &str) -> Option<&str> { + let _ = s.split_once("key=value").map_or(s, |x| x.0); + let _ = s.split_once("key=value")?.1; + let _ = s.split_once("key=value")?.1; + None + } + + // Don't lint, slices don't have `split_once` + let _ = [0, 1, 2].splitn(2, |&x| x == 1).nth(1).unwrap(); +} + +fn _msrv_1_51() { + #![clippy::msrv = "1.51"] + // `str::split_once` was stabilized in 1.16. Do not lint this + let _ = "key=value".splitn(2, '=').nth(1).unwrap(); +} + +fn _msrv_1_52() { + #![clippy::msrv = "1.52"] + let _ = "key=value".split_once('=').unwrap().1; +} diff --git a/tests/ui/manual_split_once.rs b/tests/ui/manual_split_once.rs new file mode 100644 index 00000000000..e6093b63fe8 --- /dev/null +++ b/tests/ui/manual_split_once.rs @@ -0,0 +1,50 @@ +// run-rustfix + +#![feature(custom_inner_attributes)] +#![warn(clippy::manual_split_once)] +#![allow(clippy::iter_skip_next, clippy::iter_nth_zero)] + +extern crate itertools; + +#[allow(unused_imports)] +use itertools::Itertools; + +fn main() { + let _ = "key=value".splitn(2, '=').next(); + let _ = "key=value".splitn(2, '=').nth(2); + let _ = "key=value".splitn(2, '=').next().unwrap(); + let _ = "key=value".splitn(2, '=').nth(0).unwrap(); + let _ = "key=value".splitn(2, '=').nth(1).unwrap(); + let _ = "key=value".splitn(2, '=').skip(1).next().unwrap(); + let (_, _) = "key=value".splitn(2, '=').next_tuple().unwrap(); + + let s = String::from("key=value"); + let _ = s.splitn(2, '=').next().unwrap(); + + let s = Box::::from("key=value"); + let _ = s.splitn(2, '=').nth(0).unwrap(); + + let s = &"key=value"; + let _ = s.splitn(2, '=').skip(0).next().unwrap(); + + fn _f(s: &str) -> Option<&str> { + let _ = s.splitn(2, "key=value").next()?; + let _ = s.splitn(2, "key=value").nth(1)?; + let _ = s.splitn(2, "key=value").skip(1).next()?; + None + } + + // Don't lint, slices don't have `split_once` + let _ = [0, 1, 2].splitn(2, |&x| x == 1).nth(1).unwrap(); +} + +fn _msrv_1_51() { + #![clippy::msrv = "1.51"] + // `str::split_once` was stabilized in 1.16. Do not lint this + let _ = "key=value".splitn(2, '=').nth(1).unwrap(); +} + +fn _msrv_1_52() { + #![clippy::msrv = "1.52"] + let _ = "key=value".splitn(2, '=').nth(1).unwrap(); +} diff --git a/tests/ui/manual_split_once.stderr b/tests/ui/manual_split_once.stderr new file mode 100644 index 00000000000..4f15196b469 --- /dev/null +++ b/tests/ui/manual_split_once.stderr @@ -0,0 +1,82 @@ +error: manual implementation of `split_once` + --> $DIR/manual_split_once.rs:13:13 + | +LL | let _ = "key=value".splitn(2, '=').next(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `Some("key=value".split_once('=').map_or("key=value", |x| x.0))` + | + = note: `-D clippy::manual-split-once` implied by `-D warnings` + +error: manual implementation of `split_once` + --> $DIR/manual_split_once.rs:15:13 + | +LL | let _ = "key=value".splitn(2, '=').next().unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"key=value".split_once('=').map_or("key=value", |x| x.0)` + +error: manual implementation of `split_once` + --> $DIR/manual_split_once.rs:16:13 + | +LL | let _ = "key=value".splitn(2, '=').nth(0).unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"key=value".split_once('=').map_or("key=value", |x| x.0)` + +error: manual implementation of `split_once` + --> $DIR/manual_split_once.rs:17:13 + | +LL | let _ = "key=value".splitn(2, '=').nth(1).unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"key=value".split_once('=').unwrap().1` + +error: manual implementation of `split_once` + --> $DIR/manual_split_once.rs:18:13 + | +LL | let _ = "key=value".splitn(2, '=').skip(1).next().unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"key=value".split_once('=').unwrap().1` + +error: manual implementation of `split_once` + --> $DIR/manual_split_once.rs:19:18 + | +LL | let (_, _) = "key=value".splitn(2, '=').next_tuple().unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"key=value".split_once('=')` + +error: manual implementation of `split_once` + --> $DIR/manual_split_once.rs:22:13 + | +LL | let _ = s.splitn(2, '=').next().unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `s.split_once('=').map_or(&*s, |x| x.0)` + +error: manual implementation of `split_once` + --> $DIR/manual_split_once.rs:25:13 + | +LL | let _ = s.splitn(2, '=').nth(0).unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `s.split_once('=').map_or(&*s, |x| x.0)` + +error: manual implementation of `split_once` + --> $DIR/manual_split_once.rs:28:13 + | +LL | let _ = s.splitn(2, '=').skip(0).next().unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `s.split_once('=').map_or(*s, |x| x.0)` + +error: manual implementation of `split_once` + --> $DIR/manual_split_once.rs:31:17 + | +LL | let _ = s.splitn(2, "key=value").next()?; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `s.split_once("key=value").map_or(s, |x| x.0)` + +error: manual implementation of `split_once` + --> $DIR/manual_split_once.rs:32:17 + | +LL | let _ = s.splitn(2, "key=value").nth(1)?; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `s.split_once("key=value")?.1` + +error: manual implementation of `split_once` + --> $DIR/manual_split_once.rs:33:17 + | +LL | let _ = s.splitn(2, "key=value").skip(1).next()?; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `s.split_once("key=value")?.1` + +error: manual implementation of `split_once` + --> $DIR/manual_split_once.rs:49:13 + | +LL | let _ = "key=value".splitn(2, '=').nth(1).unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"key=value".split_once('=').unwrap().1` + +error: aborting due to 13 previous errors + diff --git a/tests/ui/methods.rs b/tests/ui/methods.rs index 513d930e056..c441b35b992 100644 --- a/tests/ui/methods.rs +++ b/tests/ui/methods.rs @@ -32,7 +32,7 @@ use std::rc::{self, Rc}; use std::sync::{self, Arc}; -use option_helpers::IteratorFalsePositives; +use option_helpers::{IteratorFalsePositives, IteratorMethodFalsePositives}; struct Lt<'a> { foo: &'a u32, @@ -131,6 +131,9 @@ fn filter_next() { // Check that we don't lint if the caller is not an `Iterator`. let foo = IteratorFalsePositives { foo: 0 }; let _ = foo.filter().next(); + + let foo = IteratorMethodFalsePositives {}; + let _ = foo.filter(42).next(); } fn main() { diff --git a/tests/ui/missing-doc-crate.stderr b/tests/ui/missing-doc-crate.stderr deleted file mode 100644 index e69de29bb2d..00000000000 diff --git a/tests/ui/missing_const_for_fn/cant_be_const.stderr b/tests/ui/missing_const_for_fn/cant_be_const.stderr deleted file mode 100644 index e69de29bb2d..00000000000 diff --git a/tests/ui/option_if_let_else.fixed b/tests/ui/option_if_let_else.fixed index 769ccc14bc1..d1815d0aec3 100644 --- a/tests/ui/option_if_let_else.fixed +++ b/tests/ui/option_if_let_else.fixed @@ -1,3 +1,4 @@ +// edition:2018 // run-rustfix #![warn(clippy::option_if_let_else)] #![allow(clippy::redundant_closure)] @@ -86,4 +87,65 @@ fn main() { test_map_or_else(None); let _ = negative_tests(None); let _ = impure_else(None); + + let _ = Some(0).map_or(0, |x| loop { + if x == 0 { + break x; + } + }); + + // #7576 + const fn _f(x: Option) -> u32 { + // Don't lint, `map_or` isn't const + if let Some(x) = x { x } else { 10 } + } + + // #5822 + let s = String::new(); + // Don't lint, `Some` branch consumes `s`, but else branch uses `s` + let _ = if let Some(x) = Some(0) { + let s = s; + s.len() + x + } else { + s.len() + }; + + let s = String::new(); + // Lint, both branches immutably borrow `s`. + let _ = Some(0).map_or_else(|| s.len(), |x| s.len() + x); + + let s = String::new(); + // Lint, `Some` branch consumes `s`, but else branch doesn't use `s`. + let _ = Some(0).map_or(1, |x| { + let s = s; + s.len() + x + }); + + let s = Some(String::new()); + // Don't lint, `Some` branch borrows `s`, but else branch consumes `s` + let _ = if let Some(x) = &s { + x.len() + } else { + let _s = s; + 10 + }; + + let mut s = Some(String::new()); + // Don't lint, `Some` branch mutably borrows `s`, but else branch also borrows `s` + let _ = if let Some(x) = &mut s { + x.push_str("test"); + x.len() + } else { + let _s = &s; + 10 + }; + + async fn _f1(x: u32) -> u32 { + x + } + + async fn _f2() { + // Don't lint. `await` can't be moved into a closure. + let _ = if let Some(x) = Some(0) { _f1(x).await } else { 0 }; + } } diff --git a/tests/ui/option_if_let_else.rs b/tests/ui/option_if_let_else.rs index e2f8dec3b93..a15627338cb 100644 --- a/tests/ui/option_if_let_else.rs +++ b/tests/ui/option_if_let_else.rs @@ -1,3 +1,4 @@ +// edition:2018 // run-rustfix #![warn(clippy::option_if_let_else)] #![allow(clippy::redundant_closure)] @@ -105,4 +106,71 @@ fn main() { test_map_or_else(None); let _ = negative_tests(None); let _ = impure_else(None); + + let _ = if let Some(x) = Some(0) { + loop { + if x == 0 { + break x; + } + } + } else { + 0 + }; + + // #7576 + const fn _f(x: Option) -> u32 { + // Don't lint, `map_or` isn't const + if let Some(x) = x { x } else { 10 } + } + + // #5822 + let s = String::new(); + // Don't lint, `Some` branch consumes `s`, but else branch uses `s` + let _ = if let Some(x) = Some(0) { + let s = s; + s.len() + x + } else { + s.len() + }; + + let s = String::new(); + // Lint, both branches immutably borrow `s`. + let _ = if let Some(x) = Some(0) { s.len() + x } else { s.len() }; + + let s = String::new(); + // Lint, `Some` branch consumes `s`, but else branch doesn't use `s`. + let _ = if let Some(x) = Some(0) { + let s = s; + s.len() + x + } else { + 1 + }; + + let s = Some(String::new()); + // Don't lint, `Some` branch borrows `s`, but else branch consumes `s` + let _ = if let Some(x) = &s { + x.len() + } else { + let _s = s; + 10 + }; + + let mut s = Some(String::new()); + // Don't lint, `Some` branch mutably borrows `s`, but else branch also borrows `s` + let _ = if let Some(x) = &mut s { + x.push_str("test"); + x.len() + } else { + let _s = &s; + 10 + }; + + async fn _f1(x: u32) -> u32 { + x + } + + async fn _f2() { + // Don't lint. `await` can't be moved into a closure. + let _ = if let Some(x) = Some(0) { _f1(x).await } else { 0 }; + } } diff --git a/tests/ui/option_if_let_else.stderr b/tests/ui/option_if_let_else.stderr index 099e49ef8e3..ed748ee8b39 100644 --- a/tests/ui/option_if_let_else.stderr +++ b/tests/ui/option_if_let_else.stderr @@ -1,5 +1,5 @@ error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:7:5 + --> $DIR/option_if_let_else.rs:8:5 | LL | / if let Some(x) = string { LL | | (true, x) @@ -11,19 +11,19 @@ LL | | } = note: `-D clippy::option-if-let-else` implied by `-D warnings` error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:25:13 + --> $DIR/option_if_let_else.rs:26:13 | LL | let _ = if let Some(s) = *string { s.len() } else { 0 }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `string.map_or(0, |s| s.len())` error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:26:13 + --> $DIR/option_if_let_else.rs:27:13 | LL | let _ = if let Some(s) = &num { s } else { &0 }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)` error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:27:13 + --> $DIR/option_if_let_else.rs:28:13 | LL | let _ = if let Some(s) = &mut num { | _____________^ @@ -43,13 +43,13 @@ LL ~ }); | error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:33:13 + --> $DIR/option_if_let_else.rs:34:13 | LL | let _ = if let Some(ref s) = num { s } else { &0 }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)` error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:34:13 + --> $DIR/option_if_let_else.rs:35:13 | LL | let _ = if let Some(mut s) = num { | _____________^ @@ -69,7 +69,7 @@ LL ~ }); | error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:40:13 + --> $DIR/option_if_let_else.rs:41:13 | LL | let _ = if let Some(ref mut s) = num { | _____________^ @@ -89,7 +89,7 @@ LL ~ }); | error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:49:5 + --> $DIR/option_if_let_else.rs:50:5 | LL | / if let Some(x) = arg { LL | | let y = x * x; @@ -108,7 +108,7 @@ LL + }) | error: use Option::map_or_else instead of an if let/else - --> $DIR/option_if_let_else.rs:62:13 + --> $DIR/option_if_let_else.rs:63:13 | LL | let _ = if let Some(x) = arg { | _____________^ @@ -120,7 +120,7 @@ LL | | }; | |_____^ help: try: `arg.map_or_else(|| side_effect(), |x| x)` error: use Option::map_or_else instead of an if let/else - --> $DIR/option_if_let_else.rs:71:13 + --> $DIR/option_if_let_else.rs:72:13 | LL | let _ = if let Some(x) = arg { | _____________^ @@ -143,10 +143,58 @@ LL ~ }, |x| x * x * x * x); | error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:100:13 + --> $DIR/option_if_let_else.rs:101:13 | LL | let _ = if let Some(x) = optional { x + 2 } else { 5 }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `optional.map_or(5, |x| x + 2)` -error: aborting due to 11 previous errors +error: use Option::map_or instead of an if let/else + --> $DIR/option_if_let_else.rs:110:13 + | +LL | let _ = if let Some(x) = Some(0) { + | _____________^ +LL | | loop { +LL | | if x == 0 { +LL | | break x; +... | +LL | | 0 +LL | | }; + | |_____^ + | +help: try + | +LL ~ let _ = Some(0).map_or(0, |x| loop { +LL + if x == 0 { +LL + break x; +LL + } +LL ~ }); + | + +error: use Option::map_or_else instead of an if let/else + --> $DIR/option_if_let_else.rs:138:13 + | +LL | let _ = if let Some(x) = Some(0) { s.len() + x } else { s.len() }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(0).map_or_else(|| s.len(), |x| s.len() + x)` + +error: use Option::map_or instead of an if let/else + --> $DIR/option_if_let_else.rs:142:13 + | +LL | let _ = if let Some(x) = Some(0) { + | _____________^ +LL | | let s = s; +LL | | s.len() + x +LL | | } else { +LL | | 1 +LL | | }; + | |_____^ + | +help: try + | +LL ~ let _ = Some(0).map_or(1, |x| { +LL + let s = s; +LL + s.len() + x +LL ~ }); + | + +error: aborting due to 14 previous errors diff --git a/tests/ui/rc_buffer_redefined_string.stderr b/tests/ui/rc_buffer_redefined_string.stderr deleted file mode 100644 index e69de29bb2d..00000000000 diff --git a/tests/ui/rc_mutex.rs b/tests/ui/rc_mutex.rs index 657a3ecf6a0..18e8a2e01e0 100644 --- a/tests/ui/rc_mutex.rs +++ b/tests/ui/rc_mutex.rs @@ -1,13 +1,17 @@ #![warn(clippy::rc_mutex)] -#![allow(clippy::blacklisted_name)] +#![allow(unused, clippy::blacklisted_name)] use std::rc::Rc; use std::sync::Mutex; -pub struct MyStruct { +pub struct MyStructWithPrivItem { foo: Rc>, } +pub struct MyStructWithPubItem { + pub foo: Rc>, +} + pub struct SubT { foo: T, } @@ -17,18 +21,16 @@ pub enum MyEnum { Two, } -pub fn test1(foo: Rc>) {} +// All of these test should be trigger the lint because they are not +// part of the public api +fn test1(foo: Rc>) {} +fn test2(foo: Rc>) {} +fn test3(foo: Rc>>) {} -pub fn test2(foo: Rc>) {} +// All of these test should be allowed because they are part of the +// public api and `avoid_breaking_exported_api` is `false` by default. +pub fn pub_test1(foo: Rc>) {} +pub fn pub_test2(foo: Rc>) {} +pub fn pub_test3(foo: Rc>>) {} -pub fn test3(foo: Rc>>) {} - -fn main() { - test1(Rc::new(Mutex::new(1))); - test2(Rc::new(Mutex::new(MyEnum::One))); - test3(Rc::new(Mutex::new(SubT { foo: 1 }))); - - let _my_struct = MyStruct { - foo: Rc::new(Mutex::new(1)), - }; -} +fn main() {} diff --git a/tests/ui/rc_mutex.stderr b/tests/ui/rc_mutex.stderr index 8e58e2bc2d0..fe84361d781 100644 --- a/tests/ui/rc_mutex.stderr +++ b/tests/ui/rc_mutex.stderr @@ -1,28 +1,35 @@ -error: found `Rc>`. Consider using `Rc>` or `Arc>` instead +error: usage of `Rc>` --> $DIR/rc_mutex.rs:8:10 | LL | foo: Rc>, | ^^^^^^^^^^^^^^ | = note: `-D clippy::rc-mutex` implied by `-D warnings` + = help: consider using `Rc>` or `Arc>` instead -error: found `Rc>`. Consider using `Rc>` or `Arc>` instead - --> $DIR/rc_mutex.rs:20:22 +error: usage of `Rc>` + --> $DIR/rc_mutex.rs:26:18 | -LL | pub fn test1(foo: Rc>) {} - | ^^^^^^^^^^^^ +LL | fn test1(foo: Rc>) {} + | ^^^^^^^^^^^^ + | + = help: consider using `Rc>` or `Arc>` instead -error: found `Rc>`. Consider using `Rc>` or `Arc>` instead - --> $DIR/rc_mutex.rs:22:19 +error: usage of `Rc>` + --> $DIR/rc_mutex.rs:27:15 | -LL | pub fn test2(foo: Rc>) {} - | ^^^^^^^^^^^^^^^^^ +LL | fn test2(foo: Rc>) {} + | ^^^^^^^^^^^^^^^^^ + | + = help: consider using `Rc>` or `Arc>` instead -error: found `Rc>`. Consider using `Rc>` or `Arc>` instead - --> $DIR/rc_mutex.rs:24:19 +error: usage of `Rc>` + --> $DIR/rc_mutex.rs:28:15 | -LL | pub fn test3(foo: Rc>>) {} - | ^^^^^^^^^^^^^^^^^^^^^^ +LL | fn test3(foo: Rc>>) {} + | ^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider using `Rc>` or `Arc>` instead error: aborting due to 4 previous errors diff --git a/tests/ui/redundant_allocation.rs b/tests/ui/redundant_allocation.rs index 1b4f2a66c70..52fbc91e325 100644 --- a/tests/ui/redundant_allocation.rs +++ b/tests/ui/redundant_allocation.rs @@ -77,4 +77,24 @@ pub fn arc_test9(foo: Arc>) -> Arc>> { } } +// https://github.com/rust-lang/rust-clippy/issues/7487 +mod box_dyn { + use std::boxed::Box; + use std::rc::Rc; + use std::sync::Arc; + + pub trait T {} + + struct S { + a: Box>, + b: Rc>, + c: Arc>, + } + + pub fn test_box(_: Box>) {} + pub fn test_rc(_: Rc>) {} + pub fn test_arc(_: Arc>) {} + pub fn test_rc_box(_: Rc>>) {} +} + fn main() {} diff --git a/tests/ui/redundant_allocation.stderr b/tests/ui/redundant_allocation.stderr index fdab74eb538..c3b10e5f5e6 100644 --- a/tests/ui/redundant_allocation.stderr +++ b/tests/ui/redundant_allocation.stderr @@ -134,5 +134,14 @@ LL | pub fn arc_test9(foo: Arc>) -> Arc>> { = note: `Rc>` is already on the heap, `Arc>>` makes an extra allocation = help: consider using just `Arc>` or `Rc>` -error: aborting due to 15 previous errors +error: usage of `Rc>>` + --> $DIR/redundant_allocation.rs:97:27 + | +LL | pub fn test_rc_box(_: Rc>>) {} + | ^^^^^^^^^^^^^^^^^^^ + | + = note: `Box>` is already on the heap, `Rc>>` makes an extra allocation + = help: consider using just `Rc>` or `Box>` + +error: aborting due to 16 previous errors diff --git a/tests/ui/temporary_assignment.rs b/tests/ui/temporary_assignment.rs index b4a931043b0..ac4c1bc6597 100644 --- a/tests/ui/temporary_assignment.rs +++ b/tests/ui/temporary_assignment.rs @@ -1,5 +1,4 @@ #![warn(clippy::temporary_assignment)] -#![allow(const_item_mutation)] use std::ops::{Deref, DerefMut}; diff --git a/tests/ui/temporary_assignment.stderr b/tests/ui/temporary_assignment.stderr index 4cc32c79f05..7d79901a28d 100644 --- a/tests/ui/temporary_assignment.stderr +++ b/tests/ui/temporary_assignment.stderr @@ -1,5 +1,5 @@ error: assignment to temporary - --> $DIR/temporary_assignment.rs:48:5 + --> $DIR/temporary_assignment.rs:47:5 | LL | Struct { field: 0 }.field = 1; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -7,7 +7,7 @@ LL | Struct { field: 0 }.field = 1; = note: `-D clippy::temporary-assignment` implied by `-D warnings` error: assignment to temporary - --> $DIR/temporary_assignment.rs:49:5 + --> $DIR/temporary_assignment.rs:48:5 | LL | / MultiStruct { LL | | structure: Struct { field: 0 }, @@ -17,13 +17,13 @@ LL | | .field = 1; | |______________^ error: assignment to temporary - --> $DIR/temporary_assignment.rs:54:5 + --> $DIR/temporary_assignment.rs:53:5 | LL | ArrayStruct { array: [0] }.array[0] = 1; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: assignment to temporary - --> $DIR/temporary_assignment.rs:55:5 + --> $DIR/temporary_assignment.rs:54:5 | LL | (0, 0).0 = 1; | ^^^^^^^^^^^^ diff --git a/tests/ui/unnecessary_operation.fixed b/tests/ui/unnecessary_operation.fixed index 2fca96c4cd5..bf0ec8deb34 100644 --- a/tests/ui/unnecessary_operation.fixed +++ b/tests/ui/unnecessary_operation.fixed @@ -62,10 +62,10 @@ fn main() { get_number(); 5;get_number(); 42;get_number(); - [42, 55];get_usize(); + assert!([42, 55].len() > get_usize()); 42;get_number(); get_number(); - [42; 55];get_usize(); + assert!([42; 55].len() > get_usize()); get_number(); String::from("blah"); diff --git a/tests/ui/unnecessary_operation.stderr b/tests/ui/unnecessary_operation.stderr index f88c9f9908b..f66d08ecb82 100644 --- a/tests/ui/unnecessary_operation.stderr +++ b/tests/ui/unnecessary_operation.stderr @@ -1,128 +1,128 @@ -error: statement can be reduced +error: unnecessary operation --> $DIR/unnecessary_operation.rs:51:5 | LL | Tuple(get_number()); - | ^^^^^^^^^^^^^^^^^^^^ help: replace it with: `get_number();` + | ^^^^^^^^^^^^^^^^^^^^ help: statement can be reduced to: `get_number();` | = note: `-D clippy::unnecessary-operation` implied by `-D warnings` -error: statement can be reduced +error: unnecessary operation --> $DIR/unnecessary_operation.rs:52:5 | LL | Struct { field: get_number() }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `get_number();` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: statement can be reduced to: `get_number();` -error: statement can be reduced +error: unnecessary operation --> $DIR/unnecessary_operation.rs:53:5 | LL | Struct { ..get_struct() }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `get_struct();` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: statement can be reduced to: `get_struct();` -error: statement can be reduced +error: unnecessary operation --> $DIR/unnecessary_operation.rs:54:5 | LL | Enum::Tuple(get_number()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `get_number();` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: statement can be reduced to: `get_number();` -error: statement can be reduced +error: unnecessary operation --> $DIR/unnecessary_operation.rs:55:5 | LL | Enum::Struct { field: get_number() }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `get_number();` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: statement can be reduced to: `get_number();` -error: statement can be reduced +error: unnecessary operation --> $DIR/unnecessary_operation.rs:56:5 | LL | 5 + get_number(); - | ^^^^^^^^^^^^^^^^^ help: replace it with: `5;get_number();` + | ^^^^^^^^^^^^^^^^^ help: statement can be reduced to: `5;get_number();` -error: statement can be reduced +error: unnecessary operation --> $DIR/unnecessary_operation.rs:57:5 | LL | *&get_number(); - | ^^^^^^^^^^^^^^^ help: replace it with: `get_number();` + | ^^^^^^^^^^^^^^^ help: statement can be reduced to: `get_number();` -error: statement can be reduced +error: unnecessary operation --> $DIR/unnecessary_operation.rs:58:5 | LL | &get_number(); - | ^^^^^^^^^^^^^^ help: replace it with: `get_number();` + | ^^^^^^^^^^^^^^ help: statement can be reduced to: `get_number();` -error: statement can be reduced +error: unnecessary operation --> $DIR/unnecessary_operation.rs:59:5 | LL | (5, 6, get_number()); - | ^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `5;6;get_number();` + | ^^^^^^^^^^^^^^^^^^^^^ help: statement can be reduced to: `5;6;get_number();` -error: statement can be reduced +error: unnecessary operation --> $DIR/unnecessary_operation.rs:60:5 | LL | box get_number(); - | ^^^^^^^^^^^^^^^^^ help: replace it with: `get_number();` + | ^^^^^^^^^^^^^^^^^ help: statement can be reduced to: `get_number();` -error: statement can be reduced +error: unnecessary operation --> $DIR/unnecessary_operation.rs:61:5 | LL | get_number()..; - | ^^^^^^^^^^^^^^^ help: replace it with: `get_number();` + | ^^^^^^^^^^^^^^^ help: statement can be reduced to: `get_number();` -error: statement can be reduced +error: unnecessary operation --> $DIR/unnecessary_operation.rs:62:5 | LL | ..get_number(); - | ^^^^^^^^^^^^^^^ help: replace it with: `get_number();` + | ^^^^^^^^^^^^^^^ help: statement can be reduced to: `get_number();` -error: statement can be reduced +error: unnecessary operation --> $DIR/unnecessary_operation.rs:63:5 | LL | 5..get_number(); - | ^^^^^^^^^^^^^^^^ help: replace it with: `5;get_number();` + | ^^^^^^^^^^^^^^^^ help: statement can be reduced to: `5;get_number();` -error: statement can be reduced +error: unnecessary operation --> $DIR/unnecessary_operation.rs:64:5 | LL | [42, get_number()]; - | ^^^^^^^^^^^^^^^^^^^ help: replace it with: `42;get_number();` + | ^^^^^^^^^^^^^^^^^^^ help: statement can be reduced to: `42;get_number();` -error: statement can be reduced +error: unnecessary operation --> $DIR/unnecessary_operation.rs:65:5 | LL | [42, 55][get_usize()]; - | ^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `[42, 55];get_usize();` + | ^^^^^^^^^^^^^^^^^^^^^^ help: statement can be written as: `assert!([42, 55].len() > get_usize());` -error: statement can be reduced +error: unnecessary operation --> $DIR/unnecessary_operation.rs:66:5 | LL | (42, get_number()).1; - | ^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `42;get_number();` + | ^^^^^^^^^^^^^^^^^^^^^ help: statement can be reduced to: `42;get_number();` -error: statement can be reduced +error: unnecessary operation --> $DIR/unnecessary_operation.rs:67:5 | LL | [get_number(); 55]; - | ^^^^^^^^^^^^^^^^^^^ help: replace it with: `get_number();` + | ^^^^^^^^^^^^^^^^^^^ help: statement can be reduced to: `get_number();` -error: statement can be reduced +error: unnecessary operation --> $DIR/unnecessary_operation.rs:68:5 | LL | [42; 55][get_usize()]; - | ^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `[42; 55];get_usize();` + | ^^^^^^^^^^^^^^^^^^^^^^ help: statement can be written as: `assert!([42; 55].len() > get_usize());` -error: statement can be reduced +error: unnecessary operation --> $DIR/unnecessary_operation.rs:69:5 | LL | / { LL | | get_number() LL | | }; - | |______^ help: replace it with: `get_number();` + | |______^ help: statement can be reduced to: `get_number();` -error: statement can be reduced +error: unnecessary operation --> $DIR/unnecessary_operation.rs:72:5 | LL | / FooString { LL | | s: String::from("blah"), LL | | }; - | |______^ help: replace it with: `String::from("blah");` + | |______^ help: statement can be reduced to: `String::from("blah");` error: aborting due to 20 previous errors