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

Drop order twist of && and || chains #103107

Closed
est31 opened this issue Oct 16, 2022 · 0 comments · Fixed by #103293
Closed

Drop order twist of && and || chains #103107

est31 opened this issue Oct 16, 2022 · 0 comments · Fixed by #103293
Labels
C-bug Category: This is a bug. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@est31
Copy link
Member

est31 commented Oct 16, 2022

If you consider the following program:

struct LoudDrop(&'static str);

impl Drop for LoudDrop {
    fn drop(&mut self) {
        print!("{},", self.0);
    }
}

impl LoudDrop {
    fn foo(&self) -> bool {
        true
    }
}

fn main() {
    if LoudDrop("1").foo() &&
    LoudDrop("2").foo() &&
    LoudDrop("3").foo() &&
    LoudDrop("4").foo() {}
    println!("");

    let _b = LoudDrop("1").foo() &&
    LoudDrop("2").foo() &&
    LoudDrop("3").foo() &&
    LoudDrop("4").foo();
    println!("");

    if !LoudDrop("1").foo() ||
    !LoudDrop("2").foo() ||
    !LoudDrop("3").foo() ||
    !LoudDrop("4").foo() {}
    println!("");

    let _b = !LoudDrop("1").foo() ||
    !LoudDrop("2").foo() ||
    !LoudDrop("3").foo() ||
    !LoudDrop("4").foo();
    println!("");
}

It will print "2,3,4,1," four times. The issue occurs on recent nightly as well as all the way back to 1.0.0. This is a bit weird IMO, and makes && and || chains non-associative. IMO it's a bug that should be fixed to print "1,2,3,4,".

IDK how much we can do about this at the current point, given backwards compatibility concerns. It might not stop programs from compiling, but programs might differ in their runtime output. There is RFC 1857 with the name "Stabilize drop order" but if you read the text, it only concerns the drop order of structs, vecs, tuples, etc. Also, after the merge, the RFC author themself made a PR to add regression tests in #43125 , and mentioned that that PR was enough for getting the RFC tested. The RFC didn't include anything about && chains. I think the language should not be interpreted too generally.

It took a long time until #100526 in which we we got some further drop order tests for "established" language constructs, while there have been many PRs with drop order tests for let-else.

Originally I've discovered this in: #102998 (comment) , but given that it's so visible, I'm sure that someone else has already complained about this.

Maybe one can just implement the change to "1,2,3,4," and make a crater run for it?

cc @Nilstrieb @nathanwhit

@rustbot label T-lang

@est31 est31 added the C-bug Category: This is a bug. label Oct 16, 2022
@rustbot rustbot added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Oct 16, 2022
@bors bors closed this as completed in 19c250a Dec 4, 2022
Aaron1011 pushed a commit to Aaron1011/rust that referenced this issue Jan 6, 2023
Remove drop order twist of && and || and make them associative

Previously a short circuiting binop chain (chain of && or ||s) would drop the temporaries created by the first element after all the other elements, and otherwise follow evaluation order. So `f(1).g() && f(2).g() && f(3).g() && f(4).g()` would drop the temporaries in the order `2,3,4,1`. This made `&&` and `||` non-associative regarding drop order. In other words, adding ()'s to the expression would change drop order: `f(1).g() && (f(2).g() && f(3).g()) && f(4).g()` for example would drop in the order `3,2,4,1`.

As, except for the bool result, there is no data returned by the sub-expressions of the short circuiting binops, we can safely discard of any temporaries created by the sub-expr. Previously, code was already putting the rhs's into terminating scopes, but missed it for the lhs's.

This commit addresses this "twist". We now also put the lhs into a terminating scope. The drop order of the above expressions becomes `1,2,3,4`.

There might be code relying on the current order, and therefore I'd recommend doing a crater run to gauge the impact. I'd argue that such code is already quite wonky as it is one `foo() &&` addition away from breaking. ~~For the impact, I don't expect any *build* failures, as the compiler gets strictly more tolerant: shortening the lifetime of temporaries only expands the list of programs the compiler accepts as valid. There might be *runtime* failures caused by this change however.~~ Edit: both build and runtime failures are possible, e.g. see the example provided by dtolnay [below](rust-lang#103293 (comment)). Edit2: the crater run has finished and [results](rust-lang#103293 (comment)) are that there is only one build failure which is easy to fix with a +/- 1 line diff.

I've included a testcase that now compiles thanks to this patch.

The breakage is also limited to drop order relative to conditionals in the && chain: that is, in code like this:

```Rust
let hello = foo().hi() && bar().world();
println!("hi");
```

we already drop the temporaries of `foo().hi()` before we reach "hi".

I'd ideally have this PR merged before let chains are stabilized. If this PR is taking too long, I'd love to have a more restricted version of this change limited to `&&`'s in let chains: the `&&`'s of such chains are quite special anyways as they accept `let` bindings, in there the `&&` is therefore more a part of the "if let chain" construct than a construct of its own.

Fixes rust-lang#103107

Status: waiting on [this accepted FCP](rust-lang#103293 (comment)) finishing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants