Skip to content

unnecessary_operation: don't suggest autofixes on composite types with fields of uncertain types #15460

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
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
60 changes: 48 additions & 12 deletions clippy_lints/src/no_effect.rs
Original file line number Diff line number Diff line change
@@ -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::{
Expand Down Expand Up @@ -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)
Copy link
Contributor Author

@ada4a ada4a Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of off-topic: I was forced to use the more verbose Vec::new() syntax in all the tests because, when I used vec![], this check would prevent the lint from firing. That doesn't sound quite right to me?

{
if let ExprKind::Index(..) = &expr.kind {
Expand Down Expand Up @@ -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<Vec<&'a Expr<'a>>> {
fn reduce_expression<'a>(
cx: &LateContext<'_>,
expr: &'a Expr<'a>,
applicability: &mut Applicability,
) -> Option<Vec<&'a Expr<'a>>> {
if expr.span.from_expansion() {
return None;
}
Expand All @@ -338,16 +339,46 @@ fn reduce_expression<'a>(cx: &LateContext<'_>, expr: &'a Expr<'a>) -> Option<Vec
ExprKind::Binary(ref binop, a, b) if binop.node != BinOpKind::And && binop.node != BinOpKind::Or => {
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 {
Expand All @@ -359,6 +390,11 @@ fn reduce_expression<'a>(cx: &LateContext<'_>, expr: &'a Expr<'a>) -> Option<Vec
}
},
ExprKind::Call(callee, args) => {
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`
Expand All @@ -384,7 +420,7 @@ fn reduce_expression<'a>(cx: &LateContext<'_>, expr: &'a Expr<'a>) -> Option<Vec
BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided) => 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 {
Expand Down
33 changes: 33 additions & 0 deletions tests/ui/unnecessary_operation_unfixable.rs
Original file line number Diff line number Diff line change
@@ -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<u32>,
}

// the repro
DescriptorSet { slots: Vec::new() };
//~^ unnecessary_operation

// other cases
enum E {
Foo { f: Vec<u32> },
Bar(Vec<u32>),
}
E::Foo { f: Vec::new() };
//~^ unnecessary_operation
E::Bar(Vec::new());
//~^ unnecessary_operation

struct Tuple(Vec<u32>);
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
}
35 changes: 35 additions & 0 deletions tests/ui/unnecessary_operation_unfixable.stderr
Original file line number Diff line number Diff line change
@@ -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