Add lint for unnecessary lifetime bounded &str return

Author: GnomedDev

CHANGELOG.md

@@ -6029,6 +6029,7 @@ Released 2018-09-13
 [`unnecessary_get_then_check`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_get_then_check
 [`unnecessary_join`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_join
 [`unnecessary_lazy_evaluations`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_lazy_evaluations
+[`unnecessary_literal_bound`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_literal_bound
 [`unnecessary_literal_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_literal_unwrap
 [`unnecessary_map_on_constructor`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_map_on_constructor
 [`unnecessary_min_or_max`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_min_or_max

clippy_lints/src/declared_lints.rs

@@ -738,6 +738,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
 crate::unit_types::UNIT_CMP_INFO,
 crate::unnamed_address::FN_ADDRESS_COMPARISONS_INFO,
 crate::unnecessary_box_returns::UNNECESSARY_BOX_RETURNS_INFO,
+crate::unnecessary_literal_bound::UNNECESSARY_LITERAL_BOUND_INFO,
 crate::unnecessary_map_on_constructor::UNNECESSARY_MAP_ON_CONSTRUCTOR_INFO,
 crate::unnecessary_owned_empty_strings::UNNECESSARY_OWNED_EMPTY_STRINGS_INFO,
 crate::unnecessary_self_imports::UNNECESSARY_SELF_IMPORTS_INFO,

clippy_lints/src/lib.rs

@@ -366,6 +366,7 @@ mod unit_return_expecting_ord;
 mod unit_types;
 mod unnamed_address;
 mod unnecessary_box_returns;
+mod unnecessary_literal_bound;
 mod unnecessary_map_on_constructor;
 mod unnecessary_owned_empty_strings;
 mod unnecessary_self_imports;
@@ -949,5 +950,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
  store.register_late_pass(|_| Box::new(non_zero_suggestions::NonZeroSuggestions));
  store.register_late_pass(move |_| Box::new(unused_trait_names::UnusedTraitNames::new(conf)));
  store.register_late_pass(|_| Box::new(manual_ignore_case_cmp::ManualIgnoreCaseCmp));
+ store.register_late_pass(|_| Box::new(unnecessary_literal_bound::UnnecessaryLiteralBound));
 // add lints here, do not remove this comment, it's used in `new_lint`
 }

clippy_lints/src/unnecessary_literal_bound.rs

@@ -0,0 +1,156 @@
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use rustc_ast::ast::LitKind;
+use rustc_errors::Applicability;
+use rustc_hir::intravisit::{FnKind,Visitor};
+use rustc_hir::{Body,Expr,ExprKind,FnDecl,FnRetTy,Lit,MutTy,Mutability,Ty,TyKind, intravisit};
+use rustc_lint::{LateContext,LateLintPass};
+use rustc_session::declare_lint_pass;
+use rustc_span::Span;
+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`.
+///
+/// ### Why is this bad?
+///
+/// This leaves the caller unable to use the `&str` as `&'static str`, causing unneccessary allocations or confusion.
+/// This is also most likely what you meant to write.
+///
+/// ### Example
+/// ```no_run
+/// # struct MyType;
+/// impl MyType {
+/// fn returns_literal(&self) -> &str {
+/// "Literal"
+/// }
+/// }
+/// ```
+/// Use instead:
+/// ```no_run
+/// # struct MyType;
+/// impl MyType {
+/// fn returns_literal(&self) -> &'static str {
+/// "Literal"
+/// }
+/// }
+/// ```
+/// Or, in case you may return a non-literal `str` in future:
+/// ```no_run
+/// # struct MyType;
+/// impl MyType {
+/// fn returns_literal<'a>(&'a self) -> &'a str {
+/// "Literal"
+/// }
+/// }
+/// ```
+ #[clippy::version = "1.83.0"]
+pubUNNECESSARY_LITERAL_BOUND,
+ pedantic,
+"detects &str that could be &'static str in function return types"
+}
+
+declare_lint_pass!(UnnecessaryLiteralBound => [UNNECESSARY_LITERAL_BOUND]);
+
+fnextract_anonymous_ref<'tcx>(hir_ty:&Ty<'tcx>) -> Option<&'tcxTy<'tcx>>{
+letTyKind::Ref(lifetime,MutTy{ ty, mutbl }) = hir_ty.kindelse{
+returnNone;
+};
+
+if !lifetime.is_anonymous() || !matches!(mutbl,Mutability::Not){
+returnNone;
+}
+
+Some(ty)
+}
+
+fnis_str_literal(expr:&Expr<'_>) -> bool{
+matches!(
+ expr.kind,
+ExprKind::Lit(Lit{
+ node:LitKind::Str(..),
+ ..
+}),
+)
+}
+
+structFindNonLiteralReturn;
+
+impl<'hir>Visitor<'hir>forFindNonLiteralReturn{
+typeResult = std::ops::ControlFlow<()>;
+typeNestedFilter = intravisit::nested_filter::None;
+
+fnvisit_expr(&mutself,expr:&'hirExpr<'hir>) -> Self::Result{
+ifletExprKind::Ret(Some(ret_val_expr)) = expr.kind
+ && !is_str_literal(ret_val_expr)
+{
+Self::Result::Break(())
+}else{
+ intravisit::walk_expr(self, expr)
+}
+}
+}
+
+fncheck_implicit_returns_static_str(body:&Body<'_>) -> bool{
+// TODO: Improve this to the same complexity as the Visitor to catch more implicit return cases.
+ifletExprKind::Block(block, _) = body.value.kind
+ && letSome(implicit_ret) = block.expr
+{
+returnis_str_literal(implicit_ret);
+}
+
+false
+}
+
+fncheck_explicit_returns_static_str(expr:&Expr<'_>) -> bool{
+letmut visitor = FindNonLiteralReturn;
+ visitor.visit_expr(expr).is_continue()
+}
+
+impl<'tcx>LateLintPass<'tcx>forUnnecessaryLiteralBound{
+fncheck_fn(
+&mutself,
+cx:&LateContext<'tcx>,
+kind:FnKind<'tcx>,
+decl:&'tcxFnDecl<'_>,
+body:&'tcxBody<'_>,
+span:Span,
+ _:LocalDefId,
+){
+if span.from_expansion(){
+return;
+}
+
+// Checking closures would be a little silly
+ifmatches!(kind,FnKind::Closure){
+return;
+}
+
+// Check for `-> &str`
+letFnRetTy::Return(ret_hir_ty) = decl.outputelse{
+return;
+};
+
+letSome(inner_hir_ty) = extract_anonymous_ref(ret_hir_ty)else{
+return;
+};
+
+if !rustc_hir_analysis::lower_ty(cx.tcx, inner_hir_ty).is_str(){
+return;
+}
+
+// Check for all return statements returning literals
+ifcheck_explicit_returns_static_str(body.value) && check_implicit_returns_static_str(body){
+span_lint_and_sugg(
+ cx,
+UNNECESSARY_LITERAL_BOUND,
+ ret_hir_ty.span,
+"returning a `str` unnecessarily tied to the lifetime of arguments",
+"try",
+"&'static str".into(),// how ironic, a lint about `&'static str` requiring a `String` alloc...
+Applicability::MachineApplicable,
+);
+}
+}
+}

tests/ui/unnecessary_literal_bound.fixed

@@ -0,0 +1,65 @@
+#![warn(clippy::unnecessary_literal_bound)]
+
+struct Struct<'a> {
+ not_literal: &'a str,
+}
+
+impl Struct<'_> {
+ // Should warn
+ fn returns_lit(&self) -> &'static str {
+ "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 {
+ 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 {
+ 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;
+ }
+
+ "Literal"
+ }
+}
+
+trait ReturnsStr {
+ fn trait_method(&self) -> &str;
+}
+
+impl ReturnsStr for u8 {
+ // Should warn, even though not useful without trait refinement
+ fn trait_method(&self) -> &'static str {
+ "Literal"
+ }
+}
+
+impl ReturnsStr for Struct<'_> {
+ // Should NOT warn
+ fn trait_method(&self) -> &str {
+ self.not_literal
+ }
+}
+
+fn main() {}

