Skip to content
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

Add let_underscore_must_use lint #4823

Merged
merged 2 commits into from
Dec 23, 2019
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -1064,6 +1064,7 @@ Released 2018-09-13
[`len_without_is_empty`]: https://rust-lang.github.io/rust-clippy/master/index.html#len_without_is_empty
[`len_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#len_zero
[`let_and_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_and_return
[`let_underscore_must_use`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_must_use
[`let_unit_value`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_unit_value
[`linkedlist`]: https://rust-lang.github.io/rust-clippy/master/index.html#linkedlist
[`logic_bug`]: https://rust-lang.github.io/rust-clippy/master/index.html#logic_bug
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.

[There are 339 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
[There are 340 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)

We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:

Expand Down
49 changes: 3 additions & 46 deletions clippy_lints/src/functions.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::utils::{
attrs::is_proc_macro, iter_input_pats, match_def_path, qpath_res, return_ty, snippet, snippet_opt,
span_help_and_lint, span_lint, span_lint_and_then, trait_ref_of_method, type_is_unsafe_function,
attrs::is_proc_macro, is_must_use_ty, iter_input_pats, match_def_path, must_use_attr, qpath_res, return_ty,
snippet, snippet_opt, span_help_and_lint, span_lint, span_lint_and_then, trait_ref_of_method,
type_is_unsafe_function,
};
use matches::matches;
use rustc::hir::{self, def::Res, def_id::DefId, intravisit};
Expand Down Expand Up @@ -466,15 +467,6 @@ fn check_must_use_candidate<'a, 'tcx>(
});
}

fn must_use_attr(attrs: &[Attribute]) -> Option<&Attribute> {
attrs.iter().find(|attr| {
attr.ident().map_or(false, |ident| {
let ident: &str = &ident.as_str();
"must_use" == ident
})
})
}

fn returns_unit(decl: &hir::FnDecl) -> bool {
match decl.output {
hir::FunctionRetTy::DefaultReturn(_) => true,
Expand All @@ -486,41 +478,6 @@ fn returns_unit(decl: &hir::FnDecl) -> bool {
}
}

fn is_must_use_ty<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ty: Ty<'tcx>) -> bool {
use ty::TyKind::*;
match ty.kind {
Adt(ref adt, _) => must_use_attr(&cx.tcx.get_attrs(adt.did)).is_some(),
Foreign(ref did) => must_use_attr(&cx.tcx.get_attrs(*did)).is_some(),
Slice(ref ty) | Array(ref ty, _) | RawPtr(ty::TypeAndMut { ref ty, .. }) | Ref(_, ref ty, _) => {
// for the Array case we don't need to care for the len == 0 case
// because we don't want to lint functions returning empty arrays
is_must_use_ty(cx, *ty)
},
Tuple(ref substs) => substs.types().any(|ty| is_must_use_ty(cx, ty)),
Opaque(ref def_id, _) => {
for (predicate, _) in cx.tcx.predicates_of(*def_id).predicates {
if let ty::Predicate::Trait(ref poly_trait_predicate) = predicate {
if must_use_attr(&cx.tcx.get_attrs(poly_trait_predicate.skip_binder().trait_ref.def_id)).is_some() {
return true;
}
}
}
false
},
Dynamic(binder, _) => {
for predicate in binder.skip_binder().iter() {
if let ty::ExistentialPredicate::Trait(ref trait_ref) = predicate {
if must_use_attr(&cx.tcx.get_attrs(trait_ref.def_id)).is_some() {
return true;
}
}
}
false
},
_ => false,
}
}

fn has_mutable_arg(cx: &LateContext<'_, '_>, body: &hir::Body<'_>) -> bool {
let mut tys = FxHashSet::default();
body.params.iter().any(|param| is_mutable_pat(cx, &param.pat, &mut tys))
Expand Down
62 changes: 62 additions & 0 deletions clippy_lints/src/let_underscore.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
use if_chain::if_chain;
use rustc::declare_lint_pass;
use rustc::hir::*;
use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
use rustc_session::declare_tool_lint;

use crate::utils::{is_must_use_func_call, is_must_use_ty, span_help_and_lint};

declare_clippy_lint! {
/// **What it does:** Checks for `let _ = <expr>`
/// where expr is #[must_use]
///
/// **Why is this bad?** It's better to explicitly
/// handle the value of a #[must_use] expr
///
/// **Known problems:** None.
///
/// **Example:**
/// ```rust
/// fn f() -> Result<u32, u32> {
/// Ok(0)
/// }
///
/// let _ = f();
/// // is_ok() is marked #[must_use]
/// let _ = f().is_ok();
/// ```
pub LET_UNDERSCORE_MUST_USE,
restriction,
"non-binding let on a #[must_use] expression"
}

declare_lint_pass!(LetUnderscore => [LET_UNDERSCORE_MUST_USE]);

impl<'a, 'tcx> LateLintPass<'a, 'tcx> for LetUnderscore {
fn check_stmt(&mut self, cx: &LateContext<'_, '_>, stmt: &Stmt) {
if_chain! {
if let StmtKind::Local(ref local) = stmt.kind;
if let PatKind::Wild = local.pat.kind;
if let Some(ref init) = local.init;
then {
if is_must_use_ty(cx, cx.tables.expr_ty(init)) {
span_help_and_lint(
cx,
LET_UNDERSCORE_MUST_USE,
stmt.span,
"non-binding let on an expression with #[must_use] type",
"consider explicitly using expression value"
)
} else if is_must_use_func_call(cx, init) {
span_help_and_lint(
cx,
LET_UNDERSCORE_MUST_USE,
stmt.span,
"non-binding let on a result of a #[must_use] function",
"consider explicitly using function result"
)
}
}
}
}
}
4 changes: 4 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ pub mod large_enum_variant;
pub mod large_stack_arrays;
pub mod len_zero;
pub mod let_if_seq;
pub mod let_underscore;
pub mod lifetimes;
pub mod literal_representation;
pub mod loops;
Expand Down Expand Up @@ -555,6 +556,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
&len_zero::LEN_WITHOUT_IS_EMPTY,
&len_zero::LEN_ZERO,
&let_if_seq::USELESS_LET_IF_SEQ,
&let_underscore::LET_UNDERSCORE_MUST_USE,
&lifetimes::EXTRA_UNUSED_LIFETIMES,
&lifetimes::NEEDLESS_LIFETIMES,
&literal_representation::DECIMAL_LITERAL_REPRESENTATION,
Expand Down Expand Up @@ -970,6 +972,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
store.register_late_pass(move || box large_stack_arrays::LargeStackArrays::new(array_size_threshold));
store.register_early_pass(|| box as_conversions::AsConversions);
store.register_early_pass(|| box utils::internal_lints::ProduceIce);
store.register_late_pass(|| box let_underscore::LetUnderscore);

store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
LintId::of(&arithmetic::FLOAT_ARITHMETIC),
Expand All @@ -982,6 +985,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
LintId::of(&indexing_slicing::INDEXING_SLICING),
LintId::of(&inherent_impl::MULTIPLE_INHERENT_IMPL),
LintId::of(&integer_division::INTEGER_DIVISION),
LintId::of(&let_underscore::LET_UNDERSCORE_MUST_USE),
LintId::of(&literal_representation::DECIMAL_LITERAL_REPRESENTATION),
LintId::of(&matches::WILDCARD_ENUM_MATCH_ARM),
LintId::of(&mem_forget::MEM_FORGET),
Expand Down
70 changes: 69 additions & 1 deletion clippy_lints/src/utils/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ use rustc::ty::{
};
use rustc_errors::Applicability;
use smallvec::SmallVec;
use syntax::ast::{self, LitKind};
use syntax::ast::{self, Attribute, LitKind};
use syntax::attr;
use syntax::source_map::{Span, DUMMY_SP};
use syntax::symbol::{kw, Symbol};
Expand Down Expand Up @@ -1237,3 +1237,71 @@ pub fn parent_node_is_if_expr<'a, 'b>(expr: &Expr, cx: &LateContext<'a, 'b>) ->
_ => false,
}
}

pub fn must_use_attr(attrs: &[Attribute]) -> Option<&Attribute> {
attrs.iter().find(|attr| {
attr.ident().map_or(false, |ident| {
let ident: &str = &ident.as_str();
"must_use" == ident
})
})
}

// Returns whether the type has #[must_use] attribute
pub fn is_must_use_ty<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ty: Ty<'tcx>) -> bool {
use ty::TyKind::*;
match ty.kind {
Adt(ref adt, _) => must_use_attr(&cx.tcx.get_attrs(adt.did)).is_some(),
Foreign(ref did) => must_use_attr(&cx.tcx.get_attrs(*did)).is_some(),
Slice(ref ty) | Array(ref ty, _) | RawPtr(ty::TypeAndMut { ref ty, .. }) | Ref(_, ref ty, _) => {
// for the Array case we don't need to care for the len == 0 case
// because we don't want to lint functions returning empty arrays
is_must_use_ty(cx, *ty)
},
Tuple(ref substs) => substs.types().any(|ty| is_must_use_ty(cx, ty)),
Opaque(ref def_id, _) => {
for (predicate, _) in cx.tcx.predicates_of(*def_id).predicates {
if let ty::Predicate::Trait(ref poly_trait_predicate) = predicate {
if must_use_attr(&cx.tcx.get_attrs(poly_trait_predicate.skip_binder().trait_ref.def_id)).is_some() {
return true;
}
}
}
false
},
Dynamic(binder, _) => {
for predicate in binder.skip_binder().iter() {
if let ty::ExistentialPredicate::Trait(ref trait_ref) = predicate {
if must_use_attr(&cx.tcx.get_attrs(trait_ref.def_id)).is_some() {
return true;
}
}
}
false
},
_ => false,
}
}

