Auto merge of rust-lang#8271 - Jarcho:ptr_arg_214, r=flip1995

Check usages in `ptr_arg`

fixes rust-lang#214
fixes rust-lang#1981
fixes rust-lang#3381
fixes rust-lang#6406
fixes rust-lang#6964

This does not take into account the return type of the function currently, so `(&Vec<_>) -> &Vec<_>` functions may still be false positives.

The name given for the type also has to match the real type name, so `type Foo = Vec<u32>` won't trigger the lint, but `type Vec = Vec<u32>` will. I'm not sure if this is the best way to handle this, or if a note about the actual type should be added instead.

changelog: Check if the argument is used in a way which requires the original type in `ptr_arg`
changelog: Lint mutable references in `ptr_arg`

clippy_lints/src/unnested_or_patterns.rs

@@ -291,7 +291,7 @@ fn transform_with_focus_on_idx(alternatives: &mut Vec<P<Pat>>, focus_idx: usize)
 fn extend_with_struct_pat(
     qself1: &Option<ast::QSelf>,
     path1: &ast::Path,
-    fps1: &mut Vec<ast::PatField>,
+    fps1: &mut [ast::PatField],
     rest1: bool,
     start: usize,
     alternatives: &mut Vec<P<Pat>>,
@@ -332,7 +332,7 @@ fn extend_with_struct_pat(
 /// while also requiring `ps1[..n] ~ ps2[..n]` (pre) and `ps1[n + 1..] ~ ps2[n + 1..]` (post),
 /// where `~` denotes semantic equality.
 fn extend_with_matching_product(
-    targets: &mut Vec<P<Pat>>,
+    targets: &mut [P<Pat>],
     start: usize,
     alternatives: &mut Vec<P<Pat>>,
     predicate: impl Fn(&PatKind, &[P<Pat>], usize) -> bool,

clippy_utils/src/lib.rs

@@ -1850,7 +1850,8 @@ pub fn is_expr_identity_function(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool
 }
 
 /// Gets the node where an expression is either used, or it's type is unified with another branch.
-pub fn get_expr_use_or_unification_node<'tcx>(tcx: TyCtxt<'tcx>, expr: &Expr<'_>) -> Option<Node<'tcx>> {
+/// Returns both the node and the `HirId` of the closest child node.
+pub fn get_expr_use_or_unification_node<'tcx>(tcx: TyCtxt<'tcx>, expr: &Expr<'_>) -> Option<(Node<'tcx>, HirId)> {
     let mut child_id = expr.hir_id;
     let mut iter = tcx.hir().parent_iter(child_id);
     loop {
@@ -1862,9 +1863,9 @@ pub fn get_expr_use_or_unification_node<'tcx>(tcx: TyCtxt<'tcx>, expr: &Expr<'_>
                 ExprKind::Match(_, [arm], _) if arm.hir_id == child_id => child_id = expr.hir_id,
                 ExprKind::Block(..) | ExprKind::DropTemps(_) => child_id = expr.hir_id,
                 ExprKind::If(_, then_expr, None) if then_expr.hir_id == child_id => break None,
-                _ => break Some(Node::Expr(expr)),
+                _ => break Some((Node::Expr(expr), child_id)),
             },
-            Some((_, node)) => break Some(node),
+            Some((_, node)) => break Some((node, child_id)),
         }
     }
 }
@@ -1873,18 +1874,21 @@ pub fn get_expr_use_or_unification_node<'tcx>(tcx: TyCtxt<'tcx>, expr: &Expr<'_>
 pub fn is_expr_used_or_unified(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> bool {
     !matches!(
         get_expr_use_or_unification_node(tcx, expr),
-        None | Some(Node::Stmt(Stmt {
-            kind: StmtKind::Expr(_)
-                | StmtKind::Semi(_)
-                | StmtKind::Local(Local {
-                    pat: Pat {
-                        kind: PatKind::Wild,
+        None | Some((
+            Node::Stmt(Stmt {
+                kind: StmtKind::Expr(_)
+                    | StmtKind::Semi(_)
+                    | StmtKind::Local(Local {
+                        pat: Pat {
+                            kind: PatKind::Wild,
+                            ..
+                        },
                         ..
-                    },
-                    ..
-                }),
-            ..
-        }))
+                    }),
+                ..
+            }),
+            _
+        ))
     )
 }
 

clippy_utils/src/ty.rs

@@ -5,19 +5,22 @@
 use rustc_ast::ast::Mutability;
 use rustc_data_structures::fx::FxHashMap;
 use rustc_hir as hir;
+use rustc_hir::def::{CtorKind, DefKind, Res};
 use rustc_hir::def_id::DefId;
-use rustc_hir::{TyKind, Unsafety};
+use rustc_hir::{Expr, TyKind, Unsafety};
 use rustc_infer::infer::TyCtxtInferExt;
 use rustc_lint::LateContext;
-use rustc_middle::ty::subst::{GenericArg, GenericArgKind};
-use rustc_middle::ty::{self, AdtDef, IntTy, Predicate, Ty, TyCtxt, TypeFoldable, UintTy};
+use rustc_middle::ty::subst::{GenericArg, GenericArgKind, Subst};
+use rustc_middle::ty::{
+    self, AdtDef, Binder, FnSig, IntTy, Predicate, PredicateKind, Ty, TyCtxt, TypeFoldable, UintTy,
+};
 use rustc_span::symbol::Ident;
 use rustc_span::{sym, Span, Symbol, DUMMY_SP};
 use rustc_trait_selection::infer::InferCtxtExt;
 use rustc_trait_selection::traits::query::normalize::AtExt;
 use std::iter;
 
-use crate::{match_def_path, must_use_attr};
+use crate::{expr_path_res, match_def_path, must_use_attr};
 
 // Checks if the given type implements copy.
 pub fn is_copy<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
@@ -410,3 +413,105 @@ pub fn all_predicates_of(tcx: TyCtxt<'_>, id: DefId) -> impl Iterator<Item = &(P
     })
     .flatten()
 }
