From 56e8db476c437590f21be041c0e751ee185d8dd0 Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Mon, 24 Aug 2015 18:13:02 +0200 Subject: [PATCH] new lint: inherent methods that should be trait impls (fixes #218) --- README.md | 3 +- src/lib.rs | 1 + src/methods.rs | 118 +++++++++++++++++++++++++++++++++- tests/compile-fail/methods.rs | 23 ++++++- 4 files changed, 140 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index be7154c8c..9d86ece99 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ A collection of lints that give helpful tips to newbies and catch oversights. ##Lints -There are 45 lints included in this crate: +There are 46 lints included in this crate: name | default | meaning -------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------- @@ -44,6 +44,7 @@ ptr_arg | allow | fn arguments of the type `&Vec<...>` or `&S range_step_by_zero | warn | using Range::step_by(0), which produces an infinite iterator redundant_closure | warn | using redundant closures, i.e. `|a| foo(a)` (which can be written as just `foo`) result_unwrap_used | allow | using `Result.unwrap()`, which might be better handled +should_implement_trait | warn | defining a method that should be implementing a std trait single_match | warn | a match statement with a single nontrivial arm (i.e, where the other arm is `_ => {}`) is used; recommends `if let` instead str_to_string | warn | using `to_string()` on a str, which should be `to_owned()` string_add | allow | using `x + ..` where x is a `String`; suggests using `push_str()` instead diff --git a/src/lib.rs b/src/lib.rs index 863ba2624..bacff19ad 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -92,6 +92,7 @@ pub fn plugin_registrar(reg: &mut Registry) { matches::SINGLE_MATCH, methods::OPTION_UNWRAP_USED, methods::RESULT_UNWRAP_USED, + methods::SHOULD_IMPLEMENT_TRAIT, methods::STR_TO_STRING, methods::STRING_TO_STRING, misc::CMP_NAN, diff --git a/src/methods.rs b/src/methods.rs index df8e35d98..73cf81fbd 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -2,9 +2,12 @@ use syntax::ast::*; use rustc::lint::*; use rustc::middle::ty; -use utils::{span_lint, match_type, walk_ptrs_ty}; +use utils::{span_lint, match_path, match_type, walk_ptrs_ty}; use utils::{OPTION_PATH, RESULT_PATH, STRING_PATH}; +use self::SelfKind::*; +use self::OutType::*; + #[derive(Copy,Clone)] pub struct MethodsPass; @@ -16,10 +19,13 @@ declare_lint!(pub STR_TO_STRING, Warn, "using `to_string()` on a str, which should be `to_owned()`"); declare_lint!(pub STRING_TO_STRING, Warn, "calling `String.to_string()` which is a no-op"); +declare_lint!(pub SHOULD_IMPLEMENT_TRAIT, Warn, + "defining a method that should be implementing a std trait"); impl LintPass for MethodsPass { fn get_lints(&self) -> LintArray { - lint_array!(OPTION_UNWRAP_USED, RESULT_UNWRAP_USED, STR_TO_STRING, STRING_TO_STRING) + lint_array!(OPTION_UNWRAP_USED, RESULT_UNWRAP_USED, STR_TO_STRING, STRING_TO_STRING, + SHOULD_IMPLEMENT_TRAIT) } fn check_expr(&mut self, cx: &Context, expr: &Expr) { @@ -46,4 +52,112 @@ impl LintPass for MethodsPass { } } } + + fn check_item(&mut self, cx: &Context, item: &Item) { + if let ItemImpl(_, _, _, None, _, ref items) = item.node { + for item in items { + let name = item.ident.name; + for &(method_name, n_args, self_kind, out_type, trait_name) in &TRAIT_METHODS { + if_let_chain! { + [ + name == method_name, + let MethodImplItem(ref sig, _) = item.node, + sig.decl.inputs.len() == n_args, + out_type.matches(&sig.decl.output), + self_kind.matches(&sig.explicit_self.node) + ], { + span_lint(cx, SHOULD_IMPLEMENT_TRAIT, item.span, &format!( + "defining a method called `{}` on this type; consider implementing \ + the `{}` trait or choosing a less ambiguous name", name, trait_name)); + } + } + } + } + } + } +} + +const TRAIT_METHODS: [(&'static str, usize, SelfKind, OutType, &'static str); 30] = [ + ("add", 2, ValueSelf, AnyType, "std::ops::Add`"), + ("sub", 2, ValueSelf, AnyType, "std::ops::Sub"), + ("mul", 2, ValueSelf, AnyType, "std::ops::Mul"), + ("div", 2, ValueSelf, AnyType, "std::ops::Div"), + ("rem", 2, ValueSelf, AnyType, "std::ops::Rem"), + ("shl", 2, ValueSelf, AnyType, "std::ops::Shl"), + ("shr", 2, ValueSelf, AnyType, "std::ops::Shr"), + ("bitand", 2, ValueSelf, AnyType, "std::ops::BitAnd"), + ("bitor", 2, ValueSelf, AnyType, "std::ops::BitOr"), + ("bitxor", 2, ValueSelf, AnyType, "std::ops::BitXor"), + ("neg", 1, ValueSelf, AnyType, "std::ops::Neg"), + ("not", 1, ValueSelf, AnyType, "std::ops::Not"), + ("drop", 1, RefMutSelf, UnitType, "std::ops::Drop"), + ("index", 2, RefSelf, RefType, "std::ops::Index"), + ("index_mut", 2, RefMutSelf, RefType, "std::ops::IndexMut"), + ("deref", 1, RefSelf, RefType, "std::ops::Deref"), + ("deref_mut", 1, RefMutSelf, RefType, "std::ops::DerefMut"), + ("clone", 1, RefSelf, AnyType, "std::clone::Clone"), + ("borrow", 1, RefSelf, RefType, "std::borrow::Borrow"), + ("borrow_mut", 1, RefMutSelf, RefType, "std::borrow::BorrowMut"), + ("as_ref", 1, RefSelf, RefType, "std::convert::AsRef"), + ("as_mut", 1, RefMutSelf, RefType, "std::convert::AsMut"), + ("eq", 2, RefSelf, BoolType, "std::cmp::PartialEq"), + ("cmp", 2, RefSelf, AnyType, "std::cmp::Ord"), + ("default", 0, NoSelf, AnyType, "std::default::Default"), + ("hash", 2, RefSelf, UnitType, "std::hash::Hash"), + ("next", 1, RefMutSelf, AnyType, "std::iter::Iterator"), + ("into_iter", 1, ValueSelf, AnyType, "std::iter::IntoIterator"), + ("from_iter", 1, NoSelf, AnyType, "std::iter::FromIterator"), + ("from_str", 1, NoSelf, AnyType, "std::str::FromStr"), +]; + +#[derive(Clone, Copy)] +enum SelfKind { + ValueSelf, + RefSelf, + RefMutSelf, + NoSelf +} + +impl SelfKind { + fn matches(&self, slf: &ExplicitSelf_) -> bool { + match (self, slf) { + (&ValueSelf, &SelfValue(_)) => true, + (&RefSelf, &SelfRegion(_, Mutability::MutImmutable, _)) => true, + (&RefMutSelf, &SelfRegion(_, Mutability::MutMutable, _)) => true, + (&NoSelf, &SelfStatic) => true, + _ => false + } + } +} + +#[derive(Clone, Copy)] +enum OutType { + UnitType, + BoolType, + AnyType, + RefType, +} + +impl OutType { + fn matches(&self, ty: &FunctionRetTy) -> bool { + match (self, ty) { + (&UnitType, &DefaultReturn(_)) => true, + (&UnitType, &Return(ref ty)) if ty.node == TyTup(vec![]) => true, + (&BoolType, &Return(ref ty)) if is_bool(ty) => true, + (&AnyType, &Return(ref ty)) if ty.node != TyTup(vec![]) => true, + (&RefType, &Return(ref ty)) => { + if let TyRptr(_, _) = ty.node { true } else { false } + } + _ => false + } + } +} + +fn is_bool(ty: &Ty) -> bool { + if let TyPath(None, ref p) = ty.node { + if match_path(p, &["bool"]) { + return true; + } + } + false } diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index 91d3b72de..cb77d79f0 100755 --- a/tests/compile-fail/methods.rs +++ b/tests/compile-fail/methods.rs @@ -1,8 +1,27 @@ #![feature(plugin)] #![plugin(clippy)] -#[deny(option_unwrap_used, result_unwrap_used)] -#[deny(str_to_string, string_to_string)] +#![allow(unused)] +#![deny(clippy)] + +use std::ops::Mul; + +struct T; + +impl T { + fn add(self, other: T) -> T { self } //~ERROR defining a method called `add` + fn drop(&mut self) { } //~ERROR defining a method called `drop` + + fn sub(&self, other: T) -> &T { self } // no error, self is a ref + fn div(self) -> T { self } // no error, different #arguments + fn rem(self, other: T) { } // no error, wrong return type +} + +impl Mul for T { + type Output = T; + fn mul(self, other: T) -> T { self } // no error, obviously +} + fn main() { let opt = Some(0); let _ = opt.unwrap(); //~ERROR used unwrap() on an Option