Skip to content

Commit

Permalink
fix "needless return" for trailing item declarations
Browse files Browse the repository at this point in the history
  • Loading branch information
davidsemakula committed Feb 15, 2024
1 parent db277c7 commit 628b75c
Show file tree
Hide file tree
Showing 9 changed files with 51 additions and 19 deletions.
42 changes: 29 additions & 13 deletions crates/hir-def/src/body/lower.rs
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ impl ExprCollector<'_> {
ast::Expr::BlockExpr(e) => match e.modifier() {
Some(ast::BlockModifier::Try(_)) => self.desugar_try_block(e),
Some(ast::BlockModifier::Unsafe(_)) => {
self.collect_block_(e, |id, statements, tail| Expr::Unsafe {
self.collect_block_(e, |id, statements, tail, _| Expr::Unsafe {
id,
statements,
tail,
Expand All @@ -280,17 +280,20 @@ impl ExprCollector<'_> {
Some(ast::BlockModifier::Label(label)) => {
let label = self.collect_label(label);
self.with_labeled_rib(label, |this| {
this.collect_block_(e, |id, statements, tail| Expr::Block {
id,
statements,
tail,
label: Some(label),
this.collect_block_(e, |id, statements, tail, has_trailing_item_decl| {
Expr::Block {
id,
statements,
tail,
label: Some(label),
has_trailing_item_decl,
}
})
})
}
Some(ast::BlockModifier::Async(_)) => {
self.with_label_rib(RibKind::Closure, |this| {
this.collect_block_(e, |id, statements, tail| Expr::Async {
this.collect_block_(e, |id, statements, tail, _| Expr::Async {
id,
statements,
tail,
Expand Down Expand Up @@ -710,9 +713,15 @@ impl ExprCollector<'_> {

let (btail, expr_id) = self.with_labeled_rib(label, |this| {
let mut btail = None;
let block = this.collect_block_(e, |id, statements, tail| {
let block = this.collect_block_(e, |id, statements, tail, _| {
btail = tail;
Expr::Block { id, statements, tail, label: Some(label) }
Expr::Block {
id,
statements,
tail,
label: Some(label),
has_trailing_item_decl: false,
}
});
(btail, block)
});
Expand Down Expand Up @@ -1118,18 +1127,19 @@ impl ExprCollector<'_> {
}

fn collect_block(&mut self, block: ast::BlockExpr) -> ExprId {
self.collect_block_(block, |id, statements, tail| Expr::Block {
self.collect_block_(block, |id, statements, tail, has_trailing_item_decl| Expr::Block {
id,
statements,
tail,
label: None,
has_trailing_item_decl,
})
}

fn collect_block_(
&mut self,
block: ast::BlockExpr,
mk_block: impl FnOnce(Option<BlockId>, Box<[Statement]>, Option<ExprId>) -> Expr,
mk_block: impl FnOnce(Option<BlockId>, Box<[Statement]>, Option<ExprId>, bool) -> Expr,
) -> ExprId {
let block_has_items = {
let statement_has_item = block.statements().any(|stmt| match stmt {
Expand Down Expand Up @@ -1178,9 +1188,15 @@ impl ExprCollector<'_> {
None
});

let has_trailing_item_decl = block
.statements()
.last()
.map_or(false, |last_stmt| matches!(last_stmt, ast::Stmt::Item(_)));
let syntax_node_ptr = AstPtr::new(&block.into());
let expr_id = self
.alloc_expr(mk_block(block_id, statements.into_boxed_slice(), tail), syntax_node_ptr);
let expr_id = self.alloc_expr(
mk_block(block_id, statements.into_boxed_slice(), tail, has_trailing_item_decl),
syntax_node_ptr,
);

self.def_map = prev_def_map;
self.expander.module = prev_local_module;
Expand Down
2 changes: 1 addition & 1 deletion crates/hir-def/src/body/pretty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,7 @@ impl Printer<'_> {
w!(self, "]");
}
Expr::Literal(lit) => self.print_literal(lit),
Expr::Block { id: _, statements, tail, label } => {
Expr::Block { id: _, statements, tail, label, .. } => {
let label =
label.map(|lbl| format!("{}: ", self.body[lbl].name.display(self.db.upcast())));
self.print_block(label.as_deref(), statements, tail);
Expand Down
2 changes: 1 addition & 1 deletion crates/hir-def/src/body/scope.rs
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ fn compute_expr_scopes(expr: ExprId, body: &Body, scopes: &mut ExprScopes, scope

scopes.set_scope(expr, *scope);
match &body[expr] {
Expr::Block { statements, tail, id, label } => {
Expr::Block { statements, tail, id, label, .. } => {
let mut scope = scopes.new_block_scope(*scope, *id, make_label(label));
// Overwrite the old scope for the block expr, so that every block scope can be found
// via the block itself (important for blocks that only contain items, no expressions).
Expand Down
1 change: 1 addition & 0 deletions crates/hir-def/src/hir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ pub enum Expr {
statements: Box<[Statement]>,
tail: Option<ExprId>,
label: Option<LabelId>,
has_trailing_item_decl: bool,
},
Async {
id: Option<BlockId>,
Expand Down
5 changes: 4 additions & 1 deletion crates/hir-ty/src/diagnostics/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,10 @@ impl ExprValidator {

fn check_for_trailing_return(&mut self, body_expr: ExprId, body: &Body) {
match &body.exprs[body_expr] {
Expr::Block { statements, tail, .. } => {
Expr::Block { statements, tail, has_trailing_item_decl, .. } => {
if *has_trailing_item_decl {
return;
}
let last_stmt = tail.or_else(|| match statements.last()? {
Statement::Expr { expr, .. } => Some(*expr),
_ => None,
Expand Down
2 changes: 1 addition & 1 deletion crates/hir-ty/src/infer/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ impl InferenceContext<'_> {
self.infer_top_pat(pat, &input_ty);
self.result.standard_types.bool_.clone()
}
Expr::Block { statements, tail, label, id } => {
Expr::Block { statements, tail, label, id, .. } => {
self.infer_block(tgt_expr, *id, statements, *tail, *label, expected)
}
Expr::Unsafe { id, statements, tail } => {
Expand Down
2 changes: 1 addition & 1 deletion crates/hir-ty/src/infer/mutability.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ impl InferenceContext<'_> {
self.infer_mut_expr(loc.root, Mutability::Not);
}
Expr::Let { pat, expr } => self.infer_mut_expr(*expr, self.pat_bound_mutability(*pat)),
Expr::Block { id: _, statements, tail, label: _ }
Expr::Block { id: _, statements, tail, .. }
| Expr::Async { id: _, statements, tail }
| Expr::Unsafe { id: _, statements, tail } => {
for st in statements.iter() {
Expand Down
2 changes: 1 addition & 1 deletion crates/hir-ty/src/mir/lower.rs
Original file line number Diff line number Diff line change
Expand Up @@ -558,7 +558,7 @@ impl<'ctx> MirLowerCtx<'ctx> {
Expr::Unsafe { id: _, statements, tail } => {
self.lower_block_to_place(statements, current, *tail, place, expr_id.into())
}
Expr::Block { id: _, statements, tail, label } => {
Expr::Block { id: _, statements, tail, label, .. } => {
if let Some(label) = label {
self.lower_loop(current, place, Some(*label), expr_id.into(), |this, begin| {
if let Some(current) = this.lower_block_to_place(
Expand Down
12 changes: 12 additions & 0 deletions crates/ide-diagnostics/src/handlers/remove_trailing_return.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,18 @@ fn foo() -> u8 {
);
}

#[test]
fn no_diagnostic_if_not_last_statement2() {
check_diagnostics(
r#"
fn foo() -> u8 {
return 2;
fn bar() {}
}
"#,
);
}

#[test]
fn replace_with_expr() {
check_fix(
Expand Down

0 comments on commit 628b75c

Please sign in to comment.