Add new lint to detect unnecessarily wrapped value

hkmatsumoto · hkmatsumoto · commit ef95ca10e0bf · 2020-10-18T18:02:30.000+09:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1994,6 +1994,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_wrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_wrap
 [`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
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -318,6 +318,7 @@ mod unicode;
 mod unit_return_expecting_ord;
 mod unnamed_address;
 mod unnecessary_sort_by;
+mod unnecessary_wrap;
 mod unnested_or_patterns;
 mod unsafe_removed_from_name;
 mod unused_io_amount;
@@ -869,6 +870,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_wrap::UNNECESSARY_WRAP,
         &unnested_or_patterns::UNNESTED_OR_PATTERNS,
         &unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME,
         &unused_io_amount::UNUSED_IO_AMOUNT,
@@ -1038,6 +1040,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_wrap::UnnecessaryWrap);
     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);
@@ -1526,6 +1529,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_wrap::UNNECESSARY_WRAP),
         LintId::of(&unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME),
         LintId::of(&unused_io_amount::UNUSED_IO_AMOUNT),
         LintId::of(&unused_unit::UNUSED_UNIT),
@@ -1721,6 +1725,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_wrap::UNNECESSARY_WRAP),
         LintId::of(&unwrap::UNNECESSARY_UNWRAP),
         LintId::of(&useless_conversion::USELESS_CONVERSION),
         LintId::of(&zero_div_zero::ZERO_DIVIDED_BY_ZERO),
diff --git a/clippy_lints/src/unnecessary_wrap.rs b/clippy_lints/src/unnecessary_wrap.rs
@@ -0,0 +1,177 @@
+use crate::utils::{
+    is_type_diagnostic_item, match_qpath, multispan_sugg_with_applicability, paths, return_ty, snippet,
+    span_lint_and_then,
+};
+use if_chain::if_chain;
+use rustc_errors::Applicability;
+use rustc_hir::intravisit::{FnKind, NestedVisitorMap, Visitor};
+use rustc_hir::*;
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::hir::map::Map;
+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 codes at callee side.
+    ///
+    /// **Example:**
+    ///
+    /// ```rust
+    /// pub 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
+    /// pub fn get_cool_number(a: bool, b: bool) -> i32 {
+    ///     if a && b {
+    ///         return 50;
+    ///     }
+    ///     if a {
+    ///         0
+    ///     } else {
+    ///         10
+    ///     }
+    /// }
+    /// ```
+    pub UNNECESSARY_WRAP,
+    complexity,
+    "functions that only return `Ok` or `Some`"
+}
+
+declare_lint_pass!(UnnecessaryWrap => [UNNECESSARY_WRAP]);
+
+impl<'tcx> LateLintPass<'tcx> for UnnecessaryWrap {
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'tcx>,
+        fn_kind: FnKind<'tcx>,
+        fn_decl: &FnDecl<'tcx>,
+        body: &Body<'tcx>,
+        span: Span,
+        hir_id: HirId,
+    ) {
+        if_chain! {
+            if let FnKind::ItemFn(.., visibility, _) = fn_kind;
+            if visibility.node.is_pub();
+            then {
+                return;
+            }
+        }
+
+        if let ExprKind::Block(ref block, ..) = body.value.kind {
+            let path = if is_type_diagnostic_item(cx, return_ty(cx, hir_id), sym!(option_type)) {
+                &paths::OPTION_SOME
+            } else if is_type_diagnostic_item(cx, return_ty(cx, hir_id), sym!(result_type)) {
+                &paths::RESULT_OK
+            } else {
+                return;
+            };
+
+            let mut visitor = UnnecessaryWrapVisitor { result: Vec::new() };
+            visitor.visit_block(block);
+            let result = visitor.result;
+
+            if result.iter().any(|expr| {
+                if_chain! {
+                    if let ExprKind::Call(ref func, ref args) = expr.kind;
+                    if let ExprKind::Path(ref qpath) = func.kind;
+                    if match_qpath(qpath, path);
+                    if args.len() == 1;
+                    then {
+                        false
+                    } else {
+                        true
+                    }
+                }
+            }) {
+                return;
+            }
+
+            let suggs = result.iter().filter_map(|expr| {
+                let snippet = if let ExprKind::Call(_, ref args) = expr.kind {
+                    Some(snippet(cx, args[0].span, "..").to_string())
+                } else {
+                    None
+                };
+                snippet.map(|snip| (expr.span, snip))
+            });
+
+            span_lint_and_then(
+                cx,
+                UNNECESSARY_WRAP,
+                span,
+                "this function returns unnecessarily wrapping data",
+                move |diag| {
+                    multispan_sugg_with_applicability(
+                        diag,
+                        "factor this out to",
+                        Applicability::MachineApplicable,
+                        suggs,
+                    );
+                },
+            );
+        }
+    }
+}
+
+struct UnnecessaryWrapVisitor<'tcx> {
+    result: Vec<&'tcx Expr<'tcx>>,
+}
+
+impl<'tcx> Visitor<'tcx> for UnnecessaryWrapVisitor<'tcx> {
+    type Map = Map<'tcx>;
+
+    fn visit_block(&mut self, block: &'tcx Block<'tcx>) {
+        for stmt in block.stmts {
+            self.visit_stmt(stmt);
+        }
+        if let Some(expr) = block.expr {
+            self.visit_expr(expr)
+        }
+    }
+
+    fn visit_stmt(&mut self, stmt: &'tcx Stmt<'tcx>) {
+        match stmt.kind {
+            StmtKind::Semi(ref expr) => {
+                if let ExprKind::Ret(Some(value)) = expr.kind {
+                    self.result.push(value);
+                }
+            },
+            StmtKind::Expr(ref expr) => self.visit_expr(expr),
+            _ => (),
+        }
+    }
+
+    fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
+        match expr.kind {
+            ExprKind::Ret(Some(value)) => self.result.push(value),
+            ExprKind::Call(..) | ExprKind::Path(..) => self.result.push(expr),
+            ExprKind::Block(ref block, _) | ExprKind::Loop(ref block, ..) => {
+                self.visit_block(block);
+            },
+            ExprKind::Match(_, arms, _) => {
+                for arm in arms {
+                    self.visit_expr(arm.body);
+                }
+            },
+            _ => intravisit::walk_expr(self, expr),
+        }
+    }
+
+    fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
+        NestedVisitorMap::None
+    }
+}
diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs
@@ -2517,6 +2517,13 @@ vec![
         deprecation: None,
         module: "unwrap",
     },
