diff --git a/clippy_lints/src/methods/open_options.rs b/clippy_lints/src/methods/open_options.rs index c006ea192ee..cbe504a9c63 100644 --- a/clippy_lints/src/methods/open_options.rs +++ b/clippy_lints/src/methods/open_options.rs @@ -2,23 +2,24 @@ 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 index 3f486b76e72..726ec1b3ab0 100644 --- 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 index 11ba7444c97..3e7d5a3499d 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: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 +error: the method `read` is called more than once + --> $DIR/open_options.rs:26:41 | -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 - | -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 new file mode 100644 index 00000000000..90a129a9bdf --- /dev/null +++ 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 new file mode 100644 index 00000000000..3a9e522ba15 --- /dev/null +++ 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 new file mode 100644 index 00000000000..1ef49c54afc --- /dev/null +++ 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 +