From 7ed86bf82210042a894620e1537e86a7fe2ce5b6 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Sun, 9 Jan 2022 09:35:55 -0500 Subject: [PATCH 1/2] Changes to `ptr_arg` * Track the argument when used to initialize simple `let` bindings * Check if the argument is passed to a function requiring the original type * Use `multipart_suggestion` rather than multiple suggestions * Check if the name given in the source code matches the name of the actual type --- clippy_lints/src/ptr.rs | 605 +++++++++++++------- clippy_lints/src/unnested_or_patterns.rs | 4 +- clippy_utils/src/lib.rs | 32 +- clippy_utils/src/ty.rs | 113 +++- tests/ui/ptr_arg.rs | 29 +- tests/ui/ptr_arg.stderr | 158 +++-- tests/ui/search_is_some_fixable_none.rs | 2 +- tests/ui/search_is_some_fixable_none.stderr | 10 +- tests/ui/search_is_some_fixable_some.rs | 2 +- tests/ui/search_is_some_fixable_some.stderr | 10 +- tests/ui/slow_vector_initialization.rs | 2 +- 11 files changed, 643 insertions(+), 324 deletions(-) diff --git a/clippy_lints/src/ptr.rs b/clippy_lints/src/ptr.rs index c08a19d520b60..b74ba40d452f8 100644 --- a/clippy_lints/src/ptr.rs +++ b/clippy_lints/src/ptr.rs @@ -1,30 +1,36 @@ //! Checks for usage of `&Vec[_]` and `&String`. use clippy_utils::diagnostics::{span_lint, span_lint_and_sugg, span_lint_and_then}; -use clippy_utils::ptr::get_spans; use clippy_utils::source::snippet_opt; -use clippy_utils::ty::walk_ptrs_hir_ty; -use clippy_utils::{expr_path_res, is_lint_allowed, match_any_diagnostic_items, paths}; +use clippy_utils::ty::expr_sig; +use clippy_utils::{ + expr_path_res, get_expr_use_or_unification_node, is_lint_allowed, match_any_diagnostic_items, path_to_local, paths, +}; use if_chain::if_chain; use rustc_errors::Applicability; -use rustc_hir::def::Res; +use rustc_hir::def_id::DefId; +use rustc_hir::hir_id::HirIdMap; +use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor}; use rustc_hir::{ - BinOpKind, BodyId, Expr, ExprKind, FnDecl, FnRetTy, GenericArg, Impl, ImplItem, ImplItemKind, Item, ItemKind, - Lifetime, MutTy, Mutability, Node, PathSegment, QPath, TraitFn, TraitItem, TraitItemKind, Ty, TyKind, + self as hir, AnonConst, BinOpKind, BindingAnnotation, Body, Expr, ExprKind, FnDecl, FnRetTy, GenericArg, + ImplItemKind, ItemKind, Lifetime, LifetimeName, Mutability, Node, Param, ParamName, PatKind, QPath, TraitFn, + TraitItem, TraitItemKind, TyKind, }; use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::hir::map::Map; +use rustc_middle::ty::{self, AssocItems, AssocKind, Ty}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::source_map::Span; use rustc_span::symbol::Symbol; use rustc_span::{sym, MultiSpan}; -use std::borrow::Cow; +use std::fmt; +use std::iter; declare_clippy_lint! { /// ### What it does - /// This lint checks for function arguments of type `&String` - /// or `&Vec` unless the references are mutable. It will also suggest you - /// replace `.clone()` calls with the appropriate `.to_owned()`/`to_string()` - /// calls. + /// This lint checks for function arguments of type `&String`, `&Vec`, + /// `&PathBuf`, and `Cow<_>`. It will also suggest you replace `.clone()` calls + /// with the appropriate `.to_owned()`/`to_string()` calls. /// /// ### Why is this bad? /// Requiring the argument to be of the specific size @@ -32,28 +38,7 @@ declare_clippy_lint! { /// or `&str` usually suffice and can be obtained from other types, too. /// /// ### Known problems - /// The lint does not follow data. So if you have an - /// argument `x` and write `let y = x; y.clone()` the lint will not suggest - /// changing that `.clone()` to `.to_owned()`. - /// - /// Other functions called from this function taking a `&String` or `&Vec` - /// argument may also fail to compile if you change the argument. Applying - /// this lint on them will fix the problem, but they may be in other crates. - /// - /// One notable example of a function that may cause issues, and which cannot - /// easily be changed due to being in the standard library is `Vec::contains`. - /// when called on a `Vec>`. If a `&Vec` is passed to that method then - /// it will compile, but if a `&[T]` is passed then it will not compile. - /// - /// ```ignore - /// fn cannot_take_a_slice(v: &Vec) -> bool { - /// let vec_of_vecs: Vec> = some_other_fn(); - /// - /// vec_of_vecs.contains(v) - /// } - /// ``` - /// - /// Also there may be `fn(&Vec)`-typed references pointing to your function. + /// There may be `fn(&Vec)`-typed references pointing to your function. /// If you have them, you will get a compiler error after applying this lint's /// suggestions. You then have the choice to undo your changes or change the /// type of the reference. @@ -155,32 +140,86 @@ declare_clippy_lint! { declare_lint_pass!(Ptr => [PTR_ARG, CMP_NULL, MUT_FROM_REF, INVALID_NULL_PTR_USAGE]); impl<'tcx> LateLintPass<'tcx> for Ptr { - fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) { - if let ItemKind::Fn(ref sig, _, body_id) = item.kind { - check_fn(cx, sig.decl, Some(body_id)); - } - } + fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx TraitItem<'_>) { + if let TraitItemKind::Fn(sig, trait_method) = &item.kind { + if matches!(trait_method, TraitFn::Provided(_)) { + // Handled by check body. + return; + } - fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx ImplItem<'_>) { - if let ImplItemKind::Fn(ref sig, body_id) = item.kind { - let parent_item = cx.tcx.hir().get_parent_item(item.hir_id()); - if let Some(Node::Item(it)) = cx.tcx.hir().find(parent_item) { - if let ItemKind::Impl(Impl { of_trait: Some(_), .. }) = it.kind { - return; // ignore trait impls - } + check_mut_from_ref(cx, sig.decl); + for arg in check_fn_args( + cx, + cx.tcx.fn_sig(item.def_id).skip_binder().inputs(), + sig.decl.inputs, + &[], + ) { + span_lint_and_sugg( + cx, + PTR_ARG, + arg.span, + &arg.build_msg(), + "change this to", + format!("{}{}", arg.ref_prefix, arg.deref_ty.display(cx)), + Applicability::Unspecified, + ); } - check_fn(cx, sig.decl, Some(body_id)); } } - fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx TraitItem<'_>) { - if let TraitItemKind::Fn(ref sig, ref trait_method) = item.kind { - let body_id = if let TraitFn::Provided(b) = *trait_method { - Some(b) - } else { - None - }; - check_fn(cx, sig.decl, body_id); + fn check_body(&mut self, cx: &LateContext<'tcx>, body: &'tcx Body<'_>) { + let hir = cx.tcx.hir(); + let mut parents = hir.parent_iter(body.value.hir_id); + let (item_id, decl) = match parents.next() { + Some((_, Node::Item(i))) => { + if let ItemKind::Fn(sig, ..) = &i.kind { + (i.def_id, sig.decl) + } else { + return; + } + }, + Some((_, Node::ImplItem(i))) => { + if !matches!(parents.next(), + Some((_, Node::Item(i))) if matches!(&i.kind, ItemKind::Impl(i) if i.of_trait.is_none()) + ) { + return; + } + if let ImplItemKind::Fn(sig, _) = &i.kind { + (i.def_id, sig.decl) + } else { + return; + } + }, + Some((_, Node::TraitItem(i))) => { + if let TraitItemKind::Fn(sig, _) = &i.kind { + (i.def_id, sig.decl) + } else { + return; + } + }, + _ => return, + }; + + check_mut_from_ref(cx, decl); + let sig = cx.tcx.fn_sig(item_id).skip_binder(); + let lint_args: Vec<_> = check_fn_args(cx, sig.inputs(), decl.inputs, body.params).collect(); + let results = check_ptr_arg_usage(cx, body, &lint_args); + + for (result, args) in results.iter().zip(lint_args.iter()).filter(|(r, _)| !r.skip) { + span_lint_and_then(cx, PTR_ARG, args.span, &args.build_msg(), |diag| { + diag.multipart_suggestion( + "change this to", + iter::once((args.span, format!("{}{}", args.ref_prefix, args.deref_ty.display(cx)))) + .chain(result.replacements.iter().map(|r| { + ( + r.expr_span, + format!("{}{}", snippet_opt(cx, r.self_span).unwrap(), r.replacement), + ) + })) + .collect(), + Applicability::Unspecified, + ); + }); } } @@ -247,154 +286,202 @@ fn check_invalid_ptr_usage<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { } } -#[allow(clippy::too_many_lines)] -fn check_fn(cx: &LateContext<'_>, decl: &FnDecl<'_>, opt_body_id: Option) { - let body = opt_body_id.map(|id| cx.tcx.hir().body(id)); - - for (idx, arg) in decl.inputs.iter().enumerate() { - // Honor the allow attribute on parameters. See issue 5644. - if let Some(body) = &body { - if is_lint_allowed(cx, PTR_ARG, body.params[idx].hir_id) { - continue; - } - } +#[derive(Default)] +struct PtrArgResult { + skip: bool, + replacements: Vec, +} - let (item_name, path) = if_chain! { - if let TyKind::Rptr(_, MutTy { ty, mutbl: Mutability::Not }) = arg.kind; - if let TyKind::Path(QPath::Resolved(_, path)) = ty.kind; - if let Res::Def(_, did) = path.res; - if let Some(item_name) = cx.tcx.get_diagnostic_name(did); - then { - (item_name, path) - } else { - continue - } - }; +struct PtrArgReplacement { + expr_span: Span, + self_span: Span, + replacement: &'static str, +} - match item_name { - sym::Vec => { - if let Some(spans) = get_spans(cx, opt_body_id, idx, &[("clone", ".to_owned()")]) { - span_lint_and_then( - cx, - PTR_ARG, - arg.span, - "writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used \ - with non-Vec-based slices", - |diag| { - if let Some(ref snippet) = get_only_generic_arg_snippet(cx, arg) { - diag.span_suggestion( - arg.span, - "change this to", - format!("&[{}]", snippet), - Applicability::Unspecified, - ); - } - for (clonespan, suggestion) in spans { - diag.span_suggestion( - clonespan, - &snippet_opt(cx, clonespan).map_or("change the call to".into(), |x| { - Cow::Owned(format!("change `{}` to", x)) - }), - suggestion.into(), - Applicability::Unspecified, - ); - } - }, - ); - } - }, - sym::String => { - if let Some(spans) = get_spans(cx, opt_body_id, idx, &[("clone", ".to_string()"), ("as_str", "")]) { - span_lint_and_then( - cx, - PTR_ARG, - arg.span, - "writing `&String` instead of `&str` involves a new object where a slice will do", - |diag| { - diag.span_suggestion(arg.span, "change this to", "&str".into(), Applicability::Unspecified); - for (clonespan, suggestion) in spans { - diag.span_suggestion_short( - clonespan, - &snippet_opt(cx, clonespan).map_or("change the call to".into(), |x| { - Cow::Owned(format!("change `{}` to", x)) - }), - suggestion.into(), - Applicability::Unspecified, - ); - } - }, - ); - } +struct PtrArg<'tcx> { + idx: usize, + span: Span, + ty_did: DefId, + ty_name: Symbol, + method_renames: &'static [(&'static str, &'static str)], + ref_prefix: RefPrefix, + deref_ty: DerefTy<'tcx>, + deref_assoc_items: Option<(DefId, &'tcx AssocItems<'tcx>)>, +} +impl PtrArg<'_> { + fn build_msg(&self) -> String { + format!( + "writing `&{}{}` instead of `&{}{}` involves a new object where a slice will do", + self.ref_prefix.mutability.prefix_str(), + self.ty_name, + self.ref_prefix.mutability.prefix_str(), + self.deref_ty.argless_str(), + ) + } +} + +struct RefPrefix { + lt: LifetimeName, + mutability: Mutability, +} +impl fmt::Display for RefPrefix { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + use fmt::Write; + f.write_char('&')?; + match self.lt { + LifetimeName::Param(ParamName::Plain(name)) => { + name.fmt(f)?; + f.write_char(' ')?; }, - sym::PathBuf => { - if let Some(spans) = get_spans(cx, opt_body_id, idx, &[("clone", ".to_path_buf()"), ("as_path", "")]) { - span_lint_and_then( - cx, - PTR_ARG, - arg.span, - "writing `&PathBuf` instead of `&Path` involves a new object where a slice will do", - |diag| { - diag.span_suggestion( - arg.span, - "change this to", - "&Path".into(), - Applicability::Unspecified, - ); - for (clonespan, suggestion) in spans { - diag.span_suggestion_short( - clonespan, - &snippet_opt(cx, clonespan).map_or("change the call to".into(), |x| { - Cow::Owned(format!("change `{}` to", x)) - }), - suggestion.into(), - Applicability::Unspecified, - ); - } - }, - ); + LifetimeName::Underscore => f.write_str("'_ ")?, + LifetimeName::Static => f.write_str("'static ")?, + _ => (), + } + f.write_str(self.mutability.prefix_str()) + } +} + +struct DerefTyDisplay<'a, 'tcx>(&'a LateContext<'tcx>, &'a DerefTy<'tcx>); +impl fmt::Display for DerefTyDisplay<'_, '_> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + use std::fmt::Write; + match self.1 { + DerefTy::Str => f.write_str("str"), + DerefTy::Path => f.write_str("Path"), + DerefTy::Slice(hir_ty, ty) => { + f.write_char('[')?; + match hir_ty.and_then(|s| snippet_opt(self.0, s)) { + Some(s) => f.write_str(&s)?, + None => ty.fmt(f)?, } + f.write_char(']') }, - sym::Cow => { - if_chain! { - if let [ref bx] = *path.segments; - if let Some(params) = bx.args; - if !params.parenthesized; - if let Some(inner) = params.args.iter().find_map(|arg| match arg { - GenericArg::Type(ty) => Some(ty), - _ => None, - }); - let replacement = snippet_opt(cx, inner.span); - if let Some(r) = replacement; - then { + } + } +} + +enum DerefTy<'tcx> { + Str, + Path, + Slice(Option, Ty<'tcx>), +} +impl<'tcx> DerefTy<'tcx> { + fn argless_str(&self) -> &'static str { + match *self { + Self::Str => "str", + Self::Path => "Path", + Self::Slice(..) => "[_]", + } + } + + fn display<'a>(&'a self, cx: &'a LateContext<'tcx>) -> DerefTyDisplay<'a, 'tcx> { + DerefTyDisplay(cx, self) + } +} + +fn check_fn_args<'cx, 'tcx: 'cx>( + cx: &'cx LateContext<'tcx>, + tys: &'tcx [Ty<'_>], + hir_tys: &'tcx [hir::Ty<'_>], + params: &'tcx [Param<'_>], +) -> impl Iterator> + 'cx { + tys.iter().enumerate().filter_map(|(i, ty)| { + if_chain! { + if let ty::Ref(_, ty, mutability) = *ty.kind(); + if let ty::Adt(adt, substs) = *ty.kind(); + + let hir_ty = &hir_tys[i]; + if let TyKind::Rptr(lt, ref ty) = hir_ty.kind; + if let TyKind::Path(QPath::Resolved(None, path)) = ty.ty.kind; + + if let [.., name] = path.segments; + if cx.tcx.item_name(adt.did) == name.ident.name; + + if !is_lint_allowed(cx, PTR_ARG, hir_ty.hir_id); + if params.get(i).map_or(true, |p| !is_lint_allowed(cx, PTR_ARG, p.hir_id)); + + then { + let (method_renames, deref_ty, deref_impl_id) = match cx.tcx.get_diagnostic_name(adt.did) { + Some(sym::Vec) => ( + [("clone", ".to_owned()")].as_slice(), + DerefTy::Slice( + name.args + .and_then(|args| args.args.first()) + .and_then(|arg| if let GenericArg::Type(ty) = arg { + Some(ty.span) + } else { + None + }), + substs.type_at(0), + ), + cx.tcx.lang_items().slice_impl() + ), + Some(sym::String) => ( + [("clone", ".to_owned()"), ("as_str", "")].as_slice(), + DerefTy::Str, + cx.tcx.lang_items().str_impl() + ), + Some(sym::PathBuf) => ( + [("clone", ".to_path_buf()"), ("as_path", "")].as_slice(), + DerefTy::Path, + None, + ), + Some(sym::Cow) => { + let ty_name = name.args + .and_then(|args| { + args.args.iter().find_map(|a| match a { + GenericArg::Type(x) => Some(x), + _ => None, + }) + }) + .and_then(|arg| snippet_opt(cx, arg.span)) + .unwrap_or_else(|| substs.type_at(1).to_string()); span_lint_and_sugg( cx, PTR_ARG, - arg.span, + hir_ty.span, "using a reference to `Cow` is not recommended", "change this to", - "&".to_owned() + &r, + format!("&{}{}", mutability.prefix_str(), ty_name), Applicability::Unspecified, ); - } - } - }, - _ => {}, + return None; + }, + _ => return None, + }; + return Some(PtrArg { + idx: i, + span: hir_ty.span, + ty_did: adt.did, + ty_name: name.ident.name, + method_renames, + ref_prefix: RefPrefix { + lt: lt.name, + mutability, + }, + deref_ty, + deref_assoc_items: deref_impl_id.map(|id| (id, cx.tcx.associated_items(id))), + }); + } } - } + None + }) +} +fn check_mut_from_ref(cx: &LateContext<'_>, decl: &FnDecl<'_>) { if let FnRetTy::Return(ty) = decl.output { if let Some((out, Mutability::Mut, _)) = get_rptr_lm(ty) { let mut immutables = vec![]; - for (_, ref mutbl, ref argspan) in decl + for (_, mutbl, argspan) in decl .inputs .iter() .filter_map(get_rptr_lm) .filter(|&(lt, _, _)| lt.name == out.name) { - if *mutbl == Mutability::Mut { + if mutbl == Mutability::Mut { return; } - immutables.push(*argspan); + immutables.push(argspan); } if immutables.is_empty() { return; @@ -413,24 +500,158 @@ fn check_fn(cx: &LateContext<'_>, decl: &FnDecl<'_>, opt_body_id: Option } } -fn get_only_generic_arg_snippet(cx: &LateContext<'_>, arg: &Ty<'_>) -> Option { - if_chain! { - if let TyKind::Path(QPath::Resolved(_, path)) = walk_ptrs_hir_ty(arg).kind; - if let Some(&PathSegment{args: Some(parameters), ..}) = path.segments.last(); - let types: Vec<_> = parameters.args.iter().filter_map(|arg| match arg { - GenericArg::Type(ty) => Some(ty), - _ => None, - }).collect(); - if types.len() == 1; - then { - snippet_opt(cx, types[0].span) - } else { - None +#[allow(clippy::too_many_lines)] +fn check_ptr_arg_usage<'tcx>(cx: &LateContext<'tcx>, body: &'tcx Body<'_>, args: &[PtrArg<'tcx>]) -> Vec { + struct V<'cx, 'tcx> { + cx: &'cx LateContext<'tcx>, + /// Map from a local id to which argument it cam from (index into `Self::args` and + /// `Self::results`) + bindings: HirIdMap, + /// The arguments being checked. + args: &'cx [PtrArg<'tcx>], + /// The results for each argument (len should match args.len) + results: Vec, + /// The number of arguments which can't be linted. Used to return early. + skip_count: usize, + } + impl<'tcx> Visitor<'tcx> for V<'_, 'tcx> { + type Map = Map<'tcx>; + fn nested_visit_map(&mut self) -> NestedVisitorMap { + NestedVisitorMap::OnlyBodies(self.cx.tcx.hir()) + } + + fn visit_anon_const(&mut self, _: &'tcx AnonConst) {} + + fn visit_expr(&mut self, e: &'tcx Expr<'_>) { + if self.skip_count == self.args.len() { + return; + } + + // Check if this is local we care about + let args_idx = match path_to_local(e).and_then(|id| self.bindings.get(&id)) { + Some(&i) => i, + None => return walk_expr(self, e), + }; + let args = &self.args[args_idx]; + let result = &mut self.results[args_idx]; + + // Helper function to handle early returns. + let mut set_skip_flag = || { + if result.skip { + self.skip_count += 1; + } + result.skip = true; + }; + + match get_expr_use_or_unification_node(self.cx.tcx, e) { + Some((Node::Stmt(_), _)) => (), + Some((Node::Local(l), _)) => { + // Only trace simple bindings. e.g `let x = y;` + if let PatKind::Binding(BindingAnnotation::Unannotated, id, _, None) = l.pat.kind { + self.bindings.insert(id, args_idx); + } else { + set_skip_flag(); + } + }, + Some((Node::Expr(e), child_id)) => match e.kind { + ExprKind::Call(f, expr_args) => { + let i = expr_args.iter().position(|arg| arg.hir_id == child_id).unwrap_or(0); + if expr_sig(self.cx, f) + .map(|sig| sig.input(i).skip_binder().peel_refs()) + .map_or(true, |ty| match *ty.kind() { + ty::Param(_) => true, + ty::Adt(def, _) => def.did == args.ty_did, + _ => false, + }) + { + // Passed to a function taking the non-dereferenced type. + set_skip_flag(); + } + }, + ExprKind::MethodCall(name, _, expr_args @ [self_arg, ..], _) => { + let i = expr_args.iter().position(|arg| arg.hir_id == child_id).unwrap_or(0); + if i == 0 { + // Check if the method can be renamed. + let name = name.ident.as_str(); + if let Some((_, replacement)) = args.method_renames.iter().find(|&&(x, _)| x == name) { + result.replacements.push(PtrArgReplacement { + expr_span: e.span, + self_span: self_arg.span, + replacement, + }); + return; + } + } + + let id = if let Some(x) = self.cx.typeck_results().type_dependent_def_id(e.hir_id) { + x + } else { + set_skip_flag(); + return; + }; + + match *self.cx.tcx.fn_sig(id).skip_binder().inputs()[i].peel_refs().kind() { + ty::Param(_) => { + set_skip_flag(); + }, + // If the types match check for methods which exist on both types. e.g. `Vec::len` and + // `slice::len` + ty::Adt(def, _) + if def.did == args.ty_did + && (i != 0 + || self.cx.tcx.trait_of_item(id).is_some() + || !args.deref_assoc_items.map_or(false, |(id, items)| { + items + .find_by_name_and_kind(self.cx.tcx, name.ident, AssocKind::Fn, id) + .is_some() + })) => + { + set_skip_flag(); + }, + _ => (), + } + }, + // Indexing is fine for currently supported types. + ExprKind::Index(e, _) if e.hir_id == child_id => (), + _ => set_skip_flag(), + }, + _ => set_skip_flag(), + } } } + + let mut skip_count = 0; + let mut results = args.iter().map(|_| PtrArgResult::default()).collect::>(); + let mut v = V { + cx, + bindings: args + .iter() + .enumerate() + .filter_map(|(i, arg)| { + let param = &body.params[arg.idx]; + match param.pat.kind { + PatKind::Binding(BindingAnnotation::Unannotated, id, _, None) + if !is_lint_allowed(cx, PTR_ARG, param.hir_id) => + { + Some((id, i)) + }, + _ => { + skip_count += 1; + results[arg.idx].skip = true; + None + }, + } + }) + .collect(), + args, + results, + skip_count, + }; + v.visit_expr(&body.value); + v.results } -fn get_rptr_lm<'tcx>(ty: &'tcx Ty<'tcx>) -> Option<(&'tcx Lifetime, Mutability, Span)> { +fn get_rptr_lm<'tcx>(ty: &'tcx hir::Ty<'tcx>) -> Option<(&'tcx Lifetime, Mutability, Span)> { if let TyKind::Rptr(ref lt, ref m) = ty.kind { Some((lt, m.mutbl, ty.span)) } else { diff --git a/clippy_lints/src/unnested_or_patterns.rs b/clippy_lints/src/unnested_or_patterns.rs index 0bd151fed91cc..1d4fe9cfd3cf4 100644 --- a/clippy_lints/src/unnested_or_patterns.rs +++ b/clippy_lints/src/unnested_or_patterns.rs @@ -291,7 +291,7 @@ fn transform_with_focus_on_idx(alternatives: &mut Vec>, focus_idx: usize) fn extend_with_struct_pat( qself1: &Option, path1: &ast::Path, - fps1: &mut Vec, + fps1: &mut [ast::PatField], rest1: bool, start: usize, alternatives: &mut Vec>, @@ -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>, + targets: &mut [P], start: usize, alternatives: &mut Vec>, predicate: impl Fn(&PatKind, &[P], usize) -> bool, diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 44f634b651943..ea40752af774e 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -1843,7 +1843,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> { +/// 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 { @@ -1855,9 +1856,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)), } } } @@ -1866,18 +1867,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, + .. + }, .. - }, - .. - }), - .. - })) + }), + .. + }), + _ + )) ) } diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs index 72317447159a9..dc24bc8e25239 100644 --- a/clippy_utils/src/ty.rs +++ b/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 { + Sig(Binder<'tcx, FnSig<'tcx>>), + Closure(Binder<'tcx, FnSig<'tcx>>), + Trait(Binder<'tcx, Ty<'tcx>>, Option>>), +} +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>> { + 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> { + 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, + } + } +} diff --git a/tests/ui/ptr_arg.rs b/tests/ui/ptr_arg.rs index 67bfef06a05e8..6cd7e96d7c1bd 100644 --- a/tests/ui/ptr_arg.rs +++ b/tests/ui/ptr_arg.rs @@ -9,7 +9,6 @@ fn do_vec(x: &Vec) { } fn do_vec_mut(x: &mut Vec) { - // 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) -> Vec { 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) { + v.copy_within(1..5, 10); +} + +fn mut_vec_vec_methods(v: &mut Vec) { + v.clear(); +} + +fn vec_contains(v: &Vec) -> bool { + [vec![], vec![0]].as_slice().contains(v) +} + +fn fn_requires_vec(v: &Vec) -> bool { + vec_contains(v) +} + +// fn impl_fn_requires_vec(v: &Vec, f: impl Fn(&Vec)) { +// f(v); +// } + +// fn dyn_fn_requires_vec(v: &Vec, f: &dyn Fn(&Vec)) { +// f(v); +// } + // No error for types behind an alias (#7699) type A = Vec; fn aliased(a: &A) {} diff --git a/tests/ui/ptr_arg.stderr b/tests/ui/ptr_arg.stderr index 64594eb870c2c..a9613daadde10 100644 --- a/tests/ui/ptr_arg.stderr +++ b/tests/ui/ptr_arg.stderr @@ -1,4 +1,4 @@ -error: writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used with non-Vec-based slices +error: writing `&Vec` instead of `&[_]` involves a new object where a slice will do --> $DIR/ptr_arg.rs:7:14 | LL | fn do_vec(x: &Vec) { @@ -6,170 +6,154 @@ LL | fn do_vec(x: &Vec) { | = note: `-D clippy::ptr-arg` implied by `-D warnings` +error: writing `&mut Vec` instead of `&mut [_]` involves a new object where a slice will do + --> $DIR/ptr_arg.rs:11:18 + | +LL | fn do_vec_mut(x: &mut Vec) { + | ^^^^^^^^^^^^^ help: change this to: `&mut [i64]` + error: writing `&String` instead of `&str` involves a new object where a slice will do - --> $DIR/ptr_arg.rs:16:14 + --> $DIR/ptr_arg.rs:15:14 | LL | fn do_str(x: &String) { | ^^^^^^^ help: change this to: `&str` +error: writing `&mut String` instead of `&mut str` involves a new object where a slice will do + --> $DIR/ptr_arg.rs:19:18 + | +LL | fn do_str_mut(x: &mut String) { + | ^^^^^^^^^^^ help: change this to: `&mut str` + error: writing `&PathBuf` instead of `&Path` involves a new object where a slice will do - --> $DIR/ptr_arg.rs:25:15 + --> $DIR/ptr_arg.rs:23:15 | LL | fn do_path(x: &PathBuf) { | ^^^^^^^^ help: change this to: `&Path` -error: writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used with non-Vec-based slices - --> $DIR/ptr_arg.rs:38:18 +error: writing `&mut PathBuf` instead of `&mut Path` involves a new object where a slice will do + --> $DIR/ptr_arg.rs:27:19 + | +LL | fn do_path_mut(x: &mut PathBuf) { + | ^^^^^^^^^^^^ help: change this to: `&mut Path` + +error: writing `&Vec` instead of `&[_]` involves a new object where a slice will do + --> $DIR/ptr_arg.rs:35:18 | LL | fn do_vec(x: &Vec); | ^^^^^^^^^ help: change this to: `&[i64]` -error: writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used with non-Vec-based slices - --> $DIR/ptr_arg.rs:51:14 +error: writing `&Vec` instead of `&[_]` involves a new object where a slice will do + --> $DIR/ptr_arg.rs:48:14 | LL | fn cloned(x: &Vec) -> Vec { | ^^^^^^^^ | help: change this to | -LL | fn cloned(x: &[u8]) -> Vec { - | ~~~~~ -help: change `x.clone()` to - | -LL | let e = x.to_owned(); - | ~~~~~~~~~~~~ -help: change `x.clone()` to - | -LL | x.to_owned() - | +LL ~ fn cloned(x: &[u8]) -> Vec { +LL ~ let e = x.to_owned(); +LL | let f = e.clone(); // OK +LL | let g = x; +LL ~ let h = g.to_owned(); +LL | let i = (e).clone(); + ... error: writing `&String` instead of `&str` involves a new object where a slice will do - --> $DIR/ptr_arg.rs:60:18 + --> $DIR/ptr_arg.rs:57:18 | LL | fn str_cloned(x: &String) -> String { | ^^^^^^^ | help: change this to | -LL | fn str_cloned(x: &str) -> String { - | ~~~~ -help: change `x.clone()` to - | -LL | let a = x.to_string(); - | ~~~~~~~~~~~~~ -help: change `x.clone()` to - | -LL | let b = x.to_string(); - | ~~~~~~~~~~~~~ -help: change `x.clone()` to - | -LL | x.to_string() +LL ~ fn str_cloned(x: &str) -> String { +LL ~ let a = x.to_owned(); +LL ~ let b = x.to_owned(); +LL | let c = b.clone(); +LL | let d = a.clone().clone().clone(); +LL ~ x.to_owned() | error: writing `&PathBuf` instead of `&Path` involves a new object where a slice will do - --> $DIR/ptr_arg.rs:68:19 + --> $DIR/ptr_arg.rs:65:19 | LL | fn path_cloned(x: &PathBuf) -> PathBuf { | ^^^^^^^^ | help: change this to | -LL | fn path_cloned(x: &Path) -> PathBuf { - | ~~~~~ -help: change `x.clone()` to - | -LL | let a = x.to_path_buf(); - | ~~~~~~~~~~~~~~~ -help: change `x.clone()` to - | -LL | let b = x.to_path_buf(); - | ~~~~~~~~~~~~~~~ -help: change `x.clone()` to - | -LL | x.to_path_buf() +LL ~ fn path_cloned(x: &Path) -> PathBuf { +LL ~ let a = x.to_path_buf(); +LL ~ let b = x.to_path_buf(); +LL | let c = b.clone(); +LL | let d = a.clone().clone().clone(); +LL ~ x.to_path_buf() | error: writing `&String` instead of `&str` involves a new object where a slice will do - --> $DIR/ptr_arg.rs:76:44 + --> $DIR/ptr_arg.rs:73:44 | LL | fn false_positive_capacity(x: &Vec, y: &String) { | ^^^^^^^ | help: change this to | -LL | fn false_positive_capacity(x: &Vec, y: &str) { - | ~~~~ -help: change `y.clone()` to +LL ~ fn false_positive_capacity(x: &Vec, y: &str) { +LL | let a = x.capacity(); +LL ~ let b = y.to_owned(); +LL ~ let c = y; | -LL | let b = y.to_string(); - | ~~~~~~~~~~~~~ -help: change `y.as_str()` to - | -LL | let c = y; - | ~ error: using a reference to `Cow` is not recommended - --> $DIR/ptr_arg.rs:90:25 + --> $DIR/ptr_arg.rs:87:25 | LL | fn test_cow_with_ref(c: &Cow<[i32]>) {} | ^^^^^^^^^^^ help: change this to: `&[i32]` -error: writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used with non-Vec-based slices - --> $DIR/ptr_arg.rs:143:21 +error: writing `&Vec` instead of `&[_]` involves a new object where a slice will do + --> $DIR/ptr_arg.rs:140:21 | LL | fn foo_vec(vec: &Vec) { | ^^^^^^^^ | help: change this to | -LL | fn foo_vec(vec: &[u8]) { - | ~~~~~ -help: change `vec.clone()` to - | -LL | let _ = vec.to_owned().pop(); - | ~~~~~~~~~~~~~~ -help: change `vec.clone()` to +LL ~ fn foo_vec(vec: &[u8]) { +LL ~ let _ = vec.to_owned().pop(); +LL ~ let _ = vec.to_owned().clone(); | -LL | let _ = vec.to_owned().clone(); - | ~~~~~~~~~~~~~~ error: writing `&PathBuf` instead of `&Path` involves a new object where a slice will do - --> $DIR/ptr_arg.rs:148:23 + --> $DIR/ptr_arg.rs:145:23 | LL | fn foo_path(path: &PathBuf) { | ^^^^^^^^ | help: change this to | -LL | fn foo_path(path: &Path) { - | ~~~~~ -help: change `path.clone()` to +LL ~ fn foo_path(path: &Path) { +LL ~ let _ = path.to_path_buf().pop(); +LL ~ let _ = path.to_path_buf().clone(); | -LL | let _ = path.to_path_buf().pop(); - | ~~~~~~~~~~~~~~~~~~ -help: change `path.clone()` to - | -LL | let _ = path.to_path_buf().clone(); - | ~~~~~~~~~~~~~~~~~~ error: writing `&PathBuf` instead of `&Path` involves a new object where a slice will do - --> $DIR/ptr_arg.rs:153:21 + --> $DIR/ptr_arg.rs:150:21 | LL | fn foo_str(str: &PathBuf) { | ^^^^^^^^ | help: change this to | -LL | fn foo_str(str: &Path) { - | ~~~~~ -help: change `str.clone()` to +LL ~ fn foo_str(str: &Path) { +LL ~ let _ = str.to_path_buf().pop(); +LL ~ let _ = str.to_path_buf().clone(); | -LL | let _ = str.to_path_buf().pop(); - | ~~~~~~~~~~~~~~~~~ -help: change `str.clone()` to + +error: writing `&mut Vec` instead of `&mut [_]` involves a new object where a slice will do + --> $DIR/ptr_arg.rs:156:29 | -LL | let _ = str.to_path_buf().clone(); - | ~~~~~~~~~~~~~~~~~ +LL | fn mut_vec_slice_methods(v: &mut Vec) { + | ^^^^^^^^^^^^^ help: change this to: `&mut [u32]` -error: aborting due to 12 previous errors +error: aborting due to 16 previous errors diff --git a/tests/ui/search_is_some_fixable_none.rs b/tests/ui/search_is_some_fixable_none.rs index 778f4f6fa257e..767518ab0c0f5 100644 --- a/tests/ui/search_is_some_fixable_none.rs +++ b/tests/ui/search_is_some_fixable_none.rs @@ -189,7 +189,7 @@ mod issue7392 { let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().find(|&(&x, y)| x == *y).is_none(); } - fn test_string_1(s: &String) -> bool { + fn test_string_1(s: &str) -> bool { s.is_empty() } diff --git a/tests/ui/search_is_some_fixable_none.stderr b/tests/ui/search_is_some_fixable_none.stderr index 7c5e5eb589c1a..933ce5cf42d2e 100644 --- a/tests/ui/search_is_some_fixable_none.stderr +++ b/tests/ui/search_is_some_fixable_none.stderr @@ -251,14 +251,6 @@ 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: 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 | @@ -289,5 +281,5 @@ error: called `is_none()` after searching an `Iterator` with `find` 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 +error: aborting due to 43 previous errors diff --git a/tests/ui/search_is_some_fixable_some.rs b/tests/ui/search_is_some_fixable_some.rs index 241641fceae3d..77fd52e4ce769 100644 --- a/tests/ui/search_is_some_fixable_some.rs +++ b/tests/ui/search_is_some_fixable_some.rs @@ -188,7 +188,7 @@ mod issue7392 { let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().find(|&(&x, y)| x == *y).is_some(); } - fn test_string_1(s: &String) -> bool { + fn test_string_1(s: &str) -> bool { s.is_empty() } diff --git a/tests/ui/search_is_some_fixable_some.stderr b/tests/ui/search_is_some_fixable_some.stderr index 9212c6e71ff54..8b424f18ef5b5 100644 --- a/tests/ui/search_is_some_fixable_some.stderr +++ b/tests/ui/search_is_some_fixable_some.stderr @@ -234,14 +234,6 @@ 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: 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 | @@ -272,5 +264,5 @@ error: called `is_some()` after searching an `Iterator` with `find` 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 +error: aborting due to 43 previous errors diff --git a/tests/ui/slow_vector_initialization.rs b/tests/ui/slow_vector_initialization.rs index c5ae3ff769b11..7e3d357ae50dc 100644 --- a/tests/ui/slow_vector_initialization.rs +++ b/tests/ui/slow_vector_initialization.rs @@ -53,7 +53,7 @@ fn resize_vector() { vec1.resize(10, 0); } -fn do_stuff(vec: &mut Vec) {} +fn do_stuff(vec: &mut [u8]) {} fn extend_vector_with_manipulations_between() { let len = 300; From 048297b5b265188ca9864d410a8bebb090e2f5ba Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Fri, 21 Jan 2022 09:43:41 -0500 Subject: [PATCH 2/2] `ptr_arg` cleanup --- clippy_lints/src/ptr.rs | 162 ++++++++++++++++++++-------------------- tests/ui/ptr_arg.rs | 12 +-- 2 files changed, 89 insertions(+), 85 deletions(-) diff --git a/clippy_lints/src/ptr.rs b/clippy_lints/src/ptr.rs index b74ba40d452f8..c2ade4f0db764 100644 --- a/clippy_lints/src/ptr.rs +++ b/clippy_lints/src/ptr.rs @@ -385,87 +385,91 @@ fn check_fn_args<'cx, 'tcx: 'cx>( hir_tys: &'tcx [hir::Ty<'_>], params: &'tcx [Param<'_>], ) -> impl Iterator> + 'cx { - tys.iter().enumerate().filter_map(|(i, ty)| { - if_chain! { - if let ty::Ref(_, ty, mutability) = *ty.kind(); - if let ty::Adt(adt, substs) = *ty.kind(); - - let hir_ty = &hir_tys[i]; - if let TyKind::Rptr(lt, ref ty) = hir_ty.kind; - if let TyKind::Path(QPath::Resolved(None, path)) = ty.ty.kind; - - if let [.., name] = path.segments; - if cx.tcx.item_name(adt.did) == name.ident.name; - - if !is_lint_allowed(cx, PTR_ARG, hir_ty.hir_id); - if params.get(i).map_or(true, |p| !is_lint_allowed(cx, PTR_ARG, p.hir_id)); - - then { - let (method_renames, deref_ty, deref_impl_id) = match cx.tcx.get_diagnostic_name(adt.did) { - Some(sym::Vec) => ( - [("clone", ".to_owned()")].as_slice(), - DerefTy::Slice( - name.args - .and_then(|args| args.args.first()) - .and_then(|arg| if let GenericArg::Type(ty) = arg { - Some(ty.span) - } else { - None - }), - substs.type_at(0), + tys.iter() + .zip(hir_tys.iter()) + .enumerate() + .filter_map(|(i, (ty, hir_ty))| { + if_chain! { + if let ty::Ref(_, ty, mutability) = *ty.kind(); + if let ty::Adt(adt, substs) = *ty.kind(); + + if let TyKind::Rptr(lt, ref ty) = hir_ty.kind; + if let TyKind::Path(QPath::Resolved(None, path)) = ty.ty.kind; + + // Check that the name as typed matches the actual name of the type. + // e.g. `fn foo(_: &Foo)` shouldn't trigger the lint when `Foo` is an alias for `Vec` + if let [.., name] = path.segments; + if cx.tcx.item_name(adt.did) == name.ident.name; + + if !is_lint_allowed(cx, PTR_ARG, hir_ty.hir_id); + if params.get(i).map_or(true, |p| !is_lint_allowed(cx, PTR_ARG, p.hir_id)); + + then { + let (method_renames, deref_ty, deref_impl_id) = match cx.tcx.get_diagnostic_name(adt.did) { + Some(sym::Vec) => ( + [("clone", ".to_owned()")].as_slice(), + DerefTy::Slice( + name.args + .and_then(|args| args.args.first()) + .and_then(|arg| if let GenericArg::Type(ty) = arg { + Some(ty.span) + } else { + None + }), + substs.type_at(0), + ), + cx.tcx.lang_items().slice_impl() ), - cx.tcx.lang_items().slice_impl() - ), - Some(sym::String) => ( - [("clone", ".to_owned()"), ("as_str", "")].as_slice(), - DerefTy::Str, - cx.tcx.lang_items().str_impl() - ), - Some(sym::PathBuf) => ( - [("clone", ".to_path_buf()"), ("as_path", "")].as_slice(), - DerefTy::Path, - None, - ), - Some(sym::Cow) => { - let ty_name = name.args - .and_then(|args| { - args.args.iter().find_map(|a| match a { - GenericArg::Type(x) => Some(x), - _ => None, + Some(sym::String) => ( + [("clone", ".to_owned()"), ("as_str", "")].as_slice(), + DerefTy::Str, + cx.tcx.lang_items().str_impl() + ), + Some(sym::PathBuf) => ( + [("clone", ".to_path_buf()"), ("as_path", "")].as_slice(), + DerefTy::Path, + None, + ), + Some(sym::Cow) => { + let ty_name = name.args + .and_then(|args| { + args.args.iter().find_map(|a| match a { + GenericArg::Type(x) => Some(x), + _ => None, + }) }) - }) - .and_then(|arg| snippet_opt(cx, arg.span)) - .unwrap_or_else(|| substs.type_at(1).to_string()); - span_lint_and_sugg( - cx, - PTR_ARG, - hir_ty.span, - "using a reference to `Cow` is not recommended", - "change this to", - format!("&{}{}", mutability.prefix_str(), ty_name), - Applicability::Unspecified, - ); - return None; - }, - _ => return None, - }; - return Some(PtrArg { - idx: i, - span: hir_ty.span, - ty_did: adt.did, - ty_name: name.ident.name, - method_renames, - ref_prefix: RefPrefix { - lt: lt.name, - mutability, - }, - deref_ty, - deref_assoc_items: deref_impl_id.map(|id| (id, cx.tcx.associated_items(id))), - }); + .and_then(|arg| snippet_opt(cx, arg.span)) + .unwrap_or_else(|| substs.type_at(1).to_string()); + span_lint_and_sugg( + cx, + PTR_ARG, + hir_ty.span, + "using a reference to `Cow` is not recommended", + "change this to", + format!("&{}{}", mutability.prefix_str(), ty_name), + Applicability::Unspecified, + ); + return None; + }, + _ => return None, + }; + return Some(PtrArg { + idx: i, + span: hir_ty.span, + ty_did: adt.did, + ty_name: name.ident.name, + method_renames, + ref_prefix: RefPrefix { + lt: lt.name, + mutability, + }, + deref_ty, + deref_assoc_items: deref_impl_id.map(|id| (id, cx.tcx.associated_items(id))), + }); + } } - } - None - }) + None + }) } fn check_mut_from_ref(cx: &LateContext<'_>, decl: &FnDecl<'_>) { @@ -504,7 +508,7 @@ fn check_mut_from_ref(cx: &LateContext<'_>, decl: &FnDecl<'_>) { fn check_ptr_arg_usage<'tcx>(cx: &LateContext<'tcx>, body: &'tcx Body<'_>, args: &[PtrArg<'tcx>]) -> Vec { struct V<'cx, 'tcx> { cx: &'cx LateContext<'tcx>, - /// Map from a local id to which argument it cam from (index into `Self::args` and + /// Map from a local id to which argument it came from (index into `Self::args` and /// `Self::results`) bindings: HirIdMap, /// The arguments being checked. diff --git a/tests/ui/ptr_arg.rs b/tests/ui/ptr_arg.rs index 6cd7e96d7c1bd..ed72423780821 100644 --- a/tests/ui/ptr_arg.rs +++ b/tests/ui/ptr_arg.rs @@ -169,13 +169,13 @@ fn fn_requires_vec(v: &Vec) -> bool { vec_contains(v) } -// fn impl_fn_requires_vec(v: &Vec, f: impl Fn(&Vec)) { -// f(v); -// } +fn impl_fn_requires_vec(v: &Vec, f: impl Fn(&Vec)) { + f(v); +} -// fn dyn_fn_requires_vec(v: &Vec, f: &dyn Fn(&Vec)) { -// f(v); -// } +fn dyn_fn_requires_vec(v: &Vec, f: &dyn Fn(&Vec)) { + f(v); +} // No error for types behind an alias (#7699) type A = Vec;