diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index f8cde663d89..be2b7a26e57 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -9,6 +9,7 @@ use if_chain::if_chain; 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}; @@ -1044,7 +1045,49 @@ 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 found = match &expr.node { + hir::ExprKind::Call(..) => !is_ctor_function(self.cx, expr), + hir::ExprKind::MethodCall(..) => true, + _ => false, + }; + + if found { + 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<'_, '_>, @@ -1096,13 +1139,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( - cx: &LateContext<'_, '_>, + fn check_general_case<'a, 'tcx: 'a>( + 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, ) { @@ -1120,14 +1163,9 @@ fn lint_or_fun_call(cx: &LateContext<'_, '_>, expr: &hir::Expr, method_span: Spa } // ignore enum and struct constructors - if is_ctor_function(cx, &arg) { - return; - } - - // 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 { + let mut finder = FunCallFinder { cx: &cx, found: false }; + finder.visit_expr(&arg); + if !finder.found { return; } diff --git a/tests/ui/or_fun_call.rs b/tests/ui/or_fun_call.rs index be2165c222f..f339bae8ac6 100644 --- a/tests/ui/or_fun_call.rs +++ b/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)); - with_const_fn.unwrap_or(::std::time::Duration::from_secs(5)); + let with_const_fn = Some(Duration::from_secs(1)); + with_const_fn.unwrap_or(Duration::from_secs(5)); let with_constructor = Some(vec![1]); with_constructor.unwrap_or(make()); @@ -71,6 +72,7 @@ fn or_fun_call() { } struct Foo(u8); +struct Bar(String, Duration); #[rustfmt::skip] fn test_or_with_ctors() { let opt = Some(1); @@ -86,6 +88,12 @@ fn test_or_with_ctors() { 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() {} diff --git a/tests/ui/or_fun_call.stderr b/tests/ui/or_fun_call.stderr index 5d6ebb50a89..f6e5d202e0c 100644 --- a/tests/ui/or_fun_call.stderr +++ b/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