Skip to content

Lint unused labels and types with fn new() -> Self and no Default impl #730

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 9, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might be a candidate for an if_let_chain!

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);
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason ExprBreak and ExprAgain have Option<Spanned<Ident>> but ExprLoop and ExprWhile have Option<Ident> so the lint will span on the entire loop. Do you think a PR in rust to “fix” that is worth it and would be accepted?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure, really.

_ => (),
}

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