diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 3d6a16f9ac61..03bcf108914a 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2833,10 +2833,11 @@ declare_clippy_lint! { /// without an explicit `OpenOptions::truncate()`. /// /// ### Why is this bad? - /// create() alone will either create a new file or open an + /// `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 + /// 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. @@ -2847,10 +2848,14 @@ declare_clippy_lint! { /// `truncate(false)` will explicitely keep the default behavior. /// /// ### Example - /// ```rust + /// ```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); /// ``` diff --git a/clippy_lints/src/methods/open_options.rs b/clippy_lints/src/methods/open_options.rs index d45ba2a77285..77484ab91a9d 100644 --- a/clippy_lints/src/methods/open_options.rs +++ b/clippy_lints/src/methods/open_options.rs @@ -19,11 +19,12 @@ fn is_open_options(cx: &LateContext<'_>, ty: Ty<'_>) -> bool { 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_open_options(cx, cx.tcx.type_of(impl_id).instantiate_identity()) + && 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); + } } } @@ -55,6 +56,9 @@ impl std::fmt::Display for OpenOption { } } +/// 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<'_>, @@ -102,7 +106,16 @@ fn get_open_options( "write" => { 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) @@ -113,9 +126,17 @@ fn get_open_options( && 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]).is_some() - //is_type_diagnostic_item(cx, ty, sym::FsOpenOptions) || match_type(cx, ty, - // &paths::TOKIO_IO_OPEN_OPTIONS) + 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 } @@ -168,9 +189,17 @@ fn check_open_options(cx: &LateContext<'_>, settings: &[(OpenOption, Argument, S *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)`. Alternatively, use `.append` to append to the file instead of overwriting it."); + 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"); }, ); } diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index 2771e49736e6..0051f7845146 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -26,6 +26,7 @@ pub const EARLY_CONTEXT: [&str; 2] = ["rustc_lint", "EarlyContext"]; pub const EARLY_LINT_PASS: [&str; 3] = ["rustc_lint", "passes", "EarlyLintPass"]; pub const F32_EPSILON: [&str; 4] = ["core", "f32", "", "EPSILON"]; pub const F64_EPSILON: [&str; 4] = ["core", "f64", "", "EPSILON"]; +pub const FILE_OPTIONS: [&str; 4] = ["std", "fs", "File", "options"]; #[expect(clippy::invalid_paths)] // internal lints do not know about all external crates pub const FUTURES_IO_ASYNCREADEXT: [&str; 3] = ["futures_util", "io", "AsyncReadExt"]; #[expect(clippy::invalid_paths)] // internal lints do not know about all external crates @@ -50,8 +51,7 @@ 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 OPEN_OPTIONS_NEW: [&str; 4] = ["std", "fs", "OpenOptions", "new"]; 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"]; @@ -91,9 +91,15 @@ pub const SYMBOL_TO_IDENT_STRING: [&str; 4] = ["rustc_span", "symbol", "Symbol", pub const SYM_MODULE: [&str; 3] = ["rustc_span", "symbol", "sym"]; pub const SYNTAX_CONTEXT: [&str; 3] = ["rustc_span", "hygiene", "SyntaxContext"]; #[expect(clippy::invalid_paths)] // internal lints do not know about all external crates +pub const TOKIO_FILE_OPTIONS: [&str; 5] = ["tokio", "fs", "file", "File", "options"]; +#[expect(clippy::invalid_paths)] // internal lints do not know about all external crates pub const TOKIO_IO_ASYNCREADEXT: [&str; 5] = ["tokio", "io", "util", "async_read_ext", "AsyncReadExt"]; #[expect(clippy::invalid_paths)] // internal lints do not know about all external crates pub const TOKIO_IO_ASYNCWRITEEXT: [&str; 5] = ["tokio", "io", "util", "async_write_ext", "AsyncWriteExt"]; +#[expect(clippy::invalid_paths)] // internal lints do not know about all external crates +pub const TOKIO_IO_OPEN_OPTIONS: [&str; 4] = ["tokio", "fs", "open_options", "OpenOptions"]; +#[expect(clippy::invalid_paths)] // internal lints do not know about all external crates +pub const TOKIO_IO_OPEN_OPTIONS_NEW: [&str; 5] = ["tokio", "fs", "open_options", "OpenOptions", "new"]; pub const VEC_AS_MUT_SLICE: [&str; 4] = ["alloc", "vec", "Vec", "as_mut_slice"]; pub const VEC_AS_SLICE: [&str; 4] = ["alloc", "vec", "Vec", "as_slice"]; pub const VEC_DEQUE_ITER: [&str; 5] = ["alloc", "collections", "vec_deque", "VecDeque", "iter"]; diff --git a/tests/ui/open_options.rs b/tests/ui/open_options.rs index 873094dd7245..4acb3780df43 100644 --- a/tests/ui/open_options.rs +++ b/tests/ui/open_options.rs @@ -1,6 +1,18 @@ +#![allow(unused_must_use)] +#![warn(clippy::nonsensical_open_options)] + use std::fs::OpenOptions; -#[allow(unused_must_use)] -#[warn(clippy::nonsensical_open_options)] + +trait OpenOptionsExt { + fn truncate_write(&mut self, opt: bool) -> &mut Self; +} + +impl OpenOptionsExt for OpenOptions { + fn truncate_write(&mut self, opt: bool) -> &mut Self { + self.truncate(opt).write(opt) + } +} + fn main() { OpenOptions::new().read(true).truncate(true).open("foo.txt"); //~^ ERROR: file opened with `truncate` and `read` @@ -29,6 +41,12 @@ fn main() { let mut options = std::fs::OpenOptions::new(); options.read(true); options.read(false); - //#~^ ERROR: the method `read` is called more than once + // Possible future improvement: recognize open options method call chains across statements. options.open("foo.txt"); + + let mut options = std::fs::OpenOptions::new(); + options.truncate(true); + options.create(true).open("foo.txt"); + + OpenOptions::new().create(true).truncate_write(true).open("foo.txt"); } diff --git a/tests/ui/open_options.stderr b/tests/ui/open_options.stderr index 3e7d5a3499d3..4f84d1d09f98 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:5:5 + --> $DIR/open_options.rs:17:5 | LL | OpenOptions::new().read(true).truncate(true).open("foo.txt"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -8,43 +8,43 @@ 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:8:5 + --> $DIR/open_options.rs:20:5 | LL | OpenOptions::new().append(true).truncate(true).open("foo.txt"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: the method `read` is called more than once - --> $DIR/open_options.rs:11:35 + --> $DIR/open_options.rs:23:35 | LL | OpenOptions::new().read(true).read(false).open("foo.txt"); | ^^^^^^^^^^^ error: the method `create` is called more than once - --> $DIR/open_options.rs:16:10 + --> $DIR/open_options.rs:28:10 | LL | .create(false) | ^^^^^^^^^^^^^ error: the method `write` is called more than once - --> $DIR/open_options.rs:19:36 + --> $DIR/open_options.rs:31:36 | LL | OpenOptions::new().write(true).write(false).open("foo.txt"); | ^^^^^^^^^^^^ error: the method `append` is called more than once - --> $DIR/open_options.rs:21:37 + --> $DIR/open_options.rs:33:37 | LL | OpenOptions::new().append(true).append(false).open("foo.txt"); | ^^^^^^^^^^^^^ error: the method `truncate` is called more than once - --> $DIR/open_options.rs:23:39 + --> $DIR/open_options.rs:35:39 | LL | OpenOptions::new().truncate(true).truncate(false).open("foo.txt"); | ^^^^^^^^^^^^^^^ error: the method `read` is called more than once - --> $DIR/open_options.rs:26:41 + --> $DIR/open_options.rs:38:41 | LL | std::fs::File::options().read(true).read(false).open("foo.txt"); | ^^^^^^^^^^^ diff --git a/tests/ui/open_options_fixable.stderr b/tests/ui/open_options_fixable.stderr index de88af87cd41..e327661713bf 100644 --- a/tests/ui/open_options_fixable.stderr +++ b/tests/ui/open_options_fixable.stderr @@ -4,7 +4,9 @@ error: file opened with `create`, but `truncate` behavior not defined 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)`. Alternatively, use `.append` to append to the file instead of overwriting it. + = 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 = note: `-D clippy::suspicious-open-options` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::suspicious_open_options)]`