More helpful text, small style changes.

rust-lang · Oct 10, 2023 · 3940e9a · 3940e9a
1 parent 5161394
commit 3940e9a
Show file tree

Hide file tree

Showing 5 changed files with 75 additions and 51 deletions.
diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
@@ -2820,8 +2820,8 @@ declare_clippy_lint! {
 
 declare_clippy_lint! {
     /// ### What it does
-    /// Checks for the suspicious use of OpenOptions::create()
-    /// without an explicit OpenOptions::truncate().
+    /// 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
@@ -2841,9 +2841,11 @@ declare_clippy_lint! {
     /// ```rust
     /// use std::fs::OpenOptions;
     ///
+    /// OpenOptions::new().create(true);
+    ///
     /// OpenOptions::new().create(true).truncate(true);
     /// ```
-    #[clippy::version = "1.73.0"]
+    #[clippy::version = "1.75.0"]
     pub SUSPICIOUS_OPEN_OPTIONS,
     suspicious,
     "suspicious combination of options for opening a file"

diff --git a/clippy_lints/src/methods/open_options.rs b/clippy_lints/src/methods/open_options.rs
@@ -1,6 +1,6 @@
 use rustc_data_structures::fx::FxHashMap;
 
-use clippy_utils::diagnostics::span_lint;
+use clippy_utils::diagnostics::{span_lint, span_lint_and_then};
 use clippy_utils::paths;
 use clippy_utils::ty::match_type;
 use rustc_ast::ast::LitKind;
@@ -13,7 +13,11 @@ 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)
         && let Some(impl_id) = cx.tcx.impl_of_method(method_id)
-        && match_type(cx, cx.tcx.type_of(impl_id).instantiate_identity(), &paths::OPEN_OPTIONS)
+        && (
+            match_type(cx, cx.tcx.type_of(impl_id).instantiate_identity(), &paths::OPEN_OPTIONS) ||
+            match_type(cx, cx.tcx.type_of(impl_id).instantiate_identity(), &paths::TOKIO_IO_OPEN_OPTIONS)
+        )
+
     {
         let mut options = Vec::new();
         get_open_options(cx, recv, &mut options);
@@ -49,12 +53,15 @@ impl std::fmt::Display for OpenOption {
     }
 }
 
-fn get_open_options(cx: &LateContext<'_>, argument: &Expr<'_>, options: &mut Vec<(OpenOption, Argument)>) {
-    if let ExprKind::MethodCall(path, receiver, arguments, _) = argument.kind {
+fn get_open_options(cx: &LateContext<'_>, argument: &Expr<'_>, options: &mut Vec<(OpenOption, Argument, Span)>) {
+    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 match_type(cx, obj_ty, &paths::OPEN_OPTIONS) && !arguments.is_empty() {
+        // Only proceed if this is a call on some object of type std::fs::OpenOptions or
+        // tokio::fs::OpenOptions
+        if !arguments.is_empty()
+            && (match_type(cx, obj_ty, &paths::OPEN_OPTIONS) || match_type(cx, obj_ty, &paths::TOKIO_IO_OPEN_OPTIONS))
+        {
             let argument_option = match arguments[0].kind {
                 ExprKind::Lit(span) => {
                     if let Spanned {
@@ -74,22 +81,22 @@ fn get_open_options(cx: &LateContext<'_>, argument: &Expr<'_>, options: &mut Vec
 
             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));
+                    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));
                 },
                 _ => (),
             }
@@ -99,24 +106,25 @@ fn get_open_options(cx: &LateContext<'_>, argument: &Expr<'_>, options: &mut Vec
     }
 }
 
-fn check_open_options(cx: &LateContext<'_>, settings: &[(OpenOption, Argument)], span: Span) {
+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) in settings {
-        if options.insert(option.clone(), arg.clone()).is_some() {
+    for (option, arg, sp) in settings {
+        if let Some((_, prev_span)) = options.insert(option.clone(), (arg.clone(), *sp)) {
             span_lint(
                 cx,
                 NONSENSICAL_OPEN_OPTIONS,
-                span,
+                prev_span,
                 &format!("the method `{}` is called more than once", &option),
             );
         }
     }
 
-    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) {
+    if_chain! {
+        if let Some((Argument::Set(true), _)) = options.get(&OpenOption::Read);
+        if let Some((Argument::Set(true), _)) = options.get(&OpenOption::Truncate);
+        if let None | Some((Argument::Set(false), _)) = options.get(&OpenOption::Write);
+        then {
             span_lint(
                 cx,
                 NONSENSICAL_OPEN_OPTIONS,
@@ -126,10 +134,11 @@ fn check_open_options(cx: &LateContext<'_>, settings: &[(OpenOption, Argument)],
         }
     }
 
-    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) {
+    if_chain! {
+        if let Some((Argument::Set(true), _)) = options.get(&OpenOption::Append);
+        if let Some((Argument::Set(true), _)) = options.get(&OpenOption::Truncate);
+        if let None | Some((Argument::Set(false), _)) = options.get(&OpenOption::Write);
+        then {
             span_lint(
                 cx,
                 NONSENSICAL_OPEN_OPTIONS,
@@ -139,12 +148,21 @@ fn check_open_options(cx: &LateContext<'_>, settings: &[(OpenOption, Argument)],
         }
     }
 
-    if let (Some(Argument::Set(true)), None) = (options.get(&OpenOption::Create), options.get(&OpenOption::Truncate)) {
-        span_lint(
-            cx,
-            SUSPICIOUS_OPEN_OPTIONS,
-            span,
-            "file opened with `create`, but `truncate` behavior not defined",
-        );
+    if_chain! {
+        if let Some((Argument::Set(true), create_span)) = options.get(&OpenOption::Create);
+        if let None = options.get(&OpenOption::Truncate);
+        then {
+            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/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs
@@ -63,6 +63,8 @@ pub const MEM_SWAP: [&str; 3] = ["core", "mem", "swap"];
 #[cfg(feature = "internal")]
 pub const MSRV: [&str; 3] = ["clippy_utils", "msrvs", "Msrv"];
 pub const OPEN_OPTIONS: [&str; 3] = ["std", "fs", "OpenOptions"];
+#[expect(clippy::invalid_paths)] // internal lints do not know about all external crates
+pub const TOKIO_IO_OPEN_OPTIONS: [&str; 3] = ["tokio", "fs", "OpenOptions"];
 pub const OS_STRING_AS_OS_STR: [&str; 5] = ["std", "ffi", "os_str", "OsString", "as_os_str"];
 pub const OS_STR_TO_OS_STRING: [&str; 5] = ["std", "ffi", "os_str", "OsStr", "to_os_string"];
 pub const PARKING_LOT_MUTEX_GUARD: [&str; 3] = ["lock_api", "mutex", "MutexGuard"];

diff --git a/tests/ui/open_options.rs b/tests/ui/open_options.rs
@@ -1,5 +1,4 @@
 use std::fs::OpenOptions;
-
 #[allow(unused_must_use)]
 #[warn(clippy::nonsensical_open_options)]
 #[warn(clippy::suspicious_open_options)]

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:7:5
+  --> $DIR/open_options.rs:6:5
    |
 LL |     OpenOptions::new().read(true).truncate(true).open("foo.txt");
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -8,55 +8,58 @@ 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:10:5
+  --> $DIR/open_options.rs:9:5
    |
 LL |     OpenOptions::new().append(true).truncate(true).open("foo.txt");
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: the method `read` is called more than once
-  --> $DIR/open_options.rs:13:5
+  --> $DIR/open_options.rs:12:35
    |
 LL |     OpenOptions::new().read(true).read(false).open("foo.txt");
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |                                   ^^^^^^^^^^^
 
 error: the method `create` is called more than once
-  --> $DIR/open_options.rs:15:5
+  --> $DIR/open_options.rs:14:37
    |
 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
+  --> $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)`
    = 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:17:5
+  --> $DIR/open_options.rs:16:36
    |
 LL |     OpenOptions::new().write(true).write(false).open("foo.txt");
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |                                    ^^^^^^^^^^^^
 
 error: the method `append` is called more than once
-  --> $DIR/open_options.rs:19:5
+  --> $DIR/open_options.rs:18:37
    |
 LL |     OpenOptions::new().append(true).append(false).open("foo.txt");
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |                                     ^^^^^^^^^^^^^
 
 error: the method `truncate` is called more than once
-  --> $DIR/open_options.rs:21:5
+  --> $DIR/open_options.rs:20: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:23:5
+  --> $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)`
 
 error: aborting due to 9 previous errors