Auto merge of #11902 - GuillaumeGomez:write-and-append, r=llogiq

Add `ineffective_open_options` lint

Fixes https://github.com/rust-lang/rust-clippy/issues/9628.

For `OpenOptions`, in case you call both `write(true)` and `append(true)` on `OpenOptions`, then `write(true)` is actually useless since `append(true)` does it.

r? `@flip1995`

changelog: Add `ineffective_open_options` lint

CHANGELOG.md

@@ -5146,6 +5146,7 @@ Released 2018-09-13
 [`index_refutable_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#index_refutable_slice
 [`indexing_slicing`]: https://rust-lang.github.io/rust-clippy/master/index.html#indexing_slicing
 [`ineffective_bit_mask`]: https://rust-lang.github.io/rust-clippy/master/index.html#ineffective_bit_mask
+[`ineffective_open_options`]: https://rust-lang.github.io/rust-clippy/master/index.html#ineffective_open_options
 [`inefficient_to_string`]: https://rust-lang.github.io/rust-clippy/master/index.html#inefficient_to_string
 [`infallible_destructuring_match`]: https://rust-lang.github.io/rust-clippy/master/index.html#infallible_destructuring_match
 [`infinite_iter`]: https://rust-lang.github.io/rust-clippy/master/index.html#infinite_iter

clippy_lints/src/declared_lints.rs

@@ -215,6 +215,7 @@
     crate::index_refutable_slice::INDEX_REFUTABLE_SLICE_INFO,
     crate::indexing_slicing::INDEXING_SLICING_INFO,
     crate::indexing_slicing::OUT_OF_BOUNDS_INDEXING_INFO,
+    crate::ineffective_open_options::INEFFECTIVE_OPEN_OPTIONS_INFO,
     crate::infinite_iter::INFINITE_ITER_INFO,
     crate::infinite_iter::MAYBE_INFINITE_ITER_INFO,
     crate::inherent_impl::MULTIPLE_INHERENT_IMPL_INFO,

clippy_lints/src/ineffective_open_options.rs

@@ -0,0 +1,95 @@
+use crate::methods::method_call;
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::peel_blocks;
+use rustc_ast::LitKind;
+use rustc_errors::Applicability;
+use rustc_hir::{Expr, ExprKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty;
+use rustc_session::declare_lint_pass;
+use rustc_span::{sym, BytePos, Span};
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks if both `.write(true)` and `.append(true)` methods are called
+    /// on a same `OpenOptions`.
+    ///
+    /// ### Why is this bad?
+    /// `.append(true)` already enables `write(true)`, making this one
+    /// superflous.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// # use std::fs::OpenOptions;
+    /// let _ = OpenOptions::new()
+    ///            .write(true)
+    ///            .append(true)
+    ///            .create(true)
+    ///            .open("file.json");
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// # use std::fs::OpenOptions;
+    /// let _ = OpenOptions::new()
+    ///            .append(true)
+    ///            .create(true)
+    ///            .open("file.json");
+    /// ```
+    #[clippy::version = "1.76.0"]
+    pub INEFFECTIVE_OPEN_OPTIONS,
+    suspicious,
+    "usage of both `write(true)` and `append(true)` on same `OpenOptions`"
+}
+
+declare_lint_pass!(IneffectiveOpenOptions => [INEFFECTIVE_OPEN_OPTIONS]);
+
+fn index_if_arg_is_boolean(args: &[Expr<'_>], call_span: Span) -> Option<Span> {
+    if let [arg] = args
+        && let ExprKind::Lit(lit) = peel_blocks(arg).kind
+        && lit.node == LitKind::Bool(true)
+    {
+        // The `.` is not included in the span so we cheat a little bit to include it as well.
+        Some(call_span.with_lo(call_span.lo() - BytePos(1)))
+    } else {
+        None
+    }
+}
+
+impl<'tcx> LateLintPass<'tcx> for IneffectiveOpenOptions {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
+        let Some(("open", mut receiver, [_arg], _, _)) = method_call(expr) else {
+            return;
+        };
+        let receiver_ty = cx.typeck_results().expr_ty(receiver);
+        match receiver_ty.peel_refs().kind() {
+            ty::Adt(adt, _) if cx.tcx.is_diagnostic_item(sym::FsOpenOptions, adt.did()) => {},
+            _ => return,
+        }
+
+        let mut append = None;
+        let mut write = None;
+
+        while let Some((name, recv, args, _, span)) = method_call(receiver) {
+            if name == "append" {
+                append = index_if_arg_is_boolean(args, span);
+            } else if name == "write" {
+                write = index_if_arg_is_boolean(args, span);
+            }
+            receiver = recv;
+        }
+
+        if let Some(write_span) = write
+            && append.is_some()
+        {
+            span_lint_and_sugg(
+                cx,
+                INEFFECTIVE_OPEN_OPTIONS,
+                write_span,
+                "unnecessary use of `.write(true)` because there is `.append(true)`",
+                "remove `.write(true)`",
+                String::new(),
+                Applicability::MachineApplicable,
+            );
+        }
+    }
+}

