diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 84fb583595b9..3d6a16f9ac61 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2829,8 +2829,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 @@ -2850,9 +2850,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 index 81508a5cf65a..c54a75068366 100644 --- 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::ty::is_type_diagnostic_item; use rustc_ast::ast::LitKind; use rustc_hir::{Expr, ExprKind}; @@ -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) - && is_type_diagnostic_item(cx, cx.tcx.type_of(impl_id).instantiate_identity(), sym::FsOpenOptions) + && ( + 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) + ) + { let mut options = Vec::new(); get_open_options(cx, recv, &mut options); @@ -49,12 +53,12 @@ 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 is_type_diagnostic_item(cx, obj_ty, sym::FsOpenOptions) && !arguments.is_empty() { + if !arguments.is_empty() && (is_type_diagnostic_item(cx, obj_ty, sym::FsOpenOptions)) { let argument_option = match arguments[0].kind { ExprKind::Lit(span) => { if let Spanned { @@ -74,22 +78,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 +103,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 +131,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 +145,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 index 0a820a1754ca..2771e49736e6 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -50,6 +50,8 @@ pub const LATE_CONTEXT: [&str; 2] = ["rustc_lint", "LateContext"]; pub const LATE_LINT_PASS: [&str; 3] = ["rustc_lint", "passes", "LateLintPass"]; pub const LINT: [&str; 2] = ["rustc_lint_defs", "Lint"]; pub const MSRV: [&str; 3] = ["clippy_config", "msrvs", "Msrv"]; +#[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 index dd380f4101ad..3f486b76e723 100644 --- 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 index be6be250f22a..ecab243069d3 100644 --- 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