Skip to content

Commit

Permalink
chore: multipart suggestions for let_unit_value lint (#13754)
Browse files Browse the repository at this point in the history
This should address #13099 for the let_unit test.

changelog: [let_unit]: Updated let_unit to use multipart_suggestions
where appropriate
  • Loading branch information
blyxyas authored Dec 23, 2024
2 parents 592fd34 + ec24388 commit 988042e
Show file tree
Hide file tree
Showing 4 changed files with 228 additions and 27 deletions.
35 changes: 24 additions & 11 deletions clippy_lints/src/unit_types/let_unit_value.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::source::snippet_with_context;
use clippy_utils::visitors::{for_each_local_assignment, for_each_value_source, is_local_used};
use clippy_utils::visitors::{for_each_local_assignment, for_each_value_source};
use core::ops::ControlFlow;
use rustc_errors::Applicability;
use rustc_hir::def::{DefKind, Res};
Expand Down Expand Up @@ -71,25 +71,38 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, local: &'tcx LetStmt<'_>) {
local.span,
"this let-binding has unit value",
|diag| {
let mut suggestions = Vec::new();

// Suggest omitting the `let` binding
if let Some(expr) = &local.init {
let mut app = Applicability::MachineApplicable;
let snip = snippet_with_context(cx, expr.span, local.span.ctxt(), "()", &mut app).0;
diag.span_suggestion(local.span, "omit the `let` binding", format!("{snip};"), app);
suggestions.push((local.span, format!("{snip};")));
}

if let PatKind::Binding(_, binding_hir_id, ident, ..) = local.pat.kind
// If this is a binding pattern, we need to add suggestions to remove any usages
// of the variable
if let PatKind::Binding(_, binding_hir_id, ..) = local.pat.kind
&& let Some(body_id) = cx.enclosing_body.as_ref()
&& let body = cx.tcx.hir().body(*body_id)
&& is_local_used(cx, body, binding_hir_id)
{
let identifier = ident.as_str();
let body = cx.tcx.hir().body(*body_id);

// Collect variable usages
let mut visitor = UnitVariableCollector::new(binding_hir_id);
walk_body(&mut visitor, body);
visitor.spans.into_iter().for_each(|span| {
let msg =
format!("variable `{identifier}` of type `()` can be replaced with explicit `()`");
diag.span_suggestion(span, msg, "()", Applicability::MachineApplicable);
});

// Add suggestions for replacing variable usages
suggestions.extend(visitor.spans.into_iter().map(|span| (span, "()".to_string())));
}

// Emit appropriate diagnostic based on whether there are usages of the let binding
if !suggestions.is_empty() {
let message = if suggestions.len() == 1 {
"omit the `let` binding"
} else {
"omit the `let` binding and replace variable usages with `()`"
};
diag.multipart_suggestion(message, suggestions, Applicability::MachineApplicable);
}
},
);
Expand Down
196 changes: 196 additions & 0 deletions tests/ui/let_unit.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,196 @@
#![warn(clippy::let_unit_value)]
#![allow(unused, clippy::no_effect, clippy::needless_late_init, path_statements)]

macro_rules! let_and_return {
($n:expr) => {{
let ret = $n;
}};
}

fn main() {
println!("x");
let _y = 1; // this is fine
let _z = ((), 1); // this as well
if true {
// do not lint this, since () is explicit
let _a = ();
let () = dummy();
let () = ();
() = dummy();
() = ();
let _a: () = ();
let _a: () = dummy();
}

consume_units_with_for_loop(); // should be fine as well

multiline_sugg();

let_and_return!(()) // should be fine
}

fn dummy() {}

// Related to issue #1964
fn consume_units_with_for_loop() {
// `for_let_unit` lint should not be triggered by consuming them using for loop.
let v = vec![(), (), ()];
let mut count = 0;
for _ in v {
count += 1;
}
assert_eq!(count, 3);

// Same for consuming from some other Iterator<Item = ()>.
let (tx, rx) = ::std::sync::mpsc::channel();
tx.send(()).unwrap();
drop(tx);

count = 0;
for _ in rx.iter() {
count += 1;
}
assert_eq!(count, 1);
}

fn multiline_sugg() {
let v: Vec<u8> = vec![2];

v
.into_iter()
.map(|i| i * 2)
.filter(|i| i % 2 == 0)
.map(|_| ())
.next()
.unwrap();
}

#[derive(Copy, Clone)]
pub struct ContainsUnit(()); // should be fine

