Auto merge of #114938 - flip1995:clippy_backport, r=matthiaskrgr

Clippy backport r? `@Manishearth` This is the accompanying PR to https://github.com/rust-lang/rust/pull/114937. This needs to be merged before tomorrow, so that it gets into master, before beta is branched. The second commit is pretty much an out-of cycle sync, so that we don't get backport-debt for next release cycle right away. cc `@Mark-Simulacrum` also mentioning you here, to make sure this is included in the beta branching.
2023-08-18 02:06:50 +00:00 · 2023-08-18 02:06:50 +00:00 · fe3eae3f31
commit fe3eae3f31
parent 9b4119009e ff89efeda2
8 changed files with 201 additions and 36 deletions
--- a/src/tools/clippy/clippy_lints/src/needless_pass_by_ref_mut.rs
+++ b/src/tools/clippy/clippy_lints/src/needless_pass_by_ref_mut.rs
@ -5,14 +5,16 @@
 use rustc_data_structures::fx::{FxHashSet, FxIndexMap};
 use rustc_errors::Applicability;
 use rustc_hir::intravisit::{walk_qpath, FnKind, Visitor};
-use rustc_hir::{Body, ExprKind, FnDecl, HirId, HirIdMap, HirIdSet, Impl, ItemKind, Mutability, Node, PatKind, QPath};
+use rustc_hir::{
+    Body, Closure, Expr, ExprKind, FnDecl, HirId, HirIdMap, HirIdSet, Impl, ItemKind, Mutability, Node, PatKind, QPath,
+};
 use rustc_hir_typeck::expr_use_visitor as euv;
 use rustc_infer::infer::TyCtxtInferExt;
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::hir::map::associated_body;
 use rustc_middle::hir::nested_filter::OnlyBodies;
 use rustc_middle::mir::FakeReadCause;
-use rustc_middle::ty::{self, Ty, UpvarId, UpvarPath};
+use rustc_middle::ty::{self, Ty, TyCtxt, UpvarId, UpvarPath};
 use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::def_id::LocalDefId;
 use rustc_span::symbol::kw;
