Auto merge of #4543 - xiongmao86:issue4503, r=flip1995

Fix issue4503

Fixes #4503.

changelog: Add a lint checking user are using FileType.is_file() method and suggest using !FileType.is_dir().

- [x] Followed [lint naming conventions][lint_naming]
- [x] Added passing UI tests (including committed `.stderr` file)
- [x] `cargo test` passes locally
- [x] Executed `./util/dev update_lints`
- [x] Added lint documentation
- [x] Run `./util/dev fmt`

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

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:
 

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),

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);
+}

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"];

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",

tests/compile-test.rs

@@ -121,7 +121,7 @@ fn run_ui_toml_tests(config: &compiletest::Config, mut tests: Vec<test::TestDesc
         for file in fs::read_dir(&dir_path)? {
             let file = file?;
             let file_path = file.path();
-            if !file.file_type()?.is_file() {
+            if file.file_type()?.is_dir() {
                 continue;
             }
             if file_path.extension() != Some(OsStr::new("rs")) {

tests/ui/filetype_is_file.rs

@@ -0,0 +1,23 @@
+#![warn(clippy::filetype_is_file)]
+
+fn main() -> 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(())
+}

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
+