tests/ui/unnecessary_literal_bound.rs

@@ -0,0 +1,65 @@
+#![warn(clippy::unnecessary_literal_bound)]
+
+structStruct<'a>{
+not_literal:&'astr,
+}
+
+implStruct<'_>{
+// Should warn
+fnreturns_lit(&self) -> &str{
+"Hello"
+}
+
+// Should NOT warn
+fnreturns_non_lit(&self) -> &str{
+self.not_literal
+}
+
+// Should warn, does not currently
+fnconditionally_returns_lit(&self,cond:bool) -> &str{
+if cond {"Literal"}else{"also a literal"}
+}
+
+// Should NOT warn
+fnconditionally_returns_non_lit(&self,cond:bool) -> &str{
+if cond {"Literal"}else{self.not_literal}
+}
+
+// Should warn
+fncontionally_returns_literals_explicit(&self,cond:bool) -> &str{
+if cond {
+return"Literal";
+}
+
+"also a literal"
+}
+
+// Should NOT warn
+fnconditionally_returns_non_lit_explicit(&self,cond:bool) -> &str{
+if cond {
+returnself.not_literal;
+}
+
+"Literal"
+}
+}
+
+traitReturnsStr{
+fntrait_method(&self) -> &str;
+}
+
+implReturnsStrforu8{
+// Should warn, even though not useful without trait refinement
+fntrait_method(&self) -> &str{
+"Literal"
+}
+}
+
+implReturnsStrforStruct<'_>{
+// Should NOT warn
+fntrait_method(&self) -> &str{
+self.not_literal
+}
+}
+
+fnmain(){}

tests/ui/unnecessary_literal_bound.stderr

@@ -0,0 +1,23 @@
+error: returning a `str` unnecessarily tied to the lifetime of arguments
+ --> tests/ui/unnecessary_literal_bound.rs:9:30
+ |
+LL | fn returns_lit(&self) -> &str {
+ | ^^^^ help: try: `&'static 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
+ |
+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
+ |
+LL | fn trait_method(&self) -> &str {
+ | ^^^^ help: try: `&'static str`
+
+error: aborting due to 3 previous errors
+