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.

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

Fix #7903 #7906

Merged
merged 1 commit into from
Nov 2, 2021
Merged

Fix #7903 #7906

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
20 changes: 15 additions & 5 deletions clippy_utils/src/higher.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
//! This module contains functions that retrieves specifiec elements.
//! This module contains functions that retrieve specific elements.

#![deny(clippy::missing_docs_in_private_items)]

use crate::ty::is_type_diagnostic_item;
use crate::{is_expn_of, last_path_segment, match_def_path, paths};
use crate::{is_expn_of, last_path_segment, match_def_path, path_to_local_id, paths};
use if_chain::if_chain;
use rustc_ast::ast::{self, LitKind};
use rustc_hir as hir;
use rustc_hir::{
Arm, Block, BorrowKind, Expr, ExprKind, HirId, LoopSource, MatchSource, Node, Pat, QPath, StmtKind, UnOp,
Arm, Block, BorrowKind, Expr, ExprKind, HirId, LoopSource, MatchSource, Node, Pat, PatKind, QPath, StmtKind, UnOp,
};
use rustc_lint::LateContext;
use rustc_span::{sym, symbol, ExpnKind, Span, Symbol};
Expand Down Expand Up @@ -513,6 +513,8 @@ pub struct FormatArgsExpn<'tcx> {
pub format_string_parts: &'tcx [Expr<'tcx>],
/// Symbols corresponding to [`Self::format_string_parts`]
pub format_string_symbols: Vec<Symbol>,
/// Match arm patterns, the `arg0`, etc. from the next field `args`
pub arg_names: &'tcx [Pat<'tcx>],
/// Expressions like `ArgumentV1::new(arg0, Debug::fmt)`
pub args: &'tcx [Expr<'tcx>],
/// The final argument passed to `Arguments::new_v1_formatted`, if applicable
Expand Down Expand Up @@ -557,13 +559,15 @@ impl FormatArgsExpn<'tcx> {
_ => None,
})
.collect();
if let PatKind::Tuple(arg_names, None) = arm.pat.kind;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is okay for now since it fixes an ICE, but note that we want to move away from depending on the exact HIR per #7843.