+    Lint {
+        name: "unnecessary_wrap",
+        group: "complexity",
+        desc: "functions that only return `Ok` or `Some`",
+        deprecation: None,
+        module: "unnecessary_wrap",
+    },
     Lint {
         name: "unneeded_field_pattern",
         group: "restriction",
diff --git a/tests/ui/unnecessary_wrap.fixed b/tests/ui/unnecessary_wrap.fixed
@@ -0,0 +1,47 @@
+// run-rustfix
+#![warn(clippy::unnecessary_wrap)]
+#![allow(clippy::no_effect)]
+#![allow(clippy::needless_return)]
+#![allow(clippy::if_same_then_else)]
+
+// should be linted
+fn func1(a: bool, b: bool) -> Option<i32> {
+    if a && b {
+        return Some(42);
+    }
+    if a {
+        Some(-1);
+        Some(2)
+    } else {
+        return Some(1337);
+    }
+}
+
+// public fns should not be linted
+pub fn func2(a: bool) -> Option<i32> {
+    if a {
+        Some(1)
+    } else {
+        Some(1)
+    }
+}
+
+// should not be linted
+fn func3(a: bool) -> Option<i32> {
+    if a {
+        Some(1)
+    } else {
+        None
+    }
+}
+
+// should be linted
+fn func4() -> Option<i32> {
+    1
+}
+
+fn main() {
+    // method calls are not linted
+    func1(true, true);
+    func2(true);
+}
diff --git a/tests/ui/unnecessary_wrap.rs b/tests/ui/unnecessary_wrap.rs
@@ -0,0 +1,47 @@
+// run-rustfix
+#![warn(clippy::unnecessary_wrap)]
+#![allow(clippy::no_effect)]
+#![allow(clippy::needless_return)]
+#![allow(clippy::if_same_then_else)]
+
+// should be linted
+fn func1(a: bool, b: bool) -> Option<i32> {
+    if a && b {
+        return Some(42);
+    }
+    if a {
+        Some(-1);
+        Some(2)
+    } else {
+        return Some(1337);
+    }
+}
+
+// public fns should not be linted
+pub fn func2(a: bool) -> Option<i32> {
+    if a {
+        Some(1)
+    } else {
+        Some(1)
+    }
+}
+
+// should not be linted
+fn func3(a: bool) -> Option<i32> {
+    if a {
+        Some(1)
+    } else {
+        None
+    }
+}
+
+// should be linted
+fn func4() -> Option<i32> {
+    Some(1)
+}
+
+fn main() {
+    // method calls are not linted
+    func1(true, true);
+    func2(true);
+}
diff --git a/tests/ui/unnecessary_wrap.stderr b/tests/ui/unnecessary_wrap.stderr
@@ -0,0 +1,34 @@
+error: this function unnecessarily wrapping data
+  --> $DIR/unnecessary_wrap.rs:8:1
+   |
+LL | / fn func1(a: bool, b: bool) -> Option<i32> {
+LL | |     if a && b {
+LL | |         return Some(42);
+LL | |     }
+...  |
+LL | |     }
+LL | | }
+   | |_^
+   |
+   = note: `-D clippy::unnecessary-wrap` implied by `-D warnings`
+help: factor this out to
+   |
+LL |         return 42;
+LL |     }
+LL |     if a {
+LL |         Some(-1);
+LL |         2
+LL |     } else {
+ ...
+
+error: this function unnecessarily wrapping data
+  --> $DIR/unnecessary_wrap.rs:39:1
+   |
+LL | / fn func4() -> Option<i32> {
+LL | |     Some(1)
+   | |     ------- help: factor this out to: `1`
+LL | | }
+   | |_^
+
+error: aborting due to 2 previous errors
+
