Auto merge of #11608 - atwam:suspicious-open-options, r=y21

Add suspicious_open_options lint.

changelog: [`suspicious_open_options`]: Checks for the suspicious use of std::fs::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);
```

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

Author: bors

CHANGELOG.md

@@ -5606,6 +5606,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

clippy_lints/src/declared_lints.rs

@@ -439,6 +439,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::methods::STR_SPLIT_AT_NEWLINE_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,

clippy_lints/src/methods/mod.rs

@@ -2827,6 +2827,44 @@ 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,no_run
+    /// use std::fs::OpenOptions;
+    ///
+    /// OpenOptions::new().create(true);
+    /// ```
+    /// Use instead:
+    /// ```rust,no_run
+    /// use std::fs::OpenOptions;
+    ///
+    /// OpenOptions::new().create(true).truncate(true);
+    /// ```
+    #[clippy::version = "1.75.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)
@@ -4033,6 +4071,7 @@ impl_lint_pass!(Methods => [
     MAP_ERR_IGNORE,
     MUT_MUTEX_LOCK,
     NONSENSICAL_OPEN_OPTIONS,
+    SUSPICIOUS_OPEN_OPTIONS,
     PATH_BUF_PUSH_OVERWRITE,
     RANGE_ZIP_WITH_LEN,
     REPEAT_ONCE,

clippy_lints/src/methods/open_options.rs

@@ -1,178 +1,206 @@
-use clippy_utils::diagnostics::span_lint;
-use clippy_utils::ty::is_type_diagnostic_item;
+use rustc_data_structures::fx::FxHashMap;
+
+use clippy_utils::diagnostics::{span_lint, span_lint_and_then};
+use clippy_utils::ty::{is_type_diagnostic_item, match_type};
+use clippy_utils::{match_any_def_paths, paths};
 use rustc_ast::ast::LitKind;
 use rustc_hir::{Expr, ExprKind};
 use rustc_lint::LateContext;
+use rustc_middle::ty::Ty;
 use rustc_span::source_map::Spanned;
 use rustc_span::{sym, Span};
 
-use super::NONSENSICAL_OPEN_OPTIONS;
+use super::{NONSENSICAL_OPEN_OPTIONS, SUSPICIOUS_OPEN_OPTIONS};
+
+fn is_open_options(cx: &LateContext<'_>, ty: Ty<'_>) -> bool {
+    is_type_diagnostic_item(cx, ty, sym::FsOpenOptions) || match_type(cx, ty, &paths::TOKIO_IO_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)
         && let Some(impl_id) = cx.tcx.impl_of_method(method_id)
-        && is_type_diagnostic_item(cx, cx.tcx.type_of(impl_id).instantiate_identity(), sym::FsOpenOptions)
+        && is_open_options(cx, cx.tcx.type_of(impl_id).instantiate_identity())
     {
         let mut options = Vec::new();
-        get_open_options(cx, recv, &mut options);
-        check_open_options(cx, &options, e.span);
+        if get_open_options(cx, recv, &mut options) {
+            check_open_options(cx, &options, e.span);
+        }
     }
 }
 
-#[derive(Debug, PartialEq, Eq, Clone, Copy)]
+#[derive(Eq, PartialEq, Clone, Debug)]
 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)>) {
-    if let ExprKind::MethodCall(path, receiver, arguments, _) = argument.kind {
+/// Collects information about a method call chain on `OpenOptions`.
+/// Returns false if an unexpected expression kind was found "on the way",
+/// and linting should then be avoided.
+fn get_open_options(
+    cx: &LateContext<'_>,
+    argument: &Expr<'_>,
+    options: &mut Vec<(OpenOption, Argument, Span)>,
+) -> bool {
+    if let ExprKind::MethodCall(path, receiver, arguments, span) = argument.kind {
         let obj_ty = cx.typeck_results().expr_ty(receiver).peel_refs();
 
         // Only proceed if this is a call on some object of type std::fs::OpenOptions
-        if is_type_diagnostic_item(cx, obj_ty, sym::FsOpenOptions) && !arguments.is_empty() {
+        if !arguments.is_empty() && is_open_options(cx, obj_ty) {
             let argument_option = match arguments[0].kind {
                 ExprKind::Lit(span) => {
                     if let Spanned {
                         node: LitKind::Bool(lit),
                         ..
                     } = 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.
-                        return;
+                        // We'll ignore it for now
+                        return get_open_options(cx, receiver, options);
                     }
                 },
                 _ => Argument::Unknown,
             };
 
             match path.ident.as_str() {
                 "create" => {
-                    options.push((OpenOption::Create, argument_option));
+                    options.push((OpenOption::Create, argument_option, span));
+                },
+                "create_new" => {
+                    options.push((OpenOption::CreateNew, argument_option, span));
                 },
                 "append" => {
-                    options.push((OpenOption::Append, argument_option));
+                    options.push((OpenOption::Append, argument_option, span));
                 },
                 "truncate" => {
-                    options.push((OpenOption::Truncate, argument_option));
+                    options.push((OpenOption::Truncate, argument_option, span));
                 },
                 "read" => {
-                    options.push((OpenOption::Read, argument_option));
+                    options.push((OpenOption::Read, argument_option, span));
                 },
                 "write" => {
-                    options.push((OpenOption::Write, argument_option));
+                    options.push((OpenOption::Write, argument_option, span));
+                },
+                _ => {
+                    // Avoid linting altogether if this method is from a trait.
+                    // This might be a user defined extension trait with a method like `truncate_write`
+                    // which would be a false positive
+                    if let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(argument.hir_id)
+                        && cx.tcx.trait_of_item(method_def_id).is_some()
+                    {
+                        return false;
+                    }
                 },
-                _ => (),
             }
 
-            get_open_options(cx, receiver, options);
+            get_open_options(cx, receiver, options)
+        } else {
+            false
         }
+    } else if let ExprKind::Call(callee, _) = argument.kind
+        && let ExprKind::Path(path) = callee.kind
+        && let Some(did) = cx.qpath_res(&path, callee.hir_id).opt_def_id()
+    {
+        match_any_def_paths(
+            cx,
+            did,
+            &[
+                &paths::TOKIO_IO_OPEN_OPTIONS_NEW,
+                &paths::OPEN_OPTIONS_NEW,
+                &paths::FILE_OPTIONS,
+                &paths::TOKIO_FILE_OPTIONS,
+            ],
+        )
+        .is_some()
+    } else {
+        false
     }
 }
 
-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.
-
-    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);
-            },
+fn check_open_options(cx: &LateContext<'_>, settings: &[(OpenOption, Argument, Span)], span: Span) {
+    // The args passed to these methods, if they have been called
+    let mut options = FxHashMap::default();
+    for (option, arg, sp) in settings {
+        if let Some((_, prev_span)) = options.insert(option.clone(), (arg.clone(), *sp)) {
+            span_lint(
+                cx,
+                NONSENSICAL_OPEN_OPTIONS,
+                prev_span,
+                &format!("the method `{}` is called more than once", &option),
+            );
         }
     }
 
-    if read && truncate && read_arg && truncate_arg && !(write && write_arg) {
+    if let Some((Argument::Set(true), _)) = options.get(&OpenOption::Read)
+        && let Some((Argument::Set(true), _)) = options.get(&OpenOption::Truncate)
+        && let None | Some((Argument::Set(false), _)) = options.get(&OpenOption::Write)
+    {
         span_lint(
             cx,
             NONSENSICAL_OPEN_OPTIONS,
             span,
             "file opened with `truncate` and `read`",
         );
     }
-    if append && truncate && append_arg && truncate_arg {
+
+    if let Some((Argument::Set(true), _)) = options.get(&OpenOption::Append)
+        && let Some((Argument::Set(true), _)) = options.get(&OpenOption::Truncate)
+    {
         span_lint(
             cx,
             NONSENSICAL_OPEN_OPTIONS,
             span,
             "file opened with `append` and `truncate`",
         );
     }
+
+    if let Some((Argument::Set(true), create_span)) = options.get(&OpenOption::Create)
+        && let None = options.get(&OpenOption::Truncate)
+        && let None | Some((Argument::Set(false), _)) = options.get(&OpenOption::Append)
+    {
+        span_lint_and_then(
+            cx,
+            SUSPICIOUS_OPEN_OPTIONS,
+            *create_span,
+            "file opened with `create`, but `truncate` behavior not defined",
+            |diag| {
+                diag.span_suggestion(
+                    create_span.shrink_to_hi(),
+                    "add",
+                    ".truncate(true)".to_string(),
+                    rustc_errors::Applicability::MaybeIncorrect,
+                )
+                .help("if you intend to overwrite an existing file entirely, call `.truncate(true)`")
+                .help(
+                    "if you instead know that you may want to keep some parts of the old file, call `.truncate(false)`",
+                )
+                .help("alternatively, use `.append(true)` to append to the file instead of overwriting it");
+            },
+        );
+    }
 }