From 2d0c797da6c2d3395e95c086c3cee66fa95a0b53 Mon Sep 17 00:00:00 2001 From: Andy Russell Date: Thu, 23 May 2019 14:11:38 -0400 Subject: [PATCH] add suggestions for print/write with newline lint --- clippy_lints/src/write.rs | 164 +++++++++++++++++++---------- tests/ui/print_with_newline.rs | 3 + tests/ui/print_with_newline.stderr | 58 +++++++--- tests/ui/write_with_newline.rs | 3 + tests/ui/write_with_newline.stderr | 60 ++++++++--- 5 files changed, 206 insertions(+), 82 deletions(-) diff --git a/clippy_lints/src/write.rs b/clippy_lints/src/write.rs index 1bf90d0a8..b55d0d940 100644 --- a/clippy_lints/src/write.rs +++ b/clippy_lints/src/write.rs @@ -1,12 +1,12 @@ -use crate::utils::{snippet_with_applicability, span_lint, span_lint_and_sugg}; +use crate::utils::{snippet_with_applicability, span_lint, span_lint_and_sugg, span_lint_and_then}; use rustc::lint::{EarlyContext, EarlyLintPass, LintArray, LintPass}; use rustc::{declare_lint_pass, declare_tool_lint}; use rustc_errors::Applicability; use std::borrow::Cow; use syntax::ast::*; use syntax::parse::{parser, token}; -use syntax::tokenstream::{TokenStream, TokenTree}; -use syntax_pos::symbol::Symbol; +use syntax::tokenstream::TokenStream; +use syntax_pos::{symbol::Symbol, BytePos, Span}; declare_clippy_lint! { /// **What it does:** This lint warns when you use `println!("")` to @@ -184,8 +184,8 @@ impl EarlyLintPass for Write { fn check_mac(&mut self, cx: &EarlyContext<'_>, mac: &Mac) { if mac.node.path == sym!(println) { span_lint(cx, PRINT_STDOUT, mac.span, "use of `println!`"); - if let Some(fmtstr) = check_tts(cx, &mac.node.tts, false).0 { - if fmtstr == "" { + if let (Some(fmt_str), _) = check_tts(cx, &mac.node.tts, false) { + if fmt_str.contents.is_empty() { span_lint_and_sugg( cx, PRINTLN_EMPTY_STRING, @@ -199,35 +199,52 @@ impl EarlyLintPass for Write { } } else if mac.node.path == sym!(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 check_newlines(&fmtstr, is_raw) { - span_lint( + if let (Some(fmt_str), _) = check_tts(cx, &mac.node.tts, false) { + if check_newlines(&fmt_str) { + span_lint_and_then( cx, PRINT_WITH_NEWLINE, mac.span, - "using `print!()` with a format string that ends in a \ - single newline, consider using `println!()` instead", + "using `print!()` with a format string that ends in a single newline", + |err| { + err.multipart_suggestion( + "use `println!` instead", + vec![ + (mac.node.path.span, String::from("println")), + (fmt_str.newline_span(), String::new()), + ], + Applicability::MachineApplicable, + ); + }, ); } } } else if mac.node.path == sym!(write) { - if let (Some(fmtstr), _, is_raw) = check_tts(cx, &mac.node.tts, true) { - if check_newlines(&fmtstr, is_raw) { - span_lint( + if let (Some(fmt_str), _) = check_tts(cx, &mac.node.tts, true) { + if check_newlines(&fmt_str) { + span_lint_and_then( cx, WRITE_WITH_NEWLINE, mac.span, - "using `write!()` with a format string that ends in a \ - single newline, consider using `writeln!()` instead", - ); + "using `write!()` with a format string that ends in a single newline", + |err| { + err.multipart_suggestion( + "use `writeln!()` instead", + vec![ + (mac.node.path.span, String::from("writeln")), + (fmt_str.newline_span(), String::new()), + ], + Applicability::MachineApplicable, + ); + }, + ) } } } else if mac.node.path == sym!(writeln) { - let check_tts = check_tts(cx, &mac.node.tts, true); - if let Some(fmtstr) = check_tts.0 { - if fmtstr == "" { + if let (Some(fmt_str), expr) = check_tts(cx, &mac.node.tts, true) { + if fmt_str.contents.is_empty() { let mut applicability = Applicability::MachineApplicable; - let suggestion = check_tts.1.map_or_else( + let suggestion = expr.map_or_else( move || { applicability = Applicability::HasPlaceholders; Cow::Borrowed("v") @@ -250,10 +267,44 @@ impl EarlyLintPass for Write { } } +/// The arguments of a `print[ln]!` or `write[ln]!` invocation. +struct FmtStr { + /// The contents of the format string (inside the quotes). + contents: String, + style: StrStyle, + /// The span of the format string, including quotes, the raw marker, and any raw hashes. + span: Span, +} + +impl FmtStr { + /// Given a format string that ends in a newline and its span, calculates the span of the + /// newline. + fn newline_span(&self) -> Span { + let sp = self.span; + + let newline_sp_hi = sp.hi() + - match self.style { + StrStyle::Cooked => BytePos(1), + StrStyle::Raw(hashes) => BytePos((1 + hashes).into()), + }; + + let newline_sp_len = if self.contents.ends_with('\n') { + BytePos(1) + } else if self.contents.ends_with(r"\n") { + BytePos(2) + } else { + panic!("expected format string to contain a newline"); + }; + + sp.with_lo(newline_sp_hi - newline_sp_len).with_hi(newline_sp_hi) + } +} + /// Checks the arguments of `print[ln]!` and `write[ln]!` calls. It will return a tuple of two -/// 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. +/// `Option`s. The first `Option` of the tuple is the macro's format string. It includes +/// the contents of the string, whether it's a raw string, and the span of the literal in the +/// source. The second `Option` in the tuple is, in the `write[ln]!` case, the expression the +/// `format_str` should be written to. /// /// Example: /// @@ -266,49 +317,36 @@ impl EarlyLintPass for Write { /// ``` /// will return /// ```rust,ignore -/// (Some("string to write: {}"), Some(buf), false) +/// (Some("string to write: {}"), Some(buf)) /// ``` -fn check_tts<'a>(cx: &EarlyContext<'a>, tts: &TokenStream, is_write: bool) -> (Option, Option, bool) { +fn check_tts<'a>(cx: &EarlyContext<'a>, tts: &TokenStream, is_write: bool) -> (Option, Option) { 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.kind { - token::Str => break, - token::StrRaw(_) => { - is_raw = true; - break; - }, - _ => {}, - } - } - } - } + let mut parser = parser::Parser::new(&cx.sess.parse_sess, tts, None, false, false, None); 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, is_raw), + Err(_) => return (None, None), }; // might be `writeln!(foo)` if parser.expect(&token::Comma).map_err(|mut err| err.cancel()).is_err() { - return (None, expr, is_raw); + 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, is_raw), + let (fmtstr, fmtstyle) = match parser.parse_str().map_err(|mut err| err.cancel()) { + Ok((fmtstr, fmtstyle)) => (fmtstr.to_string(), fmtstyle), + Err(_) => return (None, expr), }; + let fmtspan = parser.prev_span; 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, is_raw); + return (None, expr); } if let Piece::NextArgument(arg) = piece { if arg.format.ty == "?" { @@ -330,11 +368,26 @@ fn check_tts<'a>(cx: &EarlyContext<'a>, tts: &TokenStream, is_write: bool) -> (O ty: "", }; if !parser.eat(&token::Comma) { - return (Some(fmtstr), expr, is_raw); + return ( + Some(FmtStr { + contents: fmtstr, + style: fmtstyle, + span: fmtspan, + }), + expr, + ); } - let token_expr = match parser.parse_expr().map_err(|mut err| err.cancel()) { - Ok(expr) => expr, - Err(_) => return (Some(fmtstr), None, is_raw), + let token_expr = if let Ok(expr) = parser.parse_expr().map_err(|mut err| err.cancel()) { + expr + } else { + return ( + Some(FmtStr { + contents: fmtstr, + style: fmtstyle, + span: fmtspan, + }), + None, + ); }; match &token_expr.node { ExprKind::Lit(_) => { @@ -383,12 +436,15 @@ fn check_tts<'a>(cx: &EarlyContext<'a>, tts: &TokenStream, is_write: bool) -> (O } } -// Checks if `s` constains a single newline that terminates it -// Literal and escaped newlines are both checked (only literal for raw strings) -fn check_newlines(s: &str, is_raw: bool) -> bool { +/// Checks if the format string constains a single newline that terminates it. +/// +/// Literal and escaped newlines are both checked (only literal for raw strings). +fn check_newlines(fmt_str: &FmtStr) -> bool { + let s = &fmt_str.contents; + if s.ends_with('\n') { return true; - } else if is_raw { + } else if let StrStyle::Raw(_) = fmt_str.style { return false; } diff --git a/tests/ui/print_with_newline.rs b/tests/ui/print_with_newline.rs index 1c219ecb3..111a29faa 100644 --- a/tests/ui/print_with_newline.rs +++ b/tests/ui/print_with_newline.rs @@ -1,3 +1,6 @@ +// FIXME: Ideally these suggestions would be fixed via rustfix. Blocked by rust-lang/rust#53934 +// // run-rustfix + #![allow(clippy::print_literal)] #![warn(clippy::print_with_newline)] diff --git a/tests/ui/print_with_newline.stderr b/tests/ui/print_with_newline.stderr index ff89b0d3f..81e8b45b5 100644 --- a/tests/ui/print_with_newline.stderr +++ b/tests/ui/print_with_newline.stderr @@ -1,52 +1,82 @@ -error: using `print!()` with a format string that ends in a single newline, consider using `println!()` instead - --> $DIR/print_with_newline.rs:5:5 +error: using `print!()` with a format string that ends in a single newline + --> $DIR/print_with_newline.rs:8:5 | LL | print!("Hello/n"); | ^^^^^^^^^^^^^^^^^ | = note: `-D clippy::print-with-newline` implied by `-D warnings` +help: use `println!` instead + | +LL | println!("Hello"); + | ^^^^^^^ -- -error: using `print!()` with a format string that ends in a single newline, consider using `println!()` instead - --> $DIR/print_with_newline.rs:6:5 +error: using `print!()` with a format string that ends in a single newline + --> $DIR/print_with_newline.rs:9:5 | LL | print!("Hello {}/n", "world"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: use `println!` instead + | +LL | println!("Hello {}", "world"); + | ^^^^^^^ -- -error: using `print!()` with a format string that ends in a single newline, consider using `println!()` instead - --> $DIR/print_with_newline.rs:7:5 +error: using `print!()` with a format string that ends in a single newline + --> $DIR/print_with_newline.rs:10:5 | LL | print!("Hello {} {}/n", "world", "#2"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: use `println!` instead + | +LL | println!("Hello {} {}", "world", "#2"); + | ^^^^^^^ -- -error: using `print!()` with a format string that ends in a single newline, consider using `println!()` instead - --> $DIR/print_with_newline.rs:8:5 +error: using `print!()` with a format string that ends in a single newline + --> $DIR/print_with_newline.rs:11:5 | LL | print!("{}/n", 1265); | ^^^^^^^^^^^^^^^^^^^^ +help: use `println!` instead + | +LL | println!("{}", 1265); + | ^^^^^^^ -- -error: using `print!()` with a format string that ends in a single newline, consider using `println!()` instead - --> $DIR/print_with_newline.rs:27:5 +error: using `print!()` with a format string that ends in a single newline + --> $DIR/print_with_newline.rs:30:5 | LL | print!("//n"); // should fail | ^^^^^^^^^^^^^^ +help: use `println!` instead + | +LL | println!("/"); // 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 +error: using `print!()` with a format string that ends in a single newline + --> $DIR/print_with_newline.rs:37:5 | LL | / print!( LL | | " LL | | " LL | | ); | |_____^ +help: use `println!` instead + | +LL | println!( +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 +error: using `print!()` with a format string that ends in a single newline + --> $DIR/print_with_newline.rs:41:5 | LL | / print!( LL | | r" LL | | " LL | | ); | |_____^ +help: use `println!` instead + | +LL | println!( +LL | r"" + | error: aborting due to 7 previous errors diff --git a/tests/ui/write_with_newline.rs b/tests/ui/write_with_newline.rs index dd80dc0cf..f0b13a698 100644 --- a/tests/ui/write_with_newline.rs +++ b/tests/ui/write_with_newline.rs @@ -1,3 +1,6 @@ +// FIXME: Ideally these suggestions would be fixed via rustfix. Blocked by rust-lang/rust#53934 +// // run-rustfix + #![allow(clippy::write_literal)] #![warn(clippy::write_with_newline)] diff --git a/tests/ui/write_with_newline.stderr b/tests/ui/write_with_newline.stderr index 3a31f61a2..c1f1d64c2 100644 --- a/tests/ui/write_with_newline.stderr +++ b/tests/ui/write_with_newline.stderr @@ -1,37 +1,57 @@ -error: using `write!()` with a format string that ends in a single newline, consider using `writeln!()` instead - --> $DIR/write_with_newline.rs:10:5 +error: using `write!()` with a format string that ends in a single newline + --> $DIR/write_with_newline.rs:13:5 | LL | write!(&mut v, "Hello/n"); | ^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `-D clippy::write-with-newline` implied by `-D warnings` +help: use `writeln!()` instead + | +LL | writeln!(&mut v, "Hello"); + | ^^^^^^^ -- -error: using `write!()` with a format string that ends in a single newline, consider using `writeln!()` instead - --> $DIR/write_with_newline.rs:11:5 +error: using `write!()` with a format string that ends in a single newline + --> $DIR/write_with_newline.rs:14:5 | LL | write!(&mut v, "Hello {}/n", "world"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: use `writeln!()` instead + | +LL | writeln!(&mut v, "Hello {}", "world"); + | ^^^^^^^ -- -error: using `write!()` with a format string that ends in a single newline, consider using `writeln!()` instead - --> $DIR/write_with_newline.rs:12:5 +error: using `write!()` with a format string that ends in a single newline + --> $DIR/write_with_newline.rs:15:5 | LL | write!(&mut v, "Hello {} {}/n", "world", "#2"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: use `writeln!()` instead + | +LL | writeln!(&mut v, "Hello {} {}", "world", "#2"); + | ^^^^^^^ -- -error: using `write!()` with a format string that ends in a single newline, consider using `writeln!()` instead - --> $DIR/write_with_newline.rs:13:5 +error: using `write!()` with a format string that ends in a single newline + --> $DIR/write_with_newline.rs:16:5 | LL | write!(&mut v, "{}/n", 1265); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: use `writeln!()` instead + | +LL | writeln!(&mut v, "{}", 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 +error: using `write!()` with a format string that ends in a single newline + --> $DIR/write_with_newline.rs:35:5 | LL | write!(&mut v, "//n"); // should fail | ^^^^^^^^^^^^^^^^^^^^^^ +help: use `writeln!()` instead + | +LL | writeln!(&mut v, "/"); // should fail + | ^^^^^^^ -- -error: using `write!()` with a format string that ends in a single newline, consider using `writeln!()` instead - --> $DIR/write_with_newline.rs:39:5 +error: using `write!()` with a format string that ends in a single newline + --> $DIR/write_with_newline.rs:42:5 | LL | / write!( LL | | &mut v, @@ -39,9 +59,15 @@ LL | | " LL | | " LL | | ); | |_____^ +help: use `writeln!()` instead + | +LL | writeln!( +LL | &mut v, +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 +error: using `write!()` with a format string that ends in a single newline + --> $DIR/write_with_newline.rs:47:5 | LL | / write!( LL | | &mut v, @@ -49,6 +75,12 @@ LL | | r" LL | | " LL | | ); | |_____^ +help: use `writeln!()` instead + | +LL | writeln!( +LL | &mut v, +LL | r"" + | error: aborting due to 7 previous errors