Implement a ReturnVisitor and use in unnecessary_literal_bound

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

@@ -9,6 +9,7 @@
 #![feature(rustc_private)]
 #![feature(assert_matches)]
 #![feature(unwrap_infallible)]
+#![feature(associated_type_defaults)]
 #![recursion_limit = "512"]
 #![allow(
  clippy::missing_errors_doc,
@@ -29,6 +30,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;
@@ -68,6 +70,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;
@@ -80,6 +83,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