From c4c9d9fc62eb244c0f219cfbf23f19b9240c2827 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Fri, 23 Nov 2018 08:18:23 +0100 Subject: [PATCH 1/7] Add suggestion for explicit_write lint --- clippy_lints/src/explicit_write.rs | 40 ++++++++++++------- clippy_lints/src/lib.rs | 1 + tests/ui/explicit_write.rs | 4 ++ tests/ui/explicit_write.stderr | 62 ++++++++++++++++++------------ 4 files changed, 68 insertions(+), 39 deletions(-) diff --git a/clippy_lints/src/explicit_write.rs b/clippy_lints/src/explicit_write.rs index 94bd0ab209cb..65fa6aeec0b5 100644 --- a/clippy_lints/src/explicit_write.rs +++ b/clippy_lints/src/explicit_write.rs @@ -10,8 +10,8 @@ use crate::rustc::hir::*; use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; use crate::rustc::{declare_tool_lint, lint_array}; -use crate::utils::opt_def_id; -use crate::utils::{is_expn_of, match_def_path, resolve_node, span_lint}; +use crate::syntax::ast::LitKind; +use crate::utils::{is_expn_of, match_def_path, opt_def_id, resolve_node, span_lint, span_lint_and_sugg}; use if_chain::if_chain; /// **What it does:** Checks for usage of `write!()` / `writeln()!` which can be @@ -51,6 +51,12 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { if unwrap_args.len() > 0; if let ExprKind::MethodCall(ref write_fun, _, ref write_args) = unwrap_args[0].node; + // Obtain the string that should be printed + if let ExprKind::Call(_, ref output_args) = write_args[1].node; + if let ExprKind::AddrOf(_, ref output_string_expr) = output_args[0].node; + if let ExprKind::Array(ref string_exprs) = output_string_expr.node; + if let ExprKind::Lit(ref lit) = string_exprs[0].node; + if let LitKind::Str(ref write_output, _) = lit.node; if write_fun.ident.name == "write_fmt"; // match calls to std::io::stdout() / std::io::stderr () if write_args.len() > 0; @@ -81,29 +87,35 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { } else { "" }; + + // We need to remove the last trailing newline from the string because the + // underlying `fmt::write` function doesn't know wether `println!` or `print!` was + // used. + let mut write_output: String = write_output.to_string(); + if write_output.ends_with('\n') { + write_output.truncate(write_output.len() - 1) + } if let Some(macro_name) = calling_macro { - span_lint( + span_lint_and_sugg( cx, EXPLICIT_WRITE, expr.span, &format!( - "use of `{}!({}(), ...).unwrap()`. Consider using `{}{}!` instead", + "use of `{}!({}(), ...).unwrap()`", macro_name, - dest_name, - prefix, - macro_name.replace("write", "print") - ) + dest_name + ), + "try this", + format!("{}{}!(\"{}\")", prefix, macro_name.replace("write", "print"), write_output.escape_default()) ); } else { - span_lint( + span_lint_and_sugg( cx, EXPLICIT_WRITE, expr.span, - &format!( - "use of `{}().write_fmt(...).unwrap()`. Consider using `{}print!` instead", - dest_name, - prefix, - ) + &format!("use of `{}().write_fmt(...).unwrap()`", dest_name), + "try this", + format!("{}print!(\"{}\")", prefix, write_output.escape_default()) ); } } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index ee41c632077c..a862d774174b 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -14,6 +14,7 @@ #![feature(slice_patterns)] #![feature(stmt_expr_attributes)] #![feature(range_contains)] +#![feature(str_escape)] #![allow(clippy::missing_docs_in_private_items)] #![recursion_limit = "256"] #![warn(rust_2018_idioms, trivial_casts, trivial_numeric_casts)] diff --git a/tests/ui/explicit_write.rs b/tests/ui/explicit_write.rs index 10a4bca9f492..01a63b3a95f2 100644 --- a/tests/ui/explicit_write.rs +++ b/tests/ui/explicit_write.rs @@ -27,6 +27,10 @@ fn main() { writeln!(std::io::stderr(), "test").unwrap(); std::io::stdout().write_fmt(format_args!("test")).unwrap(); std::io::stderr().write_fmt(format_args!("test")).unwrap(); + + // including newlines + writeln!(std::io::stdout(), "test\ntest").unwrap(); + writeln!(std::io::stderr(), "test\ntest").unwrap(); } // these should not warn, different destination { diff --git a/tests/ui/explicit_write.stderr b/tests/ui/explicit_write.stderr index 171bf312a9bd..6d318d09e584 100644 --- a/tests/ui/explicit_write.stderr +++ b/tests/ui/explicit_write.stderr @@ -1,40 +1,52 @@ -error: use of `write!(stdout(), ...).unwrap()`. Consider using `print!` instead - --> $DIR/explicit_write.rs:24:9 +error: use of `write!(stdout(), ...).unwrap()` + --> $DIR/explicit_write.rs:28:9 | -24 | write!(std::io::stdout(), "test").unwrap(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +28 | write!(std::io::stdout(), "test").unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `print!("test")` | = note: `-D clippy::explicit-write` implied by `-D warnings` -error: use of `write!(stderr(), ...).unwrap()`. Consider using `eprint!` instead - --> $DIR/explicit_write.rs:25:9 +error: use of `write!(stderr(), ...).unwrap()` + --> $DIR/explicit_write.rs:29:9 | -25 | write!(std::io::stderr(), "test").unwrap(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +29 | write!(std::io::stderr(), "test").unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `eprint!("test")` -error: use of `writeln!(stdout(), ...).unwrap()`. Consider using `println!` instead - --> $DIR/explicit_write.rs:26:9 +error: use of `writeln!(stdout(), ...).unwrap()` + --> $DIR/explicit_write.rs:30:9 | -26 | writeln!(std::io::stdout(), "test").unwrap(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +30 | writeln!(std::io::stdout(), "test").unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `println!("test")` -error: use of `writeln!(stderr(), ...).unwrap()`. Consider using `eprintln!` instead - --> $DIR/explicit_write.rs:27:9 +error: use of `writeln!(stderr(), ...).unwrap()` + --> $DIR/explicit_write.rs:31:9 | -27 | writeln!(std::io::stderr(), "test").unwrap(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +31 | writeln!(std::io::stderr(), "test").unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `eprintln!("test")` -error: use of `stdout().write_fmt(...).unwrap()`. Consider using `print!` instead - --> $DIR/explicit_write.rs:28:9 +error: use of `stdout().write_fmt(...).unwrap()` + --> $DIR/explicit_write.rs:32:9 | -28 | std::io::stdout().write_fmt(format_args!("test")).unwrap(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +32 | std::io::stdout().write_fmt(format_args!("test")).unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `print!("test")` -error: use of `stderr().write_fmt(...).unwrap()`. Consider using `eprint!` instead - --> $DIR/explicit_write.rs:29:9 +error: use of `stderr().write_fmt(...).unwrap()` + --> $DIR/explicit_write.rs:33:9 + | +33 | std::io::stderr().write_fmt(format_args!("test")).unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `eprint!("test")` + +error: use of `writeln!(stdout(), ...).unwrap()` + --> $DIR/explicit_write.rs:36:9 + | +36 | writeln!(std::io::stdout(), "test/ntest").unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `println!("test/ntest")` + +error: use of `writeln!(stderr(), ...).unwrap()` + --> $DIR/explicit_write.rs:37:9 | -29 | std::io::stderr().write_fmt(format_args!("test")).unwrap(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +37 | writeln!(std::io::stderr(), "test/ntest").unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `eprintln!("test/ntest")` -error: aborting due to 6 previous errors +error: aborting due to 8 previous errors From 7e7a33c72695bae3316bf8b211e4bfb698ed7686 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Fri, 23 Nov 2018 21:29:27 +0100 Subject: [PATCH 2/7] Check array lengths to prevent OOB access --- clippy_lints/src/explicit_write.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/clippy_lints/src/explicit_write.rs b/clippy_lints/src/explicit_write.rs index 65fa6aeec0b5..e2b643c4de6d 100644 --- a/clippy_lints/src/explicit_write.rs +++ b/clippy_lints/src/explicit_write.rs @@ -52,9 +52,12 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { if let ExprKind::MethodCall(ref write_fun, _, ref write_args) = unwrap_args[0].node; // Obtain the string that should be printed + if write_args.len() > 1; if let ExprKind::Call(_, ref output_args) = write_args[1].node; + if output_args.len() > 0; if let ExprKind::AddrOf(_, ref output_string_expr) = output_args[0].node; if let ExprKind::Array(ref string_exprs) = output_string_expr.node; + if string_exprs.len() > 0; if let ExprKind::Lit(ref lit) = string_exprs[0].node; if let LitKind::Str(ref write_output, _) = lit.node; if write_fun.ident.name == "write_fmt"; From 5f007a88b4d650068525d4300aa9d773a572de11 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Sat, 24 Nov 2018 12:17:43 +0100 Subject: [PATCH 3/7] Extract method --- clippy_lints/src/explicit_write.rs | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/clippy_lints/src/explicit_write.rs b/clippy_lints/src/explicit_write.rs index e2b643c4de6d..996dc126ec17 100644 --- a/clippy_lints/src/explicit_write.rs +++ b/clippy_lints/src/explicit_write.rs @@ -51,15 +51,6 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { if unwrap_args.len() > 0; if let ExprKind::MethodCall(ref write_fun, _, ref write_args) = unwrap_args[0].node; - // Obtain the string that should be printed - if write_args.len() > 1; - if let ExprKind::Call(_, ref output_args) = write_args[1].node; - if output_args.len() > 0; - if let ExprKind::AddrOf(_, ref output_string_expr) = output_args[0].node; - if let ExprKind::Array(ref string_exprs) = output_string_expr.node; - if string_exprs.len() > 0; - if let ExprKind::Lit(ref lit) = string_exprs[0].node; - if let LitKind::Str(ref write_output, _) = lit.node; if write_fun.ident.name == "write_fmt"; // match calls to std::io::stdout() / std::io::stderr () if write_args.len() > 0; @@ -94,7 +85,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { // We need to remove the last trailing newline from the string because the // underlying `fmt::write` function doesn't know wether `println!` or `print!` was // used. - let mut write_output: String = write_output.to_string(); + let mut write_output: String = write_output_string(write_args).unwrap(); if write_output.ends_with('\n') { write_output.truncate(write_output.len() - 1) } @@ -125,3 +116,22 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { } } } + +// Extract the output string from the given `write_args`. +fn write_output_string(write_args: &HirVec) -> Option { + if_chain! { + // Obtain the string that should be printed + if write_args.len() > 1; + if let ExprKind::Call(_, ref output_args) = write_args[1].node; + if output_args.len() > 0; + if let ExprKind::AddrOf(_, ref output_string_expr) = output_args[0].node; + if let ExprKind::Array(ref string_exprs) = output_string_expr.node; + if string_exprs.len() > 0; + if let ExprKind::Lit(ref lit) = string_exprs[0].node; + if let LitKind::Str(ref write_output, _) = lit.node; + then { + return Some(write_output.to_string()) + } + } + None +} From 9a6216ed05e353825a712c265923993b8b19d887 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Thu, 29 Nov 2018 08:15:44 +0100 Subject: [PATCH 4/7] Address review feedback * Fix typo * Handle None value instead of using `unwrap()` * `pop()` instead of `x.truncate(x.len() - 1)` --- clippy_lints/src/explicit_write.rs | 78 ++++++++++++++++++++---------- 1 file changed, 52 insertions(+), 26 deletions(-) diff --git a/clippy_lints/src/explicit_write.rs b/clippy_lints/src/explicit_write.rs index 996dc126ec17..79c4c5f05306 100644 --- a/clippy_lints/src/explicit_write.rs +++ b/clippy_lints/src/explicit_write.rs @@ -83,35 +83,61 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { }; // We need to remove the last trailing newline from the string because the - // underlying `fmt::write` function doesn't know wether `println!` or `print!` was + // underlying `fmt::write` function doesn't know whether `println!` or `print!` was // used. - let mut write_output: String = write_output_string(write_args).unwrap(); - if write_output.ends_with('\n') { - write_output.truncate(write_output.len() - 1) - } - if let Some(macro_name) = calling_macro { - span_lint_and_sugg( - cx, - EXPLICIT_WRITE, - expr.span, - &format!( - "use of `{}!({}(), ...).unwrap()`", - macro_name, - dest_name - ), - "try this", - format!("{}{}!(\"{}\")", prefix, macro_name.replace("write", "print"), write_output.escape_default()) - ); + if let Some(mut write_output) = write_output_string(write_args) { + if write_output.ends_with('\n') { + write_output.pop(); + } + + if let Some(macro_name) = calling_macro { + span_lint_and_sugg( + cx, + EXPLICIT_WRITE, + expr.span, + &format!( + "use of `{}!({}(), ...).unwrap()`", + macro_name, + dest_name + ), + "try this", + format!("{}{}!(\"{}\")", prefix, macro_name.replace("write", "print"), write_output.escape_default()) + ); + } else { + span_lint_and_sugg( + cx, + EXPLICIT_WRITE, + expr.span, + &format!("use of `{}().write_fmt(...).unwrap()`", dest_name), + "try this", + format!("{}print!(\"{}\")", prefix, write_output.escape_default()) + ); + } } else { - span_lint_and_sugg( - cx, - EXPLICIT_WRITE, - expr.span, - &format!("use of `{}().write_fmt(...).unwrap()`", dest_name), - "try this", - format!("{}print!(\"{}\")", prefix, write_output.escape_default()) - ); + // We don't have a proper suggestion + if let Some(macro_name) = calling_macro { + span_lint( + cx, + EXPLICIT_WRITE, + expr.span, + &format!( + "use of `{}!({}(), ...).unwrap()`. Consider using `{}{}!` instead", + macro_name, + dest_name, + prefix, + macro_name.replace("write", "print") + ) + ); + } else { + span_lint( + cx, + EXPLICIT_WRITE, + expr.span, + &format!("use of `{}().write_fmt(...).unwrap()`. Consider using `{}print!` instead", dest_name, prefix), + ); + } } + } } } From 752724546a664e0fe3c2315161336d99da41c2a9 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Thu, 29 Nov 2018 22:17:40 +0100 Subject: [PATCH 5/7] Make suggestion Applicability::MachineApplicable --- clippy_lints/src/explicit_write.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/explicit_write.rs b/clippy_lints/src/explicit_write.rs index 79c4c5f05306..8c3c84615043 100644 --- a/clippy_lints/src/explicit_write.rs +++ b/clippy_lints/src/explicit_write.rs @@ -7,6 +7,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +use crate::rustc_errors::Applicability; use crate::rustc::hir::*; use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; use crate::rustc::{declare_tool_lint, lint_array}; @@ -101,7 +102,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { dest_name ), "try this", - format!("{}{}!(\"{}\")", prefix, macro_name.replace("write", "print"), write_output.escape_default()) + format!("{}{}!(\"{}\")", prefix, macro_name.replace("write", "print"), write_output.escape_default()), + Applicability::MachineApplicable ); } else { span_lint_and_sugg( @@ -110,7 +112,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { expr.span, &format!("use of `{}().write_fmt(...).unwrap()`", dest_name), "try this", - format!("{}print!(\"{}\")", prefix, write_output.escape_default()) + format!("{}print!(\"{}\")", prefix, write_output.escape_default()), + Applicability::MachineApplicable ); } } else { From 499aad1e04d4388a059b636093f836117bdba5df Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Fri, 30 Nov 2018 07:25:55 +0100 Subject: [PATCH 6/7] cargo fmt and remove stabilized feature --- clippy_lints/src/explicit_write.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/explicit_write.rs b/clippy_lints/src/explicit_write.rs index 8c3c84615043..a0db3ae8df35 100644 --- a/clippy_lints/src/explicit_write.rs +++ b/clippy_lints/src/explicit_write.rs @@ -7,10 +7,10 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -use crate::rustc_errors::Applicability; use crate::rustc::hir::*; use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; use crate::rustc::{declare_tool_lint, lint_array}; +use crate::rustc_errors::Applicability; use crate::syntax::ast::LitKind; use crate::utils::{is_expn_of, match_def_path, opt_def_id, resolve_node, span_lint, span_lint_and_sugg}; use if_chain::if_chain; From 194acaf8e725619ed350ced85c480909003111d6 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Wed, 12 Dec 2018 07:33:23 +0100 Subject: [PATCH 7/7] Update .stderr after rebase --- tests/ui/explicit_write.stderr | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/tests/ui/explicit_write.stderr b/tests/ui/explicit_write.stderr index 6d318d09e584..1a11dbc169bd 100644 --- a/tests/ui/explicit_write.stderr +++ b/tests/ui/explicit_write.stderr @@ -1,51 +1,51 @@ error: use of `write!(stdout(), ...).unwrap()` - --> $DIR/explicit_write.rs:28:9 + --> $DIR/explicit_write.rs:24:9 | -28 | write!(std::io::stdout(), "test").unwrap(); +24 | write!(std::io::stdout(), "test").unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `print!("test")` | = note: `-D clippy::explicit-write` implied by `-D warnings` error: use of `write!(stderr(), ...).unwrap()` - --> $DIR/explicit_write.rs:29:9 + --> $DIR/explicit_write.rs:25:9 | -29 | write!(std::io::stderr(), "test").unwrap(); +25 | write!(std::io::stderr(), "test").unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `eprint!("test")` error: use of `writeln!(stdout(), ...).unwrap()` - --> $DIR/explicit_write.rs:30:9 + --> $DIR/explicit_write.rs:26:9 | -30 | writeln!(std::io::stdout(), "test").unwrap(); +26 | writeln!(std::io::stdout(), "test").unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `println!("test")` error: use of `writeln!(stderr(), ...).unwrap()` - --> $DIR/explicit_write.rs:31:9 + --> $DIR/explicit_write.rs:27:9 | -31 | writeln!(std::io::stderr(), "test").unwrap(); +27 | writeln!(std::io::stderr(), "test").unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `eprintln!("test")` error: use of `stdout().write_fmt(...).unwrap()` - --> $DIR/explicit_write.rs:32:9 + --> $DIR/explicit_write.rs:28:9 | -32 | std::io::stdout().write_fmt(format_args!("test")).unwrap(); +28 | std::io::stdout().write_fmt(format_args!("test")).unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `print!("test")` error: use of `stderr().write_fmt(...).unwrap()` - --> $DIR/explicit_write.rs:33:9 + --> $DIR/explicit_write.rs:29:9 | -33 | std::io::stderr().write_fmt(format_args!("test")).unwrap(); +29 | std::io::stderr().write_fmt(format_args!("test")).unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `eprint!("test")` error: use of `writeln!(stdout(), ...).unwrap()` - --> $DIR/explicit_write.rs:36:9 + --> $DIR/explicit_write.rs:32:9 | -36 | writeln!(std::io::stdout(), "test/ntest").unwrap(); +32 | writeln!(std::io::stdout(), "test/ntest").unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `println!("test/ntest")` error: use of `writeln!(stderr(), ...).unwrap()` - --> $DIR/explicit_write.rs:37:9 + --> $DIR/explicit_write.rs:33:9 | -37 | writeln!(std::io::stderr(), "test/ntest").unwrap(); +33 | writeln!(std::io::stderr(), "test/ntest").unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `eprintln!("test/ntest")` error: aborting due to 8 previous errors