Skip to content

Commit

Permalink
Auto merge of #11576 - koka831:fix/10128, r=llogiq
Browse files Browse the repository at this point in the history 
write_literal: Fix index of the remaining positional arguments

- fixes #10128
- `clippy --fix` replaces multiple warnings at once
   e.g.)
   ```rust
   writeln!(v, "{0} {1}", "hello", "world");
   // before: `writeln!(v, "hello {1}", "world");`
   // now: `writeln!(v, "hello world");`
   ```

changelog: [`print_literal`], [`write_literal`]: Now handles positional argument properly
  • Loading branch information
@bors
bors committed Sep 28, 2023
2 parents 29ed6fa + b413bf6 commit d18d01a
Show file tree
Hide file tree
Showing 9 changed files with 147 additions and 188 deletions.
94 changes: 66 additions & 28 deletions clippy_lints/src/write.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,15 @@ use clippy_utils::macros::{find_format_args, format_arg_removal_span, root_macro
use clippy_utils::source::{expand_past_previous_comma, snippet_opt};
use clippy_utils::{is_in_cfg_test, is_in_test_function};
use rustc_ast::token::LitKind;
use rustc_ast::{FormatArgPosition, FormatArgs, FormatArgsPiece, FormatOptions, FormatPlaceholder, FormatTrait};
use rustc_ast::{
FormatArgPosition, FormatArgPositionKind, FormatArgs, FormatArgsPiece, FormatOptions, FormatPlaceholder,
FormatTrait,
};
use rustc_errors::Applicability;
use rustc_hir::{Expr, Impl, Item, ItemKind};
use rustc_lint::{LateContext, LateLintPass, LintContext};
use rustc_session::{declare_tool_lint, impl_lint_pass};
use rustc_span::{sym, BytePos};
use rustc_span::{sym, BytePos, Span};

declare_clippy_lint! {
/// ### What it does
Expand Down Expand Up @@ -450,13 +453,25 @@ fn check_empty_string(cx: &LateContext<'_>, format_args: &FormatArgs, macro_call
fn check_literal(cx: &LateContext<'_>, format_args: &FormatArgs, name: &str) {
let arg_index = |argument: &FormatArgPosition| argument.index.unwrap_or_else(|pos| pos);

let lint_name = if name.starts_with("write") {
WRITE_LITERAL
} else {
PRINT_LITERAL
};

let mut counts = vec![0u32; format_args.arguments.all_args().len()];
for piece in &format_args.template {
if let FormatArgsPiece::Placeholder(placeholder) = piece {
counts[arg_index(&placeholder.argument)] += 1;
}
}

let mut suggestion: Vec<(Span, String)> = vec![];
// holds index of replaced positional arguments; used to decrement the index of the remaining
// positional arguments.
let mut replaced_position: Vec<usize> = vec![];
let mut sug_span: Option<Span> = None;

for piece in &format_args.template {
if let FormatArgsPiece::Placeholder(FormatPlaceholder {
argument,
Expand Down Expand Up @@ -493,12 +508,6 @@ fn check_literal(cx: &LateContext<'_>, format_args: &FormatArgs, name: &str) {
_ => continue,
};

let lint = if name.starts_with("write") {
WRITE_LITERAL
} else {
PRINT_LITERAL
};

let Some(format_string_snippet) = snippet_opt(cx, format_args.span) else { continue };
let format_string_is_raw = format_string_snippet.starts_with('r');

Expand All @@ -519,29 +528,58 @@ fn check_literal(cx: &LateContext<'_>, format_args: &FormatArgs, name: &str) {
},
};

span_lint_and_then(
cx,
lint,
arg.expr.span,
"literal with an empty format string",
|diag| {
if let Some(replacement) = replacement
// `format!("{}", "a")`, `format!("{named}", named = "b")
// ~~~~~ ~~~~~~~~~~~~~
&& let Some(removal_span) = format_arg_removal_span(format_args, index)
{
let replacement = replacement.replace('{', "{{").replace('}', "}}");
diag.multipart_suggestion(
"try",
vec![(*placeholder_span, replacement), (removal_span, String::new())],
Applicability::MachineApplicable,
);
}
},
);
sug_span = Some(sug_span.unwrap_or(arg.expr.span).to(arg.expr.span));

if let Some((_, index)) = positional_arg_piece_span(piece) {
replaced_position.push(index);
}

if let Some(replacement) = replacement
// `format!("{}", "a")`, `format!("{named}", named = "b")
// ~~~~~ ~~~~~~~~~~~~~
&& let Some(removal_span) = format_arg_removal_span(format_args, index) {
let replacement = replacement.replace('{', "{{").replace('}', "}}");
suggestion.push((*placeholder_span, replacement));
suggestion.push((removal_span, String::new()));
}
}
}

// Decrement the index of the remaining by the number of replaced positional arguments
if !suggestion.is_empty() {
for piece in &format_args.template {
if let Some((span, index)) = positional_arg_piece_span(piece)
&& suggestion.iter().all(|(s, _)| *s != span) {
let decrement = replaced_position.iter().filter(|i| **i < index).count();
suggestion.push((span, format!("{{{}}}", index.saturating_sub(decrement))));
}
}
}

if let Some(span) = sug_span {
span_lint_and_then(cx, lint_name, span, "literal with an empty format string", |diag| {
if !suggestion.is_empty() {
diag.multipart_suggestion("try", suggestion, Applicability::MachineApplicable);
}
});
}
}

/// Extract Span and its index from the given `piece`, iff it's positional argument.
fn positional_arg_piece_span(piece: &FormatArgsPiece) -> Option<(Span, usize)> {
match piece {
FormatArgsPiece::Placeholder(FormatPlaceholder {
argument:
FormatArgPosition {
index: Ok(index),
kind: FormatArgPositionKind::Number,
..
},
span: Some(span),
..
}) => Some((*span, *index)),
_ => None,
}
}

/// Removes the raw marker, `#`s and quotes from a str, and returns if the literal is raw
Expand Down
4 changes: 0 additions & 4 deletions tests/ui/print_literal.fixed
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,18 +39,14 @@ fn main() {
// throw a warning
println!("hello world");
//~^ ERROR: literal with an empty format string
//~| ERROR: literal with an empty format string
println!("world hello");
//~^ ERROR: literal with an empty format string
//~| ERROR: literal with an empty format string

// named args shouldn't change anything either
println!("hello world");
//~^ ERROR: literal with an empty format string
//~| ERROR: literal with an empty format string
println!("world hello");
//~^ ERROR: literal with an empty format string
//~| ERROR: literal with an empty format string

// The string literal from `file!()` has a callsite span that isn't marked as coming from an
// expansion
Expand Down
4 changes: 0 additions & 4 deletions tests/ui/print_literal.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,18 +39,14 @@ fn main() {
// throw a warning
println!("{0} {1}", "hello", "world");
//~^ ERROR: literal with an empty format string
//~| ERROR: literal with an empty format string
println!("{1} {0}", "hello", "world");
//~^ ERROR: literal with an empty format string
//~| ERROR: literal with an empty format string

// named args shouldn't change anything either
println!("{foo} {bar}", foo = "hello", bar = "world");
//~^ ERROR: literal with an empty format string
//~| ERROR: literal with an empty format string
println!("{bar} {foo}", foo = "hello", bar = "world");
//~^ ERROR: literal with an empty format string
//~| ERROR: literal with an empty format string

// The string literal from `file!()` has a callsite span that isn't marked as coming from an
// expansion
Expand Down
72 changes: 12 additions & 60 deletions tests/ui/print_literal.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,97 +52,49 @@ error: literal with an empty format string
--> $DIR/print_literal.rs:40:25
|
LL | println!("{0} {1}", "hello", "world");
| ^^^^^^^
| ^^^^^^^^^^^^^^^^
|
help: try
|
LL - println!("{0} {1}", "hello", "world");
LL + println!("hello {1}", "world");
LL + println!("hello world");
|

error: literal with an empty format string
--> $DIR/print_literal.rs:40:34
|
LL | println!("{0} {1}", "hello", "world");
| ^^^^^^^
|
help: try
|
LL - println!("{0} {1}", "hello", "world");
LL + println!("{0} world", "hello");
|

error: literal with an empty format string
--> $DIR/print_literal.rs:43:34
--> $DIR/print_literal.rs:42:25
|
LL | println!("{1} {0}", "hello", "world");
| ^^^^^^^
| ^^^^^^^^^^^^^^^^
|
help: try
|
LL - println!("{1} {0}", "hello", "world");
LL + println!("world {0}", "hello");
|

error: literal with an empty format string
--> $DIR/print_literal.rs:43:25
|
LL | println!("{1} {0}", "hello", "world");
| ^^^^^^^
|
help: try
|
LL - println!("{1} {0}", "hello", "world");
LL + println!("{1} hello", "world");
|

error: literal with an empty format string
--> $DIR/print_literal.rs:48:35
|
LL | println!("{foo} {bar}", foo = "hello", bar = "world");
| ^^^^^^^
|
help: try
|
LL - println!("{foo} {bar}", foo = "hello", bar = "world");
LL + println!("hello {bar}", bar = "world");
LL + println!("world hello");
|

error: literal with an empty format string
--> $DIR/print_literal.rs:48:50
--> $DIR/print_literal.rs:46:35
|
LL | println!("{foo} {bar}", foo = "hello", bar = "world");
| ^^^^^^^
| ^^^^^^^^^^^^^^^^^^^^^^
|
help: try
|
LL - println!("{foo} {bar}", foo = "hello", bar = "world");
LL + println!("{foo} world", foo = "hello");
LL + println!("hello world");
|

error: literal with an empty format string
--> $DIR/print_literal.rs:51:50
|
LL | println!("{bar} {foo}", foo = "hello", bar = "world");
| ^^^^^^^
|
help: try
|
LL - println!("{bar} {foo}", foo = "hello", bar = "world");
LL + println!("world {foo}", foo = "hello");
|

error: literal with an empty format string
--> $DIR/print_literal.rs:51:35
--> $DIR/print_literal.rs:48:35
|
LL | println!("{bar} {foo}", foo = "hello", bar = "world");
| ^^^^^^^
| ^^^^^^^^^^^^^^^^^^^^^^
|
help: try
|
LL - println!("{bar} {foo}", foo = "hello", bar = "world");
LL + println!("{bar} hello", bar = "world");
LL + println!("world hello");
|

error: aborting due to 12 previous errors
error: aborting due to 8 previous errors

14 changes: 10 additions & 4 deletions tests/ui/write_literal.fixed
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,16 +43,22 @@ fn main() {
// throw a warning
writeln!(v, "hello world");
//~^ ERROR: literal with an empty format string
//~| ERROR: literal with an empty format string
writeln!(v, "world hello");
//~^ ERROR: literal with an empty format string
//~| ERROR: literal with an empty format string

// named args shouldn't change anything either
writeln!(v, "hello world");
//~^ ERROR: literal with an empty format string
//~| ERROR: literal with an empty format string
writeln!(v, "world hello");
//~^ ERROR: literal with an empty format string
//~| ERROR: literal with an empty format string

// #10128
writeln!(v, "hello {0} world", 2);
//~^ ERROR: literal with an empty format string
writeln!(v, "world {0} hello", 2);
//~^ ERROR: literal with an empty format string
writeln!(v, "hello {0} {1}, {bar}", 2, 3, bar = 4);
//~^ ERROR: literal with an empty format string
writeln!(v, "hello {0} {1}, world {2}", 2, 3, 4);
//~^ ERROR: literal with an empty format string
}
14 changes: 10 additions & 4 deletions tests/ui/write_literal.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,16 +43,22 @@ fn main() {
// throw a warning
writeln!(v, "{0} {1}", "hello", "world");
//~^ ERROR: literal with an empty format string
//~| ERROR: literal with an empty format string
writeln!(v, "{1} {0}", "hello", "world");
//~^ ERROR: literal with an empty format string
//~| ERROR: literal with an empty format string

// named args shouldn't change anything either
writeln!(v, "{foo} {bar}", foo = "hello", bar = "world");
//~^ ERROR: literal with an empty format string
//~| ERROR: literal with an empty format string
writeln!(v, "{bar} {foo}", foo = "hello", bar = "world");
//~^ ERROR: literal with an empty format string
//~| ERROR: literal with an empty format string

// #10128
writeln!(v, "{0} {1} {2}", "hello", 2, "world");
//~^ ERROR: literal with an empty format string
writeln!(v, "{2} {1} {0}", "hello", 2, "world");
//~^ ERROR: literal with an empty format string
writeln!(v, "{0} {1} {2}, {bar}", "hello", 2, 3, bar = 4);
//~^ ERROR: literal with an empty format string
writeln!(v, "{0} {1} {2}, {3} {4}", "hello", 2, 3, "world", 4);
//~^ ERROR: literal with an empty format string
}

0 comments on commit d18d01a

Please sign in to comment.