From ce6e037ac5527c7dd2153a929f2ce8bda2c2bb1a Mon Sep 17 00:00:00 2001 From: sinkuu Date: Sat, 7 Jan 2017 20:35:45 +0900 Subject: [PATCH 1/2] Implement `unused_io_amount` lint --- CHANGELOG.md | 1 + README.md | 3 +- clippy_lints/src/lib.rs | 3 + clippy_lints/src/unused_io_amount.rs | 138 +++++++++++++++++++++++++ clippy_lints/src/utils/paths.rs | 3 + tests/compile-fail/unused_io_amount.rs | 35 +++++++ 6 files changed, 182 insertions(+), 1 deletion(-) create mode 100644 clippy_lints/src/unused_io_amount.rs create mode 100644 tests/compile-fail/unused_io_amount.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d848aebe0b4..d5f7c47fb689 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -414,6 +414,7 @@ All notable changes to this project will be documented in this file. [`unstable_as_mut_slice`]: https://github.com/Manishearth/rust-clippy/wiki#unstable_as_mut_slice [`unstable_as_slice`]: https://github.com/Manishearth/rust-clippy/wiki#unstable_as_slice [`unused_collect`]: https://github.com/Manishearth/rust-clippy/wiki#unused_collect +[`unused_io_amount`]: https://github.com/Manishearth/rust-clippy/wiki#unused_io_amount [`unused_label`]: https://github.com/Manishearth/rust-clippy/wiki#unused_label [`unused_lifetimes`]: https://github.com/Manishearth/rust-clippy/wiki#unused_lifetimes [`use_debug`]: https://github.com/Manishearth/rust-clippy/wiki#use_debug diff --git a/README.md b/README.md index f0a362295fca..37bd3db944d7 100644 --- a/README.md +++ b/README.md @@ -179,7 +179,7 @@ transparently: ## Lints -There are 183 lints included in this crate: +There are 184 lints included in this crate: name | default | triggers on -----------------------------------------------------------------------------------------------------------------------|---------|---------------------------------------------------------------------------------------------------------------------------------- @@ -349,6 +349,7 @@ name [unsafe_removed_from_name](https://github.com/Manishearth/rust-clippy/wiki#unsafe_removed_from_name) | warn | `unsafe` removed from API names on import [unseparated_literal_suffix](https://github.com/Manishearth/rust-clippy/wiki#unseparated_literal_suffix) | allow | literals whose suffix is not separated by an underscore [unused_collect](https://github.com/Manishearth/rust-clippy/wiki#unused_collect) | warn | `collect()`ing an iterator without using the result; this is usually better written as a for loop +[unused_io_amount](https://github.com/Manishearth/rust-clippy/wiki#unused_io_amount) | deny | unused written/read amount [unused_label](https://github.com/Manishearth/rust-clippy/wiki#unused_label) | warn | unused labels [unused_lifetimes](https://github.com/Manishearth/rust-clippy/wiki#unused_lifetimes) | warn | unused lifetimes in function definitions [use_debug](https://github.com/Manishearth/rust-clippy/wiki#use_debug) | allow | use of `Debug`-based formatting diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index f362ff12a85c..b88b62040cec 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -131,6 +131,7 @@ pub mod transmute; pub mod types; pub mod unicode; pub mod unsafe_removed_from_name; +pub mod unused_io_amount; pub mod unused_label; pub mod vec; pub mod zero_div_zero; @@ -287,6 +288,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { reg.register_late_lint_pass(box partialeq_ne_impl::Pass); reg.register_early_lint_pass(box reference::Pass); reg.register_early_lint_pass(box double_parens::DoubleParens); + reg.register_late_lint_pass(box unused_io_amount::UnusedIoAmount); reg.register_lint_group("clippy_restrictions", vec![ arithmetic::FLOAT_ARITHMETIC, @@ -480,6 +482,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { types::UNIT_CMP, unicode::ZERO_WIDTH_SPACE, unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME, + unused_io_amount::UNUSED_IO_AMOUNT, unused_label::UNUSED_LABEL, vec::USELESS_VEC, zero_div_zero::ZERO_DIVIDED_BY_ZERO, diff --git a/clippy_lints/src/unused_io_amount.rs b/clippy_lints/src/unused_io_amount.rs new file mode 100644 index 000000000000..eef8751cd0b3 --- /dev/null +++ b/clippy_lints/src/unused_io_amount.rs @@ -0,0 +1,138 @@ +use rustc::lint::*; +use rustc::hir; +use utils::{span_lint, match_path, match_trait_method, paths}; + +/// **What it does:** Checks for unused written/read amount. +/// +/// **Why is this bad?** `io::Write::write` and `io::Read::read` are not guaranteed to +/// process the entire buffer. They return how many bytes were processed, which might be smaller +/// than a given buffer's length. If you don't need to deal with partial-write/read, use +/// `write_all`/`read_exact` instead. +/// +/// **Known problems:** Detects only common patterns. +/// +/// **Example:** +/// ```rust,ignore +/// use std::io; +/// fn foo(w: &mut W) -> io::Result<()> { +/// // must be `w.write_all(b"foo")?;` +/// w.write(b"foo")?; +/// Ok(()) +/// } +/// ``` +declare_lint! { + pub UNUSED_IO_AMOUNT, + Deny, + "unused written/read amount" +} + +pub struct UnusedIoAmount; + +impl LintPass for UnusedIoAmount { + fn get_lints(&self) -> LintArray { + lint_array!(UNUSED_IO_AMOUNT) + } +} + +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedIoAmount { + fn check_stmt(&mut self, cx: &LateContext, s: &hir::Stmt) { + let expr = match s.node { + hir::StmtSemi(ref expr, _) | + hir::StmtExpr(ref expr, _) => &**expr, + _ => return, + }; + + if let hir::ExprRet(..) = expr.node { + return; + } + + match expr.node { + hir::ExprMatch(ref expr, ref arms, _) if is_try(arms) => { + if let hir::ExprCall(ref func, ref args) = expr.node { + if let hir::ExprPath(ref path) = func.node { + if match_path(path, &paths::CARRIER_TRANSLATE) && args.len() == 1 { + check_method_call(cx, &args[0], expr); + } + } + } else { + check_method_call(cx, expr, expr); + } + }, + + hir::ExprMethodCall(ref symbol, _, ref args) => { + let symbol = &*symbol.node.as_str(); + match symbol { + "expect" | "unwrap" | "unwrap_or" | "unwrap_or_else" => { + check_method_call(cx, &args[0], expr); + }, + _ => (), + } + }, + + _ => (), + } + } +} + +fn check_method_call(cx: &LateContext, call: &hir::Expr, expr: &hir::Expr) { + if let hir::ExprMethodCall(ref symbol, _, _) = call.node { + let symbol = &*symbol.node.as_str(); + if match_trait_method(cx, call, &paths::IO_READ) && symbol == "read" { + span_lint(cx, + UNUSED_IO_AMOUNT, + expr.span, + "handle read amount returned or use `Read::read_exact` instead"); + } else if match_trait_method(cx, call, &paths::IO_WRITE) && symbol == "write" { + span_lint(cx, + UNUSED_IO_AMOUNT, + expr.span, + "handle written amount returned or use `Write::write_all` instead"); + } + } +} + +fn is_try(arms: &[hir::Arm]) -> bool { + // `Ok(x) => x` or `Ok(_) => ...` + fn is_ok(arm: &hir::Arm) -> bool { + if let hir::PatKind::TupleStruct(ref path, ref pat, ref dotdot) = arm.pats[0].node { + // cut off `core` + if match_path(path, &paths::RESULT_OK[1..]) { + if *dotdot == Some(0) { + return true; + } + + match pat[0].node { + hir::PatKind::Wild => { + return true; + }, + hir::PatKind::Binding(_, defid, _, None) => { + if let hir::ExprPath(hir::QPath::Resolved(None, ref path)) = arm.body.node { + if path.def.def_id() == defid { + return true; + } + } + }, + _ => (), + } + } + } + + false + } + + /// Detects `_ => ...` or `Err(x) => ...` + fn is_err_or_wild(arm: &hir::Arm) -> bool { + match arm.pats[0].node { + hir::PatKind::Wild => true, + hir::PatKind::TupleStruct(ref path, _, _) => match_path(path, &paths::RESULT_ERR[1..]), + _ => false, + } + } + + if arms.len() == 2 && arms[0].pats.len() == 1 && arms[0].guard.is_none() && arms[1].pats.len() == 1 && + arms[1].guard.is_none() { + (is_ok(&arms[0]) && is_err_or_wild(&arms[1])) || (is_ok(&arms[1]) && is_err_or_wild(&arms[0])) + } else { + false + } +} diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 402985613aeb..5edff76d9969 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/clippy_lints/src/utils/paths.rs @@ -7,6 +7,7 @@ pub const BOX_NEW: [&'static str; 4] = ["std", "boxed", "Box", "new"]; pub const BTREEMAP: [&'static str; 4] = ["collections", "btree", "map", "BTreeMap"]; pub const BTREEMAP_ENTRY: [&'static str; 4] = ["collections", "btree", "map", "Entry"]; pub const BTREESET: [&'static str; 4] = ["collections", "btree", "set", "BTreeSet"]; +pub const CARRIER_TRANSLATE: [&'static str; 4] = ["std", "ops", "Carrier", "translate"]; pub const CLONE: [&'static str; 4] = ["core", "clone", "Clone", "clone"]; pub const CLONE_TRAIT: [&'static str; 3] = ["core", "clone", "Clone"]; pub const CMP_MAX: [&'static str; 3] = ["core", "cmp", "max"]; @@ -25,6 +26,8 @@ pub const HASHMAP_ENTRY: [&'static str; 5] = ["std", "collections", "hash", "map pub const HASHSET: [&'static str; 5] = ["std", "collections", "hash", "set", "HashSet"]; pub const INTO_ITERATOR: [&'static str; 4] = ["core", "iter", "traits", "IntoIterator"]; pub const IO_PRINT: [&'static str; 4] = ["std", "io", "stdio", "_print"]; +pub const IO_READ: [&'static str; 3] = ["std", "io", "Read"]; +pub const IO_WRITE: [&'static str; 3] = ["std", "io", "Write"]; pub const ITERATOR: [&'static str; 4] = ["core", "iter", "iterator", "Iterator"]; pub const LINKED_LIST: [&'static str; 3] = ["collections", "linked_list", "LinkedList"]; pub const LINT: [&'static str; 3] = ["rustc", "lint", "Lint"]; diff --git a/tests/compile-fail/unused_io_amount.rs b/tests/compile-fail/unused_io_amount.rs new file mode 100644 index 000000000000..1436605a1ba5 --- /dev/null +++ b/tests/compile-fail/unused_io_amount.rs @@ -0,0 +1,35 @@ +#![feature(plugin)] +#![plugin(clippy)] + +#![allow(dead_code)] + +use std::io; + +fn try_macro(s: &mut T) -> io::Result<()> { + try!(s.write(b"test")); + //~^ ERROR handle written amount returned + let mut buf = [0u8; 4]; + try!(s.read(&mut buf)); + //~^ ERROR handle read amount returned + Ok(()) +} + +fn question_mark(s: &mut T) -> io::Result<()> { + s.write(b"test")?; + //~^ ERROR handle written amount returned + let mut buf = [0u8; 4]; + s.read(&mut buf)?; + //~^ ERROR handle read amount returned + Ok(()) +} + +fn unwrap(s: &mut T) { + s.write(b"test").unwrap(); + //~^ ERROR handle written amount returned + let mut buf = [0u8; 4]; + s.read(&mut buf).unwrap(); + //~^ ERROR handle read amount returned +} + +fn main() { +} From a2bcce9dbfaddfa8fc26f4ab71747204bc2c6792 Mon Sep 17 00:00:00 2001 From: sinkuu Date: Sat, 7 Jan 2017 23:39:50 +0900 Subject: [PATCH 2/2] Move `is_try` to util Removed unnecessary condition Also changed lint span of `try` from surrounded expression to entire `try` invocation. It turned out that compiletest misses errors for macro invocations. --- clippy_lints/src/unused_io_amount.rs | 58 ++------------------------ clippy_lints/src/utils/mod.rs | 44 +++++++++++++++++++ tests/compile-fail/unused_io_amount.rs | 3 +- 3 files changed, 49 insertions(+), 56 deletions(-) diff --git a/clippy_lints/src/unused_io_amount.rs b/clippy_lints/src/unused_io_amount.rs index eef8751cd0b3..fc50e668c85d 100644 --- a/clippy_lints/src/unused_io_amount.rs +++ b/clippy_lints/src/unused_io_amount.rs @@ -1,6 +1,6 @@ use rustc::lint::*; use rustc::hir; -use utils::{span_lint, match_path, match_trait_method, paths}; +use utils::{span_lint, match_path, match_trait_method, is_try, paths}; /// **What it does:** Checks for unused written/read amount. /// @@ -42,20 +42,16 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedIoAmount { _ => return, }; - if let hir::ExprRet(..) = expr.node { - return; - } - match expr.node { - hir::ExprMatch(ref expr, ref arms, _) if is_try(arms) => { - if let hir::ExprCall(ref func, ref args) = expr.node { + hir::ExprMatch(ref res, _, _) if is_try(expr).is_some() => { + if let hir::ExprCall(ref func, ref args) = res.node { if let hir::ExprPath(ref path) = func.node { if match_path(path, &paths::CARRIER_TRANSLATE) && args.len() == 1 { check_method_call(cx, &args[0], expr); } } } else { - check_method_call(cx, expr, expr); + check_method_call(cx, res, expr); } }, @@ -90,49 +86,3 @@ fn check_method_call(cx: &LateContext, call: &hir::Expr, expr: &hir::Expr) { } } } - -fn is_try(arms: &[hir::Arm]) -> bool { - // `Ok(x) => x` or `Ok(_) => ...` - fn is_ok(arm: &hir::Arm) -> bool { - if let hir::PatKind::TupleStruct(ref path, ref pat, ref dotdot) = arm.pats[0].node { - // cut off `core` - if match_path(path, &paths::RESULT_OK[1..]) { - if *dotdot == Some(0) { - return true; - } - - match pat[0].node { - hir::PatKind::Wild => { - return true; - }, - hir::PatKind::Binding(_, defid, _, None) => { - if let hir::ExprPath(hir::QPath::Resolved(None, ref path)) = arm.body.node { - if path.def.def_id() == defid { - return true; - } - } - }, - _ => (), - } - } - } - - false - } - - /// Detects `_ => ...` or `Err(x) => ...` - fn is_err_or_wild(arm: &hir::Arm) -> bool { - match arm.pats[0].node { - hir::PatKind::Wild => true, - hir::PatKind::TupleStruct(ref path, _, _) => match_path(path, &paths::RESULT_ERR[1..]), - _ => false, - } - } - - if arms.len() == 2 && arms[0].pats.len() == 1 && arms[0].guard.is_none() && arms[1].pats.len() == 1 && - arms[1].guard.is_none() { - (is_ok(&arms[0]) && is_err_or_wild(&arms[1])) || (is_ok(&arms[1]) && is_err_or_wild(&arms[0])) - } else { - false - } -} diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index f971779b2edb..866a35427624 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -920,3 +920,47 @@ pub fn is_self_ty(slf: &Ty) -> bool { pub fn iter_input_pats<'tcx>(decl: &FnDecl, body: &'tcx Body) -> impl Iterator { (0..decl.inputs.len()).map(move |i| &body.arguments[i]) } + +/// Check if a given expression is a match expression +/// expanded from `?` operator or `try` macro. +pub fn is_try(expr: &Expr) -> Option<&Expr> { + fn is_ok(arm: &Arm) -> bool { + if_let_chain! {[ + let PatKind::TupleStruct(ref path, ref pat, None) = arm.pats[0].node, + match_path(path, &paths::RESULT_OK[1..]), + let PatKind::Binding(_, defid, _, None) = pat[0].node, + let ExprPath(QPath::Resolved(None, ref path)) = arm.body.node, + path.def.def_id() == defid, + ], { + return true; + }} + false + } + + fn is_err(arm: &Arm) -> bool { + if let PatKind::TupleStruct(ref path, _, _) = arm.pats[0].node { + match_path(path, &paths::RESULT_ERR[1..]) + } else { + false + } + } + + if let ExprMatch(_, ref arms, ref source) = expr.node { + // desugared from a `?` operator + if let MatchSource::TryDesugar = *source { + return Some(expr); + } + + if_let_chain! {[ + arms.len() == 2, + arms[0].pats.len() == 1 && arms[0].guard.is_none(), + arms[1].pats.len() == 1 && arms[1].guard.is_none(), + (is_ok(&arms[0]) && is_err(&arms[1])) || + (is_ok(&arms[1]) && is_err(&arms[0])), + ], { + return Some(expr); + }} + } + + None +} diff --git a/tests/compile-fail/unused_io_amount.rs b/tests/compile-fail/unused_io_amount.rs index 1436605a1ba5..2e63705c5107 100644 --- a/tests/compile-fail/unused_io_amount.rs +++ b/tests/compile-fail/unused_io_amount.rs @@ -5,12 +5,11 @@ use std::io; +// FIXME: compiletest doesn't understand errors from macro invocation span fn try_macro(s: &mut T) -> io::Result<()> { try!(s.write(b"test")); - //~^ ERROR handle written amount returned let mut buf = [0u8; 4]; try!(s.read(&mut buf)); - //~^ ERROR handle read amount returned Ok(()) }