rust-analyzer icon indicating copy to clipboard operation
rust-analyzer copied to clipboard

Inlining a method with an early return does not handle early return correctly.

Open VonTum opened this issue 9 months ago • 6 comments

rust-analyzer version: rust-analyzer version: 0.3.2319-standalone [/home/lennart/.vscode/extensions/rust-lang.rust-analyzer-0.3.2319-linux-x64/server/rust-analyzer]

rustc version: rustc 1.82.0 (f6e511eec 2024-10-15)

editor or extension: VSCode v0.3.2319

relevant settings: Base settings

repository link (if public, optional): https://github.com/pc2/sus-compiler

code snippet to reproduce: Using the code action "Inline early_return" also inlines the return None statement unmodified, which does not match the original function's behavior

fn early_return() -> Option<()> {
    return None;
}
fn use_early_return() -> Option<()> {
    if early_return().is_none() {
        return Some(());
    }

    None
}

produces

fn use_early_return() -> Option<()> {
    if {
        return None;
    }.is_none() {
        return Some(());
    }

    None
}

VonTum avatar Mar 16 '25 16:03 VonTum

Actually, just any return statement from an inline function is copied verbatim, which will alter the behavior of the outer function.

Perhaps inlining a function containing a return should create an embedded lambda?

VonTum avatar Mar 16 '25 17:03 VonTum

Perhaps inlining a function containing a return should create an embedded lambda?

I think that would be the best remedy for this 🥲

ShoyuVanilla avatar Mar 17 '25 03:03 ShoyuVanilla

So far the philosophy with these assists has not been to try to produce code that works perfectly as before. Wrapping the function in a lambda would make the code harder to actually refactor into what the user actually wants, and probably still not always be 100% correct. I'm not sure it's worth it.

flodiebold avatar Mar 17 '25 08:03 flodiebold

That being said, wouldn't labeled blocks and break work to handle this?

flodiebold avatar Mar 17 '25 08:03 flodiebold

That being said, wouldn't labeled blocks and break work to handle this?

Oh, this sounds more plausible. Thanks!

ShoyuVanilla avatar Mar 17 '25 08:03 ShoyuVanilla

Oh that does sound better! Indeed, inlining to a lambda isn't really inlining. Whereas the named blocks approach would match the spirit of the inlining much better

VonTum avatar Mar 17 '25 10:03 VonTum