From bcbbb4d09b2f474614134730e84afad04d2d8e48 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Tue, 11 Dec 2018 15:06:41 +0900 Subject: [PATCH 1/6] new_without_default, partialeq_ne_impl: Use span_lint_node Fixes #2892, fixes #3199 --- clippy_lints/src/new_without_default.rs | 8 +++++--- clippy_lints/src/partialeq_ne_impl.rs | 13 ++++++++----- tests/ui/new_without_default.rs | 14 ++++++++++++++ tests/ui/partialeq_ne_impl.rs | 8 ++++++++ 4 files changed, 35 insertions(+), 8 deletions(-) diff --git a/clippy_lints/src/new_without_default.rs b/clippy_lints/src/new_without_default.rs index 7b838fdee951..86c345b025ca 100644 --- a/clippy_lints/src/new_without_default.rs +++ b/clippy_lints/src/new_without_default.rs @@ -17,7 +17,7 @@ use crate::rustc_errors::Applicability; use crate::syntax::source_map::Span; use crate::utils::paths; use crate::utils::sugg::DiagnosticBuilderExt; -use crate::utils::{get_trait_def_id, implements_trait, return_ty, same_tys, span_lint_and_then}; +use crate::utils::{get_trait_def_id, implements_trait, return_ty, same_tys, span_lint_node_and_then}; use if_chain::if_chain; /// **What it does:** Checks for types with a `fn new() -> Self` method and no @@ -165,9 +165,10 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NewWithoutDefault { } if let Some(sp) = can_derive_default(self_ty, cx, default_trait_id) { - span_lint_and_then( + span_lint_node_and_then( cx, NEW_WITHOUT_DEFAULT_DERIVE, + id, impl_item.span, &format!( "you should consider deriving a `Default` implementation for `{}`", @@ -183,9 +184,10 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NewWithoutDefault { ); }); } else { - span_lint_and_then( + span_lint_node_and_then( cx, NEW_WITHOUT_DEFAULT, + id, impl_item.span, &format!( "you should consider adding a `Default` implementation for `{}`", diff --git a/clippy_lints/src/partialeq_ne_impl.rs b/clippy_lints/src/partialeq_ne_impl.rs index 70c93c5978b6..02935cf773d2 100644 --- a/clippy_lints/src/partialeq_ne_impl.rs +++ b/clippy_lints/src/partialeq_ne_impl.rs @@ -10,7 +10,7 @@ use crate::rustc::hir::*; use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; use crate::rustc::{declare_tool_lint, lint_array}; -use crate::utils::{is_automatically_derived, span_lint}; +use crate::utils::{is_automatically_derived, span_lint_node}; use if_chain::if_chain; /// **What it does:** Checks for manual re-implementations of `PartialEq::ne`. @@ -56,10 +56,13 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { then { for impl_item in impl_items { if impl_item.ident.name == "ne" { - span_lint(cx, - PARTIALEQ_NE_IMPL, - impl_item.span, - "re-implementing `PartialEq::ne` is unnecessary") + span_lint_node( + cx, + PARTIALEQ_NE_IMPL, + impl_item.id.node_id, + impl_item.span, + "re-implementing `PartialEq::ne` is unnecessary", + ); } } } diff --git a/tests/ui/new_without_default.rs b/tests/ui/new_without_default.rs index a1818e037a70..2e715a6f8ba8 100644 --- a/tests/ui/new_without_default.rs +++ b/tests/ui/new_without_default.rs @@ -139,4 +139,18 @@ impl<'a, T: 'a> OptionRefWrapper<'a, T> { } } +pub struct Allow(Foo); + +impl Allow { + #[allow(clippy::new_without_default)] + pub fn new() -> Self { unimplemented!() } +} + +pub struct AllowDerive; + +impl AllowDerive { + #[allow(clippy::new_without_default_derive)] + pub fn new() -> Self { unimplemented!() } +} + fn main() {} diff --git a/tests/ui/partialeq_ne_impl.rs b/tests/ui/partialeq_ne_impl.rs index 3f9f91c81b1d..fabeee24b305 100644 --- a/tests/ui/partialeq_ne_impl.rs +++ b/tests/ui/partialeq_ne_impl.rs @@ -20,4 +20,12 @@ impl PartialEq for Foo { } } +struct Bar; + +impl PartialEq for Bar { + fn eq(&self, _: &Bar) -> bool { true } + #[allow(clippy::partialeq_ne_impl)] + fn ne(&self, _: &Bar) -> bool { false } +} + fn main() {} From d2e5a8ccf517f19d0c3b2d0274dc50aeddd4a1fa Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Tue, 11 Dec 2018 17:31:22 +0900 Subject: [PATCH 2/6] Remove obsolete comment --- tests/ui/unused_io_amount.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ui/unused_io_amount.rs b/tests/ui/unused_io_amount.rs index a125d0397af8..0ec8ce57ad7f 100644 --- a/tests/ui/unused_io_amount.rs +++ b/tests/ui/unused_io_amount.rs @@ -12,7 +12,7 @@ use std::io; -// FIXME: compiletest doesn't understand errors from macro invocation span + fn try_macro(s: &mut T) -> io::Result<()> { try!(s.write(b"test")); let mut buf = [0u8; 4]; From 05d07155b72d39d447bff1f5c1e8fb13a1eecd31 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Tue, 11 Dec 2018 23:05:43 +0900 Subject: [PATCH 3/6] question_mark: Fix applicability --- clippy_lints/src/question_mark.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index 3a62a3a1526f..21f22f151dda 100644 --- a/clippy_lints/src/question_mark.rs +++ b/clippy_lints/src/question_mark.rs @@ -84,7 +84,7 @@ impl Pass { expr.span, "replace_it_with", format!("{}?;", receiver_str), - Applicability::MachineApplicable, // snippet + Applicability::MaybeIncorrect, // snippet ); } ) From 28635ff04be246a0e2d5bcec25b229f17b4f5974 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Tue, 11 Dec 2018 23:21:25 +0900 Subject: [PATCH 4/6] question_mark: Lint only early returns --- clippy_lints/src/question_mark.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index 21f22f151dda..61178ccdd563 100644 --- a/clippy_lints/src/question_mark.rs +++ b/clippy_lints/src/question_mark.rs @@ -133,9 +133,13 @@ impl Pass { } } - // Check if the block has an implicit return expression - if let Some(ref ret_expr) = block.expr { - return Some(ret_expr.clone()); + // Check for `return` without a semicolon. + if_chain! { + if block.stmts.len() == 0; + if let Some(ExprKind::Ret(Some(ret_expr))) = block.expr.as_ref().map(|e| &e.node); + then { + return Some(ret_expr.clone()); + } } None From eb54c1a9a0e89017bf71a7a78990ff8ab155a4f7 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Tue, 11 Dec 2018 23:33:23 +0900 Subject: [PATCH 5/6] redundant_field_names: Do not trigger on path with type params Fixes #3476 --- clippy_lints/src/redundant_field_names.rs | 5 ++++- tests/ui/redundant_field_names.rs | 11 +++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/redundant_field_names.rs b/clippy_lints/src/redundant_field_names.rs index 308f0066b69f..d8d80f2d128f 100644 --- a/clippy_lints/src/redundant_field_names.rs +++ b/clippy_lints/src/redundant_field_names.rs @@ -57,7 +57,10 @@ impl EarlyLintPass for RedundantFieldNames { continue; } if let ExprKind::Path(None, path) = &field.expr.node { - if path.segments.len() == 1 && path.segments[0].ident == field.ident { + if path.segments.len() == 1 + && path.segments[0].ident == field.ident + && path.segments[0].args.is_none() + { span_lint_and_sugg( cx, REDUNDANT_FIELD_NAMES, diff --git a/tests/ui/redundant_field_names.rs b/tests/ui/redundant_field_names.rs index 68adba92f8a2..3d727ee6e6a1 100644 --- a/tests/ui/redundant_field_names.rs +++ b/tests/ui/redundant_field_names.rs @@ -68,3 +68,14 @@ fn main() { let _ = RangeInclusive::new(start, end); let _ = RangeToInclusive { end: end }; } + +fn issue_3476() { + fn foo() { + } + + struct S { + foo: fn(), + } + + S { foo: foo:: }; +} From eba44e1c67cd08148c7e67ce6255889b7c581b98 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Wed, 12 Dec 2018 17:46:52 +0900 Subject: [PATCH 6/6] question_mark: Suggest Some(opt?) for if-else --- clippy_lints/src/question_mark.rs | 32 ++++++++++++++++++++++++++++--- tests/ui/question_mark.rs | 11 +++++++++++ tests/ui/question_mark.stderr | 29 +++++++++++++++++++++++----- 3 files changed, 64 insertions(+), 8 deletions(-) diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index 61178ccdd563..057b4850e4f4 100644 --- a/clippy_lints/src/question_mark.rs +++ b/clippy_lints/src/question_mark.rs @@ -17,7 +17,7 @@ use if_chain::if_chain; use crate::rustc_errors::Applicability; use crate::utils::paths::*; -use crate::utils::{match_def_path, match_type, span_lint_and_then}; +use crate::utils::{match_def_path, match_type, span_lint_and_then, SpanlessEq}; /// **What it does:** Checks for expressions that could be replaced by the question mark operator /// @@ -64,14 +64,40 @@ impl Pass { /// If it matches, it will suggest to use the question mark operator instead fn check_is_none_and_early_return_none(cx: &LateContext<'_, '_>, expr: &Expr) { if_chain! { - if let ExprKind::If(ref if_expr, ref body, _) = expr.node; - if let ExprKind::MethodCall(ref segment, _, ref args) = if_expr.node; + if let ExprKind::If(if_expr, body, else_) = &expr.node; + if let ExprKind::MethodCall(segment, _, args) = &if_expr.node; if segment.ident.name == "is_none"; if Self::expression_returns_none(cx, body); if let Some(subject) = args.get(0); if Self::is_option(cx, subject); then { + if let Some(else_) = else_ { + if_chain! { + if let ExprKind::Block(block, None) = &else_.node; + if block.stmts.len() == 0; + if let Some(block_expr) = &block.expr; + if SpanlessEq::new(cx).ignore_fn().eq_expr(subject, block_expr); + then { + span_lint_and_then( + cx, + QUESTION_MARK, + expr.span, + "this block may be rewritten with the `?` operator", + |db| { + db.span_suggestion_with_applicability( + expr.span, + "replace_it_with", + format!("Some({}?)", Sugg::hir(cx, subject, "..")), + Applicability::MaybeIncorrect, // snippet + ); + } + ) + } + } + return; + } + span_lint_and_then( cx, QUESTION_MARK, diff --git a/tests/ui/question_mark.rs b/tests/ui/question_mark.rs index 7f1d06fbd29e..b1edec32eeeb 100644 --- a/tests/ui/question_mark.rs +++ b/tests/ui/question_mark.rs @@ -42,11 +42,22 @@ pub struct SomeStruct { } impl SomeStruct { + #[rustfmt::skip] pub fn func(&self) -> Option { if (self.opt).is_none() { return None; } + if self.opt.is_none() { + return None + } + + let _ = if self.opt.is_none() { + return None; + } else { + self.opt + }; + self.opt } } diff --git a/tests/ui/question_mark.stderr b/tests/ui/question_mark.stderr index d3daaaa92705..c9d5538f36f4 100644 --- a/tests/ui/question_mark.stderr +++ b/tests/ui/question_mark.stderr @@ -9,12 +9,31 @@ error: this block may be rewritten with the `?` operator = note: `-D clippy::question-mark` implied by `-D warnings` error: this block may be rewritten with the `?` operator - --> $DIR/question_mark.rs:46:9 + --> $DIR/question_mark.rs:47:9 | -46 | / if (self.opt).is_none() { -47 | | return None; -48 | | } +47 | / if (self.opt).is_none() { +48 | | return None; +49 | | } | |_________^ help: replace_it_with: `(self.opt)?;` -error: aborting due to 2 previous errors +error: this block may be rewritten with the `?` operator + --> $DIR/question_mark.rs:51:9 + | +51 | / if self.opt.is_none() { +52 | | return None +53 | | } + | |_________^ help: replace_it_with: `self.opt?;` + +error: this block may be rewritten with the `?` operator + --> $DIR/question_mark.rs:55:17 + | +55 | let _ = if self.opt.is_none() { + | _________________^ +56 | | return None; +57 | | } else { +58 | | self.opt +59 | | }; + | |_________^ help: replace_it_with: `Some(self.opt?)` + +error: aborting due to 4 previous errors