Skip to content

Commit 887b500

Browse files
llogiqllogiq
authored
Fix let_unit_value suggests wrongly for format macros (#15085)
Closes #15061 changelog: [`let_unit_value`] fix wrong suggestions for format macros
2 parents 3174c05 + 31b9de6 commit 887b500

File tree

6 files changed

+131
-35
lines changed

6 files changed

+131
-35
lines changed

‎clippy_lints/src/lib.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -523,7 +523,8 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co
523523
store.register_late_pass(|_| Box::new(same_name_method::SameNameMethod));
524524
store.register_late_pass(move |_| Box::new(index_refutable_slice::IndexRefutableSlice::new(conf)));
525525
store.register_late_pass(|_| Box::<shadow::Shadow>::default());
526-
store.register_late_pass(|_| Box::new(unit_types::UnitTypes));
526+
let format_args = format_args_storage.clone();
527+
store.register_late_pass(move |_| Box::new(unit_types::UnitTypes::new(format_args.clone())));
527528
store.register_late_pass(move |_| Box::new(loops::Loops::new(conf)));
528529
store.register_late_pass(|_| Box::<main_recursion::MainRecursion>::default());
529530
store.register_late_pass(move |_| Box::new(lifetimes::Lifetimes::new(conf)));

‎clippy_lints/src/unit_types/let_unit_value.rs

Lines changed: 79 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,20 @@
11
use clippy_utils::diagnostics::span_lint_and_then;
2-
use clippy_utils::source::snippet_with_context;
2+
use clippy_utils::macros::{FormatArgsStorage, find_format_arg_expr, is_format_macro, root_macro_call_first_node};
3+
use clippy_utils::source::{indent_of, reindent_multiline, snippet_with_context};
34
use clippy_utils::visitors::{for_each_local_assignment, for_each_value_source};
45
use core::ops::ControlFlow;
6+
use rustc_ast::{FormatArgs, FormatArgumentKind};
57
use rustc_errors::Applicability;
68
use rustc_hir::def::{DefKind, Res};
7-
use rustc_hir::intravisit::{Visitor, walk_body};
9+
use rustc_hir::intravisit::{Visitor, walk_body, walk_expr};
810
use rustc_hir::{Expr, ExprKind, HirId, HirIdSet, LetStmt, MatchSource, Node, PatKind, QPath, TyKind};
911
use rustc_lint::{LateContext, LintContext};
1012
use rustc_middle::ty;
13+
use rustc_span::Span;
1114

1215
use super::LET_UNIT_VALUE;
1316

14-
pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, local: &'tcx LetStmt<'_>) {
17+
pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, format_args: &FormatArgsStorage, local: &'tcx LetStmt<'_>) {
1518
// skip `let () = { ... }`
1619
if let PatKind::Tuple(fields, ..) = local.pat.kind
1720
&& fields.is_empty()
@@ -73,65 +76,111 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, local: &'tcx LetStmt<'_>) {
7376
let mut suggestions = Vec::new();
7477

7578
// Suggest omitting the `let` binding
76-
if let Some(expr) = &local.init {
77-
let mut app = Applicability::MachineApplicable;
78-
let snip = snippet_with_context(cx, expr.span, local.span.ctxt(), "()", &mut app).0;
79-
suggestions.push((local.span, format!("{snip};")));
80-
}
79+
let mut app = Applicability::MachineApplicable;
80+
let snip = snippet_with_context(cx, init.span, local.span.ctxt(), "()", &mut app).0;
8181

8282
// If this is a binding pattern, we need to add suggestions to remove any usages
8383
// of the variable
8484
if let PatKind::Binding(_, binding_hir_id, ..) = local.pat.kind
8585
&& let Some(body_id) = cx.enclosing_body.as_ref()
8686
{
8787
let body = cx.tcx.hir_body(*body_id);
88-
89-
// Collect variable usages
90-
let mut visitor = UnitVariableCollector::new(binding_hir_id);
88+
let mut visitor = UnitVariableCollector::new(cx, format_args, binding_hir_id);
9189
walk_body(&mut visitor, body);
9290

93-
// Add suggestions for replacing variable usages
94-
suggestions.extend(visitor.spans.into_iter().map(|span| (span, "()".to_string())));
95-
}
91+
let mut has_in_format_capture = false;
92+
suggestions.extend(visitor.spans.iter().filter_map(|span| match span {
93+
MaybeInFormatCapture::Yes => {
94+
has_in_format_capture = true;
95+
None
96+
},
97+
MaybeInFormatCapture::No(span) => Some((*span, "()".to_string())),
98+
}));
9699

97-
// Emit appropriate diagnostic based on whether there are usages of the let binding
98-
if !suggestions.is_empty() {
99-
let message = if suggestions.len() == 1 {
100-
"omit the `let` binding"
101-
} else {
102-
"omit the `let` binding and replace variable usages with `()`"
103-
};
104-
diag.multipart_suggestion(message, suggestions, Applicability::MachineApplicable);
100+
if has_in_format_capture {
101+
suggestions.push((
102+
init.span,
103+
format!("();\n{}", reindent_multiline(&snip, false, indent_of(cx, local.span))),
104+
));
105+
diag.multipart_suggestion(
106+
"replace variable usages with `()`",
107+
suggestions,
108+
Applicability::MachineApplicable,
109+
);
110+
return;
111+
}
105112
}
113+
114+
suggestions.push((local.span, format!("{snip};")));
115+
let message = if suggestions.len() == 1 {
116+
"omit the `let` binding"
117+
} else {
118+
"omit the `let` binding and replace variable usages with `()`"
119+
};
120+
diag.multipart_suggestion(message, suggestions, Applicability::MachineApplicable);
106121
},
107122
);
108123
}
109124
}
110125
}
111126

