Skip to content

Commit d0a919d

Browse files
dswijdswij
authored
Find a shared context for the format string and the format! call (#17243)
Split off from #14724 changelog: none
2 parents 79499c8 + 782225d commit d0a919d

4 files changed

Lines changed: 108 additions & 55 deletions

File tree

‎clippy_lints/src/utils/format_args_collector.rs‎

Lines changed: 99 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -1,25 +1,26 @@
11
use clippy_utils::macros::FormatArgsStorage;
2-
use clippy_utils::source::SpanExt;
3-
use itertools::Itertools;
2+
use clippy_utils::source::{SpanExt, walk_span_to_context};
43
use rustc_ast::{Crate, Expr, ExprKind, FormatArgs};
54
use rustc_data_structures::fx::FxHashMap;
65
use rustc_lexer::{FrontmatterAllowed, TokenKind, tokenize};
7-
use rustc_lint::{EarlyContext, EarlyLintPass};
6+
use rustc_lint::{EarlyContext, EarlyLintPass, LintContext};
87
use rustc_session::impl_lint_pass;
9-
use rustc_span::{Span, hygiene};
10-
use std::iter::once;
8+
use rustc_span::source_map::SourceMap;
9+
use rustc_span::{Span, SpanData};
1110
use std::mem;
1211

1312
/// Populates [`FormatArgsStorage`] with AST [`FormatArgs`] nodes
1413
pub struct FormatArgsCollector {
1514
format_args: FxHashMap<Span, FormatArgs>,
15+
parent_spans: Vec<SpanData>,
1616
storage: FormatArgsStorage,
1717
}
1818

1919
impl FormatArgsCollector {
2020
pub fn new(storage: FormatArgsStorage) -> Self {
2121
Self {
2222
format_args: FxHashMap::default(),
23+
parent_spans: Vec::new(),
2324
storage,
2425
}
2526
}
@@ -30,7 +31,7 @@ impl_lint_pass!(FormatArgsCollector => []);
3031
impl EarlyLintPass for FormatArgsCollector {
3132
fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) {
3233
if let ExprKind::FormatArgs(args) = &expr.kind {
33-
if has_span_from_proc_macro(cx, args) {
34+
if self.has_span_from_external_macro(cx.sess().source_map(), expr.span, args) {
3435
return;
3536
}
3637

@@ -43,53 +44,99 @@ impl EarlyLintPass for FormatArgsCollector {
4344
}
4445
}
4546

46-
/// Detects if the format string or an argument has its span set by a proc macro to something inside
47-
/// a macro callsite, e.g.
48-
///
49-
/// ```ignore
50-
/// println!(some_proc_macro!("input {}"), a);
51-
/// ```
52-
///
53-
/// Where `some_proc_macro` expands to
54-
///
55-
/// ```ignore
56-
/// println!("output {}", a);
57-
/// ```
58-
///
59-
/// But with the span of `"output {}"` set to the macro input
60-
///
61-
/// ```ignore
62-
/// println!(some_proc_macro!("input {}"), a);
63-
/// // ^^^^^^^^^^
64-
/// ```
65-
fn has_span_from_proc_macro(cx: &EarlyContext<'_>, args: &FormatArgs) -> bool {
66-
let ctxt = args.span.ctxt();
47+
impl FormatArgsCollector {
48+
/// Detects if the format string or an argument has its span set by a proc macro to something
49+
/// inside a macro callsite, e.g.
50+
///
51+
/// ```ignore
52+
/// println!(some_proc_macro!("input {}"), a);
53+
/// ```
54+
///
55+
/// Where `some_proc_macro` expands to
56+
///
57+
/// ```ignore
58+
/// println!("output {}", a);
59+
/// ```
60+
///
61+
/// But with the span of `"output {}"` set to the macro input
62+
///
63+
/// ```ignore
64+
/// println!(some_proc_macro!("input {}"), a);
65+
/// // ^^^^^^^^^^
66+
/// ```
67+
fn has_span_from_external_macro(&mut self, sm: &SourceMap, fmt_sp: Span, args: &FormatArgs) -> bool {
68+
let mut fmt_sp = fmt_sp.data();
69+
70+
// Find the first macro call that contains the format string.
71+
let arg_sp = if let Some(arg_sp) = walk_span_to_context(args.span, fmt_sp.ctxt) {
72+
arg_sp.data()
73+
} else {
74+
// Try to find a common parent for the format call and the format string.
75+
self.parent_spans.clear();
76+
// `fmt_sp.ctxt` isn't a parent of the format string so don't add it to the
77+
// search. The first iteration will always run since it can't be the root.
78+
while !fmt_sp.ctxt.is_root() {
79+
fmt_sp = fmt_sp.ctxt.outer_expn_data().call_site.data();
80+
self.parent_spans.push(fmt_sp);
81+
}
82+
let mut arg_sp = args.span.data();
83+
// Note: A parent span will always eventually be found since the root context
84+
// is an ancestor of all contexts.
85+
loop {
86+
match self.parent_spans.iter().find(|s| s.ctxt == arg_sp.ctxt) {
87+
Some(call_sp) if call_sp.lo <= arg_sp.lo && arg_sp.hi <= call_sp.hi => {
88+
fmt_sp = *call_sp;
89+
break arg_sp;
90+
},
91+
// If the string isn't within the call span we some macro stuff we can't
92+
// easily interpret.
93+
Some(_) => return true,
94+
None => arg_sp = arg_sp.ctxt.outer_expn_data().call_site.data(),
95+
}
96+
}
97+
};
98+
if fmt_sp.ctxt.in_external_macro(sm) {
99+
return true;
100+
}
101+
let Some(src) = arg_sp.get_source_range(sm) else {
102+
return true;
103+
};
104+
let Some(src_text) = src.sf.src.as_ref().map(|x| &***x) else {
105+
return true;
106+
};
67107

68-
// `format!("{} {} {c}", "one", "two", c = "three")`
69-
// ^^^^^ ^^^^^ ^^^^^^^
70-
let argument_span = args
71-
.arguments
72-
.explicit_args()
73-
.iter()
74-
.map(|argument| hygiene::walk_chain(argument.expr.span, ctxt));
108+
// Check the spans between the format string and the arguments and between each argument.
109+
args.arguments
110+
.explicit_args()
111+
.iter()
112+
.try_fold(src.range.end, |start, arg| {
113+
let expr_sp = walk_span_to_context(arg.expr.span, fmt_sp.ctxt)?.data();
114+
let expr_start = (expr_sp.lo.0 - src.sf.start_pos.0) as usize;
115+
let expr_end = (expr_sp.hi.0 - src.sf.start_pos.0) as usize;
116+
let mut tks = tokenize(src_text.get(start..expr_start)?, FrontmatterAllowed::No)
117+
.map(|x| x.kind)
118+
.filter(|x| {
119+
!matches!(
120+
x,
121+
TokenKind::LineComment { doc_style: None }
122+
| TokenKind::BlockComment {
123+
doc_style: None,
124+
terminated: true
125+
}
126+
| TokenKind::Whitespace
127+
)
128+
});
75129

76-
// `format!("{} {} {c}", "one", "two", c = "three")`
77-
// ^^ ^^ ^^^^^^
78-
!once(args.span)
79-
.chain(argument_span)
80-
.tuple_windows()
81-
.map(|(start, end)| start.between(end))
82-
.all(|sp| {
83-
sp.check_text(cx, |src| {
84-
// text should be either `, name` or `, name =`
85-
let mut iter = tokenize(src, FrontmatterAllowed::No).filter(|t| {
86-
!matches!(
87-
t.kind,
88-
TokenKind::LineComment { .. } | TokenKind::BlockComment { .. } | TokenKind::Whitespace
89-
)
90-
});
91-
iter.next().is_some_and(|t| matches!(t.kind, TokenKind::Comma))
92-
&& iter.all(|t| matches!(t.kind, TokenKind::Ident | TokenKind::Eq))
130+
// `,` or `, ident =`
131+
let matches = matches!(tks.next(), Some(TokenKind::Comma))
132+
&& match tks.next() {
133+
Some(TokenKind::Ident) => matches!(tks.next(), Some(TokenKind::Eq)),
134+
Some(_) => false,
135+
None => true,
136+
}
137+
&& tks.next().is_none();
138+
matches.then_some(expr_end)
93139
})
94-
})
140+
.is_none()
141+
}
95142
}

‎tests/ui/unnecessary_trailing_comma.fixed‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,9 @@ fn simple() {
2626
println!{"Foo{{,}}"}; //~ unnecessary_trailing_comma
2727
println!{"Foo(,"}; //~ unnecessary_trailing_comma
2828
println!{"Foo[,"}; //~ unnecessary_trailing_comma
29+
println!(concat!("Foo", "=", "{}"), 1); //~ unnecessary_trailing_comma
2930

3031
// This should eventually work, but requires more work
31-
println!(concat!("Foo", "=", "{}"), 1,);
3232
println!("No params", /*"a,){ */);
3333
println!("No params" /* "a,){*/, /*"a,){ */);
3434

‎tests/ui/unnecessary_trailing_comma.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,9 @@ fn simple() {
2626
println!{"Foo{{,}}", }; //~ unnecessary_trailing_comma
2727
println!{"Foo(,", }; //~ unnecessary_trailing_comma
2828
println!{"Foo[,", }; //~ unnecessary_trailing_comma
29+
println!(concat!("Foo", "=", "{}"), 1,); //~ unnecessary_trailing_comma
2930

3031
// This should eventually work, but requires more work
31-
println!(concat!("Foo", "=", "{}"), 1,);
3232
println!("No params", /*"a,){ */);
3333
println!("No params" /* "a,){*/, /*"a,){ */);
3434

‎tests/ui/unnecessary_trailing_comma.stderr‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,5 +115,11 @@ error: unnecessary trailing comma
115115
LL | println!{"Foo[,", };
116116
| ^^ help: remove the trailing comma
117117

118-
error: aborting due to 19 previous errors
118+
error: unnecessary trailing comma
119+
--> tests/ui/unnecessary_trailing_comma.rs:29:42
120+
|
121+
LL | println!(concat!("Foo", "=", "{}"), 1,);
122+
| ^ help: remove the trailing comma
123+
124+
error: aborting due to 20 previous errors
119125

0 commit comments

Comments
 (0)