diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index f8cde663d899..fb13598dea9a 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,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<'_, '_>, @@ -1096,13 +1141,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, ) { @@ -1119,15 +1164,9 @@ fn lint_or_fun_call(cx: &LateContext<'_, '_>, expr: &hir::Expr, method_span: Spa return; } - // 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/methods.rs b/tests/ui/methods.rs index 395271b37ebe..3a8aedcd6598 100644 --- a/tests/ui/methods.rs +++ b/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)); -} diff --git a/tests/ui/or_fun_call.rs b/tests/ui/or_fun_call.rs index 1b4732b5b564..f339bae8ac6d 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()); @@ -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() {} diff --git a/tests/ui/or_fun_call.stderr b/tests/ui/or_fun_call.stderr index 5d6ebb50a89e..f6e5d202e0c1 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