fn _returns_generic() {
fn f<T>() -> T {
unimplemented!()
}
fn f2<T, U>(_: T) -> U {
unimplemented!()
}
fn f3<T>(x: T) -> T {
x
}
fn f5<T: Default>(x: bool) -> Option<T> {
x.then(|| T::default())
}

let _: () = f();
let x: () = f();

let _: () = f2(0i32);
let x: () = f2(0i32);

let _: () = f3(());
let x: () = f3(());

fn f4<T>(mut x: Vec<T>) -> T {
x.pop().unwrap()
}
let _: () = f4(vec![()]);
let x: () = f4(vec![()]);

let _: () = {
let x = 5;
f2(x)
};

let _: () = if true { f() } else { f2(0) };
let x: () = if true { f() } else { f2(0) };

match Some(0) {
None => f2(1),
Some(0) => f(),
Some(1) => f2(3),
Some(_) => (),
};

let _: () = f5(true).unwrap();

#[allow(clippy::let_unit_value)]
{
let x = f();
let y;
let z;
match 0 {
0 => {
y = f();
z = f();
},
1 => {
println!("test");
y = f();
z = f3(());
},
_ => panic!(),
}

let x1;
let x2;
if true {
x1 = f();
x2 = x1;
} else {
x2 = f();
x1 = x2;
}

let opt;
match f5(true) {
Some(x) => opt = x,
None => panic!(),
};

#[warn(clippy::let_unit_value)]
{
let _: () = x;
let _: () = y;
let _: () = z;
let _: () = x1;
let _: () = x2;
let _: () = opt;
}
}

let () = f();
}

fn attributes() {
fn f() {}

#[allow(clippy::let_unit_value)]
let _ = f();
#[expect(clippy::let_unit_value)]
let _ = f();
}

async fn issue10433() {
let _pending: () = std::future::pending().await;
}

pub async fn issue11502(a: ()) {}

pub fn issue12594() {
fn returns_unit() {}

fn returns_result<T>(res: T) -> Result<T, ()> {
Ok(res)
}

fn actual_test() {
// create first a unit value'd value
returns_unit();
returns_result(()).unwrap();
returns_result(()).unwrap();
// make sure we replace only the first variable
let res = 1;
returns_result(res).unwrap();
}
}
2 changes: 0 additions & 2 deletions tests/ui/let_unit.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
#![warn(clippy::let_unit_value)]
#![allow(unused, clippy::no_effect, clippy::needless_late_init, path_statements)]

//@no-rustfix: need to change the suggestion to a multipart suggestion

macro_rules! let_and_return {
($n:expr) => {{
let ret = $n;
Expand Down
22 changes: 8 additions & 14 deletions tests/ui/let_unit.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: this let-binding has unit value
--> tests/ui/let_unit.rs:13:5
--> tests/ui/let_unit.rs:11:5
|
LL | let _x = println!("x");
| ^^^^^^^^^^^^^^^^^^^^^^^ help: omit the `let` binding: `println!("x");`
Expand All @@ -8,7 +8,7 @@ LL | let _x = println!("x");
= help: to override `-D warnings` add `#[allow(clippy::let_unit_value)]`

error: this let-binding has unit value
--> tests/ui/let_unit.rs:61:5
--> tests/ui/let_unit.rs:59:5
|
LL | / let _ = v
LL | | .into_iter()
Expand All @@ -31,7 +31,7 @@ LL + .unwrap();
|

error: this let-binding has unit value
--> tests/ui/let_unit.rs:110:5
--> tests/ui/let_unit.rs:108:5
|
LL | / let x = match Some(0) {
LL | | None => f2(1),
Expand All @@ -52,23 +52,17 @@ LL + };
|

error: this let-binding has unit value
--> tests/ui/let_unit.rs:191:9
--> tests/ui/let_unit.rs:189:9
|
LL | let res = returns_unit();
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: omit the `let` binding
|
LL | returns_unit();
|
help: variable `res` of type `()` can be replaced with explicit `()`
help: omit the `let` binding and replace variable usages with `()`
|
LL | returns_result(()).unwrap();
| ~~
help: variable `res` of type `()` can be replaced with explicit `()`
LL ~ returns_unit();
LL ~ returns_result(()).unwrap();
LL ~ returns_result(()).unwrap();
|
LL | returns_result(()).unwrap();
| ~~

error: aborting due to 4 previous errors

0 comments on commit 988042e

Please sign in to comment.