From 100786419807ac5365185b659d46acc67f5b1ccf Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Tue, 1 Sep 2015 18:52:48 +0200 Subject: [PATCH] new lint: self conventions for certain method names (fixes #267) --- README.md | 3 +- src/lib.rs | 1 + src/methods.rs | 68 +++++++++++++++++++++++++++-------- tests/compile-fail/methods.rs | 3 ++ 4 files changed, 60 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index 27be09c6127..1232f526875 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 52 lints included in this crate: +There are 53 lints included in this crate: name | default | meaning -----------------------------------------------------------------------------------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------- @@ -59,6 +59,7 @@ name [unit_cmp](https://github.com/Manishearth/rust-clippy/wiki#unit_cmp) | warn | comparing unit values (which is always `true` or `false`, respectively) [unused_collect](https://github.com/Manishearth/rust-clippy/wiki#unused_collect) | warn | `collect()`ing an iterator without using the result; this is usually better written as a for loop [while_let_loop](https://github.com/Manishearth/rust-clippy/wiki#while_let_loop) | warn | `loop { if let { ... } else break }` can be written as a `while let` loop +[wrong_self_convention](https://github.com/Manishearth/rust-clippy/wiki#wrong_self_convention) | warn | defining a method named with an established prefix (like "into_") that takes `self` with the wrong convention [zero_width_space](https://github.com/Manishearth/rust-clippy/wiki#zero_width_space) | deny | using a zero-width space in a string literal, which is confusing More to come, please [file an issue](https://github.com/Manishearth/rust-clippy/issues) if you have ideas! diff --git a/src/lib.rs b/src/lib.rs index f4da8b03bc2..e51732818ba 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -114,6 +114,7 @@ pub fn plugin_registrar(reg: &mut Registry) { methods::SHOULD_IMPLEMENT_TRAIT, methods::STR_TO_STRING, methods::STRING_TO_STRING, + methods::WRONG_SELF_CONVENTION, misc::CMP_NAN, misc::CMP_OWNED, misc::FLOAT_CMP, diff --git a/src/methods.rs b/src/methods.rs index 50f3512b4f0..5a20ba06831 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -23,11 +23,14 @@ 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"); +declare_lint!(pub WRONG_SELF_CONVENTION, Warn, + "defining a method named with an established prefix (like \"into_\") that takes \ + `self` with the wrong convention"); impl LintPass for MethodsPass { fn get_lints(&self) -> LintArray { lint_array!(OPTION_UNWRAP_USED, RESULT_UNWRAP_USED, STR_TO_STRING, STRING_TO_STRING, - SHOULD_IMPLEMENT_TRAIT) + SHOULD_IMPLEMENT_TRAIT, WRONG_SELF_CONVENTION) } fn check_expr(&mut self, cx: &Context, expr: &Expr) { @@ -68,18 +71,28 @@ impl LintPass for MethodsPass { 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)); + if let MethodImplItem(ref sig, _) = item.node { + // check missing trait implementations + for &(method_name, n_args, self_kind, out_type, trait_name) in &TRAIT_METHODS { + if_let_chain! { + [ + name == method_name, + 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)); + } + } + } + // check conventions w.r.t. conversion method names and predicates + for &(prefix, self_kind) in &CONVENTIONS { + if name.as_str().starts_with(prefix) && !self_kind.matches(&sig.explicit_self.node) { + span_lint(cx, WRONG_SELF_CONVENTION, sig.explicit_self.span, &format!( + "methods called `{}*` usually take {}; consider choosing a less \ + ambiguous name", prefix, self_kind.description())); } } } @@ -88,6 +101,14 @@ impl LintPass for MethodsPass { } } +const CONVENTIONS: [(&'static str, SelfKind); 5] = [ + ("into_", ValueSelf), + ("to_", RefSelf), + ("as_", RefSelf), + ("is_", RefSelf), + ("from_", NoSelf), +]; + 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"), @@ -126,7 +147,7 @@ enum SelfKind { ValueSelf, RefSelf, RefMutSelf, - NoSelf + NoSelf, } impl SelfKind { @@ -136,9 +157,28 @@ impl SelfKind { (&RefSelf, &SelfRegion(_, Mutability::MutImmutable, _)) => true, (&RefMutSelf, &SelfRegion(_, Mutability::MutMutable, _)) => true, (&NoSelf, &SelfStatic) => true, + (_, &SelfExplicit(ref ty, _)) => self.matches_explicit_type(ty), _ => false } } + + fn matches_explicit_type(&self, ty: &Ty) -> bool { + match (self, &ty.node) { + (&ValueSelf, &TyPath(..)) => true, + (&RefSelf, &TyRptr(_, MutTy { mutbl: Mutability::MutImmutable, .. })) => true, + (&RefMutSelf, &TyRptr(_, MutTy { mutbl: Mutability::MutMutable, .. })) => true, + _ => false + } + } + + fn description(&self) -> &'static str { + match *self { + ValueSelf => "self by value", + RefSelf => "self by reference", + RefMutSelf => "self by mutable reference", + NoSelf => "no self", + } + } } #[derive(Clone, Copy)] diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index 1c81fefc4e5..560f36a9d5d 100755 --- a/tests/compile-fail/methods.rs +++ b/tests/compile-fail/methods.rs @@ -15,6 +15,9 @@ impl T { 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 + + fn into_u32(self) -> u32 { 0 } // fine + fn into_u16(&self) -> u16 { 0 } //~ERROR methods called `into_*` usually take self by value } impl Mul for T {