Skip to content

Add suggestion to quote inlined format argument as string literal #114507

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
Aug 11, 2023
Merged

Add suggestion to quote inlined format argument as string literal #114507

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
40 changes: 28 additions & 12 deletions compiler/rustc_builtin_macros/src/format.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use rustc_ast::ptr::P;
use rustc_ast::token;
use rustc_ast::tokenstream::TokenStream;
use rustc_ast::{token, StmtKind};
use rustc_ast::{
Expr, ExprKind, FormatAlignment, FormatArgPosition, FormatArgPositionKind, FormatArgs,
FormatArgsPiece, FormatArgument, FormatArgumentKind, FormatArguments, FormatCount,
Expand Down Expand Up @@ -163,25 +163,41 @@ fn make_format_args(

let MacroInput { fmtstr: efmt, mut args, is_direct_literal } = input;

let (fmt_str, fmt_style, fmt_span) = match expr_to_spanned_string(ecx, efmt, msg) {
let (fmt_str, fmt_style, fmt_span) = match expr_to_spanned_string(ecx, efmt.clone(), msg) {
Ok(mut fmt) if append_newline => {
fmt.0 = Symbol::intern(&format!("{}\n", fmt.0));
fmt
}
Ok(fmt) => fmt,
Err(err) => {
if let Some((mut err, suggested)) = err {
let sugg_fmt = match args.explicit_args().len() {
0 => "{}".to_string(),
_ => format!("{}{{}}", "{} ".repeat(args.explicit_args().len())),
};
if !suggested {
err.span_suggestion(
unexpanded_fmt_span.shrink_to_lo(),
"you might be missing a string literal to format with",
format!("\"{sugg_fmt}\", "),
Applicability::MaybeIncorrect,
);
if let ExprKind::Block(block, None) = &efmt.kind
&& block.stmts.len() == 1
&& let StmtKind::Expr(expr) = &block.stmts[0].kind
&& let ExprKind::Path(None, path) = &expr.kind
&& path.is_potential_trivial_const_arg()
{
err.multipart_suggestion(
"quote your inlined format argument to use as string literal",
vec![
(unexpanded_fmt_span.shrink_to_hi(), "\"".to_string()),
(unexpanded_fmt_span.shrink_to_lo(), "\"".to_string()),
],
Applicability::MaybeIncorrect,
Copy link
Contributor

Choose a reason for hiding this comment

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

i can't think of any situations where this suggestion would ever produce code that wouldn't compile. i'm not exactly sure of the exact use cases for Applicability::MachineApplicable and how rigorously provable the suggestions must be, but due to the relatively simplicity of the suggestion and the general unambiguity surrounding the purpose of println! and associated macros, I think its fair to annotate this suggestion as MachineApplicable. let me know if i'm overlooking something obvious though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the original you might be missing a string literal to format with suggestion, I can't think of cases where the suggestion would produce code that wouldn't compile too. Yet, it uses Applicability::MaybeIncorrect. This is why I used Applicability::MaybeIncorrect in this new suggestion. Do you think I should change both to Applicability::MachineApplicable?

Copy link
Member

Choose a reason for hiding this comment

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

I think keeping MaybeIncorrect is probably best for now.

);
} else {
let sugg_fmt = match args.explicit_args().len() {
0 => "{}".to_string(),
_ => format!("{}{{}}", "{} ".repeat(args.explicit_args().len())),
};
err.span_suggestion(
unexpanded_fmt_span.shrink_to_lo(),
"you might be missing a string literal to format with",
format!("\"{sugg_fmt}\", "),
Applicability::MaybeIncorrect,
);
}
}
err.emit();
}
Expand Down
28 changes: 28 additions & 0 deletions tests/ui/fmt/suggest-inline-args.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
mod foo {
pub fn bar() -> i32 {
1
}
}

fn bar() -> i32 {
2
}

fn main() {
let stderr = 3;
eprintln!({stderr});
//~^ ERROR format argument must be a string literal
//~| HELP quote your inlined format argument to use as string literal
eprintln!({1});
//~^ ERROR format argument must be a string literal
//~| HELP you might be missing a string literal to format with
eprintln!({foo::bar()});
//~^ ERROR format argument must be a string literal
//~| HELP you might be missing a string literal to format with
eprintln!({bar()});
//~^ ERROR format argument must be a string literal
//~| HELP you might be missing a string literal to format with
eprintln!({1; 2});
//~^ ERROR format argument must be a string literal
//~| HELP you might be missing a string literal to format with
}
57 changes: 57 additions & 0 deletions tests/ui/fmt/suggest-inline-args.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
error: format argument must be a string literal
--> $DIR/suggest-inline-args.rs:13:15
|
LL | eprintln!({stderr});
| ^^^^^^^^
|
help: quote your inlined format argument to use as string literal
|
LL | eprintln!("{stderr}");
| + +

error: format argument must be a string literal
--> $DIR/suggest-inline-args.rs:16:15
|
LL | eprintln!({1});
| ^^^
|
help: you might be missing a string literal to format with
|
LL | eprintln!("{}", {1});
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: removing the braces when there is only one distinct 'statement' would be nice, if you're up for it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the suggestion! will look into it outside of this PR 😄

| +++++

error: format argument must be a string literal
--> $DIR/suggest-inline-args.rs:19:15
|
LL | eprintln!({foo::bar()});
| ^^^^^^^^^^^^
|
help: you might be missing a string literal to format with
|
LL | eprintln!("{}", {foo::bar()});
| +++++

error: format argument must be a string literal
--> $DIR/suggest-inline-args.rs:22:15
|
LL | eprintln!({bar()});
| ^^^^^^^
|
help: you might be missing a string literal to format with
|
LL | eprintln!("{}", {bar()});
| +++++

error: format argument must be a string literal
--> $DIR/suggest-inline-args.rs:25:15
|
LL | eprintln!({1; 2});
| ^^^^^^
|
help: you might be missing a string literal to format with
|
LL | eprintln!("{}", {1; 2});
| +++++

error: aborting due to 5 previous errors