Skip to content

Commit

Permalink
Fix or_fun_call for index operator
Browse files Browse the repository at this point in the history
  • Loading branch information
camsteffen committed Nov 8, 2020
1 parent b099406 commit 9cab084
Show file tree
Hide file tree
Showing 5 changed files with 58 additions and 30 deletions.
45 changes: 19 additions & 26 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1797,11 +1797,11 @@ fn lint_or_fun_call<'tcx>(
cx: &LateContext<'tcx>,
name: &str,
method_span: Span,
fun_span: Span,
self_expr: &hir::Expr<'_>,
arg: &'tcx hir::Expr<'_>,
or_has_args: bool,
span: Span,
// None if lambda is required
fun_span: Option<Span>,
) {
// (path, fn_has_argument, methods, suffix)
static KNOW_TYPES: [(&[&str], bool, &[&str], &str); 4] = [
Expand Down Expand Up @@ -1840,10 +1840,18 @@ fn lint_or_fun_call<'tcx>(
if poss.contains(&name);

then {
let sugg: Cow<'_, _> = match (fn_has_arguments, !or_has_args) {
(true, _) => format!("|_| {}", snippet_with_macro_callsite(cx, arg.span, "..")).into(),
(false, false) => format!("|| {}", snippet_with_macro_callsite(cx, arg.span, "..")).into(),
(false, true) => snippet_with_macro_callsite(cx, fun_span, ".."),
let sugg: Cow<'_, str> = {
let (snippet_span, use_lambda) = match (fn_has_arguments, fun_span) {
(false, Some(fun_span)) => (fun_span, false),
_ => (arg.span, true),
};
let snippet = snippet_with_macro_callsite(cx, snippet_span, "..");
if use_lambda {
let l_arg = if fn_has_arguments { "_" } else { "" };
format!("|{}| {}", l_arg, snippet).into()
} else {
snippet
}
};
let span_replace_word = method_span.with_hi(span.hi());
span_lint_and_sugg(
Expand All @@ -1864,28 +1872,13 @@ fn lint_or_fun_call<'tcx>(
hir::ExprKind::Call(ref fun, ref or_args) => {
let or_has_args = !or_args.is_empty();
if !check_unwrap_or_default(cx, name, fun, &args[0], &args[1], or_has_args, expr.span) {
check_general_case(
cx,
name,
method_span,
fun.span,
&args[0],
&args[1],
or_has_args,
expr.span,
);
let fun_span = if or_has_args { None } else { Some(fun.span) };
check_general_case(cx, name, method_span, &args[0], &args[1], expr.span, fun_span);
}
},
hir::ExprKind::MethodCall(_, span, ref or_args, _) => check_general_case(
cx,
name,
method_span,
span,
&args[0],
&args[1],
!or_args.is_empty(),
expr.span,
),
hir::ExprKind::Index(..) | hir::ExprKind::MethodCall(..) => {
check_general_case(cx, name, method_span, &args[0], &args[1], expr.span, None);
},
_ => {},
}
}
Expand Down
7 changes: 6 additions & 1 deletion clippy_lints/src/utils/eager_or_lazy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
//! - or-fun-call
//! - option-if-let-else

use crate::utils::is_ctor_or_promotable_const_function;
use crate::utils::{is_ctor_or_promotable_const_function, is_type_diagnostic_item, match_type, paths};
use rustc_hir::def::{DefKind, Res};

use rustc_hir::intravisit;
Expand Down Expand Up @@ -96,6 +96,11 @@ fn identify_some_potentially_expensive_patterns<'tcx>(cx: &LateContext<'tcx>, ex
let call_found = match &expr.kind {
// ignore enum and struct constructors
ExprKind::Call(..) => !is_ctor_or_promotable_const_function(self.cx, expr),
ExprKind::Index(obj, _) => {
let ty = self.cx.typeck_results().expr_ty(obj);
is_type_diagnostic_item(self.cx, ty, sym!(hashmap_type))
|| match_type(self.cx, ty, &paths::BTREEMAP)
},
ExprKind::MethodCall(..) => true,
_ => false,
};
Expand Down
9 changes: 9 additions & 0 deletions tests/ui/or_fun_call.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,15 @@ fn or_fun_call() {
let opt = Some(1);
let hello = "Hello";
let _ = opt.ok_or(format!("{} world.", hello));

// index
let map = HashMap::<u64, u64>::new();
let _ = Some(1).unwrap_or_else(|| map[&1]);
let map = BTreeMap::<u64, u64>::new();
let _ = Some(1).unwrap_or_else(|| map[&1]);
// don't lint index vec
let vec = vec![1];
let _ = Some(1).unwrap_or(vec[1]);
}

struct Foo(u8);
Expand Down
9 changes: 9 additions & 0 deletions tests/ui/or_fun_call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,15 @@ fn or_fun_call() {
let opt = Some(1);
let hello = "Hello";
let _ = opt.ok_or(format!("{} world.", hello));

// index
let map = HashMap::<u64, u64>::new();
let _ = Some(1).unwrap_or(map[&1]);
let map = BTreeMap::<u64, u64>::new();
let _ = Some(1).unwrap_or(map[&1]);
// don't lint index vec
let vec = vec![1];
let _ = Some(1).unwrap_or(vec[1]);
}

struct Foo(u8);
Expand Down
18 changes: 15 additions & 3 deletions tests/ui/or_fun_call.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -78,17 +78,29 @@ error: use of `unwrap_or` followed by a function call
LL | let _ = stringy.unwrap_or("".to_owned());
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| "".to_owned())`

error: use of `unwrap_or` followed by a function call
--> $DIR/or_fun_call.rs:76:21
|
LL | let _ = Some(1).unwrap_or(map[&1]);
| ^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| map[&1])`

error: use of `unwrap_or` followed by a function call
--> $DIR/or_fun_call.rs:78:21
|
LL | let _ = Some(1).unwrap_or(map[&1]);
| ^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| map[&1])`

error: use of `or` followed by a function call
--> $DIR/or_fun_call.rs:93:35
--> $DIR/or_fun_call.rs:102:35
|
LL | let _ = Some("a".to_string()).or(Some("b".to_string()));
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `or_else(|| Some("b".to_string()))`

error: use of `or` followed by a function call
--> $DIR/or_fun_call.rs:97:10
--> $DIR/or_fun_call.rs:106:10
|
LL | .or(Some(Bar(b, Duration::from_secs(2))));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `or_else(|| Some(Bar(b, Duration::from_secs(2))))`

error: aborting due to 15 previous errors
error: aborting due to 17 previous errors

0 comments on commit 9cab084

Please sign in to comment.