@ -147,13 +149,26 @@ fn check_fn(
        // Collect variables mutably used and spans which will need dereferencings from the
        // function body.
        let MutablyUsedVariablesCtxt { mutably_used_vars, .. } = {
-            let mut ctx = MutablyUsedVariablesCtxt::default();
+            let mut ctx = MutablyUsedVariablesCtxt {
+                mutably_used_vars: HirIdSet::default(),
+                prev_bind: None,
+                prev_move_to_closure: HirIdSet::default(),
+                aliases: HirIdMap::default(),
+                async_closures: FxHashSet::default(),
+                tcx: cx.tcx,
+            };
            let infcx = cx.tcx.infer_ctxt().build();
            euv::ExprUseVisitor::new(&mut ctx, &infcx, fn_def_id, cx.param_env, cx.typeck_results()).consume_body(body);
            if is_async {
+                let mut checked_closures = FxHashSet::default();
+                while !ctx.async_closures.is_empty() {
                    let closures = ctx.async_closures.clone();
+                    ctx.async_closures.clear();
                    let hir = cx.tcx.hir();
                    for closure in closures {
+                        if !checked_closures.insert(closure) {
+                            continue;
+                        }
                        ctx.prev_bind = None;
                        ctx.prev_move_to_closure.clear();
                        if let Some(body) = hir
@ -166,6 +181,7 @@ fn check_fn(
                        }
                    }
                }
+            }
            ctx
        };
        for ((&input, &_), arg) in it {
@ -225,16 +241,16 @@ fn check_crate_post(&mut self, cx: &LateContext<'tcx>) {
    }
 }

-#[derive(Default)]
-struct MutablyUsedVariablesCtxt {
+struct MutablyUsedVariablesCtxt<'tcx> {
    mutably_used_vars: HirIdSet,
    prev_bind: Option<HirId>,
    prev_move_to_closure: HirIdSet,
    aliases: HirIdMap<HirId>,
    async_closures: FxHashSet<LocalDefId>,
+    tcx: TyCtxt<'tcx>,
 }

-impl MutablyUsedVariablesCtxt {
+impl<'tcx> MutablyUsedVariablesCtxt<'tcx> {
    fn add_mutably_used_var(&mut self, mut used_id: HirId) {
        while let Some(id) = self.aliases.get(&used_id) {
            self.mutably_used_vars.insert(used_id);
@ -242,9 +258,27 @@ fn add_mutably_used_var(&mut self, mut used_id: HirId) {
        }
        self.mutably_used_vars.insert(used_id);
    }
+
+    fn would_be_alias_cycle(&self, alias: HirId, mut target: HirId) -> bool {
+        while let Some(id) = self.aliases.get(&target) {
+            if *id == alias {
+                return true;
+            }
+            target = *id;
+        }
+        false
    }

-impl<'tcx> euv::Delegate<'tcx> for MutablyUsedVariablesCtxt {
+    fn add_alias(&mut self, alias: HirId, target: HirId) {
+        // This is to prevent alias loop.
+        if alias == target || self.would_be_alias_cycle(alias, target) {
+            return;
+        }
+        self.aliases.insert(alias, target);
+    }
+}
+
+impl<'tcx> euv::Delegate<'tcx> for MutablyUsedVariablesCtxt<'tcx> {
    fn consume(&mut self, cmt: &euv::PlaceWithHirId<'tcx>, _id: HirId) {
        if let euv::Place {
            base:
@ -259,7 +293,7 @@ fn consume(&mut self, cmt: &euv::PlaceWithHirId<'tcx>, _id: HirId) {
        {
            if let Some(bind_id) = self.prev_bind.take() {
                if bind_id != *vid {
-                    self.aliases.insert(bind_id, *vid);
+                    self.add_alias(bind_id, *vid);
                }
            } else if !self.prev_move_to_closure.contains(vid)
                && matches!(base_ty.ref_mutability(), Some(Mutability::Mut))
@ -289,6 +323,25 @@ fn borrow(&mut self, cmt: &euv::PlaceWithHirId<'tcx>, _id: HirId, borrow: ty::Bo
            {
                self.add_mutably_used_var(*vid);
            }
+        } else if borrow == ty::ImmBorrow {
+            // If there is an `async block`, it'll contain a call to a closure which we need to
+            // go into to ensure all "mutate" checks are found.
+            if let Node::Expr(Expr {
+                kind:
+                    ExprKind::Call(
+                        _,
+                        [
+                            Expr {
+                                kind: ExprKind::Closure(Closure { def_id, .. }),
+                                ..
+                            },
+                        ],
+                    ),
+                ..
+            }) = self.tcx.hir().get(cmt.hir_id)
+            {
+                self.async_closures.insert(*def_id);
+            }
        }
    }

@ -296,7 +349,12 @@ fn mutate(&mut self, cmt: &euv::PlaceWithHirId<'tcx>, _id: HirId) {
        self.prev_bind = None;
        if let euv::Place {
            projections,
-            base: euv::PlaceBase::Local(vid),
+            base:
+                euv::PlaceBase::Local(vid)
+                | euv::PlaceBase::Upvar(UpvarId {
+                    var_path: UpvarPath { hir_id: vid },
+                    ..
+                }),
            ..
        } = &cmt.place
        {
@ -329,8 +387,9 @@ fn fake_read(
                // Seems like we are inside an async function. We need to store the closure `DefId`
                // to go through it afterwards.
                self.async_closures.insert(inner);
-                self.aliases.insert(cmt.hir_id, *vid);
+                self.add_alias(cmt.hir_id, *vid);
                self.prev_move_to_closure.insert(*vid);
+                self.prev_bind = None;
            }
        }
    }
--- a/src/tools/clippy/clippy_lints/src/useless_conversion.rs
+++ b/src/tools/clippy/clippy_lints/src/useless_conversion.rs
@ -5,6 +5,7 @@
 use clippy_utils::{get_parent_expr, is_trait_method, is_ty_alias, match_def_path, path_to_local, paths};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
+use rustc_hir::def::DefKind;
 use rustc_hir::def_id::DefId;
 use rustc_hir::{BindingAnnotation, Expr, ExprKind, HirId, MatchSource, Node, PatKind};
 use rustc_lint::{LateContext, LateLintPass};
@ -39,6 +40,7 @@
 #[derive(Default)]
 pub struct UselessConversion {
    try_desugar_arm: Vec<HirId>,
+    expn_depth: u32,
 }

 impl_lint_pass!(UselessConversion => [USELESS_CONVERSION]);
@ -105,6 +107,7 @@ fn into_iter_deep_call<'hir>(cx: &LateContext<'_>, mut expr: &'hir Expr<'hir>) -
 impl<'tcx> LateLintPass<'tcx> for UselessConversion {
    fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) {
        if e.span.from_expansion() {
+            self.expn_depth += 1;
            return;
        }

@ -150,9 +153,14 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) {
                {
                    if let Some(parent) = get_parent_expr(cx, e) {
                        let parent_fn = match parent.kind {
-                            ExprKind::Call(recv, args) if let ExprKind::Path(ref qpath) = recv.kind => {
-                                cx.qpath_res(qpath, recv.hir_id).opt_def_id()
-                                    .map(|did| (did, args, MethodOrFunction::Function))
+                            ExprKind::Call(recv, args)
+                                if let ExprKind::Path(ref qpath) = recv.kind
+                                    && let Some(did) = cx.qpath_res(qpath, recv.hir_id).opt_def_id()
+                                    // make sure that the path indeed points to a fn-like item, so that
+                                    // `fn_sig` does not ICE. (see #11065)
+                                    && cx.tcx.opt_def_kind(did).is_some_and(DefKind::is_fn_like) =>
+                            {
+                                    Some((did, args, MethodOrFunction::Function))
                            }
                            ExprKind::MethodCall(.., args, _) => {
                                cx.typeck_results().type_dependent_def_id(parent.hir_id)
@ -168,6 +176,7 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) {
                            && let Some(&into_iter_param) = sig.inputs().get(kind.param_pos(arg_pos))
                            && let ty::Param(param) = into_iter_param.kind()
                            && let Some(span) = into_iter_bound(cx, parent_fn_did, into_iter_did, param.index)
+                            && self.expn_depth == 0
                        {
                            // Get the "innermost" `.into_iter()` call, e.g. given this expression:
                            // `foo.into_iter().into_iter()`
@ -303,5 +312,8 @@ fn check_expr_post(&mut self, _: &LateContext<'tcx>, e: &'tcx Expr<'_>) {
        if Some(&e.hir_id) == self.try_desugar_arm.last() {
            self.try_desugar_arm.pop();
        }
+        if e.span.from_expansion() {
+            self.expn_depth -= 1;
+        }
    }
 }
--- a/src/tools/clippy/tests/ui/crashes/ice-11065.rs
+++ b/src/tools/clippy/tests/ui/crashes/ice-11065.rs
@ -0,0 +1,19 @@
+#![warn(clippy::useless_conversion)]
+
+use std::iter::FromIterator;
+use std::option::IntoIter as OptionIter;
+
+fn eq<T: Eq>(a: T, b: T) -> bool {
+    a == b
+}
+
+macro_rules! tests {
+    ($($expr:expr, $ty:ty, ($($test:expr),*);)+) => (pub fn main() {$({
+        const C: $ty = $expr;
+        assert!(eq(C($($test),*), $expr($($test),*)));
+    })+})
+}
+
+tests! {
+    FromIterator::from_iter, fn(OptionIter<i32>) -> Vec<i32>, (Some(5).into_iter());
+}
--- a/src/tools/clippy/tests/ui/needless_pass_by_ref_mut.rs
+++ b/src/tools/clippy/tests/ui/needless_pass_by_ref_mut.rs
@ -196,6 +196,41 @@ fn cfg_warn(s: &mut u32) {}
    //~| NOTE: this is cfg-gated and may require further changes
 }

+// Should not warn.
+async fn inner_async(x: &mut i32, y: &mut u32) {
+    async {
+        *y += 1;
+        *x += 1;
+    }
+    .await;
+}
+
+async fn inner_async2(x: &mut i32, y: &mut u32) {
+    //~^ ERROR: this argument is a mutable reference, but not used mutably
+    async {
+        *x += 1;
+    }
+    .await;
+}
+
+async fn inner_async3(x: &mut i32, y: &mut u32) {
+    //~^ ERROR: this argument is a mutable reference, but not used mutably
+    async {
+        *y += 1;
+    }
+    .await;
+}
+
+// Should not warn.
+async fn async_vec(b: &mut Vec<bool>) {
+    b.append(&mut vec![]);
+}
+
+// Should not warn.
+async fn async_vec2(b: &mut Vec<bool>) {
+    b.push(true);
+}
+
 fn main() {
    let mut u = 0;
    let mut v = vec![0];
--- a/src/tools/clippy/tests/ui/needless_pass_by_ref_mut.stderr
+++ b/src/tools/clippy/tests/ui/needless_pass_by_ref_mut.stderr
@ -94,5 +94,17 @@ LL |     fn cfg_warn(s: &mut u32) {}
   |
   = note: this is cfg-gated and may require further changes

-error: aborting due to 15 previous errors
+error: this argument is a mutable reference, but not used mutably
+  --> $DIR/needless_pass_by_ref_mut.rs:208:39
+   |
+LL | async fn inner_async2(x: &mut i32, y: &mut u32) {
+   |                                       ^^^^^^^^ help: consider changing to: `&u32`
+
+error: this argument is a mutable reference, but not used mutably
+  --> $DIR/needless_pass_by_ref_mut.rs:216:26
+   |
+LL | async fn inner_async3(x: &mut i32, y: &mut u32) {
+   |                          ^^^^^^^^ help: consider changing to: `&i32`
+
+error: aborting due to 17 previous errors

--- a/src/tools/clippy/tests/ui/useless_conversion.fixed
+++ b/src/tools/clippy/tests/ui/useless_conversion.fixed
@ -155,6 +155,20 @@ fn main() {
    let _ = vec![s4, s4, s4].into_iter();
 }

+#[allow(dead_code)]
+fn issue11065_fp() {
+    use std::option::IntoIter;
+    fn takes_into_iter(_: impl IntoIterator<Item = i32>) {}
+
+    macro_rules! x {
+        ($e:expr) => {
+            takes_into_iter($e);
+            let _: IntoIter<i32> = $e; // removing `.into_iter()` leads to a type error here
+        };
+    }
+    x!(Some(5).into_iter());
+}
+
 #[allow(dead_code)]
 fn explicit_into_iter_fn_arg() {
    fn a<T>(_: T) {}
--- a/src/tools/clippy/tests/ui/useless_conversion.rs
+++ b/src/tools/clippy/tests/ui/useless_conversion.rs
@ -155,6 +155,20 @@ fn main() {
    let _ = vec![s4, s4, s4].into_iter().into_iter();
 }

+#[allow(dead_code)]
+fn issue11065_fp() {
+    use std::option::IntoIter;
+    fn takes_into_iter(_: impl IntoIterator<Item = i32>) {}
+
+    macro_rules! x {
+        ($e:expr) => {
+            takes_into_iter($e);
+            let _: IntoIter<i32> = $e; // removing `.into_iter()` leads to a type error here
+        };
+    }
+    x!(Some(5).into_iter());
+}
+
 #[allow(dead_code)]
 fn explicit_into_iter_fn_arg() {
    fn a<T>(_: T) {}
--- a/src/tools/clippy/tests/ui/useless_conversion.stderr
+++ b/src/tools/clippy/tests/ui/useless_conversion.stderr
@ -119,61 +119,61 @@ LL |     let _ = vec![s4, s4, s4].into_iter().into_iter();
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into_iter()`: `vec![s4, s4, s4].into_iter()`

 error: explicit call to `.into_iter()` in function argument accepting `IntoIterator`
-  --> $DIR/useless_conversion.rs:171:7
+  --> $DIR/useless_conversion.rs:185:7
   |
 LL |     b(vec![1, 2].into_iter());
   |       ^^^^^^^^^^^^^^^^^^^^^^ help: consider removing the `.into_iter()`: `vec![1, 2]`
   |
 note: this parameter accepts any `IntoIterator`, so you don't need to call `.into_iter()`
-  --> $DIR/useless_conversion.rs:161:13
+  --> $DIR/useless_conversion.rs:175:13
   |
 LL |     fn b<T: IntoIterator<Item = i32>>(_: T) {}
   |             ^^^^^^^^^^^^^^^^^^^^^^^^

 error: explicit call to `.into_iter()` in function argument accepting `IntoIterator`
-  --> $DIR/useless_conversion.rs:172:7
+  --> $DIR/useless_conversion.rs:186:7
   |
 LL |     c(vec![1, 2].into_iter());
   |       ^^^^^^^^^^^^^^^^^^^^^^ help: consider removing the `.into_iter()`: `vec![1, 2]`
   |
 note: this parameter accepts any `IntoIterator`, so you don't need to call `.into_iter()`
-  --> $DIR/useless_conversion.rs:162:18
+  --> $DIR/useless_conversion.rs:176:18
   |
 LL |     fn c(_: impl IntoIterator<Item = i32>) {}
   |                  ^^^^^^^^^^^^^^^^^^^^^^^^

 error: explicit call to `.into_iter()` in function argument accepting `IntoIterator`
-  --> $DIR/useless_conversion.rs:173:7
+  --> $DIR/useless_conversion.rs:187:7
   |
 LL |     d(vec![1, 2].into_iter());
   |       ^^^^^^^^^^^^^^^^^^^^^^ help: consider removing the `.into_iter()`: `vec![1, 2]`
   |
 note: this parameter accepts any `IntoIterator`, so you don't need to call `.into_iter()`
-  --> $DIR/useless_conversion.rs:165:12
+  --> $DIR/useless_conversion.rs:179:12
   |
 LL |         T: IntoIterator<Item = i32>,
   |            ^^^^^^^^^^^^^^^^^^^^^^^^

 error: explicit call to `.into_iter()` in function argument accepting `IntoIterator`
-  --> $DIR/useless_conversion.rs:176:7
+  --> $DIR/useless_conversion.rs:190:7
   |
 LL |     b(vec![1, 2].into_iter().into_iter());
   |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing the `.into_iter()`s: `vec![1, 2]`
   |
 note: this parameter accepts any `IntoIterator`, so you don't need to call `.into_iter()`
-  --> $DIR/useless_conversion.rs:161:13
+  --> $DIR/useless_conversion.rs:175:13
   |
 LL |     fn b<T: IntoIterator<Item = i32>>(_: T) {}
   |             ^^^^^^^^^^^^^^^^^^^^^^^^

 error: explicit call to `.into_iter()` in function argument accepting `IntoIterator`
-  --> $DIR/useless_conversion.rs:177:7
+  --> $DIR/useless_conversion.rs:191:7
   |
 LL |     b(vec![1, 2].into_iter().into_iter().into_iter());
   |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing the `.into_iter()`s: `vec![1, 2]`
   |
 note: this parameter accepts any `IntoIterator`, so you don't need to call `.into_iter()`
-  --> $DIR/useless_conversion.rs:161:13
+  --> $DIR/useless_conversion.rs:175:13
   |
 LL |     fn b<T: IntoIterator<Item = i32>>(_: T) {}
   |             ^^^^^^^^^^^^^^^^^^^^^^^^