Skip to content

Commit

Permalink
Merge pull request #730 from mcarton/unused-labels
Browse files Browse the repository at this point in the history
Lint unused labels and types with `fn new() -> Self` and no `Default` impl
  • Loading branch information
Manishearth committed Mar 9, 2016
2 parents 878041b + 052f598 commit d9b5b2a
Show file tree
Hide file tree
Showing 11 changed files with 302 additions and 49 deletions.
4 changes: 3 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ A collection of lints to catch common mistakes and improve your Rust code.
[Jump to usage instructions](#usage)

##Lints
There are 131 lints included in this crate:
There are 133 lints included in this crate:

name | default | meaning
---------------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Expand Down Expand Up @@ -83,6 +83,7 @@ name
[needless_return](https://github.com/Manishearth/rust-clippy/wiki#needless_return) | warn | using a return statement like `return expr;` where an expression would suffice
[needless_update](https://github.com/Manishearth/rust-clippy/wiki#needless_update) | warn | using `{ ..base }` when there are no missing fields
[new_ret_no_self](https://github.com/Manishearth/rust-clippy/wiki#new_ret_no_self) | warn | not returning `Self` in a `new` method
[new_without_default](https://github.com/Manishearth/rust-clippy/wiki#new_without_default) | warn | `fn new() -> Self` method without `Default` implementation
[no_effect](https://github.com/Manishearth/rust-clippy/wiki#no_effect) | warn | statements with no effect
[non_ascii_literal](https://github.com/Manishearth/rust-clippy/wiki#non_ascii_literal) | allow | using any literal non-ASCII chars in a string literal; suggests using the \\u escape instead
[nonsensical_open_options](https://github.com/Manishearth/rust-clippy/wiki#nonsensical_open_options) | warn | nonsensical combination of options for opening a file
Expand Down Expand Up @@ -131,6 +132,7 @@ name
[unstable_as_mut_slice](https://github.com/Manishearth/rust-clippy/wiki#unstable_as_mut_slice) | warn | as_mut_slice is not stable and can be replaced by &mut v[..]see https://github.com/rust-lang/rust/issues/27729
[unstable_as_slice](https://github.com/Manishearth/rust-clippy/wiki#unstable_as_slice) | warn | as_slice is not stable and can be replaced by & v[..]see https://github.com/rust-lang/rust/issues/27729
[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
[unused_label](https://github.com/Manishearth/rust-clippy/wiki#unused_label) | warn | unused label
[unused_lifetimes](https://github.com/Manishearth/rust-clippy/wiki#unused_lifetimes) | warn | unused lifetimes in function definitions
[use_debug](https://github.com/Manishearth/rust-clippy/wiki#use_debug) | allow | use `Debug`-based formatting
[used_underscore_binding](https://github.com/Manishearth/rust-clippy/wiki#used_underscore_binding) | warn | using a binding which is prefixed with an underscore
Expand Down
6 changes: 6 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ pub mod mutex_atomic;
pub mod needless_bool;
pub mod needless_features;
pub mod needless_update;
pub mod new_without_default;
pub mod no_effect;
pub mod open_options;
pub mod overflow_check_conditional;
Expand All @@ -94,6 +95,7 @@ pub mod temporary_assignment;
pub mod transmute;
pub mod types;
pub mod unicode;
pub mod unused_label;
pub mod vec;
pub mod zero_div_zero;
// end lints modules, do not remove this comment, it’s used in `update_lints`
Expand Down Expand Up @@ -175,6 +177,8 @@ pub fn plugin_registrar(reg: &mut Registry) {
reg.register_late_lint_pass(box swap::Swap);
reg.register_early_lint_pass(box if_not_else::IfNotElse);
reg.register_late_lint_pass(box overflow_check_conditional::OverflowCheckConditional);
reg.register_late_lint_pass(box unused_label::UnusedLabel);
reg.register_late_lint_pass(box new_without_default::NewWithoutDefault);

reg.register_lint_group("clippy_pedantic", vec![
enum_glob_use::ENUM_GLOB_USE,
Expand Down Expand Up @@ -283,6 +287,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
needless_features::UNSTABLE_AS_MUT_SLICE,
needless_features::UNSTABLE_AS_SLICE,
needless_update::NEEDLESS_UPDATE,
new_without_default::NEW_WITHOUT_DEFAULT,
no_effect::NO_EFFECT,
open_options::NONSENSICAL_OPEN_OPTIONS,
overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL,
Expand All @@ -309,6 +314,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
types::TYPE_COMPLEXITY,
types::UNIT_CMP,
unicode::ZERO_WIDTH_SPACE,
unused_label::UNUSED_LABEL,
vec::USELESS_VEC,
zero_div_zero::ZERO_DIVIDED_BY_ZERO,
]);
Expand Down
34 changes: 10 additions & 24 deletions src/methods.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ use std::{fmt, iter};
use syntax::codemap::Span;
use syntax::ptr::P;
use utils::{get_trait_def_id, implements_trait, in_external_macro, in_macro, match_path, match_trait_method,
match_type, method_chain_args, snippet, snippet_opt, span_lint, span_lint_and_then, span_note_and_lint,
walk_ptrs_ty, walk_ptrs_ty_depth};
match_type, method_chain_args, return_ty, same_tys, snippet, snippet_opt, span_lint,
span_lint_and_then, span_note_and_lint, walk_ptrs_ty, walk_ptrs_ty_depth};
use utils::{BTREEMAP_ENTRY_PATH, DEFAULT_TRAIT_PATH, HASHMAP_ENTRY_PATH, OPTION_PATH, RESULT_PATH, STRING_PATH,
VEC_PATH};
use utils::MethodArgs;
Expand Down Expand Up @@ -431,26 +431,12 @@ impl LateLintPass for MethodsPass {
}
}

if &name.as_str() == &"new" {
let returns_self = if let FunctionRetTy::Return(ref ret_ty) = sig.decl.output {
let ast_ty_to_ty_cache = cx.tcx.ast_ty_to_ty_cache.borrow();
let ret_ty = ast_ty_to_ty_cache.get(&ret_ty.id);

if let Some(&ret_ty) = ret_ty {
ret_ty.walk().any(|t| t == ty)
} else {
false
}
} else {
false
};

if !returns_self {
span_lint(cx,
NEW_RET_NO_SELF,
sig.explicit_self.span,
"methods called `new` usually return `Self`");
}
let ret_ty = return_ty(cx.tcx.node_id_to_type(implitem.id));
if &name.as_str() == &"new" && !ret_ty.map_or(false, |ret_ty| ret_ty.walk().any(|t| same_tys(cx, t, ty))) {
span_lint(cx,
NEW_RET_NO_SELF,
sig.explicit_self.span,
"methods called `new` usually return `Self`");
}
}
}
Expand Down Expand Up @@ -485,7 +471,7 @@ fn lint_or_fun_call(cx: &LateContext, expr: &Expr, name: &str, args: &[P<Expr>])
return false;
};

if implements_trait(cx, arg_ty, default_trait_id, None) {
if implements_trait(cx, arg_ty, default_trait_id, Vec::new()) {
span_lint(cx,
OR_FUN_CALL,
span,
Expand Down Expand Up @@ -869,7 +855,7 @@ fn get_error_type<'a>(cx: &LateContext, ty: ty::Ty<'a>) -> Option<ty::Ty<'a>> {
/// This checks whether a given type is known to implement Debug.
fn has_debug_impl<'a, 'b>(ty: ty::Ty<'a>, cx: &LateContext<'b, 'a>) -> bool {
match cx.tcx.lang_items.debug_trait() {
Some(debug) => implements_trait(cx, ty, debug, Some(vec![])),
Some(debug) => implements_trait(cx, ty, debug, Vec::new()),
None => false,
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/misc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ fn check_to_owned(cx: &LateContext, expr: &Expr, other: &Expr, left: bool, op: S
None => return,
};

if !implements_trait(cx, arg_ty, partial_eq_trait_id, Some(vec![other_ty])) {
if !implements_trait(cx, arg_ty, partial_eq_trait_id, vec![other_ty]) {
return;
}

Expand Down
65 changes: 65 additions & 0 deletions src/new_without_default.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
use rustc::lint::*;
use rustc_front::hir;
use rustc_front::intravisit::FnKind;
use syntax::ast;
use syntax::codemap::Span;
use utils::{get_trait_def_id, implements_trait, in_external_macro, return_ty, same_tys, span_lint,
DEFAULT_TRAIT_PATH};

/// **What it does:** This lints about type with a `fn new() -> Self` method and no `Default`
/// implementation.
///
/// **Why is this bad?** User might expect to be able to use `Default` is the type can be
/// constructed without arguments.
///
/// **Known problems:** Hopefully none.
///
/// **Example:**
///
/// ```rust,ignore
/// struct Foo;
///
/// impl Foo {
/// fn new() -> Self {
/// Foo
/// }
/// }
/// ```
declare_lint! {
pub NEW_WITHOUT_DEFAULT,
Warn,
"`fn new() -> Self` method without `Default` implementation"
}

#[derive(Copy,Clone)]
pub struct NewWithoutDefault;

impl LintPass for NewWithoutDefault {
fn get_lints(&self) -> LintArray {
lint_array!(NEW_WITHOUT_DEFAULT)
}
}

impl LateLintPass for NewWithoutDefault {
fn check_fn(&mut self, cx: &LateContext, kind: FnKind, decl: &hir::FnDecl, _: &hir::Block, span: Span, id: ast::NodeId) {
if in_external_macro(cx, span) {
return;
}

if let FnKind::Method(name, _, _) = kind {
if decl.inputs.is_empty() && name.as_str() == "new" {
let self_ty = cx.tcx.lookup_item_type(cx.tcx.map.local_def_id(cx.tcx.map.get_parent(id))).ty;

if_let_chain!{[
let Some(ret_ty) = return_ty(cx.tcx.node_id_to_type(id)),
same_tys(cx, self_ty, ret_ty),
let Some(default_trait_id) = get_trait_def_id(cx, &DEFAULT_TRAIT_PATH),
!implements_trait(cx, self_ty, default_trait_id, Vec::new())
], {
span_lint(cx, NEW_WITHOUT_DEFAULT, span,
&format!("you should consider adding a `Default` implementation for `{}`", self_ty));
}}
}
}
}
}
41 changes: 21 additions & 20 deletions src/ptr_arg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use rustc::front::map::NodeItem;
use rustc::lint::*;
use rustc::middle::ty;
use rustc_front::hir::*;
use syntax::ast::NodeId;
use utils::{STRING_PATH, VEC_PATH};
use utils::{span_lint, match_type};

Expand Down Expand Up @@ -35,7 +36,7 @@ impl LintPass for PtrArg {
impl LateLintPass for PtrArg {
fn check_item(&mut self, cx: &LateContext, item: &Item) {
if let ItemFn(ref decl, _, _, _, _, _) = item.node {
check_fn(cx, decl);
check_fn(cx, decl, item.id);
}
}

Expand All @@ -46,34 +47,34 @@ impl LateLintPass for PtrArg {
return; // ignore trait impls
}
}
check_fn(cx, &sig.decl);
check_fn(cx, &sig.decl, item.id);
}
}

fn check_trait_item(&mut self, cx: &LateContext, item: &TraitItem) {
if let MethodTraitItem(ref sig, _) = item.node {
check_fn(cx, &sig.decl);
check_fn(cx, &sig.decl, item.id);
}
}
}

fn check_fn(cx: &LateContext, decl: &FnDecl) {
for arg in &decl.inputs {
if let Some(ty) = cx.tcx.ast_ty_to_ty_cache.borrow().get(&arg.ty.id) {
if let ty::TyRef(_, ty::TypeAndMut { ty, mutbl: MutImmutable }) = ty.sty {
if match_type(cx, ty, &VEC_PATH) {
span_lint(cx,
PTR_ARG,
arg.ty.span,
"writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used \
with non-Vec-based slices. Consider changing the type to `&[...]`");
} else if match_type(cx, ty, &STRING_PATH) {
span_lint(cx,
PTR_ARG,
arg.ty.span,
"writing `&String` instead of `&str` involves a new object where a slice will do. \
Consider changing the type to `&str`");
}
fn check_fn(cx: &LateContext, decl: &FnDecl, fn_id: NodeId) {
let fn_ty = cx.tcx.node_id_to_type(fn_id).fn_sig().skip_binder();

for (arg, ty) in decl.inputs.iter().zip(&fn_ty.inputs) {
if let ty::TyRef(_, ty::TypeAndMut { ty, mutbl: MutImmutable }) = ty.sty {
if match_type(cx, ty, &VEC_PATH) {
span_lint(cx,
PTR_ARG,
arg.ty.span,
"writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used \
with non-Vec-based slices. Consider changing the type to `&[...]`");
} else if match_type(cx, ty, &STRING_PATH) {
span_lint(cx,
PTR_ARG,
arg.ty.span,
"writing `&String` instead of `&str` involves a new object where a slice will do. \
Consider changing the type to `&str`");
}
}
}
Expand Down
78 changes: 78 additions & 0 deletions src/unused_label.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
use rustc::lint::*;
use rustc_front::hir;
use rustc_front::intravisit::{FnKind, Visitor, walk_expr, walk_fn};
use std::collections::HashMap;
use syntax::ast;
use syntax::codemap::Span;
use syntax::parse::token::InternedString;
use utils::{in_macro, span_lint};

/// **What it does:** This lint checks for unused labels.
///
/// **Why is this bad?** Maybe the label should be used in which case there is an error in the
/// code or it should be removed.
///
/// **Known problems:** Hopefully none.
///
/// **Example:**
/// ```rust,ignore
/// fn unused_label() {
/// 'label: for i in 1..2 {
/// if i > 4 { continue }
/// }
/// ```
declare_lint! {
pub UNUSED_LABEL,
Warn,
"unused label"
}

pub struct UnusedLabel;

#[derive(Default)]
struct UnusedLabelVisitor {
labels: HashMap<InternedString, Span>,
}

impl UnusedLabelVisitor {
pub fn new() -> UnusedLabelVisitor {
::std::default::Default::default()
}
}

impl LintPass for UnusedLabel {
fn get_lints(&self) -> LintArray {
lint_array!(UNUSED_LABEL)
}
}

impl LateLintPass for UnusedLabel {
fn check_fn(&mut self, cx: &LateContext, kind: FnKind, decl: &hir::FnDecl, body: &hir::Block, span: Span, _: ast::NodeId) {
if in_macro(cx, span) {
return;
}

let mut v = UnusedLabelVisitor::new();
walk_fn(&mut v, kind, decl, body, span);

for (label, span) in v.labels {
span_lint(cx, UNUSED_LABEL, span, &format!("unused label `{}`", label));
}
}
}

impl<'v> Visitor<'v> for UnusedLabelVisitor {
fn visit_expr(&mut self, expr: &hir::Expr) {
match expr.node {
hir::ExprBreak(Some(label)) | hir::ExprAgain(Some(label)) => {
self.labels.remove(&label.node.name.as_str());
}
hir::ExprLoop(_, Some(label)) | hir::ExprWhile(_, _, Some(label)) => {
self.labels.insert(label.name.as_str(), expr.span);
}
_ => (),
}

walk_expr(self, expr);
}
}
21 changes: 19 additions & 2 deletions src/utils/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ pub fn get_trait_def_id(cx: &LateContext, path: &[&str]) -> Option<DefId> {
/// Check whether a type implements a trait.
/// See also `get_trait_def_id`.
pub fn implements_trait<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ty: ty::Ty<'tcx>, trait_id: DefId,
ty_params: Option<Vec<ty::Ty<'tcx>>>)
ty_params: Vec<ty::Ty<'tcx>>)
-> bool {
cx.tcx.populate_implementations_for_trait_if_necessary(trait_id);

Expand All @@ -274,7 +274,7 @@ pub fn implements_trait<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ty: ty::Ty<'tcx>,
trait_id,
0,
ty,
ty_params.unwrap_or_default());
ty_params);

traits::SelectionContext::new(&infcx).evaluate_obligation_conservatively(&obligation)
}
Expand Down Expand Up @@ -731,3 +731,20 @@ pub fn unsugar_range(expr: &Expr) -> Option<UnsugaredRange> {
None
}
}

/// Convenience function to get the return type of a function or `None` if the function diverges.
pub fn return_ty(fun: ty::Ty) -> Option<ty::Ty> {
if let ty::FnConverging(ret_ty) = fun.fn_sig().skip_binder().output {
Some(ret_ty)
} else {
None
}
}

/// Check if two types are the same.
// FIXME: this works correctly for lifetimes bounds (`for <'a> Foo<'a>` == `for <'b> Foo<'b>` but
// not for type parameters.
pub fn same_tys<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, a: ty::Ty<'tcx>, b: ty::Ty<'tcx>) -> bool {
let infcx = infer::new_infer_ctxt(cx.tcx, &cx.tcx.tables, None);
infcx.can_equate(&cx.tcx.erase_regions(&a), &cx.tcx.erase_regions(&b)).is_ok()
}
Loading

0 comments on commit d9b5b2a

Please sign in to comment.