diff --git a/clippy_lints/src/bytecount.rs b/clippy_lints/src/bytecount.rs index 447214c70f8..58f1227d91e 100644 --- a/clippy_lints/src/bytecount.rs +++ b/clippy_lints/src/bytecount.rs @@ -2,7 +2,8 @@ use rustc::hir::*; use rustc::lint::*; use rustc::ty; use syntax::ast::{Name, UintTy}; -use utils::{contains_name, match_type, paths, single_segment_path, snippet, span_lint_and_sugg, walk_ptrs_ty}; +use utils::{contains_name, get_pat_name, match_type, paths, single_segment_path, + snippet, span_lint_and_sugg, walk_ptrs_ty}; /// **What it does:** Checks for naive byte counts /// @@ -93,15 +94,6 @@ fn check_arg(name: Name, arg: Name, needle: &Expr) -> bool { name == arg && !contains_name(name, needle) } -fn get_pat_name(pat: &Pat) -> Option { - match pat.node { - PatKind::Binding(_, _, ref spname, _) => Some(spname.node), - PatKind::Path(ref qpath) => single_segment_path(qpath).map(|ps| ps.name), - PatKind::Box(ref p) | PatKind::Ref(ref p, _) => get_pat_name(&*p), - _ => None, - } -} - fn get_path_name(expr: &Expr) -> Option { match expr.node { ExprBox(ref e) | ExprAddrOf(_, ref e) | ExprUnary(UnOp::UnDeref, ref e) => get_path_name(e), diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index f06831556c2..3d6f7d56808 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -16,7 +16,7 @@ use utils::sugg; use utils::const_to_u64; use utils::{get_enclosing_block, get_parent_expr, higher, in_external_macro, is_integer_literal, is_refutable, - last_path_segment, match_trait_method, match_type, multispan_sugg, snippet, snippet_opt, + last_path_segment, match_trait_method, match_type, match_var, multispan_sugg, snippet, snippet_opt, span_help_and_lint, span_lint, span_lint_and_sugg, span_lint_and_then}; use utils::paths; @@ -1271,15 +1271,6 @@ fn pat_is_wild<'tcx>(pat: &'tcx PatKind, body: &'tcx Expr) -> bool { } } -fn match_var(expr: &Expr, var: Name) -> bool { - if let ExprPath(QPath::Resolved(None, ref path)) = expr.node { - if path.segments.len() == 1 && path.segments[0].name == var { - return true; - } - } - false -} - struct UsedVisitor { var: ast::Name, // var to look for used: bool, // has the var been used otherwise? diff --git a/clippy_lints/src/ptr.rs b/clippy_lints/src/ptr.rs index 4d119c3a42d..cbba4ad2610 100644 --- a/clippy_lints/src/ptr.rs +++ b/clippy_lints/src/ptr.rs @@ -1,26 +1,42 @@ //! Checks for usage of `&Vec[_]` and `&String`. use rustc::hir::*; +use rustc::hir::intravisit::{walk_expr, NestedVisitorMap, Visitor}; use rustc::hir::map::NodeItem; use rustc::lint::*; use rustc::ty; -use syntax::ast::NodeId; +use syntax::ast::{Name, NodeId}; use syntax::codemap::Span; use syntax_pos::MultiSpan; -use utils::{match_qpath, match_type, paths, snippet_opt, span_lint, span_lint_and_then, - span_lint_and_sugg, walk_ptrs_hir_ty}; +use utils::{get_pat_name, match_qpath, match_type, match_var, paths, + snippet, snippet_opt, span_lint, span_lint_and_then, + walk_ptrs_hir_ty}; /// **What it does:** This lint checks for function arguments of type `&String` -/// or `&Vec` unless -/// the references are mutable. +/// or `&Vec` unless the references are mutable. 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 -/// makes the function less -/// useful for no benefit; slices in the form of `&[T]` or `&str` usually -/// suffice and can be -/// obtained from other types, too. +/// makes the function less useful for no benefit; slices in the form of `&[T]` +/// or `&str` usually suffice and can be obtained from other types, too. /// -/// **Known problems:** None. +/// **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. +/// +/// Also 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. +/// +/// Note that if the function is part of your public interface, there may be +/// other crates referencing it you may not be aware. Carefully deprecate the +/// function before applying the lint suggestions in this case. /// /// **Example:** /// ```rust @@ -87,25 +103,26 @@ impl LintPass for PointerPass { impl<'a, 'tcx> LateLintPass<'a, 'tcx> for PointerPass { fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx Item) { - if let ItemFn(ref decl, _, _, _, _, _) = item.node { - check_fn(cx, decl, item.id); + if let ItemFn(ref decl, _, _, _, _, body_id) = item.node { + check_fn(cx, decl, item.id, Some(body_id)); } } fn check_impl_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx ImplItem) { - if let ImplItemKind::Method(ref sig, _) = item.node { + if let ImplItemKind::Method(ref sig, body_id) = item.node { if let Some(NodeItem(it)) = cx.tcx.hir.find(cx.tcx.hir.get_parent(item.id)) { if let ItemImpl(_, _, _, _, Some(_), _, _) = it.node { return; // ignore trait impls } } - check_fn(cx, &sig.decl, item.id); + check_fn(cx, &sig.decl, item.id, Some(body_id)); } } fn check_trait_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx TraitItem) { - if let TraitItemKind::Method(ref sig, _) = item.node { - check_fn(cx, &sig.decl, item.id); + if let TraitItemKind::Method(ref sig, ref trait_method) = item.node { + let body_id = if let TraitMethod::Provided(b) = *trait_method { Some(b) } else { None }; + check_fn(cx, &sig.decl, item.id, body_id); } } @@ -123,12 +140,12 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for PointerPass { } } -fn check_fn(cx: &LateContext, decl: &FnDecl, fn_id: NodeId) { +fn check_fn(cx: &LateContext, decl: &FnDecl, fn_id: NodeId, opt_body_id: Option) { let fn_def_id = cx.tcx.hir.local_def_id(fn_id); let sig = cx.tcx.fn_sig(fn_def_id); let fn_ty = sig.skip_binder(); - for (arg, ty) in decl.inputs.iter().zip(fn_ty.inputs()) { + for (idx, (arg, ty)) in decl.inputs.iter().zip(fn_ty.inputs()).enumerate() { if let ty::TyRef( _, ty::TypeAndMut { @@ -146,7 +163,7 @@ fn check_fn(cx: &LateContext, decl: &FnDecl, fn_id: NodeId) { ], { ty_snippet = snippet_opt(cx, parameters.types[0].span); }); - //TODO: Suggestion + let spans = get_spans(cx, opt_body_id, idx, "to_owned"); span_lint_and_then( cx, PTR_ARG, @@ -159,16 +176,30 @@ fn check_fn(cx: &LateContext, decl: &FnDecl, fn_id: NodeId) { "change this to", format!("&[{}]", snippet)); } + for (clonespan, suggestion) in spans { + db.span_suggestion(clonespan, + "change the `.clone()` to", + suggestion); + } } ); } else if match_type(cx, ty, &paths::STRING) { - span_lint_and_sugg( + let spans = get_spans(cx, opt_body_id, idx, "to_string"); + span_lint_and_then( cx, PTR_ARG, arg.span, "writing `&String` instead of `&str` involves a new object where a slice will do.", - "change this to", - "&str".to_string() + |db| { + db.span_suggestion(arg.span, + "change this to", + "&str".into()); + for (clonespan, suggestion) in spans { + db.span_suggestion_short(clonespan, + "change the `.clone` to ", + suggestion); + } + } ); } } @@ -198,6 +229,54 @@ fn check_fn(cx: &LateContext, decl: &FnDecl, fn_id: NodeId) { } } +fn get_spans(cx: &LateContext, opt_body_id: Option, idx: usize, fn_name: &'static str) -> Vec<(Span, String)> { + if let Some(body) = opt_body_id.map(|id| cx.tcx.hir.body(id)) { + get_binding_name(&body.arguments[idx]).map_or_else(Vec::new, + |name| extract_clone_suggestions(cx, name, fn_name, body)) + } else { + vec![] + } +} + +fn extract_clone_suggestions<'a, 'tcx: 'a>(cx: &LateContext<'a, 'tcx>, name: Name, fn_name: &'static str, body: &'tcx Body) -> Vec<(Span, String)> { + let mut visitor = PtrCloneVisitor { + cx, + name, + fn_name, + spans: vec![] + }; + visitor.visit_body(body); + visitor.spans +} + +struct PtrCloneVisitor<'a, 'tcx: 'a> { + cx: &'a LateContext<'a, 'tcx>, + name: Name, + fn_name: &'static str, + spans: Vec<(Span, String)>, +} + +impl<'a, 'tcx: 'a> Visitor<'tcx> for PtrCloneVisitor<'a, 'tcx> { + fn visit_expr(&mut self, expr: &'tcx Expr) { + if let ExprMethodCall(ref seg, _, ref args) = expr.node { + if args.len() == 1 && match_var(&args[0], self.name) && seg.name == "clone" { + self.spans.push((expr.span, format!("{}.{}()", snippet(self.cx, args[0].span, "_"), self.fn_name))); + } + return; + } + walk_expr(self, expr); + } + + fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { + NestedVisitorMap::None + } + +} + +fn get_binding_name(arg: &Arg) -> Option { + get_pat_name(&arg.pat) +} + fn get_rptr_lm(ty: &Ty) -> Option<(&Lifetime, Mutability, Span)> { if let Ty_::TyRptr(ref lt, ref m) = ty.node { Some((lt, m.mutbl, ty.span)) diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 6173073fb76..ec0521ce4f2 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -218,6 +218,17 @@ pub fn match_trait_method(cx: &LateContext, expr: &Expr, path: &[&str]) -> bool } } +/// Check if an expression references a variable of the given name. +pub fn match_var(expr: &Expr, var: Name) -> bool { + if let ExprPath(QPath::Resolved(None, ref path)) = expr.node { + if path.segments.len() == 1 && path.segments[0].name == var { + return true; + } + } + false +} + + pub fn last_path_segment(path: &QPath) -> &PathSegment { match *path { QPath::Resolved(_, ref path) => path.segments @@ -393,6 +404,16 @@ pub fn get_item_name(cx: &LateContext, expr: &Expr) -> Option { } } +/// Get the name of a `Pat`, if any +pub fn get_pat_name(pat: &Pat) -> Option { + match pat.node { + PatKind::Binding(_, _, ref spname, _) => Some(spname.node), + PatKind::Path(ref qpath) => single_segment_path(qpath).map(|ps| ps.name), + PatKind::Box(ref p) | PatKind::Ref(ref p, _) => get_pat_name(&*p), + _ => None, + } +} + struct ContainsName { name: Name, result: bool, diff --git a/tests/ui/ptr_arg.rs b/tests/ui/ptr_arg.rs index 5649bfec347..a386fcf82df 100644 --- a/tests/ui/ptr_arg.rs +++ b/tests/ui/ptr_arg.rs @@ -1,6 +1,6 @@ #![feature(plugin)] #![plugin(clippy)] -#![allow(unused)] +#![allow(unused, many_single_char_names)] #![warn(ptr_arg)] fn do_vec(x: &Vec) { @@ -34,5 +34,24 @@ struct Bar; impl Foo for Bar { type Item = Vec; fn do_vec(x: &Vec) {} - fn do_item(x: &Vec) {} + fn do_item(x: &Vec) {} +} + +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 i = (e).clone(); + x.clone() +} + +fn str_cloned(x: &String) -> String { + let a = x.clone(); + let b = x.clone(); + let c = b.clone(); + let d = a.clone() + .clone() + .clone(); + x.clone() } diff --git a/tests/ui/ptr_arg.stderr b/tests/ui/ptr_arg.stderr index 4eafc237a82..46d7cbdb031 100644 --- a/tests/ui/ptr_arg.stderr +++ b/tests/ui/ptr_arg.stderr @@ -18,5 +18,47 @@ error: writing `&Vec<_>` instead of `&[_]` involves one more reference and canno 27 | fn do_vec(x: &Vec); | ^^^^^^^^^ help: change this to: `&[i64]` -error: aborting due to 3 previous errors +error: writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used with non-Vec-based slices. + --> $DIR/ptr_arg.rs:40:14 + | +40 | fn cloned(x: &Vec) -> Vec { + | ^^^^^^^^ + | +help: change this to + | +40 | fn cloned(x: &[u8]) -> Vec { + | ^^^^^ +help: change the `.clone()` to + | +41 | let e = x.to_owned(); + | ^^^^^^^^^^^^ +help: change the `.clone()` to + | +46 | x.to_owned() + | ^^^^^^^^^^^^ + +error: writing `&String` instead of `&str` involves a new object where a slice will do. + --> $DIR/ptr_arg.rs:49:18 + | +49 | fn str_cloned(x: &String) -> String { + | ^^^^^^^ + | +help: change this to + | +49 | fn str_cloned(x: &str) -> String { + | ^^^^ +help: change the `.clone` to + | +50 | let a = x.to_string(); + | ^^^^^^^^^^^^^ +help: change the `.clone` to + | +51 | let b = x.to_string(); + | ^^^^^^^^^^^^^ +help: change the `.clone` to + | +56 | x.to_string() + | ^^^^^^^^^^^^^ + +error: aborting due to 5 previous errors