Implement a ReturnVisitor and use in unnecessary_literal_bound

Author: GnomedDev

clippy_lints/src/unnecessary_literal_bound.rs

@@ -1,10 +1,10 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::path_res;
+use clippy_utils::{ReturnType, ReturnVisitor, path_res, visit_returns};
 use rustc_ast::ast::LitKind;
 use rustc_errors::Applicability;
 use rustc_hir::def::Res;
-use rustc_hir::intravisit::{FnKind, Visitor};
-use rustc_hir::{Body, Expr, ExprKind, FnDecl, FnRetTy, Lit, MutTy, Mutability, PrimTy, Ty, TyKind, intravisit};
+use rustc_hir::intravisit::FnKind;
+use rustc_hir::{Body, ExprKind, FnDecl, FnRetTy, Lit, MutTy, Mutability, PrimTy, Ty, TyKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::declare_lint_pass;
 use rustc_span::Span;
@@ -67,49 +67,36 @@ fn extract_anonymous_ref<'tcx>(hir_ty: &Ty<'tcx>) -> Option<&'tcx Ty<'tcx>> {
     Some(ty)
 }
 
-fn is_str_literal(expr: &Expr<'_>) -> bool {
-    matches!(
-        expr.kind,
-        ExprKind::Lit(Lit {
-            node: LitKind::Str(..),
-            ..
-        }),
-    )
-}
-
-struct FindNonLiteralReturn;
+struct LiteralReturnVisitor;
 
