rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.6k stars 12.74k forks source link

Implicit returns and missing semicolons after `return` might cause a different drop order #131313

Open porky11 opened 1 month ago

porky11 commented 1 month ago

I tried this code:

struct D(&'static str);
impl Drop for D {
    fn drop(&mut self) {
        println!("dropping {}", self.0);
    }
}

fn f1() {
    println!("===== f1 =====");
    let _local = D("drop initialized first");
    (D("drop initialized second"), ()).1
}

fn f2() {
    println!("===== f2 =====");
    let _local = D("drop initialized first");
    (D("drop initialized second"), ()).1;
}

fn f3() {
    println!("===== f3 =====");
    let _local = D("drop initialized first");
    return (D("drop initialized second"), ()).1
}

fn f4() {
    println!("===== f4 =====");
    let _local = D("drop initialized first");
    return (D("drop initialized second"), ()).1;
}

fn f5() {
    println!("===== f5 =====");
    let _local = D("drop initialized first");
    let result = (D("drop initialized second"), ()).1;
    result
}

fn main() {
    f1();
    f2();
    f3();
    f4();
    f5();
}

I expected to see this happen: All these functions should do exactly the same. The parameters would be dropped in opposite initialization order. To my understanding of Rust, implicitly and explicitly returning the last argument should do the same. Also the semicolon after a return shouldn't make a difference. And binding the result of something to a variable and then returning it, should also do the same.

Instead, this happened: f1 and f3 drop parameters in initialization order while f2, f4 and f5 drop parameters in opposite initialization order. It's especially weird that the semicolon after the return has any effect. cargo fmt will add semicolons after return. So if this is intentional, there's a bug in cargo fmt. Formatting should never do a semantic change. So when reproducing this bug, be sure to turn off automatic formatting if you have it enabled.

I also had some discussion with somebody on Reddit. It seems to be necessary for lifetimes of temporary values to work correctly.

But I still think, this is confusing. So at least the return case should be fixed. The compiler could just implicitly add a semicolon after the return.

And for implicit return values, even if it's just the implicit return value of a subscope, the let transformation (like in f5) should fix the issue. And since it's possible to generally fiix this with code, I'm sure this can also be fixed in the compiler.

Meta

rustc --version --verbose:

rustc 1.81.0 (eeb90cda1 2024-09-04)
binary: rustc
commit-hash: eeb90cda1969383f56a2637cbd3037bdf598841c
commit-date: 2024-09-04
host: x86_64-unknown-linux-gnu
release: 1.81.0
LLVM version: 18.1.7

(no backtrace available, since it doesn't crash)

ChayimFriedman2 commented 1 month ago

This is documented, so at the very least changing this will be a breaking change.

@rustbot label -C-bug +C-discussion

porky11 commented 1 month ago

I think, I get it. It's about this note:

Temporaries that are created in the final expression of a function body are dropped after any named variables bound in the function body. Their drop scope is the entire function, as there is no smaller enclosing temporary scope.

But I'm not sure if this explains why return is handled differently (f3, f4).

I assume it's because if there is a semicolon at the end, then the final expression is implicitly (), but if there isn't one, the return statement is considered as the final expression (isn't return a statement and not an expression?).

ChayimFriedman2 commented 1 month ago

I assume it's because if there is a semicolon at the end, then the final expression is implicitly (), but if there isn't one, the return statement is considered as the final expression

Yes.

isn't return a statement and not an expression

No. The only statement Rust has is let.

porky11 commented 1 month ago

I guess, this issue can be closed then, and cargo fmt should not add a semicolon behind retun if it's the last expression, but either do nothing or remove the return.

ChayimFriedman2 commented 1 month ago

There are a bunch of (rare) cases where rustfmt can change the meaning of a program. I do not think that means it should not add a semicolon, but removing the return is a possibility.

PatchMixolydic commented 1 month ago

A fix is being proposed for stabilization in Rust 2024. Adding #![feature(shorter_tail_lifetimes)] to the reproducer (playground) produces consistent results in all cases:

===== f1 =====
dropping drop initialized second
dropping drop initialized first
===== f2 =====
dropping drop initialized second
dropping drop initialized first
===== f3 =====
dropping drop initialized second
dropping drop initialized first
===== f4 =====
dropping drop initialized second
dropping drop initialized first
===== f5 =====
dropping drop initialized second
dropping drop initialized first