Skip to content

Commit 3588a04

Browse files
profetiaprofetia
committed
fix: let_unit_value suggests wrongly for format macros
1 parent 62fd159 commit 3588a04

File tree

6 files changed

+134
-37
lines changed

6 files changed

+134
-37
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: 82 additions & 32 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<'a, 'tcx>(cx: &LateContext<'tcx>, format_args: &'a FormatArgsStorage, local: &'tcx LetStmt<'_>) {
1518
// skip `let () = { ... }`
1619
if let PatKind::Tuple(fields, ..) = local.pat.kind
1720
&& fields.is_empty()
@@ -73,65 +76,112 @@ 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()
86-
{
87-
let body = cx.tcx.hir_body(*body_id);
86+
&& let body = cx.tcx.hir_body(*body_id)
87+
&& let mut visitor = UnitVariableCollector::new(cx, format_args, binding_hir_id)
88+
&& {
89+
walk_body(&mut visitor, body);
8890

89-
// Collect variable usages
90-
let mut visitor = UnitVariableCollector::new(binding_hir_id);
91-
walk_body(&mut visitor, body);
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+
}));
9299

93-
// Add suggestions for replacing variable usages
94-
suggestions.extend(visitor.spans.into_iter().map(|span| (span, "()".to_string())));
100+
has_in_format_capture
101+
}
102+
{
103+
suggestions.push((
104+
init.span,
105+
format!("();\n{}", reindent_multiline(&snip, false, indent_of(cx, local.span))),
106+
));
107+
diag.multipart_suggestion(
108+
"replace variable usages with `()`",
109+
suggestions,
110+
Applicability::MachineApplicable,
111+
);
112+
return;
95113
}
96114

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);
105-
}
115+
suggestions.push((local.span, format!("{snip};")));
116+
let message = if suggestions.len() == 1 {
117+
"omit the `let` binding"
118+
} else {
119+
"omit the `let` binding and replace variable usages with `()`"
120+
};
121+
diag.multipart_suggestion(message, suggestions, Applicability::MachineApplicable);
106122
},
107123
);
108124
}
109125
}
110126
}
111127

112-
struct UnitVariableCollector {
128+
struct UnitVariableCollector<'a, 'tcx> {
129+
cx: &'a LateContext<'tcx>,
130+
format_args: &'a FormatArgsStorage,
113131
id: HirId,
114-
spans: Vec<rustc_span::Span>,
132+
spans: Vec<MaybeInFormatCapture>,
133+
macro_call: Option<&'a FormatArgs>,
134+
}
135+
136+
enum MaybeInFormatCapture {
137+
Yes,
138+
No(Span),
115139
}
116140

117-
impl UnitVariableCollector {
118-
fn new(id: HirId) -> Self {
119-
Self { id, spans: vec![] }
141+
impl<'a, 'tcx> UnitVariableCollector<'a, 'tcx> {
142+
fn new(cx: &'a LateContext<'tcx>, format_args: &'a FormatArgsStorage, id: HirId) -> Self {
143+
Self {
144+
cx,
145+
format_args,
146+
id,
147+
spans: Vec::new(),
148+
macro_call: None,
149+
}
120150
}
121151
}
122152

123153
/**
124154
* Collect all instances where a variable is used based on its `HirId`.
125155
*/
126-
impl<'tcx> Visitor<'tcx> for UnitVariableCollector {
156+
impl<'tcx> Visitor<'tcx> for UnitVariableCollector<'_, 'tcx> {
127157
fn visit_expr(&mut self, ex: &'tcx Expr<'tcx>) -> Self::Result {
158+
if let Some(macro_call) = root_macro_call_first_node(self.cx, ex)
159+
&& is_format_macro(self.cx, macro_call.def_id)
160+
&& let Some(format_args) = self.format_args.get(self.cx, ex, macro_call.expn)
161+
{
162+
let parent_macro_call = self.macro_call;
163+
self.macro_call = Some(format_args);
164+
walk_expr(self, ex);
165+
self.macro_call = parent_macro_call;
166+
return;
167+
}
168+
128169
if let ExprKind::Path(QPath::Resolved(None, path)) = ex.kind
129170
&& let Res::Local(id) = path.res
130171
&& id == self.id
131172
{
132-
self.spans.push(path.span);
173+
if let Some(macro_call) = self.macro_call
174+
&& macro_call.arguments.all_args().iter().any(|arg| {
175+
matches!(arg.kind, FormatArgumentKind::Captured(_)) && find_format_arg_expr(ex, arg).is_some()
176+
})
177+
{
178+
self.spans.push(MaybeInFormatCapture::Yes);
179+
} else {
180+
self.spans.push(MaybeInFormatCapture::No(path.span));
181+
}
133182
}
134-
rustc_hir::intravisit::walk_expr(self, ex);
183+
184+
walk_expr(self, ex);
135185
}
136186
}
137187

‎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)