diff --git a/clippy_lints/src/unnecessary_literal_bound.rs b/clippy_lints/src/unnecessary_literal_bound.rs index 80ce67111261..348593c6deec 100644 --- a/clippy_lints/src/unnecessary_literal_bound.rs +++ b/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 { + 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, ); } } diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 18d8ea509577..a1f0f62bf098 100644 --- a/clippy_utils/src/lib.rs +++ b/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; diff --git a/clippy_utils/src/returns.rs b/clippy_utils/src/returns.rs new file mode 100644 index 000000000000..5205b3d24175 --- /dev/null +++ b/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 `. + 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); + +impl Visitor<'_> for ExplicitReturnDriver { + 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(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(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) +} diff --git a/tests/compile-test.rs b/tests/compile-test.rs index b8e0413e97bc..d620a2781e5a 100644 --- a/tests/compile-test.rs +++ b/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", diff --git a/tests/ui/unnecessary_literal_bound.fixed b/tests/ui/unnecessary_literal_bound.fixed index 107e397466d0..72bdd92adeff 100644 --- a/tests/ui/unnecessary_literal_bound.fixed +++ b/tests/ui/unnecessary_literal_bound.fixed @@ -2,31 +2,44 @@ 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"; } @@ -34,7 +47,6 @@ impl Struct<'_> { "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 } diff --git a/tests/ui/unnecessary_literal_bound.rs b/tests/ui/unnecessary_literal_bound.rs index b371ff9d3a2e..060d32fb9f26 100644 --- a/tests/ui/unnecessary_literal_bound.rs +++ b/tests/ui/unnecessary_literal_bound.rs @@ -2,31 +2,44 @@ struct Struct<'a> { not_literal: &'a str, + non_literal_b: &'a [u8], } impl Struct<'_> { - // Should warn fn returns_lit(&self) -> &str { + //~^ error: returning a literal unnecessarily tied to the lifetime of arguments "Hello" } - // Should NOT warn + fn returns_lit_b(&self) -> &[u8] { + //~^ error: returning a literal unnecessarily tied to the lifetime of arguments + &[0, 1, 2] + } + + fn returns_lit_b_fixed(&self) -> &[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 returns_non_lit_b(&self) -> &[u8] { + self.non_literal_b + } + fn conditionally_returns_lit(&self, cond: bool) -> &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) -> &str { + //~^ error: returning a literal unnecessarily tied to the lifetime of arguments if cond { return "Literal"; } @@ -34,7 +47,6 @@ impl Struct<'_> { "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) -> &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 } diff --git a/tests/ui/unnecessary_literal_bound.stderr b/tests/ui/unnecessary_literal_bound.stderr index 512b2f9a0afa..019da0a4b5da 100644 --- a/tests/ui/unnecessary_literal_bound.stderr +++ b/tests/ui/unnecessary_literal_bound.stderr @@ -1,4 +1,4 @@ -error: returning a `str` unnecessarily tied to the lifetime of arguments +error: returning a literal unnecessarily tied to the lifetime of arguments --> tests/ui/unnecessary_literal_bound.rs:9:30 | LL | fn returns_lit(&self) -> &str { @@ -7,17 +7,35 @@ LL | fn returns_lit(&self) -> &str { = note: `-D clippy::unnecessary-literal-bound` implied by `-D warnings` = 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 +error: returning a literal unnecessarily tied to the lifetime of arguments + --> tests/ui/unnecessary_literal_bound.rs:14:32 + | +LL | fn returns_lit_b(&self) -> &[u8] { + | ^^^^^ help: try: `&'static [u8]` + +error: returning a literal unnecessarily tied to the lifetime of arguments + --> tests/ui/unnecessary_literal_bound.rs:19:38 + | +LL | fn returns_lit_b_fixed(&self) -> &[u8; 3] { + | ^^^^^^^^ help: try: `&'static [u8; 3]` + +error: returning a literal unnecessarily tied to the lifetime of arguments + --> tests/ui/unnecessary_literal_bound.rs:32:56 + | +LL | fn conditionally_returns_lit(&self, cond: bool) -> &str { + | ^^^^ help: try: `&'static str` + +error: returning a literal unnecessarily tied to the lifetime of arguments + --> tests/ui/unnecessary_literal_bound.rs:41: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 +error: returning a literal unnecessarily tied to the lifetime of arguments + --> tests/ui/unnecessary_literal_bound.rs:64:31 | LL | fn trait_method(&self) -> &str { | ^^^^ help: try: `&'static str` -error: aborting due to 3 previous errors +error: aborting due to 6 previous errors