Auto merge of #3535 - sinkuu:fixes, r=phansch

bors · bors · commit 379c934f3f0c · 2018-12-12T19:17:09.000Z
Fix some problems Fixes #2892, #3199, #2841, #3476
diff --git 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
@@ -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/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,
@@ -84,7 +110,7 @@ impl Pass {
                             expr.span,
                             "replace_it_with",
                             format!("{}?;", receiver_str),
-                            Applicability::MachineApplicable, // snippet
+                            Applicability::MaybeIncorrect, // snippet
                         );
                     }
                 )
@@ -133,9 +159,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
diff --git 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/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
@@ -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() {}
diff --git 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<u32> {
         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
@@ -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
 
diff --git 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<T>() {
+    }
+
+    struct S {
+        foo: fn(),
+    }
+
+    S { foo: foo::<i32> };
+}
diff --git 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<T: io::Read + io::Write>(s: &mut T) -> io::Result<()> {
     try!(s.write(b"test"));
     let mut buf = [0u8; 4];

Original file line number	Diff line number	Diff line change
`@@ -20,4 +20,12 @@ impl PartialEq for Foo {`
`20`	`20`	`}`
`21`	`21`	`}`
`22`	`22`
	`23`	`+struct Bar;`
	`24`	`+`
	`25`	`+impl PartialEq for Bar {`
	`26`	`+ fn eq(&self, _: &Bar) -> bool { true }`
	`27`	`+ #[allow(clippy::partialeq_ne_impl)]`
	`28`	`+ fn ne(&self, _: &Bar) -> bool { false }`
	`29`	`+}`
	`30`	`+`
`23`	`31`	`fn main() {}`
Original file line number	Diff line number	Diff line change
`@@ -42,11 +42,22 @@ pub struct SomeStruct {`
`42`	`42`	`}`
`43`	`43`
`44`	`44`	`impl SomeStruct {`
	`45`	`+ #[rustfmt::skip]`
`45`	`46`	`pub fn func(&self) -> Option<u32> {`
`46`	`47`	`if (self.opt).is_none() {`
`47`	`48`	`return None;`
`48`	`49`	`}`
`49`	`50`
	`51`	`+ if self.opt.is_none() {`
	`52`	`+ return None`
	`53`	`+ }`
	`54`	`+`
	`55`	`+ let _ = if self.opt.is_none() {`
	`56`	`+ return None;`
	`57`	`+ } else {`
	`58`	`+ self.opt`
	`59`	`+ };`
	`60`	`+`
`50`	`61`	`self.opt`
`51`	`62`	`}`
`52`	`63`	`}`