Auto merge of #4084 - mikerite:fix-4019, r=oli-obk

Fix 4019

Fixes #4019

clippy_lints/src/methods/mod.rs

@@ -10,6 +10,7 @@ use lazy_static::lazy_static;
 use matches::matches;
 use rustc::hir;
 use rustc::hir::def::{DefKind, Res};
+use rustc::hir::intravisit::{self, Visitor};
 use rustc::lint::{in_external_macro, LateContext, LateLintPass, Lint, LintArray, LintContext, LintPass};
 use rustc::ty::{self, Predicate, Ty};
 use rustc::{declare_lint_pass, declare_tool_lint};
@@ -1046,7 +1047,51 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
 
 /// Checks for the `OR_FUN_CALL` lint.
 #[allow(clippy::too_many_lines)]
-fn lint_or_fun_call(cx: &LateContext<'_, '_>, expr: &hir::Expr, method_span: Span, name: &str, args: &[hir::Expr]) {
+fn lint_or_fun_call<'a, 'tcx: 'a>(
+    cx: &LateContext<'a, 'tcx>,
+    expr: &hir::Expr,
+    method_span: Span,
+    name: &str,
+    args: &'tcx [hir::Expr],
+) {
+    // Searches an expression for method calls or function calls that aren't ctors
+    struct FunCallFinder<'a, 'tcx: 'a> {
+        cx: &'a LateContext<'a, 'tcx>,
+        found: bool,
+    }
+
+    impl<'a, 'tcx> intravisit::Visitor<'tcx> for FunCallFinder<'a, 'tcx> {
+        fn visit_expr(&mut self, expr: &'tcx hir::Expr) {
+            let call_found = match &expr.node {
+                // ignore enum and struct constructors
+                hir::ExprKind::Call(..) => !is_ctor_function(self.cx, expr),
+                hir::ExprKind::MethodCall(..) => true,
+                _ => false,
+            };
+
+            if call_found {
+                // don't lint for constant values
+                let owner_def = self.cx.tcx.hir().get_parent_did_by_hir_id(expr.hir_id);
+                let promotable = self
+                    .cx
+                    .tcx
+                    .rvalue_promotable_map(owner_def)
+                    .contains(&expr.hir_id.local_id);
+                if !promotable {
+                    self.found |= true;
+                }
+            }
+
+            if !self.found {
+                intravisit::walk_expr(self, expr);
+            }
+        }
+
+        fn nested_visit_map<'this>(&'this mut self) -> intravisit::NestedVisitorMap<'this, 'tcx> {
+            intravisit::NestedVisitorMap::None
+        }
+    }
+
     /// Checks for `unwrap_or(T::new())` or `unwrap_or(T::default())`.
     fn check_unwrap_or_default(
         cx: &LateContext<'_, '_>,
@@ -1099,13 +1144,13 @@ fn lint_or_fun_call(cx: &LateContext<'_, '_>, expr: &hir::Expr, method_span: Spa
 
     /// Checks for `*or(foo())`.
     #[allow(clippy::too_many_arguments)]
-    fn check_general_case(
+    fn check_general_case<'a, 'tcx: 'a>(
-        cx: &LateContext<'_, '_>,
+        cx: &LateContext<'a, 'tcx>,
         name: &str,
         method_span: Span,
         fun_span: Span,
         self_expr: &hir::Expr,
-        arg: &hir::Expr,
+        arg: &'tcx hir::Expr,
         or_has_args: bool,
         span: Span,
     ) {
@@ -1122,15 +1167,9 @@ fn lint_or_fun_call(cx: &LateContext<'_, '_>, expr: &hir::Expr, method_span: Spa
             return;
         }
 
-        // ignore enum and struct constructors
+        let mut finder = FunCallFinder { cx: &cx, found: false };
-        if is_ctor_function(cx, &arg) {
+        finder.visit_expr(&arg);
-            return;
+        if !finder.found {
-        }
-
-        // don't lint for constant values
-        let owner_def = cx.tcx.hir().get_parent_did_by_hir_id(arg.hir_id);
-        let promotable = cx.tcx.rvalue_promotable_map(owner_def).contains(&arg.hir_id.local_id);
-        if promotable {
             return;
         }
 

tests/ui/methods.rs

@@ -268,21 +268,3 @@ fn main() {
     let opt = Some(0);
     let _ = opt.unwrap();
 }
-
-struct Foo(u8);
-#[rustfmt::skip]
-fn test_or_with_ctors() {
-    let opt = Some(1);
-    let opt_opt = Some(Some(1));
-    // we also test for const promotion, this makes sure we don't hit that
-    let two = 2;
-
-    let _ = opt_opt.unwrap_or(Some(2));
-    let _ = opt_opt.unwrap_or(Some(two));
-    let _ = opt.ok_or(Some(2));
-    let _ = opt.ok_or(Some(two));
-    let _ = opt.ok_or(Foo(2));
-    let _ = opt.ok_or(Foo(two));
-    let _ = opt.or(Some(2));
-    let _ = opt.or(Some(two));
-}

tests/ui/or_fun_call.rs

@@ -2,6 +2,7 @@
 
 use std::collections::BTreeMap;
 use std::collections::HashMap;
+use std::time::Duration;
 
 /// Checks implementation of the `OR_FUN_CALL` lint.
 fn or_fun_call() {
@@ -24,8 +25,8 @@ fn or_fun_call() {
     let with_enum = Some(Enum::A(1));
     with_enum.unwrap_or(Enum::A(5));
 
-    let with_const_fn = Some(::std::time::Duration::from_secs(1));
+    let with_const_fn = Some(Duration::from_secs(1));
-    with_const_fn.unwrap_or(::std::time::Duration::from_secs(5));
+    with_const_fn.unwrap_or(Duration::from_secs(5));
 
     let with_constructor = Some(vec![1]);
     with_constructor.unwrap_or(make());
@@ -70,4 +71,29 @@ fn or_fun_call() {
     let _ = opt.ok_or(format!("{} world.", hello));
 }
 
+struct Foo(u8);
+struct Bar(String, Duration);
+#[rustfmt::skip]
+fn test_or_with_ctors() {
+    let opt = Some(1);
+    let opt_opt = Some(Some(1));
+    // we also test for const promotion, this makes sure we don't hit that
+    let two = 2;
+
+    let _ = opt_opt.unwrap_or(Some(2));
+    let _ = opt_opt.unwrap_or(Some(two));
+    let _ = opt.ok_or(Some(2));
+    let _ = opt.ok_or(Some(two));
+    let _ = opt.ok_or(Foo(2));
+    let _ = opt.ok_or(Foo(two));
+    let _ = opt.or(Some(2));
+    let _ = opt.or(Some(two));
+
+    let _ = Some("a".to_string()).or(Some("b".to_string()));
+
+    let b = "b".to_string();
+    let _ = Some(Bar("a".to_string(), Duration::from_secs(1)))
+        .or(Some(Bar(b, Duration::from_secs(2))));
+}
+
 fn main() {}

tests/ui/or_fun_call.stderr

@@ -1,5 +1,5 @@
 error: use of `unwrap_or` followed by a function call
-  --> $DIR/or_fun_call.rs:31:22
+  --> $DIR/or_fun_call.rs:32:22
    |
 LL |     with_constructor.unwrap_or(make());
    |                      ^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(make)`
@@ -7,76 +7,82 @@ LL |     with_constructor.unwrap_or(make());
    = note: `-D clippy::or-fun-call` implied by `-D warnings`
 
 error: use of `unwrap_or` followed by a call to `new`
-  --> $DIR/or_fun_call.rs:34:5
+  --> $DIR/or_fun_call.rs:35:5
    |
 LL |     with_new.unwrap_or(Vec::new());
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_new.unwrap_or_default()`
 
 error: use of `unwrap_or` followed by a function call
-  --> $DIR/or_fun_call.rs:37:21
+  --> $DIR/or_fun_call.rs:38:21
    |
 LL |     with_const_args.unwrap_or(Vec::with_capacity(12));
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| Vec::with_capacity(12))`
 
 error: use of `unwrap_or` followed by a function call
-  --> $DIR/or_fun_call.rs:40:14
+  --> $DIR/or_fun_call.rs:41:14
    |
 LL |     with_err.unwrap_or(make());
    |              ^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|_| make())`
 
 error: use of `unwrap_or` followed by a function call
-  --> $DIR/or_fun_call.rs:43:19
+  --> $DIR/or_fun_call.rs:44:19
    |
 LL |     with_err_args.unwrap_or(Vec::with_capacity(12));
    |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|_| Vec::with_capacity(12))`
 
 error: use of `unwrap_or` followed by a call to `default`
-  --> $DIR/or_fun_call.rs:46:5
+  --> $DIR/or_fun_call.rs:47:5
    |
 LL |     with_default_trait.unwrap_or(Default::default());
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_default_trait.unwrap_or_default()`
 
 error: use of `unwrap_or` followed by a call to `default`
-  --> $DIR/or_fun_call.rs:49:5
+  --> $DIR/or_fun_call.rs:50:5
    |
 LL |     with_default_type.unwrap_or(u64::default());
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_default_type.unwrap_or_default()`
 
 error: use of `unwrap_or` followed by a function call
-  --> $DIR/or_fun_call.rs:52:14
+  --> $DIR/or_fun_call.rs:53:14
    |
 LL |     with_vec.unwrap_or(vec![]);
    |              ^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| vec![])`
 
 error: use of `unwrap_or` followed by a function call
-  --> $DIR/or_fun_call.rs:57:21
+  --> $DIR/or_fun_call.rs:58:21
    |
 LL |     without_default.unwrap_or(Foo::new());
    |                     ^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(Foo::new)`
 
 error: use of `or_insert` followed by a function call
-  --> $DIR/or_fun_call.rs:60:19
+  --> $DIR/or_fun_call.rs:61:19
    |
 LL |     map.entry(42).or_insert(String::new());
    |                   ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `or_insert_with(String::new)`
 
 error: use of `or_insert` followed by a function call
-  --> $DIR/or_fun_call.rs:63:21
+  --> $DIR/or_fun_call.rs:64:21
    |
 LL |     btree.entry(42).or_insert(String::new());
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `or_insert_with(String::new)`
 
 error: use of `unwrap_or` followed by a function call
-  --> $DIR/or_fun_call.rs:66:21
+  --> $DIR/or_fun_call.rs:67:21
    |
 LL |     let _ = stringy.unwrap_or("".to_owned());
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| "".to_owned())`
 
 error: use of `ok_or` followed by a function call
-  --> $DIR/or_fun_call.rs:70:17
+  --> $DIR/or_fun_call.rs:71:17
    |
 LL |     let _ = opt.ok_or(format!("{} world.", hello));
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `ok_or_else(|| format!("{} world.", hello))`
 
-error: aborting due to 13 previous errors
+error: use of `or` followed by a function call
+  --> $DIR/or_fun_call.rs:92:35
+   |
+LL |     let _ = Some("a".to_string()).or(Some("b".to_string()));
+   |                                   ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `or_else(|| Some("b".to_string()))`
+
+error: aborting due to 14 previous errors