Fix full projection identifier + move applicability to MaybeIncorrect

ThibsG · ThibsG · commit 5ebede0c14d0 · 2021-11-20T09:40:11.000+01:00
diff --git a/clippy_lints/src/methods/search_is_some.rs b/clippy_lints/src/methods/search_is_some.rs
@@ -185,7 +185,7 @@ fn get_closure_suggestion<'tcx>(cx: &LateContext<'_>, search_expr: &'tcx hir::Ex
             closure_arg_is_double_ref,
             next_pos: search_expr.span.lo(),
             suggestion_start: String::new(),
-            applicability: Applicability::MachineApplicable,
+            applicability: Applicability::MaybeIncorrect,
         };
 
         let fn_def_id = cx.tcx.hir().local_def_id(search_expr.hir_id);
@@ -252,11 +252,15 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> {
     fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) {
         if let PlaceBase::Local(id) = cmt.place.base {
             let map = self.cx.tcx.hir();
-            let ident_str = map.name(id).to_string();
             let span = map.span(cmt.hir_id);
             let start_span = Span::new(self.next_pos, span.lo(), span.ctxt(), None);
             let mut start_snip = snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability);
 
+            // identifier referring to the variable currently triggered (i.e.: `fp`)
+            let ident_str = map.name(id).to_string();
+            // full identifier that includes projection (i.e.: `fp.field`)
+            let ident_str_with_proj = snippet(self.cx, span, "..").to_string();
+
             if cmt.place.projections.is_empty() {
                 // handle item without any projection, that needs an explicit borrowing
                 // i.e.: suggest `&x` instead of `x`
@@ -276,7 +280,8 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> {
                         // given expression is the self argument and will be handled completely by the compiler
                         // i.e.: `|x| x.is_something()`
                         ExprKind::MethodCall(_, _, [self_expr, ..], _) if self_expr.hir_id == cmt.hir_id => {
-                            self.suggestion_start.push_str(&format!("{}{}", start_snip, ident_str));
+                            self.suggestion_start
+                                .push_str(&format!("{}{}", start_snip, ident_str_with_proj));
                             self.next_pos = span.hi();
                             return;
                         },
@@ -291,13 +296,26 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> {
                                 let takes_arg_by_double_ref =
                                     self.func_takes_arg_by_double_ref(parent_expr, cmt.hir_id);
 
+                                // compiler will automatically dereference field projection, so no need
+                                // to suggest ampersand, but full identifier that includes projection is required
+                                let has_field_projection = cmt
+                                    .place
+                                    .projections
+                                    .iter()
+                                    .any(|proj| matches!(proj.kind, ProjectionKind::Field(..)));
+
                                 // no need to bind again if the function doesn't take arg by double ref
                                 // and if the item is already a double ref
                                 let ident_sugg = if !call_args.is_empty()
                                     && !takes_arg_by_double_ref
-                                    && self.closure_arg_is_double_ref
+                                    && (self.closure_arg_is_double_ref || has_field_projection)
                                 {
-                                    format!("{}{}", start_snip, ident_str)
+                                    let ident = if has_field_projection {
+                                        ident_str_with_proj
+                                    } else {
+                                        ident_str
+                                    };
+                                    format!("{}{}", start_snip, ident)
                                 } else {
                                     format!("{}&{}", start_snip, ident_str)
                                 };
@@ -318,17 +336,9 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> {
                     match proj.kind {
                         // Field projection like `|v| v.foo`
                         // no adjustment needed here, as field projections are handled by the compiler
-                        ProjectionKind::Field(idx, variant) => match cmt.place.ty_before_projection(i).kind() {
-                            ty::Adt(def, ..) => {
-                                replacement_str = format!(
-                                    "{}.{}",
-                                    replacement_str,
-                                    def.variants[variant].fields[idx as usize].ident.name.as_str()
-                                );
-                                projections_handled = true;
-                            },
-                            ty::Tuple(_) => {
-                                replacement_str = format!("{}.{}", replacement_str, idx);
+                        ProjectionKind::Field(..) => match cmt.place.ty_before_projection(i).kind() {
+                            ty::Adt(..) | ty::Tuple(_) => {
+                                replacement_str = ident_str_with_proj.clone();
                                 projections_handled = true;
                             },
                             _ => (),
diff --git a/tests/ui/search_is_some_fixable_none.fixed b/tests/ui/search_is_some_fixable_none.fixed
@@ -182,4 +182,35 @@ mod issue7392 {
         let _ = ![&(&1, 2), &(&3, 4), &(&5, 4)].iter().any(|(&x, y)| x == *y);
         let _ = ![&(&1, 2), &(&3, 4), &(&5, 4)].iter().any(|(&x, y)| x == *y);
     }
+
+    fn test_string_1(s: &str) -> bool {
+        s.is_empty()
+    }
+
+    fn test_u32_1(s: &u32) -> bool {
+        s.is_power_of_two()
+    }
+
+    fn test_u32_2(s: u32) -> bool {
+        s.is_power_of_two()
+    }
+
+    fn projection_in_args_test() {
+        // Index projections
+        let lst = &[String::from("Hello"), String::from("world")];
+        let v: Vec<&[String]> = vec![lst];
+        let _ = !v.iter().any(|s| s[0].is_empty());
+        let _ = !v.iter().any(|s| test_string_1(&s[0]));
+
+        // Field projections
+        struct FieldProjection<'a> {
+            field: &'a u32,
+        }
+        let field = 123456789;
+        let instance = FieldProjection { field: &field };
+        let v = vec![instance];
+        let _ = !v.iter().any(|fp| fp.field.is_power_of_two());
+        let _ = !v.iter().any(|fp| test_u32_1(fp.field));
+        let _ = !v.iter().any(|fp| test_u32_2(*fp.field));
+    }
 }
diff --git a/tests/ui/search_is_some_fixable_none.rs b/tests/ui/search_is_some_fixable_none.rs
@@ -188,4 +188,35 @@ mod issue7392 {
         let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().find(|(&x, y)| x == *y).is_none();
         let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().find(|&(&x, y)| x == *y).is_none();
     }
+
+    fn test_string_1(s: &String) -> bool {
+        s.is_empty()
+    }
+
+    fn test_u32_1(s: &u32) -> bool {
+        s.is_power_of_two()
+    }
+
+    fn test_u32_2(s: u32) -> bool {
+        s.is_power_of_two()
+    }
+
+    fn projection_in_args_test() {
+        // Index projections
+        let lst = &[String::from("Hello"), String::from("world")];
+        let v: Vec<&[String]> = vec![lst];
+        let _ = v.iter().find(|s| s[0].is_empty()).is_none();
+        let _ = v.iter().find(|s| test_string_1(&s[0])).is_none();
+
+        // Field projections
+        struct FieldProjection<'a> {
+            field: &'a u32,
+        }
+        let field = 123456789;
+        let instance = FieldProjection { field: &field };
+        let v = vec![instance];
+        let _ = v.iter().find(|fp| fp.field.is_power_of_two()).is_none();
+        let _ = v.iter().find(|fp| test_u32_1(fp.field)).is_none();
+        let _ = v.iter().find(|fp| test_u32_2(*fp.field)).is_none();
+    }
 }
diff --git a/tests/ui/search_is_some_fixable_none.stderr b/tests/ui/search_is_some_fixable_none.stderr
@@ -251,5 +251,43 @@ error: called `is_none()` after searching an `Iterator` with `find`
 LL |         let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().find(|&(&x, y)| x == *y).is_none();
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `![&(&1, 2), &(&3, 4), &(&5, 4)].iter().any(|(&x, y)| x == *y)`
 
-error: aborting due to 38 previous errors
+error: writing `&String` instead of `&str` involves a new object where a slice will do
+  --> $DIR/search_is_some_fixable_none.rs:192:25
+   |
+LL |     fn test_string_1(s: &String) -> bool {
+   |                         ^^^^^^^ help: change this to: `&str`
+   |
+   = note: `-D clippy::ptr-arg` implied by `-D warnings`
+
+error: called `is_none()` after searching an `Iterator` with `find`
+  --> $DIR/search_is_some_fixable_none.rs:208:17
+   |
+LL |         let _ = v.iter().find(|s| s[0].is_empty()).is_none();
+   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|s| s[0].is_empty())`
+
+error: called `is_none()` after searching an `Iterator` with `find`
+  --> $DIR/search_is_some_fixable_none.rs:209:17
+   |
+LL |         let _ = v.iter().find(|s| test_string_1(&s[0])).is_none();
+   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|s| test_string_1(&s[0]))`
+
+error: called `is_none()` after searching an `Iterator` with `find`
+  --> $DIR/search_is_some_fixable_none.rs:218:17
+   |
+LL |         let _ = v.iter().find(|fp| fp.field.is_power_of_two()).is_none();
+   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|fp| fp.field.is_power_of_two())`
+
+error: called `is_none()` after searching an `Iterator` with `find`
+  --> $DIR/search_is_some_fixable_none.rs:219:17
+   |
+LL |         let _ = v.iter().find(|fp| test_u32_1(fp.field)).is_none();
+   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|fp| test_u32_1(fp.field))`
+
+error: called `is_none()` after searching an `Iterator` with `find`
+  --> $DIR/search_is_some_fixable_none.rs:220:17
+   |
+LL |         let _ = v.iter().find(|fp| test_u32_2(*fp.field)).is_none();
+   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|fp| test_u32_2(*fp.field))`
+
+error: aborting due to 44 previous errors
 
diff --git a/tests/ui/search_is_some_fixable_some.fixed b/tests/ui/search_is_some_fixable_some.fixed
@@ -184,4 +184,35 @@ mod issue7392 {
         let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().any(|(&x, y)| x == *y);
         let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().any(|(&x, y)| x == *y);
     }
+
+    fn test_string_1(s: &str) -> bool {
+        s.is_empty()
+    }
+
+    fn test_u32_1(s: &u32) -> bool {
+        s.is_power_of_two()
+    }
+
+    fn test_u32_2(s: u32) -> bool {
+        s.is_power_of_two()
+    }
+
+    fn projection_in_args_test() {
+        // Index projections
+        let lst = &[String::from("Hello"), String::from("world")];
+        let v: Vec<&[String]> = vec![lst];
+        let _ = v.iter().any(|s| s[0].is_empty());
+        let _ = v.iter().any(|s| test_string_1(&s[0]));
+
+        // Field projections
+        struct FieldProjection<'a> {
+            field: &'a u32,
+        }
+        let field = 123456789;
+        let instance = FieldProjection { field: &field };
+        let v = vec![instance];
+        let _ = v.iter().any(|fp| fp.field.is_power_of_two());
+        let _ = v.iter().any(|fp| test_u32_1(fp.field));
+        let _ = v.iter().any(|fp| test_u32_2(*fp.field));
+    }
 }
diff --git a/tests/ui/search_is_some_fixable_some.rs b/tests/ui/search_is_some_fixable_some.rs
@@ -187,4 +187,35 @@ mod issue7392 {
         let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().find(|(&x, y)| x == *y).is_some();
         let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().find(|&(&x, y)| x == *y).is_some();
     }
+
+    fn test_string_1(s: &String) -> bool {
+        s.is_empty()
+    }
+
+    fn test_u32_1(s: &u32) -> bool {
+        s.is_power_of_two()
+    }
+
+    fn test_u32_2(s: u32) -> bool {
+        s.is_power_of_two()
+    }
+
+    fn projection_in_args_test() {
+        // Index projections
+        let lst = &[String::from("Hello"), String::from("world")];
+        let v: Vec<&[String]> = vec![lst];
+        let _ = v.iter().find(|s| s[0].is_empty()).is_some();
+        let _ = v.iter().find(|s| test_string_1(&s[0])).is_some();
+
+        // Field projections
+        struct FieldProjection<'a> {
+            field: &'a u32,
+        }
+        let field = 123456789;
+        let instance = FieldProjection { field: &field };
+        let v = vec![instance];
+        let _ = v.iter().find(|fp| fp.field.is_power_of_two()).is_some();
+        let _ = v.iter().find(|fp| test_u32_1(fp.field)).is_some();
+        let _ = v.iter().find(|fp| test_u32_2(*fp.field)).is_some();
+    }
 }
diff --git a/tests/ui/search_is_some_fixable_some.stderr b/tests/ui/search_is_some_fixable_some.stderr
@@ -234,5 +234,43 @@ error: called `is_some()` after searching an `Iterator` with `find`
 LL |         let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().find(|&(&x, y)| x == *y).is_some();
    |                                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|(&x, y)| x == *y)`
 
-error: aborting due to 38 previous errors
+error: writing `&String` instead of `&str` involves a new object where a slice will do
+  --> $DIR/search_is_some_fixable_some.rs:191:25
+   |
+LL |     fn test_string_1(s: &String) -> bool {
+   |                         ^^^^^^^ help: change this to: `&str`
+   |
+   = note: `-D clippy::ptr-arg` implied by `-D warnings`
+
+error: called `is_some()` after searching an `Iterator` with `find`
+  --> $DIR/search_is_some_fixable_some.rs:207:26
+   |
+LL |         let _ = v.iter().find(|s| s[0].is_empty()).is_some();
+   |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|s| s[0].is_empty())`
+
+error: called `is_some()` after searching an `Iterator` with `find`
+  --> $DIR/search_is_some_fixable_some.rs:208:26
+   |
+LL |         let _ = v.iter().find(|s| test_string_1(&s[0])).is_some();
+   |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|s| test_string_1(&s[0]))`
+
+error: called `is_some()` after searching an `Iterator` with `find`
+  --> $DIR/search_is_some_fixable_some.rs:217:26
+   |
+LL |         let _ = v.iter().find(|fp| fp.field.is_power_of_two()).is_some();
+   |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|fp| fp.field.is_power_of_two())`
+
+error: called `is_some()` after searching an `Iterator` with `find`
+  --> $DIR/search_is_some_fixable_some.rs:218:26
+   |
+LL |         let _ = v.iter().find(|fp| test_u32_1(fp.field)).is_some();
+   |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|fp| test_u32_1(fp.field))`
+
+error: called `is_some()` after searching an `Iterator` with `find`
+  --> $DIR/search_is_some_fixable_some.rs:219:26
+   |
+LL |         let _ = v.iter().find(|fp| test_u32_2(*fp.field)).is_some();
+   |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|fp| test_u32_2(*fp.field))`
+
+error: aborting due to 44 previous errors
 
