Skip to content

Commit 43a8c95

Browse files
committed
[WIP] Implement captured vars in format!()
1 parent 818a91e commit 43a8c95

11 files changed

+740
-18
lines changed

CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -3631,6 +3631,7 @@ Released 2018-09-13
36313631
[`inline_asm_x86_att_syntax`]: https://rust-lang.github.io/rust-clippy/master/index.html#inline_asm_x86_att_syntax
36323632
[`inline_asm_x86_intel_syntax`]: https://rust-lang.github.io/rust-clippy/master/index.html#inline_asm_x86_intel_syntax
36333633
[`inline_fn_without_body`]: https://rust-lang.github.io/rust-clippy/master/index.html#inline_fn_without_body
3634+
[`inline_format_args`]: https://rust-lang.github.io/rust-clippy/master/index.html#inline_format_args
36343635
[`inspect_for_each`]: https://rust-lang.github.io/rust-clippy/master/index.html#inspect_for_each
36353636
[`int_plus_one`]: https://rust-lang.github.io/rust-clippy/master/index.html#int_plus_one
36363637
[`integer_arithmetic`]: https://rust-lang.github.io/rust-clippy/master/index.html#integer_arithmetic
+148
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
// #![allow(unused_imports)]
2+
// #![allow(unused_variables)]
3+
4+
// use std::fs::{create_dir_all, OpenOptions};
5+
// use std::io::Write;
6+
7+
use clippy_utils::diagnostics::span_lint_and_then;
8+
use clippy_utils::macros::{FormatArgsArg, FormatArgsExpn, is_format_macro, root_macro_call_first_node};
9+
use clippy_utils::source::{expand_past_previous_comma, snippet};
10+
use if_chain::if_chain;
11+
12+
use rustc_errors::Applicability;
13+
use rustc_hir::{Expr, ExprKind, Path, QPath};
14+
use rustc_hir::def::Res;
15+
use rustc_lint::{LateContext, LateLintPass};
16+
use rustc_session::{declare_lint_pass, declare_tool_lint};
17+
use rustc_span::ExpnData;
18+
19+
declare_clippy_lint! {
20+
/// ### What it does
21+
///
22+
/// ### Why is this bad?
23+
///
24+
/// ### Example
25+
/// ```rust
26+
/// // example code where clippy issues a warning
27+
/// ```
28+
/// Use instead:
29+
/// ```rust
30+
/// // example code which does not raise clippy warning
31+
/// ```
32+
#[clippy::version = "1.64.0"]
33+
pub INLINE_FORMAT_ARGS,
34+
nursery,
35+
"default lint description"
36+
}
37+
declare_lint_pass!(InlineFormatArgs => [INLINE_FORMAT_ARGS]);
38+
39+
impl<'tcx> LateLintPass<'tcx> for InlineFormatArgs {
40+
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
41+
if_chain! {
42+
// TODO: are all these needed? They wre mostly copy/pasted from other places.
43+
if let Some(outer_macro) = root_macro_call_first_node(cx, expr);
44+
if let Some(format_args) = FormatArgsExpn::find_nested(cx, expr, outer_macro.expn);
45+
if !format_args.format_string_span.from_expansion();
46+
let expr_expn_data = expr.span.ctxt().outer_expn_data();
47+
let outermost_expn_data = outermost_expn_data(expr_expn_data);
48+
if let Some(macro_def_id) = outermost_expn_data.macro_def_id;
49+
if is_format_macro(cx, macro_def_id);
50+
// TODO: do we need this?
51+
// if let rustc_span::ExpnKind::Macro(_, name) = outermost_expn_data.kind;
52+
if let Some(args) = format_args.args(cx);
53+
if !args.is_empty();
54+
then {
55+
let mut changes = None;
56+
for (i, arg) in args.iter().enumerate() {
57+
if_chain! {
58+
// If this condition is expensive, may want to move it to the end of this if chain?
59+
if arg.argument_span.is_empty() || snippet(cx, arg.argument_span, "").trim_end().is_empty();
60+
if let ExprKind::Path(QPath::Resolved(None, Path { span, res, segments })) = arg.value.kind;
61+
if segments.len() == 1;
62+
// TODO: do we need this?
63+
if let Res::Local(_local_id) = res;
64+
if !is_aliased(&args, i);
65+
then {
66+
let c = changes.get_or_insert_with(|| vec![]);
67+
// TODO: is it ok to assume segments.len() == 1?
68+
// if not, could use this instead:
69+
// let var_name = snippet(cx, *span, "").trim_end();
70+
// if var_name.is_empty() { continue; }
71+
let var_name = segments[0].ident.name;
72+
c.push((arg.argument_span, var_name.to_string()));
73+
let arg_span = expand_past_previous_comma(cx, *span);
74+
c.push((arg_span, "".to_string()));
75+
}
76+
}
77+
}
78+
79+
if let Some(changes) = changes {
80+
span_lint_and_then(
81+
cx,
82+
INLINE_FORMAT_ARGS,
83+
outer_macro.span,
84+
"REPLACE ME",
85+
|diag| {
86+
diag.multipart_suggestion(
87+
&format!("some interesting message"),
88+
changes,
89+
Applicability::MachineApplicable,
90+
);
91+
},
92+
);
93+
}
94+
95+
// let dumps_dir = "expected";
96+
// create_dir_all(dumps_dir).unwrap();
97+
// let full_str = snippet(cx, outer_macro.span, "panic").to_string()
98+
// .replace("/","--")
99+
// .replace("\t","[TAB]")
100+
// .replace("\n","[NL]")
101+
// .replace("\\","--");
102+
// let filename = format!("{dumps_dir}/{full_str}.txt");
103+
// let mut file = OpenOptions::new()
104+
// .write(true)
105+
// .create(true)
106+
// .truncate(true)
107+
// .open(&filename)
108+
// .unwrap();
109+
// writeln!(file, "NAME {name:?}").unwrap();
110+
// writeln!(file, "FMT {macro_def_id:?}").unwrap();
111+
// for (i, part) in format_args.format_string_parts.iter().enumerate() {
112+
// writeln!(file, "FMT_PART {i} = {:?}", part).unwrap();
113+
// }
114+
// for (i, part) in format_args.formatters.iter().enumerate() {
115+
// writeln!(file, "FORMATTERS {i} = {:?}", part).unwrap();
116+
// }
117+
// for (i, part) in format_args.specs.iter().enumerate() {
118+
// writeln!(file, "SPECS {i} = {:#?}", part).unwrap();
119+
// }
120+
// for (i, arg) in args.iter().enumerate() {
121+
// writeln!(file, "#{i} = TRAIT={:?}, HAS_STRFMT={:?}, ALIAS={:?}, HAS_PRIM_FMT={:?}, SPAN={:?}, ARG_SPAN={:?}, SPEC={:#?}\nVALUE={:#?}",
122+
// arg.format_trait, arg.has_string_formatting(), is_aliased(&args, i), arg.has_primitive_formatting(),
123+
// snippet(cx, arg.span, "<<<..>>>"),
124+
// snippet(cx, arg.argument_span, "<<<..>>>"),
125+
// arg.spec, arg.value).unwrap();
126+
// }
127+
}
128+
}
129+
}
130+
}
131+
132+
// TODO: if this is a common pattern, should this be in utils?
133+
fn outermost_expn_data(expn_data: ExpnData) -> ExpnData {
134+
if expn_data.call_site.from_expansion() {
135+
outermost_expn_data(expn_data.call_site.ctxt().outer_expn_data())
136+
} else {
137+
expn_data
138+
}
139+
}
140+
141+
// Returns true if `args[i]` "refers to" or "is referred to by" another argument.
142+
// TODO: this is not catching cases when the value is (also) used as precision or width
143+
fn is_aliased(args: &[FormatArgsArg<'_>], i: usize) -> bool {
144+
let value = args[i].value;
145+
args.iter()
146+
.enumerate()
147+
.any(|(j, arg)| i != j && std::ptr::eq(value, arg.value))
148+
}

clippy_lints/src/lib.register_lints.rs

+1
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,7 @@ store.register_lints(&[
194194
inherent_to_string::INHERENT_TO_STRING_SHADOW_DISPLAY,
195195
init_numbered_fields::INIT_NUMBERED_FIELDS,
196196
inline_fn_without_body::INLINE_FN_WITHOUT_BODY,
197+
inline_format_args::INLINE_FORMAT_ARGS,
197198
int_plus_one::INT_PLUS_ONE,
198199
invalid_upcast_comparisons::INVALID_UPCAST_COMPARISONS,
199200
invalid_utf8_in_unchecked::INVALID_UTF8_IN_UNCHECKED,

clippy_lints/src/lib.register_nursery.rs

+1
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ store.register_group(true, "clippy::nursery", Some("clippy_nursery"), vec![
1212
LintId::of(floating_point_arithmetic::SUBOPTIMAL_FLOPS),
1313
LintId::of(future_not_send::FUTURE_NOT_SEND),
1414
LintId::of(index_refutable_slice::INDEX_REFUTABLE_SLICE),
15+
LintId::of(inline_format_args::INLINE_FORMAT_ARGS),
1516
LintId::of(let_if_seq::USELESS_LET_IF_SEQ),
1617
LintId::of(methods::ITER_WITH_DRAIN),
1718
LintId::of(missing_const_for_fn::MISSING_CONST_FOR_FN),

clippy_lints/src/lib.rs

+2
Original file line numberDiff line numberDiff line change
@@ -252,6 +252,7 @@ mod inherent_impl;
252252
mod inherent_to_string;
253253
mod init_numbered_fields;
254254
mod inline_fn_without_body;
255+
mod inline_format_args;
255256
mod int_plus_one;
256257
mod invalid_upcast_comparisons;
257258
mod invalid_utf8_in_unchecked;
@@ -916,6 +917,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
916917
store.register_late_pass(move || Box::new(operators::Operators::new(verbose_bit_mask_threshold)));
917918
store.register_late_pass(|| Box::new(invalid_utf8_in_unchecked::InvalidUtf8InUnchecked));
918919
store.register_late_pass(|| Box::new(std_instead_of_core::StdReexports));
920+
store.register_late_pass(|| Box::new(inline_format_args::InlineFormatArgs));
919921
// add lints here, do not remove this comment, it's used in `new_lint`
920922
}
921923

clippy_lints/src/write.rs

+2-9
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
11
use clippy_utils::diagnostics::{span_lint, span_lint_and_then};
22
use clippy_utils::macros::{root_macro_call_first_node, FormatArgsArg, FormatArgsExpn, MacroCall};
3-
use clippy_utils::source::snippet_opt;
3+
use clippy_utils::source::{snippet_opt, expand_past_previous_comma};
44
use rustc_ast::LitKind;
55
use rustc_errors::Applicability;
66
use rustc_hir::{Expr, ExprKind, HirIdMap, Impl, Item, ItemKind};
77
use rustc_lint::{LateContext, LateLintPass, LintContext};
88
use rustc_session::{declare_tool_lint, impl_lint_pass};
9-
use rustc_span::{sym, BytePos, Span};
9+
use rustc_span::{sym, BytePos};
1010

1111
declare_clippy_lint! {
1212
/// ### What it does
@@ -546,10 +546,3 @@ fn conservative_unescape(literal: &str) -> Result<String, UnescapeErr> {
546546

547547
if err { Err(UnescapeErr::Lint) } else { Ok(unescaped) }
548548
}
549-
550-
// Expand from `writeln!(o, "")` to `writeln!(o, "")`
551-
// ^^ ^^^^
552-
fn expand_past_previous_comma(cx: &LateContext<'_>, span: Span) -> Span {
553-
let extended = cx.sess().source_map().span_extend_to_prev_char(span, ',', true);
554-
extended.with_lo(extended.lo() - BytePos(1))
555-
}

clippy_utils/src/macros.rs

+23-9
Original file line numberDiff line numberDiff line change
@@ -479,8 +479,9 @@ impl<'tcx> FormatArgsExpn<'tcx> {
479479
.map(|((value, &(_, format_trait)), span)| FormatArgsArg {
480480
value,
481481
format_trait,
482-
span,
483482
spec: None,
483+
span: span.0,
484+
argument_span: span.1,
484485
})
485486
.collect();
486487
return Some(args);
@@ -500,8 +501,9 @@ impl<'tcx> FormatArgsExpn<'tcx> {
500501
Some(FormatArgsArg {
501502
value: self.value_args[j],
502503
format_trait,
503-
span,
504504
spec: Some(spec),
505+
span: span.0,
506+
argument_span: span.1,
505507
})
506508
} else {
507509
None
@@ -522,25 +524,35 @@ impl<'tcx> FormatArgsExpn<'tcx> {
522524
}
523525

524526
/// Returns the spans covering the `{}`s in the format string
525-
fn format_argument_spans(&self, cx: &LateContext<'tcx>) -> Option<Vec<Span>> {
527+
fn format_argument_spans(&self, cx: &LateContext<'tcx>) -> Option<Vec<(Span, Span)>> {
526528
let format_string = snippet_opt(cx, self.format_string_span)?.into_bytes();
527529
let lo = self.format_string_span.lo();
528530
let get = |i| format_string.get(i).copied();
529531

530532
let mut spans = Vec::new();
531533
let mut i = 0;
532534
let mut arg_start = 0;
535+
let mut spec_start = 0;
533536
loop {
534537
match (get(i), get(i + 1)) {
535538
(Some(b'{'), Some(b'{')) | (Some(b'}'), Some(b'}')) => {
536539
i += 1;
537-
},
540+
}
538541
(Some(b'{'), _) => arg_start = i,
539-
(Some(b'}'), _) => spans.push(
540-
self.format_string_span
541-
.with_lo(lo + BytePos::from_usize(arg_start))
542-
.with_hi(lo + BytePos::from_usize(i + 1)),
543-
),
542+
(Some(b':'), _) => spec_start = i,
543+
(Some(b'}'), _) => {
544+
// If ':' was found for the current argument, spec_start will be > arg_start.
545+
// Note that argument_span may be empty.
546+
let arg_without_spec = if spec_start > arg_start { spec_start } else { i };
547+
spans.push((
548+
self.format_string_span
549+
.with_lo(lo + BytePos::from_usize(arg_start))
550+
.with_hi(lo + BytePos::from_usize(i + 1)),
551+
self.format_string_span
552+
.with_lo(lo + BytePos::from_usize(arg_start + 1))
553+
.with_hi(lo + BytePos::from_usize(arg_without_spec)),
554+
));
555+
}
544556
(None, _) => break,
545557
_ => {},
546558
}
@@ -562,6 +574,8 @@ pub struct FormatArgsArg<'tcx> {
562574
pub spec: Option<&'tcx Expr<'tcx>>,
563575
/// Span of the `{}`
564576
pub span: Span,
577+
/// Span of the "argument" part of the `{arg:...}`. Can be empty.
578+
pub argument_span: Span,
565579
}
566580

567581
impl<'tcx> FormatArgsArg<'tcx> {

clippy_utils/src/source.rs

+10
Original file line numberDiff line numberDiff line change
@@ -410,6 +410,16 @@ pub fn trim_span(sm: &SourceMap, span: Span) -> Span {
410410
.span()
411411
}
412412

413+
/// Expand a span to include a preceding comma
414+
/// ```rust,ignore
415+
/// writeln!(o, "") -> writeln!(o, "")
416+
/// ^^ ^^^^
417+
/// ```
418+
pub fn expand_past_previous_comma(cx: &LateContext<'_>, span: Span) -> Span {
419+
let extended = cx.sess().source_map().span_extend_to_prev_char(span, ',', true);
420+
extended.with_lo(extended.lo() - BytePos(1))
421+
}
422+
413423
#[cfg(test)]
414424
mod test {
415425
use super::{reindent_multiline, without_block_comments};

0 commit comments

Comments
 (0)