Mirrors/rust-clippy
2
0
Fork 0
mirror of https://github.com/rust-lang/rust-clippy synced 2024-11-11 07:34:18 +00:00
Code Issues Projects Releases Packages Wiki Activity

Lint on underscore variable assignment

Browse source 
Fix tests after no_effect update

Add a drop testcase

Don't lint _ variables in macro expansion

Address review comments and update tests

Don't shadow unnecessary operation lint if no_effect is allowed

Revert shadowing change and remove no_effect allows

Update clippy_lints/src/no_effect.rs

Co-authored-by: Takayuki Nakata <f.seasons017@gmail.com>

Update clippy_lints/src/no_effect.rs

Co-authored-by: Takayuki Nakata <f.seasons017@gmail.com>

Address review comments
This commit is contained in:
F3real 2021-10-04 23:49:53 +02:00
parent 4996e17b14
commit 6b22bba902
11 changed files with 170 additions and 86 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
CHANGELOG.md
View file

@ -2899,6 +2899,7 @@ Released 2018-09-13
[`new_ret_no_self`]: https://rust-lang.github.io/rust-clippy/master/index.html#new_ret_no_self
[`new_without_default`]: https://rust-lang.github.io/rust-clippy/master/index.html#new_without_default
[`no_effect`]: https://rust-lang.github.io/rust-clippy/master/index.html#no_effect
[`no_effect_underscore_binding`]: https://rust-lang.github.io/rust-clippy/master/index.html#no_effect_underscore_binding
[`non_ascii_literal`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_ascii_literal
[`non_octal_unix_permissions`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_octal_unix_permissions
[`non_send_fields_in_send_ty`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_send_fields_in_send_ty

1
clippy_lints/src/lib.register_lints.rs
View file

@ -362,6 +362,7 @@ store.register_lints(&[
neg_multiply::NEG_MULTIPLY,
new_without_default::NEW_WITHOUT_DEFAULT,
no_effect::NO_EFFECT,
no_effect::NO_EFFECT_UNDERSCORE_BINDING,
no_effect::UNNECESSARY_OPERATION,
non_copy_const::BORROW_INTERIOR_MUTABLE_CONST,
non_copy_const::DECLARE_INTERIOR_MUTABLE_CONST,

1
clippy_lints/src/lib.register_pedantic.rs
View file

@ -72,6 +72,7 @@ store.register_group(true, "clippy::pedantic", Some("clippy_pedantic"), vec![
LintId::of(needless_continue::NEEDLESS_CONTINUE),
LintId::of(needless_for_each::NEEDLESS_FOR_EACH),
LintId::of(needless_pass_by_value::NEEDLESS_PASS_BY_VALUE),
LintId::of(no_effect::NO_EFFECT_UNDERSCORE_BINDING),
LintId::of(non_expressive_names::MANY_SINGLE_CHAR_NAMES),
LintId::of(non_expressive_names::SIMILAR_NAMES),
LintId::of(pass_by_ref_or_value::LARGE_TYPES_PASSED_BY_VALUE),

179
clippy_lints/src/no_effect.rs
View file

@ -1,9 +1,10 @@
use clippy_utils::diagnostics::{span_lint_hir, span_lint_hir_and_then};
use clippy_utils::is_lint_allowed;
use clippy_utils::source::snippet_opt;
use clippy_utils::ty::has_drop;
use rustc_errors::Applicability;
use rustc_hir::def::{DefKind, Res};
use rustc_hir::{is_range_literal, BinOpKind, BlockCheckMode, Expr, ExprKind, Stmt, StmtKind, UnsafeSource};
use rustc_hir::{is_range_literal, BinOpKind, BlockCheckMode, Expr, ExprKind, PatKind, Stmt, StmtKind, UnsafeSource};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use std::ops::Deref;
@ -13,7 +14,7 @@ declare_clippy_lint! {
/// Checks for statements which have no effect.
///
/// ### Why is this bad?
/// Similar to dead code, these statements are actually
/// Unlike dead code, these statements are actually
/// executed. However, as they have no effect, all they do is make the code less
/// readable.
///
@ -26,6 +27,28 @@ declare_clippy_lint! {
"statements with no effect"
}
declare_clippy_lint! {
/// ### What it does
/// Checks for binding to underscore prefixed variable without side-effects.
///
/// ### Why is this bad?
/// Unlike dead code, these bindings are actually
/// executed. However, as they have no effect and shouldn't be used further on, all they
/// do is make the code less readable.
///
/// ### Known problems
/// Further usage of this variable is not checked, which can lead to false positives if it is
/// used later in the code.
///
/// ### Example
/// ```rust,ignore
/// let _i_serve_no_purpose = 1;
/// ```
pub NO_EFFECT_UNDERSCORE_BINDING,
pedantic,
"binding to `_` prefixed variable with no side-effect"
}
declare_clippy_lint! {
/// ### What it does
/// Checks for expression statements that can be reduced to a
@ -44,6 +67,46 @@ declare_clippy_lint! {
"outer expressions with no effect"
}
declare_lint_pass!(NoEffect => [NO_EFFECT, UNNECESSARY_OPERATION, NO_EFFECT_UNDERSCORE_BINDING]);
impl<'tcx> LateLintPass<'tcx> for NoEffect {
fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) {
if check_no_effect(cx, stmt) {
return;
}
check_unnecessary_operation(cx, stmt);
}
}
fn check_no_effect(cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) -> bool {
if let StmtKind::Semi(expr) = stmt.kind {
if has_no_effect(cx, expr) {
span_lint_hir(cx, NO_EFFECT, expr.hir_id, stmt.span, "statement with no effect");
return true;
}
} else if let StmtKind::Local(local) = stmt.kind {
if_chain! {
if !is_lint_allowed(cx, NO_EFFECT_UNDERSCORE_BINDING, local.hir_id);
if let Some(init) = local.init;
if !local.pat.span.from_expansion();
if has_no_effect(cx, init);
if let PatKind::Binding(_, _, ident, _) = local.pat.kind;
if ident.name.to_ident_string().starts_with('_');
then {
span_lint_hir(
cx,
NO_EFFECT_UNDERSCORE_BINDING,
init.hir_id,
stmt.span,
"binding to `_` prefixed variable with no side-effect"
);
return true;
}
}
}
false
}
fn has_no_effect(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
if expr.span.from_expansion() {
return false;
@ -88,71 +151,59 @@ fn has_no_effect(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
}
}
declare_lint_pass!(NoEffect => [NO_EFFECT, UNNECESSARY_OPERATION]);
impl<'tcx> LateLintPass<'tcx> for NoEffect {
fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) {
if let StmtKind::Semi(expr) = stmt.kind {
if has_no_effect(cx, expr) {
span_lint_hir(cx, NO_EFFECT, expr.hir_id, stmt.span, "statement with no effect");
} else if let Some(reduced) = reduce_expression(cx, expr) {
for e in &reduced {
if e.span.from_expansion() {
fn check_unnecessary_operation(cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) {
if_chain! {
if let StmtKind::Semi(expr) = stmt.kind;
if let Some(reduced) = reduce_expression(cx, expr);
if !&reduced.iter().any(|e| e.span.from_expansion());
then {
if let ExprKind::Index(..) = &expr.kind {
let snippet;
if let (Some(arr), Some(func)) = (snippet_opt(cx, reduced[0].span), snippet_opt(cx, reduced[1].span)) {
snippet = format!("assert!({}.len() > {});", &arr, &func);
} else {
return;
}
span_lint_hir_and_then(
cx,
UNNECESSARY_OPERATION,
expr.hir_id,
stmt.span,
"unnecessary operation",
|diag| {
diag.span_suggestion(
stmt.span,
"statement can be written as",
snippet,
Applicability::MaybeIncorrect,
);
},
);
} else {
let mut snippet = String::new();
for e in reduced {
if let Some(snip) = snippet_opt(cx, e.span) {
snippet.push_str(&snip);
snippet.push(';');
} else {
return;
}
}
if let ExprKind::Index(..) = &expr.kind {
let snippet;
if_chain! {
if let Some(arr) = snippet_opt(cx, reduced[0].span);
if let Some(func) = snippet_opt(cx, reduced[1].span);
then {
snippet = format!("assert!({}.len() > {});", &arr, &func);
} else {
return;
}
}
span_lint_hir_and_then(
cx,
UNNECESSARY_OPERATION,
expr.hir_id,
stmt.span,
"unnecessary operation",
|diag| {
diag.span_suggestion(
stmt.span,
"statement can be written as",
snippet,
Applicability::MaybeIncorrect,
);
},
);
} else {
let mut snippet = String::new();
for e in reduced {
if let Some(snip) = snippet_opt(cx, e.span) {
snippet.push_str(&snip);
snippet.push(';');
} else {
return;
}
}
span_lint_hir_and_then(
cx,
UNNECESSARY_OPERATION,
expr.hir_id,
stmt.span,
"unnecessary operation",
|diag| {
diag.span_suggestion(
stmt.span,
"statement can be reduced to",
snippet,
Applicability::MachineApplicable,
);
},
);
}
span_lint_hir_and_then(
cx,
UNNECESSARY_OPERATION,
expr.hir_id,
stmt.span,
"unnecessary operation",
|diag| {
diag.span_suggestion(
stmt.span,
"statement can be reduced to",
snippet,
Applicability::MachineApplicable,
);
},
);
}
}
}

2
tests/ui/cfg_attr_rustfmt.fixed
View file

@ -1,7 +1,7 @@
// run-rustfix
#![feature(stmt_expr_attributes)]
#![allow(unused, clippy::no_effect)]
#![allow(unused, clippy::no_effect, clippy::unnecessary_operation)]
#![warn(clippy::deprecated_cfg_attr)]
// This doesn't get linted, see known problems

2
tests/ui/cfg_attr_rustfmt.rs
View file

@ -1,7 +1,7 @@
// run-rustfix
#![feature(stmt_expr_attributes)]
#![allow(unused, clippy::no_effect)]
#![allow(unused, clippy::no_effect, clippy::unnecessary_operation)]
#![warn(clippy::deprecated_cfg_attr)]
// This doesn't get linted, see known problems

8
tests/ui/no_effect.rs
View file

@ -1,5 +1,5 @@
#![feature(box_syntax)]
#![warn(clippy::no_effect)]
#![warn(clippy::no_effect_underscore_binding)]
#![allow(dead_code)]
#![allow(path_statements)]
#![allow(clippy::deref_addrof)]
@ -90,6 +90,10 @@ fn main() {
|| x += 5;
let s: String = "foo".into();
FooString { s: s };
let _unused = 1;
let _penguin = || println!("Some helpful closure");
let _duck = Struct { field: 0 };
let _cat = [2, 4, 6, 8][2];
#[allow(clippy::no_effect)]
0;
@ -97,6 +101,8 @@ fn main() {
// Do not warn
get_number();
unsafe { unsafe_fn() };
let _used = get_struct();
let _x = vec![1];
DropUnit;
DropStruct { field: 0 };
DropTuple(0);

28
tests/ui/no_effect.stderr
View file

@ -156,5 +156,31 @@ error: statement with no effect
LL | FooString { s: s };
| ^^^^^^^^^^^^^^^^^^^
error: aborting due to 26 previous errors
error: binding to `_` prefixed variable with no side-effect
--> $DIR/no_effect.rs:93:5
|
LL | let _unused = 1;
| ^^^^^^^^^^^^^^^^
|
= note: `-D clippy::no-effect-underscore-binding` implied by `-D warnings`
error: binding to `_` prefixed variable with no side-effect
--> $DIR/no_effect.rs:94:5
|
LL | let _penguin = || println!("Some helpful closure");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: binding to `_` prefixed variable with no side-effect
--> $DIR/no_effect.rs:95:5
|
LL | let _duck = Struct { field: 0 };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: binding to `_` prefixed variable with no side-effect
--> $DIR/no_effect.rs:96:5
|
LL | let _cat = [2, 4, 6, 8][2];
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: aborting due to 30 previous errors

3
tests/ui/option_if_let_else.fixed
View file

@ -1,8 +1,7 @@
// edition:2018
// run-rustfix
#![warn(clippy::option_if_let_else)]
#![allow(clippy::redundant_closure)]
#![allow(clippy::ref_option_ref, clippy::equatable_if_let)]
#![allow(clippy::redundant_closure, clippy::ref_option_ref, clippy::equatable_if_let)]
fn bad1(string: Option<&str>) -> (bool, &str) {
string.map_or((false, "hello"), |x| (true, x))

3
tests/ui/option_if_let_else.rs
View file

@ -1,8 +1,7 @@
// edition:2018
// run-rustfix
#![warn(clippy::option_if_let_else)]
#![allow(clippy::redundant_closure)]
#![allow(clippy::ref_option_ref, clippy::equatable_if_let)]
#![allow(clippy::redundant_closure, clippy::ref_option_ref, clippy::equatable_if_let)]
fn bad1(string: Option<&str>) -> (bool, &str) {
if let Some(x) = string {

28
tests/ui/option_if_let_else.stderr
View file

@ -1,5 +1,5 @@
error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:8:5
--> $DIR/option_if_let_else.rs:7:5
|
LL | / if let Some(x) = string {
LL | | (true, x)
@ -11,19 +11,19 @@ LL | | }
= note: `-D clippy::option-if-let-else` implied by `-D warnings`
error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:26:13
--> $DIR/option_if_let_else.rs:25:13
|
LL | let _ = if let Some(s) = *string { s.len() } else { 0 };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `string.map_or(0, |s| s.len())`
error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:27:13
--> $DIR/option_if_let_else.rs:26:13
|
LL | let _ = if let Some(s) = &num { s } else { &0 };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)`
error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:28:13
--> $DIR/option_if_let_else.rs:27:13
|
LL | let _ = if let Some(s) = &mut num {
| _____________^
@ -43,13 +43,13 @@ LL ~ });
|
error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:34:13
--> $DIR/option_if_let_else.rs:33:13
|
LL | let _ = if let Some(ref s) = num { s } else { &0 };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)`
error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:35:13
--> $DIR/option_if_let_else.rs:34:13
|
LL | let _ = if let Some(mut s) = num {
| _____________^
@ -69,7 +69,7 @@ LL ~ });
|
error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:41:13
--> $DIR/option_if_let_else.rs:40:13
|
LL | let _ = if let Some(ref mut s) = num {
| _____________^
@ -89,7 +89,7 @@ LL ~ });
|
error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:50:5
--> $DIR/option_if_let_else.rs:49:5
|
LL | / if let Some(x) = arg {
LL | | let y = x * x;
@ -108,7 +108,7 @@ LL + })
|
error: use Option::map_or_else instead of an if let/else
--> $DIR/option_if_let_else.rs:63:13
--> $DIR/option_if_let_else.rs:62:13
|
LL | let _ = if let Some(x) = arg {
| _____________^
@ -120,7 +120,7 @@ LL | | };
| |_____^ help: try: `arg.map_or_else(|| side_effect(), |x| x)`
error: use Option::map_or_else instead of an if let/else
--> $DIR/option_if_let_else.rs:72:13
--> $DIR/option_if_let_else.rs:71:13
|
LL | let _ = if let Some(x) = arg {
| _____________^
@ -143,13 +143,13 @@ LL ~ }, |x| x * x * x * x);
|
error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:101:13
--> $DIR/option_if_let_else.rs:100:13
|
LL | let _ = if let Some(x) = optional { x + 2 } else { 5 };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `optional.map_or(5, |x| x + 2)`
error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:110:13
--> $DIR/option_if_let_else.rs:109:13
|
LL | let _ = if let Some(x) = Some(0) {
| _____________^
@ -171,13 +171,13 @@ LL ~ });
|
error: use Option::map_or_else instead of an if let/else
--> $DIR/option_if_let_else.rs:138:13
--> $DIR/option_if_let_else.rs:137:13
|
LL | let _ = if let Some(x) = Some(0) { s.len() + x } else { s.len() };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(0).map_or_else(|| s.len(), |x| s.len() + x)`
error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:142:13
--> $DIR/option_if_let_else.rs:141:13
|
LL | let _ = if let Some(x) = Some(0) {
| _____________^