Add suspicious_open_options lint.

atwam · atwam · commit 4cb82616ec46 · 2023-10-04T13:56:18.000+01:00
Checks for the suspicious use of OpenOptions::create()
without an explicit OpenOptions::truncate().

create() alone will either create a new file or open an
existing file. If the file already exists, it will be
overwritten when written to, but the file will not be
truncated by default. If less data is written to the file
than it already contains, the remainder of the file will
remain unchanged, and the end of the file will contain old
data.
In most cases, one should either use `create_new` to ensure
the file is created from scratch, or ensure `truncate` is
called so that the truncation behaviour is explicit.
`truncate(true)` will ensure the file is entirely overwritten
with new data, whereas `truncate(false)` will explicitely
keep the default behavior.

```rust
use std::fs::OpenOptions;

OpenOptions::new().create(true).truncate(true);
```
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5382,6 +5382,7 @@ Released 2018-09-13
 [`suspicious_else_formatting`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_else_formatting
 [`suspicious_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_map
 [`suspicious_op_assign_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_op_assign_impl
+[`suspicious_open_options`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_open_options
 [`suspicious_operation_groupings`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_operation_groupings
 [`suspicious_splitn`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_splitn
 [`suspicious_to_owned`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_to_owned
diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs
@@ -422,6 +422,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::methods::STRING_LIT_CHARS_ANY_INFO,
     crate::methods::SUSPICIOUS_COMMAND_ARG_SPACE_INFO,
     crate::methods::SUSPICIOUS_MAP_INFO,
+    crate::methods::SUSPICIOUS_OPEN_OPTIONS_INFO,
     crate::methods::SUSPICIOUS_SPLITN_INFO,
     crate::methods::SUSPICIOUS_TO_OWNED_INFO,
     crate::methods::TYPE_ID_ON_BOX_INFO,
diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
@@ -2818,6 +2818,37 @@ declare_clippy_lint! {
     "nonsensical combination of options for opening a file"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for the suspicious use of OpenOptions::create()
+    /// without an explicit OpenOptions::truncate().
+    ///
+    /// ### Why is this bad?
+    /// create() alone will either create a new file or open an
+    /// existing file. If the file already exists, it will be
+    /// overwritten when written to, but the file will not be
+    /// truncated by default. If less data is written to the file
+    /// than it already contains, the remainder of the file will
+    /// remain unchanged, and the end of the file will contain old
+    /// data.
+    /// In most cases, one should either use `create_new` to ensure
+    /// the file is created from scratch, or ensure `truncate` is
+    /// called so that the truncation behaviour is explicit. `truncate(true)`
+    /// will ensure the file is entirely overwritten with new data, whereas
+    /// `truncate(false)` will explicitely keep the default behavior.
+    ///
+    /// ### Example
+    /// ```rust
+    /// use std::fs::OpenOptions;
+    ///
+    /// OpenOptions::new().create(true).truncate(true);
+    /// ```
+    #[clippy::version = "pre 1.29.0"]
+    pub SUSPICIOUS_OPEN_OPTIONS,
+    suspicious,
+    "suspicious combination of options for opening a file"
+}
+
 declare_clippy_lint! {
     /// ### What it does
     ///* Checks for [push](https://doc.rust-lang.org/std/path/struct.PathBuf.html#method.push)
diff --git a/clippy_lints/src/methods/open_options.rs b/clippy_lints/src/methods/open_options.rs
@@ -1,3 +1,5 @@
+use rustc_data_structures::fx::FxHashMap;
+
 use clippy_utils::diagnostics::span_lint;
 use clippy_utils::paths;
 use clippy_utils::ty::match_type;
@@ -6,7 +8,7 @@ use rustc_hir::{Expr, ExprKind};
 use rustc_lint::LateContext;
 use rustc_span::source_map::{Span, Spanned};
 
-use super::NONSENSICAL_OPEN_OPTIONS;
+use super::{NONSENSICAL_OPEN_OPTIONS, SUSPICIOUS_OPEN_OPTIONS};
 
 pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>, recv: &'tcx Expr<'_>) {
     if let Some(method_id) = cx.typeck_results().type_dependent_def_id(e.hir_id)
@@ -19,20 +21,32 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>, recv: &'tcx
     }
 }
 
-#[derive(Debug, PartialEq, Eq, Clone, Copy)]
+#[derive(Eq, PartialEq, Clone)]
 enum Argument {
-    True,
-    False,
+    Set(bool),
     Unknown,
 }
 
-#[derive(Debug)]
+#[derive(Debug, Eq, PartialEq, Hash, Clone)]
 enum OpenOption {
-    Write,
+    Append,
+    Create,
+    CreateNew,
     Read,
     Truncate,
-    Create,
-    Append,
+    Write,
+}
+impl std::fmt::Display for OpenOption {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            OpenOption::Append => write!(f, "append"),
+            OpenOption::Create => write!(f, "create"),
+            OpenOption::CreateNew => write!(f, "create_new"),
+            OpenOption::Read => write!(f, "read"),
+            OpenOption::Truncate => write!(f, "truncate"),
+            OpenOption::Write => write!(f, "write"),
+        }
+    }
 }
 
 fn get_open_options(cx: &LateContext<'_>, argument: &Expr<'_>, options: &mut Vec<(OpenOption, Argument)>) {
@@ -48,7 +62,7 @@ fn get_open_options(cx: &LateContext<'_>, argument: &Expr<'_>, options: &mut Vec
                         ..
                     } = span
                     {
-                        if *lit { Argument::True } else { Argument::False }
+                        Argument::Set(*lit)
                     } else {
                         // The function is called with a literal which is not a boolean literal.
                         // This is theoretically possible, but not very likely.
@@ -62,6 +76,9 @@ fn get_open_options(cx: &LateContext<'_>, argument: &Expr<'_>, options: &mut Vec
                 "create" => {
                     options.push((OpenOption::Create, argument_option));
                 },
+                "create_new" => {
+                    options.push((OpenOption::CreateNew, argument_option));
+                },
                 "append" => {
                     options.push((OpenOption::Append, argument_option));
                 },
@@ -82,97 +99,52 @@ fn get_open_options(cx: &LateContext<'_>, argument: &Expr<'_>, options: &mut Vec
     }
 }
 
-fn check_open_options(cx: &LateContext<'_>, options: &[(OpenOption, Argument)], span: Span) {
-    let (mut create, mut append, mut truncate, mut read, mut write) = (false, false, false, false, false);
-    let (mut create_arg, mut append_arg, mut truncate_arg, mut read_arg, mut write_arg) =
-        (false, false, false, false, false);
-    // This code is almost duplicated (oh, the irony), but I haven't found a way to
-    // unify it.
+fn check_open_options(cx: &LateContext<'_>, settings: &[(OpenOption, Argument)], span: Span) {
+    // The args passed to these methods, if they have been called
+    let mut options = FxHashMap::default();
+    for (option, arg) in settings {
+        if options.insert(option.clone(), arg.clone()).is_some() {
+            span_lint(
+                cx,
+                NONSENSICAL_OPEN_OPTIONS,
+                span,
+                &format!("the method `{}` is called more than once", &option),
+            );
+        }
+    }
 
-    for option in options {
-        match *option {
-            (OpenOption::Create, arg) => {
-                if create {
-                    span_lint(
-                        cx,
-                        NONSENSICAL_OPEN_OPTIONS,
-                        span,
-                        "the method `create` is called more than once",
-                    );
-                } else {
-                    create = true;
-                }
-                create_arg = create_arg || (arg == Argument::True);
-            },
-            (OpenOption::Append, arg) => {
-                if append {
-                    span_lint(
-                        cx,
-                        NONSENSICAL_OPEN_OPTIONS,
-                        span,
-                        "the method `append` is called more than once",
-                    );
-                } else {
-                    append = true;
-                }
-                append_arg = append_arg || (arg == Argument::True);
-            },
-            (OpenOption::Truncate, arg) => {
-                if truncate {
-                    span_lint(
-                        cx,
-                        NONSENSICAL_OPEN_OPTIONS,
-                        span,
-                        "the method `truncate` is called more than once",
-                    );
-                } else {
-                    truncate = true;
-                }
-                truncate_arg = truncate_arg || (arg == Argument::True);
-            },
-            (OpenOption::Read, arg) => {
-                if read {
-                    span_lint(
-                        cx,
-                        NONSENSICAL_OPEN_OPTIONS,
-                        span,
-                        "the method `read` is called more than once",
-                    );
-                } else {
-                    read = true;
-                }
-                read_arg = read_arg || (arg == Argument::True);
-            },
-            (OpenOption::Write, arg) => {
-                if write {
-                    span_lint(
-                        cx,
-                        NONSENSICAL_OPEN_OPTIONS,
-                        span,
-                        "the method `write` is called more than once",
-                    );
-                } else {
-                    write = true;
-                }
-                write_arg = write_arg || (arg == Argument::True);
-            },
+    if let (Some(Argument::Set(true)), Some(Argument::Set(true))) =
+        (options.get(&OpenOption::Read), options.get(&OpenOption::Truncate))
+    {
+        if options.get(&OpenOption::Write).unwrap_or(&Argument::Set(false)) == &Argument::Set(false) {
+            span_lint(
+                cx,
+                NONSENSICAL_OPEN_OPTIONS,
+                span,
+                "file opened with `truncate` and `read`",
+            );
         }
     }
 
-    if read && truncate && read_arg && truncate_arg && !(write && write_arg) {
-        span_lint(
-            cx,
-            NONSENSICAL_OPEN_OPTIONS,
-            span,
-            "file opened with `truncate` and `read`",
-        );
+    if let (Some(Argument::Set(true)), Some(Argument::Set(true))) =
+        (options.get(&OpenOption::Append), options.get(&OpenOption::Truncate))
+    {
+        if options.get(&OpenOption::Write).unwrap_or(&Argument::Set(false)) == &Argument::Set(false) {
+            span_lint(
+                cx,
+                NONSENSICAL_OPEN_OPTIONS,
+                span,
+                "file opened with `append` and `truncate`",
+            );
+        }
     }
-    if append && truncate && append_arg && truncate_arg {
+
+    if let (Some(Argument::Set(true)), None) = (options.get(&OpenOption::Create), options.get(&OpenOption::Truncate)) {
         span_lint(
             cx,
-            NONSENSICAL_OPEN_OPTIONS,
+            SUSPICIOUS_OPEN_OPTIONS,
             span,
-            "file opened with `append` and `truncate`",
+            "file opened with `create`, but `truncate` behavior not defined",
         );
     }
 }
diff --git a/tests/ui/open_options.rs b/tests/ui/open_options.rs
@@ -2,6 +2,7 @@ use std::fs::OpenOptions;
 
 #[allow(unused_must_use)]
 #[warn(clippy::nonsensical_open_options)]
+#[warn(clippy::suspicious_open_options)]
 fn main() {
     OpenOptions::new().read(true).truncate(true).open("foo.txt");
     //~^ ERROR: file opened with `truncate` and `read`
@@ -19,4 +20,6 @@ fn main() {
     //~^ ERROR: the method `append` is called more than once
     OpenOptions::new().truncate(true).truncate(false).open("foo.txt");
     //~^ ERROR: the method `truncate` is called more than once
+    OpenOptions::new().create(true).open("foo.txt");
+    //~^ ERROR: file opened with `create`, but `truncate` behavior not defined
 }
diff --git a/tests/ui/open_options.stderr b/tests/ui/open_options.stderr
@@ -1,5 +1,5 @@
 error: file opened with `truncate` and `read`
-  --> $DIR/open_options.rs:6:5
+  --> $DIR/open_options.rs:7:5
    |
 LL |     OpenOptions::new().read(true).truncate(true).open("foo.txt");
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -8,40 +8,55 @@ LL |     OpenOptions::new().read(true).truncate(true).open("foo.txt");
    = help: to override `-D warnings` add `#[allow(clippy::nonsensical_open_options)]`
 
 error: file opened with `append` and `truncate`
-  --> $DIR/open_options.rs:9:5
+  --> $DIR/open_options.rs:10:5
    |
 LL |     OpenOptions::new().append(true).truncate(true).open("foo.txt");
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: the method `read` is called more than once
-  --> $DIR/open_options.rs:12:5
+  --> $DIR/open_options.rs:13:5
    |
 LL |     OpenOptions::new().read(true).read(false).open("foo.txt");
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: the method `create` is called more than once
-  --> $DIR/open_options.rs:14:5
+  --> $DIR/open_options.rs:15:5
    |
 LL |     OpenOptions::new().create(true).create(false).open("foo.txt");
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
+error: file opened with `create`, but `truncate` behavior not defined
+  --> $DIR/open_options.rs:15:5
+   |
+LL |     OpenOptions::new().create(true).create(false).open("foo.txt");
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::suspicious-open-options` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::suspicious_open_options)]`
+
 error: the method `write` is called more than once
-  --> $DIR/open_options.rs:16:5
+  --> $DIR/open_options.rs:17:5
    |
 LL |     OpenOptions::new().write(true).write(false).open("foo.txt");
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: the method `append` is called more than once
-  --> $DIR/open_options.rs:18:5
+  --> $DIR/open_options.rs:19:5
    |
 LL |     OpenOptions::new().append(true).append(false).open("foo.txt");
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: the method `truncate` is called more than once
-  --> $DIR/open_options.rs:20:5
+  --> $DIR/open_options.rs:21:5
    |
 LL |     OpenOptions::new().truncate(true).truncate(false).open("foo.txt");
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-error: aborting due to 7 previous errors
+error: file opened with `create`, but `truncate` behavior not defined
+  --> $DIR/open_options.rs:23:5
+   |
+LL |     OpenOptions::new().create(true).open("foo.txt");
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 9 previous errors
 
diff --git a/tests/ui/seek_to_start_instead_of_rewind.fixed b/tests/ui/seek_to_start_instead_of_rewind.fixed
@@ -80,6 +80,7 @@ fn main() {
         .write(true)
         .read(true)
         .create(true)
+        .truncate(true)
         .open("foo.txt")
         .unwrap();
 
@@ -104,6 +105,7 @@ fn msrv_1_54() {
         .write(true)
         .read(true)
         .create(true)
+        .truncate(true)
         .open("foo.txt")
         .unwrap();
 
@@ -124,6 +126,7 @@ fn msrv_1_55() {
         .write(true)
         .read(true)
         .create(true)
+        .truncate(true)
         .open("foo.txt")
         .unwrap();
 
diff --git a/tests/ui/seek_to_start_instead_of_rewind.rs b/tests/ui/seek_to_start_instead_of_rewind.rs
@@ -80,6 +80,7 @@ fn main() {
         .write(true)
         .read(true)
         .create(true)
+        .truncate(true)
         .open("foo.txt")
         .unwrap();
 
@@ -104,6 +105,7 @@ fn msrv_1_54() {
         .write(true)
         .read(true)
         .create(true)
+        .truncate(true)
         .open("foo.txt")
         .unwrap();
 
@@ -124,6 +126,7 @@ fn msrv_1_55() {
         .write(true)
         .read(true)
         .create(true)
+        .truncate(true)
         .open("foo.txt")
         .unwrap();
 
diff --git a/tests/ui/seek_to_start_instead_of_rewind.stderr b/tests/ui/seek_to_start_instead_of_rewind.stderr
