From 6cb94630fb6220240730e2bed8d82f80a107696d Mon Sep 17 00:00:00 2001 From: Lachezar Lechev Date: Thu, 16 Aug 2018 18:20:06 +0200 Subject: [PATCH 1/4] WIP of #3016 for hardocded suggestion for writeln on empty string --- clippy_lints/src/write.rs | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/write.rs b/clippy_lints/src/write.rs index e8c7225041f..9b0b25f3921 100644 --- a/clippy_lints/src/write.rs +++ b/clippy_lints/src/write.rs @@ -179,7 +179,7 @@ impl EarlyLintPass for Pass { fn check_mac(&mut self, cx: &EarlyContext<'_>, mac: &Mac) { if mac.node.path == "println" { span_lint(cx, PRINT_STDOUT, mac.span, "use of `println!`"); - if let Some(fmtstr) = check_tts(cx, &mac.node.tts, false) { + if let Some(fmtstr) = check_tts(cx, &mac.node.tts, false).0 { if fmtstr == "" { span_lint_and_sugg( cx, @@ -193,7 +193,7 @@ impl EarlyLintPass for Pass { } } else if mac.node.path == "print" { span_lint(cx, PRINT_STDOUT, mac.span, "use of `print!`"); - if let Some(fmtstr) = check_tts(cx, &mac.node.tts, false) { + if let Some(fmtstr) = check_tts(cx, &mac.node.tts, false).0 { if fmtstr.ends_with("\\n") && !fmtstr.ends_with("\\n\\n") { span_lint(cx, PRINT_WITH_NEWLINE, mac.span, "using `print!()` with a format string that ends in a \ @@ -201,7 +201,7 @@ impl EarlyLintPass for Pass { } } } else if mac.node.path == "write" { - if let Some(fmtstr) = check_tts(cx, &mac.node.tts, true) { + if let Some(fmtstr) = check_tts(cx, &mac.node.tts, true).0 { if fmtstr.ends_with("\\n") && !fmtstr.ends_with("\\n\\n") { span_lint(cx, WRITE_WITH_NEWLINE, mac.span, "using `write!()` with a format string that ends in a \ @@ -209,15 +209,16 @@ impl EarlyLintPass for Pass { } } } else if mac.node.path == "writeln" { - if let Some(fmtstr) = check_tts(cx, &mac.node.tts, true) { + let check_tts = check_tts(cx, &mac.node.tts, true); + if let Some(fmtstr) = check_tts.0 { if fmtstr == "" { span_lint_and_sugg( cx, WRITELN_EMPTY_STRING, mac.span, - "using `writeln!(v, \"\")`", + format!("using `writeln!({}, \"\")`", check_tts.1).as_str(), "replace it with", - "writeln!(v)".to_string(), + format!("using `writeln!({})`", check_tts.1), ); } } @@ -225,7 +226,7 @@ impl EarlyLintPass for Pass { } } -fn check_tts<'a>(cx: &EarlyContext<'a>, tts: &ThinTokenStream, is_write: bool) -> Option { +fn check_tts<'a>(cx: &EarlyContext<'a>, tts: &ThinTokenStream, is_write: bool) -> (Option, Option) { let tts = TokenStream::from(tts.clone()); let mut parser = parser::Parser::new( &cx.sess.parse_sess, @@ -234,12 +235,14 @@ fn check_tts<'a>(cx: &EarlyContext<'a>, tts: &ThinTokenStream, is_write: bool) - false, false, ); - if is_write { - // skip the initial write target - parser.parse_expr().map_err(|mut err| err.cancel()).ok()?; - // might be `writeln!(foo)` - parser.expect(&token::Comma).map_err(|mut err| err.cancel()).ok()?; - } + // skip the initial write target + let expr: Option = match parser.parse_expr().map_err(|mut err| err.cancel()).ok() { + Some(p) => Some(p.into_vec().0), + None => None, + }; + // might be `writeln!(foo)` + parser.expect(&token::Comma).map_err(|mut err| err.cancel()).ok()?; + let fmtstr = parser.parse_str().map_err(|mut err| err.cancel()).ok()?.0.to_string(); use fmt_macros::*; let tmp = fmtstr.clone(); @@ -247,7 +250,7 @@ fn check_tts<'a>(cx: &EarlyContext<'a>, tts: &ThinTokenStream, is_write: bool) - let mut fmt_parser = Parser::new(&tmp, None); while let Some(piece) = fmt_parser.next() { if !fmt_parser.errors.is_empty() { - return None; + return (None, expr); } if let Piece::NextArgument(arg) = piece { if arg.format.ty == "?" { @@ -266,7 +269,7 @@ fn check_tts<'a>(cx: &EarlyContext<'a>, tts: &ThinTokenStream, is_write: bool) - loop { if !parser.eat(&token::Comma) { assert!(parser.eat(&token::Eof)); - return Some(fmtstr); + return (Some(fmtstr), expr); } let expr = parser.parse_expr().map_err(|mut err| err.cancel()).ok()?; const SIMPLE: FormatSpec<'_> = FormatSpec { From 3015987f279b414eea37d719c8e35ed2d8d7704a Mon Sep 17 00:00:00 2001 From: Lachezar Lechev Date: Mon, 20 Aug 2018 14:03:13 +0200 Subject: [PATCH 2/4] #3016 [WIP] Implement feedback and suggestions --- clippy_lints/src/write.rs | 41 ++++++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 14 deletions(-) diff --git a/clippy_lints/src/write.rs b/clippy_lints/src/write.rs index 9b0b25f3921..3c912756d2d 100644 --- a/clippy_lints/src/write.rs +++ b/clippy_lints/src/write.rs @@ -3,7 +3,7 @@ use rustc::{declare_lint, lint_array}; use syntax::ast::*; use syntax::tokenstream::{ThinTokenStream, TokenStream}; use syntax::parse::{token, parser}; -use crate::utils::{span_lint, span_lint_and_sugg}; +use crate::utils::{span_lint, span_lint_and_sugg, snippet}; /// **What it does:** This lint warns when you use `println!("")` to /// print a newline. @@ -212,13 +212,15 @@ impl EarlyLintPass for Pass { let check_tts = check_tts(cx, &mac.node.tts, true); if let Some(fmtstr) = check_tts.0 { if fmtstr == "" { + let suggestion = check_tts.1.map_or("v", |expr| snippet(cx, expr.span, "v").into_owned().as_str()); + span_lint_and_sugg( cx, WRITELN_EMPTY_STRING, mac.span, - format!("using `writeln!({}, \"\")`", check_tts.1).as_str(), + format!("using writeln!({}, \"\")", suggestion).as_str(), "replace it with", - format!("using `writeln!({})`", check_tts.1), + format!("writeln!({})", "v"), ); } } @@ -235,15 +237,23 @@ fn check_tts<'a>(cx: &EarlyContext<'a>, tts: &ThinTokenStream, is_write: bool) - false, false, ); - // skip the initial write target - let expr: Option = match parser.parse_expr().map_err(|mut err| err.cancel()).ok() { - Some(p) => Some(p.into_vec().0), - None => None, - }; - // might be `writeln!(foo)` - parser.expect(&token::Comma).map_err(|mut err| err.cancel()).ok()?; + let mut expr: Option = None; + if is_write { + // skip the initial write target + expr = match parser.parse_expr().map_err(|mut err| err.cancel()).ok() { + Some(p) => Some(p.and_then(|expr| expr)), + None => return (None, None), + }; + // might be `writeln!(foo)` + if let None = parser.expect(&token::Comma).map_err(|mut err| err.cancel()).ok() { + return (None, expr); + } + } - let fmtstr = parser.parse_str().map_err(|mut err| err.cancel()).ok()?.0.to_string(); + let fmtstr = match parser.parse_str().map_err(|mut err| err.cancel()).ok() { + Some(token) => token.0.to_string(), + None => return (None, expr), + }; use fmt_macros::*; let tmp = fmtstr.clone(); let mut args = vec![]; @@ -271,7 +281,10 @@ fn check_tts<'a>(cx: &EarlyContext<'a>, tts: &ThinTokenStream, is_write: bool) - assert!(parser.eat(&token::Eof)); return (Some(fmtstr), expr); } - let expr = parser.parse_expr().map_err(|mut err| err.cancel()).ok()?; + let token_expr = match parser.parse_expr().map_err(|mut err| err.cancel()).ok() { + Some(expr) => expr, + None => return (Some(fmtstr), None), + }; const SIMPLE: FormatSpec<'_> = FormatSpec { fill: None, align: AlignUnknown, @@ -280,7 +293,7 @@ fn check_tts<'a>(cx: &EarlyContext<'a>, tts: &ThinTokenStream, is_write: bool) - width: CountImplied, ty: "", }; - match &expr.node { + match &token_expr.node { ExprKind::Lit(_) => { let mut all_simple = true; let mut seen = false; @@ -296,7 +309,7 @@ fn check_tts<'a>(cx: &EarlyContext<'a>, tts: &ThinTokenStream, is_write: bool) - } } if all_simple && seen { - span_lint(cx, lint, expr.span, "literal with an empty format string"); + span_lint(cx, lint, token_expr.span, "literal with an empty format string"); } idx += 1; }, From c292b8078302a47dc47f6e772be3fb8f887dbf7a Mon Sep 17 00:00:00 2001 From: Lachezar Lechev Date: Mon, 20 Aug 2018 15:33:43 +0200 Subject: [PATCH 3/4] #3016 Add feedback and implement test for fixed hardcoded suggestion --- clippy_lints/src/write.rs | 26 +++++++++++++------------- tests/ui/writeln_empty_string.rs | 5 ++++- tests/ui/writeln_empty_string.stderr | 12 +++++++++--- 3 files changed, 26 insertions(+), 17 deletions(-) diff --git a/clippy_lints/src/write.rs b/clippy_lints/src/write.rs index 3c912756d2d..2231715c8d2 100644 --- a/clippy_lints/src/write.rs +++ b/clippy_lints/src/write.rs @@ -3,6 +3,7 @@ use rustc::{declare_lint, lint_array}; use syntax::ast::*; use syntax::tokenstream::{ThinTokenStream, TokenStream}; use syntax::parse::{token, parser}; +use std::borrow::Cow; use crate::utils::{span_lint, span_lint_and_sugg, snippet}; /// **What it does:** This lint warns when you use `println!("")` to @@ -212,7 +213,7 @@ impl EarlyLintPass for Pass { let check_tts = check_tts(cx, &mac.node.tts, true); if let Some(fmtstr) = check_tts.0 { if fmtstr == "" { - let suggestion = check_tts.1.map_or("v", |expr| snippet(cx, expr.span, "v").into_owned().as_str()); + let suggestion = check_tts.1.map_or(Cow::Borrowed("v"), |expr| snippet(cx, expr.span, "v")); span_lint_and_sugg( cx, @@ -220,7 +221,7 @@ impl EarlyLintPass for Pass { mac.span, format!("using writeln!({}, \"\")", suggestion).as_str(), "replace it with", - format!("writeln!({})", "v"), + format!("writeln!({})", suggestion), ); } } @@ -239,20 +240,19 @@ fn check_tts<'a>(cx: &EarlyContext<'a>, tts: &ThinTokenStream, is_write: bool) - ); let mut expr: Option = None; if is_write { - // skip the initial write target - expr = match parser.parse_expr().map_err(|mut err| err.cancel()).ok() { - Some(p) => Some(p.and_then(|expr| expr)), - None => return (None, None), + expr = match parser.parse_expr().map_err(|mut err| err.cancel()) { + Ok(p) => Some(p.into_inner()), + Err(_) => return (None, None), }; // might be `writeln!(foo)` - if let None = parser.expect(&token::Comma).map_err(|mut err| err.cancel()).ok() { + if parser.expect(&token::Comma).map_err(|mut err| err.cancel()).is_err() { return (None, expr); } } - let fmtstr = match parser.parse_str().map_err(|mut err| err.cancel()).ok() { - Some(token) => token.0.to_string(), - None => return (None, expr), + let fmtstr = match parser.parse_str().map_err(|mut err| err.cancel()) { + Ok(token) => token.0.to_string(), + Err(_) => return (None, expr), }; use fmt_macros::*; let tmp = fmtstr.clone(); @@ -281,9 +281,9 @@ fn check_tts<'a>(cx: &EarlyContext<'a>, tts: &ThinTokenStream, is_write: bool) - assert!(parser.eat(&token::Eof)); return (Some(fmtstr), expr); } - let token_expr = match parser.parse_expr().map_err(|mut err| err.cancel()).ok() { - Some(expr) => expr, - None => return (Some(fmtstr), None), + let token_expr = match parser.parse_expr().map_err(|mut err| err.cancel()) { + Ok(expr) => expr, + Err(_) => return (Some(fmtstr), None), }; const SIMPLE: FormatSpec<'_> = FormatSpec { fill: None, diff --git a/tests/ui/writeln_empty_string.rs b/tests/ui/writeln_empty_string.rs index c7092eb8c4b..faccfd8291c 100644 --- a/tests/ui/writeln_empty_string.rs +++ b/tests/ui/writeln_empty_string.rs @@ -5,9 +5,12 @@ use std::io::Write; fn main() { let mut v = Vec::new(); - // This should fail + // These should fail writeln!(&mut v, ""); + let mut suggestion = Vec::new(); + writeln!(&mut suggestion, ""); + // These should be fine writeln!(&mut v); writeln!(&mut v, " "); diff --git a/tests/ui/writeln_empty_string.stderr b/tests/ui/writeln_empty_string.stderr index 16a8e0a203d..8bfec673c4a 100644 --- a/tests/ui/writeln_empty_string.stderr +++ b/tests/ui/writeln_empty_string.stderr @@ -1,10 +1,16 @@ -error: using `writeln!(v, "")` +error: using writeln!(&mut v, "") --> $DIR/writeln_empty_string.rs:9:5 | 9 | writeln!(&mut v, ""); - | ^^^^^^^^^^^^^^^^^^^^ help: replace it with: `writeln!(v)` + | ^^^^^^^^^^^^^^^^^^^^ help: replace it with: `writeln!(&mut v)` | = note: `-D writeln-empty-string` implied by `-D warnings` -error: aborting due to previous error +error: using writeln!(&mut suggestion, "") + --> $DIR/writeln_empty_string.rs:12:5 + | +12 | writeln!(&mut suggestion, ""); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `writeln!(&mut suggestion)` + +error: aborting due to 2 previous errors From 76f7bfcefd6c7ad16b01864a3e616eb66fbfae2f Mon Sep 17 00:00:00 2001 From: Lachezar Lechev Date: Mon, 20 Aug 2018 15:50:15 +0200 Subject: [PATCH 4/4] #3016 Add backticks for the msg --- clippy_lints/src/write.rs | 2 +- tests/ui/writeln_empty_string.stderr | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/write.rs b/clippy_lints/src/write.rs index 2231715c8d2..97fe12f2330 100644 --- a/clippy_lints/src/write.rs +++ b/clippy_lints/src/write.rs @@ -219,7 +219,7 @@ impl EarlyLintPass for Pass { cx, WRITELN_EMPTY_STRING, mac.span, - format!("using writeln!({}, \"\")", suggestion).as_str(), + format!("using `writeln!({}, \"\")`", suggestion).as_str(), "replace it with", format!("writeln!({})", suggestion), ); diff --git a/tests/ui/writeln_empty_string.stderr b/tests/ui/writeln_empty_string.stderr index 8bfec673c4a..7bb6350ecd2 100644 --- a/tests/ui/writeln_empty_string.stderr +++ b/tests/ui/writeln_empty_string.stderr @@ -1,4 +1,4 @@ -error: using writeln!(&mut v, "") +error: using `writeln!(&mut v, "")` --> $DIR/writeln_empty_string.rs:9:5 | 9 | writeln!(&mut v, ""); @@ -6,7 +6,7 @@ error: using writeln!(&mut v, "") | = note: `-D writeln-empty-string` implied by `-D warnings` -error: using writeln!(&mut suggestion, "") +error: using `writeln!(&mut suggestion, "")` --> $DIR/writeln_empty_string.rs:12:5 | 12 | writeln!(&mut suggestion, "");