fix: transform `return` into `break` when inlining functions by Albab-Hasan · Pull Request #21908 · rust-lang/rust-analyzer

Albab-Hasan · 2026-03-29T14:00:29Z

when inlining a function containing return expressions the return would incorrectly exit the caller rather than the inlined block. wrap the inlined body in a labeled block ('inline: { ... }) and rewrite return <expr> to break 'inline <expr>.

returns inside closures, async, const, and gen blocks are not changed because they have their own return scope.

Closes #19377

ChayimFriedman2 · 2026-03-30T22:28:58Z

I have a few concerns with this PR:

It extends an assist using mutable tree editing, which I feel isn't the correct call at the moment.
To handle macros and other cursed cases, we'll need at least to descend into the tokens, and that complicates things because we can do it only for the original function. Furthermore I believe the correct way to handle that is by exposing a function from hir, say fn return_points(func: Function) -> Vec<SyntaxNode>, not using textual analysis which is inherently incomplete.

Albab-Hasan · 2026-03-31T04:08:59Z

@ChayimFriedman2 Thanks for the review.

agree the syntactic approach is likely incomplete and not the best way to go. walking .descendants() on the cloned tree can't see returns inside macro expansions.

im thinking of adding a method on hir::Function (something like return_points()) that uses Body + BodySourceMap to find returns at the HIR level. Since HIR lowering already expands macros Expr::Return naturally covers macro generated returns too. the walk would skip expressions that create their own return scope (closures, async blocks, const blocks). then expr_syntax() maps the results back to syntax locations for the assist to consume.

for macro-generated returns specifically: we can detect them (the InFile from expr_syntax() will have a macro file_id) but i dont think we can rewrite them since the macro will still expand to return after inlining. i think the safest option is to not offer the assist when the function contains macro-generated returns. does that sound right?

ChayimFriedman2 · 2026-03-31T04:16:18Z

Using the HIR is exactly what I meant.

As for macros, we can upmap the return keyword, and if it maps to something sensible (i.e. in the source code we're editing), replace it with break 'block.

Albab-Hasan · 2026-03-31T08:20:10Z

@ChayimFriedman2 done

ChayimFriedman2 · 2026-03-31T08:19:22Z

crates/hir/src/lib.rs


+    /// Returns the return expressions in this function's body that belong to the
+    /// function itself (not inside closures or async blocks which create their own
+    /// return scope). Uses HIR analysis, which handles returns inside macro expansions.


Suggested change

/// return scope). Uses HIR analysis, which handles returns inside macro expansions.

/// return scope).

No need to say it's using HIR analysis, it's obvious for any function in this file.

ChayimFriedman2 · 2026-03-31T08:20:10Z

crates/hir/src/lib.rs

+        };
+        let (body, source_map) = Body::with_source_map(db, func_id.into());
+        let mut returns = vec![];
+        let mut to_visit = vec![body.root_expr()];


A recursion is simpler IMO than an out-of-line traversal.

ChayimFriedman2 · 2026-03-31T08:21:52Z

crates/ide-assists/src/handlers/inline_call.rs

+        vec![]
+    } else if file_id.is_macro() {
+        // Function defined in a macro — the body was prettified so HIR source map ranges
+        // don't match. Walk the prettified body syntactically instead.


What? No. You can use return_points() on a function defined in a macro just fine.

ChayimFriedman2 · 2026-03-31T08:23:25Z

crates/ide-assists/src/handlers/inline_call.rs

+                    body.syntax()
+                        .descendants()
+                        .filter_map(ast::ReturnExpr::cast)
+                        .find(|r| r.syntax().text_range() == range)


Inside a (function-like) macro we don't have ReturnExpr, we have a TokenTree. That's why I deliberately suggested mapping the return keyword, and collecting Vec<TextRange>.

ChayimFriedman2 · 2026-03-31T08:24:12Z

crates/ide-assists/src/handlers/inline_call.rs

+    // Transform return expressions into break expressions with a labeled block.
+    // This is needed because `return` in the inlined body would otherwise exit
+    // the *caller* function, not just the inlined block.
+    if !returns_to_replace.is_empty() {


And then here you can just do a textual edit of the upmapped return with break 'label.

Albab-Hasan · 2026-04-05T16:16:37Z

@ChayimFriedman2 done. sorry for the delay, let me know if anything needs to be changed

ChayimFriedman2 · 2026-04-05T16:38:39Z

crates/hir/src/lib.rs


+    /// Returns the text ranges of `return` keywords in this function's body,
+    /// excluding those inside closures or async blocks.
+    pub fn return_points(self, db: &dyn HirDatabase) -> Vec<TextRange> {


This function should still IMO return Vec<ast::ReturnExpr>, without upmapping them, as a general-purpose API (that maybe will be useful elsewhere as well). Do the upmapping in this assist.

ChayimFriedman2 · 2026-04-05T16:54:10Z

crates/ide-assists/src/handlers/inline_call.rs

+/// Replaces `return` / `return expr` with `break 'inline` / `break 'inline expr`
+/// for all `ReturnExpr` nodes that belong directly to the function body
+/// (i.e., not nested inside closures or async blocks).
+fn replace_returns_with_breaks(body: &ast::BlockExpr) {


This doesn't use return_points().

replace_returns_with_breaks operates on the cloned body (a different syntax tree) so the ReturnExpr nodes from return_points() dont correspond to it. how do you want to use return_points()?

If you map the returns on the original body, the ranges will stay the same. Also as I said use text replacement.

ChayimFriedman2 · 2026-04-05T16:54:44Z

crates/hir/src/lib.rs


+    /// Returns the `return` expressions in this function's body,
+    /// excluding those inside closures or async blocks.
+    pub fn return_points(self, db: &dyn HirDatabase) -> Vec<ast::ReturnExpr> {


Probably better to return Vec<InFile<ast::ReturnExpr>>, even if this is not needed here.

Albab-Hasan · 2026-04-06T09:48:06Z

@ChayimFriedman2 done. can u pls check it out

ChayimFriedman2 · 2026-04-07T09:17:41Z

The current revision is not what I had in mind still. But I realized that what I had in mind is very difficult because the various edits we do with the cloned body make it difficult to keep track of the ranges.

This is actually a perfect place to use SyntaxEditor, as it is able of keeping this track itself. This assist was not migrated yet however.

I also stole return_points() for a branch of mine and this made me realize that it should be a method on Semantics (I called it fn_return_points()), as it works with ASTs, so it needs to cache them in the Semantics.

Albab-Hasan · 2026-04-07T09:52:28Z

@ChayimFriedman2 thanks, ive moved return_points() to semantics as fn_return_points() as you suggested.

for the SyntaxEditor migration i looked into it but the entire assist currently uses clone_for_update() + ted, and all the range based covering_element lookups (for params, self, Self type, etc.) depend on that. migrating just the return transformation to SyntaxEditor doesnt work cleanly since SyntaxEditor::finish() produces a new tree that breaks the existing range tracking. this needs a full assist migration which is gonna take a while for me to do. (btw should i push into this pr or should it be in a followup pr?)

also really cool that you picked up fn_return_points() for your rename PR. glad the API turned out to be useful beyond this assist.

Albab-Hasan · 2026-04-08T10:33:15Z

@ChayimFriedman2 can i get a approval on weather i should work with this approach or not?

ChayimFriedman2 · 2026-04-09T05:32:22Z

Indeed, inline_call is one of the more complicated assists to migrate. And it should definitely be done in a separate PR.

If you want to work on migrating it, that's of course fine. If you do not, that is also fine, but I think I'll close this PR as unactionable for now. We can return to it after the assist is migrated.

Albab-Hasan · 2026-04-09T05:56:38Z

@ChayimFriedman2 that sounds great. ill start working on the migration.

also i would appreciate it if u could check out this pr of mine. its been out for a while and might face merged conflicts soon.

#21864

transform return expressions into labeled breaks when inlining functions

332311c

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 29, 2026

Albab-Hasan added 2 commits March 31, 2026 14:02

use HIR-based return point analysis for inline call assist

3c1fc4e

fix formatting

a63e18f

ChayimFriedman2 requested changes Mar 31, 2026

View reviewed changes

address review feedback on return_points and inline call assist

17c71d1

ChayimFriedman2 requested changes Apr 5, 2026

View reviewed changes

return Vec<ast::ReturnExpr> from return_points instead of Vec<TextRange>

dd5d4db

ChayimFriedman2 requested changes Apr 5, 2026

View reviewed changes

Albab-Hasan added 2 commits April 5, 2026 23:00

return Vec<InFile<ast::ReturnExpr>> from return_points

87ee6d7

use return_points() in inline call assist instead of separate tree walk

a594047

ChayimFriedman2 mentioned this pull request Apr 7, 2026

feat: When renaming a field, rename variables in constructors as well #21979

Open

	/// return scope). Uses HIR analysis, which handles returns inside macro expansions.
	/// return scope).

Conversation

Albab-Hasan commented Mar 29, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChayimFriedman2 commented Mar 30, 2026

Uh oh!

Albab-Hasan commented Mar 31, 2026

Uh oh!

ChayimFriedman2 commented Mar 31, 2026

Uh oh!

Albab-Hasan commented Mar 31, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Albab-Hasan commented Apr 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Albab-Hasan commented Apr 6, 2026

Uh oh!

ChayimFriedman2 commented Apr 7, 2026

Uh oh!

Albab-Hasan commented Apr 7, 2026

Uh oh!

Albab-Hasan commented Apr 8, 2026

Uh oh!

ChayimFriedman2 commented Apr 9, 2026

Uh oh!

Albab-Hasan commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Albab-Hasan commented Mar 29, 2026 •

edited by rustbot

Loading

Albab-Hasan commented Apr 9, 2026 •

edited

Loading