Auto merge of #11907 - cocodery:issue11885, r=y21,xFrednet

Add a function to check whether binary oprands are nontrivial fixes [#issue11885](rust-lang/rust-clippy#11885) It's hard to check whether operator is overrided through context of lint. So, assume non-trivial structure like tuple, array or sturt, using a overrided binary operator in this lint, which might cause a side effict. This is not detected before. Althrough this might weaken the ability of this lint, it may more useful than before. Maybe this lint will cause an error, but now, it not. And assuming side effect of non-trivial structure with operator is not a bad thing, right? changelog: Fix: [`no_effect`] check if binary operands are nontrivial
rust-lang · Dec 8, 2023 · 1c8cbe7 · 1c8cbe7
2 parents 2793e8d + 56d20c2
commit 1c8cbe7
Show file tree

Hide file tree

Showing 6 changed files with 147 additions and 52 deletions.
diff --git a/clippy_lints/src/no_effect.rs b/clippy_lints/src/no_effect.rs
@@ -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),

diff --git a/tests/ui/no_effect.rs b/tests/ui/no_effect.rs
@@ -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;
 }
diff --git a/tests/ui/no_effect.stderr b/tests/ui/no_effect.stderr
@@ -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];
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^

diff --git a/tests/ui/unnecessary_operation.fixed b/tests/ui/unnecessary_operation.fixed
@@ -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;
 }
diff --git a/tests/ui/unnecessary_operation.rs b/tests/ui/unnecessary_operation.rs
@@ -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;
 }