Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
porky11 opened this issue Oct 6, 2024 · 6 comments
Labels
A-destructors Area: Destructors (`Drop`, …) C-discussion Category: Discussion or questions that doesn't represent real issues. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@porky11
Copy link

porky11 commented Oct 6, 2024

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)

@porky11 porky11 added the C-bug Category: This is a bug. label Oct 6, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 6, 2024
@ChayimFriedman2
Copy link
Contributor

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

@rustbot label -C-bug +C-discussion

@rustbot rustbot added C-discussion Category: Discussion or questions that doesn't represent real issues. and removed C-bug Category: This is a bug. labels Oct 6, 2024
@fmease fmease added the A-destructors Area: Destructors (`Drop`, …) label Oct 6, 2024
@jieyouxu jieyouxu added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Oct 6, 2024
@porky11
Copy link
Author

porky11 commented Oct 7, 2024

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
Copy link
Contributor

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
Copy link
Author

porky11 commented Oct 8, 2024

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
Copy link
Contributor

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
Copy link
Contributor

PatchMixolydic commented Oct 9, 2024

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

@Noratrieb Noratrieb removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-destructors Area: Destructors (`Drop`, …) C-discussion Category: Discussion or questions that doesn't represent real issues. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants