diff options
39 files changed, 996 insertions, 717 deletions
diff --git a/.travis.yml b/.travis.yml index 09972a12f8d..75b45c9db40 100644 --- a/.travis.yml +++ b/.travis.yml @@ -63,8 +63,7 @@ matrix: - env: INTEGRATION=chronotope/chrono - env: INTEGRATION=serde-rs/serde - env: INTEGRATION=Geal/nom -# uncomment once https://github.com/rust-lang/rust/issues/55376 is fixed -# - env: INTEGRATION=hyperium/hyper + - env: INTEGRATION=hyperium/hyper allow_failures: - os: windows env: BASE_TEST=true diff --git a/Cargo.toml b/Cargo.toml index 518c2caf671..2293913f38e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -48,7 +48,7 @@ rustc_tools_util = { version = "0.1.0", path = "rustc_tools_util"} [dev-dependencies] clippy_dev = { version = "0.0.1", path = "clippy_dev" } cargo_metadata = "0.6" -compiletest_rs = "0.3.7" +compiletest_rs = "0.3.16" lazy_static = "1.0" serde_derive = "1.0" clippy-mini-macro-test = { version = "0.2", path = "mini-macro" } diff --git a/ci/base-tests.sh b/ci/base-tests.sh index 72a38ee5e58..9b73263c24a 100755 --- a/ci/base-tests.sh +++ b/ci/base-tests.sh @@ -24,21 +24,20 @@ cd clippy_lints && cargo test && cd .. cd rustc_tools_util && cargo test && cd .. # check that the lint lists are up-to-date ./util/update_lints.py -c -mkdir -p ~/rust/cargo/bin -cp target/debug/cargo-clippy ~/rust/cargo/bin/cargo-clippy -cp target/debug/clippy-driver ~/rust/cargo/bin/clippy-driver -rm ~/.cargo/bin/cargo-clippy + +CLIPPY="`pwd`/target/debug/cargo-clippy clippy" # run clippy on its own codebase... -PATH=$PATH:~/rust/cargo/bin cargo clippy --all-targets --all-features -- -D clippy::all -D clippy::internal +${CLIPPY} --all-targets --all-features -- -D clippy::all -D clippy::internal # ... and some test directories -cd clippy_workspace_tests && PATH=$PATH:~/rust/cargo/bin cargo clippy -- -D clippy::all && cd .. -cd clippy_workspace_tests/src && PATH=$PATH:~/rust/cargo/bin cargo clippy -- -D clippy::all && cd ../.. -cd clippy_workspace_tests/subcrate && PATH=$PATH:~/rust/cargo/bin cargo clippy -- -D clippy::all && cd ../.. -cd clippy_workspace_tests/subcrate/src && PATH=$PATH:~/rust/cargo/bin cargo clippy -- -D clippy::all && cd ../../.. -cd clippy_dev && PATH=$PATH:~/rust/cargo/bin cargo clippy -- -D clippy::all && cd .. -cd rustc_tools_util/ && PATH=$PATH:~/rust/cargo/bin cargo clippy -- -D clippy::all && cd .. +for dir in clippy_workspace_tests clippy_workspace_tests/src clippy_workspace_tests/subcrate clippy_workspace_tests/subcrate/src clippy_dev rustc_tools_util +do + cd ${dir} + ${CLIPPY} -- -D clippy::all + cd - +done + # test --manifest-path -PATH=$PATH:~/rust/cargo/bin cargo clippy --manifest-path=clippy_workspace_tests/Cargo.toml -- -D clippy::all -cd clippy_workspace_tests/subcrate && PATH=$PATH:~/rust/cargo/bin cargo clippy --manifest-path=../Cargo.toml -- -D clippy::all && cd ../.. +${CLIPPY} --manifest-path=clippy_workspace_tests/Cargo.toml -- -D clippy::all +cd clippy_workspace_tests/subcrate && ${CLIPPY} --manifest-path=../Cargo.toml -- -D clippy::all && cd ../.. set +x diff --git a/ci/integration-tests.sh b/ci/integration-tests.sh index 9019a6830e6..75decab940e 100755 --- a/ci/integration-tests.sh +++ b/ci/integration-tests.sh @@ -28,9 +28,6 @@ function check() { } case ${INTEGRATION} in - rust-lang/cargo) - check - ;; *) check ;; diff --git a/clippy_lints/src/identity_conversion.rs b/clippy_lints/src/identity_conversion.rs index e9761616696..00ce58f00b0 100644 --- a/clippy_lints/src/identity_conversion.rs +++ b/clippy_lints/src/identity_conversion.rs @@ -12,7 +12,7 @@ use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; use crate::rustc::{declare_tool_lint, lint_array}; use crate::rustc::hir::*; use crate::syntax::ast::NodeId; -use crate::utils::{in_macro, match_def_path, match_trait_method, same_tys, snippet, span_lint_and_then}; +use crate::utils::{in_macro, match_def_path, match_trait_method, same_tys, snippet, snippet_with_macro_callsite, span_lint_and_then}; use crate::utils::{opt_def_id, paths, resolve_node}; use crate::rustc_errors::Applicability; @@ -72,7 +72,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IdentityConversion { let a = cx.tables.expr_ty(e); let b = cx.tables.expr_ty(&args[0]); if same_tys(cx, a, b) { - let sugg = snippet(cx, args[0].span, "<expr>").into_owned(); + let sugg = snippet_with_macro_callsite(cx, args[0].span, "<expr>").to_string(); + span_lint_and_then(cx, IDENTITY_CONVERSION, e.span, "identical conversion", |db| { db.span_suggestion_with_applicability( e.span, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index eaff87e78f8..207dc40fa1d 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -48,6 +48,7 @@ use toml; // Currently, categories "style", "correctness", "complexity" and "perf" are enabled by default, // as said in the README.md of this repository. If this changes, please update README.md. +#[macro_export] macro_rules! declare_clippy_lint { { pub $name:tt, style, $description:tt } => { declare_tool_lint! { pub clippy::$name, Warn, $description, report_in_external_macro: true } diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index e46615f4da2..4a704c3d52e 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -19,7 +19,8 @@ use crate::syntax::ast::LitKind; use crate::syntax::source_map::Span; use crate::utils::paths; use crate::utils::{expr_block, is_allowed, is_expn_of, match_qpath, match_type, multispan_sugg, - remove_blocks, snippet, span_lint_and_sugg, span_lint_and_then, span_note_and_lint, walk_ptrs_ty}; + remove_blocks, snippet, span_lint_and_sugg, span_lint_and_then, + span_note_and_lint, walk_ptrs_ty}; use crate::utils::sugg::Sugg; use crate::consts::{constant, Constant}; use crate::rustc_errors::Applicability; diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 01f97264d0b..3f1c1e73956 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -21,7 +21,7 @@ use crate::utils::sugg; use crate::utils::{ get_arg_name, get_trait_def_id, implements_trait, in_macro, is_copy, is_expn_of, is_self, is_self_ty, iter_input_pats, last_path_segment, match_def_path, match_path, match_qpath, match_trait_method, match_type, - match_var, method_chain_args, remove_blocks, return_ty, same_tys, single_segment_path, snippet, span_lint, + match_var, method_chain_args, remove_blocks, return_ty, same_tys, single_segment_path, snippet, snippet_with_macro_callsite, span_lint, span_lint_and_sugg, span_lint_and_then, span_note_and_lint, walk_ptrs_ty, walk_ptrs_ty_depth, SpanlessEq, }; use if_chain::if_chain; @@ -568,7 +568,14 @@ declare_clippy_lint! { /// **Why is this bad?** Using the Index trait (`[]`) is more clear and more /// concise. /// -/// **Known problems:** None. +/// **Known problems:** Not a replacement for error handling: Using either +/// `.unwrap()` or the Index trait (`[]`) carries the risk of causing a `panic` +/// if the value being accessed is `None`. If the use of `.get().unwrap()` is a +/// temporary placeholder for dealing with the `Option` type, then this does +/// not mitigate the need for error handling. If there is a chance that `.get()` +/// will be `None` in your program, then it is advisable that the `None` case +/// is handled in a future refactor instead of using `.unwrap()` or the Index +/// trait. /// /// **Example:** /// ```rust @@ -1062,9 +1069,9 @@ fn lint_or_fun_call(cx: &LateContext<'_, '_>, expr: &hir::Expr, method_span: Spa } let sugg: Cow<'_, _> = match (fn_has_arguments, !or_has_args) { - (true, _) => format!("|_| {}", snippet(cx, arg.span, "..")).into(), - (false, false) => format!("|| {}", snippet(cx, arg.span, "..")).into(), - (false, true) => snippet(cx, fun_span, ".."), + (true, _) => format!("|_| {}", snippet_with_macro_callsite(cx, arg.span, "..")).into(), + (false, false) => format!("|| {}", snippet_with_macro_callsite(cx, arg.span, "..")).into(), + (false, true) => snippet_with_macro_callsite(cx, fun_span, ".."), }; let span_replace_word = method_span.with_hi(span.hi()); span_lint_and_sugg( diff --git a/clippy_lints/src/needless_bool.rs b/clippy_lints/src/needless_bool.rs index f102b49d785..e13f757adb9 100644 --- a/clippy_lints/src/needless_bool.rs +++ b/clippy_lints/src/needless_bool.rs @@ -17,7 +17,7 @@ use crate::rustc::{declare_tool_lint, lint_array}; use crate::rustc::hir::*; use crate::syntax::ast::LitKind; use crate::syntax::source_map::Spanned; -use crate::utils::{snippet, span_lint, span_lint_and_sugg}; +use crate::utils::{in_macro, snippet, span_lint, span_lint_and_sugg}; use crate::utils::sugg::Sugg; /// **What it does:** Checks for expressions of the form `if c { true } else { @@ -133,6 +133,9 @@ impl LintPass for BoolComparison { impl<'a, 'tcx> LateLintPass<'a, 'tcx> for BoolComparison { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, e: &'tcx Expr) { + if in_macro(e.span) { + return; + } use self::Expression::*; if let ExprKind::Binary(Spanned { node: BinOpKind::Eq, .. }, ref left_side, ref right_side) = e.node { match (fetch_bool_expr(left_side), fetch_bool_expr(right_side)) { diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index 8c895915921..2ed877d1364 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -12,7 +12,7 @@ use crate::rustc::hir::{def_id, Body, FnDecl}; use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; use crate::rustc::mir::{ self, traversal, - visit::{PlaceContext, Visitor}, + visit::{MutatingUseContext, NonUseContext, PlaceContext, Visitor}, TerminatorKind, }; use crate::rustc::ty; @@ -279,7 +279,7 @@ impl<'tcx> mir::visit::Visitor<'tcx> for LocalUseVisitor { fn visit_local(&mut self, local: &mir::Local, ctx: PlaceContext<'tcx>, _: mir::Location) { match ctx { - PlaceContext::Drop | PlaceContext::StorageDead => return, + PlaceContext::MutatingUse(MutatingUseContext::Drop) | PlaceContext::NonUse(NonUseContext::StorageDead) => return, _ => {}, } diff --git a/clippy_lints/src/strings.rs b/clippy_lints/src/strings.rs index f4798842205..e07b1649a46 100644 --- a/clippy_lints/src/strings.rs +++ b/clippy_lints/src/strings.rs @@ -7,7 +7,6 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. - use crate::rustc::hir::*; use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; use crate::rustc::{declare_tool_lint, lint_array}; @@ -92,7 +91,14 @@ impl LintPass for StringAdd { impl<'a, 'tcx> LateLintPass<'a, 'tcx> for StringAdd { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, e: &'tcx Expr) { - if let ExprKind::Binary(Spanned { node: BinOpKind::Add, .. }, ref left, _) = e.node { + if let ExprKind::Binary( + Spanned { + node: BinOpKind::Add, .. + }, + ref left, + _, + ) = e.node + { if is_string(cx, left) { if !is_allowed(cx, STRING_ADD_ASSIGN, e.id) { let parent = get_parent_expr(cx, e); @@ -132,13 +138,15 @@ fn is_string(cx: &LateContext<'_, '_>, e: &Expr) -> bool { fn is_add(cx: &LateContext<'_, '_>, src: &Expr, target: &Expr) -> bool { match src.node { - ExprKind::Binary(Spanned { node: BinOpKind::Add, .. }, ref left, _) => SpanlessEq::new(cx).eq_expr(target, left), + ExprKind::Binary( + Spanned { + node: BinOpKind::Add, .. + }, + ref left, + _, + ) => SpanlessEq::new(cx).eq_expr(target, left), ExprKind::Block(ref block, _) => { - block.stmts.is_empty() - && block - .expr - .as_ref() - .map_or(false, |expr| is_add(cx, expr, target)) + block.stmts.is_empty() && block.expr.as_ref().map_or(false, |expr| is_add(cx, expr, target)) }, _ => false, } @@ -155,14 +163,33 @@ impl LintPass for StringLitAsBytes { impl<'a, 'tcx> LateLintPass<'a, 'tcx> for StringLitAsBytes { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, e: &'tcx Expr) { - use crate::syntax::ast::LitKind; + use crate::syntax::ast::{LitKind, StrStyle}; use crate::utils::{in_macro, snippet}; if let ExprKind::MethodCall(ref path, _, ref args) = e.node { if path.ident.name == "as_bytes" { if let ExprKind::Lit(ref lit) = args[0].node { - if let LitKind::Str(ref lit_content, _) = lit.node { - if lit_content.as_str().chars().all(|c| c.is_ascii()) && !in_macro(args[0].span) { + if let LitKind::Str(ref lit_content, style) = lit.node { + let callsite = snippet(cx, args[0].span.source_callsite(), r#""foo""#); + let expanded = if let StrStyle::Raw(n) = style { + let term = (0..n).map(|_| '#').collect::<String>(); + format!("r{0}\"{1}\"{0}", term, lit_content.as_str()) + } else { + format!("\"{}\"", lit_content.as_str()) + }; + if callsite.starts_with("include_str!") { + span_lint_and_sugg( + cx, + STRING_LIT_AS_BYTES, + e.span, + "calling `as_bytes()` on `include_str!(..)`", + "consider using `include_bytes!(..)` instead", + snippet(cx, args[0].span, r#""foo""#).replacen("include_str", "include_bytes", 1), + ); + } else if callsite == expanded + && lit_content.as_str().chars().all(|c| c.is_ascii()) + && !in_macro(args[0].span) + { span_lint_and_sugg( cx, STRING_LIT_AS_BYTES, diff --git a/clippy_lints/src/use_self.rs b/clippy_lints/src/use_self.rs index a8b7e820681..b997d76d0e7 100644 --- a/clippy_lints/src/use_self.rs +++ b/clippy_lints/src/use_self.rs @@ -36,7 +36,7 @@ use crate::syntax_pos::symbol::keywords::SelfType; /// } /// ``` /// could be -/// ``` +/// ```rust /// struct Foo {} /// impl Foo { /// fn new() -> Self { diff --git a/clippy_lints/src/utils/internal_lints.rs b/clippy_lints/src/utils/internal_lints.rs index 3a0d056bbb5..879157ec8a4 100644 --- a/clippy_lints/src/utils/internal_lints.rs +++ b/clippy_lints/src/utils/internal_lints.rs @@ -9,12 +9,13 @@ use crate::utils::{ - match_qpath, match_type, paths, span_help_and_lint, span_lint, span_lint_and_sugg, walk_ptrs_ty, + match_def_path, match_type, paths, span_help_and_lint, span_lint, span_lint_and_sugg, walk_ptrs_ty, }; use if_chain::if_chain; use crate::rustc::hir; use crate::rustc::hir::intravisit::{walk_expr, NestedVisitorMap, Visitor}; use crate::rustc::hir::*; +use crate::rustc::hir::def::Def; use crate::rustc::lint::{EarlyContext, EarlyLintPass, LateContext, LateLintPass, LintArray, LintPass}; use crate::rustc::{declare_tool_lint, lint_array}; use crate::rustc_data_structures::fx::{FxHashMap, FxHashSet}; @@ -160,15 +161,21 @@ impl LintPass for LintWithoutLintPass { impl<'a, 'tcx> LateLintPass<'a, 'tcx> for LintWithoutLintPass { fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx Item) { - if let hir::ItemKind::Static(ref ty, MutImmutable, body_id) = item.node { - if is_lint_ref_type(ty) { + if let hir::ItemKind::Static(ref ty, MutImmutable, _) = item.node { + if is_lint_ref_type(cx, ty) { self.declared_lints.insert(item.name, item.span); - } else if is_lint_array_type(ty) && item.name == "ARRAY" { - if let VisibilityKind::Inherited = item.vis.node { + } + } else if let hir::ItemKind::Impl(.., Some(ref trait_ref), _, ref impl_item_refs) = item.node { + if_chain! { + if let hir::TraitRef{path, ..} = trait_ref; + if let Def::Trait(def_id) = path.def; + if match_def_path(cx.tcx, def_id, &paths::LINT_PASS); + then { let mut collector = LintCollector { output: &mut self.registered_lints, cx, }; + let body_id = cx.tcx.hir.body_owned_by(impl_item_refs[0].id.node_id); collector.visit_expr(&cx.tcx.hir.body(body_id).value); } } @@ -203,28 +210,22 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for LintWithoutLintPass { } } -fn is_lint_ref_type(ty: &Ty) -> bool { +fn is_lint_ref_type<'tcx>(cx: &LateContext<'_, 'tcx>, ty: &Ty) -> bool { if let TyKind::Rptr( _, MutTy { ty: ref inner, mutbl: MutImmutable, }, - ) = ty.node - { + ) = ty.node { if let TyKind::Path(ref path) = inner.node { - return match_qpath(path, &paths::LINT); + if let Def::Struct(def_id) = cx.tables.qpath_def(path, inner.hir_id) { + return match_def_path(cx.tcx, def_id, &paths::LINT); + } } } - false -} -fn is_lint_array_type(ty: &Ty) -> bool { - if let TyKind::Path(ref path) = ty.node { - match_qpath(path, &paths::LINT_ARRAY) - } else { - false - } + false } struct LintCollector<'a, 'tcx: 'a> { diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 1a8db837f32..9d11950dd73 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -362,6 +362,12 @@ pub fn snippet<'a, 'b, T: LintContext<'b>>(cx: &T, span: Span, default: &'a str) snippet_opt(cx, span).map_or_else(|| Cow::Borrowed(default), From::from) } +/// Same as `snippet`, but should only be used when it's clear that the input span is +/// not a macro argument. +pub fn snippet_with_macro_callsite<'a, 'b, T: LintContext<'b>>(cx: &T, span: Span, default: &'a str) -> Cow<'a, str> { + snippet(cx, span.source_callsite(), default) +} + /// Convert a span to a code snippet. Returns `None` if not available. pub fn snippet_opt<'a, T: LintContext<'a>>(cx: &T, span: Span) -> Option<String> { cx.sess().source_map().span_to_snippet(span).ok() @@ -386,7 +392,7 @@ pub fn snippet_block<'a, 'b, T: LintContext<'b>>(cx: &T, span: Span, default: &' pub fn last_line_of_span<'a, T: LintContext<'a>>(cx: &T, span: Span) -> Span { let file_map_and_line = cx.sess().source_map().lookup_line(span.lo()).unwrap(); let line_no = file_map_and_line.line; - let line_start = &file_map_and_line.fm.lines[line_no]; + let line_start = &file_map_and_line.sf.lines[line_no]; Span::new(*line_start, span.hi(), span.ctxt()) } @@ -400,7 +406,10 @@ pub fn expr_block<'a, 'b, T: LintContext<'b>>( ) -> Cow<'a, str> { let code = snippet_block(cx, expr.span, default); let string = option.unwrap_or_default(); - if let ExprKind::Block(_, _) = expr.node { + if in_macro(expr.span) { + Cow::Owned(format!("{{ {} }}", snippet_with_macro_callsite(cx, expr.span, default))) + } + else if let ExprKind::Block(_, _) = expr.node { Cow::Owned(format!("{}{}", code, string)) } else if string.is_empty() { Cow::Owned(format!("{{ {} }}", code)) diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 8941d303156..107a8eea15d 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/clippy_lints/src/utils/paths.rs @@ -56,7 +56,7 @@ pub const ITERATOR: [&str; 4] = ["core", "iter", "iterator", "Iterator"]; pub const LATE_CONTEXT: [&str; 4] = ["rustc", "lint", "context", "LateContext"]; pub const LINKED_LIST: [&str; 4] = ["alloc", "collections", "linked_list", "LinkedList"]; pub const LINT: [&str; 3] = ["rustc", "lint", "Lint"]; -pub const LINT_ARRAY: [&str; 3] = ["rustc", "lint", "LintArray"]; +pub const LINT_PASS: [&str; 3] = ["rustc", "lint", "LintPass"]; pub const MEM_DISCRIMINANT: [&str; 3] = ["core", "mem", "discriminant"]; pub const MEM_FORGET: [&str; 3] = ["core", "mem", "forget"]; pub const MEM_REPLACE: [&str; 3] = ["core", "mem", "replace"]; diff --git a/clippy_lints/src/utils/sugg.rs b/clippy_lints/src/utils/sugg.rs index eb67838f1d1..90f48f0f83c 100644 --- a/clippy_lints/src/utils/sugg.rs +++ b/clippy_lints/src/utils/sugg.rs @@ -548,7 +548,7 @@ impl<'a, 'b, 'c, T: LintContext<'c>> DiagnosticBuilderExt<'c, T> for rustc_error let hi = cx.sess().source_map().next_point(remove_span).hi(); let fmpos = cx.sess().source_map().lookup_byte_offset(hi); - if let Some(ref src) = fmpos.fm.src { + if let Some(ref src) = fmpos.sf.src { let non_whitespace_offset = src[fmpos.pos.to_usize()..].find(|c| c != ' ' && c != '\t' && c != '\n'); if let Some(non_whitespace_offset) = non_whitespace_offset { diff --git a/tests/compile-test.rs b/tests/compile-test.rs index c9d4f658935..64360af641b 100644 --- a/tests/compile-test.rs +++ b/tests/compile-test.rs @@ -75,7 +75,10 @@ fn config(mode: &str, dir: PathBuf) -> compiletest::Config { } fn run_mode(mode: &str, dir: PathBuf) { - compiletest::run_tests(&config(mode, dir)); + let cfg = config(mode, dir); + // clean rmeta data, otherwise "cargo check; cargo test" fails (#2896) + cfg.clean_rmeta(); + compiletest::run_tests(&cfg); } fn run_ui_toml_tests(config: &compiletest::Config, mut tests: Vec<test::TestDescAndFn>) -> Result<bool, io::Error> { diff --git a/tests/ui/explicit_counter_loop.rs b/tests/ui/explicit_counter_loop.rs new file mode 100644 index 00000000000..eaed606b89e --- /dev/null +++ b/tests/ui/explicit_counter_loop.rs @@ -0,0 +1,122 @@ +// Copyright 2014-2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution. +// +// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or +// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license +// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![warn(clippy::explicit_counter_loop)] + +fn main() { + let mut vec = vec![1, 2, 3, 4]; + let mut _index = 0; + for _v in &vec { + _index += 1 + } + + let mut _index = 1; + _index = 0; + for _v in &vec { + _index += 1 + } +} + +mod issue_1219 { + pub fn test() { + // should not trigger the lint because variable is used after the loop #473 + let vec = vec![1,2,3]; + let mut index = 0; + for _v in &vec { index += 1 } + println!("index: {}", index); + + // should not trigger the lint because the count is conditional #1219 + let text = "banana"; + let mut count = 0; + for ch in text.chars() { + if ch == 'a' { + continue; + } + count += 1; + println!("{}", count); + } + + // should not trigger the lint because the count is conditional + let text = "banana"; + let mut count = 0; + for ch in text.chars() { + if ch == 'a' { + count += 1; + } + println!("{}", count); + } + + // should trigger the lint because the count is not conditional + let text = "banana"; + let mut count = 0; + for ch in text.chars() { + count += 1; + if ch == 'a' { + continue; + } + println!("{}", count); + } + + // should trigger the lint because the count is not conditional + let text = "banana"; + let mut count = 0; + for ch in text.chars() { + count += 1; + for i in 0..2 { + let _ = 123; + } + println!("{}", count); + } + + // should not trigger the lint because the count is incremented multiple times + let text = "banana"; + let mut count = 0; + for ch in text.chars() { + count += 1; + for i in 0..2 { + count += 1; + } + println!("{}", count); + } + } +} + +mod issue_3308 { + pub fn test() { + // should not trigger the lint because the count is incremented multiple times + let mut skips = 0; + let erasures = vec![]; + for i in 0..10 { + while erasures.contains(&(i + skips)) { + skips += 1; + } + println!("{}", skips); + } + + // should not trigger the lint because the count is incremented multiple times + let mut skips = 0; + for i in 0..10 { + let mut j = 0; + while j < 5 { + skips += 1; + j += 1; + } + println!("{}", skips); + } + + // should not trigger the lint because the count is incremented multiple times + let mut skips = 0; + for i in 0..10 { + for j in 0..5 { + skips += 1; + } + println!("{}", skips); + } + } +} diff --git a/tests/ui/explicit_counter_loop.stderr b/tests/ui/explicit_counter_loop.stderr new file mode 100644 index 00000000000..023f7f299a7 --- /dev/null +++ b/tests/ui/explicit_counter_loop.stderr @@ -0,0 +1,28 @@ +error: the variable `_index` is used as a loop counter. Consider using `for (_index, item) in &vec.enumerate()` or similar iterators + --> $DIR/explicit_counter_loop.rs:15:15 + | +15 | for _v in &vec { + | ^^^^ + | + = note: `-D clippy::explicit-counter-loop` implied by `-D warnings` + +error: the variable `_index` is used as a loop counter. Consider using `for (_index, item) in &vec.enumerate()` or similar iterators + --> $DIR/explicit_counter_loop.rs:21:15 + | +21 | for _v in &vec { + | ^^^^ + +error: the variable `count` is used as a loop counter. Consider using `for (count, item) in text.chars().enumerate()` or similar iterators + --> $DIR/explicit_counter_loop.rs:58:19 + | +58 | for ch in text.chars() { + | ^^^^^^^^^^^^ + +error: the variable `count` is used as a loop counter. Consider using `for (count, item) in text.chars().enumerate()` or similar iterators + --> $DIR/explicit_counter_loop.rs:69:19 + | +69 | for ch in text.chars() { + | ^^^^^^^^^^^^ + +error: aborting due to 4 previous errors + diff --git a/tests/ui/for_loop.rs b/tests/ui/for_loop.rs index f80270d9fe8..2a70149f246 100644 --- a/tests/ui/for_loop.rs +++ b/tests/ui/for_loop.rs @@ -7,10 +7,6 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. - - - - use std::collections::*; use std::rc::Rc; @@ -18,60 +14,6 @@ static STATIC: [usize; 4] = [0, 1, 8, 16]; const CONST: [usize; 4] = [0, 1, 8, 16]; #[warn(clippy::all)] -fn for_loop_over_option_and_result() { - let option = Some(1); - let result = option.ok_or("x not found"); - let v = vec![0, 1, 2]; - - // check FOR_LOOP_OVER_OPTION lint - for x in option { - println!("{}", x); - } - - // check FOR_LOOP_OVER_RESULT lint - for x in result { - println!("{}", x); - } - - for x in option.ok_or("x not found") { - println!("{}", x); - } - - // make sure LOOP_OVER_NEXT lint takes clippy::precedence when next() is the last call - // in the chain - for x in v.iter().next() { - println!("{}", x); - } - - // make sure we lint when next() is not the last call in the chain - for x in v.iter().next().and(Some(0)) { - println!("{}", x); - } - - for x in v.iter().next().ok_or("x not found") { - println!("{}", x); - } - - // check for false positives - - // for loop false positive - for x in v { - println!("{}", x); - } - - // while let false positive for Option - while let Some(x) = option { - println!("{}", x); - break; - } - - // while let false positive for Result - while let Ok(x) = result { - println!("{}", x); - break; - } -} - struct Unrelated(Vec<u8>); impl Unrelated { fn next(&self) -> std::slice::Iter<u8> { @@ -84,7 +26,7 @@ impl Unrelated { } #[warn(clippy::needless_range_loop, clippy::explicit_iter_loop, clippy::explicit_into_iter_loop, clippy::iter_next_loop, clippy::reverse_range_loop, - clippy::explicit_counter_loop, clippy::for_kv_map)] + clippy::for_kv_map)] #[warn(clippy::unused_collect)] #[allow(clippy::linkedlist, clippy::shadow_unrelated, clippy::unnecessary_mut_passed, clippy::cyclomatic_complexity, clippy::similar_names)] #[allow(clippy::many_single_char_names, unused_variables)] @@ -275,16 +217,6 @@ fn main() { let _y = vec.iter().cloned().map(|x| out.push(x)).collect::<Vec<_>>(); // this is fine // Loop with explicit counter variable - let mut _index = 0; - for _v in &vec { - _index += 1 - } - - let mut _index = 1; - _index = 0; - for _v in &vec { - _index += 1 - } // Potential false positives let mut _index = 0; @@ -389,8 +321,6 @@ fn main() { } println!("index: {}", index); - for_loop_over_option_and_result(); - let m: HashMap<u64, u64> = HashMap::new(); for (_, v) in &m { let _v = v; @@ -594,103 +524,3 @@ mod issue_2496 { unimplemented!() } } - -mod issue_1219 { - #[warn(clippy::explicit_counter_loop)] - pub fn test() { - // should not trigger the lint because variable is used after the loop #473 - let vec = vec![1,2,3]; - let mut index = 0; - for _v in &vec { index += 1 } - println!("index: {}", index); - - // should not trigger the lint because the count is conditional #1219 - let text = "banana"; - let mut count = 0; - for ch in text.chars() { - if ch == 'a' { - continue; - } - count += 1; - println!("{}", count); - } - - // should not trigger the lint because the count is conditional - let text = "banana"; - let mut count = 0; - for ch in text.chars() { - if ch == 'a' { - count += 1; - } - println!("{}", count); - } - - // should trigger the lint because the count is not conditional - let text = "banana"; - let mut count = 0; - for ch in text.chars() { - count += 1; - if ch == 'a' { - continue; - } - println!("{}", count); - } - - // should trigger the lint because the count is not conditional - let text = "banana"; - let mut count = 0; - for ch in text.chars() { - count += 1; - for i in 0..2 { - let _ = 123; - } - println!("{}", count); - } - - // should not trigger the lint because the count is incremented multiple times - let text = "banana"; - let mut count = 0; - for ch in text.chars() { - count += 1; - for i in 0..2 { - count += 1; - } - println!("{}", count); - } - } -} - -mod issue_3308 { - #[warn(clippy::explicit_counter_loop)] - pub fn test() { - // should not trigger the lint because the count is incremented multiple times - let mut skips = 0; - let erasures = vec![]; - for i in 0..10 { - while erasures.contains(&(i + skips)) { - skips += 1; - } - println!("{}", skips); - } - - // should not trigger the lint because the count is incremented multiple times - let mut skips = 0; - for i in 0..10 { - let mut j = 0; - while j < 5 { - skips += 1; - j += 1; - } - println!("{}", skips); - } - - // should not trigger the lint because the count is incremented multiple times - let mut skips = 0; - for i in 0..10 { - for j in 0..5 { - skips += 1; - } - println!("{}", skips); - } - } -} diff --git a/tests/ui/for_loop.stderr b/tests/ui/for_loop.stderr index 695209de53f..f70a6d3b32f 100644 --- a/tests/ui/for_loop.stderr +++ b/tests/ui/for_loop.stderr @@ -1,515 +1,421 @@ -error: for loop over `option`, which is an `Option`. This is more readably written as an `if let` statement. - --> $DIR/for_loop.rs:27:14 - | -27 | for x in option { - | ^^^^^^ - | - = note: `-D clippy::for-loop-over-option` implied by `-D warnings` - = help: consider replacing `for x in option` with `if let Some(x) = option` - -error: for loop over `result`, which is a `Result`. This is more readably written as an `if let` statement. - --> $DIR/for_loop.rs:32:14 - | -32 | for x in result { - | ^^^^^^ - | - = note: `-D clippy::for-loop-over-result` implied by `-D warnings` - = help: consider replacing `for x in result` with `if let Ok(x) = result` - -error: for loop over `option.ok_or("x not found")`, which is a `Result`. This is more readably written as an `if let` statement. - --> $DIR/for_loop.rs:36:14 - | -36 | for x in option.ok_or("x not found") { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = help: consider replacing `for x in option.ok_or("x not found")` with `if let Ok(x) = option.ok_or("x not found")` - -error: you are iterating over `Iterator::next()` which is an Option; this will compile but is probably not what you want - --> $DIR/for_loop.rs:42:14 - | -42 | for x in v.iter().next() { - | ^^^^^^^^^^^^^^^ - | - = note: `-D clippy::iter-next-loop` implied by `-D warnings` - -error: for loop over `v.iter().next().and(Some(0))`, which is an `Option`. This is more readably written as an `if let` statement. - --> $DIR/for_loop.rs:47:14 - | -47 | for x in v.iter().next().and(Some(0)) { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = help: consider replacing `for x in v.iter().next().and(Some(0))` with `if let Some(x) = v.iter().next().and(Some(0))` - -error: for loop over `v.iter().next().ok_or("x not found")`, which is a `Result`. This is more readably written as an `if let` statement. - --> $DIR/for_loop.rs:51:14 - | -51 | for x in v.iter().next().ok_or("x not found") { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = help: consider replacing `for x in v.iter().next().ok_or("x not found")` with `if let Ok(x) = v.iter().next().ok_or("x not found")` - -error: this loop never actually loops - --> $DIR/for_loop.rs:63:5 - | -63 | / while let Some(x) = option { -64 | | println!("{}", x); -65 | | break; -66 | | } - | |_____^ - | - = note: `-D clippy::never-loop` implied by `-D warnings` - -error: this loop never actually loops - --> $DIR/for_loop.rs:69:5 - | -69 | / while let Ok(x) = result { -70 | | println!("{}", x); -71 | | break; -72 | | } - | |_____^ - error: the loop variable `i` is only used to index `vec`. - --> $DIR/for_loop.rs:96:14 + --> $DIR/for_loop.rs:38:14 | -96 | for i in 0..vec.len() { +38 | for i in 0..vec.len() { | ^^^^^^^^^^^^ | = note: `-D clippy::needless-range-loop` implied by `-D warnings` help: consider using an iterator | -96 | for <item> in &vec { +38 | for <item> in &vec { | ^^^^^^ ^^^^ error: the loop variable `i` is only used to index `vec`. - --> $DIR/for_loop.rs:105:14 - | -105 | for i in 0..vec.len() { - | ^^^^^^^^^^^^ + --> $DIR/for_loop.rs:47:14 + | +47 | for i in 0..vec.len() { + | ^^^^^^^^^^^^ help: consider using an iterator - | -105 | for <item> in &vec { - | ^^^^^^ ^^^^ + | +47 | for <item> in &vec { + | ^^^^^^ ^^^^ error: the loop variable `j` is only used to index `STATIC`. - --> $DIR/for_loop.rs:110:14 - | -110 | for j in 0..4 { - | ^^^^ + --> $DIR/for_loop.rs:52:14 + | +52 | for j in 0..4 { + | ^^^^ help: consider using an iterator - | -110 | for <item> in &STATIC { - | ^^^^^^ ^^^^^^^ + | +52 | for <item> in &STATIC { + | ^^^^^^ ^^^^^^^ error: the loop variable `j` is only used to index `CONST`. - --> $DIR/for_loop.rs:114:14 - | -114 | for j in 0..4 { - | ^^^^ + --> $DIR/for_loop.rs:56:14 + | +56 | for j in 0..4 { + | ^^^^ help: consider using an iterator - | -114 | for <item> in &CONST { - | ^^^^^^ ^^^^^^ + | +56 | for <item> in &CONST { + | ^^^^^^ ^^^^^^ error: the loop variable `i` is used to index `vec` - --> $DIR/for_loop.rs:118:14 - | -118 | for i in 0..vec.len() { - | ^^^^^^^^^^^^ + --> $DIR/for_loop.rs:60:14 + | +60 | for i in 0..vec.len() { + | ^^^^^^^^^^^^ help: consider using an iterator - | -118 | for (i, <item>) in vec.iter().enumerate() { - | ^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^ + | +60 | for (i, <item>) in vec.iter().enumerate() { + | ^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^ error: the loop variable `i` is only used to index `vec2`. - --> $DIR/for_loop.rs:126:14 - | -126 | for i in 0..vec.len() { - | ^^^^^^^^^^^^ + --> $DIR/for_loop.rs:68:14 + | +68 | for i in 0..vec.len() { + | ^^^^^^^^^^^^ help: consider using an iterator - | -126 | for <item> in vec2.iter().take(vec.len()) { - | ^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +68 | for <item> in vec2.iter().take(vec.len()) { + | ^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: the loop variable `i` is only used to index `vec`. - --> $DIR/for_loop.rs:130:14 - | -130 | for i in 5..vec.len() { - | ^^^^^^^^^^^^ + --> $DIR/for_loop.rs:72:14 + | +72 | for i in 5..vec.len() { + | ^^^^^^^^^^^^ help: consider using an iterator - | -130 | for <item> in vec.iter().skip(5) { - | ^^^^^^ ^^^^^^^^^^^^^^^^^^ + | +72 | for <item> in vec.iter().skip(5) { + | ^^^^^^ ^^^^^^^^^^^^^^^^^^ error: the loop variable `i` is only used to index `vec`. - --> $DIR/for_loop.rs:134:14 - | -134 | for i in 0..MAX_LEN { - | ^^^^^^^^^^ + --> $DIR/for_loop.rs:76:14 + | +76 | for i in 0..MAX_LEN { + | ^^^^^^^^^^ help: consider using an iterator - | -134 | for <item> in vec.iter().take(MAX_LEN) { - | ^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^ + | +76 | for <item> in vec.iter().take(MAX_LEN) { + | ^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^ error: the loop variable `i` is only used to index `vec`. - --> $DIR/for_loop.rs:138:14 - | -138 | for i in 0..=MAX_LEN { - | ^^^^^^^^^^^ + --> $DIR/for_loop.rs:80:14 + | +80 | for i in 0..=MAX_LEN { + | ^^^^^^^^^^^ help: consider using an iterator - | -138 | for <item> in vec.iter().take(MAX_LEN + 1) { - | ^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +80 | for <item> in vec.iter().take(MAX_LEN + 1) { + | ^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: the loop variable `i` is only used to index `vec`. - --> $DIR/for_loop.rs:142:14 - | -142 | for i in 5..10 { - | ^^^^^ + --> $DIR/for_loop.rs:84:14 + | +84 | for i in 5..10 { + | ^^^^^ help: consider using an iterator - | -142 | for <item> in vec.iter().take(10).skip(5) { - | ^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +84 | for <item> in vec.iter().take(10).skip(5) { + | ^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: the loop variable `i` is only used to index `vec`. - --> $DIR/for_loop.rs:146:14 - | -146 | for i in 5..=10 { - | ^^^^^^ + --> $DIR/for_loop.rs:88:14 + | +88 | for i in 5..=10 { + | ^^^^^^ help: consider using an iterator - | -146 | for <item> in vec.iter().take(10 + 1).skip(5) { - | ^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +88 | for <item> in vec.iter().take(10 + 1).skip(5) { + | ^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: the loop variable `i` is used to index `vec` - --> $DIR/for_loop.rs:150:14 - | -150 | for i in 5..vec.len() { - | ^^^^^^^^^^^^ + --> $DIR/for_loop.rs:92:14 + | +92 | for i in 5..vec.len() { + | ^^^^^^^^^^^^ help: consider using an iterator - | -150 | for (i, <item>) in vec.iter().enumerate().skip(5) { - | ^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +92 | for (i, <item>) in vec.iter().enumerate().skip(5) { + | ^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: the loop variable `i` is used to index `vec` - --> $DIR/for_loop.rs:154:14 - | -154 | for i in 5..10 { - | ^^^^^ + --> $DIR/for_loop.rs:96:14 + | +96 | for i in 5..10 { + | ^^^^^ help: consider using an iterator - | -154 | for (i, <item>) in vec.iter().enumerate().take(10).skip(5) { - | ^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +96 | for (i, <item>) in vec.iter().enumerate().take(10).skip(5) { + | ^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: this range is empty so this for loop will never run - --> $DIR/for_loop.rs:158:14 + --> $DIR/for_loop.rs:100:14 | -158 | for i in 10..0 { +100 | for i in 10..0 { | ^^^^^ | = note: `-D clippy::reverse-range-loop` implied by `-D warnings` help: consider using the following if you are attempting to iterate over this range in reverse | -158 | for i in (0..10).rev() { +100 | for i in (0..10).rev() { | ^^^^^^^^^^^^^ error: this range is empty so this for loop will never run - --> $DIR/for_loop.rs:162:14 + --> $DIR/for_loop.rs:104:14 | -162 | for i in 10..=0 { +104 | for i in 10..=0 { | ^^^^^^ help: consider using the following if you are attempting to iterate over this range in reverse | -162 | for i in (0...10).rev() { +104 | for i in (0...10).rev() { | ^^^^^^^^^^^^^^ error: this range is empty so this for loop will never run - --> $DIR/for_loop.rs:166:14 + --> $DIR/for_loop.rs:108:14 | -166 | for i in MAX_LEN..0 { +108 | for i in MAX_LEN..0 { | ^^^^^^^^^^ help: consider using the following if you are attempting to iterate over this range in reverse | -166 | for i in (0..MAX_LEN).rev() { +108 | for i in (0..MAX_LEN).rev() { | ^^^^^^^^^^^^^^^^^^ error: this range is empty so this for loop will never run - --> $DIR/for_loop.rs:170:14 + --> $DIR/for_loop.rs:112:14 | -170 | for i in 5..5 { +112 | for i in 5..5 { | ^^^^ error: this range is empty so this for loop will never run - --> $DIR/for_loop.rs:195:14 + --> $DIR/for_loop.rs:137:14 | -195 | for i in 10..5 + 4 { +137 | for i in 10..5 + 4 { | ^^^^^^^^^ help: consider using the following if you are attempting to iterate over this range in reverse | -195 | for i in (5 + 4..10).rev() { +137 | for i in (5 + 4..10).rev() { | ^^^^^^^^^^^^^^^^^ error: this range is empty so this for loop will never run - --> $DIR/for_loop.rs:199:14 + --> $DIR/for_loop.rs:141:14 | -199 | for i in (5 + 2)..(3 - 1) { +141 | for i in (5 + 2)..(3 - 1) { | ^^^^^^^^^^^^^^^^ help: consider using the following if you are attempting to iterate over this range in reverse | -199 | for i in ((3 - 1)..(5 + 2)).rev() { +141 | for i in ((3 - 1)..(5 + 2)).rev() { | ^^^^^^^^^^^^^^^^^^^^^^^^ error: this range is empty so this for loop will never run - --> $DIR/for_loop.rs:203:14 + --> $DIR/for_loop.rs:145:14 | -203 | for i in (5 + 2)..(8 - 1) { +145 | for i in (5 + 2)..(8 - 1) { | ^^^^^^^^^^^^^^^^ error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/for_loop.rs:225:15 + --> $DIR/for_loop.rs:167:15 | -225 | for _v in vec.iter() {} +167 | for _v in vec.iter() {} | ^^^^^^^^^^ help: to write this more concisely, try: `&vec` | = note: `-D clippy::explicit-iter-loop` implied by `-D warnings` error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/for_loop.rs:227:15 + --> $DIR/for_loop.rs:169:15 | -227 | for _v in vec.iter_mut() {} +169 | for _v in vec.iter_mut() {} | ^^^^^^^^^^^^^^ help: to write this more concisely, try: `&mut vec` error: it is more concise to loop over containers instead of using explicit iteration methods` - --> $DIR/for_loop.rs:230:15 + --> $DIR/for_loop.rs:172:15 | -230 | for _v in out_vec.into_iter() {} +172 | for _v in out_vec.into_iter() {} | ^^^^^^^^^^^^^^^^^^^ help: to write this more concisely, try: `out_vec` | = note: `-D clippy::explicit-into-iter-loop` implied by `-D warnings` error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/for_loop.rs:233:15 + --> $DIR/for_loop.rs:175:15 | -233 | for _v in array.into_iter() {} +175 | for _v in array.into_iter() {} | ^^^^^^^^^^^^^^^^^ help: to write this more concisely, try: `&array` error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/for_loop.rs:238:15 + --> $DIR/for_loop.rs:180:15 | -238 | for _v in [1, 2, 3].iter() {} +180 | for _v in [1, 2, 3].iter() {} | ^^^^^^^^^^^^^^^^ help: to write this more concisely, try: `&[1, 2, 3]` error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/for_loop.rs:242:15 + --> $DIR/for_loop.rs:184:15 | -242 | for _v in [0; 32].iter() {} +184 | for _v in [0; 32].iter() {} | ^^^^^^^^^^^^^^ help: to write this more concisely, try: `&[0; 32]` error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/for_loop.rs:247:15 + --> $DIR/for_loop.rs:189:15 | -247 | for _v in ll.iter() {} +189 | for _v in ll.iter() {} | ^^^^^^^^^ help: to write this more concisely, try: `&ll` error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/for_loop.rs:250:15 + --> $DIR/for_loop.rs:192:15 | -250 | for _v in vd.iter() {} +192 | for _v in vd.iter() {} | ^^^^^^^^^ help: to write this more concisely, try: `&vd` error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/for_loop.rs:253:15 + --> $DIR/for_loop.rs:195:15 | -253 | for _v in bh.iter() {} +195 | for _v in bh.iter() {} | ^^^^^^^^^ help: to write this more concisely, try: `&bh` error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/for_loop.rs:256:15 + --> $DIR/for_loop.rs:198:15 | -256 | for _v in hm.iter() {} +198 | for _v in hm.iter() {} | ^^^^^^^^^ help: to write this more concisely, try: `&hm` error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/for_loop.rs:259:15 + --> $DIR/for_loop.rs:201:15 | -259 | for _v in bt.iter() {} +201 | for _v in bt.iter() {} | ^^^^^^^^^ help: to write this more concisely, try: `&bt` error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/for_loop.rs:262:15 + --> $DIR/for_loop.rs:204:15 | -262 | for _v in hs.iter() {} +204 | for _v in hs.iter() {} | ^^^^^^^^^ help: to write this more concisely, try: `&hs` error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/for_loop.rs:265:15 + --> $DIR/for_loop.rs:207:15 | -265 | for _v in bs.iter() {} +207 | for _v in bs.iter() {} | ^^^^^^^^^ help: to write this more concisely, try: `&bs` error: you are iterating over `Iterator::next()` which is an Option; this will compile but is probably not what you want - --> $DIR/for_loop.rs:267:15 + --> $DIR/for_loop.rs:209:15 | -267 | for _v in vec.iter().next() {} +209 | for _v in vec.iter().next() {} | ^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::iter-next-loop` implied by `-D warnings` error: you are collect()ing an iterator and throwing away the result. Consider using an explicit for loop to exhaust the iterator - --> $DIR/for_loop.rs:274:5 + --> $DIR/for_loop.rs:216:5 | -274 | vec.iter().cloned().map(|x| out.push(x)).collect::<Vec<_>>(); +216 | vec.iter().cloned().map(|x| out.push(x)).collect::<Vec<_>>(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `-D clippy::unused-collect` implied by `-D warnings` -error: the variable `_index` is used as a loop counter. Consider using `for (_index, item) in &vec.enumerate()` or similar iterators - --> $DIR/for_loop.rs:279:15 - | -279 | for _v in &vec { - | ^^^^ - | - = note: `-D clippy::explicit-counter-loop` implied by `-D warnings` - -error: the variable `_index` is used as a loop counter. Consider using `for (_index, item) in &vec.enumerate()` or similar iterators - --> $DIR/for_loop.rs:285:15 - | -285 | for _v in &vec { - | ^^^^ - error: you seem to want to iterate on a map's values - --> $DIR/for_loop.rs:395:19 + --> $DIR/for_loop.rs:325:19 | -395 | for (_, v) in &m { +325 | for (_, v) in &m { | ^^ | = note: `-D clippy::for-kv-map` implied by `-D warnings` help: use the corresponding method | -395 | for v in m.values() { +325 | for v in m.values() { | ^ ^^^^^^^^^^ error: you seem to want to iterate on a map's values - --> $DIR/for_loop.rs:400:19 + --> $DIR/for_loop.rs:330:19 | -400 | for (_, v) in &*m { +330 | for (_, v) in &*m { | ^^^ help: use the corresponding method | -400 | for v in (*m).values() { +330 | for v in (*m).values() { | ^ ^^^^^^^^^^^^^ error: you seem to want to iterate on a map's values - --> $DIR/for_loop.rs:408:19 + --> $DIR/for_loop.rs:338:19 | -408 | for (_, v) in &mut m { +338 | for (_, v) in &mut m { | ^^^^^^ help: use the corresponding method | -408 | for v in m.values_mut() { +338 | for v in m.values_mut() { | ^ ^^^^^^^^^^^^^^ error: you seem to want to iterate on a map's values - --> $DIR/for_loop.rs:413:19 + --> $DIR/for_loop.rs:343:19 | -413 | for (_, v) in &mut *m { +343 | for (_, v) in &mut *m { | ^^^^^^^ help: use the corresponding method | -413 | for v in (*m).values_mut() { +343 | for v in (*m).values_mut() { | ^ ^^^^^^^^^^^^^^^^^ error: you seem to want to iterate on a map's keys - --> $DIR/for_loop.rs:419:24 + --> $DIR/for_loop.rs:349:24 | -419 | for (k, _value) in rm { +349 | for (k, _value) in rm { | ^^ help: use the corresponding method | -419 | for k in rm.keys() { +349 | for k in rm.keys() { | ^ ^^^^^^^^^ error: it looks like you're manually copying between slices - --> $DIR/for_loop.rs:472:14 + --> $DIR/for_loop.rs:402:14 | -472 | for i in 0..src.len() { +402 | for i in 0..src.len() { | ^^^^^^^^^^^^ help: try replacing the loop by: `dst[..src.len()].clone_from_slice(&src[..])` | = note: `-D clippy::manual-memcpy` implied by `-D warnings` error: it looks like you're manually copying between slices - --> $DIR/for_loop.rs:477:14 + --> $DIR/for_loop.rs:407:14 | -477 | for i in 0..src.len() { +407 | for i in 0..src.len() { | ^^^^^^^^^^^^ help: try replacing the loop by: `dst[10..(src.len() + 10)].clone_from_slice(&src[..])` error: it looks like you're manually copying between slices - --> $DIR/for_loop.rs:482:14 + --> $DIR/for_loop.rs:412:14 | -482 | for i in 0..src.len() { +412 | for i in 0..src.len() { | ^^^^^^^^^^^^ help: try replacing the loop by: `dst[..src.len()].clone_from_slice(&src[10..])` error: it looks like you're manually copying between slices - --> $DIR/for_loop.rs:487:14 + --> $DIR/for_loop.rs:417:14 | -487 | for i in 11..src.len() { +417 | for i in 11..src.len() { | ^^^^^^^^^^^^^ help: try replacing the loop by: `dst[11..src.len()].clone_from_slice(&src[(11 - 10)..(src.len() - 10)])` error: it looks like you're manually copying between slices - --> $DIR/for_loop.rs:492:14 + --> $DIR/for_loop.rs:422:14 | -492 | for i in 0..dst.len() { +422 | for i in 0..dst.len() { | ^^^^^^^^^^^^ help: try replacing the loop by: `dst.clone_from_slice(&src[..dst.len()])` error: it looks like you're manually copying between slices - --> $DIR/for_loop.rs:505:14 + --> $DIR/for_loop.rs:435:14 | -505 | for i in 10..256 { +435 | for i in 10..256 { | ^^^^^^^ help: try replacing the loop by | -505 | for i in dst[10..256].clone_from_slice(&src[(10 - 5)..(256 - 5)]) -506 | dst2[(10 + 500)..(256 + 500)].clone_from_slice(&src[10..256]) { +435 | for i in dst[10..256].clone_from_slice(&src[(10 - 5)..(256 - 5)]) +436 | dst2[(10 + 500)..(256 + 500)].clone_from_slice(&src[10..256]) { | error: it looks like you're manually copying between slices - --> $DIR/for_loop.rs:517:14 + --> $DIR/for_loop.rs:447:14 | -517 | for i in 10..LOOP_OFFSET { +447 | for i in 10..LOOP_OFFSET { | ^^^^^^^^^^^^^^^ help: try replacing the loop by: `dst[(10 + LOOP_OFFSET)..(LOOP_OFFSET + LOOP_OFFSET)].clone_from_slice(&src[(10 - some_var)..(LOOP_OFFSET - some_var)])` error: it looks like you're manually copying between slices - --> $DIR/for_loop.rs:530:14 + --> $DIR/for_loop.rs:460:14 | -530 | for i in 0..src_vec.len() { +460 | for i in 0..src_vec.len() { | ^^^^^^^^^^^^^^^^ help: try replacing the loop by: `dst_vec[..src_vec.len()].clone_from_slice(&src_vec[..])` error: it looks like you're manually copying between slices - --> $DIR/for_loop.rs:559:14 + --> $DIR/for_loop.rs:489:14 | -559 | for i in from..from + src.len() { +489 | for i in from..from + src.len() { | ^^^^^^^^^^^^^^^^^^^^^^ help: try replacing the loop by: `dst[from..from + src.len()].clone_from_slice(&src[0..(from + src.len() - from)])` error: it looks like you're manually copying between slices - --> $DIR/for_loop.rs:563:14 + --> $DIR/for_loop.rs:493:14 | -563 | for i in from..from + 3 { +493 | for i in from..from + 3 { | ^^^^^^^^^^^^^^ help: try replacing the loop by: `dst[from..from + 3].clone_from_slice(&src[0..(from + 3 - from)])` error: it looks like you're manually copying between slices - --> $DIR/for_loop.rs:570:14 + --> $DIR/for_loop.rs:500:14 | -570 | for i in 0..src.len() { +500 | for i in 0..src.len() { | ^^^^^^^^^^^^ help: try replacing the loop by: `dst[..src.len()].clone_from_slice(&src[..])` -error: the variable `count` is used as a loop counter. Consider using `for (count, item) in text.chars().enumerate()` or similar iterators - --> $DIR/for_loop.rs:631:19 - | -631 | for ch in text.chars() { - | ^^^^^^^^^^^^ - -error: the variable `count` is used as a loop counter. Consider using `for (count, item) in text.chars().enumerate()` or similar iterators - --> $DIR/for_loop.rs:642:19 - | -642 | for ch in text.chars() { - | ^^^^^^^^^^^^ - -error: aborting due to 63 previous errors +error: aborting due to 51 previous errors diff --git a/tests/ui/for_loop_over_option_result.rs b/tests/ui/for_loop_over_option_result.rs new file mode 100644 index 00000000000..37fd4e6d038 --- /dev/null +++ b/tests/ui/for_loop_over_option_result.rs @@ -0,0 +1,68 @@ +// Copyright 2014-2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution. +// +// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or +// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license +// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![warn(clippy::for_loop_over_option, clippy::for_loop_over_result)] + +/// Tests for_loop_over_result and for_loop_over_option + +fn for_loop_over_option_and_result() { + let option = Some(1); + let result = option.ok_or("x not found"); + let v = vec![0, 1, 2]; + + // check FOR_LOOP_OVER_OPTION lint + for x in option { + println!("{}", x); + } + + // check FOR_LOOP_OVER_RESULT lint + for x in result { + println!("{}", x); + } + + for x in option.ok_or("x not found") { + println!("{}", x); + } + + // make sure LOOP_OVER_NEXT lint takes clippy::precedence when next() is the last call + // in the chain + for x in v.iter().next() { + println!("{}", x); + } + + // make sure we lint when next() is not the last call in the chain + for x in v.iter().next().and(Some(0)) { + println!("{}", x); + } + + for x in v.iter().next().ok_or("x not found") { + println!("{}", x); + } + + // check for false positives + + // for loop false positive + for x in v { + println!("{}", x); + } + + // while let false positive for Option + while let Some(x) = option { + println!("{}", x); + break; + } + + // while let false positive for Result + while let Ok(x) = result { + println!("{}", x); + break; + } +} + +fn main() {} diff --git a/tests/ui/for_loop_over_option_result.stderr b/tests/ui/for_loop_over_option_result.stderr new file mode 100644 index 00000000000..13ad5fff846 --- /dev/null +++ b/tests/ui/for_loop_over_option_result.stderr @@ -0,0 +1,72 @@ +error: for loop over `option`, which is an `Option`. This is more readably written as an `if let` statement. + --> $DIR/for_loop_over_option_result.rs:20:14 + | +20 | for x in option { + | ^^^^^^ + | + = note: `-D clippy::for-loop-over-option` implied by `-D warnings` + = help: consider replacing `for x in option` with `if let Some(x) = option` + +error: for loop over `result`, which is a `Result`. This is more readably written as an `if let` statement. + --> $DIR/for_loop_over_option_result.rs:25:14 + | +25 | for x in result { + | ^^^^^^ + | + = note: `-D clippy::for-loop-over-result` implied by `-D warnings` + = help: consider replacing `for x in result` with `if let Ok(x) = result` + +error: for loop over `option.ok_or("x not found")`, which is a `Result`. This is more readably written as an `if let` statement. + --> $DIR/for_loop_over_option_result.rs:29:14 + | +29 | for x in option.ok_or("x not found") { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider replacing `for x in option.ok_or("x not found")` with `if let Ok(x) = option.ok_or("x not found")` + +error: you are iterating over `Iterator::next()` which is an Option; this will compile but is probably not what you want + --> $DIR/for_loop_over_option_result.rs:35:14 + | +35 | for x in v.iter().next() { + | ^^^^^^^^^^^^^^^ + | + = note: #[deny(clippy::iter_next_loop)] on by default + +error: for loop over `v.iter().next().and(Some(0))`, which is an `Option`. This is more readably written as an `if let` statement. + --> $DIR/for_loop_over_option_result.rs:40:14 + | +40 | for x in v.iter().next().and(Some(0)) { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider replacing `for x in v.iter().next().and(Some(0))` with `if let Some(x) = v.iter().next().and(Some(0))` + +error: for loop over `v.iter().next().ok_or("x not found")`, which is a `Result`. This is more readably written as an `if let` statement. + --> $DIR/for_loop_over_option_result.rs:44:14 + | +44 | for x in v.iter().next().ok_or("x not found") { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider replacing `for x in v.iter().next().ok_or("x not found")` with `if let Ok(x) = v.iter().next().ok_or("x not found")` + +error: this loop never actually loops + --> $DIR/for_loop_over_option_result.rs:56:5 + | +56 | / while let Some(x) = option { +57 | | println!("{}", x); +58 | | break; +59 | | } + | |_____^ + | + = note: #[deny(clippy::never_loop)] on by default + +error: this loop never actually loops + --> $DIR/for_loop_over_option_result.rs:62:5 + | +62 | / while let Ok(x) = result { +63 | | println!("{}", x); +64 | | break; +65 | | } + | |_____^ + +error: aborting due to 8 previous errors + diff --git a/tests/ui/identity_conversion.rs b/tests/ui/identity_conversion.rs index 9384c9eb206..b5cb92c6d5a 100644 --- a/tests/ui/identity_conversion.rs +++ b/tests/ui/identity_conversion.rs @@ -53,4 +53,5 @@ fn main() { let _ = String::from(format!("A: {:04}", 123)); let _ = "".lines().into_iter(); let _ = vec![1, 2, 3].into_iter().into_iter(); + let _: String = format!("Hello {}", "world").into(); } diff --git a/tests/ui/identity_conversion.stderr b/tests/ui/identity_conversion.stderr index 2ac74191931..15bef8b125e 100644 --- a/tests/ui/identity_conversion.stderr +++ b/tests/ui/identity_conversion.stderr @@ -58,5 +58,11 @@ error: identical conversion 55 | let _ = vec![1, 2, 3].into_iter().into_iter(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into_iter()`: `vec![1, 2, 3].into_iter()` -error: aborting due to 9 previous errors +error: identical conversion + --> $DIR/identity_conversion.rs:56:21 + | +56 | let _: String = format!("Hello {}", "world").into(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into()`: `format!("Hello {}", "world")` + +error: aborting due to 10 previous errors diff --git a/tests/ui/lint_without_lint_pass.rs b/tests/ui/lint_without_lint_pass.rs new file mode 100644 index 00000000000..c7e11840a37 --- /dev/null +++ b/tests/ui/lint_without_lint_pass.rs @@ -0,0 +1,32 @@ +#![deny(clippy::internal)] + +#![feature(rustc_private)] + +#[macro_use] +extern crate rustc; +use rustc::lint; + +#[macro_use] +extern crate clippy_lints; + +declare_clippy_lint! { + pub TEST_LINT, + correctness, + "" +} + +declare_clippy_lint! { + pub TEST_LINT_REGISTERED, + correctness, + "" +} + +pub struct Pass; +impl lint::LintPass for Pass { + fn get_lints(&self) -> lint::LintArray { + lint_array!(TEST_LINT_REGISTERED) + } +} + +fn main() { +} diff --git a/tests/ui/lint_without_lint_pass.stderr b/tests/ui/lint_without_lint_pass.stderr new file mode 100644 index 00000000000..65d1283a6e3 --- /dev/null +++ b/tests/ui/lint_without_lint_pass.stderr @@ -0,0 +1,20 @@ +error: the lint `TEST_LINT` is not added to any `LintPass` + --> $DIR/lint_without_lint_pass.rs:12:1 + | +12 | / declare_clippy_lint! { +13 | | pub TEST_LINT, +14 | | correctness, +15 | | "" +16 | | } + | |_^ + | +note: lint level defined here + --> $DIR/lint_without_lint_pass.rs:1:9 + | +1 | #![deny(clippy::internal)] + | ^^^^^^^^^^^^^^^^ + = note: #[deny(clippy::lint_without_lint_pass)] implied by #[deny(clippy::internal)] + = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) + +error: aborting due to previous error + diff --git a/tests/ui/matches.stderr b/tests/ui/matches.stderr index bed903faf1a..b5f1f2ab0e7 100644 --- a/tests/ui/matches.stderr +++ b/tests/ui/matches.stderr @@ -33,7 +33,7 @@ error: you seem to be trying to use match for destructuring a single pattern. Co 51 | | &(v, 1) => println!("{}", v), 52 | | _ => println!("none"), 53 | | } - | |_____^ help: try this: `if let &(v, 1) = tup { $ crate :: io :: _print ( format_args_nl ! ( $ ( $ arg ) * ) ) ; } else { $ crate :: io :: _print ( format_args_nl ! ( $ ( $ arg ) * ) ) ; }` + | |_____^ help: try this: `if let &(v, 1) = tup { println!("{}", v) } else { println!("none") }` error: you don't need to add `&` to all patterns --> $DIR/matches.rs:50:5 diff --git a/tests/ui/methods.stderr b/tests/ui/methods.stderr index 307814824ea..896b15481bb 100644 --- a/tests/ui/methods.stderr +++ b/tests/ui/methods.stderr @@ -297,7 +297,7 @@ error: use of `unwrap_or` followed by a function call --> $DIR/methods.rs:339:14 | 339 | with_vec.unwrap_or(vec![]); - | ^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| < [ _ ] > :: into_vec ( box [ $ ( $ x ) , * ] ))` + | ^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| vec![])` error: use of `unwrap_or` followed by a function call --> $DIR/methods.rs:344:21 diff --git a/tests/ui/needless_bool.rs b/tests/ui/needless_bool.rs index a9a2e3709f1..98c2e0767d6 100644 --- a/tests/ui/needless_bool.rs +++ b/tests/ui/needless_bool.rs @@ -8,9 +8,31 @@ // except according to those terms. +#![warn(clippy::needless_bool)] +use std::cell::Cell; -#![warn(clippy::needless_bool)] +macro_rules! bool_comparison_trigger { + ($($i:ident: $def:expr, $stb:expr );+ $(;)*) => ( + + #[derive(Clone)] + pub struct Trigger { + $($i: (Cell<bool>, bool, bool)),+ + } + + #[allow(dead_code)] + impl Trigger { + pub fn trigger(&self, key: &str) -> bool { + $( + if let stringify!($i) = key { + return self.$i.1 && self.$i.2 == $def; + } + )+ + false + } + } + ) +} #[allow(clippy::if_same_then_else)] fn main() { @@ -28,6 +50,9 @@ fn main() { bool_ret5(x, x); bool_ret4(x); bool_ret6(x, x); + needless_bool(x); + needless_bool2(x); + needless_bool3(x); } #[allow(clippy::if_same_then_else, clippy::needless_return)] @@ -59,3 +84,23 @@ fn bool_ret4(x: bool) -> bool { fn bool_ret6(x: bool, y: bool) -> bool { if x && y { return false } else { return true }; } + +fn needless_bool(x: bool) { + if x == true { }; +} + +fn needless_bool2(x: bool) { + if x == false { }; +} + +fn needless_bool3(x: bool) { + + bool_comparison_trigger! { + test_one: false, false; + test_three: false, false; + test_two: true, true; + } + + if x == true { }; + if x == false { }; +} diff --git a/tests/ui/needless_bool.stderr b/tests/ui/needless_bool.stderr index 13af6fc3564..638a3f56f0f 100644 --- a/tests/ui/needless_bool.stderr +++ b/tests/ui/needless_bool.stderr @@ -1,70 +1,96 @@ error: this if-then-else expression will always return true - --> $DIR/needless_bool.rs:19:5 + --> $DIR/needless_bool.rs:41:5 | -19 | if x { true } else { true }; +41 | if x { true } else { true }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `-D clippy::needless-bool` implied by `-D warnings` error: this if-then-else expression will always return false - --> $DIR/needless_bool.rs:20:5 + --> $DIR/needless_bool.rs:42:5 | -20 | if x { false } else { false }; +42 | if x { false } else { false }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: this if-then-else expression returns a bool literal - --> $DIR/needless_bool.rs:21:5 + --> $DIR/needless_bool.rs:43:5 | -21 | if x { true } else { false }; +43 | if x { true } else { false }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `x` error: this if-then-else expression returns a bool literal - --> $DIR/needless_bool.rs:22:5 + --> $DIR/needless_bool.rs:44:5 | -22 | if x { false } else { true }; +44 | if x { false } else { true }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `!x` error: this if-then-else expression returns a bool literal - --> $DIR/needless_bool.rs:23:5 + --> $DIR/needless_bool.rs:45:5 | -23 | if x && y { false } else { true }; +45 | if x && y { false } else { true }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `!(x && y)` error: this if-then-else expression will always return true - --> $DIR/needless_bool.rs:35:5 + --> $DIR/needless_bool.rs:60:5 | -35 | if x { return true } else { return true }; +60 | if x { return true } else { return true }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: this if-then-else expression will always return false - --> $DIR/needless_bool.rs:40:5 + --> $DIR/needless_bool.rs:65:5 | -40 | if x { return false } else { return false }; +65 | if x { return false } else { return false }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: this if-then-else expression returns a bool literal - --> $DIR/needless_bool.rs:45:5 + --> $DIR/needless_bool.rs:70:5 | -45 | if x { return true } else { return false }; +70 | if x { return true } else { return false }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `return x` error: this if-then-else expression returns a bool literal - --> $DIR/needless_bool.rs:50:5 + --> $DIR/needless_bool.rs:75:5 | -50 | if x && y { return true } else { return false }; +75 | if x && y { return true } else { return false }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `return x && y` error: this if-then-else expression returns a bool literal - --> $DIR/needless_bool.rs:55:5 + --> $DIR/needless_bool.rs:80:5 | -55 | if x { return false } else { return true }; +80 | if x { return false } else { return true }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `return !x` error: this if-then-else expression returns a bool literal - --> $DIR/needless_bool.rs:60:5 + --> $DIR/needless_bool.rs:85:5 | -60 | if x && y { return false } else { return true }; +85 | if x && y { return false } else { return true }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `return !(x && y)` -error: aborting due to 11 previous errors +error: equality checks against true are unnecessary + --> $DIR/needless_bool.rs:89:7 + | +89 | if x == true { }; + | ^^^^^^^^^^ help: try simplifying it as shown: `x` + | + = note: `-D clippy::bool-comparison` implied by `-D warnings` + +error: equality checks against false can be replaced by a negation + --> $DIR/needless_bool.rs:93:7 + | +93 | if x == false { }; + | ^^^^^^^^^^^ help: try simplifying it as shown: `!x` + +error: equality checks against true are unnecessary + --> $DIR/needless_bool.rs:104:8 + | +104 | if x == true { }; + | ^^^^^^^^^ help: try simplifying it as shown: `x` + +error: equality checks against false can be replaced by a negation + --> $DIR/needless_bool.rs:105:8 + | +105 | if x == false { }; + | ^^^^^^^^^^ help: try simplifying it as shown: `!x` + +error: aborting due to 15 previous errors diff --git a/tests/ui/no_effect.rs b/tests/ui/no_effect.rs index 32e1ccb7bee..bee3aeb6f7f 100644 --- a/tests/ui/no_effect.rs +++ b/tests/ui/no_effect.rs @@ -13,7 +13,7 @@ #![feature(box_syntax)] -#![warn(clippy::no_effect, clippy::unnecessary_operation)] +#![warn(clippy::no_effect)] #![allow(dead_code)] #![allow(path_statements)] #![allow(clippy::deref_addrof)] @@ -105,33 +105,4 @@ fn main() { DropTuple(0); DropEnum::Tuple(0); DropEnum::Struct { field: 0 }; - - Tuple(get_number()); - Struct { field: get_number() }; - Struct { ..get_struct() }; - Enum::Tuple(get_number()); - Enum::Struct { field: get_number() }; - 5 + get_number(); - *&get_number(); - &get_number(); - (5, 6, get_number()); - box get_number(); - get_number()..; - ..get_number(); - 5..get_number(); - [42, get_number()]; - [42, 55][get_number() as usize]; - (42, get_number()).1; - [get_number(); 55]; - [42; 55][get_number() as usize]; - {get_number()}; - FooString { s: String::from("blah"), }; - - // Do not warn - DropTuple(get_number()); - DropStruct { field: get_number() }; - DropStruct { field: get_number() }; - DropStruct { ..get_drop_struct() }; - DropEnum::Tuple(get_number()); - DropEnum::Struct { field: get_number() }; } diff --git a/tests/ui/no_effect.stderr b/tests/ui/no_effect.stderr index eca47d7546e..7f012aa2ed4 100644 --- a/tests/ui/no_effect.stderr +++ b/tests/ui/no_effect.stderr @@ -150,127 +150,5 @@ error: statement with no effect 98 | FooString { s: s }; | ^^^^^^^^^^^^^^^^^^^ -error: statement can be reduced - --> $DIR/no_effect.rs:109:5 - | -109 | Tuple(get_number()); - | ^^^^^^^^^^^^^^^^^^^^ help: replace it with: `get_number();` - | - = note: `-D clippy::unnecessary-operation` implied by `-D warnings` - -error: statement can be reduced - --> $DIR/no_effect.rs:110:5 - | -110 | Struct { field: get_number() }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `get_number();` - -error: statement can be reduced - --> $DIR/no_effect.rs:111:5 - | -111 | Struct { ..get_struct() }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `get_struct();` - -error: statement can be reduced - --> $DIR/no_effect.rs:112:5 - | -112 | Enum::Tuple(get_number()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `get_number();` - -error: statement can be reduced - --> $DIR/no_effect.rs:113:5 - | -113 | Enum::Struct { field: get_number() }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `get_number();` - -error: statement can be reduced - --> $DIR/no_effect.rs:114:5 - | -114 | 5 + get_number(); - | ^^^^^^^^^^^^^^^^^ help: replace it with: `5;get_number();` - -error: statement can be reduced - --> $DIR/no_effect.rs:115:5 - | -115 | *&get_number(); - | ^^^^^^^^^^^^^^^ help: replace it with: `get_number();` - -error: statement can be reduced - --> $DIR/no_effect.rs:116:5 - | -116 | &get_number(); - | ^^^^^^^^^^^^^^ help: replace it with: `get_number();` - -error: statement can be reduced - --> $DIR/no_effect.rs:117:5 - | -117 | (5, 6, get_number()); - | ^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `5;6;get_number();` - -error: statement can be reduced - --> $DIR/no_effect.rs:118:5 - | -118 | box get_number(); - | ^^^^^^^^^^^^^^^^^ help: replace it with: `get_number();` - -error: statement can be reduced - --> $DIR/no_effect.rs:119:5 - | -119 | get_number()..; - | ^^^^^^^^^^^^^^^ help: replace it with: `get_number();` - -error: statement can be reduced - --> $DIR/no_effect.rs:120:5 - | -120 | ..get_number(); - | ^^^^^^^^^^^^^^^ help: replace it with: `get_number();` - -error: statement can be reduced - --> $DIR/no_effect.rs:121:5 - | -121 | 5..get_number(); - | ^^^^^^^^^^^^^^^^ help: replace it with: `5;get_number();` - -error: statement can be reduced - --> $DIR/no_effect.rs:122:5 - | -122 | [42, get_number()]; - | ^^^^^^^^^^^^^^^^^^^ help: replace it with: `42;get_number();` - -error: statement can be reduced - --> $DIR/no_effect.rs:123:5 - | -123 | [42, 55][get_number() as usize]; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `[42, 55];get_number() as usize;` - -error: statement can be reduced - --> $DIR/no_effect.rs:124:5 - | -124 | (42, get_number()).1; - | ^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `42;get_number();` - -error: statement can be reduced - --> $DIR/no_effect.rs:125:5 - | -125 | [get_number(); 55]; - | ^^^^^^^^^^^^^^^^^^^ help: replace it with: `get_number();` - -error: statement can be reduced - --> $DIR/no_effect.rs:126:5 - | -126 | [42; 55][get_number() as usize]; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `[42; 55];get_number() as usize;` - -error: statement can be reduced - --> $DIR/no_effect.rs:127:5 - | -127 | {get_number()}; - | ^^^^^^^^^^^^^^^ help: replace it with: `get_number();` - -error: statement can be reduced - --> $DIR/no_effect.rs:128:5 - | -128 | FooString { s: String::from("blah"), }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `String::from("blah");` - -error: aborting due to 45 previous errors +error: aborting due to 25 previous errors diff --git a/tests/ui/single_match.rs b/tests/ui/single_match.rs index 5c7cae249b4..dca68e179e7 100644 --- a/tests/ui/single_match.rs +++ b/tests/ui/single_match.rs @@ -23,6 +23,15 @@ fn single_match(){ _ => () }; + let x = Some(1u8); + match x { + // Note the missing block braces. + // We suggest `if let Some(y) = x { .. }` because the macro + // is expanded before we can do anything. + Some(y) => println!("{:?}", y), + _ => () + } + let z = (1u8,1u8); match z { (2...3, 7...9) => dummy(), diff --git a/tests/ui/single_match.stderr b/tests/ui/single_match.stderr index 74448391ca5..df614ad201d 100644 --- a/tests/ui/single_match.stderr +++ b/tests/ui/single_match.stderr @@ -12,38 +12,50 @@ error: you seem to be trying to use match for destructuring a single pattern. Co error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let` --> $DIR/single_match.rs:27:5 | -27 | / match z { -28 | | (2...3, 7...9) => dummy(), -29 | | _ => {} -30 | | }; +27 | / match x { +28 | | // Note the missing block braces. +29 | | // We suggest `if let Some(y) = x { .. }` because the macro +30 | | // is expanded before we can do anything. +31 | | Some(y) => println!("{:?}", y), +32 | | _ => () +33 | | } + | |_____^ help: try this: `if let Some(y) = x { println!("{:?}", y) }` + +error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let` + --> $DIR/single_match.rs:36:5 + | +36 | / match z { +37 | | (2...3, 7...9) => dummy(), +38 | | _ => {} +39 | | }; | |_____^ help: try this: `if let (2...3, 7...9) = z { dummy() }` error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let` - --> $DIR/single_match.rs:53:5 + --> $DIR/single_match.rs:62:5 | -53 | / match x { -54 | | Some(y) => dummy(), -55 | | None => () -56 | | }; +62 | / match x { +63 | | Some(y) => dummy(), +64 | | None => () +65 | | }; | |_____^ help: try this: `if let Some(y) = x { dummy() }` error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let` - --> $DIR/single_match.rs:58:5 + --> $DIR/single_match.rs:67:5 | -58 | / match y { -59 | | Ok(y) => dummy(), -60 | | Err(..) => () -61 | | }; +67 | / match y { +68 | | Ok(y) => dummy(), +69 | | Err(..) => () +70 | | }; | |_____^ help: try this: `if let Ok(y) = y { dummy() }` error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let` - --> $DIR/single_match.rs:65:5 + --> $DIR/single_match.rs:74:5 | -65 | / match c { -66 | | Cow::Borrowed(..) => dummy(), -67 | | Cow::Owned(..) => (), -68 | | }; +74 | / match c { +75 | | Cow::Borrowed(..) => dummy(), +76 | | Cow::Owned(..) => (), +77 | | }; | |_____^ help: try this: `if let Cow::Borrowed(..) = c { dummy() }` -error: aborting due to 5 previous errors +error: aborting due to 6 previous errors diff --git a/tests/ui/strings.rs b/tests/ui/strings.rs index 7bc4e6515f6..d2062b356dc 100644 --- a/tests/ui/strings.rs +++ b/tests/ui/strings.rs @@ -10,10 +10,10 @@ - #[warn(clippy::string_add)] #[allow(clippy::string_add_assign)] -fn add_only() { // ignores assignment distinction +fn add_only() { + // ignores assignment distinction let mut x = "".to_owned(); for _ in 1..3 { @@ -59,12 +59,17 @@ fn both() { fn str_lit_as_bytes() { let bs = "hello there".as_bytes(); + let bs = r###"raw string with three ### in it and some " ""###.as_bytes(); + // no warning, because this cannot be written as a byte string literal: let ubs = "☃".as_bytes(); let strify = stringify!(foobar).as_bytes(); + + let includestr = include_str!("entry.rs").as_bytes(); } +#[allow(clippy::assign_op_pattern)] fn main() { add_only(); add_assign_only(); @@ -72,6 +77,6 @@ fn main() { // the add is only caught for `String` let mut x = 1; - ; x = x + 1; + x = x + 1; assert_eq!(2, x); } diff --git a/tests/ui/strings.stderr b/tests/ui/strings.stderr index bcdf91568d2..21115d8e97e 100644 --- a/tests/ui/strings.stderr +++ b/tests/ui/strings.stderr @@ -61,16 +61,16 @@ error: calling `as_bytes()` on a string literal = note: `-D clippy::string-lit-as-bytes` implied by `-D warnings` error: calling `as_bytes()` on a string literal - --> $DIR/strings.rs:65:18 + --> $DIR/strings.rs:62:14 | -65 | let strify = stringify!(foobar).as_bytes(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using a byte string literal instead: `bstringify!(foobar)` +62 | let bs = r###"raw string with three ### in it and some " ""###.as_bytes(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using a byte string literal instead: `br###"raw string with three ### in it and some " ""###` -error: manual implementation of an assign operation - --> $DIR/strings.rs:75:7 +error: calling `as_bytes()` on `include_str!(..)` + --> $DIR/strings.rs:69:22 | -75 | ; x = x + 1; - | ^^^^^^^^^ help: replace it with: `x += 1` +69 | let includestr = include_str!("entry.rs").as_bytes(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `include_bytes!(..)` instead: `include_bytes!("entry.rs")` error: aborting due to 11 previous errors diff --git a/tests/ui/unnecessary_operation.rs b/tests/ui/unnecessary_operation.rs new file mode 100644 index 00000000000..de44047c867 --- /dev/null +++ b/tests/ui/unnecessary_operation.rs @@ -0,0 +1,76 @@ +// Copyright 2014-2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution. +// +// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or +// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license +// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![feature(box_syntax)] +#![allow(clippy::deref_addrof)] +#![warn(clippy::unnecessary_operation)] + +struct Tuple(i32); +struct Struct { + field: i32 +} +enum Enum { + Tuple(i32), + Struct { field: i32 }, +} +struct DropStruct { + field: i32 +} +impl Drop for DropStruct { + fn drop(&mut self) {} +} +struct DropTuple(i32); +impl Drop for DropTuple { + fn drop(&mut self) {} +} +enum DropEnum { + Tuple(i32), + Struct { field: i32 }, +} +impl Drop for DropEnum { + fn drop(&mut self) {} +} +struct FooString { + s: String, +} + +fn get_number() -> i32 { 0 } +fn get_struct() -> Struct { Struct { field: 0 } } +fn get_drop_struct() -> DropStruct { DropStruct { field: 0 } } + +fn main() { + Tuple(get_number()); + Struct { field: get_number() }; + Struct { ..get_struct() }; + Enum::Tuple(get_number()); + Enum::Struct { field: get_number() }; + 5 + get_number(); + *&get_number(); + &get_number(); + (5, 6, get_number()); + box get_number(); + get_number()..; + ..get_number(); + 5..get_number(); + [42, get_number()]; + [42, 55][get_number() as usize]; + (42, get_number()).1; + [get_number(); 55]; + [42; 55][get_number() as usize]; + {get_number()}; + FooString { s: String::from("blah"), }; + + // Do not warn + DropTuple(get_number()); + DropStruct { field: get_number() }; + DropStruct { field: get_number() }; + DropStruct { ..get_drop_struct() }; + DropEnum::Tuple(get_number()); + DropEnum::Struct { field: get_number() }; +} diff --git a/tests/ui/unnecessary_operation.stderr b/tests/ui/unnecessary_operation.stderr new file mode 100644 index 00000000000..8e5417eb13e --- /dev/null +++ b/tests/ui/unnecessary_operation.stderr @@ -0,0 +1,124 @@ +error: statement can be reduced + --> $DIR/unnecessary_operation.rs:48:5 + | +48 | Tuple(get_number()); + | ^^^^^^^^^^^^^^^^^^^^ help: replace it with: `get_number();` + | + = note: `-D clippy::unnecessary-operation` implied by `-D warnings` + +error: statement can be reduced + --> $DIR/unnecessary_operation.rs:49:5 + | +49 | Struct { field: get_number() }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `get_number();` + +error: statement can be reduced + --> $DIR/unnecessary_operation.rs:50:5 + | +50 | Struct { ..get_struct() }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `get_struct();` + +error: statement can be reduced + --> $DIR/unnecessary_operation.rs:51:5 + | +51 | Enum::Tuple(get_number()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `get_number();` + +error: statement can be reduced + --> $DIR/unnecessary_operation.rs:52:5 + | +52 | Enum::Struct { field: get_number() }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `get_number();` + +error: statement can be reduced + --> $DIR/unnecessary_operation.rs:53:5 + | +53 | 5 + get_number(); + | ^^^^^^^^^^^^^^^^^ help: replace it with: `5;get_number();` + +error: statement can be reduced + --> $DIR/unnecessary_operation.rs:54:5 + | +54 | *&get_number(); + | ^^^^^^^^^^^^^^^ help: replace it with: `get_number();` + +error: statement can be reduced + --> $DIR/unnecessary_operation.rs:55:5 + | +55 | &get_number(); + | ^^^^^^^^^^^^^^ help: replace it with: `get_number();` + +error: statement can be reduced + --> $DIR/unnecessary_operation.rs:56:5 + | +56 | (5, 6, get_number()); + | ^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `5;6;get_number();` + +error: statement can be reduced + --> $DIR/unnecessary_operation.rs:57:5 + | +57 | box get_number(); + | ^^^^^^^^^^^^^^^^^ help: replace it with: `get_number();` + +error: statement can be reduced + --> $DIR/unnecessary_operation.rs:58:5 + | +58 | get_number()..; + | ^^^^^^^^^^^^^^^ help: replace it with: `get_number();` + +error: statement can be reduced + --> $DIR/unnecessary_operation.rs:59:5 + | +59 | ..get_number(); + | ^^^^^^^^^^^^^^^ help: replace it with: `get_number();` + +error: statement can be reduced + --> $DIR/unnecessary_operation.rs:60:5 + | +60 | 5..get_number(); + | ^^^^^^^^^^^^^^^^ help: replace it with: `5;get_number();` + +error: statement can be reduced + --> $DIR/unnecessary_operation.rs:61:5 + | +61 | [42, get_number()]; + | ^^^^^^^^^^^^^^^^^^^ help: replace it with: `42;get_number();` + +error: statement can be reduced + --> $DIR/unnecessary_operation.rs:62:5 + | +62 | [42, 55][get_number() as usize]; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `[42, 55];get_number() as usize;` + +error: statement can be reduced + --> $DIR/unnecessary_operation.rs:63:5 + | +63 | (42, get_number()).1; + | ^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `42;get_number();` + +error: statement can be reduced + --> $DIR/unnecessary_operation.rs:64:5 + | +64 | [get_number(); 55]; + | ^^^^^^^^^^^^^^^^^^^ help: replace it with: `get_number();` + +error: statement can be reduced + --> $DIR/unnecessary_operation.rs:65:5 + | +65 | [42; 55][get_number() as usize]; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `[42; 55];get_number() as usize;` + +error: statement can be reduced + --> $DIR/unnecessary_operation.rs:66:5 + | +66 | {get_number()}; + | ^^^^^^^^^^^^^^^ help: replace it with: `get_number();` + +error: statement can be reduced + --> $DIR/unnecessary_operation.rs:67:5 + | +67 | FooString { s: String::from("blah"), }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `String::from("blah");` + +error: aborting due to 20 previous errors + |