Auto merge of rust-lang#3781 - uniphil:write_with_newline_false_positive, r=oli-obk

Don't check [print/write]_with_newline on raw strings

Some tests for rust-lang#3778 and some maybe-not-the-greatest code that passes those tests!

I didn't run `fmt` because a) it doesn't seem to install on nightly for me, and b) on stable it wanted to apply formatting to over 90 files. Happy to make any tweaks though!

I suspect this contribution may require more than just tweaks. I'm still sort of new to rust so it may not be idiomatic, and the specific approach I took feels a little heavy-handed and brittle. I'm happy to make changes with some guidance, or equally happy if this gives a starting place for someone else to do it better :)

Author: bors

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 check_newlines(&fmtstr, is_raw) {
                     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 check_newlines(&fmtstr, is_raw) {
                     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<String>, Option<Expr>) {
+fn check_tts<'a>(cx: &EarlyContext<'a>, tts: &TokenStream, is_write: bool) -> (Option<String>, Option<Expr>, 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<Expr> = 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(_) => {
@@ -366,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;
     }

tests/ui/print_with_newline.rs

@@ -21,4 +21,22 @@ 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
+
+    // Literal newlines should also fail
+    print!(
+        "
+"
+    );
+    print!(
+        r"
+"
+    );
 }

tests/ui/print_with_newline.stderr

@@ -24,5 +24,29 @@ 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: 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
 

tests/ui/write_with_newline.rs

@@ -29,6 +29,21 @@ fn main() {
 
     // Escaping
     write!(&mut v, "\\n"); // #3514
-    write!(&mut v, "\\\n");
+    write!(&mut v, "\\\n"); // should fail
     write!(&mut v, "\\\\n");
+
+    // Raw strings
+    write!(&mut v, r"\n"); // #3778
+
+    // Literal newlines should also fail
+    write!(
+        &mut v,
+        "
+"
+    );
+    write!(
+        &mut v,
+        r"
+"
+    );
 }

tests/ui/write_with_newline.stderr

@@ -27,8 +27,28 @@ 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
+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