Auto merge of #3789 - bzzzzzz:needless_range_loop_bugfix, r=oli-obk

Make needless_range_loop not applicable to structures without iter method

Fixes https://github.com/rust-lang/rust-clippy/issues/3788

Now we will start lint indexed structure only if it has known iter or iter_mut method implemented.

clippy_lints/src/loops.rs

@@ -27,7 +27,7 @@ use syntax_pos::BytePos;
 
 use crate::utils::paths;
 use crate::utils::{
-    get_enclosing_block, get_parent_expr, higher, is_integer_literal, is_refutable, last_path_segment,
+    get_enclosing_block, get_parent_expr, has_iter_method, higher, is_integer_literal, is_refutable, last_path_segment,
     match_trait_method, match_type, match_var, multispan_sugg, snippet, snippet_opt, snippet_with_applicability,
     span_help_and_lint, span_lint, span_lint_and_sugg, span_lint_and_then, SpanlessEq,
 };
@@ -1118,6 +1118,12 @@ fn check_for_loop_range<'a, 'tcx>(
                     }
                 }
 
+                // don't lint if the container that is indexed does not have .iter() method
+                let has_iter = has_iter_method(cx, indexed_ty);
+                if has_iter.is_none() {
+                    return;
+                }
+
                 // don't lint if the container that is indexed into is also used without
                 // indexing
                 if visitor.referenced.contains(&indexed) {

clippy_lints/src/methods/mod.rs

@@ -1,11 +1,11 @@
 use crate::utils::paths;
 use crate::utils::sugg;
 use crate::utils::{
-    get_arg_name, get_parent_expr, get_trait_def_id, implements_trait, in_macro, is_copy, is_expn_of, is_self,
-    is_self_ty, iter_input_pats, last_path_segment, match_def_path, match_path, match_qpath, match_trait_method,
-    match_type, match_var, method_calls, method_chain_args, remove_blocks, return_ty, same_tys, single_segment_path,
-    snippet, snippet_with_applicability, snippet_with_macro_callsite, span_lint, span_lint_and_sugg,
-    span_lint_and_then, span_note_and_lint, walk_ptrs_ty, walk_ptrs_ty_depth, SpanlessEq,
+    get_arg_name, get_parent_expr, get_trait_def_id, has_iter_method, implements_trait, in_macro, is_copy, is_expn_of,
+    is_self, is_self_ty, iter_input_pats, last_path_segment, match_def_path, match_path, match_qpath,
+    match_trait_method, match_type, match_var, method_calls, method_chain_args, remove_blocks, return_ty, same_tys,
+    single_segment_path, snippet, snippet_with_applicability, snippet_with_macro_callsite, span_lint,
+    span_lint_and_sugg, span_lint_and_then, span_note_and_lint, walk_ptrs_ty, walk_ptrs_ty_depth, SpanlessEq,
 };
 use if_chain::if_chain;
 use matches::matches;
@@ -2237,47 +2237,23 @@ fn ty_has_iter_method(
     cx: &LateContext<'_, '_>,
     self_ref_ty: ty::Ty<'_>,
 ) -> Option<(&'static Lint, &'static str, &'static str)> {
-    // FIXME: instead of this hard-coded list, we should check if `<adt>::iter`
-    // exists and has the desired signature. Unfortunately FnCtxt is not exported
-    // so we can't use its `lookup_method` method.
-    static INTO_ITER_COLLECTIONS: [(&Lint, &[&str]); 13] = [
-        (INTO_ITER_ON_REF, &paths::VEC),
-        (INTO_ITER_ON_REF, &paths::OPTION),
-        (INTO_ITER_ON_REF, &paths::RESULT),
-        (INTO_ITER_ON_REF, &paths::BTREESET),
-        (INTO_ITER_ON_REF, &paths::BTREEMAP),
-        (INTO_ITER_ON_REF, &paths::VEC_DEQUE),
-        (INTO_ITER_ON_REF, &paths::LINKED_LIST),
-        (INTO_ITER_ON_REF, &paths::BINARY_HEAP),
-        (INTO_ITER_ON_REF, &paths::HASHSET),
-        (INTO_ITER_ON_REF, &paths::HASHMAP),
-        (INTO_ITER_ON_ARRAY, &["std", "path", "PathBuf"]),
-        (INTO_ITER_ON_REF, &["std", "path", "Path"]),
-        (INTO_ITER_ON_REF, &["std", "sync", "mpsc", "Receiver"]),
-    ];
-
-    let (self_ty, mutbl) = match self_ref_ty.sty {
-        ty::Ref(_, self_ty, mutbl) => (self_ty, mutbl),
-        _ => unreachable!(),
-    };
-    let method_name = match mutbl {
-        hir::MutImmutable => "iter",
-        hir::MutMutable => "iter_mut",
-    };
-
-    let def_id = match self_ty.sty {
-        ty::Array(..) => return Some((INTO_ITER_ON_ARRAY, "array", method_name)),
-        ty::Slice(..) => return Some((INTO_ITER_ON_REF, "slice", method_name)),
-        ty::Adt(adt, _) => adt.did,
-        _ => return None,
-    };
-
-    for (lint, path) in &INTO_ITER_COLLECTIONS {
-        if match_def_path(cx.tcx, def_id, path) {
-            return Some((lint, path.last().unwrap(), method_name));
-        }
+    if let Some(ty_name) = has_iter_method(cx, self_ref_ty) {
+        let lint = match ty_name {
+            "array" | "PathBuf" => INTO_ITER_ON_ARRAY,
+            _ => INTO_ITER_ON_REF,
+        };
+        let mutbl = match self_ref_ty.sty {
+            ty::Ref(_, _, mutbl) => mutbl,
+            _ => unreachable!(),
+        };
+        let method_name = match mutbl {
+            hir::MutImmutable => "iter",
+            hir::MutMutable => "iter_mut",
+        };
+        Some((lint, ty_name, method_name))
+    } else {
+        None
     }
-    None
 }
 
 fn lint_into_iter(cx: &LateContext<'_, '_>, expr: &hir::Expr, self_ref_ty: ty::Ty<'_>, method_span: Span) {

clippy_lints/src/utils/mod.rs

@@ -1155,6 +1155,47 @@ pub fn any_parent_is_automatically_derived(tcx: TyCtxt<'_, '_, '_>, node: NodeId
     false
 }
 
+/// Returns true if ty has `iter` or `iter_mut` methods
+pub fn has_iter_method(cx: &LateContext<'_, '_>, probably_ref_ty: ty::Ty<'_>) -> Option<&'static str> {
+    // FIXME: instead of this hard-coded list, we should check if `<adt>::iter`
+    // exists and has the desired signature. Unfortunately FnCtxt is not exported
+    // so we can't use its `lookup_method` method.
+    static INTO_ITER_COLLECTIONS: [&[&str]; 13] = [
+        &paths::VEC,
+        &paths::OPTION,
+        &paths::RESULT,
+        &paths::BTREESET,
+        &paths::BTREEMAP,
+        &paths::VEC_DEQUE,
+        &paths::LINKED_LIST,
+        &paths::BINARY_HEAP,
+        &paths::HASHSET,
+        &paths::HASHMAP,
+        &paths::PATH_BUF,
+        &paths::PATH,
+        &paths::RECEIVER,
+    ];
+
+    let ty_to_check = match probably_ref_ty.sty {
+        ty::Ref(_, ty_to_check, _) => ty_to_check,
+        _ => probably_ref_ty,
+    };
+
+    let def_id = match ty_to_check.sty {
+        ty::Array(..) => return Some("array"),
+        ty::Slice(..) => return Some("slice"),
+        ty::Adt(adt, _) => adt.did,
+        _ => return None,
+    };
+
+    for path in &INTO_ITER_COLLECTIONS {
+        if match_def_path(cx.tcx, def_id, path) {
+            return Some(path.last().unwrap());
+        }
+    }
+    None
+}
+
 #[cfg(test)]
 mod test {
     use super::{trim_multiline, without_block_comments};

clippy_lints/src/utils/paths.rs

@@ -62,6 +62,7 @@ pub const ORD: [&str; 3] = ["core", "cmp", "Ord"];
 pub const OS_STRING: [&str; 4] = ["std", "ffi", "os_str", "OsString"];
 pub const OS_STR_TO_OS_STRING: [&str; 5] = ["std", "ffi", "os_str", "OsStr", "to_os_string"];
 pub const PARTIAL_ORD: [&str; 3] = ["core", "cmp", "PartialOrd"];
+pub const PATH: [&str; 3] = ["std", "path", "Path"];
 pub const PATH_BUF: [&str; 3] = ["std", "path", "PathBuf"];
 pub const PATH_TO_PATH_BUF: [&str; 4] = ["std", "path", "Path", "to_path_buf"];
 pub const PTR_NULL: [&str; 2] = ["ptr", "null"];
@@ -80,6 +81,7 @@ pub const RANGE_TO_INCLUSIVE: [&str; 3] = ["core", "ops", "RangeToInclusive"];
 pub const RANGE_TO_INCLUSIVE_STD: [&str; 3] = ["std", "ops", "RangeToInclusive"];
 pub const RANGE_TO_STD: [&str; 3] = ["std", "ops", "RangeTo"];
 pub const RC: [&str; 3] = ["alloc", "rc", "Rc"];
+pub const RECEIVER: [&str; 4] = ["std", "sync", "mpsc", "Receiver"];
 pub const REGEX: [&str; 3] = ["regex", "re_unicode", "Regex"];
 pub const REGEX_BUILDER_NEW: [&str; 5] = ["regex", "re_builder", "unicode", "RegexBuilder", "new"];
 pub const REGEX_BYTES_BUILDER_NEW: [&str; 5] = ["regex", "re_builder", "bytes", "RegexBuilder", "new"];

tests/ui/needless_range_loop.rs

@@ -85,4 +85,23 @@ fn main() {
     for i in 0..vec.len() {
         vec[i] = Some(1).unwrap_or_else(|| panic!("error on {}", i));
     }
+
+    // #3788
+    let test = Test {
+        inner: vec![1, 2, 3, 4],
+    };
+    for i in 0..2 {
+        println!("{}", test[i]);
+    }
+}
+
+struct Test {
+    inner: Vec<usize>,
+}
+
+impl std::ops::Index<usize> for Test {
+    type Output = usize;
+    fn index(&self, index: usize) -> &Self::Output {
+        &self.inner[index]
+    }
 }