Add suggestion/fix to suspicious_open_options

atwam · atwam · commit 84588a881564 · 2024-01-15T17:15:09.000Z
Also rebase and fix conflicts
diff --git a/clippy_lints/src/methods/open_options.rs b/clippy_lints/src/methods/open_options.rs
@@ -2,23 +2,24 @@ use rustc_data_structures::fx::FxHashMap;
 
 use clippy_utils::diagnostics::{span_lint, span_lint_and_then};
 use clippy_utils::paths;
-use clippy_utils::ty::is_type_diagnostic_item;
+use clippy_utils::ty::{is_type_diagnostic_item, match_type};
 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, 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) ||
-            match_type(cx, cx.tcx.type_of(impl_id).instantiate_identity(), &paths::TOKIO_IO_OPEN_OPTIONS)
-        )
-
+        && is_open_options(cx, cx.tcx.type_of(impl_id).instantiate_identity())
     {
         let mut options = Vec::new();
         get_open_options(cx, recv, &mut options);
@@ -59,7 +60,7 @@ fn get_open_options(cx: &LateContext<'_>, argument: &Expr<'_>, options: &mut Vec
         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 !arguments.is_empty() && (is_type_diagnostic_item(cx, obj_ty, sym::FsOpenOptions)) {
+        if !arguments.is_empty() && is_open_options(cx, obj_ty) {
             let argument_option = match arguments[0].kind {
                 ExprKind::Lit(span) => {
                     if let Spanned {
@@ -121,36 +122,39 @@ fn check_open_options(cx: &LateContext<'_>, settings: &[(OpenOption, Argument, S
     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`",
-            );
-        }
+    {
+        span_lint(
+            cx,
+            NONSENSICAL_OPEN_OPTIONS,
+            span,
+            "file opened with `truncate` and `read`",
+        );
+    }
 
     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`",
-            );
+    {
+        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)
-        {
-            span_lint_and_help(
-                cx,
-                SUSPICIOUS_OPEN_OPTIONS,
-                *create_span,
-                "file opened with `create`, but `truncate` behavior not defined",
-                Some(span),
-                "if you intend to overwrite an existing file entirely, call `.truncate(true)`. if you instead know that you may want to keep some parts of the old file, call `.truncate(false)`"
-            );
+    {
+        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)`. if you instead know that you may want to keep some parts of the old file, call `.truncate(false)`");
+            },
+        );
     }
 }
diff --git a/tests/ui/open_options.rs b/tests/ui/open_options.rs
@@ -1,7 +1,6 @@
 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`
@@ -11,14 +10,24 @@ fn main() {
 
     OpenOptions::new().read(true).read(false).open("foo.txt");
     //~^ ERROR: the method `read` is called more than once
-    OpenOptions::new().create(true).create(false).open("foo.txt");
-    //~^ ERROR: the method `create` is called more than once
+    OpenOptions::new()
+        .create(true)
+        .truncate(true) // Ensure we don't trigger suspicious open options by having create without truncate
+        .create(false)
+        //~^ ERROR: the method `create` is called more than once
+        .open("foo.txt");
     OpenOptions::new().write(true).write(false).open("foo.txt");
     //~^ ERROR: the method `write` is called more than once
     OpenOptions::new().append(true).append(false).open("foo.txt");
     //~^ 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
+
+    std::fs::File::options().read(true).read(false).open("foo.txt");
+    //~^ ERROR: the method `read` is called more than once
+
+    let mut options = std::fs::OpenOptions::new();
+    options.read(true);
+    options.read(false);
+    //~^ ERROR: the method `read` is called more than once
 }
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:5:5
    |
 LL |     OpenOptions::new().read(true).truncate(true).open("foo.txt");
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -8,66 +8,46 @@ 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:8: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:35
+  --> $DIR/open_options.rs:11:35
    |
 LL |     OpenOptions::new().read(true).read(false).open("foo.txt");
    |                                   ^^^^^^^^^^^
 
 error: the method `create` is called more than once
-  --> $DIR/open_options.rs:14:37
+  --> $DIR/open_options.rs:16:10
    |
-LL |     OpenOptions::new().create(true).create(false).open("foo.txt");
-   |                                     ^^^^^^^^^^^^^
-
-error: file opened with `create`, but `truncate` behavior not defined
-  --> $DIR/open_options.rs:14:24
-   |
-LL |     OpenOptions::new().create(true).create(false).open("foo.txt");
-   |                        ^^^^^^^^^^^^
-   |
-help: if you intend to overwrite an existing file entirely, call `.truncate(true)`. if you instead know that you may want to keep some parts of the old file, call `.truncate(false)`
-  --> $DIR/open_options.rs:14: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)]`
+LL |         .create(false)
+   |          ^^^^^^^^^^^^^
 
 error: the method `write` is called more than once
-  --> $DIR/open_options.rs:16:36
+  --> $DIR/open_options.rs:19:36
    |
 LL |     OpenOptions::new().write(true).write(false).open("foo.txt");
    |                                    ^^^^^^^^^^^^
 
 error: the method `append` is called more than once
-  --> $DIR/open_options.rs:18:37
+  --> $DIR/open_options.rs:21:37
    |
 LL |     OpenOptions::new().append(true).append(false).open("foo.txt");
    |                                     ^^^^^^^^^^^^^
 
 error: the method `truncate` is called more than once
-  --> $DIR/open_options.rs:20:39
+  --> $DIR/open_options.rs:23:39
    |
 LL |     OpenOptions::new().truncate(true).truncate(false).open("foo.txt");
    |                                       ^^^^^^^^^^^^^^^
 
-error: file opened with `create`, but `truncate` behavior not defined
-  --> $DIR/open_options.rs:22:24
-   |
-LL |     OpenOptions::new().create(true).open("foo.txt");
-   |                        ^^^^^^^^^^^^
-   |
-help: if you intend to overwrite an existing file entirely, call `.truncate(true)`. if you instead know that you may want to keep some parts of the old file, call `.truncate(false)`
-  --> $DIR/open_options.rs:22:5
+error: the method `read` is called more than once
+  --> $DIR/open_options.rs:26:41
    |
-LL |     OpenOptions::new().create(true).open("foo.txt");
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+LL |     std::fs::File::options().read(true).read(false).open("foo.txt");
+   |                                         ^^^^^^^^^^^
 
-error: aborting due to 9 previous errors
+error: aborting due to 8 previous errors
 
diff --git a/tests/ui/open_options_fixable.fixed b/tests/ui/open_options_fixable.fixed
@@ -0,0 +1,7 @@
+use std::fs::OpenOptions;
+#[allow(unused_must_use)]
+#[warn(clippy::suspicious_open_options)]
+fn main() {
+    OpenOptions::new().create(true).truncate(true).open("foo.txt");
+    //~^ ERROR: file opened with `create`, but `truncate` behavior not defined
+}
diff --git a/tests/ui/open_options_fixable.rs b/tests/ui/open_options_fixable.rs
@@ -0,0 +1,7 @@
+use std::fs::OpenOptions;
+#[allow(unused_must_use)]
+#[warn(clippy::suspicious_open_options)]
+fn main() {
+    OpenOptions::new().create(true).open("foo.txt");
+    //~^ ERROR: file opened with `create`, but `truncate` behavior not defined
+}
diff --git a/tests/ui/open_options_fixable.stderr b/tests/ui/open_options_fixable.stderr
@@ -0,0 +1,12 @@
+error: file opened with `create`, but `truncate` behavior not defined
+  --> $DIR/open_options_fixable.rs:5:24
+   |
+LL |     OpenOptions::new().create(true).open("foo.txt");
+   |                        ^^^^^^^^^^^^- help: add: `.truncate(true)`
+   |
+   = help: if you intend to overwrite an existing file entirely, call `.truncate(true)`. if you instead know that you may want to keep some parts of the old file, call `.truncate(false)`
+   = note: `-D clippy::suspicious-open-options` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::suspicious_open_options)]`
+
+error: aborting due to previous error
+