clippy_lints/src/lib.rs

@@ -153,6 +153,7 @@
 mod inconsistent_struct_constructor;
 mod index_refutable_slice;
 mod indexing_slicing;
+mod ineffective_open_options;
 mod infinite_iter;
 mod inherent_impl;
 mod inherent_to_string;
@@ -1073,6 +1074,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     store.register_late_pass(|_| Box::new(impl_hash_with_borrow_str_and_bytes::ImplHashWithBorrowStrBytes));
     store.register_late_pass(|_| Box::new(repeat_vec_with_capacity::RepeatVecWithCapacity));
     store.register_late_pass(|_| Box::new(uninhabited_references::UninhabitedReferences));
+    store.register_late_pass(|_| Box::new(ineffective_open_options::IneffectiveOpenOptions));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 

clippy_lints/src/methods/mod.rs

@@ -3906,7 +3906,7 @@ pub fn new(
 ]);
 
 /// Extracts a method call name, args, and `Span` of the method name.
-fn method_call<'tcx>(
+pub fn method_call<'tcx>(
     recv: &'tcx hir::Expr<'tcx>,
 ) -> Option<(&'tcx str, &'tcx hir::Expr<'tcx>, &'tcx [hir::Expr<'tcx>], Span, Span)> {
     if let ExprKind::MethodCall(path, receiver, args, call_span) = recv.kind {

tests/ui/ineffective_open_options.fixed

@@ -0,0 +1,41 @@
+#![warn(clippy::ineffective_open_options)]
+
+use std::fs::OpenOptions;
+
+fn main() {
+    let file = OpenOptions::new()
+        .create(true)
+         //~ ERROR: unnecessary use of `.write(true)`
+        .append(true)
+        .open("dump.json")
+        .unwrap();
+
+    let file = OpenOptions::new()
+        .create(true)
+        .append(true)
+         //~ ERROR: unnecessary use of `.write(true)`
+        .open("dump.json")
+        .unwrap();
+
+    // All the next calls are ok.
+    let file = OpenOptions::new()
+        .create(true)
+        .write(false)
+        .append(true)
+        .open("dump.json")
+        .unwrap();
+    let file = OpenOptions::new()
+        .create(true)
+        .write(true)
+        .append(false)
+        .open("dump.json")
+        .unwrap();
+    let file = OpenOptions::new()
+        .create(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();
+}

tests/ui/ineffective_open_options.rs

@@ -0,0 +1,41 @@
+#![warn(clippy::ineffective_open_options)]
+
+use std::fs::OpenOptions;
+
+fn main() {
+    let file = OpenOptions::new()
+        .create(true)
+        .write(true) //~ ERROR: unnecessary use of `.write(true)`
+        .append(true)
+        .open("dump.json")
+        .unwrap();
+
+    let file = OpenOptions::new()
+        .create(true)
+        .append(true)
+        .write(true) //~ ERROR: unnecessary use of `.write(true)`
+        .open("dump.json")
+        .unwrap();
+
+    // All the next calls are ok.
+    let file = OpenOptions::new()
+        .create(true)
+        .write(false)
+        .append(true)
+        .open("dump.json")
+        .unwrap();
+    let file = OpenOptions::new()
+        .create(true)
+        .write(true)
+        .append(false)
+        .open("dump.json")
+        .unwrap();
+    let file = OpenOptions::new()
+        .create(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();
+}

tests/ui/ineffective_open_options.stderr

@@ -0,0 +1,17 @@
+error: unnecessary use of `.write(true)` because there is `.append(true)`
+  --> $DIR/ineffective_open_options.rs:8:9
+   |
+LL |         .write(true)
+   |         ^^^^^^^^^^^^ help: remove `.write(true)`
+   |
+   = note: `-D clippy::ineffective-open-options` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::ineffective_open_options)]`
+
+error: unnecessary use of `.write(true)` because there is `.append(true)`
+  --> $DIR/ineffective_open_options.rs:16:9
+   |
+LL |         .write(true)
+   |         ^^^^^^^^^^^^ help: remove `.write(true)`
+
+error: aborting due to 2 previous errors
+