Auto merge of #8071 - GuillaumeGomez:method-must-use, r=xFrednet

Add new lint to warn when #[must_use] attribute should be used on a method

This lint is somewhat similar to https://rust-lang.github.io/rust-clippy/master/index.html#must_use_candidate but also different: it emits a warning by default and only targets methods (so not functions nor associated functions).

Someone suggested it to me after this tweet: https://twitter.com/m_ou_se/status/1466439813230477312

I think it would reduce the number of cases of API misuses quite a lot.

What do you think?

Author: bors

CHANGELOG.md

@@ -3118,6 +3118,7 @@ Released 2018-09-13
 [`result_map_or_into_option`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_or_into_option
 [`result_map_unit_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_unit_fn
 [`result_unit_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_unit_err
+[`return_self_not_must_use`]: https://rust-lang.github.io/rust-clippy/master/index.html#return_self_not_must_use
 [`reversed_empty_ranges`]: https://rust-lang.github.io/rust-clippy/master/index.html#reversed_empty_ranges
 [`same_functions_in_if_condition`]: https://rust-lang.github.io/rust-clippy/master/index.html#same_functions_in_if_condition
 [`same_item_push`]: https://rust-lang.github.io/rust-clippy/master/index.html#same_item_push

clippy_lints/src/lib.register_all.rs

@@ -245,6 +245,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(reference::REF_IN_DEREF),
     LintId::of(regex::INVALID_REGEX),
     LintId::of(repeat_once::REPEAT_ONCE),
+    LintId::of(return_self_not_must_use::RETURN_SELF_NOT_MUST_USE),
     LintId::of(returns::LET_AND_RETURN),
     LintId::of(returns::NEEDLESS_RETURN),
     LintId::of(self_assignment::SELF_ASSIGNMENT),

clippy_lints/src/lib.register_lints.rs

@@ -422,6 +422,7 @@ store.register_lints(&[
     regex::INVALID_REGEX,
     regex::TRIVIAL_REGEX,
     repeat_once::REPEAT_ONCE,
+    return_self_not_must_use::RETURN_SELF_NOT_MUST_USE,
     returns::LET_AND_RETURN,
     returns::NEEDLESS_RETURN,
     same_name_method::SAME_NAME_METHOD,

clippy_lints/src/lib.register_suspicious.rs

@@ -16,6 +16,7 @@ store.register_group(true, "clippy::suspicious", Some("clippy_suspicious"), vec!
     LintId::of(methods::SUSPICIOUS_MAP),
     LintId::of(mut_key::MUTABLE_KEY_TYPE),
     LintId::of(octal_escapes::OCTAL_ESCAPES),
+    LintId::of(return_self_not_must_use::RETURN_SELF_NOT_MUST_USE),
     LintId::of(suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL),
     LintId::of(suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL),
 ])

clippy_lints/src/lib.rs

@@ -341,6 +341,7 @@ mod ref_option_ref;
 mod reference;
 mod regex;
 mod repeat_once;
+mod return_self_not_must_use;
 mod returns;
 mod same_name_method;
 mod self_assignment;
@@ -853,6 +854,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| Box::new(trailing_empty_array::TrailingEmptyArray));
     store.register_early_pass(|| Box::new(octal_escapes::OctalEscapes));
     store.register_late_pass(|| Box::new(needless_late_init::NeedlessLateInit));
+    store.register_late_pass(|| Box::new(return_self_not_must_use::ReturnSelfNotMustUse));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 

clippy_lints/src/return_self_not_must_use.rs

@@ -0,0 +1,105 @@
+use clippy_utils::{diagnostics::span_lint, must_use_attr, nth_arg, return_ty};
+use rustc_hir::def_id::LocalDefId;
+use rustc_hir::intravisit::FnKind;
+use rustc_hir::{Body, FnDecl, HirId, TraitItem, TraitItemKind};
+use rustc_lint::{LateContext, LateLintPass, LintContext};
+use rustc_middle::lint::in_external_macro;
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::Span;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// This lint warns when a method returning `Self` doesn't have the `#[must_use]` attribute.
+    ///
+    /// ### Why is this bad?
+    /// It prevents to "forget" to use the newly created value.
+    ///
+    /// ### Limitations
+    /// This lint is only applied on methods taking a `self` argument. It would be mostly noise
+    /// if it was added on constructors for example.
+    ///
+    /// ### Example
+    /// ```rust
+    /// pub struct Bar;
+    ///
+    /// impl Bar {
+    ///     // Bad
+    ///     pub fn bar(&self) -> Self {
+    ///         Self
+    ///     }
+    ///
+    ///     // Good
+    ///     #[must_use]
+    ///     pub fn foo(&self) -> Self {
+    ///         Self
+    ///     }
+    /// }
+    /// ```
+    #[clippy::version = "1.59.0"]
+    pub RETURN_SELF_NOT_MUST_USE,
+    suspicious,
+    "missing `#[must_use]` annotation on a method returning `Self`"
+}
+
+declare_lint_pass!(ReturnSelfNotMustUse => [RETURN_SELF_NOT_MUST_USE]);
+
+fn check_method(cx: &LateContext<'tcx>, decl: &'tcx FnDecl<'tcx>, fn_def: LocalDefId, span: Span, hir_id: HirId) {
+    if_chain! {
+        // If it comes from an external macro, better ignore it.
+        if !in_external_macro(cx.sess(), span);
+        if decl.implicit_self.has_implicit_self();
+        // We only show this warning for public exported methods.
+        if cx.access_levels.is_exported(fn_def);
+        if cx.tcx.visibility(fn_def.to_def_id()).is_public();
+        // No need to warn if the attribute is already present.
+        if must_use_attr(cx.tcx.hir().attrs(hir_id)).is_none();
+        let ret_ty = return_ty(cx, hir_id);
+        let self_arg = nth_arg(cx, hir_id, 0);
+        // If `Self` has the same type as the returned type, then we want to warn.
+        //
+        // For this check, we don't want to remove the reference on the returned type because if
+        // there is one, we shouldn't emit a warning!
+        if self_arg.peel_refs() == ret_ty;
+
+        then {
+            span_lint(
+                cx,
+                RETURN_SELF_NOT_MUST_USE,
+                span,
+                "missing `#[must_use]` attribute on a method returning `Self`",
+            );
+        }
+    }
+}
+
+impl<'tcx> LateLintPass<'tcx> for ReturnSelfNotMustUse {
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'tcx>,
+        kind: FnKind<'tcx>,
+        decl: &'tcx FnDecl<'tcx>,
+        _: &'tcx Body<'tcx>,
+        span: Span,
+        hir_id: HirId,
+    ) {
+        if_chain! {
+            // We are only interested in methods, not in functions or associated functions.
+            if matches!(kind, FnKind::Method(_, _, _));
+            if let Some(fn_def) = cx.tcx.hir().opt_local_def_id(hir_id);
+            if let Some(impl_def) = cx.tcx.impl_of_method(fn_def.to_def_id());
+            // We don't want this method to be te implementation of a trait because the
+            // `#[must_use]` should be put on the trait definition directly.
+            if cx.tcx.trait_id_of_impl(impl_def).is_none();
+
+            then {
+                check_method(cx, decl, fn_def, span, hir_id);
+            }
+        }
+    }
+
+    fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx TraitItem<'tcx>) {
+        if let TraitItemKind::Fn(ref sig, _) = item.kind {
+            check_method(cx, sig.decl, item.def_id, item.span, item.hir_id());
+        }
+    }
+}

