Skip to content

FormatArgsExpn: Find comma spans and ignore weird proc macro spans #9586

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 5, 2022
Merged

FormatArgsExpn: Find comma spans and ignore weird proc macro spans #9586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 6 additions & 7 deletions clippy_lints/src/format_args.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
use clippy_utils::macros::FormatParamKind::{Implicit, Named, Numbered, Starred};
use clippy_utils::macros::{is_format_macro, FormatArgsExpn, FormatParam, FormatParamUsage};
use clippy_utils::source::{expand_past_previous_comma, snippet_opt};
use clippy_utils::source::snippet_opt;
use clippy_utils::ty::implements_trait;
use clippy_utils::{is_diag_trait_item, meets_msrv, msrvs};
use if_chain::if_chain;
use itertools::Itertools;
use rustc_errors::Applicability;
use rustc_hir::{Expr, ExprKind, HirId, Path, QPath};
use rustc_hir::{Expr, ExprKind, HirId, QPath};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty::adjustment::{Adjust, Adjustment};
use rustc_middle::ty::Ty;
Expand Down Expand Up @@ -169,7 +169,7 @@ fn check_uninlined_args(cx: &LateContext<'_>, args: &FormatArgsExpn<'_>, call_si
// we cannot remove any other arguments in the format string,
// because the index numbers might be wrong after inlining.
// Example of an un-inlinable format: print!("{}{1}", foo, 2)
if !args.params().all(|p| check_one_arg(cx, &p, &mut fixes)) || fixes.is_empty() {
if !args.params().all(|p| check_one_arg(args, &p, &mut fixes)) || fixes.is_empty() {
return;
}

Expand All @@ -196,19 +196,18 @@ fn check_uninlined_args(cx: &LateContext<'_>, args: &FormatArgsExpn<'_>, call_si
);
}

fn check_one_arg(cx: &LateContext<'_>, param: &FormatParam<'_>, fixes: &mut Vec<(Span, String)>) -> bool {
fn check_one_arg(args: &FormatArgsExpn<'_>, param: &FormatParam<'_>, fixes: &mut Vec<(Span, String)>) -> bool {
if matches!(param.kind, Implicit | Starred | Named(_) | Numbered)
&& let ExprKind::Path(QPath::Resolved(None, path)) = param.value.kind
&& let Path { span, segments, .. } = path
&& let [segment] = segments
&& let [segment] = path.segments
&& let Some(arg_span) = args.value_with_prev_comma_span(param.value.hir_id)
{
let replacement = match param.usage {
FormatParamUsage::Argument => segment.ident.name.to_string(),
FormatParamUsage::Width => format!("{}$", segment.ident.name),
FormatParamUsage::Precision => format!(".{}$", segment.ident.name),
};
fixes.push((param.span, replacement));
let arg_span = expand_past_previous_comma(cx, *span);
fixes.push((arg_span, String::new()));
true // successful inlining, continue checking
} else {
Expand Down
6 changes: 3 additions & 3 deletions clippy_lints/src/write.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,11 +475,11 @@ fn check_literal(cx: &LateContext<'_>, format_args: &FormatArgsExpn<'_>, name: &
value.span,
"literal with an empty format string",
|diag| {
if let Some(replacement) = replacement {
if let Some(replacement) = replacement
// `format!("{}", "a")`, `format!("{named}", named = "b")
// ~~~~~ ~~~~~~~~~~~~~
let value_span = expand_past_previous_comma(cx, value.span);

&& let Some(value_span) = format_args.value_with_prev_comma_span(value.hir_id)
{
let replacement = replacement.replace('{', "{{").replace('}', "}}");
diag.multipart_suggestion(
"try this",
Expand Down
109 changes: 104 additions & 5 deletions clippy_utils/src/macros.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use rustc_parse_format::{self as rpf, Alignment};
use rustc_span::def_id::DefId;
use rustc_span::hygiene::{self, MacroKind, SyntaxContext};
use rustc_span::{sym, BytePos, ExpnData, ExpnId, ExpnKind, Pos, Span, SpanData, Symbol};
use std::iter::{once, zip};
use std::ops::ControlFlow;

const FORMAT_MACRO_DIAG_ITEMS: &[Symbol] = &[
Expand Down Expand Up @@ -412,7 +413,8 @@ impl FormatString {
}

struct FormatArgsValues<'tcx> {
/// See `FormatArgsExpn::value_args`
/// Values passed after the format string and implicit captures. `[1, z + 2, x]` for
/// `format!("{x} {} {y}", 1, z + 2)`.
value_args: Vec<&'tcx Expr<'tcx>>,
/// Maps an `rt::v1::Argument::position` or an `rt::v1::Count::Param` to its index in
/// `value_args`
Expand Down Expand Up @@ -765,12 +767,82 @@ pub struct FormatArgsExpn<'tcx> {
/// Has an added newline due to `println!()`/`writeln!()`/etc. The last format string part will
/// include this added newline.
pub newline: bool,
/// Values passed after the format string and implicit captures. `[1, z + 2, x]` for
/// Spans of the commas between the format string and explicit values, excluding any trailing
/// comma
///
/// ```ignore
/// format!("..", 1, 2, 3,)
/// // ^ ^ ^
/// ```
comma_spans: Vec<Span>,
/// Explicit values passed after the format string, ignoring implicit captures. `[1, z + 2]` for
/// `format!("{x} {} {y}", 1, z + 2)`.
value_args: Vec<&'tcx Expr<'tcx>>,
explicit_values: Vec<&'tcx Expr<'tcx>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we may want to clarify this comment -- the above format!(...) doesn't look like valid code.

Copy link
Member Author

@Alexendoo Alexendoo Oct 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah whoops, the {y} should be {}. I also forgot to remove the FIXME if you fancy doing a cleanup PR, otherwise I'll get around to it

}

impl<'tcx> FormatArgsExpn<'tcx> {
/// Gets the spans of the commas inbetween the format string and explicit args, not including
/// any trailing comma
///
/// ```ignore
/// format!("{} {}", a, b)
/// // ^ ^
/// ```
///
/// Ensures that the format string and values aren't coming from a proc macro that sets the
/// output span to that of its input
fn comma_spans(cx: &LateContext<'_>, explicit_values: &[&Expr<'_>], fmt_span: Span) -> Option<Vec<Span>> {
// `format!("{} {} {c}", "one", "two", c = "three")`
// ^^^^^ ^^^^^ ^^^^^^^
let value_spans = explicit_values
.iter()
.map(|val| hygiene::walk_chain(val.span, fmt_span.ctxt()));

// `format!("{} {} {c}", "one", "two", c = "three")`
// ^^ ^^ ^^^^^^
let between_spans = once(fmt_span)
.chain(value_spans)
.tuple_windows()
.map(|(start, end)| start.between(end));

let mut comma_spans = Vec::new();
for between_span in between_spans {
let mut offset = 0;
let mut seen_comma = false;

for token in tokenize(&snippet_opt(cx, between_span)?) {
match token.kind {
TokenKind::LineComment { .. } | TokenKind::BlockComment { .. } | TokenKind::Whitespace => {},
TokenKind::Comma if !seen_comma => {
seen_comma = true;

let base = between_span.data();
comma_spans.push(Span::new(
base.lo + BytePos(offset),
base.lo + BytePos(offset + 1),
base.ctxt,
base.parent,
));
},
// named arguments, `start_val, name = end_val`
// ^^^^^^^^^ between_span
TokenKind::Ident | TokenKind::Eq if seen_comma => {},
// An unexpected token usually indicates the format string or a value came from a proc macro output
// that sets the span of its output to an input, e.g. `println!(some_proc_macro!("input"), ..)` that
// emits a string literal with the span set to that of `"input"`
_ => return None,
}
offset += token.len;
}

if !seen_comma {
return None;
}
}

Some(comma_spans)
}

pub fn parse(cx: &LateContext<'_>, expr: &'tcx Expr<'tcx>) -> Option<Self> {
let macro_name = macro_backtrace(expr.span)
.map(|macro_call| cx.tcx.item_name(macro_call.def_id))
Expand Down Expand Up @@ -845,11 +917,22 @@ impl<'tcx> FormatArgsExpn<'tcx> {
})
.collect::<Option<Vec<_>>>()?;

let mut explicit_values = values.value_args;
// remove values generated for implicitly captured vars
let len = explicit_values
.iter()
.take_while(|val| !format_string.span.contains(val.span))
.count();
explicit_values.truncate(len);

let comma_spans = Self::comma_spans(cx, &explicit_values, format_string.span)?;

Some(Self {
format_string,
args,
value_args: values.value_args,
newline,
comma_spans,
explicit_values,
})
} else {
None
Expand All @@ -875,7 +958,7 @@ impl<'tcx> FormatArgsExpn<'tcx> {

/// Source callsite span of all inputs
pub fn inputs_span(&self) -> Span {
match *self.value_args {
match *self.explicit_values {
[] => self.format_string.span,
[.., last] => self
.format_string
Expand All @@ -884,6 +967,22 @@ impl<'tcx> FormatArgsExpn<'tcx> {
}
}

/// Get the span of a value expanded to the previous comma, e.g. for the value `10`
///
/// ```ignore
/// format("{}.{}", 10, 11)
/// // ^^^^
/// ```
pub fn value_with_prev_comma_span(&self, value_id: HirId) -> Option<Span> {
for (comma_span, value) in zip(&self.comma_spans, &self.explicit_values) {
if value.hir_id == value_id {
return Some(comma_span.to(hygiene::walk_chain(value.span, comma_span.ctxt())));
}
}

None
}

/// Iterator of all format params, both values and those referenced by `width`/`precision`s.
pub fn params(&'tcx self) -> impl Iterator<Item = FormatParam<'tcx>> {
self.args
Expand Down
6 changes: 6 additions & 0 deletions tests/ui/expect_fun_call.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,4 +100,10 @@ fn main() {
let opt_ref = &opt;
opt_ref.unwrap_or_else(|| panic!("{:?}", opt_ref));
}

let format_capture: Option<i32> = None;
format_capture.unwrap_or_else(|| panic!("{error_code}"));

let format_capture_and_value: Option<i32> = None;
format_capture_and_value.unwrap_or_else(|| panic!("{error_code}, {}", 1));
}
6 changes: 6 additions & 0 deletions tests/ui/expect_fun_call.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,4 +100,10 @@ fn main() {
let opt_ref = &opt;
opt_ref.expect(&format!("{:?}", opt_ref));
}

let format_capture: Option<i32> = None;
format_capture.expect(&format!("{error_code}"));

let format_capture_and_value: Option<i32> = None;
format_capture_and_value.expect(&format!("{error_code}, {}", 1));
}
14 changes: 13 additions & 1 deletion tests/ui/expect_fun_call.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,5 +78,17 @@ error: use of `expect` followed by a function call
LL | opt_ref.expect(&format!("{:?}", opt_ref));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| panic!("{:?}", opt_ref))`

error: aborting due to 13 previous errors
error: use of `expect` followed by a function call
--> $DIR/expect_fun_call.rs:105:20
|
LL | format_capture.expect(&format!("{error_code}"));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| panic!("{error_code}"))`

error: use of `expect` followed by a function call
--> $DIR/expect_fun_call.rs:108:30
|
LL | format_capture_and_value.expect(&format!("{error_code}, {}", 1));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| panic!("{error_code}, {}", 1))`

error: aborting due to 15 previous errors

22 changes: 11 additions & 11 deletions tests/ui/uninlined_format_args.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,19 @@
// aux-build:proc_macro_with_span.rs
// run-rustfix
#![feature(custom_inner_attributes)]
#![warn(clippy::uninlined_format_args)]
#![allow(named_arguments_used_positionally, unused_imports, unused_macros, unused_variables)]
#![allow(clippy::eq_op, clippy::format_in_format_args, clippy::print_literal)]

extern crate proc_macro_with_span;
use proc_macro_with_span::with_span;

macro_rules! no_param_str {
() => {
"{}"
};
}

macro_rules! pass_through {
($expr:expr) => {
$expr
};
}

macro_rules! my_println {
($($args:tt),*) => {{
println!($($args),*)
Expand Down Expand Up @@ -140,11 +138,13 @@ fn tester(fn_arg: i32) {
);
println!(no_param_str!(), local_i32);

// FIXME: bugs!
// println!(pass_through!("foo={local_i32}"), local_i32 = local_i32);
// println!(pass_through!("foo={}"), local_i32);
// println!(indoc!("foo={}"), local_i32);
// printdoc!("foo={}", local_i32);
println!(
"{val}",
);
println!("{val}");

println!(with_span!("{0} {1}" "{1} {0}"), local_i32, local_f64);
println!("{}", with_span!(span val));
}

fn main() {
Expand Down
24 changes: 13 additions & 11 deletions tests/ui/uninlined_format_args.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,19 @@
// aux-build:proc_macro_with_span.rs
// run-rustfix
#![feature(custom_inner_attributes)]
#![warn(clippy::uninlined_format_args)]
#![allow(named_arguments_used_positionally, unused_imports, unused_macros, unused_variables)]
#![allow(clippy::eq_op, clippy::format_in_format_args, clippy::print_literal)]

extern crate proc_macro_with_span;
use proc_macro_with_span::with_span;

macro_rules! no_param_str {
() => {
"{}"
};
}

macro_rules! pass_through {
($expr:expr) => {
$expr
};
}

macro_rules! my_println {
($($args:tt),*) => {{
println!($($args),*)
Expand Down Expand Up @@ -143,11 +141,15 @@ fn tester(fn_arg: i32) {
);
println!(no_param_str!(), local_i32);

// FIXME: bugs!
// println!(pass_through!("foo={local_i32}"), local_i32 = local_i32);
// println!(pass_through!("foo={}"), local_i32);
// println!(indoc!("foo={}"), local_i32);
// printdoc!("foo={}", local_i32);
println!(
"{}",
// comment with a comma , in it
val,
);
println!("{}", /* comment with a comma , in it */ val);

println!(with_span!("{0} {1}" "{1} {0}"), local_i32, local_f64);
println!("{}", with_span!(span val));
}

fn main() {
Expand Down
Loading