From 2d28d9f02a9059eb449521a62fcedd8cca104b3f Mon Sep 17 00:00:00 2001 From: phil Date: Sun, 17 Feb 2019 22:32:58 -0500 Subject: [PATCH 1/4] Add failing test for #3778 write_with_newline Literal `\n` characters (not a newline) in a `r"raw"` string should not fail the lint. --- tests/ui/write_with_newline.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/ui/write_with_newline.rs b/tests/ui/write_with_newline.rs index 2f53d4561af..02e043f9233 100644 --- a/tests/ui/write_with_newline.rs +++ b/tests/ui/write_with_newline.rs @@ -31,4 +31,7 @@ fn main() { write!(&mut v, "\\n"); // #3514 write!(&mut v, "\\\n"); write!(&mut v, "\\\\n"); + + // Raw strings + write!(&mut v, r"\n"); // #3778 } From cf5d4a1b45dbffad33c5236bfb9620bd9e2014b3 Mon Sep 17 00:00:00 2001 From: phil Date: Sun, 17 Feb 2019 22:32:58 -0500 Subject: [PATCH 2/4] Add failing test for #3778 write_with_newline Literal `\n` characters (not a newline) in a `r"raw"` string should not fail the lint. This affects both write_with_newline and print_with_newline, so it is added in both places. I also copied a missing test case from write_with_newline over to print_with_newline and added a note that one of those tests is supposed to fail. --- tests/ui/print_with_newline.rs | 8 ++++++++ tests/ui/print_with_newline.stderr | 8 +++++++- tests/ui/write_with_newline.rs | 2 +- tests/ui/write_with_newline.stderr | 2 +- 4 files changed, 17 insertions(+), 3 deletions(-) diff --git a/tests/ui/print_with_newline.rs b/tests/ui/print_with_newline.rs index 991cd7311e5..9df4b9052de 100644 --- a/tests/ui/print_with_newline.rs +++ b/tests/ui/print_with_newline.rs @@ -21,4 +21,12 @@ fn main() { print!("Hello {} {}\n\n", "world", "#2"); println!("\ndon't\nwarn\nfor\nmultiple\nnewlines\n"); // #3126 println!("\nbla\n\n"); // #3126 + + // Escaping + print!("\\n"); // #3514 + print!("\\\n"); // should fail + print!("\\\\n"); + + // Raw strings + print!(r"\n"); // #3778 } diff --git a/tests/ui/print_with_newline.stderr b/tests/ui/print_with_newline.stderr index a731212be87..1d89a16e090 100644 --- a/tests/ui/print_with_newline.stderr +++ b/tests/ui/print_with_newline.stderr @@ -24,5 +24,11 @@ error: using `print!()` with a format string that ends in a single newline, cons LL | print!("{}/n", 1265); | ^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 4 previous errors +error: using `print!()` with a format string that ends in a single newline, consider using `println!()` instead + --> $DIR/print_with_newline.rs:27:5 + | +LL | print!("//n"); // should fail + | ^^^^^^^^^^^^^^ + +error: aborting due to 5 previous errors diff --git a/tests/ui/write_with_newline.rs b/tests/ui/write_with_newline.rs index 02e043f9233..3575dd6da80 100644 --- a/tests/ui/write_with_newline.rs +++ b/tests/ui/write_with_newline.rs @@ -29,7 +29,7 @@ fn main() { // Escaping write!(&mut v, "\\n"); // #3514 - write!(&mut v, "\\\n"); + write!(&mut v, "\\\n"); // should fail write!(&mut v, "\\\\n"); // Raw strings diff --git a/tests/ui/write_with_newline.stderr b/tests/ui/write_with_newline.stderr index 1f4395c2621..b0761f3b081 100644 --- a/tests/ui/write_with_newline.stderr +++ b/tests/ui/write_with_newline.stderr @@ -27,7 +27,7 @@ LL | write!(&mut v, "{}/n", 1265); error: using `write!()` with a format string that ends in a single newline, consider using `writeln!()` instead --> $DIR/write_with_newline.rs:32:5 | -LL | write!(&mut v, "//n"); +LL | write!(&mut v, "//n"); // should fail | ^^^^^^^^^^^^^^^^^^^^^^ error: aborting due to 5 previous errors From 3fa3bd8e9464f0bb74366e78972f77353bdd29a5 Mon Sep 17 00:00:00 2001 From: phil Date: Mon, 18 Feb 2019 00:53:30 -0500 Subject: [PATCH 3/4] Don't fail for raw string ending in \n Pass tests for #3778, {print,write}_with_newline false positive This change guards the lint from checking newlines with a sort of complicated check to see if it's a raw string. Raw strings shouldn't be newline-checked, since r"\n" is literally \-n, not a newline. I think it's ok not to check for _literal_ newlines at the end of raw strings, but maybe that's debatable. I... don't think this code is that great. I wanted to write the check after `check_tts`, but that was too late -- raw string type info is lost (or I couldn't find it). Putting it inside `check_tts` feels heavy-duty and the check itself feels like a brittle reach possibly into places it shouldn't. Maybe someone can fix this up :) --- clippy_lints/src/write.rs | 46 ++++++++++++++++++++++++++------------- 1 file changed, 31 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/write.rs b/clippy_lints/src/write.rs index 36f7fee969b..4f771b2af55 100644 --- a/clippy_lints/src/write.rs +++ b/clippy_lints/src/write.rs @@ -5,7 +5,7 @@ use rustc_errors::Applicability; use std::borrow::Cow; use syntax::ast::*; use syntax::parse::{parser, token}; -use syntax::tokenstream::TokenStream; +use syntax::tokenstream::{TokenStream, TokenTree}; /// **What it does:** This lint warns when you use `println!("")` to /// print a newline. @@ -200,8 +200,8 @@ 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).0 { - if check_newlines(&fmtstr) { + if let (Some(fmtstr), _, is_raw) = check_tts(cx, &mac.node.tts, false) { + if !is_raw && check_newlines(&fmtstr) { span_lint( cx, PRINT_WITH_NEWLINE, @@ -212,8 +212,8 @@ impl EarlyLintPass for Pass { } } } else if mac.node.path == "write" { - if let Some(fmtstr) = check_tts(cx, &mac.node.tts, true).0 { - if check_newlines(&fmtstr) { + if let (Some(fmtstr), _, is_raw) = check_tts(cx, &mac.node.tts, true) { + if !is_raw && check_newlines(&fmtstr) { span_lint( cx, WRITE_WITH_NEWLINE, @@ -252,8 +252,9 @@ impl EarlyLintPass for Pass { } /// Checks the arguments of `print[ln]!` and `write[ln]!` calls. It will return a tuple of two -/// options. The first part of the tuple is `format_str` of the macros. The second part of the tuple -/// is in the `write[ln]!` case the expression the `format_str` should be written to. +/// options and a bool. The first part of the tuple is `format_str` of the macros. The second part +/// of the tuple is in the `write[ln]!` case the expression the `format_str` should be written to. +/// The final part is a boolean flag indicating if the string is a raw string. /// /// Example: /// @@ -263,34 +264,49 @@ impl EarlyLintPass for Pass { /// ``` /// will return /// ```rust,ignore -/// (Some("string to write: {}"), Some(buf)) +/// (Some("string to write: {}"), Some(buf), false) /// ``` -fn check_tts<'a>(cx: &EarlyContext<'a>, tts: &TokenStream, is_write: bool) -> (Option, Option) { +fn check_tts<'a>(cx: &EarlyContext<'a>, tts: &TokenStream, is_write: bool) -> (Option, Option, bool) { use fmt_macros::*; let tts = tts.clone(); + let mut is_raw = false; + if let TokenStream(Some(tokens)) = &tts { + for token in tokens.iter() { + if let (TokenTree::Token(_, token::Token::Literal(lit, _)), _) = token { + match lit { + token::Lit::Str_(_) => break, + token::Lit::StrRaw(_, _) => { + is_raw = true; + break; + }, + _ => {}, + } + } + } + } let mut parser = parser::Parser::new(&cx.sess.parse_sess, tts, None, false, false); let mut expr: Option = None; if is_write { expr = match parser.parse_expr().map_err(|mut err| err.cancel()) { Ok(p) => Some(p.into_inner()), - Err(_) => return (None, None), + Err(_) => return (None, None, is_raw), }; // might be `writeln!(foo)` if parser.expect(&token::Comma).map_err(|mut err| err.cancel()).is_err() { - return (None, expr); + return (None, expr, is_raw); } } let fmtstr = match parser.parse_str().map_err(|mut err| err.cancel()) { Ok(token) => token.0.to_string(), - Err(_) => return (None, expr), + Err(_) => return (None, expr, is_raw), }; let tmp = fmtstr.clone(); let mut args = vec![]; let mut fmt_parser = Parser::new(&tmp, None, Vec::new(), false); while let Some(piece) = fmt_parser.next() { if !fmt_parser.errors.is_empty() { - return (None, expr); + return (None, expr, is_raw); } if let Piece::NextArgument(arg) = piece { if arg.format.ty == "?" { @@ -312,11 +328,11 @@ fn check_tts<'a>(cx: &EarlyContext<'a>, tts: &TokenStream, is_write: bool) -> (O ty: "", }; if !parser.eat(&token::Comma) { - return (Some(fmtstr), expr); + return (Some(fmtstr), expr, is_raw); } let token_expr = match parser.parse_expr().map_err(|mut err| err.cancel()) { Ok(expr) => expr, - Err(_) => return (Some(fmtstr), None), + Err(_) => return (Some(fmtstr), None, is_raw), }; match &token_expr.node { ExprKind::Lit(_) => { From ef72b2cac0c9e0715d384711b3f08567c013300d Mon Sep 17 00:00:00 2001 From: phil Date: Mon, 18 Feb 2019 11:39:23 -0500 Subject: [PATCH 4/4] Check {print,write}_with_newline for literal newline Both regular strings and raw strings can contain literal newlines. This commit extends the lint to also warn about terminating strings with these. Behaviour handling for raw strings is also moved into `check_newlines` by passing in the `is_raw` boolean from `check_tts` as [suggested](https://github.com/rust-lang/rust-clippy/pull/3781#pullrequestreview-204663732) --- clippy_lints/src/write.rs | 13 ++++++++++--- tests/ui/print_with_newline.rs | 10 ++++++++++ tests/ui/print_with_newline.stderr | 20 +++++++++++++++++++- tests/ui/write_with_newline.rs | 12 ++++++++++++ tests/ui/write_with_newline.stderr | 22 +++++++++++++++++++++- 5 files changed, 72 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/write.rs b/clippy_lints/src/write.rs index 4f771b2af55..012c34e9b53 100644 --- a/clippy_lints/src/write.rs +++ b/clippy_lints/src/write.rs @@ -201,7 +201,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), _, is_raw) = check_tts(cx, &mac.node.tts, false) { - if !is_raw && check_newlines(&fmtstr) { + if check_newlines(&fmtstr, is_raw) { span_lint( cx, PRINT_WITH_NEWLINE, @@ -213,7 +213,7 @@ impl EarlyLintPass for Pass { } } else if mac.node.path == "write" { if let (Some(fmtstr), _, is_raw) = check_tts(cx, &mac.node.tts, true) { - if !is_raw && check_newlines(&fmtstr) { + if check_newlines(&fmtstr, is_raw) { span_lint( cx, WRITE_WITH_NEWLINE, @@ -382,7 +382,14 @@ fn check_tts<'a>(cx: &EarlyContext<'a>, tts: &TokenStream, is_write: bool) -> (O } // Checks if `s` constains a single newline that terminates it -fn check_newlines(s: &str) -> bool { +// Literal and escaped newlines are both checked (only literal for raw strings) +fn check_newlines(s: &str, is_raw: bool) -> bool { + if s.ends_with('\n') { + return true; + } else if is_raw { + return false; + } + if s.len() < 2 { return false; } diff --git a/tests/ui/print_with_newline.rs b/tests/ui/print_with_newline.rs index 9df4b9052de..1c219ecb325 100644 --- a/tests/ui/print_with_newline.rs +++ b/tests/ui/print_with_newline.rs @@ -29,4 +29,14 @@ fn main() { // Raw strings print!(r"\n"); // #3778 + + // Literal newlines should also fail + print!( + " +" + ); + print!( + r" +" + ); } diff --git a/tests/ui/print_with_newline.stderr b/tests/ui/print_with_newline.stderr index 1d89a16e090..ff89b0d3fd4 100644 --- a/tests/ui/print_with_newline.stderr +++ b/tests/ui/print_with_newline.stderr @@ -30,5 +30,23 @@ error: using `print!()` with a format string that ends in a single newline, cons LL | print!("//n"); // should fail | ^^^^^^^^^^^^^^ -error: aborting due to 5 previous errors +error: using `print!()` with a format string that ends in a single newline, consider using `println!()` instead + --> $DIR/print_with_newline.rs:34:5 + | +LL | / print!( +LL | | " +LL | | " +LL | | ); + | |_____^ + +error: using `print!()` with a format string that ends in a single newline, consider using `println!()` instead + --> $DIR/print_with_newline.rs:38:5 + | +LL | / print!( +LL | | r" +LL | | " +LL | | ); + | |_____^ + +error: aborting due to 7 previous errors diff --git a/tests/ui/write_with_newline.rs b/tests/ui/write_with_newline.rs index 3575dd6da80..dd80dc0cf9c 100644 --- a/tests/ui/write_with_newline.rs +++ b/tests/ui/write_with_newline.rs @@ -34,4 +34,16 @@ fn main() { // Raw strings write!(&mut v, r"\n"); // #3778 + + // Literal newlines should also fail + write!( + &mut v, + " +" + ); + write!( + &mut v, + r" +" + ); } diff --git a/tests/ui/write_with_newline.stderr b/tests/ui/write_with_newline.stderr index b0761f3b081..3a31f61a277 100644 --- a/tests/ui/write_with_newline.stderr +++ b/tests/ui/write_with_newline.stderr @@ -30,5 +30,25 @@ error: using `write!()` with a format string that ends in a single newline, cons LL | write!(&mut v, "//n"); // should fail | ^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 5 previous errors +error: using `write!()` with a format string that ends in a single newline, consider using `writeln!()` instead + --> $DIR/write_with_newline.rs:39:5 + | +LL | / write!( +LL | | &mut v, +LL | | " +LL | | " +LL | | ); + | |_____^ + +error: using `write!()` with a format string that ends in a single newline, consider using `writeln!()` instead + --> $DIR/write_with_newline.rs:44:5 + | +LL | / write!( +LL | | &mut v, +LL | | r" +LL | | " +LL | | ); + | |_____^ + +error: aborting due to 7 previous errors