if let ExprKind::Array(args) = arm.body.kind;
then {
Some(FormatArgsExpn {
format_string_span: strs_ref.span,
value_args,
format_string_parts,
format_string_symbols,
arg_names,
args,
fmt_expr,
})
Expand All @@ -587,9 +591,15 @@ impl FormatArgsExpn<'tcx> {
if let Some(position_field) = fields.iter().find(|f| f.ident.name == sym::position);
if let ExprKind::Lit(lit) = &position_field.expr.kind;
if let LitKind::Int(position, _) = lit.node;
if let Ok(i) = usize::try_from(position);
let arg = &self.args[i];
if let ExprKind::Call(_, [arg_name, _]) = arg.kind;
if let Some(j) = self
.arg_names
.iter()
.position(|pat| path_to_local_id(arg_name, pat.hir_id));
then {
let i = usize::try_from(position).unwrap();
Some(FormatArgsArg { value: self.value_args[i], arg: &self.args[i], fmt: Some(fmt) })
Some(FormatArgsArg { value: self.value_args[j], arg, fmt: Some(fmt) })
} else {
None
}
Expand Down
12 changes: 12 additions & 0 deletions tests/ui/format_args.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#![allow(unused_variables)]
#![allow(clippy::assertions_on_constants)]
#![allow(clippy::eq_op)]
#![allow(clippy::print_literal)]
#![warn(clippy::to_string_in_format_args)]

use std::io::{stdout, Write};
Expand Down Expand Up @@ -97,9 +98,20 @@ fn main() {
println!("{}", Z(1));
println!("{}", **x);
println!("{}", ***x_ref);
// https://github.com/rust-lang/rust-clippy/issues/7903
println!("{foo}{bar}", foo = "foo", bar = "bar");
println!("{foo}{bar}", foo = "foo", bar = "bar");
println!("{foo}{bar}", bar = "bar", foo = "foo");
println!("{foo}{bar}", bar = "bar", foo = "foo");

// negative tests
println!("error: something failed at {}", Somewhere.to_string());
// The next two tests are negative because caching the string might be faster than calling `<X as
// Display>::fmt` twice.
println!("{} and again {0}", x.to_string());
println!("{foo}{foo}", foo = "foo".to_string());
my_macro!();
println!("error: something failed at {}", my_other_macro!());
// https://github.com/rust-lang/rust-clippy/issues/7903
println!("{foo}{foo:?}", foo = "foo".to_string());
}
12 changes: 12 additions & 0 deletions tests/ui/format_args.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#![allow(unused_variables)]
#![allow(clippy::assertions_on_constants)]
#![allow(clippy::eq_op)]
#![allow(clippy::print_literal)]
#![warn(clippy::to_string_in_format_args)]

use std::io::{stdout, Write};
Expand Down Expand Up @@ -97,9 +98,20 @@ fn main() {
println!("{}", Z(1).to_string());
println!("{}", x.to_string());
println!("{}", x_ref.to_string());
// https://github.com/rust-lang/rust-clippy/issues/7903
println!("{foo}{bar}", foo = "foo".to_string(), bar = "bar");
println!("{foo}{bar}", foo = "foo", bar = "bar".to_string());
println!("{foo}{bar}", bar = "bar".to_string(), foo = "foo");
println!("{foo}{bar}", bar = "bar", foo = "foo".to_string());

// negative tests
println!("error: something failed at {}", Somewhere.to_string());
// The next two tests are negative because caching the string might be faster than calling `<X as
// Display>::fmt` twice.
println!("{} and again {0}", x.to_string());
Copy link
Contributor

Choose a reason for hiding this comment

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

See my other comment on this. Comments would be helpful here. One to separate // negative tests and one to explain the intent behind this test since it isn't obvious.

println!("{foo}{foo}", foo = "foo".to_string());
my_macro!();
println!("error: something failed at {}", my_other_macro!());
// https://github.com/rust-lang/rust-clippy/issues/7903
println!("{foo}{foo:?}", foo = "foo".to_string());
}
60 changes: 42 additions & 18 deletions tests/ui/format_args.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,106 +1,130 @@
error: `to_string` applied to a type that implements `Display` in `format!` args
--> $DIR/format_args.rs:75:72
--> $DIR/format_args.rs:76:72
|
LL | let _ = format!("error: something failed at {}", Location::caller().to_string());
| ^^^^^^^^^^^^ help: remove this
|
= note: `-D clippy::to-string-in-format-args` implied by `-D warnings`

error: `to_string` applied to a type that implements `Display` in `write!` args
--> $DIR/format_args.rs:79:27
--> $DIR/format_args.rs:80:27
|
LL | Location::caller().to_string()
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `writeln!` args
--> $DIR/format_args.rs:84:27
--> $DIR/format_args.rs:85:27
|
LL | Location::caller().to_string()
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `print!` args
--> $DIR/format_args.rs:86:63
--> $DIR/format_args.rs:87:63
|
LL | print!("error: something failed at {}", Location::caller().to_string());
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `println!` args
--> $DIR/format_args.rs:87:65
--> $DIR/format_args.rs:88:65
|
LL | println!("error: something failed at {}", Location::caller().to_string());
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `eprint!` args
--> $DIR/format_args.rs:88:64
--> $DIR/format_args.rs:89:64
|
LL | eprint!("error: something failed at {}", Location::caller().to_string());
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `eprintln!` args
--> $DIR/format_args.rs:89:66
--> $DIR/format_args.rs:90:66
|
LL | eprintln!("error: something failed at {}", Location::caller().to_string());
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `format_args!` args
--> $DIR/format_args.rs:90:77
--> $DIR/format_args.rs:91:77
|
LL | let _ = format_args!("error: something failed at {}", Location::caller().to_string());
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `assert!` args
--> $DIR/format_args.rs:91:70
--> $DIR/format_args.rs:92:70
|
LL | assert!(true, "error: something failed at {}", Location::caller().to_string());
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `assert_eq!` args
--> $DIR/format_args.rs:92:73
--> $DIR/format_args.rs:93:73
|
LL | assert_eq!(0, 0, "error: something failed at {}", Location::caller().to_string());
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `assert_ne!` args
--> $DIR/format_args.rs:93:73
--> $DIR/format_args.rs:94:73
|
LL | assert_ne!(0, 0, "error: something failed at {}", Location::caller().to_string());
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `panic!` args
--> $DIR/format_args.rs:94:63
--> $DIR/format_args.rs:95:63
|
LL | panic!("error: something failed at {}", Location::caller().to_string());
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `println!` args
--> $DIR/format_args.rs:95:20
--> $DIR/format_args.rs:96:20
|
LL | println!("{}", X(1).to_string());
| ^^^^^^^^^^^^^^^^ help: use this: `*X(1)`

error: `to_string` applied to a type that implements `Display` in `println!` args
--> $DIR/format_args.rs:96:20
--> $DIR/format_args.rs:97:20
|
LL | println!("{}", Y(&X(1)).to_string());
| ^^^^^^^^^^^^^^^^^^^^ help: use this: `***Y(&X(1))`

error: `to_string` applied to a type that implements `Display` in `println!` args
--> $DIR/format_args.rs:97:24
--> $DIR/format_args.rs:98:24
|
LL | println!("{}", Z(1).to_string());
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `println!` args
--> $DIR/format_args.rs:98:20
--> $DIR/format_args.rs:99:20
|
LL | println!("{}", x.to_string());
| ^^^^^^^^^^^^^ help: use this: `**x`

error: `to_string` applied to a type that implements `Display` in `println!` args
--> $DIR/format_args.rs:99:20
--> $DIR/format_args.rs:100:20
|
LL | println!("{}", x_ref.to_string());
| ^^^^^^^^^^^^^^^^^ help: use this: `***x_ref`

error: aborting due to 17 previous errors
error: `to_string` applied to a type that implements `Display` in `println!` args
--> $DIR/format_args.rs:102:39
|
LL | println!("{foo}{bar}", foo = "foo".to_string(), bar = "bar");
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `println!` args
--> $DIR/format_args.rs:103:52
|
LL | println!("{foo}{bar}", foo = "foo", bar = "bar".to_string());
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `println!` args
--> $DIR/format_args.rs:104:39
|
LL | println!("{foo}{bar}", bar = "bar".to_string(), foo = "foo");
| ^^^^^^^^^^^^ help: remove this

error: `to_string` applied to a type that implements `Display` in `println!` args
--> $DIR/format_args.rs:105:52
|
LL | println!("{foo}{bar}", bar = "bar", foo = "foo".to_string());
| ^^^^^^^^^^^^ help: remove this

error: aborting due to 21 previous errors

1 change: 1 addition & 0 deletions tests/ui/format_args_unfixable.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ fn main() {
assert_ne!(0, 0, "error: {}", format!("something failed at {}", Location::caller()));
panic!("error: {}", format!("something failed at {}", Location::caller()));

// negative tests
println!("error: {}", format_args!("something failed at {}", Location::caller()));
println!("error: {:>70}", format!("something failed at {}", Location::caller()));
println!("error: {} {0}", format!("something failed at {}", Location::caller()));
Expand Down