From 90e6c477d57f616f2d6fe07a01018921c1cf450f Mon Sep 17 00:00:00 2001 From: GnomedDev Date: Wed, 16 Oct 2024 21:34:00 +0100 Subject: [PATCH] Implement a ReturnVisitor and use in unnecessary_literal_bound --- clippy_lints/src/unnecessary_literal_bound.rs | 65 +++++------ clippy_utils/src/lib.rs | 4 + clippy_utils/src/returns.rs | 109 ++++++++++++++++++ tests/compile-test.rs | 2 +- tests/ui/unnecessary_literal_bound.fixed | 14 +-- tests/ui/unnecessary_literal_bound.rs | 12 +- tests/ui/unnecessary_literal_bound.stderr | 14 ++- 7 files changed, 159 insertions(+), 61 deletions(-) create mode 100644 clippy_utils/src/returns.rs diff --git a/clippy_lints/src/unnecessary_literal_bound.rs b/clippy_lints/src/unnecessary_literal_bound.rs index 80ce67111261..f587eacb513a 100644 --- a/clippy_lints/src/unnecessary_literal_bound.rs +++ b/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, diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index ad85dfa2d1eb..c81fac639fd7 100644 --- a/clippy_utils/src/lib.rs +++ b/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; 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..5859e2107f71 100644 --- a/tests/ui/unnecessary_literal_bound.fixed +++ b/tests/ui/unnecessary_literal_bound.fixed @@ -5,28 +5,26 @@ 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"; } @@ -34,7 +32,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 +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 } diff --git a/tests/ui/unnecessary_literal_bound.rs b/tests/ui/unnecessary_literal_bound.rs index b371ff9d3a2e..de06180d4165 100644 --- a/tests/ui/unnecessary_literal_bound.rs +++ b/tests/ui/unnecessary_literal_bound.rs @@ -5,28 +5,26 @@ 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"; } @@ -34,7 +32,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 +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 } diff --git a/tests/ui/unnecessary_literal_bound.stderr b/tests/ui/unnecessary_literal_bound.stderr index 512b2f9a0afa..c4c82d50584d 100644 --- a/tests/ui/unnecessary_literal_bound.stderr +++ b/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