diff --git a/clippy_lints/src/needless_return.rs b/clippy_lints/src/needless_return.rs index 861a7ec558c..ba280cd5a61 100644 --- a/clippy_lints/src/needless_return.rs +++ b/clippy_lints/src/needless_return.rs @@ -1,13 +1,13 @@ -use rustc_lint::{LateLintPass, LateContext}; use rustc_ast::ast::Attribute; -use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_errors::Applicability; -use rustc_hir::intravisit::{FnKind, walk_expr, NestedVisitorMap, Visitor}; -use rustc_span::source_map::Span; -use rustc_middle::lint::in_external_macro; -use rustc_middle::hir::map::Map; -use rustc_middle::ty::subst::GenericArgKind; +use rustc_hir::intravisit::{walk_expr, FnKind, NestedVisitorMap, Visitor}; use rustc_hir::{Block, Body, Expr, ExprKind, FnDecl, HirId, MatchSource, StmtKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::hir::map::Map; +use rustc_middle::lint::in_external_macro; +use rustc_middle::ty::subst::GenericArgKind; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::source_map::Span; use crate::utils::{fn_def_id, snippet_opt, span_lint_and_sugg, span_lint_and_then}; @@ -46,16 +46,39 @@ enum RetReplacement { declare_lint_pass!(NeedlessReturn => [NEEDLESS_RETURN]); impl<'tcx> LateLintPass<'tcx> for NeedlessReturn { - fn check_fn(&mut self, cx: &LateContext<'tcx>, kind: FnKind<'tcx>, _: &'tcx FnDecl<'tcx>, body: &'tcx Body<'tcx>, _: Span, _: HirId) { + fn check_fn( + &mut self, + cx: &LateContext<'tcx>, + kind: FnKind<'tcx>, + _: &'tcx FnDecl<'tcx>, + body: &'tcx Body<'tcx>, + _: Span, + _: HirId, + ) { match kind { FnKind::Closure(_) => { - check_final_expr(cx, &body.value, Some(body.value.span), RetReplacement::Empty) - } + if !last_statement_borrows(cx, &body.value) { + check_final_expr(cx, &body.value, Some(body.value.span), RetReplacement::Empty) + } + }, FnKind::ItemFn(..) | FnKind::Method(..) => { if let ExprKind::Block(ref block, _) = body.value.kind { - check_block_return(cx, block) + if let Some(expr) = block.expr { + if !last_statement_borrows(cx, expr) { + check_final_expr(cx, expr, Some(expr.span), RetReplacement::Empty); + } + } else if let Some(stmt) = block.stmts.iter().last() { + match stmt.kind { + StmtKind::Expr(ref expr) | StmtKind::Semi(ref expr) => { + if !last_statement_borrows(cx, expr) { + check_final_expr(cx, expr, Some(stmt.span), RetReplacement::Empty); + } + }, + _ => (), + } + } } - } + }, } } } @@ -71,23 +94,16 @@ fn check_block_return(cx: &LateContext<'_>, block: &Block<'_>) { match stmt.kind { StmtKind::Expr(ref expr) | StmtKind::Semi(ref expr) => { check_final_expr(cx, expr, Some(stmt.span), RetReplacement::Empty); - } + }, _ => (), } } } - -fn check_final_expr( - cx: &LateContext<'_>, - expr: &Expr<'_>, - span: Option, - replacement: RetReplacement, -) { +fn check_final_expr(cx: &LateContext<'_>, expr: &Expr<'_>, span: Option, replacement: RetReplacement) { match expr.kind { // simple return is always "bad" ExprKind::Ret(ref inner) => { - // allow `#[cfg(a)] return a; #[cfg(b)] return b;` if !expr.attrs.iter().any(attr_is_cfg) { emit_return_lint( @@ -97,32 +113,34 @@ fn check_final_expr( replacement, ); } - } + }, // a whole block? check it! ExprKind::Block(ref block, _) => { check_block_return(cx, block); - } + }, // a match expr, check all arms // an if/if let expr, check both exprs // note, if without else is going to be a type checking error anyways // (except for unit type functions) so we don't match it - - ExprKind::Match(_, ref arms, source) => { - match source { - MatchSource::Normal => { - for arm in arms.iter() { - check_final_expr(cx, &arm.body, Some(arm.body.span), RetReplacement::Block); - } + ExprKind::Match(_, ref arms, source) => match source { + MatchSource::Normal => { + for arm in arms.iter() { + check_final_expr(cx, &arm.body, Some(arm.body.span), RetReplacement::Block); } - MatchSource::IfDesugar { contains_else_clause: true } | MatchSource::IfLetDesugar { contains_else_clause: true } => { - if let ExprKind::Block(ref ifblock, _) = arms[0].body.kind { - check_block_return(cx, ifblock); - } - check_final_expr(cx, arms[1].body, None, RetReplacement::Empty); - } - _ => () + }, + MatchSource::IfDesugar { + contains_else_clause: true, } - } + | MatchSource::IfLetDesugar { + contains_else_clause: true, + } => { + if let ExprKind::Block(ref ifblock, _) = arms[0].body.kind { + check_block_return(cx, ifblock); + } + check_final_expr(cx, arms[1].body, None, RetReplacement::Empty); + }, + _ => (), + }, _ => (), } } @@ -139,7 +157,7 @@ fn emit_return_lint(cx: &LateContext<'_>, ret_span: Span, inner_span: Option match replacement { RetReplacement::Empty => { span_lint_and_sugg( @@ -151,7 +169,7 @@ fn emit_return_lint(cx: &LateContext<'_>, ret_span: Span, inner_span: Option { span_lint_and_sugg( cx, @@ -162,7 +180,7 @@ fn emit_return_lint(cx: &LateContext<'_>, ret_span: Span, inner_span: Option Visitor<'tcx> for BorrowVisitor<'_, 'tcx> { NestedVisitorMap::None } } - diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs index 2bd0cccd39d..593e2f6c74b 100644 --- a/clippy_lints/src/returns.rs +++ b/clippy_lints/src/returns.rs @@ -7,7 +7,7 @@ use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::source_map::Span; use rustc_span::BytePos; -use crate::utils::{span_lint_and_sugg}; +use crate::utils::span_lint_and_sugg; declare_clippy_lint! { /// **What it does:** Checks for unit (`()`) expressions that can be removed. @@ -30,12 +30,10 @@ declare_clippy_lint! { "needless unit expression" } - declare_lint_pass!(Return => [UNUSED_UNIT]); impl EarlyLintPass for Return { fn check_fn(&mut self, cx: &EarlyContext<'_>, kind: FnKind<'_>, span: Span, _: ast::NodeId) { - if_chain! { if let ast::FnRetTy::Ty(ref ty) = kind.decl().output; if let ast::TyKind::Tup(ref vals) = ty.kind; @@ -102,7 +100,6 @@ impl EarlyLintPass for Return { } } - // get the def site #[must_use] fn get_def(span: Span) -> Option { diff --git a/tests/ui/needless_return.fixed b/tests/ui/needless_return.fixed index ad20e238107..6b5cf2626df 100644 --- a/tests/ui/needless_return.fixed +++ b/tests/ui/needless_return.fixed @@ -69,6 +69,16 @@ fn test_void_match(x: u32) { } } +mod no_lint_if_stmt_borrows { + mod issue_5858 { + fn read_line() -> String { + use std::io::BufRead; + let stdin = ::std::io::stdin(); + return stdin.lock().lines().next().unwrap().unwrap(); + } + } +} + fn main() { let _ = test_end_of_fn(); let _ = test_no_semicolon(); diff --git a/tests/ui/needless_return.rs b/tests/ui/needless_return.rs index af0cdfb207f..1a693c9aa53 100644 --- a/tests/ui/needless_return.rs +++ b/tests/ui/needless_return.rs @@ -69,6 +69,16 @@ fn test_void_match(x: u32) { } } +mod no_lint_if_stmt_borrows { + mod issue_5858 { + fn read_line() -> String { + use std::io::BufRead; + let stdin = ::std::io::stdin(); + return stdin.lock().lines().next().unwrap().unwrap(); + } + } +} + fn main() { let _ = test_end_of_fn(); let _ = test_no_semicolon();