From c44a882a84313877b48ff3793288fa4b69c6220b Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Sun, 12 May 2019 09:32:39 +0200 Subject: [PATCH 1/3] Move ctor tests from methods.rs to or_fun_calls.rs --- tests/ui/methods.rs | 18 ------------------ tests/ui/or_fun_call.rs | 18 ++++++++++++++++++ 2 files changed, 18 insertions(+), 18 deletions(-) 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..be2165c222f3 100644 --- a/tests/ui/or_fun_call.rs +++ b/tests/ui/or_fun_call.rs @@ -70,4 +70,22 @@ fn or_fun_call() { let _ = opt.ok_or(format!("{} world.", hello)); } +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)); +} + fn main() {} From e6e3f24e0cdea503be1a2e8c47297106d26d9aaa Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Sun, 12 May 2019 10:18:38 +0200 Subject: [PATCH 2/3] Fix #4019 --- clippy_lints/src/methods/mod.rs | 62 ++++++++++++++++++++++++++------- tests/ui/or_fun_call.rs | 12 +++++-- tests/ui/or_fun_call.stderr | 34 ++++++++++-------- 3 files changed, 80 insertions(+), 28 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index f8cde663d899..be2b7a26e570 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 be2165c222f3..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()); @@ -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 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 From 2efd8c6e05777a0245437e9b9d1e47431701faf0 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Sun, 12 May 2019 10:32:19 +0200 Subject: [PATCH 3/3] Fix comments; minor refactoring --- clippy_lints/src/methods/mod.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index be2b7a26e570..fb13598dea9a 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -1060,13 +1060,15 @@ fn lint_or_fun_call<'a, 'tcx: 'a>( impl<'a, 'tcx> intravisit::Visitor<'tcx> for FunCallFinder<'a, 'tcx> { fn visit_expr(&mut self, expr: &'tcx hir::Expr) { - let found = match &expr.node { + 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 found { + 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 @@ -1162,7 +1164,6 @@ fn lint_or_fun_call<'a, 'tcx: 'a>( return; } - // ignore enum and struct constructors let mut finder = FunCallFinder { cx: &cx, found: false }; finder.visit_expr(&arg); if !finder.found {