From 0f7f30711e4eb2f5f84fe198f325e512e404ee0c Mon Sep 17 00:00:00 2001 From: Jacob Meyers Date: Wed, 4 Mar 2020 21:13:57 -0500 Subject: [PATCH 1/2] add lint on File::read_to_string and File::read_to_end --- CHANGELOG.md | 1 + README.md | 2 +- clippy_lints/src/lib.rs | 5 ++ clippy_lints/src/utils/paths.rs | 1 + clippy_lints/src/verbose_file_reads.rs | 82 ++++++++++++++++++++++++++ src/lintlist/mod.rs | 9 ++- tests/ui/verbose_file_reads.rs | 30 ++++++++++ tests/ui/verbose_file_reads.stderr | 19 ++++++ 8 files changed, 147 insertions(+), 2 deletions(-) create mode 100644 clippy_lints/src/verbose_file_reads.rs create mode 100644 tests/ui/verbose_file_reads.rs create mode 100644 tests/ui/verbose_file_reads.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index c3f291bc0..32cbbe801 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1417,6 +1417,7 @@ Released 2018-09-13 [`useless_vec`]: https://rust-lang.github.io/rust-clippy/master/index.html#useless_vec [`vec_box`]: https://rust-lang.github.io/rust-clippy/master/index.html#vec_box [`verbose_bit_mask`]: https://rust-lang.github.io/rust-clippy/master/index.html#verbose_bit_mask +[`verbose_file_reads`]: https://rust-lang.github.io/rust-clippy/master/index.html#verbose_file_reads [`while_immutable_condition`]: https://rust-lang.github.io/rust-clippy/master/index.html#while_immutable_condition [`while_let_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#while_let_loop [`while_let_on_iterator`]: https://rust-lang.github.io/rust-clippy/master/index.html#while_let_on_iterator diff --git a/README.md b/README.md index 8635b2b6d..2181c296e 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 360 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 361 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 02c016c38..1eac69678 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -310,6 +310,7 @@ pub mod unused_self; pub mod unwrap; pub mod use_self; pub mod vec; +pub mod verbose_file_reads; pub mod wildcard_dependencies; pub mod wildcard_imports; pub mod write; @@ -815,6 +816,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &unwrap::UNNECESSARY_UNWRAP, &use_self::USE_SELF, &vec::USELESS_VEC, + &verbose_file_reads::VERBOSE_FILE_READS, &wildcard_dependencies::WILDCARD_DEPENDENCIES, &wildcard_imports::ENUM_GLOB_USE, &wildcard_imports::WILDCARD_IMPORTS, @@ -1015,6 +1017,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_early_pass(|| box option_env_unwrap::OptionEnvUnwrap); store.register_late_pass(|| box wildcard_imports::WildcardImports); store.register_early_pass(|| box macro_use::MacroUseImports); + store.register_late_pass(|| box verbose_file_reads::VerboseFileReads); store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![ LintId::of(&arithmetic::FLOAT_ARITHMETIC), @@ -1372,6 +1375,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&unwrap::PANICKING_UNWRAP), LintId::of(&unwrap::UNNECESSARY_UNWRAP), LintId::of(&vec::USELESS_VEC), + LintId::of(&verbose_file_reads::VERBOSE_FILE_READS), LintId::of(&write::PRINTLN_EMPTY_STRING), LintId::of(&write::PRINT_LITERAL), LintId::of(&write::PRINT_WITH_NEWLINE), @@ -1555,6 +1559,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&types::UNNECESSARY_CAST), LintId::of(&types::VEC_BOX), LintId::of(&unwrap::UNNECESSARY_UNWRAP), + LintId::of(&verbose_file_reads::VERBOSE_FILE_READS), LintId::of(&zero_div_zero::ZERO_DIVIDED_BY_ZERO), ]); diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 5ec04d965..6cb1f694f 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/clippy_lints/src/utils/paths.rs @@ -31,6 +31,7 @@ pub const DROP_TRAIT: [&str; 4] = ["core", "ops", "drop", "Drop"]; pub const DURATION: [&str; 3] = ["core", "time", "Duration"]; pub const EARLY_CONTEXT: [&str; 4] = ["rustc", "lint", "context", "EarlyContext"]; pub const EXIT: [&str; 3] = ["std", "process", "exit"]; +pub const FILE: [&str; 3] = ["std", "fs", "File"]; pub const FILE_TYPE: [&str; 3] = ["std", "fs", "FileType"]; pub const FMT_ARGUMENTS_NEW_V1: [&str; 4] = ["core", "fmt", "Arguments", "new_v1"]; pub const FMT_ARGUMENTS_NEW_V1_FORMATTED: [&str; 4] = ["core", "fmt", "Arguments", "new_v1_formatted"]; diff --git a/clippy_lints/src/verbose_file_reads.rs b/clippy_lints/src/verbose_file_reads.rs new file mode 100644 index 000000000..932567902 --- /dev/null +++ b/clippy_lints/src/verbose_file_reads.rs @@ -0,0 +1,82 @@ +use crate::utils::{match_type, paths, span_lint_and_help}; +use if_chain::if_chain; +use rustc_hir::{Expr, ExprKind, QPath}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; + +declare_clippy_lint! { + /// **What it does:** Checks for use of File::read_to_end and File::read_to_string. + /// + /// **Why is this bad?** `fs::{read, read_to_string}` provide the same functionality when `buf` is empty with fewer imports and no intermediate values. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust, ignore + /// let mut f = File::open("foo.txt")?; + /// let mut bytes = Vec::new(); + /// f.read_to_end(&mut bytes)?; + /// ``` + /// Can be written more concisely as + /// ```rust, ignore + /// let mut bytes = fs::read("foo.txt")?; + /// ``` + pub VERBOSE_FILE_READS, + complexity, + "use of `File::read_to_end` or `File::read_to_string`" +} + +declare_lint_pass!(VerboseFileReads => [VERBOSE_FILE_READS]); + +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for VerboseFileReads { + fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'tcx>) { + if is_file_read_to_end(cx, expr) { + span_lint_and_help( + cx, + VERBOSE_FILE_READS, + expr.span, + "use of File::read_to_end", + "consider using fs::read instead", + ); + } else if is_file_read_to_string(cx, expr) { + span_lint_and_help( + cx, + VERBOSE_FILE_READS, + expr.span, + "use of File::read_to_string", + "consider using fs::read_to_string instead", + ) + } else { + // Don't care + } + } +} + +fn is_file_read_to_end<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'tcx>) -> bool { + if_chain! { + if let ExprKind::MethodCall(method_name, _, exprs) = expr.kind; + if method_name.ident.as_str() == "read_to_end"; + if let ExprKind::Path(QPath::Resolved(None, _)) = &exprs[0].kind; + let ty = cx.tables.expr_ty(&exprs[0]); + if match_type(cx, ty, &paths::FILE); + then { + return true + } + } + false +} + +fn is_file_read_to_string<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'tcx>) -> bool { + if_chain! { + if let ExprKind::MethodCall(method_name, _, exprs) = expr.kind; + if method_name.ident.as_str() == "read_to_string"; + if let ExprKind::Path(QPath::Resolved(None, _)) = &exprs[0].kind; + let ty = cx.tables.expr_ty(&exprs[0]); + if match_type(cx, ty, &paths::FILE); + then { + return true + } + } + false +} diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 4ae4ccc91..fd948953e 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -6,7 +6,7 @@ pub use lint::Lint; pub use lint::LINT_LEVELS; // begin lint list, do not remove this comment, it’s used in `update_lints` -pub const ALL_LINTS: [Lint; 360] = [ +pub const ALL_LINTS: [Lint; 361] = [ Lint { name: "absurd_extreme_comparisons", group: "correctness", @@ -2401,6 +2401,13 @@ pub const ALL_LINTS: [Lint; 360] = [ deprecation: None, module: "bit_mask", }, + Lint { + name: "verbose_file_reads", + group: "complexity", + desc: "use of `File::read_to_end` or `File::read_to_string`", + deprecation: None, + module: "verbose_file_reads", + }, Lint { name: "while_immutable_condition", group: "correctness", diff --git a/tests/ui/verbose_file_reads.rs b/tests/ui/verbose_file_reads.rs new file mode 100644 index 000000000..3c7c4be84 --- /dev/null +++ b/tests/ui/verbose_file_reads.rs @@ -0,0 +1,30 @@ +#![warn(clippy::verbose_file_reads)] +use std::env::temp_dir; +use std::fs::File; +use std::io::Read; + +struct Struct; +// To make sure we only warn on File::{read_to_end, read_to_string} calls +impl Struct { + pub fn read_to_end(&self) {} + + pub fn read_to_string(&self) {} +} + +fn main() -> std::io::Result<()> { + let mut path = temp_dir(); + path.push("test.txt"); + let file = File::create(&path)?; + // Lint shouldn't catch this + let s = Struct; + s.read_to_end(); + s.read_to_string(); + // Should catch this + let mut f = File::open(&path)?; + let mut buffer = Vec::new(); + f.read_to_end(&mut buffer)?; + // ...and this + let mut string_buffer = String::new(); + f.read_to_string(&mut string_buffer)?; + Ok(()) +} diff --git a/tests/ui/verbose_file_reads.stderr b/tests/ui/verbose_file_reads.stderr new file mode 100644 index 000000000..73dc22fd4 --- /dev/null +++ b/tests/ui/verbose_file_reads.stderr @@ -0,0 +1,19 @@ +error: use of File::read_to_end + --> $DIR/verbose_file_reads.rs:25:5 + | +LL | f.read_to_end(&mut buffer)?; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::verbose-file-reads` implied by `-D warnings` + = help: consider using fs::read instead + +error: use of File::read_to_string + --> $DIR/verbose_file_reads.rs:28:5 + | +LL | f.read_to_string(&mut string_buffer)?; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider using fs::read_to_string instead + +error: aborting due to 2 previous errors + From a4ba1027fc581379e55dce66d0b7e21d8d91dfa0 Mon Sep 17 00:00:00 2001 From: Jacob Meyers Date: Tue, 10 Mar 2020 05:41:24 -0400 Subject: [PATCH 2/2] add CR feedback --- clippy_lints/src/verbose_file_reads.rs | 25 +++++++++++++------------ tests/ui/verbose_file_reads.rs | 4 +--- tests/ui/verbose_file_reads.stderr | 12 ++++++------ 3 files changed, 20 insertions(+), 21 deletions(-) diff --git a/clippy_lints/src/verbose_file_reads.rs b/clippy_lints/src/verbose_file_reads.rs index 932567902..37885317c 100644 --- a/clippy_lints/src/verbose_file_reads.rs +++ b/clippy_lints/src/verbose_file_reads.rs @@ -8,19 +8,22 @@ declare_clippy_lint! { /// **What it does:** Checks for use of File::read_to_end and File::read_to_string. /// /// **Why is this bad?** `fs::{read, read_to_string}` provide the same functionality when `buf` is empty with fewer imports and no intermediate values. - /// + /// See also: [fs::read docs](https://doc.rust-lang.org/std/fs/fn.read.html), [fs::read_to_string docs](https://doc.rust-lang.org/std/fs/fn.read_to_string.html) /// **Known problems:** None. /// /// **Example:** /// - /// ```rust, ignore - /// let mut f = File::open("foo.txt")?; + /// ```rust,no_run + /// # use std::io::Read; + /// # use std::fs::File; + /// let mut f = File::open("foo.txt").unwrap(); /// let mut bytes = Vec::new(); - /// f.read_to_end(&mut bytes)?; + /// f.read_to_end(&mut bytes).unwrap(); /// ``` /// Can be written more concisely as - /// ```rust, ignore - /// let mut bytes = fs::read("foo.txt")?; + /// ```rust,no_run + /// # use std::fs; + /// let mut bytes = fs::read("foo.txt").unwrap(); /// ``` pub VERBOSE_FILE_READS, complexity, @@ -36,19 +39,17 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for VerboseFileReads { cx, VERBOSE_FILE_READS, expr.span, - "use of File::read_to_end", - "consider using fs::read instead", + "use of `File::read_to_end`", + "consider using `fs::read` instead", ); } else if is_file_read_to_string(cx, expr) { span_lint_and_help( cx, VERBOSE_FILE_READS, expr.span, - "use of File::read_to_string", - "consider using fs::read_to_string instead", + "use of `File::read_to_string`", + "consider using `fs::read_to_string` instead", ) - } else { - // Don't care } } } diff --git a/tests/ui/verbose_file_reads.rs b/tests/ui/verbose_file_reads.rs index 3c7c4be84..e0065e05a 100644 --- a/tests/ui/verbose_file_reads.rs +++ b/tests/ui/verbose_file_reads.rs @@ -12,9 +12,7 @@ impl Struct { } fn main() -> std::io::Result<()> { - let mut path = temp_dir(); - path.push("test.txt"); - let file = File::create(&path)?; + let path = "foo.txt"; // Lint shouldn't catch this let s = Struct; s.read_to_end(); diff --git a/tests/ui/verbose_file_reads.stderr b/tests/ui/verbose_file_reads.stderr index 73dc22fd4..550b6ab67 100644 --- a/tests/ui/verbose_file_reads.stderr +++ b/tests/ui/verbose_file_reads.stderr @@ -1,19 +1,19 @@ -error: use of File::read_to_end - --> $DIR/verbose_file_reads.rs:25:5 +error: use of `File::read_to_end` + --> $DIR/verbose_file_reads.rs:23:5 | LL | f.read_to_end(&mut buffer)?; | ^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `-D clippy::verbose-file-reads` implied by `-D warnings` - = help: consider using fs::read instead + = help: consider using `fs::read` instead -error: use of File::read_to_string - --> $DIR/verbose_file_reads.rs:28:5 +error: use of `File::read_to_string` + --> $DIR/verbose_file_reads.rs:26:5 | LL | f.read_to_string(&mut string_buffer)?; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | - = help: consider using fs::read_to_string instead + = help: consider using `fs::read_to_string` instead error: aborting due to 2 previous errors