clippy_utils/src/hir_utils.rs

@@ -41,13 +41,15 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> {
     }
 
     /// Consider expressions containing potential side effects as not equal.
+    #[must_use]
     pub fn deny_side_effects(self) -> Self {
         Self {
             allow_side_effects: false,
             ..self
         }
     }
 
+    #[must_use]
     pub fn expr_fallback(self, expr_fallback: impl FnMut(&Expr<'_>, &Expr<'_>) -> bool + 'a) -> Self {
         Self {
             expr_fallback: Some(Box::new(expr_fallback)),

clippy_utils/src/lib.rs

@@ -1393,6 +1393,13 @@ pub fn return_ty<'tcx>(cx: &LateContext<'tcx>, fn_item: hir::HirId) -> Ty<'tcx>
     cx.tcx.erase_late_bound_regions(ret_ty)
 }
 
+/// Convenience function to get the nth argument type of a function.
+pub fn nth_arg<'tcx>(cx: &LateContext<'tcx>, fn_item: hir::HirId, nth: usize) -> Ty<'tcx> {
+    let fn_def_id = cx.tcx.hir().local_def_id(fn_item);
+    let arg = cx.tcx.fn_sig(fn_def_id).input(nth);
+    cx.tcx.erase_late_bound_regions(arg)
+}
+
 /// Checks if an expression is constructing a tuple-like enum variant or struct
 pub fn is_ctor_or_promotable_const_function(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
     if let ExprKind::Call(fun, _) = expr.kind {

clippy_utils/src/sugg.rs

@@ -294,6 +294,7 @@ impl<'a> Sugg<'a> {
     /// Adds parentheses to any expression that might need them. Suitable to the
     /// `self` argument of a method call
     /// (e.g., to build `bar.foo()` or `(1 + 2).foo()`).
+    #[must_use]
     pub fn maybe_par(self) -> Self {
         match self {
             Sugg::NonParen(..) => self,

tests/ui/auxiliary/option_helpers.rs

@@ -1,4 +1,4 @@
-#![allow(dead_code, unused_variables)]
+#![allow(dead_code, unused_variables, clippy::return_self_not_must_use)]
 
 /// Utility macro to test linting behavior in `option_methods()`
 /// The lints included in `option_methods()` should not lint if the call to map is partially

tests/ui/deref_addrof.fixed

@@ -1,4 +1,5 @@
 // run-rustfix
+#![allow(clippy::return_self_not_must_use)]
 #![warn(clippy::deref_addrof)]
 
 fn get_number() -> usize {

tests/ui/deref_addrof.rs

@@ -1,4 +1,5 @@
 // run-rustfix
+#![allow(clippy::return_self_not_must_use)]
 #![warn(clippy::deref_addrof)]
 
 fn get_number() -> usize {

tests/ui/deref_addrof.stderr

@@ -1,55 +1,55 @@
 error: immediately dereferencing a reference
-  --> $DIR/deref_addrof.rs:18:13
+  --> $DIR/deref_addrof.rs:19:13
    |
 LL |     let b = *&a;
    |             ^^^ help: try this: `a`
    |
    = note: `-D clippy::deref-addrof` implied by `-D warnings`
 
 error: immediately dereferencing a reference
-  --> $DIR/deref_addrof.rs:20:13
+  --> $DIR/deref_addrof.rs:21:13
    |
 LL |     let b = *&get_number();
    |             ^^^^^^^^^^^^^^ help: try this: `get_number()`
 
 error: immediately dereferencing a reference
-  --> $DIR/deref_addrof.rs:25:13
+  --> $DIR/deref_addrof.rs:26:13
    |
 LL |     let b = *&bytes[1..2][0];
    |             ^^^^^^^^^^^^^^^^ help: try this: `bytes[1..2][0]`
 
 error: immediately dereferencing a reference
-  --> $DIR/deref_addrof.rs:29:13
+  --> $DIR/deref_addrof.rs:30:13
    |
 LL |     let b = *&(a);
    |             ^^^^^ help: try this: `(a)`
 
 error: immediately dereferencing a reference
-  --> $DIR/deref_addrof.rs:31:13
+  --> $DIR/deref_addrof.rs:32:13
    |
 LL |     let b = *(&a);
    |             ^^^^^ help: try this: `a`
 
 error: immediately dereferencing a reference
-  --> $DIR/deref_addrof.rs:34:13
+  --> $DIR/deref_addrof.rs:35:13
    |
 LL |     let b = *((&a));
    |             ^^^^^^^ help: try this: `a`
 
 error: immediately dereferencing a reference
-  --> $DIR/deref_addrof.rs:36:13
+  --> $DIR/deref_addrof.rs:37:13
    |
 LL |     let b = *&&a;
    |             ^^^^ help: try this: `&a`
 
 error: immediately dereferencing a reference
-  --> $DIR/deref_addrof.rs:38:14
+  --> $DIR/deref_addrof.rs:39:14
    |
 LL |     let b = **&aref;
    |              ^^^^^^ help: try this: `aref`
 
 error: immediately dereferencing a reference
-  --> $DIR/deref_addrof.rs:44:9
+  --> $DIR/deref_addrof.rs:45:9
    |
 LL |         *& $visitor
    |         ^^^^^^^^^^^ help: try this: `$visitor`
@@ -60,7 +60,7 @@ LL |         m!(self)
    = note: this error originates in the macro `m` (in Nightly builds, run with -Z macro-backtrace for more info)
 
 error: immediately dereferencing a reference
-  --> $DIR/deref_addrof.rs:51:9
+  --> $DIR/deref_addrof.rs:52:9
    |
 LL |         *& mut $visitor
    |         ^^^^^^^^^^^^^^^ help: try this: `$visitor`

tests/ui/return_self_not_must_use.rs

@@ -0,0 +1,42 @@
+#![crate_type = "lib"]
+
+#[derive(Clone)]
+pub struct Bar;
+
+pub trait Whatever {
+    fn what(&self) -> Self;
+    // There should be no warning here!
+    fn what2(&self) -> &Self;
+}
+
+impl Bar {
+    // There should be no warning here!
+    pub fn not_new() -> Self {
+        Self
+    }
+    pub fn foo(&self) -> Self {
+        Self
+    }
+    pub fn bar(self) -> Self {
+        self
+    }
+    // There should be no warning here!
+    fn foo2(&self) -> Self {
+        Self
+    }
+    // There should be no warning here!
+    pub fn foo3(&self) -> &Self {
+        self
+    }
+}
+
+impl Whatever for Bar {
+    // There should be no warning here!
+    fn what(&self) -> Self {
+        self.foo2()
+    }
+    // There should be no warning here!
+    fn what2(&self) -> &Self {
+        self
+    }
+}

tests/ui/return_self_not_must_use.stderr

@@ -0,0 +1,26 @@
+error: missing `#[must_use]` attribute on a method returning `Self`
+  --> $DIR/return_self_not_must_use.rs:7:5
+   |
+LL |     fn what(&self) -> Self;
+   |     ^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::return-self-not-must-use` implied by `-D warnings`
+
+error: missing `#[must_use]` attribute on a method returning `Self`
+  --> $DIR/return_self_not_must_use.rs:17:5
+   |
+LL | /     pub fn foo(&self) -> Self {
+LL | |         Self
+LL | |     }
+   | |_____^
+
+error: missing `#[must_use]` attribute on a method returning `Self`
+  --> $DIR/return_self_not_must_use.rs:20:5
+   |
+LL | /     pub fn bar(self) -> Self {
+LL | |         self
+LL | |     }
+   | |_____^
+
+error: aborting due to 3 previous errors
+

tests/ui/should_impl_trait/corner_cases.rs

@@ -7,7 +7,8 @@
     clippy::needless_lifetimes,
     clippy::missing_safety_doc,
     clippy::wrong_self_convention,
-    clippy::missing_panics_doc
+    clippy::missing_panics_doc,
+    clippy::return_self_not_must_use
 )]
 
 use std::ops::Mul;

tests/ui/should_impl_trait/method_list_1.rs

@@ -7,7 +7,8 @@
     clippy::needless_lifetimes,
     clippy::missing_safety_doc,
     clippy::wrong_self_convention,
-    clippy::missing_panics_doc
+    clippy::missing_panics_doc,
+    clippy::return_self_not_must_use
 )]
 
 use std::ops::Mul;