From d7129919172d3f4df4e985d377b5ade41a345e9f Mon Sep 17 00:00:00 2001 From: Evan Simmons Date: Wed, 4 Apr 2018 20:46:39 -0700 Subject: [PATCH] New lints for write! / writeln! macros. --- clippy_lints/src/lib.rs | 21 +- clippy_lints/src/print.rs | 292 ------------------ clippy_lints/src/utils/paths.rs | 1 + clippy_lints/src/write.rs | 437 +++++++++++++++++++++++++++ tests/ui/print.rs | 2 +- tests/ui/write_literal.rs | 35 +++ tests/ui/write_literal.stderr | 100 ++++++ tests/ui/write_with_newline.rs | 25 ++ tests/ui/write_with_newline.stderr | 28 ++ tests/ui/writeln_empty_string.rs | 16 + tests/ui/writeln_empty_string.stderr | 10 + 11 files changed, 663 insertions(+), 304 deletions(-) delete mode 100644 clippy_lints/src/print.rs create mode 100644 clippy_lints/src/write.rs create mode 100644 tests/ui/write_literal.rs create mode 100644 tests/ui/write_literal.stderr create mode 100644 tests/ui/write_with_newline.rs create mode 100644 tests/ui/write_with_newline.stderr create mode 100644 tests/ui/writeln_empty_string.rs create mode 100644 tests/ui/writeln_empty_string.stderr diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 8b8da64eaea..64661b1f545 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -10,7 +10,6 @@ #![feature(macro_vis_matcher)] #![allow(unknown_lints, indexing_slicing, shadow_reuse, missing_docs_in_private_items)] #![recursion_limit = "256"] - // FIXME(mark-i-m) remove after i128 stablization merges #![allow(stable_features)] #![feature(i128, i128_type)] @@ -172,7 +171,6 @@ pub mod overflow_check_conditional; pub mod panic; pub mod partialeq_ne_impl; pub mod precedence; -pub mod print; pub mod ptr; pub mod question_mark; pub mod ranges; @@ -195,6 +193,7 @@ pub mod unused_io_amount; pub mod unused_label; pub mod use_self; pub mod vec; +pub mod write; pub mod zero_div_zero; // end lints modules, do not remove this comment, it’s used in `update_lints` @@ -343,7 +342,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { reg.register_late_lint_pass(box strings::StringLitAsBytes); reg.register_late_lint_pass(box derive::Derive); reg.register_late_lint_pass(box types::CharLitAsU8); - reg.register_late_lint_pass(box print::Pass); + reg.register_late_lint_pass(box write::Pass); reg.register_late_lint_pass(box vec::Pass); reg.register_early_lint_pass(box non_expressive_names::NonExpressiveNames { single_char_binding_names_threshold: conf.single_char_binding_names_threshold, @@ -418,8 +417,8 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { methods::WRONG_PUB_SELF_CONVENTION, misc::FLOAT_CMP_CONST, missing_doc::MISSING_DOCS_IN_PRIVATE_ITEMS, - print::PRINT_STDOUT, - print::USE_DEBUG, + write::PRINT_STDOUT, + write::USE_DEBUG, shadow::SHADOW_REUSE, shadow::SHADOW_SAME, shadow::SHADOW_UNRELATED, @@ -610,9 +609,9 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { panic::PANIC_PARAMS, partialeq_ne_impl::PARTIALEQ_NE_IMPL, precedence::PRECEDENCE, - print::PRINT_LITERAL, - print::PRINT_WITH_NEWLINE, - print::PRINTLN_EMPTY_STRING, + write::PRINT_LITERAL, + write::PRINT_WITH_NEWLINE, + write::PRINTLN_EMPTY_STRING, ptr::CMP_NULL, ptr::MUT_FROM_REF, ptr::PTR_ARG, @@ -724,9 +723,9 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { non_expressive_names::MANY_SINGLE_CHAR_NAMES, ok_if_let::IF_LET_SOME_RESULT, panic::PANIC_PARAMS, - print::PRINT_LITERAL, - print::PRINT_WITH_NEWLINE, - print::PRINTLN_EMPTY_STRING, + write::PRINT_LITERAL, + write::PRINT_WITH_NEWLINE, + write::PRINTLN_EMPTY_STRING, ptr::CMP_NULL, ptr::PTR_ARG, question_mark::QUESTION_MARK, diff --git a/clippy_lints/src/print.rs b/clippy_lints/src/print.rs deleted file mode 100644 index ddfe6d68f4a..00000000000 --- a/clippy_lints/src/print.rs +++ /dev/null @@ -1,292 +0,0 @@ -use std::ops::Deref; -use rustc::hir::*; -use rustc::hir::map::Node::{NodeImplItem, NodeItem}; -use rustc::lint::*; -use syntax::ast::LitKind; -use syntax::symbol::InternedString; -use syntax_pos::Span; -use utils::{is_expn_of, match_def_path, match_path, resolve_node, span_lint, span_lint_and_sugg}; -use utils::{opt_def_id, paths}; - -/// **What it does:** This lint warns when you use `println!("")` to -/// print a newline. -/// -/// **Why is this bad?** You should use `println!()`, which is simpler. -/// -/// **Known problems:** None. -/// -/// **Example:** -/// ```rust -/// println!(""); -/// ``` -declare_clippy_lint! { - pub PRINTLN_EMPTY_STRING, - style, - "using `println!(\"\")` with an empty string" -} - -/// **What it does:** This lint warns when you use `print!()` with a format -/// string that -/// ends in a newline. -/// -/// **Why is this bad?** You should use `println!()` instead, which appends the -/// newline. -/// -/// **Known problems:** None. -/// -/// **Example:** -/// ```rust -/// print!("Hello {}!\n", name); -/// ``` -declare_clippy_lint! { - pub PRINT_WITH_NEWLINE, - style, - "using `print!()` with a format string that ends in a newline" -} - -/// **What it does:** Checks for printing on *stdout*. The purpose of this lint -/// is to catch debugging remnants. -/// -/// **Why is this bad?** People often print on *stdout* while debugging an -/// application and might forget to remove those prints afterward. -/// -/// **Known problems:** Only catches `print!` and `println!` calls. -/// -/// **Example:** -/// ```rust -/// println!("Hello world!"); -/// ``` -declare_clippy_lint! { - pub PRINT_STDOUT, - restriction, - "printing on stdout" -} - -/// **What it does:** Checks for use of `Debug` formatting. The purpose of this -/// lint is to catch debugging remnants. -/// -/// **Why is this bad?** The purpose of the `Debug` trait is to facilitate -/// debugging Rust code. It should not be used in in user-facing output. -/// -/// **Example:** -/// ```rust -/// println!("{:?}", foo); -/// ``` -declare_clippy_lint! { - pub USE_DEBUG, - restriction, - "use of `Debug`-based formatting" -} - -/// **What it does:** This lint warns about the use of literals as `print!`/`println!` args. -/// -/// **Why is this bad?** Using literals as `println!` args is inefficient -/// (c.f., https://github.com/matthiaskrgr/rust-str-bench) and unnecessary -/// (i.e., just put the literal in the format string) -/// -/// **Known problems:** Will also warn with macro calls as arguments that expand to literals -/// -- e.g., `println!("{}", env!("FOO"))`. -/// -/// **Example:** -/// ```rust -/// println!("{}", "foo"); -/// ``` -declare_clippy_lint! { - pub PRINT_LITERAL, - style, - "printing a literal with a format string" -} - -#[derive(Copy, Clone, Debug)] -pub struct Pass; - -impl LintPass for Pass { - fn get_lints(&self) -> LintArray { - lint_array!(PRINT_WITH_NEWLINE, PRINTLN_EMPTY_STRING, PRINT_STDOUT, USE_DEBUG, PRINT_LITERAL) - } -} - -impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { - fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { - if_chain! { - if let ExprCall(ref fun, ref args) = expr.node; - if let ExprPath(ref qpath) = fun.node; - if let Some(fun_id) = opt_def_id(resolve_node(cx, qpath, fun.hir_id)); - then { - - // Search for `std::io::_print(..)` which is unique in a - // `print!` expansion. - if match_def_path(cx.tcx, fun_id, &paths::IO_PRINT) { - if let Some(span) = is_expn_of(expr.span, "print") { - // `println!` uses `print!`. - let (span, name) = match is_expn_of(span, "println") { - Some(span) => (span, "println"), - None => (span, "print"), - }; - - span_lint(cx, PRINT_STDOUT, span, &format!("use of `{}!`", name)); - - // Check for literals in the print!/println! args - // Also, ensure the format string is `{}` with no special options, like `{:X}` - check_print_args_for_literal(cx, args); - - if_chain! { - // ensure we're calling Arguments::new_v1 - if args.len() == 1; - if let ExprCall(ref args_fun, ref args_args) = args[0].node; - if let ExprPath(ref qpath) = args_fun.node; - if let Some(const_def_id) = opt_def_id(resolve_node(cx, qpath, args_fun.hir_id)); - if match_def_path(cx.tcx, const_def_id, &paths::FMT_ARGUMENTS_NEWV1); - if args_args.len() == 2; - if let ExprAddrOf(_, ref match_expr) = args_args[1].node; - if let ExprMatch(ref args, _, _) = match_expr.node; - if let ExprTup(ref args) = args.node; - if let Some((fmtstr, fmtlen)) = get_argument_fmtstr_parts(&args_args[0]); - then { - match name { - "print" => check_print(cx, span, args, fmtstr, fmtlen), - "println" => check_println(cx, span, fmtstr, fmtlen), - _ => (), - } - } - } - } - } - // Search for something like - // `::std::fmt::ArgumentV1::new(__arg0, ::std::fmt::Debug::fmt)` - else if args.len() == 2 && match_def_path(cx.tcx, fun_id, &paths::FMT_ARGUMENTV1_NEW) { - if let ExprPath(ref qpath) = args[1].node { - if let Some(def_id) = opt_def_id(cx.tables.qpath_def(qpath, args[1].hir_id)) { - if match_def_path(cx.tcx, def_id, &paths::DEBUG_FMT_METHOD) - && !is_in_debug_impl(cx, expr) && is_expn_of(expr.span, "panic").is_none() { - span_lint(cx, USE_DEBUG, args[0].span, "use of `Debug`-based formatting"); - } - } - } - } - } - } - } -} - -// Check for literals in print!/println! args -// ensuring the format string for the literal is `DISPLAY_FMT_METHOD` -// e.g., `println!("... {} ...", "foo")` -// ^ literal in `println!` -fn check_print_args_for_literal<'a, 'tcx>( - cx: &LateContext<'a, 'tcx>, - args: &HirVec -) { - if_chain! { - if args.len() == 1; - if let ExprCall(_, ref args_args) = args[0].node; - if args_args.len() > 1; - if let ExprAddrOf(_, ref match_expr) = args_args[1].node; - if let ExprMatch(ref matchee, ref arms, _) = match_expr.node; - if let ExprTup(ref tup) = matchee.node; - if arms.len() == 1; - if let ExprArray(ref arm_body_exprs) = arms[0].body.node; - then { - // it doesn't matter how many args there are in the `print!`/`println!`, - // if there's one literal, we should warn the user - for (idx, tup_arg) in tup.iter().enumerate() { - if_chain! { - // first, make sure we're dealing with a literal (i.e., an ExprLit) - if let ExprAddrOf(_, ref tup_val) = tup_arg.node; - if let ExprLit(_) = tup_val.node; - - // next, check the corresponding match arm body to ensure - // this is "{}", or DISPLAY_FMT_METHOD - if let ExprCall(_, ref body_args) = arm_body_exprs[idx].node; - if body_args.len() == 2; - if let ExprPath(ref body_qpath) = body_args[1].node; - if let Some(fun_def_id) = opt_def_id(resolve_node(cx, body_qpath, body_args[1].hir_id)); - if match_def_path(cx.tcx, fun_def_id, &paths::DISPLAY_FMT_METHOD) || - match_def_path(cx.tcx, fun_def_id, &paths::DEBUG_FMT_METHOD); - then { - span_lint(cx, PRINT_LITERAL, tup_val.span, "printing a literal with an empty format string"); - } - } - } - } - } -} - -// Check for print!("... \n", ...). -fn check_print<'a, 'tcx>( - cx: &LateContext<'a, 'tcx>, - span: Span, - args: &HirVec, - fmtstr: InternedString, - fmtlen: usize, -) { - if_chain! { - // check the final format string part - if let Some('\n') = fmtstr.chars().last(); - - // "foo{}bar" is made into two strings + one argument, - // if the format string starts with `{}` (eg. "{}foo"), - // the string array is prepended an empty string "". - // We only want to check the last string after any `{}`: - if args.len() < fmtlen; - then { - span_lint(cx, PRINT_WITH_NEWLINE, span, - "using `print!()` with a format string that ends in a \ - newline, consider using `println!()` instead"); - } - } -} - -/// Check for println!("") -fn check_println<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, span: Span, fmtstr: InternedString, fmtlen: usize) { - if_chain! { - // check that the string is empty - if fmtlen == 1; - if fmtstr.deref() == "\n"; - - // check the presence of that string - if let Ok(snippet) = cx.sess().codemap().span_to_snippet(span); - if snippet.contains("\"\""); - then { - span_lint_and_sugg( - cx, - PRINT_WITH_NEWLINE, - span, - "using `println!(\"\")`", - "replace it with", - "println!()".to_string(), - ); - } - } -} - -fn is_in_debug_impl(cx: &LateContext, expr: &Expr) -> bool { - let map = &cx.tcx.hir; - - // `fmt` method - if let Some(NodeImplItem(item)) = map.find(map.get_parent(expr.id)) { - // `Debug` impl - if let Some(NodeItem(item)) = map.find(map.get_parent(item.id)) { - if let ItemImpl(_, _, _, _, Some(ref tr), _, _) = item.node { - return match_path(&tr.path, &["Debug"]); - } - } - } - - false -} - -/// Returns the slice of format string parts in an `Arguments::new_v1` call. -fn get_argument_fmtstr_parts(expr: &Expr) -> Option<(InternedString, usize)> { - if_chain! { - if let ExprAddrOf(_, ref expr) = expr.node; // &["…", "…", …] - if let ExprArray(ref exprs) = expr.node; - if let Some(expr) = exprs.last(); - if let ExprLit(ref lit) = expr.node; - if let LitKind::Str(ref lit, _) = lit.node; - then { - return Some((lit.as_str(), exprs.len())); - } - } - None -} diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index eecae8b230b..8f823c12cf3 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/clippy_lints/src/utils/paths.rs @@ -26,6 +26,7 @@ pub const DISPLAY_FMT_METHOD: [&str; 4] = ["core", "fmt", "Display", "fmt"]; pub const DOUBLE_ENDED_ITERATOR: [&str; 4] = ["core", "iter", "traits", "DoubleEndedIterator"]; pub const DROP: [&str; 3] = ["core", "mem", "drop"]; pub const FMT_ARGUMENTS_NEWV1: [&str; 4] = ["core", "fmt", "Arguments", "new_v1"]; +pub const FMT_ARGUMENTS_NEWV1FORMATTED: [&str; 4] = ["core", "fmt", "Arguments", "new_v1_formatted"]; pub const FMT_ARGUMENTV1_NEW: [&str; 4] = ["core", "fmt", "ArgumentV1", "new"]; pub const FROM_FROM: [&str; 4] = ["core", "convert", "From", "from"]; pub const FROM_TRAIT: [&str; 3] = ["core", "convert", "From"]; diff --git a/clippy_lints/src/write.rs b/clippy_lints/src/write.rs new file mode 100644 index 00000000000..0531c14cba8 --- /dev/null +++ b/clippy_lints/src/write.rs @@ -0,0 +1,437 @@ +use rustc::hir::map::Node::{NodeImplItem, NodeItem}; +use rustc::hir::*; +use rustc::lint::*; +use std::ops::Deref; +use syntax::ast::LitKind; +use syntax::ptr; +use syntax::symbol::InternedString; +use syntax_pos::Span; +use utils::{is_expn_of, match_def_path, match_path, resolve_node, span_lint, span_lint_and_sugg}; +use utils::{opt_def_id, paths}; + +/// **What it does:** This lint warns when you use `println!("")` to +/// print a newline. +/// +/// **Why is this bad?** You should use `println!()`, which is simpler. +/// +/// **Known problems:** None. +/// +/// **Example:** +/// ```rust +/// println!(""); +/// ``` +declare_clippy_lint! { + pub PRINTLN_EMPTY_STRING, + style, + "using `println!(\"\")` with an empty string" +} + +/// **What it does:** This lint warns when you use `print!()` with a format +/// string that +/// ends in a newline. +/// +/// **Why is this bad?** You should use `println!()` instead, which appends the +/// newline. +/// +/// **Known problems:** None. +/// +/// **Example:** +/// ```rust +/// print!("Hello {}!\n", name); +/// ``` +declare_clippy_lint! { + pub PRINT_WITH_NEWLINE, + style, + "using `print!()` with a format string that ends in a newline" +} + +/// **What it does:** Checks for printing on *stdout*. The purpose of this lint +/// is to catch debugging remnants. +/// +/// **Why is this bad?** People often print on *stdout* while debugging an +/// application and might forget to remove those prints afterward. +/// +/// **Known problems:** Only catches `print!` and `println!` calls. +/// +/// **Example:** +/// ```rust +/// println!("Hello world!"); +/// ``` +declare_clippy_lint! { + pub PRINT_STDOUT, + restriction, + "printing on stdout" +} + +/// **What it does:** Checks for use of `Debug` formatting. The purpose of this +/// lint is to catch debugging remnants. +/// +/// **Why is this bad?** The purpose of the `Debug` trait is to facilitate +/// debugging Rust code. It should not be used in in user-facing output. +/// +/// **Example:** +/// ```rust +/// println!("{:?}", foo); +/// ``` +declare_clippy_lint! { + pub USE_DEBUG, + restriction, + "use of `Debug`-based formatting" +} + +/// **What it does:** This lint warns about the use of literals as `print!`/`println!` args. +/// +/// **Why is this bad?** Using literals as `println!` args is inefficient +/// (c.f., https://github.com/matthiaskrgr/rust-str-bench) and unnecessary +/// (i.e., just put the literal in the format string) +/// +/// **Known problems:** Will also warn with macro calls as arguments that expand to literals +/// -- e.g., `println!("{}", env!("FOO"))`. +/// +/// **Example:** +/// ```rust +/// println!("{}", "foo"); +/// ``` +declare_clippy_lint! { + pub PRINT_LITERAL, + style, + "printing a literal with a format string" +} + +/// **What it does:** This lint warns when you use `writeln!(buf, "")` to +/// print a newline. +/// +/// **Why is this bad?** You should use `writeln!(buf)`, which is simpler. +/// +/// **Known problems:** None. +/// +/// **Example:** +/// ```rust +/// writeln!(""); +/// ``` +declare_clippy_lint! { + pub WRITELN_EMPTY_STRING, + style, + "using `writeln!(\"\")` with an empty string" +} + +/// **What it does:** This lint warns when you use `write!()` with a format +/// string that +/// ends in a newline. +/// +/// **Why is this bad?** You should use `writeln!()` instead, which appends the +/// newline. +/// +/// **Known problems:** None. +/// +/// **Example:** +/// ```rust +/// write!(buf, "Hello {}!\n", name); +/// ``` +declare_clippy_lint! { + pub WRITE_WITH_NEWLINE, + style, + "using `write!()` with a format string that ends in a newline" +} + +/// **What it does:** This lint warns about the use of literals as `write!`/`writeln!` args. +/// +/// **Why is this bad?** Using literals as `writeln!` args is inefficient +/// (c.f., https://github.com/matthiaskrgr/rust-str-bench) and unnecessary +/// (i.e., just put the literal in the format string) +/// +/// **Known problems:** Will also warn with macro calls as arguments that expand to literals +/// -- e.g., `writeln!(buf, "{}", env!("FOO"))`. +/// +/// **Example:** +/// ```rust +/// writeln!(buf, "{}", "foo"); +/// ``` +declare_clippy_lint! { + pub WRITE_LITERAL, + style, + "writing a literal with a format string" +} + +#[derive(Copy, Clone, Debug)] +pub struct Pass; + +impl LintPass for Pass { + fn get_lints(&self) -> LintArray { + lint_array!( + PRINT_WITH_NEWLINE, + PRINTLN_EMPTY_STRING, + PRINT_STDOUT, + USE_DEBUG, + PRINT_LITERAL, + WRITE_WITH_NEWLINE, + WRITELN_EMPTY_STRING, + WRITE_LITERAL + ) + } +} + +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { + fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { + match expr.node { + // print!() + ExprCall(ref fun, ref args) => { + if_chain! { + if let ExprPath(ref qpath) = fun.node; + if let Some(fun_id) = opt_def_id(resolve_node(cx, qpath, fun.hir_id)); + then { + check_print_variants(cx, expr, fun_id, args); + } + } + }, + // write!() + ExprMethodCall(ref fun, _, ref args) => { + if fun.name == "write_fmt" { + check_write_variants(cx, expr, args); + } + }, + _ => (), + } + } +} + +fn check_write_variants<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr, write_args: &ptr::P<[Expr]>) { + // `writeln!` uses `write!`. + if let Some(span) = is_expn_of(expr.span, "write") { + let (span, name) = match is_expn_of(span, "writeln") { + Some(span) => (span, "writeln"), + None => (span, "write"), + }; + + if_chain! { + // ensure we're calling Arguments::new_v1 or Arguments::new_v1_formatted + if write_args.len() == 2; + if let ExprCall(ref args_fun, ref args_args) = write_args[1].node; + if let ExprPath(ref qpath) = args_fun.node; + if let Some(const_def_id) = opt_def_id(resolve_node(cx, qpath, args_fun.hir_id)); + if match_def_path(cx.tcx, const_def_id, &paths::FMT_ARGUMENTS_NEWV1) || + match_def_path(cx.tcx, const_def_id, &paths::FMT_ARGUMENTS_NEWV1FORMATTED); + then { + // Check for literals in the write!/writeln! args + check_fmt_args_for_literal(cx, args_args, |span| { + span_lint(cx, WRITE_LITERAL, span, "writing a literal with an empty format string"); + }); + + if_chain! { + if args_args.len() >= 2; + if let ExprAddrOf(_, ref match_expr) = args_args[1].node; + if let ExprMatch(ref args, _, _) = match_expr.node; + if let ExprTup(ref args) = args.node; + if let Some((fmtstr, fmtlen)) = get_argument_fmtstr_parts(&args_args[0]); + then { + match name { + "write" => if has_newline_end(args, fmtstr, fmtlen) { + span_lint(cx, WRITE_WITH_NEWLINE, span, + "using `write!()` with a format string that ends in a \ + newline, consider using `writeln!()` instead"); + }, + "writeln" => if has_empty_arg(cx, span, fmtstr, fmtlen) { + span_lint_and_sugg( + cx, + WRITE_WITH_NEWLINE, + span, + "using `writeln!(v, \"\")`", + "replace it with", + "writeln!(v)".to_string(), + ); + }, + _ => (), + } + } + } + } + } + } +} + +fn check_print_variants<'a, 'tcx>( + cx: &LateContext<'a, 'tcx>, + expr: &'tcx Expr, + fun_id: def_id::DefId, + args: &ptr::P<[Expr]>, +) { + // Search for `std::io::_print(..)` which is unique in a + // `print!` expansion. + if match_def_path(cx.tcx, fun_id, &paths::IO_PRINT) { + if let Some(span) = is_expn_of(expr.span, "print") { + // `println!` uses `print!`. + let (span, name) = match is_expn_of(span, "println") { + Some(span) => (span, "println"), + None => (span, "print"), + }; + + span_lint(cx, PRINT_STDOUT, span, &format!("use of `{}!`", name)); + + if_chain! { + // ensure we're calling Arguments::new_v1 + if args.len() == 1; + if let ExprCall(ref args_fun, ref args_args) = args[0].node; + then { + // Check for literals in the print!/println! args + check_fmt_args_for_literal(cx, args_args, |span| { + span_lint(cx, PRINT_LITERAL, span, "printing a literal with an empty format string"); + }); + + if_chain! { + if let ExprPath(ref qpath) = args_fun.node; + if let Some(const_def_id) = opt_def_id(resolve_node(cx, qpath, args_fun.hir_id)); + if match_def_path(cx.tcx, const_def_id, &paths::FMT_ARGUMENTS_NEWV1); + if args_args.len() == 2; + if let ExprAddrOf(_, ref match_expr) = args_args[1].node; + if let ExprMatch(ref args, _, _) = match_expr.node; + if let ExprTup(ref args) = args.node; + if let Some((fmtstr, fmtlen)) = get_argument_fmtstr_parts(&args_args[0]); + then { + match name { + "print" => + if has_newline_end(args, fmtstr, fmtlen) { + span_lint(cx, PRINT_WITH_NEWLINE, span, + "using `print!()` with a format string that ends in a \ + newline, consider using `println!()` instead"); + }, + "println" => + if has_empty_arg(cx, span, fmtstr, fmtlen) { + span_lint_and_sugg( + cx, + PRINT_WITH_NEWLINE, + span, + "using `println!(\"\")`", + "replace it with", + "println!()".to_string(), + ); + }, + _ => (), + } + } + } + } + } + } + } + // Search for something like + // `::std::fmt::ArgumentV1::new(__arg0, ::std::fmt::Debug::fmt)` + else if args.len() == 2 && match_def_path(cx.tcx, fun_id, &paths::FMT_ARGUMENTV1_NEW) { + if let ExprPath(ref qpath) = args[1].node { + if let Some(def_id) = opt_def_id(cx.tables.qpath_def(qpath, args[1].hir_id)) { + if match_def_path(cx.tcx, def_id, &paths::DEBUG_FMT_METHOD) && !is_in_debug_impl(cx, expr) + && is_expn_of(expr.span, "panic").is_none() + { + span_lint(cx, USE_DEBUG, args[0].span, "use of `Debug`-based formatting"); + } + } + } + } +} + +// Check for literals in write!/writeln! and print!/println! args +// ensuring the format string for the literal is `DISPLAY_FMT_METHOD` +// e.g., `writeln!(buf, "... {} ...", "foo")` +// ^ literal in `writeln!` +// e.g., `println!("... {} ...", "foo")` +// ^ literal in `println!` +fn check_fmt_args_for_literal<'a, 'tcx, F>(cx: &LateContext<'a, 'tcx>, args: &HirVec, lint_fn: F) +where + F: Fn(Span), +{ + if_chain! { + if args.len() > 1; + if let ExprAddrOf(_, ref match_expr) = args[1].node; + if let ExprMatch(ref matchee, ref arms, _) = match_expr.node; + if let ExprTup(ref tup) = matchee.node; + if arms.len() == 1; + if let ExprArray(ref arm_body_exprs) = arms[0].body.node; + then { + // it doesn't matter how many args there are in the `write!`/`writeln!`, + // if there's one literal, we should warn the user + for (idx, tup_arg) in tup.iter().enumerate() { + if_chain! { + // first, make sure we're dealing with a literal (i.e., an ExprLit) + if let ExprAddrOf(_, ref tup_val) = tup_arg.node; + if let ExprLit(_) = tup_val.node; + + // next, check the corresponding match arm body to ensure + // this is "{}", or DISPLAY_FMT_METHOD + if let ExprCall(_, ref body_args) = arm_body_exprs[idx].node; + if body_args.len() == 2; + if let ExprPath(ref body_qpath) = body_args[1].node; + if let Some(fun_def_id) = opt_def_id(resolve_node(cx, body_qpath, body_args[1].hir_id)); + if match_def_path(cx.tcx, fun_def_id, &paths::DISPLAY_FMT_METHOD) || + match_def_path(cx.tcx, fun_def_id, &paths::DEBUG_FMT_METHOD); + then { + lint_fn(tup_val.span); + } + } + } + } + } +} + +/// Check for fmtstr = "... \n" +fn has_newline_end(args: &HirVec, fmtstr: InternedString, fmtlen: usize) -> bool { + if_chain! { + // check the final format string part + if let Some('\n') = fmtstr.chars().last(); + + // "foo{}bar" is made into two strings + one argument, + // if the format string starts with `{}` (eg. "{}foo"), + // the string array is prepended an empty string "". + // We only want to check the last string after any `{}`: + if args.len() < fmtlen; + then { + return true + } + } + false +} + +/// Check for writeln!(v, "") / println!("") +fn has_empty_arg<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, span: Span, fmtstr: InternedString, fmtlen: usize) -> bool { + if_chain! { + // check that the string is empty + if fmtlen == 1; + if fmtstr.deref() == "\n"; + + // check the presence of that string + if let Ok(snippet) = cx.sess().codemap().span_to_snippet(span); + if snippet.contains("\"\""); + then { + return true + } + } + false +} + +/// Returns the slice of format string parts in an `Arguments::new_v1` call. +fn get_argument_fmtstr_parts(expr: &Expr) -> Option<(InternedString, usize)> { + if_chain! { + if let ExprAddrOf(_, ref expr) = expr.node; // &["…", "…", …] + if let ExprArray(ref exprs) = expr.node; + if let Some(expr) = exprs.last(); + if let ExprLit(ref lit) = expr.node; + if let LitKind::Str(ref lit, _) = lit.node; + then { + return Some((lit.as_str(), exprs.len())); + } + } + None +} + +fn is_in_debug_impl(cx: &LateContext, expr: &Expr) -> bool { + let map = &cx.tcx.hir; + + // `fmt` method + if let Some(NodeImplItem(item)) = map.find(map.get_parent(expr.id)) { + // `Debug` impl + if let Some(NodeItem(item)) = map.find(map.get_parent(item.id)) { + if let ItemImpl(_, _, _, _, Some(ref tr), _, _) = item.node { + return match_path(&tr.path, &["Debug"]); + } + } + } + false +} diff --git a/tests/ui/print.rs b/tests/ui/print.rs index 786398cfe5e..8719a691d43 100644 --- a/tests/ui/print.rs +++ b/tests/ui/print.rs @@ -1,6 +1,6 @@ -#![allow(print_literal)] +#![allow(print_literal, write_literal)] #![warn(print_stdout, use_debug)] use std::fmt::{Debug, Display, Formatter, Result}; diff --git a/tests/ui/write_literal.rs b/tests/ui/write_literal.rs new file mode 100644 index 00000000000..dd3a869eb4e --- /dev/null +++ b/tests/ui/write_literal.rs @@ -0,0 +1,35 @@ +#![allow(unused_must_use)] +#![warn(write_literal)] + +use std::io::Write; + +fn main() { + let mut v = Vec::new(); + + // These should be fine + write!(&mut v, "Hello"); + writeln!(&mut v, "Hello"); + let world = "world"; + writeln!(&mut v, "Hello {}", world); + writeln!(&mut v, "3 in hex is {:X}", 3); + + // These should throw warnings + write!(&mut v, "Hello {}", "world"); + writeln!(&mut v, "Hello {} {}", world, "world"); + writeln!(&mut v, "Hello {}", "world"); + writeln!(&mut v, "10 / 4 is {}", 2.5); + writeln!(&mut v, "2 + 1 = {}", 3); + writeln!(&mut v, "2 + 1 = {:.4}", 3); + writeln!(&mut v, "2 + 1 = {:5.4}", 3); + writeln!(&mut v, "Debug test {:?}", "hello, world"); + + // positional args don't change the fact + // that we're using a literal -- this should + // throw a warning + writeln!(&mut v, "{0} {1}", "hello", "world"); + writeln!(&mut v, "{1} {0}", "hello", "world"); + + // named args shouldn't change anything either + writeln!(&mut v, "{foo} {bar}", foo = "hello", bar = "world"); + writeln!(&mut v, "{bar} {foo}", foo = "hello", bar = "world"); +} diff --git a/tests/ui/write_literal.stderr b/tests/ui/write_literal.stderr new file mode 100644 index 00000000000..9c068f1332d --- /dev/null +++ b/tests/ui/write_literal.stderr @@ -0,0 +1,100 @@ +error: writing a literal with an empty format string + --> $DIR/write_literal.rs:17:32 + | +17 | write!(&mut v, "Hello {}", "world"); + | ^^^^^^^ + | + = note: `-D write-literal` implied by `-D warnings` + +error: writing a literal with an empty format string + --> $DIR/write_literal.rs:18:44 + | +18 | writeln!(&mut v, "Hello {} {}", world, "world"); + | ^^^^^^^ + +error: writing a literal with an empty format string + --> $DIR/write_literal.rs:19:34 + | +19 | writeln!(&mut v, "Hello {}", "world"); + | ^^^^^^^ + +error: writing a literal with an empty format string + --> $DIR/write_literal.rs:20:38 + | +20 | writeln!(&mut v, "10 / 4 is {}", 2.5); + | ^^^ + +error: writing a literal with an empty format string + --> $DIR/write_literal.rs:21:36 + | +21 | writeln!(&mut v, "2 + 1 = {}", 3); + | ^ + +error: writing a literal with an empty format string + --> $DIR/write_literal.rs:22:39 + | +22 | writeln!(&mut v, "2 + 1 = {:.4}", 3); + | ^ + +error: writing a literal with an empty format string + --> $DIR/write_literal.rs:23:40 + | +23 | writeln!(&mut v, "2 + 1 = {:5.4}", 3); + | ^ + +error: writing a literal with an empty format string + --> $DIR/write_literal.rs:24:41 + | +24 | writeln!(&mut v, "Debug test {:?}", "hello, world"); + | ^^^^^^^^^^^^^^ + +error: writing a literal with an empty format string + --> $DIR/write_literal.rs:29:33 + | +29 | writeln!(&mut v, "{0} {1}", "hello", "world"); + | ^^^^^^^ + +error: writing a literal with an empty format string + --> $DIR/write_literal.rs:29:42 + | +29 | writeln!(&mut v, "{0} {1}", "hello", "world"); + | ^^^^^^^ + +error: writing a literal with an empty format string + --> $DIR/write_literal.rs:30:33 + | +30 | writeln!(&mut v, "{1} {0}", "hello", "world"); + | ^^^^^^^ + +error: writing a literal with an empty format string + --> $DIR/write_literal.rs:30:42 + | +30 | writeln!(&mut v, "{1} {0}", "hello", "world"); + | ^^^^^^^ + +error: writing a literal with an empty format string + --> $DIR/write_literal.rs:33:43 + | +33 | writeln!(&mut v, "{foo} {bar}", foo = "hello", bar = "world"); + | ^^^^^^^ + +error: writing a literal with an empty format string + --> $DIR/write_literal.rs:33:58 + | +33 | writeln!(&mut v, "{foo} {bar}", foo = "hello", bar = "world"); + | ^^^^^^^ + +error: writing a literal with an empty format string + --> $DIR/write_literal.rs:34:43 + | +34 | writeln!(&mut v, "{bar} {foo}", foo = "hello", bar = "world"); + | ^^^^^^^ + +error: writing a literal with an empty format string + --> $DIR/write_literal.rs:34:58 + | +34 | writeln!(&mut v, "{bar} {foo}", foo = "hello", bar = "world"); + | ^^^^^^^ + +error: aborting due to 16 previous errors + diff --git a/tests/ui/write_with_newline.rs b/tests/ui/write_with_newline.rs new file mode 100644 index 00000000000..0427bd3ec04 --- /dev/null +++ b/tests/ui/write_with_newline.rs @@ -0,0 +1,25 @@ +#![allow(write_literal)] +#![warn(write_with_newline)] + +use std::io::Write; + +fn main() { + let mut v = Vec::new(); + + // These should fail + write!(&mut v, "Hello\n"); + write!(&mut v, "Hello {}\n", "world"); + write!(&mut v, "Hello {} {}\n\n", "world", "#2"); + write!(&mut v, "{}\n", 1265); + + // These should be fine + write!(&mut v, ""); + write!(&mut v, "Hello"); + writeln!(&mut v, "Hello"); + writeln!(&mut v, "Hello\n"); + writeln!(&mut v, "Hello {}\n", "world"); + write!(&mut v, "Issue\n{}", 1265); + write!(&mut v, "{}", 1265); + write!(&mut v, "\n{}", 1275); + +} diff --git a/tests/ui/write_with_newline.stderr b/tests/ui/write_with_newline.stderr new file mode 100644 index 00000000000..37f03afb016 --- /dev/null +++ b/tests/ui/write_with_newline.stderr @@ -0,0 +1,28 @@ +error: using `write!()` with a format string that ends in a newline, consider using `writeln!()` instead + --> $DIR/write_with_newline.rs:10:5 + | +10 | write!(&mut v, "Hello/n"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D write-with-newline` implied by `-D warnings` + +error: using `write!()` with a format string that ends in a newline, consider using `writeln!()` instead + --> $DIR/write_with_newline.rs:11:5 + | +11 | write!(&mut v, "Hello {}/n", "world"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: using `write!()` with a format string that ends in a newline, consider using `writeln!()` instead + --> $DIR/write_with_newline.rs:12:5 + | +12 | write!(&mut v, "Hello {} {}/n/n", "world", "#2"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: using `write!()` with a format string that ends in a newline, consider using `writeln!()` instead + --> $DIR/write_with_newline.rs:13:5 + | +13 | write!(&mut v, "{}/n", 1265); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 4 previous errors + diff --git a/tests/ui/writeln_empty_string.rs b/tests/ui/writeln_empty_string.rs new file mode 100644 index 00000000000..c7092eb8c4b --- /dev/null +++ b/tests/ui/writeln_empty_string.rs @@ -0,0 +1,16 @@ +#![allow(unused_must_use)] +#![warn(writeln_empty_string)] +use std::io::Write; + +fn main() { + let mut v = Vec::new(); + + // This should fail + writeln!(&mut v, ""); + + // These should be fine + writeln!(&mut v); + writeln!(&mut v, " "); + write!(&mut v, ""); + +} diff --git a/tests/ui/writeln_empty_string.stderr b/tests/ui/writeln_empty_string.stderr new file mode 100644 index 00000000000..e20aad779d9 --- /dev/null +++ b/tests/ui/writeln_empty_string.stderr @@ -0,0 +1,10 @@ +error: using `writeln!(v, "")` + --> $DIR/writeln_empty_string.rs:9:5 + | +9 | writeln!(&mut v, ""); + | ^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `writeln!(v)` + | + = note: `-D write-with-newline` implied by `-D warnings` + +error: aborting due to previous error +