From 515fe65ba8f1006bec5e4e906aaeb345b05834bc Mon Sep 17 00:00:00 2001 From: atwam Date: Thu, 11 Jan 2024 12:49:49 +0000 Subject: [PATCH] Fix conflicts - New ineffective_open_options had to be fixed. - Now not raising an issue on missing `truncate` when `append(true)` makes the intent clear. - Try implementing more advanced tests for non-chained operations. Fail --- clippy_lints/src/methods/open_options.rs | 31 ++++++++++++++++++------ tests/ui/ineffective_open_options.fixed | 9 ++++++- tests/ui/ineffective_open_options.rs | 9 ++++++- tests/ui/open_options.rs | 3 ++- tests/ui/open_options_fixable.stderr | 4 +-- 5 files changed, 44 insertions(+), 12 deletions(-) diff --git a/clippy_lints/src/methods/open_options.rs b/clippy_lints/src/methods/open_options.rs index cbe504a9c63..d45ba2a7728 100644 --- a/clippy_lints/src/methods/open_options.rs +++ b/clippy_lints/src/methods/open_options.rs @@ -1,8 +1,8 @@ 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, match_type}; +use clippy_utils::{match_any_def_paths, paths}; use rustc_ast::ast::LitKind; use rustc_hir::{Expr, ExprKind}; use rustc_lint::LateContext; @@ -19,7 +19,7 @@ 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); @@ -27,7 +27,7 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>, recv: &'tcx } } -#[derive(Eq, PartialEq, Clone)] +#[derive(Eq, PartialEq, Clone, Debug)] enum Argument { Set(bool), Unknown, @@ -55,7 +55,11 @@ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { } } -fn get_open_options(cx: &LateContext<'_>, argument: &Expr<'_>, options: &mut Vec<(OpenOption, Argument, Span)>) { +fn get_open_options( + cx: &LateContext<'_>, + argument: &Expr<'_>, + options: &mut Vec<(OpenOption, Argument, Span)>, +) -> bool { if let ExprKind::MethodCall(path, receiver, arguments, span) = argument.kind { let obj_ty = cx.typeck_results().expr_ty(receiver).peel_refs(); @@ -72,7 +76,8 @@ fn get_open_options(cx: &LateContext<'_>, argument: &Expr<'_>, options: &mut Vec } else { // The function is called with a literal which is not a boolean literal. // This is theoretically possible, but not very likely. - return; + // We'll ignore it for now + return get_open_options(cx, receiver, options); } }, _ => Argument::Unknown, @@ -100,8 +105,19 @@ fn get_open_options(cx: &LateContext<'_>, argument: &Expr<'_>, options: &mut Vec _ => (), } - get_open_options(cx, receiver, options); + get_open_options(cx, receiver, options) + } else { + false } + } else if let ExprKind::Call(callee, _) = argument.kind + && 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) + } else { + false } } @@ -144,6 +160,7 @@ fn check_open_options(cx: &LateContext<'_>, settings: &[(OpenOption, Argument, S if let Some((Argument::Set(true), create_span)) = options.get(&OpenOption::Create) && let None = options.get(&OpenOption::Truncate) + && let None | Some((Argument::Set(false), _)) = options.get(&OpenOption::Append) { span_lint_and_then( cx, @@ -153,7 +170,7 @@ fn check_open_options(cx: &LateContext<'_>, settings: &[(OpenOption, Argument, S |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)`"); + .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."); }, ); } diff --git a/tests/ui/ineffective_open_options.fixed b/tests/ui/ineffective_open_options.fixed index 3af8f3c5aaa..88489dc46bb 100644 --- a/tests/ui/ineffective_open_options.fixed +++ b/tests/ui/ineffective_open_options.fixed @@ -26,16 +26,23 @@ fn main() { .unwrap(); let file = OpenOptions::new() .create(true) + .truncate(true) .write(true) .append(false) .open("dump.json") .unwrap(); let file = OpenOptions::new() .create(true) + .truncate(true) .write(false) .append(false) .open("dump.json") .unwrap(); let file = OpenOptions::new().create(true).append(true).open("dump.json").unwrap(); - let file = OpenOptions::new().create(true).write(true).open("dump.json").unwrap(); + let file = OpenOptions::new() + .create(true) + .truncate(true) + .write(true) + .open("dump.json") + .unwrap(); } diff --git a/tests/ui/ineffective_open_options.rs b/tests/ui/ineffective_open_options.rs index 4eaf6293c40..db8bca3e2ac 100644 --- a/tests/ui/ineffective_open_options.rs +++ b/tests/ui/ineffective_open_options.rs @@ -26,16 +26,23 @@ fn main() { .unwrap(); let file = OpenOptions::new() .create(true) + .truncate(true) .write(true) .append(false) .open("dump.json") .unwrap(); let file = OpenOptions::new() .create(true) + .truncate(true) .write(false) .append(false) .open("dump.json") .unwrap(); let file = OpenOptions::new().create(true).append(true).open("dump.json").unwrap(); - let file = OpenOptions::new().create(true).write(true).open("dump.json").unwrap(); + let file = OpenOptions::new() + .create(true) + .truncate(true) + .write(true) + .open("dump.json") + .unwrap(); } diff --git a/tests/ui/open_options.rs b/tests/ui/open_options.rs index 726ec1b3ab0..873094dd724 100644 --- a/tests/ui/open_options.rs +++ b/tests/ui/open_options.rs @@ -29,5 +29,6 @@ fn main() { let mut options = std::fs::OpenOptions::new(); options.read(true); options.read(false); - //~^ ERROR: the method `read` is called more than once + //#~^ ERROR: the method `read` is called more than once + options.open("foo.txt"); } diff --git a/tests/ui/open_options_fixable.stderr b/tests/ui/open_options_fixable.stderr index 1ef49c54afc..de88af87cd4 100644 --- a/tests/ui/open_options_fixable.stderr +++ b/tests/ui/open_options_fixable.stderr @@ -4,9 +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)` + = 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. = 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 +error: aborting due to 1 previous error