Skip to content

Commit

Permalink
Auto merge of #13395 - GnomedDev:unnecessary-lit-bound, r=Manishearth
Browse files Browse the repository at this point in the history
Add lint for unnecessary lifetime bounded &str return

Closes #305.

Currently implemented with a pretty strong limitation that it can only see the most basic implicit return, but this should be fixable by something with more time and brain energy than me. Cavets from #13388 apply, as I have not had a review on my clippy lints yet so am pretty new to this.

```
changelog: [`unnecessary_literal_bound`]:  Add lint for unnecessary lifetime bounded &str return.
```
  • Loading branch information
bors committed Oct 15, 2024
2 parents 1563ce5 + b62d262 commit f328623
Show file tree
Hide file tree
Showing 7 changed files with 315 additions and 0 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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`
}
158 changes: 158 additions & 0 deletions clippy_lints/src/unnecessary_literal_bound.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::path_res;
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_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"]
pub UNNECESSARY_LITERAL_BOUND,
pedantic,
"detects &str that could be &'static str in function return types"
}

declare_lint_pass!(UnnecessaryLiteralBound => [UNNECESSARY_LITERAL_BOUND]);

fn extract_anonymous_ref<'tcx>(hir_ty: &Ty<'tcx>) -> Option<&'tcx Ty<'tcx>> {
let TyKind::Ref(lifetime, MutTy { ty, mutbl }) = hir_ty.kind else {
return None;
};

if !lifetime.is_anonymous() || !matches!(mutbl, Mutability::Not) {
return None;
}

Some(ty)
}

fn is_str_literal(expr: &Expr<'_>) -> bool {
matches!(
expr.kind,
ExprKind::Lit(Lit {
node: LitKind::Str(..),
..
}),
)
}

struct FindNonLiteralReturn;

impl<'hir> Visitor<'hir> for FindNonLiteralReturn {
type Result = std::ops::ControlFlow<()>;
type NestedFilter = intravisit::nested_filter::None;

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(())
} else {
intravisit::walk_expr(self, expr)
}
}
}

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,
cx: &LateContext<'tcx>,
kind: FnKind<'tcx>,
decl: &'tcx FnDecl<'_>,
body: &'tcx Body<'_>,
span: Span,
_: LocalDefId,
) {
if span.from_expansion() {
return;
}

// Checking closures would be a little silly
if matches!(kind, FnKind::Closure) {
return;
}

// Check for `-> &str`
let FnRetTy::Return(ret_hir_ty) = decl.output else {
return;
};

let Some(inner_hir_ty) = extract_anonymous_ref(ret_hir_ty) else {
return;
};

if path_res(cx, inner_hir_ty) != Res::PrimTy(PrimTy::Str) {
return;
}

// Check for all return statements returning literals
if check_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,
);
}
}
}
65 changes: 65 additions & 0 deletions tests/ui/unnecessary_literal_bound.fixed
Original file line number Diff line number Diff line change
@@ -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() {}
65 changes: 65 additions & 0 deletions tests/ui/unnecessary_literal_bound.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
#![warn(clippy::unnecessary_literal_bound)]

struct Struct<'a> {
not_literal: &'a str,
}

impl Struct<'_> {
// Should warn
fn returns_lit(&self) -> &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) -> &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) -> &str {
"Literal"
}
}

impl ReturnsStr for Struct<'_> {
// Should NOT warn
fn trait_method(&self) -> &str {
self.not_literal
}
}

fn main() {}
23 changes: 23 additions & 0 deletions tests/ui/unnecessary_literal_bound.stderr
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit f328623

Please sign in to comment.