-impl<'hir> Visitor<'hir> for FindNonLiteralReturn {
+impl ReturnVisitor for LiteralReturnVisitor {
     type Result = std::ops::ControlFlow<()>;
-    type NestedFilter = intravisit::nested_filter::None;
+    fn visit_return(&mut self, kind: ReturnType<'_>) -> Self::Result {
+        let expr = match kind {
+            ReturnType::Implicit(e) | ReturnType::Explicit(e) => e,
+            ReturnType::UnitReturnExplicit(_) | ReturnType::MissingElseImplicit(_) => {
+                panic!("Function which returns `&str` has a unit return!")
+            },
+            ReturnType::DivergingImplicit(_) => {
+                // If this block is implicitly returning `!`, it can return `&'static str`.
+                return Self::Result::Continue(());
+            },
+        };
 
-    fn visit_expr(&mut self, expr: &'hir Expr<'hir>) -> Self::Result {
-        if let ExprKind::Ret(Some(ret_val_expr)) = expr.kind
-            && !is_str_literal(ret_val_expr)
-        {
-            Self::Result::Break(())
+        if matches!(
+            expr.kind,
+            ExprKind::Lit(Lit {
+                node: LitKind::Str(..),
+                ..
+            })
+        ) {
+            Self::Result::Continue(())
         } else {
-            intravisit::walk_expr(self, expr)
+            Self::Result::Break(())
         }
     }
 }
 
-fn check_implicit_returns_static_str(body: &Body<'_>) -> bool {
-    // TODO: Improve this to the same complexity as the Visitor to catch more implicit return cases.
-    if let ExprKind::Block(block, _) = body.value.kind
-        && let Some(implicit_ret) = block.expr
-    {
-        return is_str_literal(implicit_ret);
-    }
-
-    false
-}
-
-fn check_explicit_returns_static_str(expr: &Expr<'_>) -> bool {
-    let mut visitor = FindNonLiteralReturn;
-    visitor.visit_expr(expr).is_continue()
-}
-
 impl<'tcx> LateLintPass<'tcx> for UnnecessaryLiteralBound {
     fn check_fn(
         &mut self,
@@ -143,7 +130,7 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryLiteralBound {
         }
 
         // Check for all return statements returning literals
-        if check_explicit_returns_static_str(body.value) && check_implicit_returns_static_str(body) {
+        if visit_returns(LiteralReturnVisitor, body.value).is_continue() {
             span_lint_and_sugg(
                 cx,
                 UNNECESSARY_LITERAL_BOUND,

clippy_utils/src/lib.rs

@@ -10,6 +10,7 @@
 #![feature(assert_matches)]
 #![feature(unwrap_infallible)]
 #![feature(array_windows)]
+#![feature(associated_type_defaults)]
 #![recursion_limit = "512"]
 #![allow(
     clippy::missing_errors_doc,
@@ -30,6 +31,7 @@
 // FIXME: switch to something more ergonomic here, once available.
 // (Currently there is no way to opt into sysroot crates without `extern crate`.)
 extern crate rustc_ast;
+extern crate rustc_ast_ir;
 extern crate rustc_ast_pretty;
 extern crate rustc_attr;
 extern crate rustc_const_eval;
@@ -69,6 +71,7 @@ pub mod numeric_literal;
 pub mod paths;
 pub mod ptr;
 pub mod qualify_min_const_fn;
+mod returns;
 pub mod source;
 pub mod str_utils;
 pub mod sugg;
@@ -81,6 +84,7 @@ pub use self::check_proc_macro::{is_from_proc_macro, is_span_if, is_span_match};
 pub use self::hir_utils::{
     HirEqInterExpr, SpanlessEq, SpanlessHash, both, count_eq, eq_expr_value, hash_expr, hash_stmt, is_bool, over,
 };
+pub use returns::{ReturnType, ReturnVisitor, visit_returns};
 
 use core::mem;
 use core::ops::ControlFlow;

clippy_utils/src/returns.rs

@@ -0,0 +1,109 @@
+use std::ops::ControlFlow;
+
+use rustc_ast::visit::VisitorResult;
+use rustc_ast_ir::try_visit;
+use rustc_hir::intravisit::{self, Visitor};
+use rustc_hir::{Block, Expr, ExprKind};
+
+pub enum ReturnType<'tcx> {
+    /// An implicit return.
+    ///
+    /// This is an expression that evaluates directly to a value, like a literal or operation.
+    Implicit(&'tcx Expr<'tcx>),
+    /// An explicit return.
+    ///
+    /// This is the return expression of `return <expr>`.
+    Explicit(&'tcx Expr<'tcx>),
+    /// An explicit unit type return.
+    ///
+    /// This is the return expression `return`.
+    UnitReturnExplicit(&'tcx Expr<'tcx>),
+    /// A `()` implicit return.
+    ///
+    /// The expression is the `ExprKind::If` with no `else` block.
+    ///
+    /// ```no_run
+    /// fn example() -> () {
+    ///     if true {
+    ///
+    ///     } // no else!
+    /// }
+    /// ```
+    MissingElseImplicit(&'tcx Expr<'tcx>),
+    /// A diverging implict return.
+    ///
+    /// ```no_run
+    /// fn example() -> u8 {
+    ///     { todo!(); }
+    /// }
+    /// ```
+    DivergingImplicit(&'tcx Block<'tcx>),
+}
+
+pub trait ReturnVisitor {
+    type Result: VisitorResult = ();
+
+    fn visit_return(&mut self, return_type: ReturnType<'_>) -> Self::Result;
+}
+
+struct ExplicitReturnDriver<V>(V);
+
+impl<V: ReturnVisitor> Visitor<'_> for ExplicitReturnDriver<V> {
+    type Result = V::Result;
+    type NestedFilter = intravisit::nested_filter::None;
+
+    fn visit_expr(&mut self, expr: &Expr<'_>) -> Self::Result {
+        if let ExprKind::Ret(ret_val_expr) = expr.kind {
+            if let Some(ret_val_expr) = ret_val_expr {
+                try_visit!(self.0.visit_return(ReturnType::Explicit(ret_val_expr)));
+            } else {
+                try_visit!(self.0.visit_return(ReturnType::UnitReturnExplicit(expr)));
+            }
+        }
+
+        intravisit::walk_expr(self, expr)
+    }
+}
+
+fn visit_implicit_returns<V>(visitor: &mut V, expr: &Expr<'_>) -> V::Result
+where
+    V: ReturnVisitor,
+{
+    let cont = || V::Result::from_branch(ControlFlow::Continue(()));
+    match expr.kind {
+        ExprKind::Block(block, _) => {
+            if let Some(expr) = block.expr {
+                visit_implicit_returns(visitor, expr)
+            } else {
+                visitor.visit_return(ReturnType::DivergingImplicit(block))
+            }
+        },
+        ExprKind::If(_, true_block, else_block) => {
+            try_visit!(visit_implicit_returns(visitor, true_block));
+            if let Some(expr) = else_block {
+                visit_implicit_returns(visitor, expr)
+            } else {
+                visitor.visit_return(ReturnType::MissingElseImplicit(expr))
+            }
+        },
+        ExprKind::Match(_, arms, _) => {
+            for arm in arms {
+                try_visit!(visit_implicit_returns(visitor, arm.body));
+            }
+
+            cont()
+        },
+
+        _ => visitor.visit_return(ReturnType::Implicit(expr)),
+    }
+}
+
+pub fn visit_returns<V>(visitor: V, expr: &Expr<'_>) -> V::Result
+where
+    V: ReturnVisitor,
+{
+    let mut explicit_driver = ExplicitReturnDriver(visitor);
+    try_visit!(explicit_driver.visit_expr(expr));
+
+    visit_implicit_returns(&mut explicit_driver.0, expr)
+}

tests/compile-test.rs

@@ -584,7 +584,7 @@ impl LintMetadata {
         }
     }
 
-    fn applicability_str(&self) -> &str {
+    fn applicability_str(&self) -> &'static str {
         match self.applicability {
             Applicability::MachineApplicable => "MachineApplicable",
             Applicability::HasPlaceholders => "HasPlaceholders",

tests/ui/unnecessary_literal_bound.fixed

@@ -5,36 +5,33 @@ struct Struct<'a> {
 }
 
 impl Struct<'_> {
-    // Should warn
     fn returns_lit(&self) -> &'static str {
+        //~^ error: returning a `str` unnecessarily tied to the lifetime of arguments
         "Hello"
     }
 
-    // Should NOT warn
     fn returns_non_lit(&self) -> &str {
         self.not_literal
     }
 
-    // Should warn, does not currently
-    fn conditionally_returns_lit(&self, cond: bool) -> &str {
+    fn conditionally_returns_lit(&self, cond: bool) -> &'static str {
+        //~^ error: returning a `str` unnecessarily tied to the lifetime of arguments
         if cond { "Literal" } else { "also a literal" }
     }
 
-    // Should NOT warn
     fn conditionally_returns_non_lit(&self, cond: bool) -> &str {
         if cond { "Literal" } else { self.not_literal }
     }
 
-    // Should warn
     fn contionally_returns_literals_explicit(&self, cond: bool) -> &'static str {
+        //~^ error: returning a `str` unnecessarily tied to the lifetime of arguments
         if cond {
             return "Literal";
         }
 
         "also a literal"
     }
 
-    // Should NOT warn
     fn conditionally_returns_non_lit_explicit(&self, cond: bool) -> &str {
         if cond {
             return self.not_literal;
@@ -49,14 +46,13 @@ trait ReturnsStr {
 }
 
 impl ReturnsStr for u8 {
-    // Should warn, even though not useful without trait refinement
     fn trait_method(&self) -> &'static str {
+        //~^ error: returning a `str` unnecessarily tied to the lifetime of arguments
         "Literal"
     }
 }
 
 impl ReturnsStr for Struct<'_> {
-    // Should NOT warn
     fn trait_method(&self) -> &str {
         self.not_literal
     }

tests/ui/unnecessary_literal_bound.rs

@@ -5,36 +5,33 @@ struct Struct<'a> {
 }
 
 impl Struct<'_> {
-    // Should warn
     fn returns_lit(&self) -> &str {
+        //~^ error: returning a `str` unnecessarily tied to the lifetime of arguments
         "Hello"
     }
 
-    // Should NOT warn
     fn returns_non_lit(&self) -> &str {
         self.not_literal
     }
 
-    // Should warn, does not currently
     fn conditionally_returns_lit(&self, cond: bool) -> &str {
+        //~^ error: returning a `str` unnecessarily tied to the lifetime of arguments
         if cond { "Literal" } else { "also a literal" }
     }
 
-    // Should NOT warn
     fn conditionally_returns_non_lit(&self, cond: bool) -> &str {
         if cond { "Literal" } else { self.not_literal }
     }
 
-    // Should warn
     fn contionally_returns_literals_explicit(&self, cond: bool) -> &str {
+        //~^ error: returning a `str` unnecessarily tied to the lifetime of arguments
         if cond {
             return "Literal";
         }
 
         "also a literal"
     }
 
-    // Should NOT warn
     fn conditionally_returns_non_lit_explicit(&self, cond: bool) -> &str {
         if cond {
             return self.not_literal;
@@ -49,14 +46,13 @@ trait ReturnsStr {
 }
 
 impl ReturnsStr for u8 {
-    // Should warn, even though not useful without trait refinement
     fn trait_method(&self) -> &str {
+        //~^ error: returning a `str` unnecessarily tied to the lifetime of arguments
         "Literal"
     }
 }
 
 impl ReturnsStr for Struct<'_> {
-    // Should NOT warn
     fn trait_method(&self) -> &str {
         self.not_literal
     }

tests/ui/unnecessary_literal_bound.stderr

@@ -1,5 +1,5 @@
 error: returning a `str` unnecessarily tied to the lifetime of arguments
-  --> tests/ui/unnecessary_literal_bound.rs:9:30
+  --> tests/ui/unnecessary_literal_bound.rs:8:30
    |
 LL |     fn returns_lit(&self) -> &str {
    |                              ^^^^ help: try: `&'static str`
@@ -8,16 +8,22 @@ LL |     fn returns_lit(&self) -> &str {
    = help: to override `-D warnings` add `#[allow(clippy::unnecessary_literal_bound)]`
 
 error: returning a `str` unnecessarily tied to the lifetime of arguments
-  --> tests/ui/unnecessary_literal_bound.rs:29:68
+  --> tests/ui/unnecessary_literal_bound.rs:17:56
+   |
+LL |     fn conditionally_returns_lit(&self, cond: bool) -> &str {
+   |                                                        ^^^^ help: try: `&'static str`
+
+error: returning a `str` unnecessarily tied to the lifetime of arguments
+  --> tests/ui/unnecessary_literal_bound.rs:26:68
    |
 LL |     fn contionally_returns_literals_explicit(&self, cond: bool) -> &str {
    |                                                                    ^^^^ help: try: `&'static str`
 
 error: returning a `str` unnecessarily tied to the lifetime of arguments
-  --> tests/ui/unnecessary_literal_bound.rs:53:31
+  --> tests/ui/unnecessary_literal_bound.rs:49:31
    |
 LL |     fn trait_method(&self) -> &str {
    |                               ^^^^ help: try: `&'static str`
 
-error: aborting due to 3 previous errors
+error: aborting due to 4 previous errors