From b0d7aea946e88e87af136fb0fe5c4a7e87b6c16e Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Tue, 9 Oct 2018 19:25:03 -0700 Subject: [PATCH 01/50] Fixes 3289, cmp_owned wording and false positive --- clippy_lints/src/misc.rs | 26 ++++++++++++++++++++++---- tests/ui/cmp_owned.rs | 15 +++++++++++++++ tests/ui/cmp_owned.stderr | 14 ++++++++++---- 3 files changed, 47 insertions(+), 8 deletions(-) diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs index 4e6d5b72efc..4c6a9d6bd11 100644 --- a/clippy_lints/src/misc.rs +++ b/clippy_lints/src/misc.rs @@ -535,10 +535,29 @@ fn check_to_owned(cx: &LateContext<'_, '_>, expr: &Expr, other: &Expr) { return; } + let other_gets_derefed = match other.node { + ExprKind::Unary(UnDeref, _) => true, + _ => false, + }; + + let (lint_span, try_hint) = if deref_arg_impl_partial_eq_other { + // suggest deref on the left + (expr.span, format!("*{}", snip)) + } else if other_gets_derefed { + // suggest dropping the to_owned on the left and the deref on the right + let other_snippet = snippet(cx, other.span, "..").into_owned(); + let other_without_deref = other_snippet.trim_left_matches("*"); + + (expr.span.to(other.span), format!("{} == {}", snip.to_string(), other_without_deref)) + } else { + // suggest dropping the to_owned on the left + (expr.span, snip.to_string()) + }; + span_lint_and_then( cx, CMP_OWNED, - expr.span, + lint_span, "this creates an owned instance just for comparison", |db| { // this is as good as our recursion check can get, we can't prove that the @@ -554,15 +573,14 @@ fn check_to_owned(cx: &LateContext<'_, '_>, expr: &Expr, other: &Expr) { // we are implementing PartialEq, don't suggest not doing `to_owned`, otherwise // we go into // recursion - db.span_label(expr.span, "try calling implementing the comparison without allocating"); + db.span_label(lint_span, "try implementing the comparison without allocating"); return; } } } } - let try_hint = if deref_arg_impl_partial_eq_other { format!("*{}", snip) } else { snip.to_string() }; db.span_suggestion_with_applicability( - expr.span, + lint_span, "try", try_hint, Applicability::MachineApplicable, // snippet diff --git a/tests/ui/cmp_owned.rs b/tests/ui/cmp_owned.rs index 031809f5df5..65351cd9b9d 100644 --- a/tests/ui/cmp_owned.rs +++ b/tests/ui/cmp_owned.rs @@ -35,6 +35,11 @@ fn main() { "abc".chars().filter(|c| c.to_owned() != 'X'); "abc".chars().filter(|c| *c != 'X'); + + let x = &Baz; + let y = &Baz; + + y.to_owned() == *x; } struct Foo; @@ -67,3 +72,13 @@ impl std::borrow::Borrow for Bar { &FOO } } + +#[derive(PartialEq)] +struct Baz; + +impl ToOwned for Baz { + type Owned = Baz; + fn to_owned(&self) -> Baz { + Baz + } +} diff --git a/tests/ui/cmp_owned.stderr b/tests/ui/cmp_owned.stderr index 5434b68de9f..2613d3b7500 100644 --- a/tests/ui/cmp_owned.stderr +++ b/tests/ui/cmp_owned.stderr @@ -37,10 +37,16 @@ error: this creates an owned instance just for comparison | ^^^^^^^^^^^^ help: try: `*c` error: this creates an owned instance just for comparison - --> $DIR/cmp_owned.rs:44:9 + --> $DIR/cmp_owned.rs:42:5 | -44 | self.to_owned() == *other - | ^^^^^^^^^^^^^^^ try calling implementing the comparison without allocating +42 | y.to_owned() == *x; + | ^^^^^^^^^^^^^^^^^^ help: try: `y == x` -error: aborting due to 7 previous errors +error: this creates an owned instance just for comparison + --> $DIR/cmp_owned.rs:49:9 + | +49 | self.to_owned() == *other + | ^^^^^^^^^^^^^^^^^^^^^^^^^ try implementing the comparison without allocating + +error: aborting due to 8 previous errors From 88ee209a1d2596efa1582cb7f993aca4308bf1c7 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Tue, 9 Oct 2018 20:01:12 -0700 Subject: [PATCH 02/50] Corrected single-character string constant used as pattern found in dogfood test --- clippy_lints/src/misc.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs index 4c6a9d6bd11..5f8480d8282 100644 --- a/clippy_lints/src/misc.rs +++ b/clippy_lints/src/misc.rs @@ -546,7 +546,7 @@ fn check_to_owned(cx: &LateContext<'_, '_>, expr: &Expr, other: &Expr) { } else if other_gets_derefed { // suggest dropping the to_owned on the left and the deref on the right let other_snippet = snippet(cx, other.span, "..").into_owned(); - let other_without_deref = other_snippet.trim_left_matches("*"); + let other_without_deref = other_snippet.trim_left_matches('*'); (expr.span.to(other.span), format!("{} == {}", snip.to_string(), other_without_deref)) } else { From d41615548e47f57c60ac8aed5d47a74dba048c13 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Wed, 10 Oct 2018 04:51:06 -0700 Subject: [PATCH 03/50] cmp_owned add test for multiple dereference --- tests/ui/cmp_owned.rs | 5 +++++ tests/ui/cmp_owned.stderr | 12 +++++++++--- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/tests/ui/cmp_owned.rs b/tests/ui/cmp_owned.rs index 65351cd9b9d..dc0880e7089 100644 --- a/tests/ui/cmp_owned.rs +++ b/tests/ui/cmp_owned.rs @@ -40,6 +40,11 @@ fn main() { let y = &Baz; y.to_owned() == *x; + + let x = &&Baz; + let y = &Baz; + + y.to_owned() == **x; } struct Foo; diff --git a/tests/ui/cmp_owned.stderr b/tests/ui/cmp_owned.stderr index 2613d3b7500..0982467aeee 100644 --- a/tests/ui/cmp_owned.stderr +++ b/tests/ui/cmp_owned.stderr @@ -43,10 +43,16 @@ error: this creates an owned instance just for comparison | ^^^^^^^^^^^^^^^^^^ help: try: `y == x` error: this creates an owned instance just for comparison - --> $DIR/cmp_owned.rs:49:9 + --> $DIR/cmp_owned.rs:47:5 | -49 | self.to_owned() == *other +47 | y.to_owned() == **x; + | ^^^^^^^^^^^^^^^^^^^ help: try: `y == x` + +error: this creates an owned instance just for comparison + --> $DIR/cmp_owned.rs:54:9 + | +54 | self.to_owned() == *other | ^^^^^^^^^^^^^^^^^^^^^^^^^ try implementing the comparison without allocating -error: aborting due to 8 previous errors +error: aborting due to 9 previous errors From 0b65462ca52599162949df162239ce55de96b4b3 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Thu, 11 Oct 2018 05:03:02 -0700 Subject: [PATCH 04/50] cmp_owned current suggestion for multiple deref --- clippy_lints/src/misc.rs | 2 +- tests/ui/cmp_owned.stderr | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs index 5f8480d8282..be863cd7bc8 100644 --- a/clippy_lints/src/misc.rs +++ b/clippy_lints/src/misc.rs @@ -546,7 +546,7 @@ fn check_to_owned(cx: &LateContext<'_, '_>, expr: &Expr, other: &Expr) { } else if other_gets_derefed { // suggest dropping the to_owned on the left and the deref on the right let other_snippet = snippet(cx, other.span, "..").into_owned(); - let other_without_deref = other_snippet.trim_left_matches('*'); + let other_without_deref = other_snippet.replacen('*', "", 1); (expr.span.to(other.span), format!("{} == {}", snip.to_string(), other_without_deref)) } else { diff --git a/tests/ui/cmp_owned.stderr b/tests/ui/cmp_owned.stderr index 0982467aeee..1db60be54d6 100644 --- a/tests/ui/cmp_owned.stderr +++ b/tests/ui/cmp_owned.stderr @@ -46,7 +46,7 @@ error: this creates an owned instance just for comparison --> $DIR/cmp_owned.rs:47:5 | 47 | y.to_owned() == **x; - | ^^^^^^^^^^^^^^^^^^^ help: try: `y == x` + | ^^^^^^^^^^^^^^^^^^^ help: try: `y == *x` error: this creates an owned instance just for comparison --> $DIR/cmp_owned.rs:54:9 From 9afd8abbe345095ee8755e2872a33cc7666e7790 Mon Sep 17 00:00:00 2001 From: Devon Hollowood Date: Thu, 11 Oct 2018 15:18:58 -0700 Subject: [PATCH 05/50] Fix `similar_names` warnings Most of these are just `#![allow]`ed, because they are things like using l vs r to differentiate left vs right. These would be made less clear by taking the advice of `similar_names` --- clippy_lints/src/double_comparison.rs | 1 + clippy_lints/src/enum_clike.rs | 12 ++++++------ clippy_lints/src/enum_variants.rs | 1 + clippy_lints/src/eq_op.rs | 1 + .../src/if_let_redundant_pattern_matching.rs | 1 + clippy_lints/src/transmute.rs | 1 + clippy_lints/src/utils/hir_utils.rs | 3 +++ clippy_lints/src/utils/inspector.rs | 2 ++ clippy_lints/src/utils/usage.rs | 1 + 9 files changed, 17 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/double_comparison.rs b/clippy_lints/src/double_comparison.rs index e151918c1fb..3710301c8ab 100644 --- a/clippy_lints/src/double_comparison.rs +++ b/clippy_lints/src/double_comparison.rs @@ -49,6 +49,7 @@ impl LintPass for DoubleComparisonPass { } impl<'a, 'tcx> DoubleComparisonPass { + #[allow(clippy::similar_names)] fn check_binop( &self, cx: &LateContext<'a, 'tcx>, diff --git a/clippy_lints/src/enum_clike.rs b/clippy_lints/src/enum_clike.rs index 313175aee84..315bc54cd17 100644 --- a/clippy_lints/src/enum_clike.rs +++ b/clippy_lints/src/enum_clike.rs @@ -63,16 +63,16 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnportableVariant { let variant = &var.node; if let Some(ref anon_const) = variant.disr_expr { let param_env = ty::ParamEnv::empty(); - let did = cx.tcx.hir.body_owner_def_id(anon_const.body); - let substs = Substs::identity_for_item(cx.tcx.global_tcx(), did); - let instance = ty::Instance::new(did, substs); - let cid = GlobalId { + let def_id = cx.tcx.hir.body_owner_def_id(anon_const.body); + let substs = Substs::identity_for_item(cx.tcx.global_tcx(), def_id); + let instance = ty::Instance::new(def_id, substs); + let c_id = GlobalId { instance, promoted: None }; - let constant = cx.tcx.const_eval(param_env.and(cid)).ok(); + let constant = cx.tcx.const_eval(param_env.and(c_id)).ok(); if let Some(Constant::Int(val)) = constant.and_then(|c| miri_to_const(cx.tcx, c)) { - let mut ty = cx.tcx.type_of(did); + let mut ty = cx.tcx.type_of(def_id); if let ty::Adt(adt, _) = ty.sty { if adt.is_enum() { ty = adt.repr.discr_type().to_ty(cx.tcx); diff --git a/clippy_lints/src/enum_variants.rs b/clippy_lints/src/enum_variants.rs index 8d708f01720..3454eff08a9 100644 --- a/clippy_lints/src/enum_variants.rs +++ b/clippy_lints/src/enum_variants.rs @@ -255,6 +255,7 @@ impl EarlyLintPass for EnumVariantNames { assert!(last.is_some()); } + #[allow(clippy::similar_names)] fn check_item(&mut self, cx: &EarlyContext<'_>, item: &Item) { let item_name = item.ident.as_str(); let item_name_chars = item_name.chars().count(); diff --git a/clippy_lints/src/eq_op.rs b/clippy_lints/src/eq_op.rs index a454ea83695..dfe0c0180a7 100644 --- a/clippy_lints/src/eq_op.rs +++ b/clippy_lints/src/eq_op.rs @@ -63,6 +63,7 @@ impl LintPass for EqOp { } impl<'a, 'tcx> LateLintPass<'a, 'tcx> for EqOp { + #[allow(clippy::similar_names)] fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, e: &'tcx Expr) { if let ExprKind::Binary(op, ref left, ref right) = e.node { if in_macro(e.span) { diff --git a/clippy_lints/src/if_let_redundant_pattern_matching.rs b/clippy_lints/src/if_let_redundant_pattern_matching.rs index 8b42eaa528e..bced0c9552d 100644 --- a/clippy_lints/src/if_let_redundant_pattern_matching.rs +++ b/clippy_lints/src/if_let_redundant_pattern_matching.rs @@ -56,6 +56,7 @@ impl LintPass for Pass { } impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { + #[allow(clippy::similar_names)] fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { if let ExprKind::Match(ref op, ref arms, MatchSource::IfLetDesugar { .. }) = expr.node { if arms[0].pats.len() == 1 { diff --git a/clippy_lints/src/transmute.rs b/clippy_lints/src/transmute.rs index 0d49f5de265..801b6db63f5 100644 --- a/clippy_lints/src/transmute.rs +++ b/clippy_lints/src/transmute.rs @@ -227,6 +227,7 @@ impl LintPass for Transmute { } impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Transmute { + #[allow(clippy::similar_names)] fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, e: &'tcx Expr) { if let ExprKind::Call(ref path_expr, ref args) = e.node { if let ExprKind::Path(ref qpath) = path_expr.node { diff --git a/clippy_lints/src/utils/hir_utils.rs b/clippy_lints/src/utils/hir_utils.rs index bc55c22979b..7a0b28d15d8 100644 --- a/clippy_lints/src/utils/hir_utils.rs +++ b/clippy_lints/src/utils/hir_utils.rs @@ -73,6 +73,7 @@ impl<'a, 'tcx: 'a> SpanlessEq<'a, 'tcx> { && both(&left.expr, &right.expr, |l, r| self.eq_expr(l, r)) } + #[allow(clippy::similar_names)] pub fn eq_expr(&mut self, left: &Expr, right: &Expr) -> bool { if self.ignore_fn && differing_macro_contexts(left.span, right.span) { return false; @@ -208,6 +209,7 @@ impl<'a, 'tcx: 'a> SpanlessEq<'a, 'tcx> { } } + #[allow(clippy::similar_names)] fn eq_qpath(&mut self, left: &QPath, right: &QPath) -> bool { match (left, right) { (&QPath::Resolved(ref lty, ref lpath), &QPath::Resolved(ref rty, ref rpath)) => { @@ -262,6 +264,7 @@ impl<'a, 'tcx: 'a> SpanlessEq<'a, 'tcx> { self.eq_ty_kind(&left.node, &right.node) } + #[allow(clippy::similar_names)] pub fn eq_ty_kind(&mut self, left: &TyKind, right: &TyKind) -> bool { match (left, right) { (&TyKind::Slice(ref l_vec), &TyKind::Slice(ref r_vec)) => self.eq_ty(l_vec, r_vec), diff --git a/clippy_lints/src/utils/inspector.rs b/clippy_lints/src/utils/inspector.rs index 841aaaabdfa..ea48aa9ab5e 100644 --- a/clippy_lints/src/utils/inspector.rs +++ b/clippy_lints/src/utils/inspector.rs @@ -166,6 +166,7 @@ fn print_decl(cx: &LateContext<'_, '_>, decl: &hir::Decl) { } } +#[allow(clippy::similar_names)] fn print_expr(cx: &LateContext<'_, '_>, expr: &hir::Expr, indent: usize) { let ind = " ".repeat(indent); println!("{}+", ind); @@ -424,6 +425,7 @@ fn print_item(cx: &LateContext<'_, '_>, item: &hir::Item) { } } +#[allow(clippy::similar_names)] fn print_pat(cx: &LateContext<'_, '_>, pat: &hir::Pat, indent: usize) { let ind = " ".repeat(indent); println!("{}+", ind); diff --git a/clippy_lints/src/utils/usage.rs b/clippy_lints/src/utils/usage.rs index d26ffc715e8..f3af698ffa2 100644 --- a/clippy_lints/src/utils/usage.rs +++ b/clippy_lints/src/utils/usage.rs @@ -54,6 +54,7 @@ struct MutVarsDelegate { } impl<'tcx> MutVarsDelegate { + #[allow(clippy::similar_names)] fn update(&mut self, cat: &'tcx Categorization<'_>) { match *cat { Categorization::Local(id) => { From dcef9d07952eae2a2fbfe8f01e3885352c4ce8fb Mon Sep 17 00:00:00 2001 From: Devon Hollowood Date: Thu, 11 Oct 2018 15:36:40 -0700 Subject: [PATCH 06/50] Fix `stutter` lints --- clippy_lints/src/double_comparison.rs | 1 + clippy_lints/src/enum_variants.rs | 12 ++++----- clippy_lints/src/question_mark.rs | 1 + clippy_lints/src/utils/camel_case.rs | 38 +++++++++++++-------------- clippy_lints/src/utils/mod.rs | 3 +-- 5 files changed, 28 insertions(+), 27 deletions(-) diff --git a/clippy_lints/src/double_comparison.rs b/clippy_lints/src/double_comparison.rs index 3710301c8ab..314ca41ba21 100644 --- a/clippy_lints/src/double_comparison.rs +++ b/clippy_lints/src/double_comparison.rs @@ -40,6 +40,7 @@ declare_clippy_lint! { "unnecessary double comparisons that can be simplified" } +#[allow(clippy::stutter)] pub struct DoubleComparisonPass; impl LintPass for DoubleComparisonPass { diff --git a/clippy_lints/src/enum_variants.rs b/clippy_lints/src/enum_variants.rs index 3454eff08a9..16d1e40484d 100644 --- a/clippy_lints/src/enum_variants.rs +++ b/clippy_lints/src/enum_variants.rs @@ -16,7 +16,7 @@ use crate::syntax::ast::*; use crate::syntax::source_map::Span; use crate::syntax::symbol::LocalInternedString; use crate::utils::{span_help_and_lint, span_lint}; -use crate::utils::{camel_case_from, camel_case_until, in_macro}; +use crate::utils::{camel_case, in_macro}; /// **What it does:** Detects enumeration variants that are prefixed or suffixed /// by the same characters. @@ -184,19 +184,19 @@ fn check_variant( } } let first = var2str(&def.variants[0]); - let mut pre = &first[..camel_case_until(&*first)]; - let mut post = &first[camel_case_from(&*first)..]; + let mut pre = &first[..camel_case::until(&*first)]; + let mut post = &first[camel_case::from(&*first)..]; for var in &def.variants { let name = var2str(var); let pre_match = partial_match(pre, &name); pre = &pre[..pre_match]; - let pre_camel = camel_case_until(pre); + let pre_camel = camel_case::until(pre); pre = &pre[..pre_camel]; while let Some((next, last)) = name[pre.len()..].chars().zip(pre.chars().rev()).next() { if next.is_lowercase() { let last = pre.len() - last.len_utf8(); - let last_camel = camel_case_until(&pre[..last]); + let last_camel = camel_case::until(&pre[..last]); pre = &pre[..last_camel]; } else { break; @@ -206,7 +206,7 @@ fn check_variant( let post_match = partial_rmatch(post, &name); let post_end = post.len() - post_match; post = &post[post_end..]; - let post_camel = camel_case_from(post); + let post_camel = camel_case::from(post); post = &post[post_camel..]; } let (what, value) = match (pre.is_empty(), post.is_empty()) { diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index 0ec57e0be80..93a40e13540 100644 --- a/clippy_lints/src/question_mark.rs +++ b/clippy_lints/src/question_mark.rs @@ -44,6 +44,7 @@ declare_clippy_lint!{ "checks for expressions that could be replaced by the question mark operator" } +#[allow(clippy::stutter)] #[derive(Copy, Clone)] pub struct QuestionMarkPass; diff --git a/clippy_lints/src/utils/camel_case.rs b/clippy_lints/src/utils/camel_case.rs index 2b60e2c32fa..5ce1e08d8b5 100644 --- a/clippy_lints/src/utils/camel_case.rs +++ b/clippy_lints/src/utils/camel_case.rs @@ -10,7 +10,7 @@ /// Return the index of the character after the first camel-case component of /// `s`. -pub fn camel_case_until(s: &str) -> usize { +pub fn until(s: &str) -> usize { let mut iter = s.char_indices(); if let Some((_, first)) = iter.next() { if !first.is_uppercase() { @@ -43,7 +43,7 @@ pub fn camel_case_until(s: &str) -> usize { } /// Return index of the last camel-case component of `s`. -pub fn camel_case_from(s: &str) -> usize { +pub fn from(s: &str) -> usize { let mut iter = s.char_indices().rev(); if let Some((_, first)) = iter.next() { if !first.is_lowercase() { @@ -73,52 +73,52 @@ pub fn camel_case_from(s: &str) -> usize { #[cfg(test)] mod test { - use super::{camel_case_from, camel_case_until}; + use super::{from, until}; #[test] fn from_full() { - assert_eq!(camel_case_from("AbcDef"), 0); - assert_eq!(camel_case_from("Abc"), 0); + assert_eq!(from("AbcDef"), 0); + assert_eq!(from("Abc"), 0); } #[test] fn from_partial() { - assert_eq!(camel_case_from("abcDef"), 3); - assert_eq!(camel_case_from("aDbc"), 1); + assert_eq!(from("abcDef"), 3); + assert_eq!(from("aDbc"), 1); } #[test] fn from_not() { - assert_eq!(camel_case_from("AbcDef_"), 7); - assert_eq!(camel_case_from("AbcDD"), 5); + assert_eq!(from("AbcDef_"), 7); + assert_eq!(from("AbcDD"), 5); } #[test] fn from_caps() { - assert_eq!(camel_case_from("ABCD"), 4); + assert_eq!(from("ABCD"), 4); } #[test] fn until_full() { - assert_eq!(camel_case_until("AbcDef"), 6); - assert_eq!(camel_case_until("Abc"), 3); + assert_eq!(until("AbcDef"), 6); + assert_eq!(until("Abc"), 3); } #[test] fn until_not() { - assert_eq!(camel_case_until("abcDef"), 0); - assert_eq!(camel_case_until("aDbc"), 0); + assert_eq!(until("abcDef"), 0); + assert_eq!(until("aDbc"), 0); } #[test] fn until_partial() { - assert_eq!(camel_case_until("AbcDef_"), 6); - assert_eq!(camel_case_until("CallTypeC"), 8); - assert_eq!(camel_case_until("AbcDD"), 3); + assert_eq!(until("AbcDef_"), 6); + assert_eq!(until("CallTypeC"), 8); + assert_eq!(until("AbcDD"), 3); } #[test] fn until_caps() { - assert_eq!(camel_case_until("ABCD"), 0); + assert_eq!(until("ABCD"), 0); } -} \ No newline at end of file +} diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 2a9b1cb0a10..05356f8d385 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -33,8 +33,7 @@ use crate::syntax::source_map::{Span, DUMMY_SP}; use crate::syntax::errors::DiagnosticBuilder; use crate::syntax::symbol::keywords; -mod camel_case; -pub use self::camel_case::{camel_case_from, camel_case_until}; +pub mod camel_case; pub mod comparisons; pub mod conf; From 73ba33dd2b22c4b434a9189304c39f8dfa608f70 Mon Sep 17 00:00:00 2001 From: Devon Hollowood Date: Thu, 11 Oct 2018 15:43:13 -0700 Subject: [PATCH 07/50] Fix `doc_markdown` lints --- clippy_lints/src/excessive_precision.rs | 1 + clippy_lints/src/map_unit_fn.rs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/excessive_precision.rs b/clippy_lints/src/excessive_precision.rs index 99668880f72..15a8d47337a 100644 --- a/clippy_lints/src/excessive_precision.rs +++ b/clippy_lints/src/excessive_precision.rs @@ -108,6 +108,7 @@ impl ExcessivePrecision { } } +#[allow(clippy::doc_markdown)] /// Should we exclude the float because it has a `.0` or `.` suffix /// Ex 1_000_000_000.0 /// Ex 1_000_000_000. diff --git a/clippy_lints/src/map_unit_fn.rs b/clippy_lints/src/map_unit_fn.rs index e620a1815ce..503a2ee7032 100644 --- a/clippy_lints/src/map_unit_fn.rs +++ b/clippy_lints/src/map_unit_fn.rs @@ -179,7 +179,7 @@ fn unit_closure<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'a hir::Expr) -> Op None } -/// Builds a name for the let binding variable (var_arg) +/// Builds a name for the let binding variable (`var_arg`) /// /// `x.field` => `x_field` /// `y` => `_y` From ff9dfccadee1a724f357999fb9eb671fe04e837c Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Fri, 12 Oct 2018 07:19:34 +0200 Subject: [PATCH 08/50] Add Travis windows build See https://blog.travis-ci.com/2018-10-11-windows-early-release --- .travis.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.travis.yml b/.travis.yml index 6d44c7faa93..20c98062dd8 100644 --- a/.travis.yml +++ b/.travis.yml @@ -5,6 +5,7 @@ rust: nightly os: - linux - osx + - windows sudo: false @@ -36,6 +37,8 @@ matrix: env: BASE_TESTS=true - os: linux env: BASE_TESTS=true + - os: windows + env: BASE_TEST=true - env: INTEGRATION=rust-lang/cargo - env: INTEGRATION=rust-lang-nursery/rand - env: INTEGRATION=rust-lang-nursery/stdsimd @@ -53,6 +56,7 @@ matrix: exclude: - os: linux - os: osx + - os: windows script: - | From 024ccb4f508c184127e2522ce8ac77a9229b31e9 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Fri, 12 Oct 2018 07:41:25 +0200 Subject: [PATCH 09/50] Move Travis Windows build to allowed failures Until the remaining issues are fixed. This also enabled `fast_finish`. It will finish even if the windows build is still running. --- .travis.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.travis.yml b/.travis.yml index 20c98062dd8..7abfe0e03dd 100644 --- a/.travis.yml +++ b/.travis.yml @@ -52,6 +52,9 @@ matrix: - env: INTEGRATION=serde-rs/serde - env: INTEGRATION=Geal/nom - env: INTEGRATION=hyperium/hyper + allow_failures: + - os: windows + env: BASE_TEST=true # prevent these jobs with default env vars exclude: - os: linux From f5a38f2323006fb56cc730cc1313f9578bc84c68 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Fri, 12 Oct 2018 07:59:08 +0200 Subject: [PATCH 10/50] Only run markdown linter on linux Because: * There's no need to run it on more than one platform * It doesn't work on windows --- .travis.yml | 2 +- ci/base-tests.sh | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 7abfe0e03dd..5446f1ab568 100644 --- a/.travis.yml +++ b/.travis.yml @@ -24,7 +24,7 @@ before_install: install: - | - if [ -z ${INTEGRATION} ]; then + if [ -z ${INTEGRATION} ] && [ "$TRAVIS_OS_NAME" == "linux" ]; then . $HOME/.nvm/nvm.sh nvm install stable nvm use stable diff --git a/ci/base-tests.sh b/ci/base-tests.sh index ebf4a127cdc..72a38ee5e58 100755 --- a/ci/base-tests.sh +++ b/ci/base-tests.sh @@ -14,7 +14,9 @@ set -ex echo "Running clippy base tests" PATH=$PATH:./node_modules/.bin -remark -f *.md > /dev/null +if [ "$TRAVIS_OS_NAME" == "linux" ]; then + remark -f *.md > /dev/null +fi # build clippy in debug mode and run tests cargo build --features debugging cargo test --features debugging From 34fd4af503f6cf193e42cd7760c848bc45852875 Mon Sep 17 00:00:00 2001 From: sigustin Date: Fri, 12 Oct 2018 12:15:20 +0200 Subject: [PATCH 11/50] Specify which categories are enabled by default Closes #3293 --- README.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/README.md b/README.md index 14a5e56cdea..cf3b3e1ffdd 100644 --- a/README.md +++ b/README.md @@ -24,6 +24,15 @@ We have a bunch of lint categories to allow you to choose how much Clippy is sup More to come, please [file an issue](https://github.com/rust-lang-nursery/rust-clippy/issues) if you have ideas! +Only the following of those categories are enabled by default: + +* `clippy::style` +* `clippy::correctness` +* `clippy::complexity` +* `clippy::perf` + +Other categories need to be enabled in order for their lints to be executed. + Table of contents: * [Usage instructions](#usage) From 4e2062518775c7c42db48aea2678d16eaba2d72b Mon Sep 17 00:00:00 2001 From: sigustin Date: Fri, 12 Oct 2018 12:32:48 +0200 Subject: [PATCH 12/50] Add a comment reminding to update README if the default changes --- clippy_lints/src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index c343cf364f6..7cdc0f43b34 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -46,6 +46,8 @@ extern crate syntax_pos; 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_rules! declare_clippy_lint { { pub $name:tt, style, $description:tt } => { declare_tool_lint! { pub clippy::$name, Warn, $description, report_in_external_macro: true } From c9718fa589552476ee277c52a35271663383cf6a Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Fri, 12 Oct 2018 04:34:41 -0700 Subject: [PATCH 13/50] cmp_owned correct error message if rhs is deref --- clippy_lints/src/misc.rs | 4 ++++ tests/ui/cmp_owned.stderr | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs index be863cd7bc8..0a65953313e 100644 --- a/clippy_lints/src/misc.rs +++ b/clippy_lints/src/misc.rs @@ -579,6 +579,10 @@ fn check_to_owned(cx: &LateContext<'_, '_>, expr: &Expr, other: &Expr) { } } } + if other_gets_derefed { + db.span_label(lint_span, "try implementing the comparison without allocating"); + return; + } db.span_suggestion_with_applicability( lint_span, "try", diff --git a/tests/ui/cmp_owned.stderr b/tests/ui/cmp_owned.stderr index 1db60be54d6..a7371ab4b6c 100644 --- a/tests/ui/cmp_owned.stderr +++ b/tests/ui/cmp_owned.stderr @@ -40,13 +40,13 @@ error: this creates an owned instance just for comparison --> $DIR/cmp_owned.rs:42:5 | 42 | y.to_owned() == *x; - | ^^^^^^^^^^^^^^^^^^ help: try: `y == x` + | ^^^^^^^^^^^^^^^^^^ try implementing the comparison without allocating error: this creates an owned instance just for comparison --> $DIR/cmp_owned.rs:47:5 | 47 | y.to_owned() == **x; - | ^^^^^^^^^^^^^^^^^^^ help: try: `y == *x` + | ^^^^^^^^^^^^^^^^^^^ try implementing the comparison without allocating error: this creates an owned instance just for comparison --> $DIR/cmp_owned.rs:54:9 From 352863065cb644a4f59fa5655601960e34bf77e7 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Fri, 12 Oct 2018 04:48:54 -0700 Subject: [PATCH 14/50] cmp_owned refactor --- clippy_lints/src/misc.rs | 45 +++++++++++++--------------------------- 1 file changed, 14 insertions(+), 31 deletions(-) diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs index 0a65953313e..1cf7345e8df 100644 --- a/clippy_lints/src/misc.rs +++ b/clippy_lints/src/misc.rs @@ -21,7 +21,7 @@ use crate::utils::{get_item_name, get_parent_expr, implements_trait, in_constant iter_input_pats, last_path_segment, match_qpath, match_trait_method, paths, snippet, span_lint, span_lint_and_then, walk_ptrs_ty, SpanlessEq}; use crate::utils::sugg::Sugg; -use crate::syntax::ast::{LitKind, CRATE_NODE_ID}; +use crate::syntax::ast::LitKind; use crate::consts::{constant, Constant}; use crate::rustc_errors::Applicability; @@ -540,18 +540,10 @@ fn check_to_owned(cx: &LateContext<'_, '_>, expr: &Expr, other: &Expr) { _ => false, }; - let (lint_span, try_hint) = if deref_arg_impl_partial_eq_other { - // suggest deref on the left - (expr.span, format!("*{}", snip)) - } else if other_gets_derefed { - // suggest dropping the to_owned on the left and the deref on the right - let other_snippet = snippet(cx, other.span, "..").into_owned(); - let other_without_deref = other_snippet.replacen('*', "", 1); - - (expr.span.to(other.span), format!("{} == {}", snip.to_string(), other_without_deref)) + let lint_span = if other_gets_derefed { + expr.span.to(other.span) } else { - // suggest dropping the to_owned on the left - (expr.span, snip.to_string()) + expr.span }; span_lint_and_then( @@ -560,29 +552,20 @@ fn check_to_owned(cx: &LateContext<'_, '_>, expr: &Expr, other: &Expr) { lint_span, "this creates an owned instance just for comparison", |db| { - // this is as good as our recursion check can get, we can't prove that the - // current function is - // called by - // PartialEq::eq, but we can at least ensure that this code is not part of it - let parent_fn = cx.tcx.hir.get_parent(expr.id); - let parent_impl = cx.tcx.hir.get_parent(parent_fn); - if parent_impl != CRATE_NODE_ID { - if let Node::Item(item) = cx.tcx.hir.get(parent_impl) { - if let ItemKind::Impl(.., Some(ref trait_ref), _, _) = item.node { - if trait_ref.path.def.def_id() == partial_eq_trait_id { - // we are implementing PartialEq, don't suggest not doing `to_owned`, otherwise - // we go into - // recursion - db.span_label(lint_span, "try implementing the comparison without allocating"); - return; - } - } - } - } + // this also catches PartialEq implementations that call to_owned if other_gets_derefed { db.span_label(lint_span, "try implementing the comparison without allocating"); return; } + + let try_hint = if deref_arg_impl_partial_eq_other { + // suggest deref on the left + format!("*{}", snip) + } else { + // suggest dropping the to_owned on the left + snip.to_string() + }; + db.span_suggestion_with_applicability( lint_span, "try", From d3c06f7252fdca30b19aa6ff8ecf63c86675676e Mon Sep 17 00:00:00 2001 From: Joshua Holmer Date: Fri, 12 Oct 2018 10:26:55 -0400 Subject: [PATCH 15/50] Exclude pattern guards from unnecessary_fold lint Methods like `Iterator::any` borrow the iterator mutably, which is not allowed within a pattern guard and will fail to compile. This commit prevents clippy from suggesting this type of change. Closes #3069 --- clippy_lints/src/methods/mod.rs | 18 ++++++++++++++++++ tests/ui/unnecessary_fold.rs | 7 +++++++ 2 files changed, 25 insertions(+) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 7c15eb677cc..66c94746f5a 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -9,6 +9,7 @@ use crate::rustc::hir; +use crate::rustc::hir::{ExprKind, Guard, Node}; use crate::rustc::hir::def::Def; use crate::rustc::lint::{in_external_macro, LateContext, LateLintPass, Lint, LintArray, LintContext, LintPass}; use crate::rustc::ty::{self, Ty}; @@ -1428,6 +1429,23 @@ fn lint_unnecessary_fold(cx: &LateContext<'_, '_>, expr: &hir::Expr, fold_args: return; } + // `Iterator::any` cannot be used within a pattern guard + // See https://github.com/rust-lang-nursery/rust-clippy/issues/3069 + if_chain! { + if let Some(fold_parent) = cx.tcx.hir.find(cx.tcx.hir.get_parent_node(expr.id)); + if let Node::Expr(fold_parent) = fold_parent; + if let ExprKind::Match(_, ref arms, _) = fold_parent.node; + if arms.iter().any(|arm| { + if let Some(Guard::If(ref guard)) = arm.guard { + return guard.id == expr.id; + } + false + }); + then { + return; + } + } + assert!(fold_args.len() == 3, "Expected fold_args to have three entries - the receiver, the initial value and the closure"); diff --git a/tests/ui/unnecessary_fold.rs b/tests/ui/unnecessary_fold.rs index e8d84ecea8c..3b70602d4ad 100644 --- a/tests/ui/unnecessary_fold.rs +++ b/tests/ui/unnecessary_fold.rs @@ -45,6 +45,13 @@ fn unnecessary_fold_should_ignore() { let _ = [(0..2), (0..3)].iter().fold(0, |a, b| a + b.len()); let _ = [(0..2), (0..3)].iter().fold(1, |a, b| a * b.len()); + + // Because `any` takes the iterator as a mutable reference, + // it cannot be used in a pattern guard, and we must use `fold`. + match 1 { + _ if (0..3).fold(false, |acc, x| acc || x > 2) => {} + _ => {} + } } fn main() {} From 863c8e26fc66e856456cb05e75e1d6c821a04ec6 Mon Sep 17 00:00:00 2001 From: Joshua Holmer Date: Fri, 12 Oct 2018 13:15:55 -0400 Subject: [PATCH 16/50] Revert "Exclude pattern guards from unnecessary_fold lint" This reverts commit d3c06f7252fdca30b19aa6ff8ecf63c86675676e. --- clippy_lints/src/methods/mod.rs | 18 ------------------ tests/ui/unnecessary_fold.rs | 7 ------- 2 files changed, 25 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 66c94746f5a..7c15eb677cc 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -9,7 +9,6 @@ use crate::rustc::hir; -use crate::rustc::hir::{ExprKind, Guard, Node}; use crate::rustc::hir::def::Def; use crate::rustc::lint::{in_external_macro, LateContext, LateLintPass, Lint, LintArray, LintContext, LintPass}; use crate::rustc::ty::{self, Ty}; @@ -1429,23 +1428,6 @@ fn lint_unnecessary_fold(cx: &LateContext<'_, '_>, expr: &hir::Expr, fold_args: return; } - // `Iterator::any` cannot be used within a pattern guard - // See https://github.com/rust-lang-nursery/rust-clippy/issues/3069 - if_chain! { - if let Some(fold_parent) = cx.tcx.hir.find(cx.tcx.hir.get_parent_node(expr.id)); - if let Node::Expr(fold_parent) = fold_parent; - if let ExprKind::Match(_, ref arms, _) = fold_parent.node; - if arms.iter().any(|arm| { - if let Some(Guard::If(ref guard)) = arm.guard { - return guard.id == expr.id; - } - false - }); - then { - return; - } - } - assert!(fold_args.len() == 3, "Expected fold_args to have three entries - the receiver, the initial value and the closure"); diff --git a/tests/ui/unnecessary_fold.rs b/tests/ui/unnecessary_fold.rs index 3b70602d4ad..e8d84ecea8c 100644 --- a/tests/ui/unnecessary_fold.rs +++ b/tests/ui/unnecessary_fold.rs @@ -45,13 +45,6 @@ fn unnecessary_fold_should_ignore() { let _ = [(0..2), (0..3)].iter().fold(0, |a, b| a + b.len()); let _ = [(0..2), (0..3)].iter().fold(1, |a, b| a * b.len()); - - // Because `any` takes the iterator as a mutable reference, - // it cannot be used in a pattern guard, and we must use `fold`. - match 1 { - _ if (0..3).fold(false, |acc, x| acc || x > 2) => {} - _ => {} - } } fn main() {} From 0a1bae95074a682318e167bee55078dd8ccbdc51 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Fri, 12 Oct 2018 22:04:58 +0200 Subject: [PATCH 17/50] Install Windows SDK 10.0 on travis --- .travis.yml | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/.travis.yml b/.travis.yml index 5446f1ab568..818353e0c16 100644 --- a/.travis.yml +++ b/.travis.yml @@ -24,11 +24,16 @@ before_install: install: - | - if [ -z ${INTEGRATION} ] && [ "$TRAVIS_OS_NAME" == "linux" ]; then - . $HOME/.nvm/nvm.sh - nvm install stable - nvm use stable - npm install remark-cli remark-lint + if [ -z ${INTEGRATION} ]; then + if [ "$TRAVIS_OS_NAME" == "linux" ]; then + . $HOME/.nvm/nvm.sh + nvm install stable + nvm use stable + npm install remark-cli remark-lint + fi + if [ "$TRAVIS_OS_NAME" == "windows" ]; then + choco install windows-sdk-10.0 + fi fi matrix: From e8687a6677b2352228a6edd2ba05282cbb1ddb65 Mon Sep 17 00:00:00 2001 From: Andre Bogus Date: Thu, 27 Sep 2018 19:10:20 +0200 Subject: [PATCH 18/50] unused unit lint --- CHANGELOG.md | 1 + README.md | 2 +- clippy_lints/src/lib.rs | 2 + clippy_lints/src/returns.rs | 100 ++++++++++++++++++++++++++++++++++-- tests/ui/copies.rs | 8 +-- tests/ui/my_lint.rs | 7 +++ tests/ui/unused_unit.rs | 52 +++++++++++++++++++ tests/ui/unused_unit.stderr | 52 +++++++++++++++++++ 8 files changed, 216 insertions(+), 8 deletions(-) create mode 100644 tests/ui/my_lint.rs create mode 100644 tests/ui/unused_unit.rs create mode 100644 tests/ui/unused_unit.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index c9bea1e8ef5..e6792c06894 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -878,6 +878,7 @@ All notable changes to this project will be documented in this file. [`unused_collect`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#unused_collect [`unused_io_amount`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#unused_io_amount [`unused_label`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#unused_label +[`unused_unit`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#unused_unit [`use_debug`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#use_debug [`use_self`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#use_self [`used_underscore_binding`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#used_underscore_binding diff --git a/README.md b/README.md index 14a5e56cdea..b07bac9b11a 100644 --- a/README.md +++ b/README.md @@ -9,7 +9,7 @@ We are currently in the process of discussing Clippy 1.0 via the RFC process in A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 279 lints included in this crate!](https://rust-lang-nursery.github.io/rust-clippy/master/index.html) +[There are 280 lints included in this crate!](https://rust-lang-nursery.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index c343cf364f6..3d8179e4782 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -685,6 +685,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { regex::TRIVIAL_REGEX, returns::LET_AND_RETURN, returns::NEEDLESS_RETURN, + returns::UNUSED_UNIT, serde_api::SERDE_API_MISUSE, strings::STRING_LIT_AS_BYTES, suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL, @@ -801,6 +802,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { regex::TRIVIAL_REGEX, returns::LET_AND_RETURN, returns::NEEDLESS_RETURN, + returns::UNUSED_UNIT, strings::STRING_LIT_AS_BYTES, types::FN_TO_NUMERIC_CAST, types::FN_TO_NUMERIC_CAST_WITH_TRUNCATION, diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs index f4360802483..d083387e852 100644 --- a/clippy_lints/src/returns.rs +++ b/clippy_lints/src/returns.rs @@ -14,8 +14,8 @@ use if_chain::if_chain; use crate::syntax::ast; use crate::syntax::source_map::Span; use crate::syntax::visit::FnKind; +use crate::syntax_pos::BytePos; use crate::rustc_errors::Applicability; - use crate::utils::{in_macro, match_path_ast, snippet_opt, span_lint_and_then, span_note_and_lint}; /// **What it does:** Checks for return statements at the end of a block. @@ -68,6 +68,25 @@ declare_clippy_lint! { the end of a block" } +/// **What it does:** Checks for unit (`()`) expressions that can be removed. +/// +/// **Why is this bad?** Such expressions add no value, but can make the code +/// less readable. Depending on formatting they can make a `break` or `return` +/// statement look like a function call. +/// +/// **Known problems:** The lint currently misses unit return types in types, +/// e.g. the `F` in `fn generic_unit ()>(f: F) { .. }`. +/// +/// **Example:** +/// ```rust +/// fn return_unit() -> () { () } +/// ``` +declare_clippy_lint! { + pub UNUSED_UNIT, + style, + "needless unit expression" +} + #[derive(Copy, Clone)] pub struct ReturnPass; @@ -162,23 +181,98 @@ impl ReturnPass { impl LintPass for ReturnPass { fn get_lints(&self) -> LintArray { - lint_array!(NEEDLESS_RETURN, LET_AND_RETURN) + lint_array!(NEEDLESS_RETURN, LET_AND_RETURN, UNUSED_UNIT) } } impl EarlyLintPass for ReturnPass { - fn check_fn(&mut self, cx: &EarlyContext<'_>, kind: FnKind<'_>, _: &ast::FnDecl, _: Span, _: ast::NodeId) { + fn check_fn(&mut self, cx: &EarlyContext<'_>, kind: FnKind<'_>, decl: &ast::FnDecl, span: Span, _: ast::NodeId) { match kind { FnKind::ItemFn(.., block) | FnKind::Method(.., block) => self.check_block_return(cx, block), FnKind::Closure(body) => self.check_final_expr(cx, body, Some(body.span)), } + if_chain! { + if let ast::FunctionRetTy::Ty(ref ty) = decl.output; + if let ast::TyKind::Tup(ref vals) = ty.node; + if vals.is_empty() && !in_macro(ty.span) && get_def(span) == get_def(ty.span); + then { + let (rspan, appl) = if let Ok(fn_source) = + cx.sess().source_map() + .span_to_snippet(span.with_hi(ty.span.hi())) { + if let Some(rpos) = fn_source.rfind("->") { + (ty.span.with_lo(BytePos(span.lo().0 + rpos as u32)), + Applicability::MachineApplicable) + } else { + (ty.span, Applicability::MaybeIncorrect) + } + } else { + (ty.span, Applicability::MaybeIncorrect) + }; + span_lint_and_then(cx, UNUSED_UNIT, rspan, "unneeded unit return type", |db| { + db.span_suggestion_with_applicability( + rspan, + "remove the `-> ()`", + String::new(), + appl, + ); + }); + } + } } fn check_block(&mut self, cx: &EarlyContext<'_>, block: &ast::Block) { self.check_let_return(cx, block); + if_chain! { + if let Some(ref stmt) = block.stmts.last(); + if let ast::StmtKind::Expr(ref expr) = stmt.node; + if is_unit_expr(expr) && !in_macro(expr.span); + then { + let sp = expr.span; + span_lint_and_then(cx, UNUSED_UNIT, sp, "unneeded unit expression", |db| { + db.span_suggestion_with_applicability( + sp, + "remove the final `()`", + String::new(), + Applicability::MachineApplicable, + ); + }); + } + } + } + + fn check_expr(&mut self, cx: &EarlyContext<'_>, e: &ast::Expr) { + match e.node { + ast::ExprKind::Ret(Some(ref expr)) | ast::ExprKind::Break(_, Some(ref expr)) => { + if is_unit_expr(expr) && !in_macro(expr.span) { + span_lint_and_then(cx, UNUSED_UNIT, expr.span, "unneeded `()`", |db| { + db.span_suggestion_with_applicability( + expr.span, + "remove the `()`", + String::new(), + Applicability::MachineApplicable, + ); + }); + } + } + _ => () + } } } fn attr_is_cfg(attr: &ast::Attribute) -> bool { attr.meta_item_list().is_some() && attr.name() == "cfg" } + +// get the def site +fn get_def(span: Span) -> Option { + span.ctxt().outer().expn_info().and_then(|info| info.def_site) +} + +// is this expr a `()` unit? +fn is_unit_expr(expr: &ast::Expr) -> bool { + if let ast::ExprKind::Tup(ref vals) = expr.node { + vals.is_empty() + } else { + false + } +} diff --git a/tests/ui/copies.rs b/tests/ui/copies.rs index 8d0bc802daf..5c4bbecf822 100644 --- a/tests/ui/copies.rs +++ b/tests/ui/copies.rs @@ -7,11 +7,10 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. - - - #![allow(clippy::blacklisted_name, clippy::collapsible_if, clippy::cyclomatic_complexity, clippy::eq_op, clippy::needless_continue, - clippy::needless_return, clippy::never_loop, clippy::no_effect, clippy::zero_divided_by_zero)] + clippy::needless_return, clippy::never_loop, clippy::no_effect, clippy::zero_divided_by_zero, clippy::unused_unit)] + + fn bar(_: T) {} fn foo() -> bool { unimplemented!() } @@ -28,6 +27,7 @@ pub enum Abc { #[warn(clippy::if_same_then_else)] #[warn(clippy::match_same_arms)] +#[allow(clippy::unused_unit)] fn if_same_then_else() -> Result<&'static str, ()> { if true { Foo { bar: 42 }; diff --git a/tests/ui/my_lint.rs b/tests/ui/my_lint.rs new file mode 100644 index 00000000000..c27fd5be134 --- /dev/null +++ b/tests/ui/my_lint.rs @@ -0,0 +1,7 @@ +#[clippy::author] +#[cfg(any(target_arch = "x86"))] +pub struct Foo { + x: u32, +} + +fn main() {} diff --git a/tests/ui/unused_unit.rs b/tests/ui/unused_unit.rs new file mode 100644 index 00000000000..a7f08c28939 --- /dev/null +++ b/tests/ui/unused_unit.rs @@ -0,0 +1,52 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// 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. + +// run-rustfix +// compile-pass + +// The output for humans should just highlight the whole span without showing +// the suggested replacement, but we also want to test that suggested +// replacement only removes one set of parentheses, rather than naïvely +// stripping away any starting or ending parenthesis characters—hence this +// test of the JSON error format. + + +#![deny(clippy::unused_unit)] +#![allow(clippy::needless_return)] + +struct Unitter; + +impl Unitter { + // try to disorient the lint with multiple unit returns and newlines + pub fn get_unit (), G>(&self, f: F, _g: G) -> + () + where G: Fn() -> () { + let _y: &Fn() -> () = &f; + (); // this should not lint, as it's not in return type position + } +} + +impl Into<()> for Unitter { + fn into(self) -> () { + () + } +} + +fn return_unit() -> () { () } + +fn main() { + let u = Unitter; + assert_eq!(u.get_unit(|| {}, return_unit), u.into()); + return_unit(); + loop { + break(); + } + return(); +} diff --git a/tests/ui/unused_unit.stderr b/tests/ui/unused_unit.stderr new file mode 100644 index 00000000000..b5d5bdbcbee --- /dev/null +++ b/tests/ui/unused_unit.stderr @@ -0,0 +1,52 @@ +error: unneeded unit return type + --> $DIR/unused_unit.rs:28:59 + | +28 | pub fn get_unit (), G>(&self, f: F, _g: G) -> + | ___________________________________________________________^ +29 | | () + | |__________^ help: remove the `-> ()` + | +note: lint level defined here + --> $DIR/unused_unit.rs:21:9 + | +21 | #![deny(clippy::unused_unit)] + | ^^^^^^^^^^^^^^^^^^^ + +error: unneeded unit return type + --> $DIR/unused_unit.rs:37:19 + | +37 | fn into(self) -> () { + | ^^^^^ help: remove the `-> ()` + +error: unneeded unit expression + --> $DIR/unused_unit.rs:38:9 + | +38 | () + | ^^ help: remove the final `()` + +error: unneeded unit return type + --> $DIR/unused_unit.rs:42:18 + | +42 | fn return_unit() -> () { () } + | ^^^^^ help: remove the `-> ()` + +error: unneeded unit expression + --> $DIR/unused_unit.rs:42:26 + | +42 | fn return_unit() -> () { () } + | ^^ help: remove the final `()` + +error: unneeded `()` + --> $DIR/unused_unit.rs:49:14 + | +49 | break(); + | ^^ help: remove the `()` + +error: unneeded `()` + --> $DIR/unused_unit.rs:51:11 + | +51 | return(); + | ^^ help: remove the `()` + +error: aborting due to 7 previous errors + From 335bc1e820c2fd1316e3c225189361b43f2654c3 Mon Sep 17 00:00:00 2001 From: Devon Hollowood Date: Fri, 12 Oct 2018 17:07:48 -0700 Subject: [PATCH 19/50] Fix some more `stutter` warnings --- clippy_lints/src/double_comparison.rs | 9 ++++----- clippy_lints/src/lib.rs | 4 ++-- clippy_lints/src/question_mark.rs | 9 ++++----- 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/clippy_lints/src/double_comparison.rs b/clippy_lints/src/double_comparison.rs index 314ca41ba21..0171ac1e784 100644 --- a/clippy_lints/src/double_comparison.rs +++ b/clippy_lints/src/double_comparison.rs @@ -40,16 +40,15 @@ declare_clippy_lint! { "unnecessary double comparisons that can be simplified" } -#[allow(clippy::stutter)] -pub struct DoubleComparisonPass; +pub struct Pass; -impl LintPass for DoubleComparisonPass { +impl LintPass for Pass { fn get_lints(&self) -> LintArray { lint_array!(DOUBLE_COMPARISONS) } } -impl<'a, 'tcx> DoubleComparisonPass { +impl<'a, 'tcx> Pass { #[allow(clippy::similar_names)] fn check_binop( &self, @@ -89,7 +88,7 @@ impl<'a, 'tcx> DoubleComparisonPass { } } -impl<'a, 'tcx> LateLintPass<'a, 'tcx> for DoubleComparisonPass { +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { if let ExprKind::Binary(ref kind, ref lhs, ref rhs) = expr.node { self.check_binop(cx, kind.node, lhs, rhs, expr.span); diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index c343cf364f6..9b749bdcebe 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -428,8 +428,8 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { reg.register_late_lint_pass(box fallible_impl_from::FallibleImplFrom); reg.register_late_lint_pass(box replace_consts::ReplaceConsts); reg.register_late_lint_pass(box types::UnitArg); - reg.register_late_lint_pass(box double_comparison::DoubleComparisonPass); - reg.register_late_lint_pass(box question_mark::QuestionMarkPass); + reg.register_late_lint_pass(box double_comparison::Pass); + reg.register_late_lint_pass(box question_mark::Pass); reg.register_late_lint_pass(box suspicious_trait_impl::SuspiciousImpl); reg.register_early_lint_pass(box multiple_crate_versions::Pass); reg.register_late_lint_pass(box map_unit_fn::Pass); diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index 93a40e13540..72d33e58cd3 100644 --- a/clippy_lints/src/question_mark.rs +++ b/clippy_lints/src/question_mark.rs @@ -44,17 +44,16 @@ declare_clippy_lint!{ "checks for expressions that could be replaced by the question mark operator" } -#[allow(clippy::stutter)] #[derive(Copy, Clone)] -pub struct QuestionMarkPass; +pub struct Pass; -impl LintPass for QuestionMarkPass { +impl LintPass for Pass { fn get_lints(&self) -> LintArray { lint_array!(QUESTION_MARK) } } -impl QuestionMarkPass { +impl Pass { /// Check if the given expression on the given context matches the following structure: /// /// ```ignore @@ -146,7 +145,7 @@ impl QuestionMarkPass { } } -impl<'a, 'tcx> LateLintPass<'a, 'tcx> for QuestionMarkPass { +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { Self::check_is_none_and_early_return_none(cx, expr); } From eb854b233c353441f86c4a346a941c5965a2333a Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Tue, 2 Oct 2018 20:11:56 -0700 Subject: [PATCH 20/50] new_ret_no_self added positive test cases --- clippy_lints/src/methods/mod.rs | 22 ++++++------ tests/ui/new_ret_no_self.rs | 63 +++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 11 deletions(-) create mode 100644 tests/ui/new_ret_no_self.rs diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 7c15eb677cc..d11dbf0e773 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -878,6 +878,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { let name = implitem.ident.name; let parent = cx.tcx.hir.get_parent(implitem.id); let item = cx.tcx.hir.expect_item(parent); + let def_id = cx.tcx.hir.local_def_id(item.id); + let ty = cx.tcx.type_of(def_id); if_chain! { if let hir::ImplItemKind::Method(ref sig, id) = implitem.node; if let Some(first_arg_ty) = sig.decl.inputs.get(0); @@ -899,8 +901,6 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { } // check conventions w.r.t. conversion method names and predicates - let def_id = cx.tcx.hir.local_def_id(item.id); - let ty = cx.tcx.type_of(def_id); let is_copy = is_copy(cx, ty); for &(ref conv, self_kinds) in &CONVENTIONS { if conv.check(&name.as_str()) { @@ -928,17 +928,17 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { break; } } - - let ret_ty = return_ty(cx, implitem.id); - if name == "new" && - !ret_ty.walk().any(|t| same_tys(cx, t, ty)) { - span_lint(cx, - NEW_RET_NO_SELF, - implitem.span, - "methods called `new` usually return `Self`"); - } } } + + let ret_ty = return_ty(cx, implitem.id); + if name == "new" && + !ret_ty.walk().any(|t| same_tys(cx, t, ty)) { + span_lint(cx, + NEW_RET_NO_SELF, + implitem.span, + "methods called `new` usually return `Self`"); + } } } diff --git a/tests/ui/new_ret_no_self.rs b/tests/ui/new_ret_no_self.rs new file mode 100644 index 00000000000..67933f00262 --- /dev/null +++ b/tests/ui/new_ret_no_self.rs @@ -0,0 +1,63 @@ +#![feature(tool_lints)] + +#![warn(clippy::new_ret_no_self)] +#![allow(dead_code, clippy::trivially_copy_pass_by_ref)] + +fn main(){} + +//trait R { +// type Item; +//} +// +//struct S; +// +//impl R for S { +// type Item = Self; +//} +// +//impl S { +// // should not trigger the lint +// pub fn new() -> impl R { +// S +// } +//} +// +//struct S2; +// +//impl R for S2 { +// type Item = Self; +//} +// +//impl S2 { +// // should not trigger the lint +// pub fn new(_: String) -> impl R { +// S2 +// } +//} +// +//struct T; +// +//impl T { +// // should not trigger lint +// pub fn new() -> Self { +// unimplemented!(); +// } +//} + +struct U; + +impl U { + // should trigger lint + pub fn new() -> u32 { + unimplemented!(); + } +} + +struct V; + +impl V { + // should trigger lint + pub fn new(_: String) -> u32 { + unimplemented!(); + } +} From 13ce96c4bfd236ec49bcd7b63f42f9a51b0ee599 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Wed, 3 Oct 2018 03:55:31 -0700 Subject: [PATCH 21/50] new_ret_no_self corrected panic and added test stderr --- clippy_lints/src/methods/mod.rs | 16 ++++--- tests/ui/new_ret_no_self.rs | 76 ++++++++++++++++----------------- tests/ui/new_ret_no_self.stderr | 18 ++++++++ 3 files changed, 65 insertions(+), 45 deletions(-) create mode 100644 tests/ui/new_ret_no_self.stderr diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index d11dbf0e773..81cb1cd1182 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -931,13 +931,15 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { } } - let ret_ty = return_ty(cx, implitem.id); - if name == "new" && - !ret_ty.walk().any(|t| same_tys(cx, t, ty)) { - span_lint(cx, - NEW_RET_NO_SELF, - implitem.span, - "methods called `new` usually return `Self`"); + if let hir::ImplItemKind::Method(ref sig, id) = implitem.node { + let ret_ty = return_ty(cx, implitem.id); + if name == "new" && + !ret_ty.walk().any(|t| same_tys(cx, t, ty)) { + span_lint(cx, + NEW_RET_NO_SELF, + implitem.span, + "methods called `new` usually return `Self`"); + } } } } diff --git a/tests/ui/new_ret_no_self.rs b/tests/ui/new_ret_no_self.rs index 67933f00262..762dd582168 100644 --- a/tests/ui/new_ret_no_self.rs +++ b/tests/ui/new_ret_no_self.rs @@ -5,44 +5,44 @@ fn main(){} -//trait R { -// type Item; -//} -// -//struct S; -// -//impl R for S { -// type Item = Self; -//} -// -//impl S { -// // should not trigger the lint -// pub fn new() -> impl R { -// S -// } -//} -// -//struct S2; -// -//impl R for S2 { -// type Item = Self; -//} -// -//impl S2 { -// // should not trigger the lint -// pub fn new(_: String) -> impl R { -// S2 -// } -//} -// -//struct T; -// -//impl T { -// // should not trigger lint -// pub fn new() -> Self { -// unimplemented!(); -// } -//} +trait R { + type Item; +} + +struct S; + +impl R for S { + type Item = Self; +} + +impl S { + // should not trigger the lint + pub fn new() -> impl R { + S + } +} + +struct S2; + +impl R for S2 { + type Item = Self; +} + +impl S2 { + // should not trigger the lint + pub fn new(_: String) -> impl R { + S2 + } +} + +struct T; + +impl T { + // should not trigger lint + pub fn new() -> Self { + unimplemented!(); + } +} struct U; diff --git a/tests/ui/new_ret_no_self.stderr b/tests/ui/new_ret_no_self.stderr new file mode 100644 index 00000000000..1d698892449 --- /dev/null +++ b/tests/ui/new_ret_no_self.stderr @@ -0,0 +1,18 @@ +error: methods called `new` usually return `Self` + --> $DIR/new_ret_no_self.rs:51:5 + | +51 | / pub fn new() -> u32 { +52 | | unimplemented!(); +53 | | } + | |_____^ + +error: methods called `new` usually return `Self` + --> $DIR/new_ret_no_self.rs:60:5 + | +60 | / pub fn new(_: String) -> u32 { +61 | | unimplemented!(); +62 | | } + | |_____^ + +error: aborting due to 2 previous errors + From 1c4fa419f33211db3fa60f3bc8d59a8f42992558 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Wed, 3 Oct 2018 04:59:14 -0700 Subject: [PATCH 22/50] new_ret_no_self fix false positive for impl trait return with associated type self --- clippy_lints/src/methods/mod.rs | 3 ++- tests/ui/new_ret_no_self.rs | 13 +++++++++++++ tests/ui/new_ret_no_self.stderr | 18 ++++++++++-------- 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 81cb1cd1182..7426ece5ba9 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -934,7 +934,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { if let hir::ImplItemKind::Method(ref sig, id) = implitem.node { let ret_ty = return_ty(cx, implitem.id); if name == "new" && - !ret_ty.walk().any(|t| same_tys(cx, t, ty)) { + !same_tys(cx, ret_ty, ty) && + !ret_ty.is_impl_trait() { span_lint(cx, NEW_RET_NO_SELF, implitem.span, diff --git a/tests/ui/new_ret_no_self.rs b/tests/ui/new_ret_no_self.rs index 762dd582168..3b7ff7780ef 100644 --- a/tests/ui/new_ret_no_self.rs +++ b/tests/ui/new_ret_no_self.rs @@ -35,6 +35,19 @@ impl S2 { } } +struct S3; + +impl R for S3 { + type Item = u32; +} + +impl S3 { + // should trigger the lint, but currently does not + pub fn new(_: String) -> impl R { + S3 + } +} + struct T; impl T { diff --git a/tests/ui/new_ret_no_self.stderr b/tests/ui/new_ret_no_self.stderr index 1d698892449..cab5fa55cb6 100644 --- a/tests/ui/new_ret_no_self.stderr +++ b/tests/ui/new_ret_no_self.stderr @@ -1,17 +1,19 @@ error: methods called `new` usually return `Self` - --> $DIR/new_ret_no_self.rs:51:5 + --> $DIR/new_ret_no_self.rs:64:5 | -51 | / pub fn new() -> u32 { -52 | | unimplemented!(); -53 | | } +64 | / pub fn new() -> u32 { +65 | | unimplemented!(); +66 | | } | |_____^ + | + = note: `-D clippy::new-ret-no-self` implied by `-D warnings` error: methods called `new` usually return `Self` - --> $DIR/new_ret_no_self.rs:60:5 + --> $DIR/new_ret_no_self.rs:73:5 | -60 | / pub fn new(_: String) -> u32 { -61 | | unimplemented!(); -62 | | } +73 | / pub fn new(_: String) -> u32 { +74 | | unimplemented!(); +75 | | } | |_____^ error: aborting due to 2 previous errors From 2ef4af7db23c5522db2d71b60908b93127df5036 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Wed, 3 Oct 2018 05:00:43 -0700 Subject: [PATCH 23/50] Removed unused variables --- clippy_lints/src/methods/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 7426ece5ba9..c78bb48db2a 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -931,7 +931,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { } } - if let hir::ImplItemKind::Method(ref sig, id) = implitem.node { + if let hir::ImplItemKind::Method(_, _) = implitem.node { let ret_ty = return_ty(cx, implitem.id); if name == "new" && !same_tys(cx, ret_ty, ty) && From a5e4805ecf1ff5b38f0d467ba90530e43bfd0d9c Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Thu, 4 Oct 2018 19:01:04 -0700 Subject: [PATCH 24/50] new_ret_no_self correctly lint impl return --- clippy_lints/src/methods/mod.rs | 26 ++++++++++++++++++++++---- tests/ui/new_ret_no_self.rs | 21 ++++++++++++++++++++- tests/ui/new_ret_no_self.stderr | 26 +++++++++++++++++--------- 3 files changed, 59 insertions(+), 14 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index c78bb48db2a..f9c010beea7 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -11,7 +11,7 @@ use crate::rustc::hir; use crate::rustc::hir::def::Def; use crate::rustc::lint::{in_external_macro, LateContext, LateLintPass, Lint, LintArray, LintContext, LintPass}; -use crate::rustc::ty::{self, Ty}; +use crate::rustc::ty::{self, Ty, TyKind, Predicate}; use crate::rustc::{declare_tool_lint, lint_array}; use crate::rustc_errors::Applicability; use crate::syntax::ast; @@ -933,9 +933,27 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { if let hir::ImplItemKind::Method(_, _) = implitem.node { let ret_ty = return_ty(cx, implitem.id); - if name == "new" && - !same_tys(cx, ret_ty, ty) && - !ret_ty.is_impl_trait() { + + // if return type is impl trait + if let TyKind::Opaque(def_id, _) = ret_ty.sty { + + // then one of the associated types must be Self + for predicate in cx.tcx.predicates_of(def_id).predicates.iter() { + match predicate { + (Predicate::Projection(poly_projection_predicate), _) => { + let binder = poly_projection_predicate.ty(); + let associated_type = binder.skip_binder(); + let associated_type_is_self_type = same_tys(cx, ty, associated_type); + + // if the associated type is self, early return and do not trigger lint + if associated_type_is_self_type { return; } + }, + (_, _) => {}, + } + } + } + + if name == "new" && !same_tys(cx, ret_ty, ty) { span_lint(cx, NEW_RET_NO_SELF, implitem.span, diff --git a/tests/ui/new_ret_no_self.rs b/tests/ui/new_ret_no_self.rs index 3b7ff7780ef..e9f41d34133 100644 --- a/tests/ui/new_ret_no_self.rs +++ b/tests/ui/new_ret_no_self.rs @@ -9,6 +9,11 @@ trait R { type Item; } +trait Q { + type Item; + type Item2; +} + struct S; impl R for S { @@ -42,12 +47,26 @@ impl R for S3 { } impl S3 { - // should trigger the lint, but currently does not + // should trigger the lint pub fn new(_: String) -> impl R { S3 } } +struct S4; + +impl Q for S4 { + type Item = u32; + type Item2 = Self; +} + +impl S4 { + // should not trigger the lint + pub fn new(_: String) -> impl Q { + S4 + } +} + struct T; impl T { diff --git a/tests/ui/new_ret_no_self.stderr b/tests/ui/new_ret_no_self.stderr index cab5fa55cb6..aa3a633c418 100644 --- a/tests/ui/new_ret_no_self.stderr +++ b/tests/ui/new_ret_no_self.stderr @@ -1,20 +1,28 @@ error: methods called `new` usually return `Self` - --> $DIR/new_ret_no_self.rs:64:5 + --> $DIR/new_ret_no_self.rs:51:5 | -64 | / pub fn new() -> u32 { -65 | | unimplemented!(); -66 | | } +51 | / pub fn new(_: String) -> impl R { +52 | | S3 +53 | | } | |_____^ | = note: `-D clippy::new-ret-no-self` implied by `-D warnings` error: methods called `new` usually return `Self` - --> $DIR/new_ret_no_self.rs:73:5 + --> $DIR/new_ret_no_self.rs:83:5 | -73 | / pub fn new(_: String) -> u32 { -74 | | unimplemented!(); -75 | | } +83 | / pub fn new() -> u32 { +84 | | unimplemented!(); +85 | | } | |_____^ -error: aborting due to 2 previous errors +error: methods called `new` usually return `Self` + --> $DIR/new_ret_no_self.rs:92:5 + | +92 | / pub fn new(_: String) -> u32 { +93 | | unimplemented!(); +94 | | } + | |_____^ + +error: aborting due to 3 previous errors From 348d18ebd8ee5182d4705aba8341fb469f936ff5 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Thu, 4 Oct 2018 21:37:28 -0700 Subject: [PATCH 25/50] Removed new_ret_no_self tests from method.rs --- tests/ui/methods.rs | 4 ++-- tests/ui/methods.stderr | 12 ++---------- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/tests/ui/methods.rs b/tests/ui/methods.rs index 883dbf589d7..ae1b1642be7 100644 --- a/tests/ui/methods.rs +++ b/tests/ui/methods.rs @@ -14,7 +14,7 @@ #![warn(clippy::all, clippy::pedantic, clippy::option_unwrap_used)] #![allow(clippy::blacklisted_name, unused, clippy::print_stdout, clippy::non_ascii_literal, clippy::new_without_default, clippy::new_without_default_derive, clippy::missing_docs_in_private_items, clippy::needless_pass_by_value, - clippy::default_trait_access, clippy::use_self, clippy::useless_format)] + clippy::default_trait_access, clippy::use_self, clippy::new_ret_no_self, clippy::useless_format)] use std::collections::BTreeMap; use std::collections::HashMap; @@ -43,7 +43,7 @@ impl T { fn to_something(self) -> u32 { 0 } - fn new(self) {} + fn new(self) -> Self { unimplemented!(); } } struct Lt<'a> { diff --git a/tests/ui/methods.stderr b/tests/ui/methods.stderr index 124edee6a52..307814824ea 100644 --- a/tests/ui/methods.stderr +++ b/tests/ui/methods.stderr @@ -23,17 +23,9 @@ error: methods called `to_*` usually take self by reference; consider choosing a error: methods called `new` usually take no self; consider choosing a less ambiguous name --> $DIR/methods.rs:46:12 | -46 | fn new(self) {} +46 | fn new(self) -> Self { unimplemented!(); } | ^^^^ -error: methods called `new` usually return `Self` - --> $DIR/methods.rs:46:5 - | -46 | fn new(self) {} - | ^^^^^^^^^^^^^^^ - | - = note: `-D clippy::new-ret-no-self` implied by `-D warnings` - error: called `map(f).unwrap_or(a)` on an Option value. This can be done more directly by calling `map_or(a, f)` instead --> $DIR/methods.rs:114:13 | @@ -465,5 +457,5 @@ error: used unwrap() on an Option value. If you don't want to handle the None ca | = note: `-D clippy::option-unwrap-used` implied by `-D warnings` -error: aborting due to 58 previous errors +error: aborting due to 57 previous errors From 54506705cec65652c0607cfe8af7284546e9b576 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Mon, 8 Oct 2018 19:35:37 -0700 Subject: [PATCH 26/50] Added new_ret_no_self exception to clippy to pass dogfood tests --- clippy_lints/src/types.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 035ca2b0496..59c55168232 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -1920,6 +1920,7 @@ enum ImplicitHasherType<'tcx> { impl<'tcx> ImplicitHasherType<'tcx> { /// Checks that `ty` is a target type without a BuildHasher. + #[allow(clippy::new_ret_no_self)] fn new<'a>(cx: &LateContext<'a, 'tcx>, hir_ty: &hir::Ty) -> Option { if let TyKind::Path(QPath::Resolved(None, ref path)) = hir_ty.node { let params: Vec<_> = path.segments.last().as_ref()?.args.as_ref()? From 3f386d33f92c4bf439043cf2866f44fc0ee5b27c Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Sat, 13 Oct 2018 06:33:46 -0700 Subject: [PATCH 27/50] new_ret_no_self test remove tool lints cfg flag --- tests/ui/new_ret_no_self.rs | 2 -- tests/ui/new_ret_no_self.stderr | 24 ++++++++++++------------ 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/tests/ui/new_ret_no_self.rs b/tests/ui/new_ret_no_self.rs index e9f41d34133..1a4b91cc9da 100644 --- a/tests/ui/new_ret_no_self.rs +++ b/tests/ui/new_ret_no_self.rs @@ -1,5 +1,3 @@ -#![feature(tool_lints)] - #![warn(clippy::new_ret_no_self)] #![allow(dead_code, clippy::trivially_copy_pass_by_ref)] diff --git a/tests/ui/new_ret_no_self.stderr b/tests/ui/new_ret_no_self.stderr index aa3a633c418..ad26438d4ef 100644 --- a/tests/ui/new_ret_no_self.stderr +++ b/tests/ui/new_ret_no_self.stderr @@ -1,27 +1,27 @@ error: methods called `new` usually return `Self` - --> $DIR/new_ret_no_self.rs:51:5 + --> $DIR/new_ret_no_self.rs:49:5 | -51 | / pub fn new(_: String) -> impl R { -52 | | S3 -53 | | } +49 | / pub fn new(_: String) -> impl R { +50 | | S3 +51 | | } | |_____^ | = note: `-D clippy::new-ret-no-self` implied by `-D warnings` error: methods called `new` usually return `Self` - --> $DIR/new_ret_no_self.rs:83:5 + --> $DIR/new_ret_no_self.rs:81:5 | -83 | / pub fn new() -> u32 { -84 | | unimplemented!(); -85 | | } +81 | / pub fn new() -> u32 { +82 | | unimplemented!(); +83 | | } | |_____^ error: methods called `new` usually return `Self` - --> $DIR/new_ret_no_self.rs:92:5 + --> $DIR/new_ret_no_self.rs:90:5 | -92 | / pub fn new(_: String) -> u32 { -93 | | unimplemented!(); -94 | | } +90 | / pub fn new(_: String) -> u32 { +91 | | unimplemented!(); +92 | | } | |_____^ error: aborting due to 3 previous errors From c6f79c7ba0dcefaae7e96912a066ecbf4f63e8ca Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Sat, 13 Oct 2018 06:57:52 -0700 Subject: [PATCH 28/50] explicit_counter_loop fix #3308 false positive --- clippy_lints/src/loops.rs | 5 +---- tests/ui/for_loop.rs | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index b807e4fb9e1..064a8d5229d 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -1952,10 +1952,7 @@ impl<'a, 'tcx> Visitor<'tcx> for IncrementVisitor<'a, 'tcx> { _ => (), } } - } else if is_loop(expr) { - walk_expr(self, expr); - return; - } else if is_conditional(expr) { + } else if is_loop(expr) || is_conditional(expr) { self.depth += 1; walk_expr(self, expr); self.depth -= 1; diff --git a/tests/ui/for_loop.rs b/tests/ui/for_loop.rs index bdb6b56e0bb..89c452f44df 100644 --- a/tests/ui/for_loop.rs +++ b/tests/ui/for_loop.rs @@ -646,3 +646,38 @@ mod issue_1219 { } } } + +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); + } + } +} From c4928181107314fa437cbd6cdd078d07f53caef1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Kr=C3=BCger?= Date: Sun, 14 Oct 2018 10:30:04 +0200 Subject: [PATCH 29/50] mem_forget: fix syntax error in code sample --- clippy_lints/src/mem_forget.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/mem_forget.rs b/clippy_lints/src/mem_forget.rs index dd2a1ca50e7..accd7bc220c 100644 --- a/clippy_lints/src/mem_forget.rs +++ b/clippy_lints/src/mem_forget.rs @@ -23,7 +23,7 @@ use crate::utils::{match_def_path, opt_def_id, paths, span_lint}; /// /// **Example:** /// ```rust -/// mem::forget(Rc::new(55))) +/// mem::forget(Rc::new(55)) /// ``` declare_clippy_lint! { pub MEM_FORGET, From 212a4fe4f4aaa8e592f4e7178d9c9a7bcd8c97be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20S=CC=B6c=CC=B6h=CC=B6n=CC=B6e=CC=B6i=CC=B6d=CC=B6?= =?UTF-8?q?e=CC=B6r=20Scherer?= Date: Sun, 14 Oct 2018 22:55:26 +0200 Subject: [PATCH 30/50] fix for rustc master --- clippy_lints/src/len_zero.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/len_zero.rs b/clippy_lints/src/len_zero.rs index defb5892e51..1dd775051d9 100644 --- a/clippy_lints/src/len_zero.rs +++ b/clippy_lints/src/len_zero.rs @@ -276,7 +276,7 @@ fn has_is_empty(cx: &LateContext<'_, '_>, expr: &Expr) -> bool { let ty = &walk_ptrs_ty(cx.tables.expr_ty(expr)); match ty.sty { ty::Dynamic(ref tt, ..) => cx.tcx - .associated_items(tt.principal().expect("trait impl not found").def_id()) + .associated_items(tt.principal().def_id()) .any(|item| is_is_empty(cx, &item)), ty::Projection(ref proj) => has_is_empty_impl(cx, proj.item_def_id), ty::Adt(id, _) => has_is_empty_impl(cx, id.did), From 456843f1cd7d64015202338597cd3db79558dcbf Mon Sep 17 00:00:00 2001 From: Owen Sanchez Date: Sun, 14 Oct 2018 20:07:21 -0700 Subject: [PATCH 31/50] Swap order of methods in `needless_range_loop` suggestion in some cases --- clippy_lints/src/loops.rs | 40 ++++++++++++++++++++++++----- tests/ui/author/for_loop.stderr | 0 tests/ui/needless_range_loop.rs | 14 ++++++++++ tests/ui/needless_range_loop.stderr | 22 +++++++++++++++- tests/ui/ty_fn_sig.stderr | 0 5 files changed, 69 insertions(+), 7 deletions(-) create mode 100644 tests/ui/author/for_loop.stderr create mode 100644 tests/ui/ty_fn_sig.stderr diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 064a8d5229d..3c4f06077d9 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -27,6 +27,7 @@ use crate::rustc::ty::subst::Subst; use crate::rustc_errors::Applicability; use crate::rustc_data_structures::fx::{FxHashMap, FxHashSet}; use std::iter::{once, Iterator}; +use std::mem; use crate::syntax::ast; use crate::syntax::source_map::Span; use crate::syntax_pos::BytePos; @@ -1082,16 +1083,35 @@ fn check_for_loop_range<'a, 'tcx>( format!(".skip({})", snippet(cx, start.span, "..")) }; + let mut end_is_start_plus_val = false; + let take = if let Some(end) = *end { + let mut take_expr = end; + + if let ExprKind::Binary(ref op, ref left, ref right) = end.node { + if let BinOpKind::Add = op.node { + let start_equal_left = SpanlessEq::new(cx).eq_expr(start, left); + let start_equal_right = SpanlessEq::new(cx).eq_expr(start, right); + + if start_equal_left { + take_expr = right; + } else if start_equal_right { + take_expr = left; + } + + end_is_start_plus_val = start_equal_left | start_equal_right; + } + } + if is_len_call(end, indexed) { String::new() } else { match limits { ast::RangeLimits::Closed => { - let end = sugg::Sugg::hir(cx, end, ""); - format!(".take({})", end + sugg::ONE) + let take_expr = sugg::Sugg::hir(cx, take_expr, ""); + format!(".take({})", take_expr + sugg::ONE) }, - ast::RangeLimits::HalfOpen => format!(".take({})", snippet(cx, end.span, "..")), + ast::RangeLimits::HalfOpen => format!(".take({})", snippet(cx, take_expr.span, "..")), } } } else { @@ -1104,6 +1124,14 @@ fn check_for_loop_range<'a, 'tcx>( ("", "iter") }; + let take_is_empty = take.is_empty(); + let mut method_1 = take; + let mut method_2 = skip; + + if end_is_start_plus_val { + mem::swap(&mut method_1, &mut method_2); + } + if visitor.nonindex { span_lint_and_then( cx, @@ -1116,16 +1144,16 @@ fn check_for_loop_range<'a, 'tcx>( "consider using an iterator".to_string(), vec![ (pat.span, format!("({}, )", ident.name)), - (arg.span, format!("{}.{}().enumerate(){}{}", indexed, method, take, skip)), + (arg.span, format!("{}.{}().enumerate(){}{}", indexed, method, method_1, method_2)), ], ); }, ); } else { - let repl = if starts_at_zero && take.is_empty() { + let repl = if starts_at_zero && take_is_empty { format!("&{}{}", ref_mut, indexed) } else { - format!("{}.{}(){}{}", indexed, method, take, skip) + format!("{}.{}(){}{}", indexed, method, method_1, method_2) }; span_lint_and_then( diff --git a/tests/ui/author/for_loop.stderr b/tests/ui/author/for_loop.stderr new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/ui/needless_range_loop.rs b/tests/ui/needless_range_loop.rs index 44515502835..3da9267d38b 100644 --- a/tests/ui/needless_range_loop.rs +++ b/tests/ui/needless_range_loop.rs @@ -62,4 +62,18 @@ fn main() { g[i] = g[i+1..].iter().sum(); } assert_eq!(g, vec![20, 18, 15, 11, 6, 0]); + + let x = 5; + let mut vec = vec![0; 9]; + + for i in x..x + 4 { + vec[i] += 1; + } + + let x = 5; + let mut vec = vec![0; 10]; + + for i in x..=x + 4 { + vec[i] += 1; + } } diff --git a/tests/ui/needless_range_loop.stderr b/tests/ui/needless_range_loop.stderr index 64b1f3c08f7..d62a0434d0b 100644 --- a/tests/ui/needless_range_loop.stderr +++ b/tests/ui/needless_range_loop.stderr @@ -30,5 +30,25 @@ help: consider using an iterator 45 | for in &mut ms { | ^^^^^^ ^^^^^^^ -error: aborting due to 3 previous errors +error: the loop variable `i` is only used to index `vec`. + --> $DIR/needless_range_loop.rs:69:14 + | +69 | for i in x..x + 4 { + | ^^^^^^^^ +help: consider using an iterator + | +69 | for in vec.iter_mut().skip(x).take(4) { + | ^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: the loop variable `i` is only used to index `vec`. + --> $DIR/needless_range_loop.rs:76:14 + | +76 | for i in x..=x + 4 { + | ^^^^^^^^^ +help: consider using an iterator + | +76 | for in vec.iter_mut().skip(x).take(4 + 1) { + | ^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 5 previous errors diff --git a/tests/ui/ty_fn_sig.stderr b/tests/ui/ty_fn_sig.stderr new file mode 100644 index 00000000000..e69de29bb2d From 3ad9290ea43fdf995dbb06af850ff7dbf53af620 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Sun, 14 Oct 2018 23:41:35 -0700 Subject: [PATCH 32/50] Restore clippy_dummy's placeholder name Fixes #3317 --- clippy_dummy/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_dummy/Cargo.toml b/clippy_dummy/Cargo.toml index b53e89071ce..ed97cc45725 100644 --- a/clippy_dummy/Cargo.toml +++ b/clippy_dummy/Cargo.toml @@ -1,5 +1,5 @@ [package] -name = "clippy" # rename to clippy before publishing +name = "clippy_dummy" # rename to clippy before publishing version = "0.0.302" authors = ["Manish Goregaokar "] edition = "2018" From 4c88362a9dd166388bfd7508041145dfdfb965e6 Mon Sep 17 00:00:00 2001 From: Park Juhyung Date: Mon, 15 Oct 2018 22:32:49 +0900 Subject: [PATCH 33/50] Website: Make lint categories linkable Fixes #2973 --- util/gh-pages/index.html | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/util/gh-pages/index.html b/util/gh-pages/index.html index 656a7341257..277eeaf39f4 100644 --- a/util/gh-pages/index.html +++ b/util/gh-pages/index.html @@ -167,6 +167,19 @@ }); } + function selectGroup($scope, selectedGroup) { + var groups = $scope.groups; + for (var group in groups) { + if (groups.hasOwnProperty(group)) { + if (group === selectedGroup) { + groups[group] = true; + } else { + groups[group] = false; + } + } + } + } + angular.module("clippy", []) .filter('markdown', function ($sce) { return function (text) { @@ -223,6 +236,11 @@ return result; }, {}); + var selectedGroup = getQueryVariable("sel"); + if (selectedGroup) { + selectGroup($scope, selectedGroup.toLowerCase()); + } + scrollToLintByURL($scope); }) .error(function (data) { @@ -243,6 +261,17 @@ }, false); }); })(); + + function getQueryVariable(variable) { + var query = window.location.search.substring(1); + var vars = query.split('&'); + for (var i = 0; i < vars.length; i++) { + var pair = vars[i].split('='); + if (decodeURIComponent(pair[0]) == variable) { + return decodeURIComponent(pair[1]); + } + } + } From 2d8b4f3d5cb771a67f119f3903dd4aca1e4c9136 Mon Sep 17 00:00:00 2001 From: Bruno Kirschner Date: Mon, 15 Oct 2018 20:20:50 +0200 Subject: [PATCH 34/50] Avoid linting `boxed_local` on trait implementations. --- clippy_lints/src/escape.rs | 13 ++++++++++++- tests/ui/escape_analysis.rs | 34 +++++++++++++++++++++++++++------ tests/ui/escape_analysis.stderr | 16 ++++++++++++++++ 3 files changed, 56 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/escape.rs b/clippy_lints/src/escape.rs index 0491cde4fed..b7646dd6fdf 100644 --- a/clippy_lints/src/escape.rs +++ b/clippy_lints/src/escape.rs @@ -65,6 +65,7 @@ impl LintPass for Pass { } impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { + fn check_fn( &mut self, cx: &LateContext<'a, 'tcx>, @@ -74,13 +75,23 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { _: Span, node_id: NodeId, ) { - let fn_def_id = cx.tcx.hir.local_def_id(node_id); + // If the method is an impl for a trait, don't warn + let parent_id = cx.tcx.hir.get_parent(node_id); + let parent_node = cx.tcx.hir.find(parent_id); + + if let Some(Node::Item(item)) = parent_node { + if let ItemKind::Impl(_, _, _, _, Some(..), _, _) = item.node { + return; + } + } + let mut v = EscapeDelegate { cx, set: NodeSet(), too_large_for_stack: self.too_large_for_stack, }; + let fn_def_id = cx.tcx.hir.local_def_id(node_id); let region_scope_tree = &cx.tcx.region_scope_tree(fn_def_id); ExprUseVisitor::new(&mut v, cx.tcx, cx.param_env, region_scope_tree, cx.tables, None).consume_body(body); diff --git a/tests/ui/escape_analysis.rs b/tests/ui/escape_analysis.rs index 1f2f46b03cd..b35071546e7 100644 --- a/tests/ui/escape_analysis.rs +++ b/tests/ui/escape_analysis.rs @@ -7,12 +7,10 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. - #![feature(box_syntax)] -#![allow(warnings, clippy)] - -#![warn(boxed_local)] +#![allow(clippy::borrowed_box, clippy::needless_pass_by_value, clippy::unused_unit)] +#![warn(clippy::boxed_local)] #[derive(Clone)] struct A; @@ -70,8 +68,7 @@ fn warn_pass() { } fn nowarn_return() -> Box { - let fx = box A; - fx // moved out, "escapes" + box A // moved out, "escapes" } fn nowarn_move() { @@ -139,3 +136,28 @@ pub struct PeekableSeekable { pub fn new(_needs_name: Box>) -> () { } + +/// Regression for #916, #1123 +/// +/// This shouldn't warn for `boxed_local`as the implementation of a trait +/// can't change much about the trait definition. +trait BoxedAction { + fn do_sth(self: Box); +} + +impl BoxedAction for u64 { + fn do_sth(self: Box) { + println!("{}", *self) + } +} + +/// Regression for #1478 +/// +/// This shouldn't warn for `boxed_local`as self itself is a box type. +trait MyTrait { + fn do_sth(self); +} + +impl MyTrait for Box { + fn do_sth(self) {} +} diff --git a/tests/ui/escape_analysis.stderr b/tests/ui/escape_analysis.stderr index e69de29bb2d..25ba413b75a 100644 --- a/tests/ui/escape_analysis.stderr +++ b/tests/ui/escape_analysis.stderr @@ -0,0 +1,16 @@ +error: local variable doesn't need to be boxed here + --> $DIR/escape_analysis.rs:45:13 + | +45 | fn warn_arg(x: Box) { + | ^ + | + = note: `-D clippy::boxed-local` implied by `-D warnings` + +error: local variable doesn't need to be boxed here + --> $DIR/escape_analysis.rs:137:12 + | +137 | pub fn new(_needs_name: Box>) -> () { + | ^^^^^^^^^^^ + +error: aborting due to 2 previous errors + From 7da97a94dfe42768b38f6ba7dc5804cf8e5821fb Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Tue, 9 Oct 2018 08:08:50 +0200 Subject: [PATCH 35/50] Use `WalkDir` to also gather from subdirectories `fs::read_dir` does not recurse into subdirectories. --- clippy_dev/Cargo.toml | 1 + clippy_dev/src/lib.rs | 9 +++++---- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/clippy_dev/Cargo.toml b/clippy_dev/Cargo.toml index d6057ba970c..5380ecd9814 100644 --- a/clippy_dev/Cargo.toml +++ b/clippy_dev/Cargo.toml @@ -9,3 +9,4 @@ clap = "~2.32" itertools = "0.7" regex = "1" lazy_static = "1.0" +walkdir = "2" diff --git a/clippy_dev/src/lib.rs b/clippy_dev/src/lib.rs index eee9089e7c4..d312eadf89e 100644 --- a/clippy_dev/src/lib.rs +++ b/clippy_dev/src/lib.rs @@ -14,6 +14,7 @@ use itertools::Itertools; use lazy_static::lazy_static; use regex::Regex; +use walkdir::WalkDir; use std::collections::HashMap; use std::ffi::OsStr; use std::fs; @@ -70,7 +71,7 @@ pub fn gather_all() -> impl Iterator { lint_files().flat_map(|f| gather_from_file(&f)) } -fn gather_from_file(dir_entry: &fs::DirEntry) -> impl Iterator { +fn gather_from_file(dir_entry: &walkdir::DirEntry) -> impl Iterator { let mut file = fs::File::open(dir_entry.path()).unwrap(); let mut content = String::new(); file.read_to_string(&mut content).unwrap(); @@ -89,9 +90,9 @@ fn parse_contents(content: &str, filename: &str) -> impl Iterator { } /// Collects all .rs files in the `clippy_lints/src` directory -fn lint_files() -> impl Iterator { - fs::read_dir("../clippy_lints/src") - .unwrap() +fn lint_files() -> impl Iterator { + WalkDir::new("../clippy_lints/src") + .into_iter() .filter_map(|f| f.ok()) .filter(|f| f.path().extension() == Some(OsStr::new("rs"))) } From fb830c53db356b22a2635ed50d0698fafe310321 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Mon, 15 Oct 2018 20:47:19 +0200 Subject: [PATCH 36/50] Some more documentation for clippy_dev --- clippy_dev/src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/clippy_dev/src/lib.rs b/clippy_dev/src/lib.rs index d312eadf89e..1bd9bffff06 100644 --- a/clippy_dev/src/lib.rs +++ b/clippy_dev/src/lib.rs @@ -36,6 +36,7 @@ lazy_static! { pub static ref DOCS_LINK: String = "https://rust-lang-nursery.github.io/rust-clippy/master/index.html".to_string(); } +/// Lint data parsed from the Clippy source code. #[derive(Clone, PartialEq, Debug)] pub struct Lint { pub name: String, @@ -67,6 +68,7 @@ impl Lint { } } +/// Gathers all files in `src/clippy_lints` and gathers all lints inside pub fn gather_all() -> impl Iterator { lint_files().flat_map(|f| gather_from_file(&f)) } From b61ca63c5e91d8659af239e658bf313894aebd23 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Mon, 15 Oct 2018 21:02:38 +0200 Subject: [PATCH 37/50] sort_by -> sort_by_key --- clippy_dev/src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_dev/src/main.rs b/clippy_dev/src/main.rs index d1161323b02..9e78def78fe 100644 --- a/clippy_dev/src/main.rs +++ b/clippy_dev/src/main.rs @@ -44,7 +44,7 @@ fn print_lints() { if lint_group == "Deprecated" { continue; } println!("\n## {}", lint_group); - lints.sort_by(|a, b| a.name.cmp(&b.name)); + lints.sort_by_key(|l| l.name.clone()); for lint in lints { println!("* [{}]({}#{}) ({})", lint.name, clippy_dev::DOCS_LINK.clone(), lint.name, lint.desc); From b5dd8f17d12ec6ccc96910c07f930e340fafeb3b Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Mon, 15 Oct 2018 21:10:22 +0200 Subject: [PATCH 38/50] Add comment on WalkDir vs. fs::read_dir --- clippy_dev/src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/clippy_dev/src/lib.rs b/clippy_dev/src/lib.rs index 1bd9bffff06..8477183ae56 100644 --- a/clippy_dev/src/lib.rs +++ b/clippy_dev/src/lib.rs @@ -93,6 +93,8 @@ fn parse_contents(content: &str, filename: &str) -> impl Iterator { /// Collects all .rs files in the `clippy_lints/src` directory fn lint_files() -> impl Iterator { + // We use `WalkDir` instead of `fs::read_dir` here in order to recurse into subdirectories. + // Otherwise we would not collect all the lints, for example in `clippy_lints/src/methods/`. WalkDir::new("../clippy_lints/src") .into_iter() .filter_map(|f| f.ok()) From af441b5b072eb01c939d2e92a3496dbedf6d412b Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Tue, 16 Oct 2018 07:24:32 +0200 Subject: [PATCH 39/50] Rename `active_lints` to `usable_lints` Because now `usable_lints` will also exclude internal lints. --- clippy_dev/src/lib.rs | 14 ++++++++------ clippy_dev/src/main.rs | 8 +++++--- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/clippy_dev/src/lib.rs b/clippy_dev/src/lib.rs index 8477183ae56..d4b74f55717 100644 --- a/clippy_dev/src/lib.rs +++ b/clippy_dev/src/lib.rs @@ -57,9 +57,9 @@ impl Lint { } } - /// Returns all non-deprecated lints - pub fn active_lints(lints: impl Iterator) -> impl Iterator { - lints.filter(|l| l.deprecation.is_none()) + /// Returns all non-deprecated lints and non-internal lints + pub fn usable_lints(lints: impl Iterator) -> impl Iterator { + lints.filter(|l| l.deprecation.is_none() && !l.group.starts_with("internal")) } /// Returns the lints in a HashMap, grouped by the different lint groups @@ -141,15 +141,17 @@ declare_deprecated_lint! { } #[test] -fn test_active_lints() { +fn test_usable_lints() { let lints = vec![ Lint::new("should_assert_eq", "Deprecated", "abc", Some("Reason"), "module_name"), - Lint::new("should_assert_eq2", "Not Deprecated", "abc", None, "module_name") + Lint::new("should_assert_eq2", "Not Deprecated", "abc", None, "module_name"), + Lint::new("should_assert_eq2", "internal", "abc", None, "module_name"), + Lint::new("should_assert_eq2", "internal_style", "abc", None, "module_name") ]; let expected = vec![ Lint::new("should_assert_eq2", "Not Deprecated", "abc", None, "module_name") ]; - assert_eq!(expected, Lint::active_lints(lints.into_iter()).collect::>()); + assert_eq!(expected, Lint::usable_lints(lints.into_iter()).collect::>()); } #[test] diff --git a/clippy_dev/src/main.rs b/clippy_dev/src/main.rs index 9e78def78fe..ff7d47366ae 100644 --- a/clippy_dev/src/main.rs +++ b/clippy_dev/src/main.rs @@ -37,8 +37,10 @@ fn main() { } fn print_lints() { - let lint_list = gather_all().collect::>(); - let grouped_by_lint_group = Lint::by_lint_group(&lint_list); + let lint_list = gather_all(); + let usable_lints: Vec = Lint::usable_lints(lint_list).collect(); + let lint_count = usable_lints.len(); + let grouped_by_lint_group = Lint::by_lint_group(&usable_lints); for (lint_group, mut lints) in grouped_by_lint_group { if lint_group == "Deprecated" { continue; } @@ -51,5 +53,5 @@ fn print_lints() { } } - println!("there are {} lints", Lint::active_lints(lint_list.into_iter()).count()); + println!("there are {} lints", lint_count); } From 956987f43e3012c3487973cc0dd47f7c4eaa7942 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Tue, 16 Oct 2018 08:00:31 +0200 Subject: [PATCH 40/50] RIIR update_lints: Replace lint count in README.md This allows the usage of `util/dev update_lints` which will write the new lint_count to the `README.md`. --- clippy_dev/src/lib.rs | 119 +++++++++++++++++++++++++++++++++++++++++ clippy_dev/src/main.rs | 20 +++++++ 2 files changed, 139 insertions(+) diff --git a/clippy_dev/src/lib.rs b/clippy_dev/src/lib.rs index d4b74f55717..1c303d180d2 100644 --- a/clippy_dev/src/lib.rs +++ b/clippy_dev/src/lib.rs @@ -101,6 +101,88 @@ fn lint_files() -> impl Iterator { .filter(|f| f.path().extension() == Some(OsStr::new("rs"))) } +/// Replace a region in a file delimited by two lines matching regexes. +/// +/// `path` is the relative path to the file on which you want to perform the replacement. +/// +/// See `replace_region_in_text` for documentation of the other options. +pub fn replace_region_in_file(path: &str, start: &str, end: &str, replace_start: bool, replacements: F) where F: Fn() -> Vec { + let mut f = fs::File::open(path).expect(&format!("File not found: {}", path)); + let mut contents = String::new(); + f.read_to_string(&mut contents).expect("Something went wrong reading the file"); + let replaced = replace_region_in_text(&contents, start, end, replace_start, replacements); + + let mut f = fs::File::create(path).expect(&format!("File not found: {}", path)); + f.write_all(replaced.as_bytes()).expect("Unable to write file"); + // Ensure we write the changes with a trailing newline so that + // the file has the proper line endings. + f.write(b"\n").expect("Unable to write file"); +} + +/// Replace a region in a text delimited by two lines matching regexes. +/// +/// * `text` is the input text on which you want to perform the replacement +/// * `start` is a `&str` that describes the delimiter line before the region you want to replace. +/// As the `&str` will be converted to a `Regex`, this can contain regex syntax, too. +/// * `end` is a `&str` that describes the delimiter line until where the replacement should +/// happen. As the `&str` will be converted to a `Regex`, this can contain regex syntax, too. +/// * If `replace_start` is true, the `start` delimiter line is replaced as well. +/// The `end` delimiter line is never replaced. +/// * `replacements` is a closure that has to return a `Vec` which contains the new text. +/// +/// If you want to perform the replacement on files instead of already parsed text, +/// use `replace_region_in_file`. +/// +/// # Example +/// +/// ``` +/// let the_text = "replace_start\nsome text\nthat will be replaced\nreplace_end"; +/// let result = clippy_dev::replace_region_in_text( +/// the_text, +/// r#"replace_start"#, +/// r#"replace_end"#, +/// false, +/// || { +/// vec!["a different".to_string(), "text".to_string()] +/// } +/// ); +/// assert_eq!("replace_start\na different\ntext\nreplace_end", result); +/// ``` +pub fn replace_region_in_text(text: &str, start: &str, end: &str, replace_start: bool, replacements: F) -> String where F: Fn() -> Vec { + let lines = text.lines(); + let mut in_old_region = false; + let mut found = false; + let mut new_lines = vec![]; + let start = Regex::new(start).unwrap(); + let end = Regex::new(end).unwrap(); + + for line in lines { + if in_old_region { + if end.is_match(&line) { + in_old_region = false; + new_lines.extend(replacements()); + new_lines.push(line.to_string()); + } + } else if start.is_match(&line) { + if !replace_start { + new_lines.push(line.to_string()); + } + in_old_region = true; + found = true; + } else { + new_lines.push(line.to_string()); + } + } + + if !found { + // This happens if the provided regex in `clippy_dev/src/main.rs` is not found in the + // given text or file. Most likely this is an error on the programmer's side and the Regex + // is incorrect. + println!("regex {:?} not found. You may have to update it.", start); + } + new_lines.join("\n") +} + #[test] fn test_parse_contents() { let result: Vec = parse_contents( @@ -140,6 +222,43 @@ declare_deprecated_lint! { assert_eq!(expected, result); } +#[test] +fn test_replace_region() { + let text = r#" +abc +123 +789 +def +ghi"#; + let expected = r#" +abc +hello world +def +ghi"#; + let result = replace_region_in_text(text, r#"^\s*abc$"#, r#"^\s*def"#, false, || { + vec!["hello world".to_string()] + }); + assert_eq!(expected, result); +} + +#[test] +fn test_replace_region_with_start() { + let text = r#" +abc +123 +789 +def +ghi"#; + let expected = r#" +hello world +def +ghi"#; + let result = replace_region_in_text(text, r#"^\s*abc$"#, r#"^\s*def"#, true, || { + vec!["hello world".to_string()] + }); + assert_eq!(expected, result); +} + #[test] fn test_usable_lints() { let lints = vec![ diff --git a/clippy_dev/src/main.rs b/clippy_dev/src/main.rs index ff7d47366ae..7b688836a95 100644 --- a/clippy_dev/src/main.rs +++ b/clippy_dev/src/main.rs @@ -32,6 +32,8 @@ fn main() { if let Some(matches) = matches.subcommand_matches("update_lints") { if matches.is_present("print-only") { print_lints(); + } else { + update_lints(); } } } @@ -55,3 +57,21 @@ fn print_lints() { println!("there are {} lints", lint_count); } + +fn update_lints() { + let lint_list = gather_all(); + let usable_lints: Vec = Lint::usable_lints(lint_list).collect(); + let lint_count = usable_lints.len(); + + replace_region_in_file( + "../README.md", + r#"\[There are \d+ lints included in this crate!\]\(https://rust-lang-nursery.github.io/rust-clippy/master/index.html\)"#, + "", + true, + || { + vec![ + format!("[There are {} lints included in this crate!](https://rust-lang-nursery.github.io/rust-clippy/master/index.html)", lint_count) + ] + } + ); +} From 33847b579eb5479f888a0caae37ebc469dd9ccd2 Mon Sep 17 00:00:00 2001 From: Joshua Holmer Date: Tue, 16 Oct 2018 09:04:02 -0400 Subject: [PATCH 41/50] Update known problems for unnecessary_fold --- clippy_lints/src/methods/mod.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 7c15eb677cc..6fc563a42ea 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -688,7 +688,8 @@ declare_clippy_lint! { /// /// **Why is this bad?** Readability. /// -/// **Known problems:** None. +/// **Known problems:** False positive in pattern guards. Will be resolved once +/// non-lexical lifetimes are stable. /// /// **Example:** /// ```rust From 05ffc2d05743c0cc4ad5e551c147dc8385e67bac Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Tue, 16 Oct 2018 20:58:00 +0200 Subject: [PATCH 42/50] Fix dogfood `expect_fun_call` causes a false-positive, so I disabled it for now. --- clippy_dev/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/clippy_dev/src/lib.rs b/clippy_dev/src/lib.rs index 1c303d180d2..dcb2de2b1f8 100644 --- a/clippy_dev/src/lib.rs +++ b/clippy_dev/src/lib.rs @@ -106,6 +106,7 @@ fn lint_files() -> impl Iterator { /// `path` is the relative path to the file on which you want to perform the replacement. /// /// See `replace_region_in_text` for documentation of the other options. +#[allow(clippy::expect_fun_call)] pub fn replace_region_in_file(path: &str, start: &str, end: &str, replace_start: bool, replacements: F) where F: Fn() -> Vec { let mut f = fs::File::open(path).expect(&format!("File not found: {}", path)); let mut contents = String::new(); @@ -116,7 +117,7 @@ pub fn replace_region_in_file(path: &str, start: &str, end: &str, replace_sta f.write_all(replaced.as_bytes()).expect("Unable to write file"); // Ensure we write the changes with a trailing newline so that // the file has the proper line endings. - f.write(b"\n").expect("Unable to write file"); + f.write_all(b"\n").expect("Unable to write file"); } /// Replace a region in a text delimited by two lines matching regexes. From 8c902d1cf263c38416ccf4bf48d143d40c75f8db Mon Sep 17 00:00:00 2001 From: Owen Sanchez Date: Mon, 15 Oct 2018 21:12:59 -0700 Subject: [PATCH 43/50] Simplify manual_memcpy suggestion in some cases --- clippy_lints/src/loops.rs | 16 ++++++++++++++-- tests/ui/for_loop.rs | 13 +++++++++++++ tests/ui/for_loop.stderr | 26 +++++++++++++++++++------- tests/ui/for_loop.stdout | 0 4 files changed, 46 insertions(+), 9 deletions(-) create mode 100644 tests/ui/for_loop.stdout diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 3c4f06077d9..4c505ababf1 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -950,8 +950,20 @@ fn detect_manual_memcpy<'a, 'tcx>( ("0", _, x, false) | (x, false, "0", false) => x.into(), ("0", _, x, true) | (x, false, "0", true) => format!("-{}", x), (x, false, y, false) => format!("({} + {})", x, y), - (x, false, y, true) => format!("({} - {})", x, y), - (x, true, y, false) => format!("({} - {})", y, x), + (x, false, y, true) => { + if x == y { + "0".into() + } else { + format!("({} - {})", x, y) + } + }, + (x, true, y, false) => { + if x == y { + "0".into() + } else { + format!("({} - {})", y, x) + } + }, (x, true, y, true) => format!("-({} + {})", x, y), } }; diff --git a/tests/ui/for_loop.rs b/tests/ui/for_loop.rs index 89c452f44df..f80270d9fe8 100644 --- a/tests/ui/for_loop.rs +++ b/tests/ui/for_loop.rs @@ -550,6 +550,19 @@ pub fn manual_copy(src: &[i32], dst: &mut [i32], dst2: &mut [i32]) { for i in 0..10 { dst_vec[i] = src[i]; } + + // Simplify suggestion (issue #3004) + let src = [0, 1, 2, 3, 4]; + let mut dst = [0, 0, 0, 0, 0, 0]; + let from = 1; + + for i in from..from + src.len() { + dst[i] = src[i - from]; + } + + for i in from..from + 3 { + dst[i] = src[i - from]; + } } #[warn(clippy::needless_range_loop)] diff --git a/tests/ui/for_loop.stderr b/tests/ui/for_loop.stderr index 472fa148609..33176335783 100644 --- a/tests/ui/for_loop.stderr +++ b/tests/ui/for_loop.stderr @@ -482,22 +482,34 @@ error: it looks like you're manually copying between slices | ^^^^^^^^^^^^^^^^ 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:557:14 + --> $DIR/for_loop.rs:559:14 | -557 | for i in 0..src.len() { +559 | 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 + | +563 | 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 + | +570 | 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:618:19 + --> $DIR/for_loop.rs:631:19 | -618 | for ch in text.chars() { +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:629:19 + --> $DIR/for_loop.rs:642:19 | -629 | for ch in text.chars() { +642 | for ch in text.chars() { | ^^^^^^^^^^^^ -error: aborting due to 61 previous errors +error: aborting due to 63 previous errors diff --git a/tests/ui/for_loop.stdout b/tests/ui/for_loop.stdout new file mode 100644 index 00000000000..e69de29bb2d From aa88e68902663db1bc5e5aa93c7884e405dd6b32 Mon Sep 17 00:00:00 2001 From: Giorgio Gambino Date: Tue, 16 Oct 2018 23:23:31 +0200 Subject: [PATCH 44/50] Fix issue #3322: reword help message for len_zero --- clippy_lints/src/len_zero.rs | 2 +- tests/ui/len_zero.stderr | 28 ++++++++++++++-------------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/len_zero.rs b/clippy_lints/src/len_zero.rs index 1dd775051d9..789a569f4cd 100644 --- a/clippy_lints/src/len_zero.rs +++ b/clippy_lints/src/len_zero.rs @@ -240,7 +240,7 @@ fn check_len(cx: &LateContext<'_, '_>, span: Span, method_name: Name, args: &[Ex LEN_ZERO, span, &format!("length comparison to {}", if compare_to == 0 { "zero" } else { "one" }), - "using `is_empty` is more concise", + "using `is_empty` is clearer and more explicit", format!("{}{}.is_empty()", op, snippet(cx, args[0].span, "_")), ); } diff --git a/tests/ui/len_zero.stderr b/tests/ui/len_zero.stderr index ffba33a65f8..1f937bafdef 100644 --- a/tests/ui/len_zero.stderr +++ b/tests/ui/len_zero.stderr @@ -46,7 +46,7 @@ error: length comparison to zero --> $DIR/len_zero.rs:151:8 | 151 | if x.len() == 0 { - | ^^^^^^^^^^^^ help: using `is_empty` is more concise: `x.is_empty()` + | ^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `x.is_empty()` | = note: `-D clippy::len-zero` implied by `-D warnings` @@ -54,79 +54,79 @@ error: length comparison to zero --> $DIR/len_zero.rs:155:8 | 155 | if "".len() == 0 {} - | ^^^^^^^^^^^^^ help: using `is_empty` is more concise: `"".is_empty()` + | ^^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `"".is_empty()` error: length comparison to zero --> $DIR/len_zero.rs:170:8 | 170 | if has_is_empty.len() == 0 { - | ^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `has_is_empty.is_empty()` + | ^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `has_is_empty.is_empty()` error: length comparison to zero --> $DIR/len_zero.rs:173:8 | 173 | if has_is_empty.len() != 0 { - | ^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `!has_is_empty.is_empty()` + | ^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `!has_is_empty.is_empty()` error: length comparison to zero --> $DIR/len_zero.rs:176:8 | 176 | if has_is_empty.len() > 0 { - | ^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `!has_is_empty.is_empty()` + | ^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `!has_is_empty.is_empty()` error: length comparison to one --> $DIR/len_zero.rs:179:8 | 179 | if has_is_empty.len() < 1 { - | ^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `has_is_empty.is_empty()` + | ^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `has_is_empty.is_empty()` error: length comparison to one --> $DIR/len_zero.rs:182:8 | 182 | if has_is_empty.len() >= 1 { - | ^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `!has_is_empty.is_empty()` + | ^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `!has_is_empty.is_empty()` error: length comparison to zero --> $DIR/len_zero.rs:193:8 | 193 | if 0 == has_is_empty.len() { - | ^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `has_is_empty.is_empty()` + | ^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `has_is_empty.is_empty()` error: length comparison to zero --> $DIR/len_zero.rs:196:8 | 196 | if 0 != has_is_empty.len() { - | ^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `!has_is_empty.is_empty()` + | ^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `!has_is_empty.is_empty()` error: length comparison to zero --> $DIR/len_zero.rs:199:8 | 199 | if 0 < has_is_empty.len() { - | ^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `!has_is_empty.is_empty()` + | ^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `!has_is_empty.is_empty()` error: length comparison to one --> $DIR/len_zero.rs:202:8 | 202 | if 1 <= has_is_empty.len() { - | ^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `!has_is_empty.is_empty()` + | ^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `!has_is_empty.is_empty()` error: length comparison to one --> $DIR/len_zero.rs:205:8 | 205 | if 1 > has_is_empty.len() { - | ^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `has_is_empty.is_empty()` + | ^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `has_is_empty.is_empty()` error: length comparison to zero --> $DIR/len_zero.rs:219:8 | 219 | if with_is_empty.len() == 0 { - | ^^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `with_is_empty.is_empty()` + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `with_is_empty.is_empty()` error: length comparison to zero --> $DIR/len_zero.rs:232:8 | 232 | if b.len() != 0 {} - | ^^^^^^^^^^^^ help: using `is_empty` is more concise: `!b.is_empty()` + | ^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `!b.is_empty()` error: trait `DependsOnFoo` has a `len` method but no (possibly inherited) `is_empty` method --> $DIR/len_zero.rs:238:1 From 3b7c88888bb9b01eb9ca8f40f59b48006462f2b5 Mon Sep 17 00:00:00 2001 From: CYBAI Date: Sun, 7 Oct 2018 21:36:42 +0800 Subject: [PATCH 45/50] Add lint for redundant pattern matching for explicit return boolean --- .../src/if_let_redundant_pattern_matching.rs | 198 ++++++++++++++---- tests/ui/if_let_redundant_pattern_matching.rs | 30 +++ .../if_let_redundant_pattern_matching.stderr | 56 ++++- 3 files changed, 244 insertions(+), 40 deletions(-) diff --git a/clippy_lints/src/if_let_redundant_pattern_matching.rs b/clippy_lints/src/if_let_redundant_pattern_matching.rs index bced0c9552d..6f786fc5659 100644 --- a/clippy_lints/src/if_let_redundant_pattern_matching.rs +++ b/clippy_lints/src/if_let_redundant_pattern_matching.rs @@ -11,6 +11,8 @@ use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; use crate::rustc::{declare_tool_lint, lint_array}; use crate::rustc::hir::*; +use crate::syntax::ptr::P; +use crate::syntax::ast::LitKind; use crate::utils::{match_qpath, paths, snippet, span_lint_and_then}; use crate::rustc_errors::Applicability; @@ -58,46 +60,164 @@ impl LintPass for Pass { impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { #[allow(clippy::similar_names)] fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { - if let ExprKind::Match(ref op, ref arms, MatchSource::IfLetDesugar { .. }) = expr.node { - if arms[0].pats.len() == 1 { - let good_method = match arms[0].pats[0].node { - PatKind::TupleStruct(ref path, ref pats, _) if pats.len() == 1 => { - if let PatKind::Wild = pats[0].node { - if match_qpath(path, &paths::RESULT_OK) { - "is_ok()" - } else if match_qpath(path, &paths::RESULT_ERR) { - "is_err()" - } else if match_qpath(path, &paths::OPTION_SOME) { - "is_some()" - } else { - return; - } - } else { - return; - } - }, - - PatKind::Path(ref path) if match_qpath(path, &paths::OPTION_NONE) => "is_none()", - - _ => return, - }; - - span_lint_and_then( - cx, - IF_LET_REDUNDANT_PATTERN_MATCHING, - arms[0].pats[0].span, - &format!("redundant pattern matching, consider using `{}`", good_method), - |db| { - let span = expr.span.to(op.span); - db.span_suggestion_with_applicability( - span, - "try this", - format!("if {}.{}", snippet(cx, op.span, "_"), good_method), - Applicability::MachineApplicable, // snippet - ); - }, - ); + if let ExprKind::Match(ref op, ref arms, ref match_source) = expr.node { + match match_source { + MatchSource::Normal => find_sugg_for_match(cx, expr, op, arms), + MatchSource::IfLetDesugar { .. } => find_sugg_for_if_let(cx, expr, op, arms), + _ => return, } } } } + +fn find_sugg_for_if_let<'a, 'tcx>( + cx: &LateContext<'a, 'tcx>, + expr: &'tcx Expr, + op: &P, + arms: &HirVec +) { + if arms[0].pats.len() == 1 { + let good_method = match arms[0].pats[0].node { + PatKind::TupleStruct(ref path, ref pats, _) if pats.len() == 1 => { + if let PatKind::Wild = pats[0].node { + if match_qpath(path, &paths::RESULT_OK) { + "is_ok()" + } else if match_qpath(path, &paths::RESULT_ERR) { + "is_err()" + } else if match_qpath(path, &paths::OPTION_SOME) { + "is_some()" + } else { + return; + } + } else { + return; + } + }, + + PatKind::Path(ref path) if match_qpath(path, &paths::OPTION_NONE) => "is_none()", + + _ => return, + }; + + span_lint_and_then( + cx, + IF_LET_REDUNDANT_PATTERN_MATCHING, + arms[0].pats[0].span, + &format!("redundant pattern matching, consider using `{}`", good_method), + |db| { + let span = expr.span.to(op.span); + db.span_suggestion_with_applicability( + span, + "try this", + format!("if {}.{}", snippet(cx, op.span, "_"), good_method), + Applicability::MachineApplicable, // snippet + ); + }, + ); + } else { + return; + } +} + +fn find_sugg_for_match<'a, 'tcx>( + cx: &LateContext<'a, 'tcx>, + expr: &'tcx Expr, + op: &P, + arms: &HirVec +) { + if arms.len() == 2 { + let node_pair = (&arms[0].pats[0].node, &arms[1].pats[0].node); + + let found_good_method = match node_pair { + ( + PatKind::TupleStruct(ref path_left, ref pats_left, _), + PatKind::TupleStruct(ref path_right, ref pats_right, _) + ) if pats_left.len() == 1 && pats_right.len() == 1 => { + if let (PatKind::Wild, PatKind::Wild) = (&pats_left[0].node, &pats_right[0].node) { + find_good_method_for_match( + arms, + path_left, + path_right, + &paths::RESULT_OK, + &paths::RESULT_ERR, + "is_ok()", + "is_err()" + ) + } else { + None + } + }, + ( + PatKind::TupleStruct(ref path_left, ref pats, _), + PatKind::Path(ref path_right) + ) | ( + PatKind::Path(ref path_left), + PatKind::TupleStruct(ref path_right, ref pats, _) + ) if pats.len() == 1 => { + if let PatKind::Wild = pats[0].node { + find_good_method_for_match( + arms, + path_left, + path_right, + &paths::OPTION_SOME, + &paths::OPTION_NONE, + "is_some()", + "is_none()" + ) + } else { + None + } + }, + _ => None, + }; + + if let Some(good_method) = found_good_method { + span_lint_and_then( + cx, + IF_LET_REDUNDANT_PATTERN_MATCHING, + expr.span, + &format!("redundant pattern matching, consider using `{}`", good_method), + |db| { + let span = expr.span.to(op.span); + db.span_suggestion_with_applicability( + span, + "try this", + format!("{}.{}", snippet(cx, op.span, "_"), good_method), + Applicability::MachineApplicable, // snippet + ); + }, + ); + } + } else { + return; + } +} + +fn find_good_method_for_match<'a>( + arms: &HirVec, + path_left: &QPath, + path_right: &QPath, + expected_left: &[&str], + expected_right: &[&str], + should_be_left: &'a str, + should_be_right: &'a str +) -> Option<&'a str> { + let body_node_pair = if match_qpath(path_left, expected_left) && match_qpath(path_right, expected_right) { + (&(*arms[0].body).node, &(*arms[1].body).node) + } else if match_qpath(path_right, expected_left) && match_qpath(path_left, expected_right) { + (&(*arms[1].body).node, &(*arms[0].body).node) + } else { + return None; + }; + + match body_node_pair { + (ExprKind::Lit(ref lit_left), ExprKind::Lit(ref lit_right)) => { + match (&lit_left.node, &lit_right.node) { + (LitKind::Bool(true), LitKind::Bool(false)) => Some(should_be_left), + (LitKind::Bool(false), LitKind::Bool(true)) => Some(should_be_right), + _ => None, + } + }, + _ => None, + } +} diff --git a/tests/ui/if_let_redundant_pattern_matching.rs b/tests/ui/if_let_redundant_pattern_matching.rs index 1c0e7e79c68..3f7d0c8e1bd 100644 --- a/tests/ui/if_let_redundant_pattern_matching.rs +++ b/tests/ui/if_let_redundant_pattern_matching.rs @@ -42,4 +42,34 @@ fn main() { if let Ok(x) = Ok::(42) { println!("{}", x); } + + match Ok::(42) { + Ok(_) => true, + Err(_) => false, + }; + + match Ok::(42) { + Ok(_) => false, + Err(_) => true, + }; + + match Err::(42) { + Ok(_) => false, + Err(_) => true, + }; + + match Err::(42) { + Ok(_) => true, + Err(_) => false, + }; + + match Some(42) { + Some(_) => true, + None => false, + }; + + match None::<()> { + Some(_) => false, + None => true, + }; } diff --git a/tests/ui/if_let_redundant_pattern_matching.stderr b/tests/ui/if_let_redundant_pattern_matching.stderr index 5111de67189..93bafa7fcbd 100644 --- a/tests/ui/if_let_redundant_pattern_matching.stderr +++ b/tests/ui/if_let_redundant_pattern_matching.stderr @@ -30,5 +30,59 @@ error: redundant pattern matching, consider using `is_some()` 28 | | } | |_____- help: try this: `if Some(42).is_some()` -error: aborting due to 4 previous errors +error: redundant pattern matching, consider using `is_ok()` + --> $DIR/if_let_redundant_pattern_matching.rs:46:5 + | +46 | / match Ok::(42) { +47 | | Ok(_) => true, +48 | | Err(_) => false, +49 | | }; + | |_____^ help: try this: `Ok::(42).is_ok()` + +error: redundant pattern matching, consider using `is_err()` + --> $DIR/if_let_redundant_pattern_matching.rs:51:5 + | +51 | / match Ok::(42) { +52 | | Ok(_) => false, +53 | | Err(_) => true, +54 | | }; + | |_____^ help: try this: `Ok::(42).is_err()` + +error: redundant pattern matching, consider using `is_err()` + --> $DIR/if_let_redundant_pattern_matching.rs:56:5 + | +56 | / match Err::(42) { +57 | | Ok(_) => false, +58 | | Err(_) => true, +59 | | }; + | |_____^ help: try this: `Err::(42).is_err()` + +error: redundant pattern matching, consider using `is_ok()` + --> $DIR/if_let_redundant_pattern_matching.rs:61:5 + | +61 | / match Err::(42) { +62 | | Ok(_) => true, +63 | | Err(_) => false, +64 | | }; + | |_____^ help: try this: `Err::(42).is_ok()` + +error: redundant pattern matching, consider using `is_some()` + --> $DIR/if_let_redundant_pattern_matching.rs:66:5 + | +66 | / match Some(42) { +67 | | Some(_) => true, +68 | | None => false, +69 | | }; + | |_____^ help: try this: `Some(42).is_some()` + +error: redundant pattern matching, consider using `is_none()` + --> $DIR/if_let_redundant_pattern_matching.rs:71:5 + | +71 | / match None::<()> { +72 | | Some(_) => false, +73 | | None => true, +74 | | }; + | |_____^ help: try this: `None::<()>.is_none()` + +error: aborting due to 10 previous errors From 66ae3b124949d07c2a50e051b166b93029ecc4ca Mon Sep 17 00:00:00 2001 From: CYBAI Date: Wed, 10 Oct 2018 23:13:53 +0800 Subject: [PATCH 46/50] Rename if_let_redundant_pattern_matching to redundant_pattern_matching Also, making the old one deprecated --- CHANGELOG.md | 1 + clippy_lints/src/deprecated_lints.rs | 12 +++++++++- clippy_lints/src/lib.rs | 12 ++++++---- ...ching.rs => redundant_pattern_matching.rs} | 13 +++++++---- tests/ui/matches.rs | 2 +- tests/ui/needless_pass_by_value.rs | 2 +- ...ching.rs => redundant_pattern_matching.rs} | 2 +- ...derr => redundant_pattern_matching.stderr} | 22 +++++++++---------- 8 files changed, 43 insertions(+), 23 deletions(-) rename clippy_lints/src/{if_let_redundant_pattern_matching.rs => redundant_pattern_matching.rs} (96%) rename tests/ui/{if_let_redundant_pattern_matching.rs => redundant_pattern_matching.rs} (96%) rename tests/ui/{if_let_redundant_pattern_matching.stderr => redundant_pattern_matching.stderr} (78%) diff --git a/CHANGELOG.md b/CHANGELOG.md index e6792c06894..626c39457e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -816,6 +816,7 @@ All notable changes to this project will be documented in this file. [`redundant_closure_call`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#redundant_closure_call [`redundant_field_names`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#redundant_field_names [`redundant_pattern`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#redundant_pattern +[`redundant_pattern_matching`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#redundant_pattern_matching [`ref_in_deref`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#ref_in_deref [`regex_macro`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#regex_macro [`replace_consts`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#replace_consts diff --git a/clippy_lints/src/deprecated_lints.rs b/clippy_lints/src/deprecated_lints.rs index 0067629bbd0..904036fe888 100644 --- a/clippy_lints/src/deprecated_lints.rs +++ b/clippy_lints/src/deprecated_lints.rs @@ -16,7 +16,7 @@ macro_rules! declare_deprecated_lint { /// **What it does:** Nothing. This lint has been deprecated. /// -/// **Deprecation reason:** This used to check for `assert!(a == b)` and recommend +/// **Deprecation reason:** This used to check for `assert!(a == b)` and recommend /// replacement with `assert_eq!(a, b)`, but this is no longer needed after RFC 2011. declare_deprecated_lint! { pub SHOULD_ASSERT_EQ, @@ -102,3 +102,13 @@ declare_deprecated_lint! { pub ASSIGN_OPS, "using compound assignment operators (e.g. `+=`) is harmless" } + +/// **What it does:** Nothing. This lint has been deprecated. +/// +/// **Deprecation reason:** The original rule will only lint for `if let`. After +/// making it support to lint `match`, naming as `if let` is not suitable for it. +/// So, this lint is deprecated. +declare_deprecated_lint! { + pub IF_LET_REDUNDANT_PATTERN_MATCHING, + "this lint has been changed to redundant_pattern_matching" +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 35c89e4efde..23bd71a08ab 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -126,7 +126,6 @@ pub mod formatting; pub mod functions; pub mod identity_conversion; pub mod identity_op; -pub mod if_let_redundant_pattern_matching; pub mod if_not_else; pub mod indexing_slicing; pub mod infallible_destructuring_match; @@ -180,6 +179,7 @@ pub mod ptr_offset_with_cast; pub mod question_mark; pub mod ranges; pub mod redundant_field_names; +pub mod redundant_pattern_matching; pub mod reference; pub mod regex; pub mod replace_consts; @@ -303,6 +303,10 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { "assign_ops", "using compound assignment operators (e.g. `+=`) is harmless", ); + store.register_removed( + "if_let_redundant_pattern_matching", + "this lint has been changed to redundant_pattern_matching", + ); // end deprecated lints, do not remove this comment, it’s used in `update_lints` reg.register_late_lint_pass(box serde_api::Serde); @@ -402,7 +406,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { reg.register_late_lint_pass(box missing_doc::MissingDoc::new()); reg.register_late_lint_pass(box missing_inline::MissingInline); reg.register_late_lint_pass(box ok_if_let::Pass); - reg.register_late_lint_pass(box if_let_redundant_pattern_matching::Pass); + reg.register_late_lint_pass(box redundant_pattern_matching::Pass); reg.register_late_lint_pass(box partialeq_ne_impl::Pass); reg.register_early_lint_pass(box reference::Pass); reg.register_early_lint_pass(box reference::DerefPass); @@ -565,7 +569,6 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { functions::TOO_MANY_ARGUMENTS, identity_conversion::IDENTITY_CONVERSION, identity_op::IDENTITY_OP, - if_let_redundant_pattern_matching::IF_LET_REDUNDANT_PATTERN_MATCHING, indexing_slicing::OUT_OF_BOUNDS_INDEXING, infallible_destructuring_match::INFALLIBLE_DESTRUCTURING_MATCH, infinite_iter::INFINITE_ITER, @@ -680,6 +683,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { ranges::RANGE_PLUS_ONE, ranges::RANGE_ZIP_WITH_LEN, redundant_field_names::REDUNDANT_FIELD_NAMES, + redundant_pattern_matching::REDUNDANT_PATTERN_MATCHING, reference::DEREF_ADDROF, reference::REF_IN_DEREF, regex::INVALID_REGEX, @@ -749,7 +753,6 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { excessive_precision::EXCESSIVE_PRECISION, formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING, formatting::SUSPICIOUS_ELSE_FORMATTING, - if_let_redundant_pattern_matching::IF_LET_REDUNDANT_PATTERN_MATCHING, infallible_destructuring_match::INFALLIBLE_DESTRUCTURING_MATCH, len_zero::LEN_WITHOUT_IS_EMPTY, len_zero::LEN_ZERO, @@ -800,6 +803,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { ptr::PTR_ARG, question_mark::QUESTION_MARK, redundant_field_names::REDUNDANT_FIELD_NAMES, + redundant_pattern_matching::REDUNDANT_PATTERN_MATCHING, regex::REGEX_MACRO, regex::TRIVIAL_REGEX, returns::LET_AND_RETURN, diff --git a/clippy_lints/src/if_let_redundant_pattern_matching.rs b/clippy_lints/src/redundant_pattern_matching.rs similarity index 96% rename from clippy_lints/src/if_let_redundant_pattern_matching.rs rename to clippy_lints/src/redundant_pattern_matching.rs index 6f786fc5659..f8c5b29bad1 100644 --- a/clippy_lints/src/if_let_redundant_pattern_matching.rs +++ b/clippy_lints/src/redundant_pattern_matching.rs @@ -31,6 +31,10 @@ use crate::rustc_errors::Applicability; /// if let Err(_) = Err::(42) {} /// if let None = None::<()> {} /// if let Some(_) = Some(42) {} +/// match Ok::(42) { +/// Ok(_) => true, +/// Err(_) => false, +/// }; /// ``` /// /// The more idiomatic use would be: @@ -40,10 +44,11 @@ use crate::rustc_errors::Applicability; /// if Err::(42).is_err() {} /// if None::<()>.is_none() {} /// if Some(42).is_some() {} +/// Ok::(42).is_ok(); /// ``` /// declare_clippy_lint! { - pub IF_LET_REDUNDANT_PATTERN_MATCHING, + pub REDUNDANT_PATTERN_MATCHING, style, "use the proper utility function avoiding an `if let`" } @@ -53,7 +58,7 @@ pub struct Pass; impl LintPass for Pass { fn get_lints(&self) -> LintArray { - lint_array!(IF_LET_REDUNDANT_PATTERN_MATCHING) + lint_array!(REDUNDANT_PATTERN_MATCHING) } } @@ -101,7 +106,7 @@ fn find_sugg_for_if_let<'a, 'tcx>( span_lint_and_then( cx, - IF_LET_REDUNDANT_PATTERN_MATCHING, + REDUNDANT_PATTERN_MATCHING, arms[0].pats[0].span, &format!("redundant pattern matching, consider using `{}`", good_method), |db| { @@ -174,7 +179,7 @@ fn find_sugg_for_match<'a, 'tcx>( if let Some(good_method) = found_good_method { span_lint_and_then( cx, - IF_LET_REDUNDANT_PATTERN_MATCHING, + REDUNDANT_PATTERN_MATCHING, expr.span, &format!("redundant pattern matching, consider using `{}`", good_method), |db| { diff --git a/tests/ui/matches.rs b/tests/ui/matches.rs index c43fead08f8..d31e97c7959 100644 --- a/tests/ui/matches.rs +++ b/tests/ui/matches.rs @@ -13,7 +13,7 @@ #![warn(clippy::all)] -#![allow(unused, clippy::if_let_redundant_pattern_matching)] +#![allow(unused, clippy::redundant_pattern_matching)] #![warn(clippy::single_match_else, clippy::match_same_arms)] enum ExprNode { diff --git a/tests/ui/needless_pass_by_value.rs b/tests/ui/needless_pass_by_value.rs index 5825d9e9074..48b7b42cc8c 100644 --- a/tests/ui/needless_pass_by_value.rs +++ b/tests/ui/needless_pass_by_value.rs @@ -11,7 +11,7 @@ #![warn(clippy::needless_pass_by_value)] -#![allow(dead_code, clippy::single_match, clippy::if_let_redundant_pattern_matching, clippy::many_single_char_names, clippy::option_option)] +#![allow(dead_code, clippy::single_match, clippy::redundant_pattern_matching, clippy::many_single_char_names, clippy::option_option)] use std::borrow::Borrow; use std::convert::AsRef; diff --git a/tests/ui/if_let_redundant_pattern_matching.rs b/tests/ui/redundant_pattern_matching.rs similarity index 96% rename from tests/ui/if_let_redundant_pattern_matching.rs rename to tests/ui/redundant_pattern_matching.rs index 3f7d0c8e1bd..50838584f66 100644 --- a/tests/ui/if_let_redundant_pattern_matching.rs +++ b/tests/ui/redundant_pattern_matching.rs @@ -12,7 +12,7 @@ #![warn(clippy::all)] -#![warn(clippy::if_let_redundant_pattern_matching)] +#![warn(clippy::redundant_pattern_matching)] fn main() { diff --git a/tests/ui/if_let_redundant_pattern_matching.stderr b/tests/ui/redundant_pattern_matching.stderr similarity index 78% rename from tests/ui/if_let_redundant_pattern_matching.stderr rename to tests/ui/redundant_pattern_matching.stderr index 93bafa7fcbd..a42ac7ba04d 100644 --- a/tests/ui/if_let_redundant_pattern_matching.stderr +++ b/tests/ui/redundant_pattern_matching.stderr @@ -1,13 +1,13 @@ error: redundant pattern matching, consider using `is_ok()` - --> $DIR/if_let_redundant_pattern_matching.rs:19:12 + --> $DIR/redundant_pattern_matching.rs:19:12 | 19 | if let Ok(_) = Ok::(42) {} | -------^^^^^------------------------ help: try this: `if Ok::(42).is_ok()` | - = note: `-D clippy::if-let-redundant-pattern-matching` implied by `-D warnings` + = note: `-D clippy::redundant-pattern-matching` implied by `-D warnings` error: redundant pattern matching, consider using `is_err()` - --> $DIR/if_let_redundant_pattern_matching.rs:21:12 + --> $DIR/redundant_pattern_matching.rs:21:12 | 21 | if let Err(_) = Err::(42) { | _____- ^^^^^^ @@ -15,7 +15,7 @@ error: redundant pattern matching, consider using `is_err()` | |_____- help: try this: `if Err::(42).is_err()` error: redundant pattern matching, consider using `is_none()` - --> $DIR/if_let_redundant_pattern_matching.rs:24:12 + --> $DIR/redundant_pattern_matching.rs:24:12 | 24 | if let None = None::<()> { | _____- ^^^^ @@ -23,7 +23,7 @@ error: redundant pattern matching, consider using `is_none()` | |_____- help: try this: `if None::<()>.is_none()` error: redundant pattern matching, consider using `is_some()` - --> $DIR/if_let_redundant_pattern_matching.rs:27:12 + --> $DIR/redundant_pattern_matching.rs:27:12 | 27 | if let Some(_) = Some(42) { | _____- ^^^^^^^ @@ -31,7 +31,7 @@ error: redundant pattern matching, consider using `is_some()` | |_____- help: try this: `if Some(42).is_some()` error: redundant pattern matching, consider using `is_ok()` - --> $DIR/if_let_redundant_pattern_matching.rs:46:5 + --> $DIR/redundant_pattern_matching.rs:46:5 | 46 | / match Ok::(42) { 47 | | Ok(_) => true, @@ -40,7 +40,7 @@ error: redundant pattern matching, consider using `is_ok()` | |_____^ help: try this: `Ok::(42).is_ok()` error: redundant pattern matching, consider using `is_err()` - --> $DIR/if_let_redundant_pattern_matching.rs:51:5 + --> $DIR/redundant_pattern_matching.rs:51:5 | 51 | / match Ok::(42) { 52 | | Ok(_) => false, @@ -49,7 +49,7 @@ error: redundant pattern matching, consider using `is_err()` | |_____^ help: try this: `Ok::(42).is_err()` error: redundant pattern matching, consider using `is_err()` - --> $DIR/if_let_redundant_pattern_matching.rs:56:5 + --> $DIR/redundant_pattern_matching.rs:56:5 | 56 | / match Err::(42) { 57 | | Ok(_) => false, @@ -58,7 +58,7 @@ error: redundant pattern matching, consider using `is_err()` | |_____^ help: try this: `Err::(42).is_err()` error: redundant pattern matching, consider using `is_ok()` - --> $DIR/if_let_redundant_pattern_matching.rs:61:5 + --> $DIR/redundant_pattern_matching.rs:61:5 | 61 | / match Err::(42) { 62 | | Ok(_) => true, @@ -67,7 +67,7 @@ error: redundant pattern matching, consider using `is_ok()` | |_____^ help: try this: `Err::(42).is_ok()` error: redundant pattern matching, consider using `is_some()` - --> $DIR/if_let_redundant_pattern_matching.rs:66:5 + --> $DIR/redundant_pattern_matching.rs:66:5 | 66 | / match Some(42) { 67 | | Some(_) => true, @@ -76,7 +76,7 @@ error: redundant pattern matching, consider using `is_some()` | |_____^ help: try this: `Some(42).is_some()` error: redundant pattern matching, consider using `is_none()` - --> $DIR/if_let_redundant_pattern_matching.rs:71:5 + --> $DIR/redundant_pattern_matching.rs:71:5 | 71 | / match None::<()> { 72 | | Some(_) => false, From 9f3ac4e5a396170c6f59e0f654c65b5ba0c4e5e5 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Wed, 17 Oct 2018 08:18:05 +0200 Subject: [PATCH 47/50] RIIR update_lints: Update changelog links This now also updates the link list at the bottom of the changelog. --- clippy_dev/src/lib.rs | 32 +++++++++++++++++++++++++++++++- clippy_dev/src/main.rs | 12 ++++++++++-- 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/clippy_dev/src/lib.rs b/clippy_dev/src/lib.rs index dcb2de2b1f8..77351233381 100644 --- a/clippy_dev/src/lib.rs +++ b/clippy_dev/src/lib.rs @@ -59,13 +59,29 @@ impl Lint { /// Returns all non-deprecated lints and non-internal lints pub fn usable_lints(lints: impl Iterator) -> impl Iterator { - lints.filter(|l| l.deprecation.is_none() && !l.group.starts_with("internal")) + lints.filter(|l| l.deprecation.is_none() && !l.is_internal()) } /// Returns the lints in a HashMap, grouped by the different lint groups pub fn by_lint_group(lints: &[Self]) -> HashMap> { lints.iter().map(|lint| (lint.group.to_string(), lint.clone())).into_group_map() } + + pub fn is_internal(&self) -> bool { + self.group.starts_with("internal") + } +} + +pub fn gen_changelog_lint_list(lints: Vec) -> Vec { + let mut lint_list_sorted: Vec = lints; + lint_list_sorted.sort_by_key(|l| l.name.clone()); + lint_list_sorted + .iter() + .filter(|l| !l.is_internal()) + .map(|l| { + format!("[`{}`]: {}#{}", l.name, DOCS_LINK.clone(), l.name) + }) + .collect() } /// Gathers all files in `src/clippy_lints` and gathers all lints inside @@ -291,3 +307,17 @@ fn test_by_lint_group() { ]); assert_eq!(expected, Lint::by_lint_group(&lints)); } + +#[test] +fn test_gen_changelog_lint_list() { + let lints = vec![ + Lint::new("should_assert_eq", "group1", "abc", None, "module_name"), + Lint::new("should_assert_eq2", "group2", "abc", None, "module_name"), + Lint::new("incorrect_internal", "internal_style", "abc", None, "module_name"), + ]; + let expected = vec![ + format!("[`should_assert_eq`]: {}#should_assert_eq", DOCS_LINK.to_string()), + format!("[`should_assert_eq2`]: {}#should_assert_eq2", DOCS_LINK.to_string()) + ]; + assert_eq!(expected, gen_changelog_lint_list(lints)); +} diff --git a/clippy_dev/src/main.rs b/clippy_dev/src/main.rs index 7b688836a95..8769ee6b810 100644 --- a/clippy_dev/src/main.rs +++ b/clippy_dev/src/main.rs @@ -59,8 +59,8 @@ fn print_lints() { } fn update_lints() { - let lint_list = gather_all(); - let usable_lints: Vec = Lint::usable_lints(lint_list).collect(); + let lint_list: Vec = gather_all().collect(); + let usable_lints: Vec = Lint::usable_lints(lint_list.clone().into_iter()).collect(); let lint_count = usable_lints.len(); replace_region_in_file( @@ -74,4 +74,12 @@ fn update_lints() { ] } ); + + replace_region_in_file( + "../CHANGELOG.md", + "", + "", + false, + || { gen_changelog_lint_list(lint_list.clone()) } + ); } From 4b68c965feec8ce0e86d0b396baca5e33c3cf0af Mon Sep 17 00:00:00 2001 From: Joshua Holmer Date: Wed, 17 Oct 2018 10:43:32 -0400 Subject: [PATCH 48/50] Resolve ICE in needless range loop lint An ICE would occur if the needless range loop was triggered within a procedural macro, because Clippy would try to produce a code suggestion which was invalid, and caused the compiler to crash. This commit takes the same approach which Clippy currently takes to work around this type of crash in the needless pass by value lint, which is to skip the lint if Clippy is inside of a macro. --- clippy_lints/src/loops.rs | 6 +++++- mini-macro/src/lib.rs | 5 +++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 3c4f06077d9..950c1043802 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -31,7 +31,7 @@ use std::mem; use crate::syntax::ast; use crate::syntax::source_map::Span; use crate::syntax_pos::BytePos; -use crate::utils::{sugg, sext}; +use crate::utils::{in_macro, sugg, sext}; use crate::utils::usage::mutated_variables; use crate::consts::{constant, Constant}; @@ -1030,6 +1030,10 @@ fn check_for_loop_range<'a, 'tcx>( body: &'tcx Expr, expr: &'tcx Expr, ) { + if in_macro(expr.span) { + return; + } + if let Some(higher::Range { start: Some(start), ref end, diff --git a/mini-macro/src/lib.rs b/mini-macro/src/lib.rs index d326dd7e679..b6405975862 100644 --- a/mini-macro/src/lib.rs +++ b/mini-macro/src/lib.rs @@ -17,5 +17,10 @@ use proc_macro::{TokenStream, quote}; pub fn mini_macro(_: TokenStream) -> TokenStream { quote!( #[allow(unused)] fn needless_take_by_value(s: String) { println!("{}", s.len()); } + #[allow(unused)] fn needless_loop(items: &[u8]) { + for i in 0..items.len() { + println!("{}", items[i]); + } + } ) } From 8753e568bf0d8bdc591ca56d9c3bc442efffaede Mon Sep 17 00:00:00 2001 From: Lukas Stevens Date: Tue, 16 Oct 2018 22:20:27 +0200 Subject: [PATCH 49/50] Check for comments in collapsible ifs --- clippy_lints/src/collapsible_if.rs | 9 ++++++ tests/ui/collapsible_if.rs | 45 ++++++++++++++++++++++++++++++ tests/ui/collapsible_if.stderr | 18 +++++++++++- 3 files changed, 71 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/collapsible_if.rs b/clippy_lints/src/collapsible_if.rs index 85fdca1d421..67ef1048299 100644 --- a/clippy_lints/src/collapsible_if.rs +++ b/clippy_lints/src/collapsible_if.rs @@ -112,9 +112,17 @@ fn check_if(cx: &EarlyContext<'_>, expr: &ast::Expr) { } } +fn block_starts_with_comment(cx: &EarlyContext<'_>, expr: &ast::Block) -> bool { + // The zeroth character in the trimmed block text is "{", which marks the beginning of the block. + // Therefore, we check if the first string after that is a comment, i.e. starts with //. + let trimmed_block_text = snippet_block(cx, expr.span, "..").trim_left().to_owned(); + trimmed_block_text[1..trimmed_block_text.len()].trim_left().starts_with("//") +} + fn check_collapsible_maybe_if_let(cx: &EarlyContext<'_>, else_: &ast::Expr) { if_chain! { if let ast::ExprKind::Block(ref block, _) = else_.node; + if !block_starts_with_comment(cx, block); if let Some(else_) = expr_block(block); if !in_macro(else_.span); then { @@ -135,6 +143,7 @@ fn check_collapsible_maybe_if_let(cx: &EarlyContext<'_>, else_: &ast::Expr) { fn check_collapsible_no_if_let(cx: &EarlyContext<'_>, expr: &ast::Expr, check: &ast::Expr, then: &ast::Block) { if_chain! { + if !block_starts_with_comment(cx, then); if let Some(inner) = expr_block(then); if let ast::ExprKind::If(ref check_inner, ref content, None) = inner.node; then { diff --git a/tests/ui/collapsible_if.rs b/tests/ui/collapsible_if.rs index a6df9109df9..c186d9e577f 100644 --- a/tests/ui/collapsible_if.rs +++ b/tests/ui/collapsible_if.rs @@ -151,4 +151,49 @@ fn main() { } else { assert!(true); // assert! is just an `if` } + + + // The following tests check for the fix of https://github.com/rust-lang-nursery/rust-clippy/issues/798 + if x == "hello" {// Not collapsible + if y == "world" { + println!("Hello world!"); + } + } + + if x == "hello" { // Not collapsible + if y == "world" { + println!("Hello world!"); + } + } + + if x == "hello" { + // Not collapsible + if y == "world" { + println!("Hello world!"); + } + } + + if x == "hello" { + if y == "world" { // Collapsible + println!("Hello world!"); + } + } + + if x == "hello" { + print!("Hello "); + } else { + // Not collapsible + if y == "world" { + println!("world!") + } + } + + if x == "hello" { + print!("Hello "); + } else { + // Not collapsible + if let Some(42) = Some(42) { + println!("world!") + } + } } diff --git a/tests/ui/collapsible_if.stderr b/tests/ui/collapsible_if.stderr index 87c279cd725..3f06dca5495 100644 --- a/tests/ui/collapsible_if.stderr +++ b/tests/ui/collapsible_if.stderr @@ -240,5 +240,21 @@ help: try 122 | } | -error: aborting due to 13 previous errors +error: this if statement can be collapsed + --> $DIR/collapsible_if.rs:176:5 + | +176 | / if x == "hello" { +177 | | if y == "world" { // Collapsible +178 | | println!("Hello world!"); +179 | | } +180 | | } + | |_____^ +help: try + | +176 | if x == "hello" && y == "world" { // Collapsible +177 | println!("Hello world!"); +178 | } + | + +error: aborting due to 14 previous errors From 5614dcb4ead82724e5873172961a26ffbfddcd18 Mon Sep 17 00:00:00 2001 From: Lukas Stevens Date: Thu, 18 Oct 2018 18:57:16 +0200 Subject: [PATCH 50/50] Support multiline comments and hopefully fix panic --- clippy_lints/src/collapsible_if.rs | 8 ++++---- tests/ui/collapsible_if.rs | 13 +++++++++++++ 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/collapsible_if.rs b/clippy_lints/src/collapsible_if.rs index 67ef1048299..a55ca04f706 100644 --- a/clippy_lints/src/collapsible_if.rs +++ b/clippy_lints/src/collapsible_if.rs @@ -113,10 +113,10 @@ fn check_if(cx: &EarlyContext<'_>, expr: &ast::Expr) { } fn block_starts_with_comment(cx: &EarlyContext<'_>, expr: &ast::Block) -> bool { - // The zeroth character in the trimmed block text is "{", which marks the beginning of the block. - // Therefore, we check if the first string after that is a comment, i.e. starts with //. - let trimmed_block_text = snippet_block(cx, expr.span, "..").trim_left().to_owned(); - trimmed_block_text[1..trimmed_block_text.len()].trim_left().starts_with("//") + // We trim all opening braces and whitespaces and then check if the next string is a comment. + let trimmed_block_text = + snippet_block(cx, expr.span, "..").trim_left_matches(|c: char| c.is_whitespace() || c == '{').to_owned(); + trimmed_block_text.starts_with("//") || trimmed_block_text.starts_with("/*") } fn check_collapsible_maybe_if_let(cx: &EarlyContext<'_>, else_: &ast::Expr) { diff --git a/tests/ui/collapsible_if.rs b/tests/ui/collapsible_if.rs index c186d9e577f..1bc866010fd 100644 --- a/tests/ui/collapsible_if.rs +++ b/tests/ui/collapsible_if.rs @@ -196,4 +196,17 @@ fn main() { println!("world!") } } + + if x == "hello" { + /* Not collapsible */ + if y == "world" { + println!("Hello world!"); + } + } + + if x == "hello" { /* Not collapsible */ + if y == "world" { + println!("Hello world!"); + } + } }