Skip to content
Merged
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
47 changes: 33 additions & 14 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::macros::{FormatArgsStorage, find_format_arg_expr, is_format_macro, root_macro_call_first_node};
use clippy_utils::source::{indent_of, reindent_multiline, snippet_with_context};
use clippy_utils::source::{snippet_indent, walk_span_to_context};
use clippy_utils::visitors::{for_each_local_assignment, for_each_value_source};
use core::ops::ControlFlow;
use rustc_ast::{FormatArgs, FormatArgumentKind};
Expand Down Expand Up @@ -74,10 +74,10 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, format_args: &FormatArgsStorag
"this let-binding has unit value",
|diag| {
let mut suggestions = Vec::new();
let init_new_span = walk_span_to_context(init.span, local.span.ctxt()).unwrap();

// Suggest omitting the `let` binding
let mut app = Applicability::MachineApplicable;
let snip = snippet_with_context(cx, init.span, local.span.ctxt(), "()", &mut app).0;
let app = Applicability::MachineApplicable;

// If this is a binding pattern, we need to add suggestions to remove any usages
// of the variable
Expand All @@ -89,35 +89,48 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, format_args: &FormatArgsStorag
walk_body(&mut visitor, body);

let mut has_in_format_capture = false;
suggestions.extend(visitor.spans.iter().filter_map(|span| match span {
suggestions.extend(visitor.spans.into_iter().filter_map(|span| match span {
MaybeInFormatCapture::Yes => {
has_in_format_capture = true;
None
},
MaybeInFormatCapture::No(span) => Some((*span, "()".to_string())),
MaybeInFormatCapture::No(span) => Some((span, "()".to_string())),
}));

if has_in_format_capture {
// In a case like this:
// ```
// let unit = returns_unit();
// eprintln!("{unit}");
// ```
// we can't remove the `unit` binding and replace its uses with a `()`,
// because the `eprintln!` would break.
//
// So do the following instead:
// ```
// let unit = ();
// returns_unit();
// eprintln!("{unit}");
// ```
// TODO: find a less awkward way to do this
suggestions.push((
init.span,
format!("();\n{}", reindent_multiline(&snip, false, indent_of(cx, local.span))),
init_new_span.shrink_to_lo(),
format!("();\n{}", snippet_indent(cx, local.span).as_deref().unwrap_or("")),
));
diag.multipart_suggestion(
"replace variable usages with `()`",
suggestions,
Applicability::MachineApplicable,
);
diag.multipart_suggestion_verbose("replace variable usages with `()`", suggestions, app);
return;
}
}

suggestions.push((local.span, format!("{snip};")));
// let local = returns_unit();
// ^^^^^^^^^^^^ remove this
suggestions.push((local.span.until(init_new_span), String::new()));
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);
diag.multipart_suggestion_verbose(message, suggestions, app);
},
);
}
Expand All @@ -132,6 +145,12 @@ struct UnitVariableCollector<'a, 'tcx> {
macro_call: Option<&'a FormatArgs>,
}

/// Whether the unit variable is captured in a `format!`:
///
/// ```ignore
/// let unit = ();
/// eprintln!("{unit}");
/// ```
enum MaybeInFormatCapture {
Yes,
No(Span),
Expand Down
42 changes: 31 additions & 11 deletions tests/ui/let_unit.fixed
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
#![warn(clippy::let_unit_value)]
#![allow(unused, clippy::no_effect, clippy::needless_late_init, path_statements)]
#![allow(
clippy::no_effect,
clippy::needless_late_init,
path_statements,
clippy::match_single_binding
)]

