Skip to content

Commit

Permalink
Auto merge of #4084 - mikerite:fix-4019, r=oli-obk
Browse files Browse the repository at this point in the history 
Fix 4019

Fixes #4019
  • Loading branch information
@bors
bors committed May 13, 2019
2 parents c79838e + 2efd8c6 commit 3edb395
Show file tree
Hide file tree
Showing 4 changed files with 100 additions and 47 deletions.
65 changes: 52 additions & 13 deletions clippy_lints/src/methods/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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<'_, '_>,
Expand Down Expand Up @@ -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,
) {
Expand All @@ -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;
}

Expand Down
18 changes: 0 additions & 18 deletions tests/ui/methods.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
30 changes: 28 additions & 2 deletions tests/ui/or_fun_call.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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());
Expand Down Expand Up @@ -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() {}
34 changes: 20 additions & 14 deletions tests/ui/or_fun_call.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,82 +1,88 @@
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)`
|
= 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

0 comments on commit 3edb395

Please sign in to comment.