Add suspicious_open_options lint.

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