diff --git a/clippy_lints/src/no_effect.rs b/clippy_lints/src/no_effect.rs index 72e6503e7e49..18725aa4d254 100644 --- a/clippy_lints/src/no_effect.rs +++ b/clippy_lints/src/no_effect.rs @@ -1,4 +1,5 @@ use clippy_utils::diagnostics::{span_lint_hir, span_lint_hir_and_then}; +use clippy_utils::higher::Range; use clippy_utils::source::SpanRangeExt; use clippy_utils::ty::{expr_type_is_certain, has_drop}; use clippy_utils::{ @@ -271,11 +272,12 @@ fn has_no_effect(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { } fn check_unnecessary_operation(cx: &LateContext<'_>, stmt: &Stmt<'_>) { + let mut applicability = Applicability::MachineApplicable; if let StmtKind::Semi(expr) = stmt.kind && !stmt.span.in_external_macro(cx.sess().source_map()) && let ctxt = stmt.span.ctxt() && expr.span.ctxt() == ctxt - && let Some(reduced) = reduce_expression(cx, expr) + && let Some(reduced) = reduce_expression(cx, expr, &mut applicability) && reduced.iter().all(|e| e.span.ctxt() == ctxt) { if let ExprKind::Index(..) = &expr.kind { @@ -317,19 +319,18 @@ fn check_unnecessary_operation(cx: &LateContext<'_>, stmt: &Stmt<'_>) { stmt.span, "unnecessary operation", |diag| { - diag.span_suggestion( - stmt.span, - "statement can be reduced to", - snippet, - Applicability::MachineApplicable, - ); + diag.span_suggestion(stmt.span, "statement can be reduced to", snippet, applicability); }, ); } } } -fn reduce_expression<'a>(cx: &LateContext<'_>, expr: &'a Expr<'a>) -> Option>> { +fn reduce_expression<'a>( + cx: &LateContext<'_>, + expr: &'a Expr<'a>, + applicability: &mut Applicability, +) -> Option>> { if expr.span.from_expansion() { return None; } @@ -338,16 +339,46 @@ fn reduce_expression<'a>(cx: &LateContext<'_>, expr: &'a Expr<'a>) -> Option { Some(vec![a, b]) }, - ExprKind::Array(v) | ExprKind::Tup(v) => Some(v.iter().collect()), + ExprKind::Array(elems) => { + if elems.iter().any(|elem| !expr_type_is_certain(cx, elem)) { + // there's a risk that if we take the elem exprs out of the context of the array, + // their types might become ambiguous + *applicability = Applicability::MaybeIncorrect; + } + Some(elems.iter().collect()) + }, + ExprKind::Tup(v) => Some(v.iter().collect()), ExprKind::Repeat(inner, _) | ExprKind::Type(inner, _) | ExprKind::Unary(_, inner) | ExprKind::Field(inner, _) - | ExprKind::AddrOf(_, _, inner) => reduce_expression(cx, inner).or_else(|| Some(vec![inner])), + | ExprKind::AddrOf(_, _, inner) => reduce_expression(cx, inner, applicability).or_else(|| Some(vec![inner])), ExprKind::Cast(inner, _) if expr_type_is_certain(cx, inner) => { - reduce_expression(cx, inner).or_else(|| Some(vec![inner])) + reduce_expression(cx, inner, applicability).or_else(|| Some(vec![inner])) + }, + // In the normal `Struct` case, we bail out if any of the fields has an uncertain type. + // But for two-sided ranges, we know that if the type of one of the sides is certain, then so is the other + // one's. So we only check that, more relaxed pre-condition. + // + // Note that that condition true in general for any struct with a generic present in two fields, but + // generalizing the check to those would be cumbersome. + ExprKind::Struct(..) + if let Some(range) = Range::hir(expr) + && let Some(start) = range.start + && let Some(end) = range.end => + { + if [start, end].into_iter().any(|e| expr_type_is_certain(cx, e)) { + Some([start, end].into_iter().collect()) + } else { + None + } }, ExprKind::Struct(_, fields, ref base) => { + if fields.iter().any(|f| !expr_type_is_certain(cx, f.expr)) { + // there's a risk that if we take the field exprs out of the context of the struct constructor, + // their types might become ambiguous + *applicability = Applicability::MaybeIncorrect; + } if has_drop(cx, cx.typeck_results().expr_ty(expr)) { None } else { @@ -359,6 +390,11 @@ fn reduce_expression<'a>(cx: &LateContext<'_>, expr: &'a Expr<'a>) -> Option { + if args.iter().any(|a| !expr_type_is_certain(cx, a)) { + // there's a risk that if we take the args out of the context of the + // call/constructor, their types might become ambiguous + *applicability = Applicability::MaybeIncorrect; + } if let ExprKind::Path(ref qpath) = callee.kind { if cx.typeck_results().type_dependent_def(expr.hir_id).is_some() { // type-dependent function call like `impl FnOnce for X` @@ -384,7 +420,7 @@ fn reduce_expression<'a>(cx: &LateContext<'_>, expr: &'a Expr<'a>) -> Option None, BlockCheckMode::DefaultBlock => Some(vec![&**e]), // in case of compiler-inserted signaling blocks - BlockCheckMode::UnsafeBlock(_) => reduce_expression(cx, e), + BlockCheckMode::UnsafeBlock(_) => reduce_expression(cx, e, applicability), } }) } else { diff --git a/tests/ui/unnecessary_operation_unfixable.rs b/tests/ui/unnecessary_operation_unfixable.rs new file mode 100644 index 000000000000..6ded3a593eee --- /dev/null +++ b/tests/ui/unnecessary_operation_unfixable.rs @@ -0,0 +1,33 @@ +//@no-rustfix +#![expect(unused)] +#![warn(clippy::unnecessary_operation)] + +// don't lint if any of the fields has an ambiguous type when used by themselves +fn issue15381() { + struct DescriptorSet { + slots: Vec, + } + + // the repro + DescriptorSet { slots: Vec::new() }; + //~^ unnecessary_operation + + // other cases + enum E { + Foo { f: Vec }, + Bar(Vec), + } + E::Foo { f: Vec::new() }; + //~^ unnecessary_operation + E::Bar(Vec::new()); + //~^ unnecessary_operation + + struct Tuple(Vec); + Tuple(Vec::new()); + //~^ unnecessary_operation + + // the type of the second slice gets inferred based on it needing to be the same to that of the + // first one, but that doesn't happen when they're outside the array + [[1, 2, 3].as_slice(), [].as_slice()]; + //~^ unnecessary_operation +} diff --git a/tests/ui/unnecessary_operation_unfixable.stderr b/tests/ui/unnecessary_operation_unfixable.stderr new file mode 100644 index 000000000000..6f30d334d854 --- /dev/null +++ b/tests/ui/unnecessary_operation_unfixable.stderr @@ -0,0 +1,35 @@ +error: unnecessary operation + --> tests/ui/unnecessary_operation_unfixable.rs:12:5 + | +LL | DescriptorSet { slots: Vec::new() }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: statement can be reduced to: `Vec::new();` + | + = note: `-D clippy::unnecessary-operation` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::unnecessary_operation)]` + +error: unnecessary operation + --> tests/ui/unnecessary_operation_unfixable.rs:20:5 + | +LL | E::Foo { f: Vec::new() }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: statement can be reduced to: `Vec::new();` + +error: unnecessary operation + --> tests/ui/unnecessary_operation_unfixable.rs:22:5 + | +LL | E::Bar(Vec::new()); + | ^^^^^^^^^^^^^^^^^^^ help: statement can be reduced to: `Vec::new();` + +error: unnecessary operation + --> tests/ui/unnecessary_operation_unfixable.rs:26:5 + | +LL | Tuple(Vec::new()); + | ^^^^^^^^^^^^^^^^^^ help: statement can be reduced to: `Vec::new();` + +error: unnecessary operation + --> tests/ui/unnecessary_operation_unfixable.rs:31:5 + | +LL | [[1, 2, 3].as_slice(), [].as_slice()]; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: statement can be reduced to: `[1, 2, 3].as_slice();[].as_slice();` + +error: aborting due to 5 previous errors +