// check if expr is calling method or function with #[must_use] attribyte
pub fn is_must_use_func_call(cx: &LateContext<'_, '_>, expr: &Expr) -> bool {
let did = match expr.kind {
ExprKind::Call(ref path, _) => if_chain! {
if let ExprKind::Path(ref qpath) = path.kind;
if let def::Res::Def(_, did) = cx.tables.qpath_res(qpath, path.hir_id);
then {
Some(did)
} else {
None
}
},
ExprKind::MethodCall(_, _, _) => cx.tables.type_dependent_def_id(expr.hir_id),
_ => None,
};

if let Some(did) = did {
must_use_attr(&cx.tcx.get_attrs(did)).is_some()
} else {
false
}
}
9 changes: 8 additions & 1 deletion src/lintlist/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ pub use lint::Lint;
pub use lint::LINT_LEVELS;

// begin lint list, do not remove this comment, it’s used in `update_lints`
pub const ALL_LINTS: [Lint; 339] = [
pub const ALL_LINTS: [Lint; 340] = [
Lint {
name: "absurd_extreme_comparisons",
group: "correctness",
Expand Down Expand Up @@ -938,6 +938,13 @@ pub const ALL_LINTS: [Lint; 339] = [
deprecation: None,
module: "returns",
},
Lint {
name: "let_underscore_must_use",
group: "restriction",
desc: "non-binding let on a #[must_use] expression",
deprecation: None,
module: "let_underscore",
},
Lint {
name: "let_unit_value",
group: "style",
Expand Down
84 changes: 84 additions & 0 deletions tests/ui/let_underscore.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
#![warn(clippy::let_underscore_must_use)]

#[must_use]
fn f() -> u32 {
0
}

fn g() -> Result<u32, u32> {
Ok(0)
}

#[must_use]
fn l<T>(x: T) -> T {
x
}

fn h() -> u32 {
0
}

struct S {}

impl S {
#[must_use]
pub fn f(&self) -> u32 {
0
}

pub fn g(&self) -> Result<u32, u32> {
Ok(0)
}

fn k(&self) -> u32 {
0
}

#[must_use]
fn h() -> u32 {
0
}

fn p() -> Result<u32, u32> {
Ok(0)
}
}

trait Trait {
#[must_use]
fn a() -> u32;
}

impl Trait for S {
fn a() -> u32 {
0
}
}

fn main() {
let _ = f();
Copy link
Member

Choose a reason for hiding this comment

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

Please add a test for a function, that is not must_use and doesn't have a must_use type.

let _ = g();
let _ = h();
let _ = l(0_u32);

let s = S {};

let _ = s.f();
let _ = s.g();
let _ = s.k();

let _ = S::h();
let _ = S::p();

let _ = S::a();

let _ = if true { Ok(()) } else { Err(()) };

let a = Result::<(), ()>::Ok(());

let _ = a.is_ok();

let _ = a.map(|_| ());

let _ = a;
}
Loading