Skip to content

Add a function to check whether binary oprands are nontrivial #11907

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 5 commits into from
Dec 8, 2023
Merged
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
34 changes: 30 additions & 4 deletions clippy_lints/src/no_effect.rs
Original file line number Diff line number Diff line change
@@ -87,6 +87,17 @@ impl<'tcx> LateLintPass<'tcx> for NoEffect {

fn check_no_effect(cx: &LateContext<'_>, stmt: &Stmt<'_>) -> bool {
if let StmtKind::Semi(expr) = stmt.kind {
// move `expr.span.from_expansion()` ahead
if expr.span.from_expansion() {
return false;
}
let expr = peel_blocks(expr);

if is_operator_overridden(cx, expr) {
// Return `true`, to prevent `check_unnecessary_operation` from
// linting on this statement as well.
return true;
}
if has_no_effect(cx, expr) {
span_lint_hir_and_then(
cx,
@@ -153,11 +164,26 @@ fn check_no_effect(cx: &LateContext<'_>, stmt: &Stmt<'_>) -> bool {
false
}

fn has_no_effect(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
if expr.span.from_expansion() {
return false;
fn is_operator_overridden(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
// It's very hard or impossable to check whether overridden operator have side-effect this lint.
// So, this function assume user-defined operator is overridden with an side-effect.
// The definition of user-defined structure here is ADT-type,
// Althrough this will weaken the ability of this lint, less error lint-fix happen.
match expr.kind {
ExprKind::Binary(..) | ExprKind::Unary(..) => {
// No need to check type of `lhs` and `rhs`
// because if the operator is overridden, at least one operand is ADT type

// reference: rust/compiler/rustc_middle/src/ty/typeck_results.rs: `is_method_call`.
// use this function to check whether operator is overridden in `ExprKind::{Binary, Unary}`.
cx.typeck_results().is_method_call(expr)
},
_ => false,
}
match peel_blocks(expr).kind {
}

fn has_no_effect(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
match expr.kind {
ExprKind::Lit(..) | ExprKind::Closure { .. } => true,
ExprKind::Path(..) => !has_drop(cx, cx.typeck_results().expr_ty(expr)),
ExprKind::Index(a, b, _) | ExprKind::Binary(_, a, b) => has_no_effect(cx, a) && has_no_effect(cx, b),
31 changes: 31 additions & 0 deletions tests/ui/no_effect.rs
Original file line number Diff line number Diff line change
@@ -9,6 +9,30 @@
clippy::useless_vec
)]

use std::fmt::Display;
use std::ops::{Neg, Shl};

struct Cout;

impl<T> Shl<T> for Cout
where
T: Display,
{
type Output = Self;
fn shl(self, rhs: T) -> Self::Output {
println!("{}", rhs);
self
}
}

impl Neg for Cout {
type Output = Self;
fn neg(self) -> Self::Output {
println!("hello world");
self
}
}

struct Unit;
struct Tuple(i32);
struct Struct {
@@ -174,4 +198,11 @@ fn main() {
GreetStruct1("world");
GreetStruct2()("world");
GreetStruct3 {}("world");

fn n() -> i32 {
42
}

Cout << 142;
-Cout;
}
58 changes: 29 additions & 29 deletions tests/ui/no_effect.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: statement with no effect
--> $DIR/no_effect.rs:98:5
--> $DIR/no_effect.rs:122:5
|
LL | 0;
| ^^
@@ -8,151 +8,151 @@ LL | 0;
= help: to override `-D warnings` add `#[allow(clippy::no_effect)]`

error: statement with no effect
--> $DIR/no_effect.rs:101:5
--> $DIR/no_effect.rs:125:5
|
LL | s2;
| ^^^

error: statement with no effect
--> $DIR/no_effect.rs:103:5
--> $DIR/no_effect.rs:127:5
|
LL | Unit;
| ^^^^^

error: statement with no effect
--> $DIR/no_effect.rs:105:5
--> $DIR/no_effect.rs:129:5
|
LL | Tuple(0);
| ^^^^^^^^^

error: statement with no effect
--> $DIR/no_effect.rs:107:5
--> $DIR/no_effect.rs:131:5
|
LL | Struct { field: 0 };
| ^^^^^^^^^^^^^^^^^^^^

error: statement with no effect
--> $DIR/no_effect.rs:109:5
--> $DIR/no_effect.rs:133:5
|
LL | Struct { ..s };
| ^^^^^^^^^^^^^^^

error: statement with no effect
--> $DIR/no_effect.rs:111:5
--> $DIR/no_effect.rs:135:5
|
LL | Union { a: 0 };
| ^^^^^^^^^^^^^^^

error: statement with no effect
--> $DIR/no_effect.rs:113:5
--> $DIR/no_effect.rs:137:5
|
LL | Enum::Tuple(0);
| ^^^^^^^^^^^^^^^

error: statement with no effect
--> $DIR/no_effect.rs:115:5
--> $DIR/no_effect.rs:139:5
|
LL | Enum::Struct { field: 0 };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^

error: statement with no effect
--> $DIR/no_effect.rs:117:5
--> $DIR/no_effect.rs:141:5
|
LL | 5 + 6;
| ^^^^^^

error: statement with no effect
--> $DIR/no_effect.rs:119:5
--> $DIR/no_effect.rs:143:5
|
LL | *&42;
| ^^^^^

error: statement with no effect
--> $DIR/no_effect.rs:121:5
--> $DIR/no_effect.rs:145:5
|
LL | &6;
| ^^^

error: statement with no effect
--> $DIR/no_effect.rs:123:5
--> $DIR/no_effect.rs:147:5
|
LL | (5, 6, 7);
| ^^^^^^^^^^

error: statement with no effect
--> $DIR/no_effect.rs:125:5
--> $DIR/no_effect.rs:149:5
|
LL | ..;
| ^^^

error: statement with no effect
--> $DIR/no_effect.rs:127:5
--> $DIR/no_effect.rs:151:5
|
LL | 5..;
| ^^^^

error: statement with no effect
--> $DIR/no_effect.rs:129:5
--> $DIR/no_effect.rs:153:5
|
LL | ..5;
| ^^^^

error: statement with no effect
--> $DIR/no_effect.rs:131:5
--> $DIR/no_effect.rs:155:5
|
LL | 5..6;
| ^^^^^

error: statement with no effect
--> $DIR/no_effect.rs:133:5
--> $DIR/no_effect.rs:157:5
|
LL | 5..=6;
| ^^^^^^

error: statement with no effect
--> $DIR/no_effect.rs:135:5
--> $DIR/no_effect.rs:159:5
|
LL | [42, 55];
| ^^^^^^^^^

error: statement with no effect
--> $DIR/no_effect.rs:137:5
--> $DIR/no_effect.rs:161:5
|
LL | [42, 55][1];
| ^^^^^^^^^^^^

error: statement with no effect
--> $DIR/no_effect.rs:139:5
--> $DIR/no_effect.rs:163:5
|
LL | (42, 55).1;
| ^^^^^^^^^^^

error: statement with no effect
--> $DIR/no_effect.rs:141:5
--> $DIR/no_effect.rs:165:5
|
LL | [42; 55];
| ^^^^^^^^^

error: statement with no effect
--> $DIR/no_effect.rs:143:5
--> $DIR/no_effect.rs:167:5
|
LL | [42; 55][13];
| ^^^^^^^^^^^^^

error: statement with no effect
--> $DIR/no_effect.rs:146:5
--> $DIR/no_effect.rs:170:5
|
LL | || x += 5;
| ^^^^^^^^^^

error: statement with no effect
--> $DIR/no_effect.rs:149:5
--> $DIR/no_effect.rs:173:5
|
LL | FooString { s: s };
| ^^^^^^^^^^^^^^^^^^^

error: binding to `_` prefixed variable with no side-effect
--> $DIR/no_effect.rs:151:5
--> $DIR/no_effect.rs:175:5
|
LL | let _unused = 1;
| ^^^^^^^^^^^^^^^^
@@ -161,19 +161,19 @@ LL | let _unused = 1;
= help: to override `-D warnings` add `#[allow(clippy::no_effect_underscore_binding)]`

error: binding to `_` prefixed variable with no side-effect
--> $DIR/no_effect.rs:154:5
--> $DIR/no_effect.rs:178:5
|
LL | let _penguin = || println!("Some helpful closure");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: binding to `_` prefixed variable with no side-effect
--> $DIR/no_effect.rs:156:5
--> $DIR/no_effect.rs:180:5
|
LL | let _duck = Struct { field: 0 };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: binding to `_` prefixed variable with no side-effect
--> $DIR/no_effect.rs:158:5
--> $DIR/no_effect.rs:182:5
|
LL | let _cat = [2, 4, 6, 8][2];
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
19 changes: 19 additions & 0 deletions tests/ui/unnecessary_operation.fixed
Original file line number Diff line number Diff line change
@@ -7,6 +7,9 @@
)]
#![warn(clippy::unnecessary_operation)]

use std::fmt::Display;
use std::ops::Shl;

struct Tuple(i32);
struct Struct {
field: i32,
@@ -50,6 +53,19 @@ fn get_drop_struct() -> DropStruct {
DropStruct { field: 0 }
}

struct Cout;

impl<T> Shl<T> for Cout
where
T: Display,
{
type Output = Self;
fn shl(self, rhs: T) -> Self::Output {
println!("{}", rhs);
self
}
}

fn main() {
get_number();
get_number();
@@ -87,4 +103,7 @@ fn main() {
($($e:expr),*) => {{ $($e;)* }}
}
use_expr!(isize::MIN / -(one() as isize), i8::MIN / -one());

// Issue #11885
Cout << 16;
}
19 changes: 19 additions & 0 deletions tests/ui/unnecessary_operation.rs
Original file line number Diff line number Diff line change
@@ -7,6 +7,9 @@
)]
#![warn(clippy::unnecessary_operation)]

use std::fmt::Display;
use std::ops::Shl;

struct Tuple(i32);
struct Struct {
field: i32,
@@ -50,6 +53,19 @@ fn get_drop_struct() -> DropStruct {
DropStruct { field: 0 }
}

struct Cout;

impl<T> Shl<T> for Cout
where
T: Display,
{
type Output = Self;
fn shl(self, rhs: T) -> Self::Output {
println!("{}", rhs);
self
}
}

fn main() {
Tuple(get_number());
Struct { field: get_number() };
@@ -91,4 +107,7 @@ fn main() {
($($e:expr),*) => {{ $($e;)* }}
}
use_expr!(isize::MIN / -(one() as isize), i8::MIN / -one());

// Issue #11885
Cout << 16;
}
38 changes: 19 additions & 19 deletions tests/ui/unnecessary_operation.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: unnecessary operation
--> $DIR/unnecessary_operation.rs:54:5
--> $DIR/unnecessary_operation.rs:70:5
|
LL | Tuple(get_number());
| ^^^^^^^^^^^^^^^^^^^^ help: statement can be reduced to: `get_number();`
@@ -8,111 +8,111 @@ LL | Tuple(get_number());
= help: to override `-D warnings` add `#[allow(clippy::unnecessary_operation)]`

error: unnecessary operation
--> $DIR/unnecessary_operation.rs:55:5
--> $DIR/unnecessary_operation.rs:71:5
|
LL | Struct { field: get_number() };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: statement can be reduced to: `get_number();`

error: unnecessary operation
--> $DIR/unnecessary_operation.rs:56:5
--> $DIR/unnecessary_operation.rs:72:5
|
LL | Struct { ..get_struct() };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: statement can be reduced to: `get_struct();`

error: unnecessary operation
--> $DIR/unnecessary_operation.rs:57:5
--> $DIR/unnecessary_operation.rs:73:5
|
LL | Enum::Tuple(get_number());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: statement can be reduced to: `get_number();`

error: unnecessary operation
--> $DIR/unnecessary_operation.rs:58:5
--> $DIR/unnecessary_operation.rs:74:5
|
LL | Enum::Struct { field: get_number() };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: statement can be reduced to: `get_number();`

error: unnecessary operation
--> $DIR/unnecessary_operation.rs:59:5
--> $DIR/unnecessary_operation.rs:75:5
|
LL | 5 + get_number();
| ^^^^^^^^^^^^^^^^^ help: statement can be reduced to: `5;get_number();`

error: unnecessary operation
--> $DIR/unnecessary_operation.rs:60:5
--> $DIR/unnecessary_operation.rs:76:5
|
LL | *&get_number();
| ^^^^^^^^^^^^^^^ help: statement can be reduced to: `get_number();`

error: unnecessary operation
--> $DIR/unnecessary_operation.rs:61:5
--> $DIR/unnecessary_operation.rs:77:5
|
LL | &get_number();
| ^^^^^^^^^^^^^^ help: statement can be reduced to: `get_number();`

error: unnecessary operation
--> $DIR/unnecessary_operation.rs:62:5
--> $DIR/unnecessary_operation.rs:78:5
|
LL | (5, 6, get_number());
| ^^^^^^^^^^^^^^^^^^^^^ help: statement can be reduced to: `5;6;get_number();`

error: unnecessary operation
--> $DIR/unnecessary_operation.rs:63:5
--> $DIR/unnecessary_operation.rs:79:5
|
LL | get_number()..;
| ^^^^^^^^^^^^^^^ help: statement can be reduced to: `get_number();`

error: unnecessary operation
--> $DIR/unnecessary_operation.rs:64:5
--> $DIR/unnecessary_operation.rs:80:5
|
LL | ..get_number();
| ^^^^^^^^^^^^^^^ help: statement can be reduced to: `get_number();`

error: unnecessary operation
--> $DIR/unnecessary_operation.rs:65:5
--> $DIR/unnecessary_operation.rs:81:5
|
LL | 5..get_number();
| ^^^^^^^^^^^^^^^^ help: statement can be reduced to: `5;get_number();`

error: unnecessary operation
--> $DIR/unnecessary_operation.rs:66:5
--> $DIR/unnecessary_operation.rs:82:5
|
LL | [42, get_number()];
| ^^^^^^^^^^^^^^^^^^^ help: statement can be reduced to: `42;get_number();`

error: unnecessary operation
--> $DIR/unnecessary_operation.rs:67:5
--> $DIR/unnecessary_operation.rs:83:5
|
LL | [42, 55][get_usize()];
| ^^^^^^^^^^^^^^^^^^^^^^ help: statement can be written as: `assert!([42, 55].len() > get_usize());`

error: unnecessary operation
--> $DIR/unnecessary_operation.rs:68:5
--> $DIR/unnecessary_operation.rs:84:5
|
LL | (42, get_number()).1;
| ^^^^^^^^^^^^^^^^^^^^^ help: statement can be reduced to: `42;get_number();`

error: unnecessary operation
--> $DIR/unnecessary_operation.rs:69:5
--> $DIR/unnecessary_operation.rs:85:5
|
LL | [get_number(); 55];
| ^^^^^^^^^^^^^^^^^^^ help: statement can be reduced to: `get_number();`

error: unnecessary operation
--> $DIR/unnecessary_operation.rs:70:5
--> $DIR/unnecessary_operation.rs:86:5
|
LL | [42; 55][get_usize()];
| ^^^^^^^^^^^^^^^^^^^^^^ help: statement can be written as: `assert!([42; 55].len() > get_usize());`

error: unnecessary operation
--> $DIR/unnecessary_operation.rs:71:5
--> $DIR/unnecessary_operation.rs:87:5
|
LL | / {
LL | | get_number()
LL | | };
| |______^ help: statement can be reduced to: `get_number();`

error: unnecessary operation
--> $DIR/unnecessary_operation.rs:74:5
--> $DIR/unnecessary_operation.rs:90:5
|
LL | / FooString {
LL | | s: String::from("blah"),