+
+/// A signature for a function like type.
+#[derive(Clone, Copy)]
+pub enum ExprFnSig<'tcx> {
+    Sig(Binder<'tcx, FnSig<'tcx>>),
+    Closure(Binder<'tcx, FnSig<'tcx>>),
+    Trait(Binder<'tcx, Ty<'tcx>>, Option<Binder<'tcx, Ty<'tcx>>>),
+}
+impl<'tcx> ExprFnSig<'tcx> {
+    /// Gets the argument type at the given offset.
+    pub fn input(self, i: usize) -> Binder<'tcx, Ty<'tcx>> {
+        match self {
+            Self::Sig(sig) => sig.input(i),
+            Self::Closure(sig) => sig.input(0).map_bound(|ty| ty.tuple_element_ty(i).unwrap()),
+            Self::Trait(inputs, _) => inputs.map_bound(|ty| ty.tuple_element_ty(i).unwrap()),
+        }
+    }
+
+    /// Gets the result type, if one could be found. Note that the result type of a trait may not be
+    /// specified.
+    pub fn output(self) -> Option<Binder<'tcx, Ty<'tcx>>> {
+        match self {
+            Self::Sig(sig) | Self::Closure(sig) => Some(sig.output()),
+            Self::Trait(_, output) => output,
+        }
+    }
+}
+
+/// If the expression is function like, get the signature for it.
+pub fn expr_sig<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>) -> Option<ExprFnSig<'tcx>> {
+    if let Res::Def(DefKind::Fn | DefKind::Ctor(_, CtorKind::Fn) | DefKind::AssocFn, id) = expr_path_res(cx, expr) {
+        Some(ExprFnSig::Sig(cx.tcx.fn_sig(id)))
+    } else {
+        let ty = cx.typeck_results().expr_ty_adjusted(expr).peel_refs();
+        match *ty.kind() {
+            ty::Closure(_, subs) => Some(ExprFnSig::Closure(subs.as_closure().sig())),
+            ty::FnDef(id, subs) => Some(ExprFnSig::Sig(cx.tcx.fn_sig(id).subst(cx.tcx, subs))),
+            ty::FnPtr(sig) => Some(ExprFnSig::Sig(sig)),
+            ty::Dynamic(bounds, _) => {
+                let lang_items = cx.tcx.lang_items();
+                match bounds.principal() {
+                    Some(bound)
+                        if Some(bound.def_id()) == lang_items.fn_trait()
+                            || Some(bound.def_id()) == lang_items.fn_once_trait()
+                            || Some(bound.def_id()) == lang_items.fn_mut_trait() =>
+                    {
+                        let output = bounds
+                            .projection_bounds()
+                            .find(|p| lang_items.fn_once_output().map_or(false, |id| id == p.item_def_id()))
+                            .map(|p| p.map_bound(|p| p.ty));
+                        Some(ExprFnSig::Trait(bound.map_bound(|b| b.substs.type_at(0)), output))
+                    },
+                    _ => None,
+                }
+            },
+            ty::Param(_) | ty::Projection(..) => {
+                let mut inputs = None;
+                let mut output = None;
+                let lang_items = cx.tcx.lang_items();
+
+                for (pred, _) in all_predicates_of(cx.tcx, cx.typeck_results().hir_owner.to_def_id()) {
+                    let mut is_input = false;
+                    if let Some(ty) = pred
+                        .kind()
+                        .map_bound(|pred| match pred {
+                            PredicateKind::Trait(p)
+                                if (lang_items.fn_trait() == Some(p.def_id())
+                                    || lang_items.fn_mut_trait() == Some(p.def_id())
+                                    || lang_items.fn_once_trait() == Some(p.def_id()))
+                                    && p.self_ty() == ty =>
+                            {
+                                is_input = true;
+                                Some(p.trait_ref.substs.type_at(1))
+                            },
+                            PredicateKind::Projection(p)
+                                if Some(p.projection_ty.item_def_id) == lang_items.fn_once_output()
+                                    && p.projection_ty.self_ty() == ty =>
+                            {
+                                is_input = false;
+                                Some(p.ty)
+                            },
+                            _ => None,
+                        })
+                        .transpose()
+                    {
+                        if is_input && inputs.is_none() {
+                            inputs = Some(ty);
+                        } else if !is_input && output.is_none() {
+                            output = Some(ty);
+                        } else {
+                            // Multiple different fn trait impls. Is this even allowed?
+                            return None;
+                        }
+                    }
+                }
+
+                inputs.map(|ty| ExprFnSig::Trait(ty, output))
+            },
+            _ => None,
+        }
+    }
+}

