Auto merge of #6070 - matsujika:unnecessary_wrap, r=flip1995

Add new lint `unnecessary_wrap`

Fixes #5969

changelog: New lint [`unnecessary_wraps`]

Author: bors

CHANGELOG.md

@@ -2008,6 +2008,7 @@ Released 2018-09-13
 [`unnecessary_operation`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_operation
 [`unnecessary_sort_by`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_sort_by
 [`unnecessary_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_unwrap
+[`unnecessary_wraps`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_wraps
 [`unneeded_field_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#unneeded_field_pattern
 [`unneeded_wildcard_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#unneeded_wildcard_pattern
 [`unnested_or_patterns`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnested_or_patterns

clippy_lints/src/lib.rs

@@ -323,6 +323,7 @@ mod unicode;
 mod unit_return_expecting_ord;
 mod unnamed_address;
 mod unnecessary_sort_by;
+mod unnecessary_wraps;
 mod unnested_or_patterns;
 mod unsafe_removed_from_name;
 mod unused_io_amount;
@@ -892,6 +893,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &unnamed_address::FN_ADDRESS_COMPARISONS,
         &unnamed_address::VTABLE_ADDRESS_COMPARISONS,
         &unnecessary_sort_by::UNNECESSARY_SORT_BY,
+        &unnecessary_wraps::UNNECESSARY_WRAPS,
         &unnested_or_patterns::UNNESTED_OR_PATTERNS,
         &unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME,
         &unused_io_amount::UNUSED_IO_AMOUNT,
@@ -1064,6 +1066,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| box redundant_clone::RedundantClone);
     store.register_late_pass(|| box slow_vector_initialization::SlowVectorInit);
     store.register_late_pass(|| box unnecessary_sort_by::UnnecessarySortBy);
+    store.register_late_pass(|| box unnecessary_wraps::UnnecessaryWraps);
     store.register_late_pass(|| box types::RefToMut);
     store.register_late_pass(|| box assertions_on_constants::AssertionsOnConstants);
     store.register_late_pass(|| box missing_const_for_fn::MissingConstForFn);
@@ -1571,6 +1574,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&unnamed_address::FN_ADDRESS_COMPARISONS),
         LintId::of(&unnamed_address::VTABLE_ADDRESS_COMPARISONS),
         LintId::of(&unnecessary_sort_by::UNNECESSARY_SORT_BY),
+        LintId::of(&unnecessary_wraps::UNNECESSARY_WRAPS),
         LintId::of(&unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME),
         LintId::of(&unused_io_amount::UNUSED_IO_AMOUNT),
         LintId::of(&unused_unit::UNUSED_UNIT),
@@ -1775,6 +1779,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&types::UNNECESSARY_CAST),
         LintId::of(&types::VEC_BOX),
         LintId::of(&unnecessary_sort_by::UNNECESSARY_SORT_BY),
+        LintId::of(&unnecessary_wraps::UNNECESSARY_WRAPS),
         LintId::of(&unwrap::UNNECESSARY_UNWRAP),
         LintId::of(&useless_conversion::USELESS_CONVERSION),
         LintId::of(&zero_div_zero::ZERO_DIVIDED_BY_ZERO),

clippy_lints/src/methods/bind_instead_of_map.rs

@@ -1,14 +1,12 @@
 use super::{contains_return, BIND_INSTEAD_OF_MAP};
 use crate::utils::{
     in_macro, match_qpath, match_type, method_calls, multispan_sugg_with_applicability, paths, remove_blocks, snippet,
-    snippet_with_macro_callsite, span_lint_and_sugg, span_lint_and_then,
+    snippet_with_macro_callsite, span_lint_and_sugg, span_lint_and_then, visitors::find_all_ret_expressions,
 };
 use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir as hir;
-use rustc_hir::intravisit::{self, Visitor};
 use rustc_lint::LateContext;
-use rustc_middle::hir::map::Map;
 use rustc_span::Span;
 
 pub(crate) struct OptionAndThenSome;
@@ -193,124 +191,3 @@ pub(crate) trait BindInsteadOfMap {
         }
     }
 }
-
-/// returns `true` if expr contains match expr desugared from try
-fn contains_try(expr: &hir::Expr<'_>) -> bool {
-    struct TryFinder {
-        found: bool,
-    }
-
-    impl<'hir> intravisit::Visitor<'hir> for TryFinder {
-        type Map = Map<'hir>;
-
-        fn nested_visit_map(&mut self) -> intravisit::NestedVisitorMap<Self::Map> {
-            intravisit::NestedVisitorMap::None
-        }
-
-        fn visit_expr(&mut self, expr: &'hir hir::Expr<'hir>) {
-            if self.found {
-                return;
-            }
-            match expr.kind {
-                hir::ExprKind::Match(_, _, hir::MatchSource::TryDesugar) => self.found = true,
-                _ => intravisit::walk_expr(self, expr),
-            }
-        }
-    }
-
-    let mut visitor = TryFinder { found: false };
-    visitor.visit_expr(expr);
-    visitor.found
-}
-
-fn find_all_ret_expressions<'hir, F>(_cx: &LateContext<'_>, expr: &'hir hir::Expr<'hir>, callback: F) -> bool
-where
-    F: FnMut(&'hir hir::Expr<'hir>) -> bool,
-{
-    struct RetFinder<F> {
-        in_stmt: bool,
-        failed: bool,
-        cb: F,
-    }
-
-    struct WithStmtGuarg<'a, F> {
-        val: &'a mut RetFinder<F>,
-        prev_in_stmt: bool,
-    }
-
-    impl<F> RetFinder<F> {
-        fn inside_stmt(&mut self, in_stmt: bool) -> WithStmtGuarg<'_, F> {
-            let prev_in_stmt = std::mem::replace(&mut self.in_stmt, in_stmt);
-            WithStmtGuarg {
-                val: self,
-                prev_in_stmt,
-            }
-        }
-    }
-
-    impl<F> std::ops::Deref for WithStmtGuarg<'_, F> {
-        type Target = RetFinder<F>;
-
-        fn deref(&self) -> &Self::Target {
-            self.val
-        }
-    }
-
-    impl<F> std::ops::DerefMut for WithStmtGuarg<'_, F> {
-        fn deref_mut(&mut self) -> &mut Self::Target {
-            self.val
-        }
-    }
-
-    impl<F> Drop for WithStmtGuarg<'_, F> {
-        fn drop(&mut self) {
-            self.val.in_stmt = self.prev_in_stmt;
-        }
-    }
-
-    impl<'hir, F: FnMut(&'hir hir::Expr<'hir>) -> bool> intravisit::Visitor<'hir> for RetFinder<F> {
-        type Map = Map<'hir>;
-
-        fn nested_visit_map(&mut self) -> intravisit::NestedVisitorMap<Self::Map> {
-            intravisit::NestedVisitorMap::None
-        }
-
-        fn visit_stmt(&mut self, stmt: &'hir hir::Stmt<'_>) {
-            intravisit::walk_stmt(&mut *self.inside_stmt(true), stmt)
-        }
-
-        fn visit_expr(&mut self, expr: &'hir hir::Expr<'_>) {
-            if self.failed {
-                return;
-            }
-            if self.in_stmt {
-                match expr.kind {
-                    hir::ExprKind::Ret(Some(expr)) => self.inside_stmt(false).visit_expr(expr),
-                    _ => intravisit::walk_expr(self, expr),
-                }
-            } else {
-                match expr.kind {
-                    hir::ExprKind::Match(cond, arms, _) => {
-                        self.inside_stmt(true).visit_expr(cond);
-                        for arm in arms {
-                            self.visit_expr(arm.body);
-                        }
-                    },
-                    hir::ExprKind::Block(..) => intravisit::walk_expr(self, expr),
-                    hir::ExprKind::Ret(Some(expr)) => self.visit_expr(expr),
-                    _ => self.failed |= !(self.cb)(expr),
-                }
-            }
-        }
-    }
-
-    !contains_try(expr) && {
-        let mut ret_finder = RetFinder {
-            in_stmt: false,
-            failed: false,
-            cb: callback,
-        };
-        ret_finder.visit_expr(expr);
-        !ret_finder.failed
-    }
-}

clippy_lints/src/redundant_clone.rs

@@ -320,11 +320,11 @@ fn find_stmt_assigns_to<'tcx>(
 
     match (by_ref, &*rvalue) {
         (true, mir::Rvalue::Ref(_, _, place)) | (false, mir::Rvalue::Use(mir::Operand::Copy(place))) => {
-            base_local_and_movability(cx, mir, *place)
+            Some(base_local_and_movability(cx, mir, *place))
         },
         (false, mir::Rvalue::Ref(_, _, place)) => {
             if let [mir::ProjectionElem::Deref] = place.as_ref().projection {
-                base_local_and_movability(cx, mir, *place)
+                Some(base_local_and_movability(cx, mir, *place))
             } else {
                 None
             }
@@ -341,7 +341,7 @@ fn base_local_and_movability<'tcx>(
     cx: &LateContext<'tcx>,
     mir: &mir::Body<'tcx>,
     place: mir::Place<'tcx>,
-) -> Option<(mir::Local, CannotMoveOut)> {
+) -> (mir::Local, CannotMoveOut) {
     use rustc_middle::mir::PlaceRef;
 
     // Dereference. You cannot move things out from a borrowed value.
@@ -362,7 +362,7 @@ fn base_local_and_movability<'tcx>(
             && !is_copy(cx, mir::Place::ty_from(local, projection, &mir.local_decls, cx.tcx).ty);
     }
 
-    Some((local, deref || field || slice))
+    (local, deref || field || slice)
 }
 
 struct LocalUseVisitor {

clippy_lints/src/unnecessary_wraps.rs

@@ -0,0 +1,143 @@
+use crate::utils::{
+    in_macro, is_type_diagnostic_item, match_qpath, paths, return_ty, snippet, span_lint_and_then,
+    visitors::find_all_ret_expressions,
+};
+use if_chain::if_chain;
+use rustc_errors::Applicability;
+use rustc_hir::intravisit::FnKind;
+use rustc_hir::{Body, ExprKind, FnDecl, HirId, ItemKind, Node};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty::subst::GenericArgKind;
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::Span;
+
+declare_clippy_lint! {
+    /// **What it does:** Checks for private functions that only return `Ok` or `Some`.
+    ///
+    /// **Why is this bad?** It is not meaningful to wrap values when no `None` or `Err` is returned.
+    ///
+    /// **Known problems:** Since this lint changes function type signature, you may need to
+    /// adjust some code at callee side.
+    ///
+    /// **Example:**
+    ///
+    /// ```rust
+    /// fn get_cool_number(a: bool, b: bool) -> Option<i32> {
+    ///     if a && b {
+    ///         return Some(50);
+    ///     }
+    ///     if a {
+    ///         Some(0)
+    ///     } else {
+    ///         Some(10)
+    ///     }
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// fn get_cool_number(a: bool, b: bool) -> i32 {
+    ///     if a && b {
+    ///         return 50;
+    ///     }
+    ///     if a {
+    ///         0
+    ///     } else {
+    ///         10
+    ///     }
+    /// }
+    /// ```
+    pub UNNECESSARY_WRAPS,
+    complexity,
+    "functions that only return `Ok` or `Some`"
+}
+
+declare_lint_pass!(UnnecessaryWraps => [UNNECESSARY_WRAPS]);
+
+impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps {
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'tcx>,
+        fn_kind: FnKind<'tcx>,
+        fn_decl: &FnDecl<'tcx>,
+        body: &Body<'tcx>,
+        span: Span,
+        hir_id: HirId,
+    ) {
+        match fn_kind {
+            FnKind::ItemFn(.., visibility, _) | FnKind::Method(.., Some(visibility), _) => {
+                if visibility.node.is_pub() {
+                    return;
+                }
+            },
+            FnKind::Closure(..) => return,
+            _ => (),
+        }
+
+        if let Some(Node::Item(item)) = cx.tcx.hir().find(cx.tcx.hir().get_parent_node(hir_id)) {
+            if matches!(item.kind, ItemKind::Impl{ of_trait: Some(_), ..} | ItemKind::Trait(..)) {
+                return;
+            }
+        }
+
+        let (return_type, path) = if is_type_diagnostic_item(cx, return_ty(cx, hir_id), sym!(option_type)) {
+            ("Option", &paths::OPTION_SOME)
+        } else if is_type_diagnostic_item(cx, return_ty(cx, hir_id), sym!(result_type)) {
+            ("Result", &paths::RESULT_OK)
+        } else {
+            return;
+        };
+
+        let mut suggs = Vec::new();
+        let can_sugg = find_all_ret_expressions(cx, &body.value, |ret_expr| {
+            if_chain! {
+                if !in_macro(ret_expr.span);
+                if let ExprKind::Call(ref func, ref args) = ret_expr.kind;
+                if let ExprKind::Path(ref qpath) = func.kind;
+                if match_qpath(qpath, path);
+                if args.len() == 1;
+                then {
+                    suggs.push((ret_expr.span, snippet(cx, args[0].span.source_callsite(), "..").to_string()));
+                    true
+                } else {
+                    false
+                }
+            }
+        });
+
+        if can_sugg && !suggs.is_empty() {
+            span_lint_and_then(
+                cx,
+                UNNECESSARY_WRAPS,
+                span,
+                format!(
+                    "this function's return value is unnecessarily wrapped by `{}`",
+                    return_type
+                )
+                .as_str(),
+                |diag| {
+                    let inner_ty = return_ty(cx, hir_id)
+                        .walk()
+                        .skip(1) // skip `std::option::Option` or `std::result::Result`
+                        .take(1) // take the first outermost inner type
+                        .filter_map(|inner| match inner.unpack() {
+                            GenericArgKind::Type(inner_ty) => Some(inner_ty.to_string()),
+                            _ => None,
+                        });
+                    inner_ty.for_each(|inner_ty| {
+                        diag.span_suggestion(
+                            fn_decl.output.span(),
+                            format!("remove `{}` from the return type...", return_type).as_str(),
+                            inner_ty,
+                            Applicability::MaybeIncorrect,
+                        );
+                    });
+                    diag.multipart_suggestion(
+                        "...and change the returning expressions",
+                        suggs,
+                        Applicability::MachineApplicable,
+                    );
+                },
+            );
+        }
+    }
+}

clippy_lints/src/utils/mod.rs

@@ -21,6 +21,7 @@ pub mod ptr;
 pub mod qualify_min_const_fn;
 pub mod sugg;
 pub mod usage;
+pub mod visitors;
 
 pub use self::attrs::*;
 pub use self::diagnostics::*;