diff options
| -rw-r--r-- | CHANGELOG.md | 1 | ||||
| -rw-r--r-- | clippy_lints/src/declared_lints.rs | 1 | ||||
| -rw-r--r-- | clippy_lints/src/lib.rs | 2 | ||||
| -rw-r--r-- | clippy_lints/src/unnecessary_literal_bound.rs | 156 | ||||
| -rw-r--r-- | tests/ui/unnecessary_literal_bound.fixed | 65 | ||||
| -rw-r--r-- | tests/ui/unnecessary_literal_bound.rs | 65 | ||||
| -rw-r--r-- | tests/ui/unnecessary_literal_bound.stderr | 23 |
7 files changed, 313 insertions, 0 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index b7ac0ddfd32..d1d70f4ddfb 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 0f9dbec8a75..3c4e75df8ab 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/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, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index b2229b53afd..15382014012 100644 --- a/clippy_lints/src/lib.rs +++ b/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` } diff --git a/clippy_lints/src/unnecessary_literal_bound.rs b/clippy_lints/src/unnecessary_literal_bound.rs new file mode 100644 index 00000000000..8d9443b71ef --- /dev/null +++ b/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"] + 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 !rustc_hir_analysis::lower_ty(cx.tcx, inner_hir_ty).is_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, + ); + } + } +} diff --git a/tests/ui/unnecessary_literal_bound.fixed b/tests/ui/unnecessary_literal_bound.fixed new file mode 100644 index 00000000000..107e397466d --- /dev/null +++ b/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() {} diff --git a/tests/ui/unnecessary_literal_bound.rs b/tests/ui/unnecessary_literal_bound.rs new file mode 100644 index 00000000000..b371ff9d3a2 --- /dev/null +++ b/tests/ui/unnecessary_literal_bound.rs @@ -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() {} diff --git a/tests/ui/unnecessary_literal_bound.stderr b/tests/ui/unnecessary_literal_bound.stderr new file mode 100644 index 00000000000..512b2f9a0af --- /dev/null +++ b/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 + |