tests/ui/ptr_arg.rs

@@ -9,7 +9,6 @@ fn do_vec(x: &Vec<i64>) {
 }
 
 fn do_vec_mut(x: &mut Vec<i64>) {
-    // no error here
     //Nothing here
 }
 
@@ -18,7 +17,6 @@ fn do_str(x: &String) {
 }
 
 fn do_str_mut(x: &mut String) {
-    // no error here
     //Nothing here either
 }
 
@@ -27,7 +25,6 @@ fn do_path(x: &PathBuf) {
 }
 
 fn do_path_mut(x: &mut PathBuf) {
-    // no error here
     //Nothing here either
 }
 
@@ -52,7 +49,7 @@ fn cloned(x: &Vec<u8>) -> Vec<u8> {
     let e = x.clone();
     let f = e.clone(); // OK
     let g = x;
-    let h = g.clone(); // Alas, we cannot reliably detect this without following data.
+    let h = g.clone();
     let i = (e).clone();
     x.clone()
 }
@@ -156,6 +153,30 @@ mod issue6509 {
     }
 }
 
+fn mut_vec_slice_methods(v: &mut Vec<u32>) {
+    v.copy_within(1..5, 10);
+}
+
+fn mut_vec_vec_methods(v: &mut Vec<u32>) {
+    v.clear();
+}
+
+fn vec_contains(v: &Vec<u32>) -> bool {
+    [vec![], vec![0]].as_slice().contains(v)
+}
+
+fn fn_requires_vec(v: &Vec<u32>) -> bool {
+    vec_contains(v)
+}
+
+fn impl_fn_requires_vec(v: &Vec<u32>, f: impl Fn(&Vec<u32>)) {
+    f(v);
+}
+
+fn dyn_fn_requires_vec(v: &Vec<u32>, f: &dyn Fn(&Vec<u32>)) {
+    f(v);
+}
+
 // No error for types behind an alias (#7699)
 type A = Vec<u8>;
 fn aliased(a: &A) {}