From 89774234be1ce368ea2b1c6c127fe254331ee47e Mon Sep 17 00:00:00 2001 From: cocodery Date: Mon, 4 Dec 2023 15:57:27 +0800 Subject: [PATCH] Rewrite logic of `has_nontrivial_oprand`. Check whether operator is overrided with a `struct` operand. The struct here refers to `struct`, `enum`, `union`. Add and fix test for `no_effect` lint. --- clippy_lints/src/no_effect.rs | 90 +++++++++++++---------------------- tests/ui/no_effect.rs | 23 +++++++++ tests/ui/no_effect.stderr | 58 +++++++++++----------- 3 files changed, 86 insertions(+), 85 deletions(-) diff --git a/clippy_lints/src/no_effect.rs b/clippy_lints/src/no_effect.rs index 66c013b4f35..e3930b0568d 100644 --- a/clippy_lints/src/no_effect.rs +++ b/clippy_lints/src/no_effect.rs @@ -10,6 +10,7 @@ use rustc_hir::{ use rustc_infer::infer::TyCtxtInferExt as _; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; +use rustc_middle::ty; use rustc_session::declare_lint_pass; use std::ops::Deref; @@ -87,8 +88,13 @@ 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); // assume nontrivial oprand of `Binary` Expr can skip `check_unnecessary_operation` - if has_nontrivial_oprand(expr) { + if has_nontrivial_oprand(cx, expr) { return true; } if has_no_effect(cx, expr) { @@ -157,66 +163,38 @@ fn check_no_effect(cx: &LateContext<'_>, stmt: &Stmt<'_>) -> bool { false } -fn has_nontrivial_oprand(expr: &Expr<'_>) -> bool { - if expr.span.from_expansion() { - return false; - } - return match peel_blocks(expr).kind { - ExprKind::Binary(_, lhs, rhs) => !check_nontrivial_operand(lhs, rhs), +fn has_nontrivial_oprand(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { + // It's very hard or impossable to check whether overrided operator have side-effect this lint. + // So, this function assume user-defined binary operator is overrided with an side-effect. + // The definition of user-defined structure here is `struct`, `enum`, `uniom`, + // Althrough this will weaken the ability of this lint, less error lint-fix happen. + match expr.kind { + ExprKind::Binary(_, lhs, rhs) => { + // get type of lhs and rhs + let tyck_result = cx.typeck_results(); + let ty_lhs = tyck_result.expr_ty(lhs).kind(); + let ty_rhs = tyck_result.expr_ty(rhs).kind(); + // check whether lhs is a user-defined structure + // only need to check lhs in fact + let ud_lhs = match ty_lhs { + ty::Adt(adt_def, _) => adt_def.is_struct() || adt_def.is_enum() || adt_def.is_union(), + _ => false, + }; + let ud_rhs = match ty_rhs { + ty::Adt(adt_def, _) => adt_def.is_struct() || adt_def.is_enum() || adt_def.is_union(), + _ => false, + }; + + // reference: rust/compiler/rustc_middle/src/ty/typeck_results.rs: `is_method_call`. + // use this function to check whether operator is overrided in `ExprKind::Binary`. + (ud_lhs || ud_rhs) && tyck_result.is_method_call(expr) + }, _ => false, - }; -} - -fn check_nontrivial_operand(lhs: &Expr<'_>, rhs: &Expr<'_>) -> bool { - // It's seem that impossable to check whether operator is overrided through context of this lint, - // so, this function assume user-defined binary operator is overrided with an side-effect. - // The definition of user-defined structure here is `tuple`, `array`, `struct`, - // it looks like a little bit simple, but useful. - // Althrough this will weaken the ability of this lint, - // less miss lint-fix happen. - - // a closure to check whether expr belongs to user-defined structure - let closure = |expr: &Expr<'_>| -> bool { - match &expr.kind { - // check whether expr is a user-defined sturcture - ExprKind::Tup(..) | ExprKind::Array(..) | ExprKind::Struct(..) => true, - // resolve expr's path - ExprKind::Path(rustc_hir::QPath::Resolved( - _, - rustc_hir::Path { - span: _, - res, - segments: _, - }, - )) => { - match res { - Res::Def(defkind, _) => match defkind { - // user-defined - DefKind::Struct | DefKind::Ctor(_, _) => true, - _ => false, - }, - _ => false, - }; - false - }, - _ => false, - } - }; - - let lhs_ud = closure(lhs); - let rhs_ud = closure(rhs); - // one of lhs or rhs is user-defined structure - if lhs_ud || rhs_ud { - return false; } - true } fn has_no_effect(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { - if expr.span.from_expansion() { - return false; - } - match peel_blocks(expr).kind { + 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 index c52f4389192..7ffdeef6582 100644 --- a/tests/ui/no_effect.rs +++ b/tests/ui/no_effect.rs @@ -9,6 +9,22 @@ clippy::useless_vec )] +use std::fmt::Display; +use std::ops::Shl; + +struct Cout; + +impl Shl for Cout +where + T: Display, +{ + type Output = Self; + fn shl(self, rhs: T) -> Self::Output { + println!("{}", rhs); + self + } +} + struct Unit; struct Tuple(i32); struct Struct { @@ -174,4 +190,11 @@ fn main() { GreetStruct1("world"); GreetStruct2()("world"); GreetStruct3 {}("world"); + + fn n() -> i32 { + 42 + } + + Cout << 142; + Cout << n(); } diff --git a/tests/ui/no_effect.stderr b/tests/ui/no_effect.stderr index feba35697f5..437e556a7a8 100644 --- 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:114: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:117:5 | LL | s2; | ^^^ error: statement with no effect - --> $DIR/no_effect.rs:103:5 + --> $DIR/no_effect.rs:119:5 | LL | Unit; | ^^^^^ error: statement with no effect - --> $DIR/no_effect.rs:105:5 + --> $DIR/no_effect.rs:121:5 | LL | Tuple(0); | ^^^^^^^^^ error: statement with no effect - --> $DIR/no_effect.rs:107:5 + --> $DIR/no_effect.rs:123:5 | LL | Struct { field: 0 }; | ^^^^^^^^^^^^^^^^^^^^ error: statement with no effect - --> $DIR/no_effect.rs:109:5 + --> $DIR/no_effect.rs:125:5 | LL | Struct { ..s }; | ^^^^^^^^^^^^^^^ error: statement with no effect - --> $DIR/no_effect.rs:111:5 + --> $DIR/no_effect.rs:127:5 | LL | Union { a: 0 }; | ^^^^^^^^^^^^^^^ error: statement with no effect - --> $DIR/no_effect.rs:113:5 + --> $DIR/no_effect.rs:129:5 | LL | Enum::Tuple(0); | ^^^^^^^^^^^^^^^ error: statement with no effect - --> $DIR/no_effect.rs:115:5 + --> $DIR/no_effect.rs:131:5 | LL | Enum::Struct { field: 0 }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^ error: statement with no effect - --> $DIR/no_effect.rs:117:5 + --> $DIR/no_effect.rs:133:5 | LL | 5 + 6; | ^^^^^^ error: statement with no effect - --> $DIR/no_effect.rs:119:5 + --> $DIR/no_effect.rs:135:5 | LL | *&42; | ^^^^^ error: statement with no effect - --> $DIR/no_effect.rs:121:5 + --> $DIR/no_effect.rs:137:5 | LL | &6; | ^^^ error: statement with no effect - --> $DIR/no_effect.rs:123:5 + --> $DIR/no_effect.rs:139:5 | LL | (5, 6, 7); | ^^^^^^^^^^ error: statement with no effect - --> $DIR/no_effect.rs:125:5 + --> $DIR/no_effect.rs:141:5 | LL | ..; | ^^^ error: statement with no effect - --> $DIR/no_effect.rs:127:5 + --> $DIR/no_effect.rs:143:5 | LL | 5..; | ^^^^ error: statement with no effect - --> $DIR/no_effect.rs:129:5 + --> $DIR/no_effect.rs:145:5 | LL | ..5; | ^^^^ error: statement with no effect - --> $DIR/no_effect.rs:131:5 + --> $DIR/no_effect.rs:147:5 | LL | 5..6; | ^^^^^ error: statement with no effect - --> $DIR/no_effect.rs:133:5 + --> $DIR/no_effect.rs:149:5 | LL | 5..=6; | ^^^^^^ error: statement with no effect - --> $DIR/no_effect.rs:135:5 + --> $DIR/no_effect.rs:151:5 | LL | [42, 55]; | ^^^^^^^^^ error: statement with no effect - --> $DIR/no_effect.rs:137:5 + --> $DIR/no_effect.rs:153:5 | LL | [42, 55][1]; | ^^^^^^^^^^^^ error: statement with no effect - --> $DIR/no_effect.rs:139:5 + --> $DIR/no_effect.rs:155:5 | LL | (42, 55).1; | ^^^^^^^^^^^ error: statement with no effect - --> $DIR/no_effect.rs:141:5 + --> $DIR/no_effect.rs:157:5 | LL | [42; 55]; | ^^^^^^^^^ error: statement with no effect - --> $DIR/no_effect.rs:143:5 + --> $DIR/no_effect.rs:159:5 | LL | [42; 55][13]; | ^^^^^^^^^^^^^ error: statement with no effect - --> $DIR/no_effect.rs:146:5 + --> $DIR/no_effect.rs:162:5 | LL | || x += 5; | ^^^^^^^^^^ error: statement with no effect - --> $DIR/no_effect.rs:149:5 + --> $DIR/no_effect.rs:165:5 | LL | FooString { s: s }; | ^^^^^^^^^^^^^^^^^^^ error: binding to `_` prefixed variable with no side-effect - --> $DIR/no_effect.rs:151:5 + --> $DIR/no_effect.rs:167: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:170: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:172: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:174:5 | LL | let _cat = [2, 4, 6, 8][2]; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^