Skip to content
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.

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

Add suggestion for captured and passed arg case in fmt macro #105635

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
81 changes: 78 additions & 3 deletions compiler/rustc_builtin_macros/src/format.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use parse::Piece::NextArgument;
use parse::Position::ArgumentNamed;
use rustc_ast::ptr::P;
use rustc_ast::token;
use rustc_ast::tokenstream::TokenStream;
Expand Down Expand Up @@ -34,6 +36,14 @@ enum PositionUsedAs {
Precision,
Width,
}

#[derive(Clone, Copy, Debug)]
struct RedundantArgDiagBuilder<'a> {
arg_span: Span,
fmt_span: InnerSpan,
fmt_str: &'a str,
}

use PositionUsedAs::*;

struct MacroInput {
Expand Down Expand Up @@ -335,8 +345,8 @@ fn make_format_args(
let mut unfinished_literal = String::new();
let mut placeholder_index = 0;

for piece in pieces {
match piece {
for piece in &pieces {
match *piece {
parse::Piece::String(s) => {
unfinished_literal.push_str(s);
}
Expand Down Expand Up @@ -470,6 +480,20 @@ fn make_format_args(
report_invalid_references(ecx, &invalid_refs, &template, fmt_span, &args, parser);
}

// Vec that contains all redundant arg, will be printed for suggestion
let mut redundant_expl_args = Vec::new();

// `placeholder` contains pieces that may be linked to one of redundant explicit argument
let _placeholders: Vec<&parse::Piece<'_>> = pieces
.iter()
.filter(|p| {
if let NextArgument(arg) = *p && let ArgumentNamed(_) = arg.position {
return true;
}
false
})
.collect();

let unused = used
.iter()
.enumerate()
Expand All @@ -478,6 +502,25 @@ fn make_format_args(
let msg = if let FormatArgumentKind::Named(_) = args.explicit_args()[i].kind {
"named argument never used"
} else {
if let Some(expr) = args.explicit_args()[i].expr.to_ty()
&& let Some(symbol) = expr.kind.is_simple_path() {
for piece in &pieces {
if let NextArgument(arg) = piece
&& let ArgumentNamed(specifier) = arg.position {
if specifier == symbol.to_string() {
redundant_expl_args.push(
RedundantArgDiagBuilder {
arg_span: expr.span,
fmt_span: InnerSpan {
start: arg.position_span.start,
end: arg.position_span.end,
},
fmt_str: specifier,
});
}
}
}
}
"argument never used"
};
(args.explicit_args()[i].expr.span, msg)
Expand All @@ -488,7 +531,15 @@ fn make_format_args(
// If there's a lot of unused arguments,
// let's check if this format arguments looks like another syntax (printf / shell).
let detect_foreign_fmt = unused.len() > args.explicit_args().len() / 2;
report_missing_placeholders(ecx, unused, detect_foreign_fmt, str_style, fmt_str, fmt_span);
report_missing_placeholders(
ecx,
unused,
redundant_expl_args,
detect_foreign_fmt,
str_style,
fmt_str,
fmt_span,
);
}

// Only check for unused named argument names if there are no other errors to avoid causing
Expand Down Expand Up @@ -571,6 +622,7 @@ fn invalid_placeholder_type_error(
fn report_missing_placeholders(
ecx: &mut ExtCtxt<'_>,
unused: Vec<(Span, &str)>,
redundant_expl_args: Vec<RedundantArgDiagBuilder<'_>>,
detect_foreign_fmt: bool,
str_style: Option<usize>,
fmt_str: &str,
Expand Down Expand Up @@ -676,6 +728,29 @@ fn report_missing_placeholders(
diag.span_label(fmt_span, "formatting specifier missing");
}

// Diag message buileder
if !redundant_expl_args.is_empty() {
let mut fmt_spans = Vec::new();
let mut arg_spans = Vec::new();
let mut arg = Vec::new();
for rd in &redundant_expl_args {
fmt_spans.push(Span::from_inner(fmt_span, rd.fmt_span));
arg_spans.push(rd.arg_span);
arg.push(rd.fmt_str);
}
let mut m_span: MultiSpan = fmt_spans.clone().into();
for i in 0..fmt_spans.len() {
m_span.push_span_label(
fmt_spans[i],
format!("this formatting argument captures `{}` directly", arg[i]),
);
}
for arg_span in arg_spans {
m_span.push_span_label(arg_span, "this can be removed".to_string());
}
diag.span_help(m_span, "the formatting string captures that binding directly, it doesn't need to be included in the argument list");
Copy link
Contributor

@estebank estebank Jan 14, 2023

Choose a reason for hiding this comment

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

We should customize the output depending on the number of bindings:

Suggested change
diag.span_help(m_span, "the formatting string captures that binding directly, it doesn't need to be included in the argument list");
let mut bindings: Vec<_> = arg.iter().collect();
bindings.sort();
bindings.dedup();
diag.span_help(
m_span,
&format!(
"the formatting string captures {} directly, {} need to \
be included in the argument list",
match &bindings[..] {
[.., last] if bindings.len() < 8 => format!("bindings `{}` and `{last}`"),
[binding] => format!("binding {binding}"),
_ => "these bindings".to_string(),
},
if bindings.len() == 1 {
"it doesn't"
} else {
"they don't"
},
);

Copy link
Contributor

Choose a reason for hiding this comment

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

This request is still valid :)

You can choose to simplify it, but at least I'd like to specialize the output for a single binding as opposed to multiple.

}

diag.emit();
}

Expand Down
16 changes: 16 additions & 0 deletions tests/ui/macros/issue-105225.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
fn main() {
let x = 10;
println!("{x}", x);
//~^ ERROR argument never used
let y = 20;
println!("{x} {y}", x, y);
//~^ ERROR multiple unused formatting arguments
println!("{x} {y}", y, x);
//~^ ERROR multiple unused formatting arguments
println!("{} {y}", x, y);
//~^ ERROR argument never used
println!("{} {} {y} {} {}", y, y, y, y, y);
//~^ ERROR argument never used
println!("{x} {x} {y} {x} {x}", x, y, x, y, y);
//~^ ERROR argument never used
}
107 changes: 107 additions & 0 deletions tests/ui/macros/issue-105225.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
error: argument never used
--> $DIR/issue-105225.rs:3:21
|
LL | println!("{x}", x);
| ----- ^ argument never used
| |
| formatting specifier missing
|
help: the formatting string captures that binding directly, it doesn't need to be included in the argument list
--> $DIR/issue-105225.rs:3:16
|
LL | println!("{x}", x);
| ^ - this can be removed
| |
| this formatting argument captures `x` directly

error: multiple unused formatting arguments
--> $DIR/issue-105225.rs:6:25
|
LL | println!("{x} {y}", x, y);
| --------- ^ ^ argument never used
| | |
| | argument never used
| multiple missing formatting specifiers
|
help: the formatting string captures that binding directly, it doesn't need to be included in the argument list
--> $DIR/issue-105225.rs:6:16
|
LL | println!("{x} {y}", x, y);
| ^ ^ - - this can be removed
| | | |
| | | this can be removed
| | this formatting argument captures `y` directly
| this formatting argument captures `x` directly

error: multiple unused formatting arguments
--> $DIR/issue-105225.rs:8:25
|
LL | println!("{x} {y}", y, x);
| --------- ^ ^ argument never used
| | |
| | argument never used
| multiple missing formatting specifiers
|
help: the formatting string captures that binding directly, it doesn't need to be included in the argument list
--> $DIR/issue-105225.rs:8:16
|
LL | println!("{x} {y}", y, x);
| ^ ^ - - this can be removed
| | | |
| | | this can be removed
| | this formatting argument captures `y` directly
| this formatting argument captures `x` directly

error: argument never used
--> $DIR/issue-105225.rs:10:27
|
LL | println!("{} {y}", x, y);
| -------- ^ argument never used
| |
| formatting specifier missing
|
help: the formatting string captures that binding directly, it doesn't need to be included in the argument list
--> $DIR/issue-105225.rs:10:19
|
LL | println!("{} {y}", x, y);
| ^ - this can be removed
| |
| this formatting argument captures `y` directly

error: argument never used
--> $DIR/issue-105225.rs:12:45
|
LL | println!("{} {} {y} {} {}", y, y, y, y, y);
| ----------------- ^ argument never used
| |
| formatting specifier missing
|
help: the formatting string captures that binding directly, it doesn't need to be included in the argument list
--> $DIR/issue-105225.rs:12:22
|
LL | println!("{} {} {y} {} {}", y, y, y, y, y);
| ^ - this can be removed
| |
| this formatting argument captures `y` directly

error: multiple unused formatting arguments
--> new.rs:14:37
|
14 | println!("{x} {x} {y} {x} {x}", x, y, x, y, y);
| --------------------- ^ ^ ^ ^ ^ argument never used
| | | | | |
| | | | | argument never used
| | | | argument never used
| | | argument never used
| | argument never used
| multiple missing formatting specifiers
|
help: the formatting string captures that binding directly, it doesn't need to be included in the argument list
--> new.rs:14:16
|
LL | println!("{x} {x} {y} {x} {x}", x, y, x, y, y);
| ^ - this can be removed
| |
| this formatting argument captures `y` directly
error: aborting due to 6 previous errors