From c209fc9349ff750dc983ecfe23d8e0bb74f002df Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Fri, 5 Oct 2018 09:06:05 -0700 Subject: [PATCH 01/31] Fix string_lit_as_bytes lint for macros Prior to this change, string_lit_as_bytes would trigger for constructs like `include_str!("filename").as_bytes()` and would recommend fixing it by rewriting as `binclude_str!("filename")`. This change updates the lint to act as an EarlyLintPass lint. It then differentiates between string literals and macros that have bytes yielding alternatives. Closes #3205 --- clippy_lints/src/strings.rs | 39 +++++++++++++++++++++++++++++-------- tests/ui/strings.rs | 8 +++++--- tests/ui/strings.stderr | 12 ------------ 3 files changed, 36 insertions(+), 23 deletions(-) diff --git a/clippy_lints/src/strings.rs b/clippy_lints/src/strings.rs index f4798842205a..9b6478fb9cd6 100644 --- a/clippy_lints/src/strings.rs +++ b/clippy_lints/src/strings.rs @@ -92,7 +92,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 +139,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, } @@ -162,7 +171,21 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for StringLitAsBytes { 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) { + let callsite = snippet(cx, args[0].span.source_callsite(), ""); + let expanded = 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/tests/ui/strings.rs b/tests/ui/strings.rs index 7bc4e6515f65..6693776a9617 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 { @@ -63,6 +63,8 @@ fn str_lit_as_bytes() { let ubs = "☃".as_bytes(); let strify = stringify!(foobar).as_bytes(); + + let includestr = include_str!("entry.rs").as_bytes(); } fn main() { @@ -72,6 +74,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 bcdf91568d27..8a93733732ea 100644 --- a/tests/ui/strings.stderr +++ b/tests/ui/strings.stderr @@ -60,17 +60,5 @@ 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 - | -65 | let strify = stringify!(foobar).as_bytes(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using a byte string literal instead: `bstringify!(foobar)` - -error: manual implementation of an assign operation - --> $DIR/strings.rs:75:7 - | -75 | ; x = x + 1; - | ^^^^^^^^^ help: replace it with: `x += 1` - error: aborting due to 11 previous errors From f9020bb2dded44e97fd997ab71ab4edf6d88033b Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Wed, 24 Oct 2018 11:49:39 -0400 Subject: [PATCH 02/31] fix: extra semicolon, only create callsite once --- clippy_lints/src/strings.rs | 2 +- tests/ui/strings.rs | 5 ++++- tests/ui/strings.stderr | 2 -- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/strings.rs b/clippy_lints/src/strings.rs index 9b6478fb9cd6..fe3d461ab43a 100644 --- a/clippy_lints/src/strings.rs +++ b/clippy_lints/src/strings.rs @@ -171,7 +171,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for StringLitAsBytes { if path.ident.name == "as_bytes" { if let ExprKind::Lit(ref lit) = args[0].node { if let LitKind::Str(ref lit_content, _) = lit.node { - let callsite = snippet(cx, args[0].span.source_callsite(), ""); + let callsite = snippet(cx, args[0].span.source_callsite(), r#""foo""#); let expanded = format!("\"{}\"", lit_content.as_str()); if callsite.starts_with("include_str!") { span_lint_and_sugg( diff --git a/tests/ui/strings.rs b/tests/ui/strings.rs index 6693776a9617..d2062b356dc0 100644 --- a/tests/ui/strings.rs +++ b/tests/ui/strings.rs @@ -59,6 +59,8 @@ 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(); @@ -67,6 +69,7 @@ fn str_lit_as_bytes() { let includestr = include_str!("entry.rs").as_bytes(); } +#[allow(clippy::assign_op_pattern)] fn main() { add_only(); add_assign_only(); @@ -74,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 8a93733732ea..2496270ba0d5 100644 --- a/tests/ui/strings.stderr +++ b/tests/ui/strings.stderr @@ -60,5 +60,3 @@ error: calling `as_bytes()` on a string literal | = note: `-D clippy::string-lit-as-bytes` implied by `-D warnings` -error: aborting due to 11 previous errors - From 19ac2e94c6cb12ae4f9fb410f165e2aa5309e124 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Fri, 26 Oct 2018 09:10:20 -0700 Subject: [PATCH 03/31] fix: correctly reconstruct raw strings --- clippy_lints/src/strings.rs | 12 ++++++++---- tests/ui/strings.stderr | 14 ++++++++++++++ 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/strings.rs b/clippy_lints/src/strings.rs index fe3d461ab43a..e07b1649a466 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}; @@ -164,15 +163,20 @@ 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 let LitKind::Str(ref lit_content, style) = lit.node { let callsite = snippet(cx, args[0].span.source_callsite(), r#""foo""#); - let expanded = format!("\"{}\"", lit_content.as_str()); + let expanded = if let StrStyle::Raw(n) = style { + let term = (0..n).map(|_| '#').collect::(); + 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, diff --git a/tests/ui/strings.stderr b/tests/ui/strings.stderr index 2496270ba0d5..21115d8e97ec 100644 --- a/tests/ui/strings.stderr +++ b/tests/ui/strings.stderr @@ -60,3 +60,17 @@ 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:62:14 + | +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: calling `as_bytes()` on `include_str!(..)` + --> $DIR/strings.rs:69:22 + | +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 + From d6ca12a70dad2c34b8bcb421bf5b6d9b79d06b2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Kr=C3=BCger?= Date: Fri, 26 Oct 2018 19:58:50 +0200 Subject: [PATCH 04/31] simplify ci base-tests --- ci/base-tests.sh | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/ci/base-tests.sh b/ci/base-tests.sh index 72a38ee5e586..6ff3c41607b7 100755 --- a/ci/base-tests.sh +++ b/ci/base-tests.sh @@ -24,21 +24,21 @@ 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 .. +CWD_OLD=`pwd` +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 ${CWD_OLD} +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 From a90084d587a4c75c9fd42773bd5395cc7e62f528 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Kr=C3=BCger?= Date: Fri, 26 Oct 2018 20:00:49 +0200 Subject: [PATCH 05/31] slightly simplify integration tests --- ci/base-tests.sh | 3 +-- ci/integration-tests.sh | 3 --- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/ci/base-tests.sh b/ci/base-tests.sh index 6ff3c41607b7..9b73263c24a7 100755 --- a/ci/base-tests.sh +++ b/ci/base-tests.sh @@ -29,12 +29,11 @@ CLIPPY="`pwd`/target/debug/cargo-clippy clippy" # run clippy on its own codebase... ${CLIPPY} --all-targets --all-features -- -D clippy::all -D clippy::internal # ... and some test directories -CWD_OLD=`pwd` 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 ${CWD_OLD} + cd - done diff --git a/ci/integration-tests.sh b/ci/integration-tests.sh index 9019a6830e63..75decab940eb 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 ;; From aa7bcb9074f3a7235e43d1da910d80248e53357d Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Sat, 27 Oct 2018 11:01:27 +0200 Subject: [PATCH 06/31] Don't expand macro in identity_conversion suggestion --- clippy_lints/src/identity_conversion.rs | 5 +++-- clippy_lints/src/utils/mod.rs | 6 ++++++ tests/ui/identity_conversion.rs | 1 + tests/ui/identity_conversion.stderr | 8 +++++++- 4 files changed, 17 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/identity_conversion.rs b/clippy_lints/src/identity_conversion.rs index e9761616696b..00ce58f00b0d 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, "").into_owned(); + let sugg = snippet_with_macro_callsite(cx, args[0].span, "").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/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 1a8db837f32b..5ff246630e03 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 { cx.sess().source_map().span_to_snippet(span).ok() diff --git a/tests/ui/identity_conversion.rs b/tests/ui/identity_conversion.rs index 9384c9eb206e..b5cb92c6d5a1 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 2ac741919317..15bef8b125eb 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 From af1548f58f2a9a356a7c122f2fba25c816492a91 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Sat, 27 Oct 2018 14:45:02 +0200 Subject: [PATCH 07/31] Don't expand macro in single_match suggestion --- clippy_lints/src/matches.rs | 3 +- clippy_lints/src/utils/mod.rs | 5 +++- tests/ui/matches.stderr | 2 +- tests/ui/single_match.rs | 9 ++++++ tests/ui/single_match.stderr | 52 +++++++++++++++++++++-------------- 5 files changed, 48 insertions(+), 23 deletions(-) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index e46615f4da2f..4a704c3d52ec 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/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 5ff246630e03..72a6bda26c36 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -406,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/tests/ui/matches.stderr b/tests/ui/matches.stderr index bed903faf1a1..b5f1f2ab0e73 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/single_match.rs b/tests/ui/single_match.rs index 5c7cae249b4f..dca68e179e74 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 74448391ca52..df614ad201d1 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 From 840e50e97f023c2d0bfeb0222d094fa407f12f2f Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Sat, 27 Oct 2018 15:37:56 +0200 Subject: [PATCH 08/31] Don't expand macro in or_fun_call suggestion --- clippy_lints/src/methods/mod.rs | 8 ++++---- tests/ui/methods.stderr | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 01f97264d0b5..8d0cd32e23bf 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; @@ -1062,9 +1062,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/tests/ui/methods.stderr b/tests/ui/methods.stderr index 307814824eaa..896b15481bb4 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 From 0d899562cd805bd4335d6ee8d88e2bf1f743f000 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Sun, 28 Oct 2018 08:11:18 +0100 Subject: [PATCH 09/31] Disable rust master toolchain build temporarily --- .travis.yml | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/.travis.yml b/.travis.yml index 09972a12f8d3..d3f8c116607b 100644 --- a/.travis.yml +++ b/.travis.yml @@ -75,13 +75,14 @@ matrix: - os: windows script: - - | - rm rust-toolchain - cargo install rustup-toolchain-install-master || echo "rustup-toolchain-install-master already installed" - RUSTC_HASH=$(git ls-remote https://github.com/rust-lang/rust.git master | awk '{print $1}') - travis_retry rustup-toolchain-install-master -f -n master $RUSTC_HASH - rustup default master - export LD_LIBRARY_PATH=$(rustc --print sysroot)/lib + # uncomment once https://github.com/rust-lang/rust/issues/55376 is fixed + # - | + # rm rust-toolchain + # cargo install rustup-toolchain-install-master || echo "rustup-toolchain-install-master already installed" + # RUSTC_HASH=$(git ls-remote https://github.com/rust-lang/rust.git master | awk '{print $1}') + # travis_retry rustup-toolchain-install-master -f -n master $RUSTC_HASH + # rustup default master + # export LD_LIBRARY_PATH=$(rustc --print sysroot)/lib - | if [ -z ${INTEGRATION} ]; then ./ci/base-tests.sh && sleep 5 From 061a48321c46c4050afcdadbc57f5869cae37eeb Mon Sep 17 00:00:00 2001 From: Michael Rutter Date: Sun, 28 Oct 2018 08:12:47 +0000 Subject: [PATCH 10/31] added downsides to "known problems" for get_unwrap lint --- clippy_lints/src/methods/mod.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 01f97264d0b5..8e3a75f2c7b5 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -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 syntax (`[]`) 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 eventually handled in a future refactor instead of using `.unwrap()` +/// or the Index syntax. /// /// **Example:** /// ```rust From 232a483331242a3c10097b0c182c80b1403b7a1e Mon Sep 17 00:00:00 2001 From: Michael Rutter Date: Sun, 28 Oct 2018 12:31:02 +0000 Subject: [PATCH 11/31] more consistent use of terminology; trait > syntax --- clippy_lints/src/methods/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 8e3a75f2c7b5..10e6644b70e1 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -569,13 +569,13 @@ declare_clippy_lint! { /// concise. /// /// **Known problems:** Not a replacement for error handling: Using either -/// `.unwrap()` or the Index syntax (`[]`) carries the risk of causing a `panic` +/// `.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 eventually handled in a future refactor instead of using `.unwrap()` -/// or the Index syntax. +/// is handled in a future refactor instead of using `.unwrap()` or the Index +/// trait. /// /// **Example:** /// ```rust From 6eb1f23555102cbd0619a79423a4d9bc56ee30b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Kr=C3=BCger?= Date: Sun, 28 Oct 2018 12:50:32 +0100 Subject: [PATCH 12/31] rustup: fix build with rustc 1.31.0-nightly (cae6efc37 2018-10-27) --- clippy_lints/src/redundant_clone.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index 8c8959159217..2ed877d13649 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, _ => {}, } From 7cfde9cfa95b0c466efe867e064d05fd8adea568 Mon Sep 17 00:00:00 2001 From: Giorgio Gambino Date: Sun, 28 Oct 2018 15:37:39 +0100 Subject: [PATCH 13/31] Fix #3335: bool_comparison triggers 3 times on same code --- clippy_lints/src/needless_bool.rs | 100 +++++++++++++++--------------- tests/ui/needless_bool.rs | 49 ++++++++++++++- tests/ui/needless_bool.stderr | 94 ++++++++++++++++++---------- 3 files changed, 158 insertions(+), 85 deletions(-) diff --git a/clippy_lints/src/needless_bool.rs b/clippy_lints/src/needless_bool.rs index f102b49d785e..3afccf9f984d 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,54 +133,56 @@ impl LintPass for BoolComparison { impl<'a, 'tcx> LateLintPass<'a, 'tcx> for BoolComparison { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, e: &'tcx Expr) { - 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)) { - (Bool(true), Other) => { - let hint = snippet(cx, right_side.span, "..").into_owned(); - span_lint_and_sugg( - cx, - BOOL_COMPARISON, - e.span, - "equality checks against true are unnecessary", - "try simplifying it as shown", - hint, - ); - }, - (Other, Bool(true)) => { - let hint = snippet(cx, left_side.span, "..").into_owned(); - span_lint_and_sugg( - cx, - BOOL_COMPARISON, - e.span, - "equality checks against true are unnecessary", - "try simplifying it as shown", - hint, - ); - }, - (Bool(false), Other) => { - let hint = Sugg::hir(cx, right_side, ".."); - span_lint_and_sugg( - cx, - BOOL_COMPARISON, - e.span, - "equality checks against false can be replaced by a negation", - "try simplifying it as shown", - (!hint).to_string(), - ); - }, - (Other, Bool(false)) => { - let hint = Sugg::hir(cx, left_side, ".."); - span_lint_and_sugg( - cx, - BOOL_COMPARISON, - e.span, - "equality checks against false can be replaced by a negation", - "try simplifying it as shown", - (!hint).to_string(), - ); - }, - _ => (), + if !in_macro(e.span) { + 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)) { + (Bool(true), Other) => { + let hint = snippet(cx, right_side.span, "..").into_owned(); + span_lint_and_sugg( + cx, + BOOL_COMPARISON, + e.span, + "equality checks against true are unnecessary", + "try simplifying it as shown", + hint, + ); + }, + (Other, Bool(true)) => { + let hint = snippet(cx, left_side.span, "..").into_owned(); + span_lint_and_sugg( + cx, + BOOL_COMPARISON, + e.span, + "equality checks against true are unnecessary", + "try simplifying it as shown", + hint, + ); + }, + (Bool(false), Other) => { + let hint = Sugg::hir(cx, right_side, ".."); + span_lint_and_sugg( + cx, + BOOL_COMPARISON, + e.span, + "equality checks against false can be replaced by a negation", + "try simplifying it as shown", + (!hint).to_string(), + ); + }, + (Other, Bool(false)) => { + let hint = Sugg::hir(cx, left_side, ".."); + span_lint_and_sugg( + cx, + BOOL_COMPARISON, + e.span, + "equality checks against false can be replaced by a negation", + "try simplifying it as shown", + (!hint).to_string(), + ); + }, + _ => (), + } } } } diff --git a/tests/ui/needless_bool.rs b/tests/ui/needless_bool.rs index a9a2e3709f1b..aca4ccabf0ee 100644 --- a/tests/ui/needless_bool.rs +++ b/tests/ui/needless_bool.rs @@ -8,10 +8,32 @@ // except according to those terms. - - #![warn(clippy::needless_bool)] +use std::cell::Cell; + +macro_rules! bool_comparison_trigger { + ($($i:ident: $def:expr, $stb:expr );+ $(;)*) => ( + + #[derive(Clone)] + pub struct Trigger { + $($i: (Cell, 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() { let x = true; @@ -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 { }; +} \ No newline at end of file diff --git a/tests/ui/needless_bool.stderr b/tests/ui/needless_bool.stderr index 13af6fc3564f..638a3f56f0f2 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 - | -23 | 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 - | -35 | if x { return true } else { return true }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - -error: this if-then-else expression will always return false - --> $DIR/needless_bool.rs:40:5 - | -40 | if x { return false } else { return false }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - error: this if-then-else expression returns a bool literal --> $DIR/needless_bool.rs:45:5 | -45 | if x { return true } else { return false }; +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:60:5 + | +60 | if x { return true } else { return true }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: this if-then-else expression will always return false + --> $DIR/needless_bool.rs:65:5 + | +65 | if x { return false } else { return false }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: this if-then-else expression returns a bool literal + --> $DIR/needless_bool.rs:70:5 + | +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 From 62f16803e8abb176279d3160f0287ad0f0575105 Mon Sep 17 00:00:00 2001 From: Giorgio Gambino Date: Sun, 28 Oct 2018 16:28:17 +0100 Subject: [PATCH 14/31] Fix #3335 rev1: bool_comparison triggers 3 times on same code --- tests/ui/needless_bool.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ui/needless_bool.rs b/tests/ui/needless_bool.rs index aca4ccabf0ee..98c2e0767d6a 100644 --- a/tests/ui/needless_bool.rs +++ b/tests/ui/needless_bool.rs @@ -103,4 +103,4 @@ fn needless_bool3(x: bool) { if x == true { }; if x == false { }; -} \ No newline at end of file +} From 349697531ffe4eef947d5755a6045232a8198bbf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Kr=C3=BCger?= Date: Sun, 28 Oct 2018 16:52:38 +0100 Subject: [PATCH 15/31] pin compiletest dependency to git version (12c980f47971b5ba6beb7cb2ffebf8b32f6766ea) while we are waiting for a new release --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 518c2caf6718..81ca6c468178 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 = { git = "https://github.com/laumann/compiletest-rs", rev = "12c980f47971b5ba6beb7cb2ffebf8b32f6766ea" } lazy_static = "1.0" serde_derive = "1.0" clippy-mini-macro-test = { version = "0.2", path = "mini-macro" } From 3f0161918871403b4e0547191a93f395b8bf5b35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Kr=C3=BCger?= Date: Sun, 28 Oct 2018 17:14:39 +0100 Subject: [PATCH 16/31] appveyor: use rustc nightly instead of master --- appveyor.yml | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/appveyor.yml b/appveyor.yml index fb0b326c713b..18f25e916f3f 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -20,11 +20,12 @@ install: - set PATH=%PATH%;C:\Users\appveyor\.cargo\bin - git ls-remote https://github.com/rust-lang/rust.git master | awk '{print $1}' >rustc-hash.txt - set /p RUSTC_HASH= Date: Sat, 27 Oct 2018 19:31:47 +0200 Subject: [PATCH 17/31] UI test cleanup: Extract unnecessary_operation tests --- tests/ui/no_effect.rs | 31 +------ tests/ui/no_effect.stderr | 124 +------------------------- tests/ui/unnecessary_operation.rs | 76 ++++++++++++++++ tests/ui/unnecessary_operation.stderr | 124 ++++++++++++++++++++++++++ 4 files changed, 202 insertions(+), 153 deletions(-) create mode 100644 tests/ui/unnecessary_operation.rs create mode 100644 tests/ui/unnecessary_operation.stderr diff --git a/tests/ui/no_effect.rs b/tests/ui/no_effect.rs index 32e1ccb7beea..bee3aeb6f7f2 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 eca47d7546e0..7f012aa2ed4b 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/unnecessary_operation.rs b/tests/ui/unnecessary_operation.rs new file mode 100644 index 000000000000..de44047c8671 --- /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 or the MIT license +// , 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 000000000000..8e5417eb13e3 --- /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 + From 18b122005fa297c9c39a1ef3fb6973e955ea43c4 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Sat, 27 Oct 2018 19:16:43 +0200 Subject: [PATCH 18/31] UI test cleanup: Extract explicit_counter_loop tests --- tests/ui/explicit_counter_loop.rs | 122 ++++++++++++++++++++++++++ tests/ui/explicit_counter_loop.stderr | 28 ++++++ tests/ui/for_loop.rs | 112 +---------------------- tests/ui/for_loop.stderr | 120 ++++++++++--------------- 4 files changed, 198 insertions(+), 184 deletions(-) create mode 100644 tests/ui/explicit_counter_loop.rs create mode 100644 tests/ui/explicit_counter_loop.stderr diff --git a/tests/ui/explicit_counter_loop.rs b/tests/ui/explicit_counter_loop.rs new file mode 100644 index 000000000000..eaed606b89e9 --- /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 or the MIT license +// , 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 000000000000..023f7f299a72 --- /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 f80270d9fe8f..eefb43172762 100644 --- a/tests/ui/for_loop.rs +++ b/tests/ui/for_loop.rs @@ -84,7 +84,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 +275,6 @@ fn main() { let _y = vec.iter().cloned().map(|x| out.push(x)).collect::>(); // 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; @@ -594,103 +584,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 695209de53f3..0318b6694e41 100644 --- a/tests/ui/for_loop.stderr +++ b/tests/ui/for_loop.stderr @@ -360,156 +360,130 @@ error: you are collect()ing an iterator and throwing away the result. Consider u | = 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:385:19 | -395 | for (_, v) in &m { +385 | 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() { +385 | 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:390:19 | -400 | for (_, v) in &*m { +390 | for (_, v) in &*m { | ^^^ help: use the corresponding method | -400 | for v in (*m).values() { +390 | 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:398:19 | -408 | for (_, v) in &mut m { +398 | for (_, v) in &mut m { | ^^^^^^ help: use the corresponding method | -408 | for v in m.values_mut() { +398 | 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:403:19 | -413 | for (_, v) in &mut *m { +403 | for (_, v) in &mut *m { | ^^^^^^^ help: use the corresponding method | -413 | for v in (*m).values_mut() { +403 | 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:409:24 | -419 | for (k, _value) in rm { +409 | for (k, _value) in rm { | ^^ help: use the corresponding method | -419 | for k in rm.keys() { +409 | 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:462:14 | -472 | for i in 0..src.len() { +462 | 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:467:14 + | +467 | 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:472:14 + | +472 | 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:477:14 | -477 | for i in 0..src.len() { - | ^^^^^^^^^^^^ help: try replacing the loop by: `dst[10..(src.len() + 10)].clone_from_slice(&src[..])` +477 | 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:482:14 | -482 | 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 - | -487 | 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 - | -492 | for i in 0..dst.len() { +482 | 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:495:14 | -505 | for i in 10..256 { +495 | 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]) { +495 | for i in dst[10..256].clone_from_slice(&src[(10 - 5)..(256 - 5)]) +496 | 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:507:14 | -517 | for i in 10..LOOP_OFFSET { +507 | 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:520:14 | -530 | for i in 0..src_vec.len() { +520 | 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:549:14 | -559 | for i in from..from + src.len() { +549 | 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:553:14 | -563 | for i in from..from + 3 { +553 | 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:560:14 | -570 | for i in 0..src.len() { +560 | 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 59 previous errors From 53edeacdc01bf9d9d434cc88c52e2ed587cfd118 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Kr=C3=BCger?= Date: Mon, 29 Oct 2018 09:52:49 +0100 Subject: [PATCH 19/31] dependencies: bump compiletest-rs from git to 0.3.16 --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 81ca6c468178..2293913f38e7 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 = { git = "https://github.com/laumann/compiletest-rs", rev = "12c980f47971b5ba6beb7cb2ffebf8b32f6766ea" } +compiletest_rs = "0.3.16" lazy_static = "1.0" serde_derive = "1.0" clippy-mini-macro-test = { version = "0.2", path = "mini-macro" } From be7656d9928b6e07fe19eed6938f6cf5316f2de0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Kr=C3=BCger?= Date: Mon, 29 Oct 2018 10:27:40 +0100 Subject: [PATCH 20/31] compiletest: clean rmeta data (from "cargo check") before running compiletest. Fixes #2896 Fixes #2139 --- tests/compile-test.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/compile-test.rs b/tests/compile-test.rs index c9d4f6589357..64360af641b5 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) -> Result { From 267d5d3433c08c6867fc212ed091a7b178d1c141 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Fri, 12 Oct 2018 08:09:04 +0200 Subject: [PATCH 21/31] Fix lint_without_lint_pass --- clippy_lints/src/lib.rs | 1 + clippy_lints/src/utils/internal_lints.rs | 16 ++++++++++------ tests/ui/lint_without_lint_pass.rs | 19 +++++++++++++++++++ tests/ui/lint_without_lint_pass.stderr | 21 +++++++++++++++++++++ 4 files changed, 51 insertions(+), 6 deletions(-) create mode 100644 tests/ui/lint_without_lint_pass.rs create mode 100644 tests/ui/lint_without_lint_pass.stderr diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index eaff87e78f8b..207dc40fa1d7 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/utils/internal_lints.rs b/clippy_lints/src/utils/internal_lints.rs index 3a0d056bbb5d..e89c2d28953a 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_qpath, 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}; @@ -161,7 +162,8 @@ 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 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 { @@ -203,19 +205,21 @@ 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 } diff --git a/tests/ui/lint_without_lint_pass.rs b/tests/ui/lint_without_lint_pass.rs new file mode 100644 index 000000000000..41e7fea1abe8 --- /dev/null +++ b/tests/ui/lint_without_lint_pass.rs @@ -0,0 +1,19 @@ +#![deny(clippy::internal)] + +#![feature(rustc_private)] + +#[macro_use] +extern crate rustc; + +#[macro_use] +extern crate clippy_lints; + +declare_clippy_lint! +{ + pub TEST_LINT, + correctness, + "" +} + +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 000000000000..48d511ce92e5 --- /dev/null +++ b/tests/ui/lint_without_lint_pass.stderr @@ -0,0 +1,21 @@ +error: the lint `TEST_LINT` is not added to any `LintPass` + --> $DIR/lint_without_lint_pass.rs:11:1 + | +11 | / declare_clippy_lint! +12 | | { +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 + From a7fc6799df27a6ca0ad0eabcd71f9fb30d4e10a2 Mon Sep 17 00:00:00 2001 From: flip1995 Date: Mon, 29 Oct 2018 20:37:47 +0100 Subject: [PATCH 22/31] Rewrite registered lint collection --- clippy_lints/src/utils/internal_lints.rs | 23 ++++++++++------------- clippy_lints/src/utils/paths.rs | 2 +- tests/ui/lint_without_lint_pass.rs | 17 +++++++++++++++-- 3 files changed, 26 insertions(+), 16 deletions(-) diff --git a/clippy_lints/src/utils/internal_lints.rs b/clippy_lints/src/utils/internal_lints.rs index e89c2d28953a..879157ec8a47 100644 --- a/clippy_lints/src/utils/internal_lints.rs +++ b/clippy_lints/src/utils/internal_lints.rs @@ -9,7 +9,7 @@ use crate::utils::{ - match_def_path, 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; @@ -161,16 +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 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); } } @@ -223,14 +228,6 @@ fn is_lint_ref_type<'tcx>(cx: &LateContext<'_, 'tcx>, ty: &Ty) -> bool { 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 - } -} - struct LintCollector<'a, 'tcx: 'a> { output: &'a mut FxHashSet, cx: &'a LateContext<'a, 'tcx>, diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 8941d3031562..107a8eea15d7 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/tests/ui/lint_without_lint_pass.rs b/tests/ui/lint_without_lint_pass.rs index 41e7fea1abe8..c7e11840a37c 100644 --- a/tests/ui/lint_without_lint_pass.rs +++ b/tests/ui/lint_without_lint_pass.rs @@ -4,16 +4,29 @@ #[macro_use] extern crate rustc; +use rustc::lint; #[macro_use] extern crate clippy_lints; -declare_clippy_lint! -{ +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() { } From 1e43c3bb9f929695514e8d4854e9471962d2dde4 Mon Sep 17 00:00:00 2001 From: flip1995 Date: Mon, 29 Oct 2018 20:54:21 +0100 Subject: [PATCH 23/31] Register MISTYPED_LITERAL_SUFFIXES lint --- clippy_lints/src/literal_representation.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/literal_representation.rs b/clippy_lints/src/literal_representation.rs index a123415cca9d..145faf0b6b65 100644 --- a/clippy_lints/src/literal_representation.rs +++ b/clippy_lints/src/literal_representation.rs @@ -329,7 +329,8 @@ impl LintPass for LiteralDigitGrouping { lint_array!( UNREADABLE_LITERAL, INCONSISTENT_DIGIT_GROUPING, - LARGE_DIGIT_GROUPS + LARGE_DIGIT_GROUPS, + MISTYPED_LITERAL_SUFFIXES, ) } } From 3d84ffb5eca1b1ca5f7fee0448c197c06247714a Mon Sep 17 00:00:00 2001 From: flip1995 Date: Mon, 29 Oct 2018 20:55:52 +0100 Subject: [PATCH 24/31] Update .stderr file --- tests/ui/lint_without_lint_pass.stderr | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/ui/lint_without_lint_pass.stderr b/tests/ui/lint_without_lint_pass.stderr index 48d511ce92e5..65d1283a6e34 100644 --- a/tests/ui/lint_without_lint_pass.stderr +++ b/tests/ui/lint_without_lint_pass.stderr @@ -1,8 +1,7 @@ error: the lint `TEST_LINT` is not added to any `LintPass` - --> $DIR/lint_without_lint_pass.rs:11:1 + --> $DIR/lint_without_lint_pass.rs:12:1 | -11 | / declare_clippy_lint! -12 | | { +12 | / declare_clippy_lint! { 13 | | pub TEST_LINT, 14 | | correctness, 15 | | "" From c0c1f1f7fa1cd2d9d65d3b2cd4b3943c62106328 Mon Sep 17 00:00:00 2001 From: Giorgio Gambino Date: Mon, 29 Oct 2018 22:23:45 +0100 Subject: [PATCH 25/31] Fix #3335 rev2: bool_comparison triggers 3 times on same code --- clippy_lints/src/needless_bool.rs | 101 +++++++++++++++--------------- 1 file changed, 51 insertions(+), 50 deletions(-) diff --git a/clippy_lints/src/needless_bool.rs b/clippy_lints/src/needless_bool.rs index 3afccf9f984d..e13f757adb9e 100644 --- a/clippy_lints/src/needless_bool.rs +++ b/clippy_lints/src/needless_bool.rs @@ -133,56 +133,57 @@ 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) { - 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)) { - (Bool(true), Other) => { - let hint = snippet(cx, right_side.span, "..").into_owned(); - span_lint_and_sugg( - cx, - BOOL_COMPARISON, - e.span, - "equality checks against true are unnecessary", - "try simplifying it as shown", - hint, - ); - }, - (Other, Bool(true)) => { - let hint = snippet(cx, left_side.span, "..").into_owned(); - span_lint_and_sugg( - cx, - BOOL_COMPARISON, - e.span, - "equality checks against true are unnecessary", - "try simplifying it as shown", - hint, - ); - }, - (Bool(false), Other) => { - let hint = Sugg::hir(cx, right_side, ".."); - span_lint_and_sugg( - cx, - BOOL_COMPARISON, - e.span, - "equality checks against false can be replaced by a negation", - "try simplifying it as shown", - (!hint).to_string(), - ); - }, - (Other, Bool(false)) => { - let hint = Sugg::hir(cx, left_side, ".."); - span_lint_and_sugg( - cx, - BOOL_COMPARISON, - e.span, - "equality checks against false can be replaced by a negation", - "try simplifying it as shown", - (!hint).to_string(), - ); - }, - _ => (), - } + 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)) { + (Bool(true), Other) => { + let hint = snippet(cx, right_side.span, "..").into_owned(); + span_lint_and_sugg( + cx, + BOOL_COMPARISON, + e.span, + "equality checks against true are unnecessary", + "try simplifying it as shown", + hint, + ); + }, + (Other, Bool(true)) => { + let hint = snippet(cx, left_side.span, "..").into_owned(); + span_lint_and_sugg( + cx, + BOOL_COMPARISON, + e.span, + "equality checks against true are unnecessary", + "try simplifying it as shown", + hint, + ); + }, + (Bool(false), Other) => { + let hint = Sugg::hir(cx, right_side, ".."); + span_lint_and_sugg( + cx, + BOOL_COMPARISON, + e.span, + "equality checks against false can be replaced by a negation", + "try simplifying it as shown", + (!hint).to_string(), + ); + }, + (Other, Bool(false)) => { + let hint = Sugg::hir(cx, left_side, ".."); + span_lint_and_sugg( + cx, + BOOL_COMPARISON, + e.span, + "equality checks against false can be replaced by a negation", + "try simplifying it as shown", + (!hint).to_string(), + ); + }, + _ => (), } } } From a06296f8363788f75b7f295463a893bfb5743cae Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Tue, 30 Oct 2018 04:06:37 +0000 Subject: [PATCH 26/31] Rustup to rustc 1.31.0-nightly (fb2446ad5 2018-10-30) --- clippy_lints/src/utils/mod.rs | 2 +- clippy_lints/src/utils/sugg.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 72a6bda26c36..9d11950dd733 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -392,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()) } diff --git a/clippy_lints/src/utils/sugg.rs b/clippy_lints/src/utils/sugg.rs index eb67838f1d18..90f48f0f83c7 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 { From b421f5ad4833e93025b9180bf8e296d1eb0a81d2 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Tue, 30 Oct 2018 21:25:34 +0100 Subject: [PATCH 27/31] UI test cleanup: Extract for_loop_over_x tests --- tests/ui/for_loop.rs | 60 --- tests/ui/for_loop.stderr | 424 ++++++++------------ tests/ui/for_loop_over_option_result.rs | 68 ++++ tests/ui/for_loop_over_option_result.stderr | 72 ++++ 4 files changed, 318 insertions(+), 306 deletions(-) create mode 100644 tests/ui/for_loop_over_option_result.rs create mode 100644 tests/ui/for_loop_over_option_result.stderr diff --git a/tests/ui/for_loop.rs b/tests/ui/for_loop.rs index eefb43172762..2a70149f2460 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); impl Unrelated { fn next(&self) -> std::slice::Iter { @@ -379,8 +321,6 @@ fn main() { } println!("index: {}", index); - for_loop_over_option_and_result(); - let m: HashMap = HashMap::new(); for (_, v) in &m { let _v = v; diff --git a/tests/ui/for_loop.stderr b/tests/ui/for_loop.stderr index 0318b6694e41..f70a6d3b32f2 100644 --- a/tests/ui/for_loop.stderr +++ b/tests/ui/for_loop.stderr @@ -1,489 +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 in &vec { +38 | for 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 in &vec { - | ^^^^^^ ^^^^ + | +47 | for 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 in &STATIC { - | ^^^^^^ ^^^^^^^ + | +52 | for 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 in &CONST { - | ^^^^^^ ^^^^^^ + | +56 | for 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, ) in vec.iter().enumerate() { - | ^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^ + | +60 | for (i, ) 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 in vec2.iter().take(vec.len()) { - | ^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +68 | for 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 in vec.iter().skip(5) { - | ^^^^^^ ^^^^^^^^^^^^^^^^^^ + | +72 | for 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 in vec.iter().take(MAX_LEN) { - | ^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^ + | +76 | for 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 in vec.iter().take(MAX_LEN + 1) { - | ^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +80 | for 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 in vec.iter().take(10).skip(5) { - | ^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +84 | for 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 in vec.iter().take(10 + 1).skip(5) { - | ^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +88 | for 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, ) in vec.iter().enumerate().skip(5) { - | ^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +92 | for (i, ) 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, ) in vec.iter().enumerate().take(10).skip(5) { - | ^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +96 | for (i, ) 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::>(); +216 | vec.iter().cloned().map(|x| out.push(x)).collect::>(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `-D clippy::unused-collect` implied by `-D warnings` error: you seem to want to iterate on a map's values - --> $DIR/for_loop.rs:385:19 + --> $DIR/for_loop.rs:325:19 | -385 | for (_, v) in &m { +325 | for (_, v) in &m { | ^^ | = note: `-D clippy::for-kv-map` implied by `-D warnings` help: use the corresponding method | -385 | 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:390:19 + --> $DIR/for_loop.rs:330:19 | -390 | for (_, v) in &*m { +330 | for (_, v) in &*m { | ^^^ help: use the corresponding method | -390 | 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:398:19 + --> $DIR/for_loop.rs:338:19 | -398 | for (_, v) in &mut m { +338 | for (_, v) in &mut m { | ^^^^^^ help: use the corresponding method | -398 | 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:403:19 + --> $DIR/for_loop.rs:343:19 | -403 | for (_, v) in &mut *m { +343 | for (_, v) in &mut *m { | ^^^^^^^ help: use the corresponding method | -403 | 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:409:24 + --> $DIR/for_loop.rs:349:24 | -409 | for (k, _value) in rm { +349 | for (k, _value) in rm { | ^^ help: use the corresponding method | -409 | 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:462:14 + --> $DIR/for_loop.rs:402:14 | -462 | 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:467:14 + --> $DIR/for_loop.rs:407:14 | -467 | 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:472:14 + --> $DIR/for_loop.rs:412:14 | -472 | 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:477:14 + --> $DIR/for_loop.rs:417:14 | -477 | 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:482:14 + --> $DIR/for_loop.rs:422:14 | -482 | 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:495:14 + --> $DIR/for_loop.rs:435:14 | -495 | for i in 10..256 { +435 | for i in 10..256 { | ^^^^^^^ help: try replacing the loop by | -495 | for i in dst[10..256].clone_from_slice(&src[(10 - 5)..(256 - 5)]) -496 | 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:507:14 + --> $DIR/for_loop.rs:447:14 | -507 | 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:520:14 + --> $DIR/for_loop.rs:460:14 | -520 | 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:549:14 + --> $DIR/for_loop.rs:489:14 | -549 | 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:553:14 + --> $DIR/for_loop.rs:493:14 | -553 | 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:560:14 + --> $DIR/for_loop.rs:500:14 | -560 | 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: aborting due to 59 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 000000000000..37fd4e6d0385 --- /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 or the MIT license +// , 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 000000000000..13ad5fff846d --- /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 + From 650eb099810095e7b2dc31ba4fda08f3ad7aaecc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Kr=C3=BCger?= Date: Wed, 31 Oct 2018 01:42:17 +0100 Subject: [PATCH 28/31] docs: use_self: hightlight the "should be" code sample as rust code as well. --- clippy_lints/src/use_self.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/use_self.rs b/clippy_lints/src/use_self.rs index a8b7e8206814..b997d76d0e77 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 { From 627ca6b57857de8967cb857e04283d92a28dd9ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Kr=C3=BCger?= Date: Tue, 30 Oct 2018 15:41:59 +0100 Subject: [PATCH 29/31] Revert "Disable rust master toolchain build temporarily" This reverts commit 0d899562cd805bd4335d6ee8d88e2bf1f743f000. --- .travis.yml | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/.travis.yml b/.travis.yml index d3f8c116607b..09972a12f8d3 100644 --- a/.travis.yml +++ b/.travis.yml @@ -75,14 +75,13 @@ matrix: - os: windows script: - # uncomment once https://github.com/rust-lang/rust/issues/55376 is fixed - # - | - # rm rust-toolchain - # cargo install rustup-toolchain-install-master || echo "rustup-toolchain-install-master already installed" - # RUSTC_HASH=$(git ls-remote https://github.com/rust-lang/rust.git master | awk '{print $1}') - # travis_retry rustup-toolchain-install-master -f -n master $RUSTC_HASH - # rustup default master - # export LD_LIBRARY_PATH=$(rustc --print sysroot)/lib + - | + rm rust-toolchain + cargo install rustup-toolchain-install-master || echo "rustup-toolchain-install-master already installed" + RUSTC_HASH=$(git ls-remote https://github.com/rust-lang/rust.git master | awk '{print $1}') + travis_retry rustup-toolchain-install-master -f -n master $RUSTC_HASH + rustup default master + export LD_LIBRARY_PATH=$(rustc --print sysroot)/lib - | if [ -z ${INTEGRATION} ]; then ./ci/base-tests.sh && sleep 5 From 9b0f767b43a8f67bc22355b72799358ee79063c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Kr=C3=BCger?= Date: Tue, 30 Oct 2018 15:42:15 +0100 Subject: [PATCH 30/31] Revert "appveyor: use rustc nightly instead of master" This reverts commit 3f0161918871403b4e0547191a93f395b8bf5b35. --- appveyor.yml | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/appveyor.yml b/appveyor.yml index 18f25e916f3f..fb0b326c713b 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -20,12 +20,11 @@ install: - set PATH=%PATH%;C:\Users\appveyor\.cargo\bin - git ls-remote https://github.com/rust-lang/rust.git master | awk '{print $1}' >rustc-hash.txt - set /p RUSTC_HASH= Date: Tue, 30 Oct 2018 15:58:35 +0100 Subject: [PATCH 31/31] Revert "travis: work around temporary test failure due to rustc crashing on hyper." This reverts commit 326270ad1221b54028f9d029881fa0b1fb742db9. --- .travis.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 09972a12f8d3..75b45c9db403 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