diff --git a/CHANGELOG.md b/CHANGELOG.md index 69694da1520..11d745a4c23 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1093,6 +1093,7 @@ Released 2018-09-13 [`extend_from_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#extend_from_slice [`extra_unused_lifetimes`]: https://rust-lang.github.io/rust-clippy/master/index.html#extra_unused_lifetimes [`fallible_impl_from`]: https://rust-lang.github.io/rust-clippy/master/index.html#fallible_impl_from +[`filetype_is_file`]: https://rust-lang.github.io/rust-clippy/master/index.html#filetype_is_file [`filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_map [`filter_map_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_map_next [`filter_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_next diff --git a/README.md b/README.md index 4de256e9e72..7b1d14e9afc 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 346 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 347 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 45fcf624b4b..576a4a8cd7c 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -614,6 +614,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &methods::CLONE_ON_COPY, &methods::CLONE_ON_REF_PTR, &methods::EXPECT_FUN_CALL, + &methods::FILETYPE_IS_FILE, &methods::FILTER_MAP, &methods::FILTER_MAP_NEXT, &methods::FILTER_NEXT, @@ -1007,6 +1008,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&matches::WILDCARD_ENUM_MATCH_ARM), LintId::of(&mem_forget::MEM_FORGET), LintId::of(&methods::CLONE_ON_REF_PTR), + LintId::of(&methods::FILETYPE_IS_FILE), LintId::of(&methods::GET_UNWRAP), LintId::of(&methods::OPTION_EXPECT_USED), LintId::of(&methods::OPTION_UNWRAP_USED), diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 5fd3c9bb9e0..30322ec8ebb 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -1131,6 +1131,40 @@ declare_clippy_lint! { "Check for offset calculations on raw pointers to zero-sized types" } +declare_clippy_lint! { + /// **What it does:** Checks for `FileType::is_file()`. + /// + /// **Why is this bad?** When people testing a file type with `FileType::is_file` + /// they are testing whether a path is something they can get bytes from. But + /// `is_file` doesn't cover special file types in unix-like systems, and doesn't cover + /// symlink in windows. Using `!FileType::is_dir()` is a better way to that intention. + /// + /// **Example:** + /// + /// ```rust,ignore + /// let metadata = std::fs::metadata("foo.txt")?; + /// let filetype = metadata.file_type(); + /// + /// if filetype.is_file() { + /// // read file + /// } + /// ``` + /// + /// should be written as: + /// + /// ```rust,ignore + /// let metadata = std::fs::metadata("foo.txt")?; + /// let filetype = metadata.file_type(); + /// + /// if !filetype.is_dir() { + /// // read file + /// } + /// ``` + pub FILETYPE_IS_FILE, + restriction, + "`FileType::is_file` is not recommended to test for readable file type" +} + declare_lint_pass!(Methods => [ OPTION_UNWRAP_USED, RESULT_UNWRAP_USED, @@ -1178,6 +1212,7 @@ declare_lint_pass!(Methods => [ UNINIT_ASSUMED_INIT, MANUAL_SATURATING_ARITHMETIC, ZST_OFFSET, + FILETYPE_IS_FILE, ]); impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods { @@ -1241,6 +1276,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods { ["add"] | ["offset"] | ["sub"] | ["wrapping_offset"] | ["wrapping_add"] | ["wrapping_sub"] => { check_pointer_offset(cx, expr, arg_lists[0]) }, + ["is_file", ..] => lint_filetype_is_file(cx, expr, arg_lists[0]), _ => {}, } @@ -3225,3 +3261,35 @@ fn check_pointer_offset(cx: &LateContext<'_, '_>, expr: &hir::Expr<'_>, args: &[ } } } + +fn lint_filetype_is_file(cx: &LateContext<'_, '_>, expr: &hir::Expr<'_>, args: &[hir::Expr<'_>]) { + let ty = cx.tables.expr_ty(&args[0]); + + if !match_type(cx, ty, &paths::FILE_TYPE) { + return; + } + + let span: Span; + let verb: &str; + let lint_unary: &str; + let help_unary: &str; + if_chain! { + if let Some(parent) = get_parent_expr(cx, expr); + if let hir::ExprKind::Unary(op, _) = parent.kind; + if op == hir::UnOp::UnNot; + then { + lint_unary = "!"; + verb = "denies"; + help_unary = ""; + span = parent.span; + } else { + lint_unary = ""; + verb = "covers"; + help_unary = "!"; + span = expr.span; + } + } + let lint_msg = format!("`{}FileType::is_file()` only {} regular files", lint_unary, verb); + let help_msg = format!("use `{}FileType::is_dir()` instead", help_unary); + span_help_and_lint(cx, FILETYPE_IS_FILE, span, &lint_msg, &help_msg); +} diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 9338daf0574..4f8fe02dfd2 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/clippy_lints/src/utils/paths.rs @@ -28,6 +28,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_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"]; pub const FMT_ARGUMENTV1_NEW: [&str; 4] = ["core", "fmt", "ArgumentV1", "new"]; diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index f576bb152de..9cad1ac786a 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; 346] = [ +pub const ALL_LINTS: [Lint; 347] = [ Lint { name: "absurd_extreme_comparisons", group: "correctness", @@ -560,6 +560,13 @@ pub const ALL_LINTS: [Lint; 346] = [ deprecation: None, module: "fallible_impl_from", }, + Lint { + name: "filetype_is_file", + group: "restriction", + desc: "`FileType::is_file` is not recommended to test for readable file type", + deprecation: None, + module: "methods", + }, Lint { name: "filter_map", group: "pedantic", diff --git a/tests/compile-test.rs b/tests/compile-test.rs index 43139e95666..5c05e74dcb6 100644 --- a/tests/compile-test.rs +++ b/tests/compile-test.rs @@ -121,7 +121,7 @@ fn run_ui_toml_tests(config: &compiletest::Config, mut tests: Vec std::io::Result<()> { + use std::fs; + use std::ops::BitOr; + + // !filetype.is_dir() + if fs::metadata("foo.txt")?.file_type().is_file() { + // read file + } + + // positive of filetype.is_dir() + if !fs::metadata("foo.txt")?.file_type().is_file() { + // handle dir + } + + // false positive of filetype.is_dir() + if !fs::metadata("foo.txt")?.file_type().is_file().bitor(true) { + // ... + } + + Ok(()) +} diff --git a/tests/ui/filetype_is_file.stderr b/tests/ui/filetype_is_file.stderr new file mode 100644 index 00000000000..cd1e3ac37fe --- /dev/null +++ b/tests/ui/filetype_is_file.stderr @@ -0,0 +1,27 @@ +error: `FileType::is_file()` only covers regular files + --> $DIR/filetype_is_file.rs:8:8 + | +LL | if fs::metadata("foo.txt")?.file_type().is_file() { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::filetype-is-file` implied by `-D warnings` + = help: use `!FileType::is_dir()` instead + +error: `!FileType::is_file()` only denies regular files + --> $DIR/filetype_is_file.rs:13:8 + | +LL | if !fs::metadata("foo.txt")?.file_type().is_file() { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use `FileType::is_dir()` instead + +error: `FileType::is_file()` only covers regular files + --> $DIR/filetype_is_file.rs:18:9 + | +LL | if !fs::metadata("foo.txt")?.file_type().is_file().bitor(true) { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use `!FileType::is_dir()` instead + +error: aborting due to 3 previous errors +