Auto merge of rust-lang#6267 - camsteffen:or-fun-idx, r=flip1995

Fix or_fun_call for index operator changelog: Fix or_fun_call for index operator Fixes rust-lang#6266
flip1995 · Nov 8, 2020 · 2067a01 · 2067a01
2 parents 040d0ca + 9cab084
commit 2067a01
Show file tree

Hide file tree

Showing 5 changed files with 68 additions and 40 deletions.
diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
@@ -1797,12 +1797,20 @@ 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] = [
+            (&paths::BTREEMAP_ENTRY, false, &["or_insert"], "with"),
+            (&paths::HASHMAP_ENTRY, false, &["or_insert"], "with"),
+            (&paths::OPTION, false, &["map_or", "ok_or", "or", "unwrap_or"], "else"),
+            (&paths::RESULT, true, &["or", "unwrap_or"], "else"),
+        ];
+
         if let hir::ExprKind::MethodCall(ref path, _, ref args, _) = &arg.kind {
             if path.ident.as_str() == "len" {
                 let ty = cx.typeck_results().expr_ty(&args[0]).peel_refs();
@@ -1818,32 +1826,32 @@ fn lint_or_fun_call<'tcx>(
             }
         }
 
-        // (path, fn_has_argument, methods, suffix)
-        let know_types: &[(&[_], _, &[_], _)] = &[
-            (&paths::BTREEMAP_ENTRY, false, &["or_insert"], "with"),
-            (&paths::HASHMAP_ENTRY, false, &["or_insert"], "with"),
-            (&paths::OPTION, false, &["map_or", "ok_or", "or", "unwrap_or"], "else"),
-            (&paths::RESULT, true, &["or", "unwrap_or"], "else"),
-        ];
-
         if_chain! {
-            if know_types.iter().any(|k| k.2.contains(&name));
+            if KNOW_TYPES.iter().any(|k| k.2.contains(&name));
 
             if is_lazyness_candidate(cx, arg);
             if !contains_return(&arg);
 
             let self_ty = cx.typeck_results().expr_ty(self_expr);
 
             if let Some(&(_, fn_has_arguments, poss, suffix)) =
-                know_types.iter().find(|&&i| match_type(cx, self_ty, i.0));
+                KNOW_TYPES.iter().find(|&&i| match_type(cx, self_ty, i.0));
 
             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(
@@ -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);
+            },
             _ => {},
         }
     }

diff --git a/clippy_lints/src/utils/eager_or_lazy.rs b/clippy_lints/src/utils/eager_or_lazy.rs
@@ -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;
@@ -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,
             };

diff --git a/tests/ui/or_fun_call.fixed b/tests/ui/or_fun_call.fixed
@@ -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);

diff --git a/tests/ui/or_fun_call.rs b/tests/ui/or_fun_call.rs
@@ -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);

diff --git a/tests/ui/or_fun_call.stderr b/tests/ui/or_fun_call.stderr
@@ -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