diff --git a/crates/hir/src/diagnostics.rs b/crates/hir/src/diagnostics.rs index 83c6f2c3b8a6..a28c284e0f4d 100644 --- a/crates/hir/src/diagnostics.rs +++ b/crates/hir/src/diagnostics.rs @@ -49,6 +49,7 @@ diagnostics![ UnresolvedMacroCall, UnresolvedModule, UnresolvedProcMacro, + RemoveTrailingReturn, ]; #[derive(Debug)] @@ -174,3 +175,9 @@ pub struct AddReferenceHere { } pub use hir_ty::diagnostics::IncorrectCase; + +#[derive(Debug)] +pub struct RemoveTrailingReturn { + pub file: HirFileId, + pub return_expr: AstPtr, +} diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs index 7add0f4a4362..14aeb3ca17a3 100644 --- a/crates/hir/src/lib.rs +++ b/crates/hir/src/lib.rs @@ -81,9 +81,9 @@ pub use crate::{ AddReferenceHere, AnyDiagnostic, BreakOutsideOfLoop, InactiveCode, IncorrectCase, InvalidDeriveTarget, MacroError, MalformedDerive, MismatchedArgCount, MissingFields, MissingMatchArms, MissingOkOrSomeInTailExpr, MissingUnsafe, NoSuchField, - RemoveThisSemicolon, ReplaceFilterMapNextWithFindMap, UnimplementedBuiltinMacro, - UnresolvedExternCrate, UnresolvedImport, UnresolvedMacroCall, UnresolvedModule, - UnresolvedProcMacro, + RemoveThisSemicolon, RemoveTrailingReturn, ReplaceFilterMapNextWithFindMap, + UnimplementedBuiltinMacro, UnresolvedExternCrate, UnresolvedImport, UnresolvedMacroCall, + UnresolvedModule, UnresolvedProcMacro, }, has_source::HasSource, semantics::{PathResolution, Semantics, SemanticsScope, TypeInfo}, @@ -1232,6 +1232,16 @@ impl DefWithBody { ); } } + + BodyValidationDiagnostic::RemoveTrailingReturn { return_expr } => { + if let Ok(expr) = source_map.expr_syntax(return_expr) { + acc.push( + RemoveTrailingReturn { file: expr.file_id, return_expr: expr.value } + .into(), + ); + } + } + BodyValidationDiagnostic::MismatchedArgCount { call_expr, expected, found } => { match source_map.expr_syntax(call_expr) { Ok(source_ptr) => acc.push( diff --git a/crates/hir_ty/src/diagnostics/expr.rs b/crates/hir_ty/src/diagnostics/expr.rs index 195c53c17e21..253a71e970cd 100644 --- a/crates/hir_ty/src/diagnostics/expr.rs +++ b/crates/hir_ty/src/diagnostics/expr.rs @@ -55,6 +55,9 @@ pub enum BodyValidationDiagnostic { arg_expr: ExprId, mutability: Mutability, }, + RemoveTrailingReturn { + return_expr: ExprId, + }, } impl BodyValidationDiagnostic { @@ -80,6 +83,7 @@ impl ExprValidator { fn validate_body(&mut self, db: &dyn HirDatabase) { self.check_for_filter_map_next(db); + self.check_for_trailing_return(db); let body = db.body(self.owner); @@ -141,6 +145,30 @@ impl ExprValidator { }); } + fn check_for_trailing_return(&mut self, db: &dyn HirDatabase) { + // todo: handle inner functions and lambdas. + if !matches!(self.owner, DefWithBodyId::FunctionId(_)) { + return; + } + + let body = db.body(self.owner); + + let return_expr = match &body.exprs[body.body_expr] { + Expr::Block { statements, tail, .. } => tail + .or_else(|| match statements.last()? { + Statement::Expr { expr, .. } => Some(*expr), + _ => None, + }) + .filter(|expr| matches!(body.exprs[*expr], Expr::Return { .. })), + Expr::Return { .. } => Some(body.body_expr), + _ => return, + }; + + if let Some(return_expr) = return_expr { + self.diagnostics.push(BodyValidationDiagnostic::RemoveTrailingReturn { return_expr }) + } + } + fn check_for_filter_map_next(&mut self, db: &dyn HirDatabase) { // Find the FunctionIds for Iterator::filter_map and Iterator::next let iterator_path = path![core::iter::Iterator]; diff --git a/crates/ide_diagnostics/src/handlers/remove_trailing_return.rs b/crates/ide_diagnostics/src/handlers/remove_trailing_return.rs new file mode 100644 index 000000000000..417cbc7fa460 --- /dev/null +++ b/crates/ide_diagnostics/src/handlers/remove_trailing_return.rs @@ -0,0 +1,149 @@ +use hir::{db::AstDatabase, diagnostics::RemoveTrailingReturn, InFile}; +use ide_db::{assists::Assist, source_change::SourceChange}; +use syntax::{ast, AstNode}; +use text_edit::TextEdit; + +use crate::{fix, Diagnostic, DiagnosticsContext, Severity}; + +// Diagnostic: remove-trailing-return +// +// This diagnostic is triggered when there is a redundant `return` at the end of a function. +pub(crate) fn remove_trailing_return( + ctx: &DiagnosticsContext<'_>, + d: &RemoveTrailingReturn, +) -> Diagnostic { + Diagnostic::new( + "remove-trailing-return", + "replace return ; with ", + ctx.sema.diagnostics_display_range(InFile::new(d.file, d.return_expr.clone().into())).range, + ) + .severity(Severity::WeakWarning) + .with_fixes(fixes(ctx, d)) +} + +fn fixes(ctx: &DiagnosticsContext<'_>, d: &RemoveTrailingReturn) -> Option> { + let root = ctx.sema.db.parse_or_expand(d.file)?; + + let return_expr = d.return_expr.to_node(&root); + + let return_expr = ast::ReturnExpr::cast(return_expr.syntax().clone())?; + + let stmt = return_expr.syntax().parent().and_then(ast::ExprStmt::cast); + let range_to_replace = match stmt { + Some(stmt) => stmt.syntax().text_range(), + None => return_expr.syntax().text_range(), + }; + + let replacement = + return_expr.expr().map_or_else(String::new, |expr| format!("{}", expr.syntax().text())); + + // this *seems* like a reasonable range to trigger in? + let trigger_range = range_to_replace; + + let edit = TextEdit::replace(range_to_replace, replacement); + + let source_change = SourceChange::from_text_edit(d.file.original_file(ctx.sema.db), edit); + + Some(vec![fix( + "replace_with_inner", + "Replace return ; with ", + source_change, + trigger_range, + )]) +} + +#[cfg(test)] +mod tests { + use crate::tests::{check_diagnostics, check_fix}; + + #[test] + fn remove_trailing_return() { + // fixme: this should include the semi. + check_diagnostics( + r#" +fn foo() -> i8 { + return 2; +} //^^^^^^^^ 💡 weak: replace return ; with +"#, + ); + } + + // fixme: implement this for lambdas and inner functions. + #[test] + fn remove_trailing_return_no_lambda() { + // fixme: this should include the semi. + check_diagnostics( + r#" +fn foo() -> i8 { + let bar = || return 2; + bar() +} +"#, + ); + } + + #[test] + fn remove_trailing_return_unit() { + check_diagnostics( + r#" +fn foo() -> i8 { + return +} //^^^^^^ 💡 weak: replace return ; with +"#, + ); + } + + #[test] + fn remove_trailing_return_no_diagnostic_if_no_return_keyword() { + check_diagnostics( + r#" +fn foo() -> i8 { + 3 +} +"#, + ); + } + + #[test] + fn remove_trailing_return_no_diagnostic_if_not_at_and() { + check_diagnostics( + r#" +fn foo() -> i8 { + if true { return 2; } + 3 +} +"#, + ); + } + + #[test] + fn replace_with_expr() { + check_fix( + r#" +fn foo() -> i8 { + return$0 2; +} +"#, + r#" +fn foo() -> i8 { + 2 +} +"#, + ); + } + #[test] + fn replace_with_unit() { + check_fix( + r#" +fn foo() { + return$0/*ensure tidy is happy*/ +} +"#, + r#" +fn foo() { + /*ensure tidy is happy*/ +} +"#, + ); + } +} diff --git a/crates/ide_diagnostics/src/lib.rs b/crates/ide_diagnostics/src/lib.rs index c921d989bd3c..92599c8d9bee 100644 --- a/crates/ide_diagnostics/src/lib.rs +++ b/crates/ide_diagnostics/src/lib.rs @@ -38,6 +38,7 @@ mod handlers { pub(crate) mod missing_unsafe; pub(crate) mod no_such_field; pub(crate) mod remove_this_semicolon; + pub(crate) mod remove_trailing_return; pub(crate) mod replace_filter_map_next_with_find_map; pub(crate) mod unimplemented_builtin_macro; pub(crate) mod unresolved_extern_crate; @@ -204,6 +205,8 @@ pub fn diagnostics( Some(it) => it, None => continue, } + + AnyDiagnostic::RemoveTrailingReturn(d) => handlers::remove_trailing_return::remove_trailing_return(&ctx, &d), }; res.push(d) }