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
This commit is contained in:
Jason Newcomb 2022-01-09 09:35:55 -05:00
parent 60e68d68c6
commit 7ed86bf822
11 changed files with 654 additions and 335 deletions

View file

@ -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<Vec<T>>`. 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<u8>) -> bool {
/// let vec_of_vecs: Vec<Vec<u8>> = 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_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_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);
if let TraitItemKind::Fn(sig, trait_method) = &item.kind {
if matches!(trait_method, TraitFn::Provided(_)) {
// Handled by check body.
return;
}
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,
);
}
}
}
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<BodyId>) {
let body = opt_body_id.map(|id| cx.tcx.hir().body(id));
#[derive(Default)]
struct PtrArgResult {
skip: bool,
replacements: Vec<PtrArgReplacement>,
}
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;
}
struct PtrArgReplacement {
expr_span: Span,
self_span: Span,
replacement: &'static str,
}
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(' ')?;
},
LifetimeName::Underscore => f.write_str("'_ ")?,
LifetimeName::Static => f.write_str("'static ")?,
_ => (),
}
f.write_str(self.mutability.prefix_str())
}
}
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 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(']')
},
}
}
}
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,
);
}
},
);
}
},
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,
);
}
},
);
}
},
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 {
span_lint_and_sugg(
cx,
PTR_ARG,
arg.span,
"using a reference to `Cow` is not recommended",
"change this to",
"&".to_owned() + &r,
Applicability::Unspecified,
);
}
}
},
_ => {},
enum DerefTy<'tcx> {
Str,
Path,
Slice(Option<Span>, 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<Item = PtrArg<'tcx>> + '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,
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
})
}
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<BodyId>
}
}
fn get_only_generic_arg_snippet(cx: &LateContext<'_>, arg: &Ty<'_>) -> Option<String> {
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<PtrArgResult> {
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<usize>,
/// The arguments being checked.
args: &'cx [PtrArg<'tcx>],
/// The results for each argument (len should match args.len)
results: Vec<PtrArgResult>,
/// 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<Self::Map> {
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::<Vec<_>>();
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 {

View file

@ -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,

View file

@ -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<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 {
@ -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,
..
},
..
},
..
}),
..
}))
}),
..
}),
_
))
)
}

View file

@ -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,
}
}
}

View file

@ -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) {}

View file

@ -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<i64>) {
@ -6,170 +6,154 @@ LL | fn do_vec(x: &Vec<i64>) {
|
= 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<i64>) {
| ^^^^^^^^^^^^^ 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<i64>);
| ^^^^^^^^^ 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<u8>) -> Vec<u8> {
| ^^^^^^^^
|
help: change this to
|
LL | fn cloned(x: &[u8]) -> Vec<u8> {
| ~~~~~
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<u8> {
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<u8>, y: &String) {
| ^^^^^^^
|
help: change this to
|
LL | fn false_positive_capacity(x: &Vec<u8>, y: &str) {
| ~~~~
help: change `y.clone()` to
LL ~ fn false_positive_capacity(x: &Vec<u8>, 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<u8>) {
| ^^^^^^^^
|
help: change this to
|
LL | fn foo_vec(vec: &[u8]) {
| ~~~~~
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().pop();
| ~~~~~~~~~~~~~~
help: change `vec.clone()` to
|
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<u32>) {
| ^^^^^^^^^^^^^ help: change this to: `&mut [u32]`
error: aborting due to 12 previous errors
error: aborting due to 16 previous errors

View file

@ -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()
}

View file

@ -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

View file

@ -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()
}

View file

@ -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

View file

@ -53,7 +53,7 @@ fn resize_vector() {
vec1.resize(10, 0);
}
fn do_stuff(vec: &mut Vec<u8>) {}
fn do_stuff(vec: &mut [u8]) {}
fn extend_vector_with_manipulations_between() {
let len = 300;