macro_rules! let_and_return {
($n:expr) => {{
Expand All @@ -15,12 +20,12 @@ fn main() {
if true {
// do not lint this, since () is explicit
let _a = ();
let () = dummy();
let () = returns_unit();
let () = ();
() = dummy();
() = returns_unit();
() = ();
let _a: () = ();
let _a: () = dummy();
let _a: () = returns_unit();
}

consume_units_with_for_loop(); // should be fine as well
Expand All @@ -30,7 +35,7 @@ fn main() {
let_and_return!(()) // should be fine
}

fn dummy() {}
fn returns_unit() {}

// Related to issue #1964
fn consume_units_with_for_loop() {
Expand Down Expand Up @@ -181,8 +186,6 @@ async fn issue10433() {
pub async fn issue11502(a: ()) {}

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

fn returns_result<T>(res: T) -> Result<T, ()> {
Ok(res)
}
Expand All @@ -199,13 +202,30 @@ pub fn issue12594() {
}
}

fn takes_unit(x: ()) {}

fn issue15061() {
fn return_unit() {}
fn do_something(x: ()) {}
let res = ();
returns_unit();
//~^ let_unit_value
takes_unit(());
println!("{res:?}");
}

fn issue15771() {
match "Example String" {
_ => returns_unit(),
//~^ let_unit_value
}

if true {}
//~^ let_unit_value
}

fn issue_15784() {
let res = ();
return_unit();
eprintln!("I return unit");
//~^ let_unit_value
do_something(());
takes_unit(());
println!("{res:?}");
}
41 changes: 30 additions & 11 deletions tests/ui/let_unit.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
#![warn(clippy::let_unit_value)]
#![allow(unused, clippy::no_effect, clippy::needless_late_init, path_statements)]
#![allow(
clippy::no_effect,
clippy::needless_late_init,
path_statements,
clippy::match_single_binding
)]

macro_rules! let_and_return {
($n:expr) => {{
Expand All @@ -15,12 +20,12 @@ fn main() {
if true {
// do not lint this, since () is explicit
let _a = ();
let () = dummy();
let () = returns_unit();
let () = ();
() = dummy();
() = returns_unit();
() = ();
let _a: () = ();
let _a: () = dummy();
let _a: () = returns_unit();
}

consume_units_with_for_loop(); // should be fine as well
Expand All @@ -30,7 +35,7 @@ fn main() {
let_and_return!(()) // should be fine
}

fn dummy() {}
fn returns_unit() {}

// Related to issue #1964
fn consume_units_with_for_loop() {
Expand Down Expand Up @@ -181,8 +186,6 @@ async fn issue10433() {
pub async fn issue11502(a: ()) {}

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

fn returns_result<T>(res: T) -> Result<T, ()> {
Ok(res)
}
Expand All @@ -199,12 +202,28 @@ pub fn issue12594() {
}
}

fn takes_unit(x: ()) {}

fn issue15061() {
fn return_unit() {}
fn do_something(x: ()) {}
let res = returns_unit();
//~^ let_unit_value
takes_unit(res);
println!("{res:?}");
}

fn issue15771() {
match "Example String" {
_ => _ = returns_unit(),
//~^ let_unit_value
}

_ = if true {}
//~^ let_unit_value
}

let res = return_unit();
fn issue_15784() {
let res = eprintln!("I return unit");
//~^ let_unit_value
do_something(res);
takes_unit(res);
println!("{res:?}");
}
84 changes: 58 additions & 26 deletions tests/ui/let_unit.stderr
Original file line number Diff line number Diff line change
@@ -1,14 +1,19 @@
error: this let-binding has unit value
--> tests/ui/let_unit.rs:11:5
--> tests/ui/let_unit.rs:16:5
|
LL | let _x = println!("x");
| ^^^^^^^^^^^^^^^^^^^^^^^ help: omit the `let` binding: `println!("x");`
| ^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `-D clippy::let-unit-value` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::let_unit_value)]`
help: omit the `let` binding
|
LL - let _x = println!("x");
LL + println!("x");
|

error: this let-binding has unit value
--> tests/ui/let_unit.rs:60:5
--> tests/ui/let_unit.rs:65:5
|
LL | / let _ = v
LL | |
Expand All @@ -21,18 +26,12 @@ LL | | .unwrap();
|
help: omit the `let` binding
|
LL ~ v
LL +
LL + .into_iter()
LL + .map(|i| i * 2)
LL + .filter(|i| i.is_multiple_of(2))
LL + .map(|_| ())
LL + .next()
LL + .unwrap();
LL - let _ = v
LL + v
|

error: this let-binding has unit value
--> tests/ui/let_unit.rs:110:5
--> tests/ui/let_unit.rs:115:5
|
LL | / let x = match Some(0) {
LL | |
Expand All @@ -45,17 +44,12 @@ LL | | };
|
help: omit the `let` binding
|
LL ~ match Some(0) {
LL +
LL + None => f2(1),
LL + Some(0) => f(),
LL + Some(1) => f2(3),
LL + Some(_) => (),
LL + };
LL - let x = match Some(0) {
LL + match Some(0) {
|

error: this let-binding has unit value
--> tests/ui/let_unit.rs:192:9
--> tests/ui/let_unit.rs:195:9
|
LL | let res = returns_unit();
| ^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -69,18 +63,56 @@ LL ~ returns_result(()).unwrap();
|

error: this let-binding has unit value
--> tests/ui/let_unit.rs:206:5
--> tests/ui/let_unit.rs:208:5
|
LL | let res = returns_unit();
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: replace variable usages with `()`
|
LL ~ let res = ();
LL ~ returns_unit();
LL |
LL ~ takes_unit(());
|

error: this let-binding has unit value
--> tests/ui/let_unit.rs:216:14
|
LL | _ => _ = returns_unit(),
| ^^^^^^^^^^^^^^^^^^
|
help: omit the `let` binding
|
LL - _ => _ = returns_unit(),
LL + _ => returns_unit(),
|

error: this let-binding has unit value
--> tests/ui/let_unit.rs:220:5
|
LL | _ = if true {}
| ^^^^^^^^^^^^^^
|
help: omit the `let` binding
|
LL - _ = if true {}
LL + if true {}
|

error: this let-binding has unit value
--> tests/ui/let_unit.rs:225:5
|
LL | let res = return_unit();
| ^^^^^^^^^^^^^^^^^^^^^^^^
LL | let res = eprintln!("I return unit");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: replace variable usages with `()`
|
LL ~ let res = ();
LL ~ return_unit();
LL ~ eprintln!("I return unit");
LL |
LL ~ do_something(());
LL ~ takes_unit(());
|

error: aborting due to 5 previous errors
error: aborting due to 8 previous errors

Loading