Expand unnecessary_literal_bound to literal arrays and slices

clippy_lints/src/unnecessary_literal_bound.rs

@@ -1,10 +1,12 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::path_res;
+use clippy_utils::source::snippet_with_applicability;
+use clippy_utils::{ReturnType, ReturnVisitor, visit_returns};
+use rustc_ast::BorrowKind;
 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, Expr, ExprKind, FnDecl, FnRetTy, Lit, MutTy, Mutability, PrimTy, Ty, TyKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::declare_lint_pass;
 use rustc_span::Span;
@@ -13,11 +15,11 @@ use rustc_span::def_id::LocalDefId;
 declare_clippy_lint! {
     /// ### What it does
     ///
-    /// Detects functions that are written to return `&str` that could return `&'static str` but instead return a `&'a str`.
+    /// Detects functions that are written to return a reference to a literal that could return a static reference but instead return a lifetime-bounded reference.
     ///
     /// ### Why is this bad?
     ///
-    /// This leaves the caller unable to use the `&str` as `&'static str`, causing unneccessary allocations or confusion.
+    /// This leaves the caller unable to use the reference as `'static`, causing unneccessary allocations or confusion.
     /// This is also most likely what you meant to write.
     ///
     /// ### Example
@@ -67,49 +69,70 @@ 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(..),
-            ..
-        }),
-    )
+enum ReturnTy {
+    Str,
+    Slice,
+    Array,
 }
 
-struct FindNonLiteralReturn;
+fn fetch_return_mode(cx: &LateContext<'_>, hir_ty: &Ty<'_>) -> Option<ReturnTy> {
+    match &hir_ty.kind {
+        TyKind::Array(_, _) => Some(ReturnTy::Array),
+        TyKind::Slice(_) => Some(ReturnTy::Slice),
+        TyKind::Path(other) => {
+            if let Res::PrimTy(PrimTy::Str) = cx.qpath_res(other, hir_ty.hir_id) {
+                Some(ReturnTy::Str)
+            } else {
+                None
+            }
+        },
+        _ => None,
+    }
+}
 
-impl<'hir> Visitor<'hir> for FindNonLiteralReturn {
+struct LiteralReturnVisitor {
+    return_mode: ReturnTy,
+}
+
+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 a type has a unit return!")
+            },
+            ReturnType::DivergingImplicit(_) => {
+                // If this block is implicitly returning `!`, it can return `&'static T`.
+                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(())
+        let returns_literal = match self.return_mode {
+            ReturnTy::Str => matches!(
+                expr.kind,
+                ExprKind::Lit(Lit {
+                    node: LitKind::Str(..),
+                    ..
+                })
+            ),
+            ReturnTy::Slice | ReturnTy::Array => matches!(
+                expr.kind,
+                ExprKind::AddrOf(BorrowKind::Ref, Mutability::Not, Expr {
+                    kind: ExprKind::Array(_),
+                    ..
+                })
+            ),
+        };
+
+        if returns_literal {
+            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,
@@ -129,7 +152,7 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryLiteralBound {
             return;
         }
 
-        // Check for `-> &str`
+        // Check for `-> &str/&[T]/&[T; N]`
         let FnRetTy::Return(ret_hir_ty) = decl.output else {
             return;
         };
@@ -138,20 +161,22 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryLiteralBound {
             return;
         };
 
-        if path_res(cx, inner_hir_ty) != Res::PrimTy(PrimTy::Str) {
+        let Some(return_mode) = fetch_return_mode(cx, inner_hir_ty) else {
             return;
-        }
+        };
 
         // 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 { return_mode }, body.value).is_continue() {
+            let mut applicability = Applicability::MachineApplicable;
+            let snippet = snippet_with_applicability(cx, inner_hir_ty.span, "..", &mut applicability);
             span_lint_and_sugg(
                 cx,
                 UNNECESSARY_LITERAL_BOUND,
                 ret_hir_ty.span,
-                "returning a `str` unnecessarily tied to the lifetime of arguments",
+                "returning a literal unnecessarily tied to the lifetime of arguments",
                 "try",
-                "&'static str".into(), // how ironic, a lint about `&'static str` requiring a `String` alloc...
-                Applicability::MachineApplicable,
+                format!("&'static {snippet}"),
+                applicability,
             );
         }
     }

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

@@ -2,39 +2,51 @@
 
 struct Struct<'a> {
     not_literal: &'a str,
+    non_literal_b: &'a [u8],
 }
 
 impl Struct<'_> {
-    // Should warn
     fn returns_lit(&self) -> &'static str {
+        //~^ error: returning a literal unnecessarily tied to the lifetime of arguments
         "Hello"
     }
 
-    // Should NOT warn
+    fn returns_lit_b(&self) -> &'static [u8] {
+        //~^ error: returning a literal unnecessarily tied to the lifetime of arguments
+        &[0, 1, 2]
+    }
+
+    fn returns_lit_b_fixed(&self) -> &'static [u8; 3] {
+        //~^ error: returning a literal unnecessarily tied to the lifetime of arguments
+        &[0, 1, 2]
+    }
+
     fn returns_non_lit(&self) -> &str {
         self.not_literal
     }
 
-    // Should warn, does not currently
-    fn conditionally_returns_lit(&self, cond: bool) -> &str {
+    fn returns_non_lit_b(&self) -> &[u8] {
+        self.non_literal_b
+    }
+
+    fn conditionally_returns_lit(&self, cond: bool) -> &'static str {
+        //~^ error: returning a literal 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 literal 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 +61,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 literal unnecessarily tied to the lifetime of arguments
         "Literal"
     }
 }
 
 impl ReturnsStr for Struct<'_> {
-    // Should NOT warn
     fn trait_method(&self) -> &str {
         self.not_literal
     }