From 87454594f7d7da79208783a167f20470c024a19e Mon Sep 17 00:00:00 2001 From: Skyler Rain Ross Date: Tue, 14 Dec 2021 23:09:22 -0800 Subject: [PATCH 1/3] feat: Add a diagnostic with a fix to remove `return` at the end of functions --- crates/hir/src/diagnostics.rs | 7 + crates/hir/src/lib.rs | 11 ++ crates/hir_ty/src/diagnostics/expr.rs | 28 ++++ .../src/handlers/remove_trailing_return.rs | 155 ++++++++++++++++++ crates/ide_diagnostics/src/lib.rs | 3 + 5 files changed, 204 insertions(+) create mode 100644 crates/ide_diagnostics/src/handlers/remove_trailing_return.rs 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..b8c80d56dfc4 100644 --- a/crates/hir/src/lib.rs +++ b/crates/hir/src/lib.rs @@ -35,6 +35,7 @@ use std::{iter, ops::ControlFlow, sync::Arc}; use arrayvec::ArrayVec; use base_db::{CrateDisplayName, CrateId, CrateOrigin, Edition, FileId}; +use diagnostics::RemoveTrailingReturn; use either::Either; use hir_def::{ adt::{ReprKind, VariantData}, @@ -1232,6 +1233,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..0d99f35d85da --- /dev/null +++ b/crates/ide_diagnostics/src/handlers/remove_trailing_return.rs @@ -0,0 +1,155 @@ +use hir::db::AstDatabase; +use hir::diagnostics::RemoveTrailingReturn; +use hir::InFile; +use ide_db::assists::Assist; +use ide_db::source_change::SourceChange; +use syntax::{ast, AstNode, NodeOrToken, TextRange, T}; +use text_edit::TextEdit; + +use crate::{fix, Diagnostic, DiagnosticsContext, Severity}; + +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 semi = match return_expr.syntax().next_sibling_or_token() { + Some(NodeOrToken::Token(token)) if token.kind() == T![;] => Some(token), + _ => None, + }; + + let range_to_replace = match semi { + Some(semi) => { + TextRange::new(return_expr.syntax().text_range().start(), semi.text_range().end()) + } + 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 +} +"#, + r#" +fn foo() { + +} +"#, + ); + } +} 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) } From 113b795391c9883a2e44e264288777381a38c89e Mon Sep 17 00:00:00 2001 From: Skyler Rain Ross Date: Tue, 14 Dec 2021 23:56:09 -0800 Subject: [PATCH 2/3] chore: make tidy happy --- .../ide_diagnostics/src/handlers/remove_trailing_return.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/crates/ide_diagnostics/src/handlers/remove_trailing_return.rs b/crates/ide_diagnostics/src/handlers/remove_trailing_return.rs index 0d99f35d85da..43265bc2d981 100644 --- a/crates/ide_diagnostics/src/handlers/remove_trailing_return.rs +++ b/crates/ide_diagnostics/src/handlers/remove_trailing_return.rs @@ -8,6 +8,9 @@ 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, @@ -142,12 +145,12 @@ fn foo() -> i8 { check_fix( r#" fn foo() { - return$0 + return$0/*ensure tidy is happy*/ } "#, r#" fn foo() { - + /*ensure tidy is happy*/ } "#, ); From 8699c025124466eb2cd2177b71daa3fd859be545 Mon Sep 17 00:00:00 2001 From: Skyler Rain Ross Date: Wed, 15 Dec 2021 16:17:03 -0800 Subject: [PATCH 3/3] internal: fix style nits --- crates/hir/src/lib.rs | 7 +++---- .../src/handlers/remove_trailing_return.rs | 21 ++++++------------- 2 files changed, 9 insertions(+), 19 deletions(-) diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs index b8c80d56dfc4..14aeb3ca17a3 100644 --- a/crates/hir/src/lib.rs +++ b/crates/hir/src/lib.rs @@ -35,7 +35,6 @@ use std::{iter, ops::ControlFlow, sync::Arc}; use arrayvec::ArrayVec; use base_db::{CrateDisplayName, CrateId, CrateOrigin, Edition, FileId}; -use diagnostics::RemoveTrailingReturn; use either::Either; use hir_def::{ adt::{ReprKind, VariantData}, @@ -82,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}, diff --git a/crates/ide_diagnostics/src/handlers/remove_trailing_return.rs b/crates/ide_diagnostics/src/handlers/remove_trailing_return.rs index 43265bc2d981..417cbc7fa460 100644 --- a/crates/ide_diagnostics/src/handlers/remove_trailing_return.rs +++ b/crates/ide_diagnostics/src/handlers/remove_trailing_return.rs @@ -1,9 +1,6 @@ -use hir::db::AstDatabase; -use hir::diagnostics::RemoveTrailingReturn; -use hir::InFile; -use ide_db::assists::Assist; -use ide_db::source_change::SourceChange; -use syntax::{ast, AstNode, NodeOrToken, TextRange, T}; +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}; @@ -31,15 +28,9 @@ fn fixes(ctx: &DiagnosticsContext<'_>, d: &RemoveTrailingReturn) -> Option Some(token), - _ => None, - }; - - let range_to_replace = match semi { - Some(semi) => { - TextRange::new(return_expr.syntax().text_range().start(), semi.text_range().end()) - } + 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(), };