Skip to content

Commit 501830b

Browse files
borsbors
committed
Auto merge of #4084 - mikerite:fix-4019, r=oli-obk
Fix 4019 Fixes #4019
2 parents ad3269c + 2efd8c6 commit 501830b

File tree

4 files changed

+100
-47
lines changed

4 files changed

+100
-47
lines changed

‎clippy_lints/src/methods/mod.rs

+52-13
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ use lazy_static::lazy_static;
1010
use matches::matches;
1111
use rustc::hir;
1212
use rustc::hir::def::{DefKind, Res};
13+
use rustc::hir::intravisit::{self, Visitor};
1314
use rustc::lint::{in_external_macro, LateContext, LateLintPass, Lint, LintArray, LintContext, LintPass};
1415
use rustc::ty::{self, Predicate, Ty};
1516
use rustc::{declare_lint_pass, declare_tool_lint};
@@ -1046,7 +1047,51 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
10461047

10471048
/// Checks for the `OR_FUN_CALL` lint.
10481049
#[allow(clippy::too_many_lines)]
1049-
fn lint_or_fun_call(cx: &LateContext<'_, '_>, expr: &hir::Expr, method_span: Span, name: &str, args: &[hir::Expr]) {
1050+
fn lint_or_fun_call<'a, 'tcx: 'a>(
1051+
cx: &LateContext<'a, 'tcx>,
1052+
expr: &hir::Expr,
1053+
method_span: Span,
1054+
name: &str,
1055+
args: &'tcx [hir::Expr],
1056+
) {
1057+
// Searches an expression for method calls or function calls that aren't ctors
1058+
struct FunCallFinder<'a, 'tcx: 'a> {
1059+
cx: &'a LateContext<'a, 'tcx>,
1060+
found: bool,
1061+
}
1062+
1063+
impl<'a, 'tcx> intravisit::Visitor<'tcx> for FunCallFinder<'a, 'tcx> {
1064+
fn visit_expr(&mut self, expr: &'tcx hir::Expr) {
1065+
let call_found = match &expr.node {
1066+
// ignore enum and struct constructors
1067+
hir::ExprKind::Call(..) => !is_ctor_function(self.cx, expr),
1068+
hir::ExprKind::MethodCall(..) => true,
1069+
_ => false,
1070+
};
1071+
1072+
if call_found {
1073+
// don't lint for constant values
1074+
let owner_def = self.cx.tcx.hir().get_parent_did_by_hir_id(expr.hir_id);
1075+
let promotable = self
1076+
.cx
1077+
.tcx
1078+
.rvalue_promotable_map(owner_def)
1079+
.contains(&expr.hir_id.local_id);
1080+
if !promotable {
1081+
self.found |= true;
1082+
}
1083+
}
1084+
1085+
if !self.found {
1086+
intravisit::walk_expr(self, expr);
1087+
}
1088+
}
1089+
1090+
fn nested_visit_map<'this>(&'this mut self) -> intravisit::NestedVisitorMap<'this, 'tcx> {
1091+
intravisit::NestedVisitorMap::None
1092+
}
1093+
}
1094+
10501095
/// Checks for `unwrap_or(T::new())` or `unwrap_or(T::default())`.
10511096
fn check_unwrap_or_default(
10521097
cx: &LateContext<'_, '_>,
@@ -1099,13 +1144,13 @@ fn lint_or_fun_call(cx: &LateContext<'_, '_>, expr: &hir::Expr, method_span: Spa
10991144

11001145
/// Checks for `*or(foo())`.
11011146
#[allow(clippy::too_many_arguments)]
1102-
fn check_general_case(
1103-
cx: &LateContext<'_, '_>,
1147+
fn check_general_case<'a, 'tcx: 'a>(
1148+
cx: &LateContext<'a, 'tcx>,
11041149
name: &str,
11051150
method_span: Span,
11061151
fun_span: Span,
11071152
self_expr: &hir::Expr,
1108-
arg: &hir::Expr,
1153+
arg: &'tcx hir::Expr,
11091154
or_has_args: bool,
11101155
span: Span,
11111156
) {
@@ -1122,15 +1167,9 @@ fn lint_or_fun_call(cx: &LateContext<'_, '_>, expr: &hir::Expr, method_span: Spa
11221167
return;
11231168
}
11241169

1125-
// ignore enum and struct constructors
1126-
if is_ctor_function(cx, &arg) {
1127-
return;
1128-
}
1129-
1130-
// don't lint for constant values
1131-
let owner_def = cx.tcx.hir().get_parent_did_by_hir_id(arg.hir_id);
1132-
let promotable = cx.tcx.rvalue_promotable_map(owner_def).contains(&arg.hir_id.local_id);
1133-
if promotable {
1170+
let mut finder = FunCallFinder { cx: &cx, found: false };
1171+
finder.visit_expr(&arg);
1172+
if !finder.found {
11341173
return;
11351174
}
11361175

‎tests/ui/methods.rs

-18
Original file line numberDiff line numberDiff line change
@@ -268,21 +268,3 @@ fn main() {
268268
let opt = Some(0);
269269
let _ = opt.unwrap();
270270
}
271-
272-
struct Foo(u8);
273-
#[rustfmt::skip]
274-
fn test_or_with_ctors() {
275-
let opt = Some(1);
276-
let opt_opt = Some(Some(1));
277-
// we also test for const promotion, this makes sure we don't hit that
278-
let two = 2;
279-
280-
let _ = opt_opt.unwrap_or(Some(2));
281-
let _ = opt_opt.unwrap_or(Some(two));
282-
let _ = opt.ok_or(Some(2));
283-
let _ = opt.ok_or(Some(two));
284-
let _ = opt.ok_or(Foo(2));
285-
let _ = opt.ok_or(Foo(two));
286-
let _ = opt.or(Some(2));
287-
let _ = opt.or(Some(two));
288-
}

‎tests/ui/or_fun_call.rs

+28-2
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
use std::collections::BTreeMap;
44
use std::collections::HashMap;
5+
use std::time::Duration;
56

67
/// Checks implementation of the `OR_FUN_CALL` lint.
78
fn or_fun_call() {
@@ -24,8 +25,8 @@ fn or_fun_call() {
2425
let with_enum = Some(Enum::A(1));
2526
with_enum.unwrap_or(Enum::A(5));
2627

27-
let with_const_fn = Some(::std::time::Duration::from_secs(1));
28-
with_const_fn.unwrap_or(::std::time::Duration::from_secs(5));
28+
let with_const_fn = Some(Duration::from_secs(1));
29+
with_const_fn.unwrap_or(Duration::from_secs(5));
2930

3031
let with_constructor = Some(vec![1]);
3132
with_constructor.unwrap_or(make());
@@ -70,4 +71,29 @@ fn or_fun_call() {
7071
let _ = opt.ok_or(format!("{} world.", hello));
7172
}
7273

74+
struct Foo(u8);
75+
struct Bar(String, Duration);
76+
#[rustfmt::skip]
77+
fn test_or_with_ctors() {
78+
let opt = Some(1);
79+
let opt_opt = Some(Some(1));
80+
// we also test for const promotion, this makes sure we don't hit that
81+
let two = 2;
82+
83+
let _ = opt_opt.unwrap_or(Some(2));
84+
let _ = opt_opt.unwrap_or(Some(two));
85+
let _ = opt.ok_or(Some(2));
86+
let _ = opt.ok_or(Some(two));
87+
let _ = opt.ok_or(Foo(2));
88+
let _ = opt.ok_or(Foo(two));
89+
let _ = opt.or(Some(2));
90+
let _ = opt.or(Some(two));
91+
92+
let _ = Some("a".to_string()).or(Some("b".to_string()));
93+
94+
let b = "b".to_string();
95+
let _ = Some(Bar("a".to_string(), Duration::from_secs(1)))
96+
.or(Some(Bar(b, Duration::from_secs(2))));
97+
}
98+
7399
fn main() {}

‎tests/ui/or_fun_call.stderr

+20-14
Original file line numberDiff line numberDiff line change
@@ -1,82 +1,88 @@
11
error: use of `unwrap_or` followed by a function call
2-
--> $DIR/or_fun_call.rs:31:22
2+
--> $DIR/or_fun_call.rs:32:22
33
|
44
LL | with_constructor.unwrap_or(make());
55
| ^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(make)`
66
|
77
= note: `-D clippy::or-fun-call` implied by `-D warnings`
88

99
error: use of `unwrap_or` followed by a call to `new`
10-
--> $DIR/or_fun_call.rs:34:5
10+
--> $DIR/or_fun_call.rs:35:5
1111
|
1212
LL | with_new.unwrap_or(Vec::new());
1313
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_new.unwrap_or_default()`
1414

1515
error: use of `unwrap_or` followed by a function call
16-
--> $DIR/or_fun_call.rs:37:21
16+
--> $DIR/or_fun_call.rs:38:21
1717
|
1818
LL | with_const_args.unwrap_or(Vec::with_capacity(12));
1919
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| Vec::with_capacity(12))`
2020

2121
error: use of `unwrap_or` followed by a function call
22-
--> $DIR/or_fun_call.rs:40:14
22+
--> $DIR/or_fun_call.rs:41:14
2323
|
2424
LL | with_err.unwrap_or(make());
2525
| ^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|_| make())`
2626

2727
error: use of `unwrap_or` followed by a function call
28-
--> $DIR/or_fun_call.rs:43:19
28+
--> $DIR/or_fun_call.rs:44:19
2929
|
3030
LL | with_err_args.unwrap_or(Vec::with_capacity(12));
3131
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|_| Vec::with_capacity(12))`
3232

3333
error: use of `unwrap_or` followed by a call to `default`
34-
--> $DIR/or_fun_call.rs:46:5
34+
--> $DIR/or_fun_call.rs:47:5
3535
|
3636
LL | with_default_trait.unwrap_or(Default::default());
3737
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_default_trait.unwrap_or_default()`
3838

3939
error: use of `unwrap_or` followed by a call to `default`
40-
--> $DIR/or_fun_call.rs:49:5
40+
--> $DIR/or_fun_call.rs:50:5
4141
|
4242
LL | with_default_type.unwrap_or(u64::default());
4343
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_default_type.unwrap_or_default()`
4444

4545
error: use of `unwrap_or` followed by a function call
46-
--> $DIR/or_fun_call.rs:52:14
46+
--> $DIR/or_fun_call.rs:53:14
4747
|
4848
LL | with_vec.unwrap_or(vec![]);
4949
| ^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| vec![])`
5050

5151
error: use of `unwrap_or` followed by a function call
52-
--> $DIR/or_fun_call.rs:57:21
52+
--> $DIR/or_fun_call.rs:58:21
5353
|
5454
LL | without_default.unwrap_or(Foo::new());
5555
| ^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(Foo::new)`
5656

5757
error: use of `or_insert` followed by a function call
58-
--> $DIR/or_fun_call.rs:60:19
58+
--> $DIR/or_fun_call.rs:61:19
5959
|
6060
LL | map.entry(42).or_insert(String::new());
6161
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `or_insert_with(String::new)`
6262

6363
error: use of `or_insert` followed by a function call
64-
--> $DIR/or_fun_call.rs:63:21
64+
--> $DIR/or_fun_call.rs:64:21
6565
|
6666
LL | btree.entry(42).or_insert(String::new());
6767
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `or_insert_with(String::new)`
6868

6969
error: use of `unwrap_or` followed by a function call
70-
--> $DIR/or_fun_call.rs:66:21
70+
--> $DIR/or_fun_call.rs:67:21
7171
|
7272
LL | let _ = stringy.unwrap_or("".to_owned());
7373
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| "".to_owned())`
7474

7575
error: use of `ok_or` followed by a function call
76-
--> $DIR/or_fun_call.rs:70:17
76+
--> $DIR/or_fun_call.rs:71:17
7777
|
7878
LL | let _ = opt.ok_or(format!("{} world.", hello));
7979
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `ok_or_else(|| format!("{} world.", hello))`
8080

81-
error: aborting due to 13 previous errors
81+
error: use of `or` followed by a function call
82+
--> $DIR/or_fun_call.rs:92:35
83+
|
84+
LL | let _ = Some("a".to_string()).or(Some("b".to_string()));
85+
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `or_else(|| Some("b".to_string()))`
86+
87+
error: aborting due to 14 previous errors
8288

0 commit comments

Comments
 (0)