From 0182a6640e27aacd11782e3d4344c4564c1f8bc8 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Sun, 24 Feb 2019 07:30:08 +0200 Subject: [PATCH] Fix `useless_format` suggestions --- ci/base-tests.sh | 2 +- clippy_lints/src/format.rs | 40 ++++++++++++++----------- tests/ui/format.fixed | 60 ++++++++++++++++++++++++++++++++++++++ tests/ui/format.rs | 2 ++ tests/ui/format.stderr | 52 ++++++++++++--------------------- 5 files changed, 104 insertions(+), 52 deletions(-) create mode 100644 tests/ui/format.fixed diff --git a/ci/base-tests.sh b/ci/base-tests.sh index 9af3c3a335d..ac928645487 100755 --- a/ci/base-tests.sh +++ b/ci/base-tests.sh @@ -59,7 +59,7 @@ rustup override set nightly # avoid loop spam and allow cmds with exit status != 0 set +ex -for file in `find tests -not -path "tests/ui/format.rs" -not -path "tests/ui/formatting.rs" -not -path "tests/ui/empty_line_after_outer_attribute.rs" -not -path "tests/ui/double_parens.rs" -not -path "tests/ui/doc.rs" -not -path "tests/ui/unused_unit.rs" | grep "\.rs$"` ; do +for file in `find tests -not -path "tests/ui/formatting.rs" -not -path "tests/ui/empty_line_after_outer_attribute.rs" -not -path "tests/ui/double_parens.rs" -not -path "tests/ui/doc.rs" -not -path "tests/ui/unused_unit.rs" | grep "\.rs$"` ; do rustfmt ${file} --check if [ $? -ne 0 ]; then echo "${file} needs reformatting!" diff --git a/clippy_lints/src/format.rs b/clippy_lints/src/format.rs index aaef5b39aeb..ba956b77f46 100644 --- a/clippy_lints/src/format.rs +++ b/clippy_lints/src/format.rs @@ -5,11 +5,12 @@ }; use if_chain::if_chain; use rustc::hir::*; -use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; +use rustc::lint::{LateContext, LateLintPass, LintArray, LintContext, LintPass}; use rustc::ty; use rustc::{declare_tool_lint, lint_array}; use rustc_errors::Applicability; use syntax::ast::LitKind; +use syntax::source_map::Span; /// **What it does:** Checks for the use of `format!("string literal with no /// argument")` and `format!("{}", foo)` where `foo` is a string. @@ -82,14 +83,7 @@ fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { } }; - span_lint_and_then(cx, USELESS_FORMAT, span, "useless use of `format!`", |db| { - db.span_suggestion( - expr.span, - message, - sugg, - Applicability::MachineApplicable, - ); - }); + span_useless_format(cx, span, message, sugg); } } }, @@ -98,14 +92,7 @@ fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { if let ExprKind::Tup(ref tup) = matchee.node { if tup.is_empty() { let sugg = format!("{}.to_string()", snippet(cx, expr.span, "").into_owned()); - span_lint_and_then(cx, USELESS_FORMAT, span, "useless use of `format!`", |db| { - db.span_suggestion( - span, - "consider using .to_string()", - sugg, - Applicability::MachineApplicable, // snippet - ); - }); + span_useless_format(cx, span, "consider using .to_string()", sugg); } } }, @@ -115,6 +102,25 @@ fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { } } +fn span_useless_format<'a, 'tcx: 'a, T: LintContext<'tcx>>(cx: &'a T, span: Span, help: &str, mut sugg: String) { + let to_replace = span.source_callsite(); + + // The callsite span contains the statement semicolon for some reason. + let snippet = snippet(cx, to_replace, ".."); + if snippet.ends_with(';') { + sugg.push(';'); + } + + span_lint_and_then(cx, USELESS_FORMAT, span, "useless use of `format!`", |db| { + db.span_suggestion( + to_replace, + help, + sugg, + Applicability::MachineApplicable, // snippet + ); + }); +} + /// Checks if the expressions matches `&[""]` fn check_single_piece(expr: &Expr) -> bool { if_chain! { diff --git a/tests/ui/format.fixed b/tests/ui/format.fixed new file mode 100644 index 00000000000..2200552430a --- /dev/null +++ b/tests/ui/format.fixed @@ -0,0 +1,60 @@ +// run-rustfix + +#![allow(clippy::print_literal)] +#![warn(clippy::useless_format)] + +struct Foo(pub String); + +macro_rules! foo { + ($($t:tt)*) => (Foo(format!($($t)*))) +} + +fn main() { + "foo".to_string(); + + "foo".to_string(); + format!("{:?}", "foo"); // don't warn about debug + format!("{:8}", "foo"); + format!("{:width$}", "foo", width = 8); + "foo".to_string(); // warn when the format makes no difference + "foo".to_string(); // warn when the format makes no difference + format!("foo {}", "bar"); + format!("{} bar", "foo"); + + let arg: String = "".to_owned(); + arg.to_string(); + format!("{:?}", arg); // don't warn about debug + format!("{:8}", arg); + format!("{:width$}", arg, width = 8); + arg.to_string(); // warn when the format makes no difference + arg.to_string(); // warn when the format makes no difference + format!("foo {}", arg); + format!("{} bar", arg); + + // we don’t want to warn for non-string args, see #697 + format!("{}", 42); + format!("{:?}", 42); + format!("{:+}", 42); + format!("foo {}", 42); + format!("{} bar", 42); + + // we only want to warn about `format!` itself + println!("foo"); + println!("{}", "foo"); + println!("foo {}", "foo"); + println!("{}", 42); + println!("foo {}", 42); + + // A format! inside a macro should not trigger a warning + foo!("should not warn"); + + // precision on string means slicing without panicking on size: + format!("{:.1}", "foo"); // could be "foo"[..1] + format!("{:.10}", "foo"); // could not be "foo"[..10] + format!("{:.prec$}", "foo", prec = 1); + format!("{:.prec$}", "foo", prec = 10); + + 42.to_string(); + let x = std::path::PathBuf::from("/bar/foo/qux"); + x.display().to_string(); +} diff --git a/tests/ui/format.rs b/tests/ui/format.rs index f2892c5b84a..e974a4957ea 100644 --- a/tests/ui/format.rs +++ b/tests/ui/format.rs @@ -1,3 +1,5 @@ +// run-rustfix + #![allow(clippy::print_literal)] #![warn(clippy::useless_format)] diff --git a/tests/ui/format.stderr b/tests/ui/format.stderr index d5f2711bb37..236ac74b2e9 100644 --- a/tests/ui/format.stderr +++ b/tests/ui/format.stderr @@ -1,74 +1,58 @@ error: useless use of `format!` - --> $DIR/format.rs:11:5 + --> $DIR/format.rs:13:5 | LL | format!("foo"); - | ^^^^^^^^^^^^^^^ help: consider using .to_string(): `"foo".to_string()` + | ^^^^^^^^^^^^^^^ help: consider using .to_string(): `"foo".to_string();` | = note: `-D clippy::useless-format` implied by `-D warnings` error: useless use of `format!` - --> $DIR/format.rs:13:5 + --> $DIR/format.rs:15:5 | LL | format!("{}", "foo"); - | ^^^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `"foo".to_string()` - | - = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) + | ^^^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `"foo".to_string();` error: useless use of `format!` - --> $DIR/format.rs:17:5 + --> $DIR/format.rs:19:5 | LL | format!("{:+}", "foo"); // warn when the format makes no difference - | ^^^^^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `"foo".to_string()` - | - = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) + | ^^^^^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `"foo".to_string();` error: useless use of `format!` - --> $DIR/format.rs:18:5 + --> $DIR/format.rs:20:5 | LL | format!("{:<}", "foo"); // warn when the format makes no difference - | ^^^^^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `"foo".to_string()` - | - = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) + | ^^^^^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `"foo".to_string();` error: useless use of `format!` - --> $DIR/format.rs:23:5 + --> $DIR/format.rs:25:5 | LL | format!("{}", arg); - | ^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `arg.to_string()` - | - = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) + | ^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `arg.to_string();` error: useless use of `format!` - --> $DIR/format.rs:27:5 + --> $DIR/format.rs:29:5 | LL | format!("{:+}", arg); // warn when the format makes no difference - | ^^^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `arg.to_string()` - | - = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) + | ^^^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `arg.to_string();` error: useless use of `format!` - --> $DIR/format.rs:28:5 + --> $DIR/format.rs:30:5 | LL | format!("{:<}", arg); // warn when the format makes no difference - | ^^^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `arg.to_string()` - | - = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) + | ^^^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `arg.to_string();` error: useless use of `format!` - --> $DIR/format.rs:55:5 + --> $DIR/format.rs:57:5 | LL | format!("{}", 42.to_string()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: `to_string()` is enough: `42.to_string()` - | - = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: `to_string()` is enough: `42.to_string();` error: useless use of `format!` - --> $DIR/format.rs:57:5 + --> $DIR/format.rs:59:5 | LL | format!("{}", x.display().to_string()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: `to_string()` is enough: `x.display().to_string()` - | - = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: `to_string()` is enough: `x.display().to_string();` error: aborting due to 9 previous errors -- 2.44.0