112-
struct UnitVariableCollector {
127+
struct UnitVariableCollector<'a, 'tcx> {
128+
cx: &'a LateContext<'tcx>,
129+
format_args: &'a FormatArgsStorage,
113130
id: HirId,
114-
spans: Vec<rustc_span::Span>,
131+
spans: Vec<MaybeInFormatCapture>,
132+
macro_call: Option<&'a FormatArgs>,
115133
}
116134

117-
impl UnitVariableCollector {
118-
fn new(id: HirId) -> Self {
119-
Self { id, spans: vec![] }
135+
enum MaybeInFormatCapture {
136+
Yes,
137+
No(Span),
138+
}
139+
140+
impl<'a, 'tcx> UnitVariableCollector<'a, 'tcx> {
141+
fn new(cx: &'a LateContext<'tcx>, format_args: &'a FormatArgsStorage, id: HirId) -> Self {
142+
Self {
143+
cx,
144+
format_args,
145+
id,
146+
spans: Vec::new(),
147+
macro_call: None,
148+
}
120149
}
121150
}
122151

123152
/**
124153
* Collect all instances where a variable is used based on its `HirId`.
125154
*/
126-
impl<'tcx> Visitor<'tcx> for UnitVariableCollector {
155+
impl<'tcx> Visitor<'tcx> for UnitVariableCollector<'_, 'tcx> {
127156
fn visit_expr(&mut self, ex: &'tcx Expr<'tcx>) -> Self::Result {
157+
if let Some(macro_call) = root_macro_call_first_node(self.cx, ex)
158+
&& is_format_macro(self.cx, macro_call.def_id)
159+
&& let Some(format_args) = self.format_args.get(self.cx, ex, macro_call.expn)
160+
{
161+
let parent_macro_call = self.macro_call;
162+
self.macro_call = Some(format_args);
163+
walk_expr(self, ex);
164+
self.macro_call = parent_macro_call;
165+
return;
166+
}
167+
128168
if let ExprKind::Path(QPath::Resolved(None, path)) = ex.kind
129169
&& let Res::Local(id) = path.res
130170
&& id == self.id
131171
{
132-
self.spans.push(path.span);
172+
if let Some(macro_call) = self.macro_call
173+
&& macro_call.arguments.all_args().iter().any(|arg| {
174+
matches!(arg.kind, FormatArgumentKind::Captured(_)) && find_format_arg_expr(ex, arg).is_some()
175+
})
176+
{
177+
self.spans.push(MaybeInFormatCapture::Yes);
178+
} else {
179+
self.spans.push(MaybeInFormatCapture::No(path.span));
180+
}
133181
}
134-
rustc_hir::intravisit::walk_expr(self, ex);
182+
183+
walk_expr(self, ex);
135184
}
136185
}
137186

‎clippy_lints/src/unit_types/mod.rs

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,10 @@ mod unit_arg;
33
mod unit_cmp;
44
mod utils;
55

6+
use clippy_utils::macros::FormatArgsStorage;
67
use rustc_hir::{Expr, LetStmt};
78
use rustc_lint::{LateContext, LateLintPass};
8-
use rustc_session::declare_lint_pass;
9+
use rustc_session::impl_lint_pass;
910

1011
declare_clippy_lint! {
1112
/// ### What it does
@@ -96,11 +97,21 @@ declare_clippy_lint! {
9697
"passing unit to a function"
9798
}
9899

99-
declare_lint_pass!(UnitTypes => [LET_UNIT_VALUE, UNIT_CMP, UNIT_ARG]);
100+
pub struct UnitTypes {
101+
format_args: FormatArgsStorage,
102+
}
103+
104+
impl_lint_pass!(UnitTypes => [LET_UNIT_VALUE, UNIT_CMP, UNIT_ARG]);
105+
106+
impl UnitTypes {
107+
pub fn new(format_args: FormatArgsStorage) -> Self {
108+
Self { format_args }
109+
}
110+
}
100111

101112
impl<'tcx> LateLintPass<'tcx> for UnitTypes {
102113
fn check_local(&mut self, cx: &LateContext<'tcx>, local: &'tcx LetStmt<'tcx>) {
103-
let_unit_value::check(cx, local);
114+
let_unit_value::check(cx, &self.format_args, local);
104115
}
105116

106117
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {

‎tests/ui/let_unit.fixed

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -198,3 +198,14 @@ pub fn issue12594() {
198198
returns_result(res).unwrap();
199199
}
200200
}
201+
202+
fn issue15061() {
203+
fn return_unit() {}
204+
fn do_something(x: ()) {}
205+
206+
let res = ();
207+
return_unit();
208+
//~^ let_unit_value
209+
do_something(());
210+
println!("{res:?}");
211+
}

‎tests/ui/let_unit.rs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -198,3 +198,13 @@ pub fn issue12594() {
198198
returns_result(res).unwrap();
199199
}
200200
}
201+
202+
fn issue15061() {
203+
fn return_unit() {}
204+
fn do_something(x: ()) {}
205+
206+
let res = return_unit();
207+
//~^ let_unit_value
208+
do_something(res);
209+
println!("{res:?}");
210+
}

‎tests/ui/let_unit.stderr

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,5 +68,19 @@ LL ~ returns_result(()).unwrap();
6868
LL ~ returns_result(()).unwrap();
6969
|
7070

71-
error: aborting due to 4 previous errors
71+
error: this let-binding has unit value
72+
--> tests/ui/let_unit.rs:206:5
73+
|
74+
LL | let res = return_unit();
75+
| ^^^^^^^^^^^^^^^^^^^^^^^^
76+
|
77+
help: replace variable usages with `()`
78+
|
79+
LL ~ let res = ();
80+
LL ~ return_unit();
81+
LL |
82+
LL ~ do_something(());
83+
|
84+
85+
error: aborting due to 5 previous errors
7286

0 commit comments

Comments
 (0)