Remove drop order twist of && and || and make them associative

[est31]

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. Edit2: the crater run has finished and results 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:

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 #103107

Status: waiting on this accepted FCP finishing.

[rust-highfive]

r? @wesleywiser

(rust-highfive has picked a reviewer for you, use r? to override)

[dtolnay]

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.

This is not quite right, for example this PR would break the following program.

struct Thing;

impl Drop for Thing {
    fn drop(&mut self) {}
}

struct Ref<'a>(&'a Thing);

impl<'a> Drop for Ref<'a> {
    fn drop(&mut self) {}
}

fn main() {
    let tmp;
    let _ = match Thing {
        ref thing => {
            tmp = thing;
            true
        }
    } && match Ref(tmp) {
        _ => false,
    };
}

It seems weird but it's not that much weirder than things like https://github.com/dtolnay/syn/blob/1.0.102/src/data.rs#L289-L293, so it's hard for me to gauge how widespread this might be.

[est31]

Good point, I confirm this fails with the PR applied. Text edited.

[joshtriplett]

I think this is worth a crater run, and worth attempting even if we need to fix a couple of crates; this is a substantial improvement to understandability.

With this change, will that also make let chains able to use a consistent order?

[est31]

@joshtriplett indeed my investigation of how let chains work has motivated this change. let chains are affected by this twist, yes: if there is a bool condition at the very start of a let chain, it will be dropped last (but if there is any number of let conditions before, this twist doesn't activate, so it's not just "first bool condition comes last"). After this PR, the drop order rule for let chains will be simple: first bool conditions are dropped in reading order, then comes the body, then let conditions in reverse order (as the body might rely on them, as well as the let conditions on each other, so reverse order is the only one you can really do here).

The other behaviour that from my POV should be improved is the drop order of let temporaries regarding the else block. You can either choose consistency with if let and match here, or on the other side, with if and let else (edit: see #103248 as the issue). Personally I'd prefer the latter two as it frees up resources earlier, but feel free to disagree (it might be surprising to people that adding/removing a bool conditional changes what the still-existing let bindings are doing regarding the else drop order).

The entire topic of what a good drop order would be would benefit from lang team input. I've made a longer description of behaviour and how I would like it to change here. I've edited the comment to reflect recent changes so it should be up to date.

[est31]

@bors try

[bors]

⌛ Trying commit 0e70267b50790094287a0d6711fab2849b9fe948 with merge 51ad93974f526eaf4ede64932870490e6c96e4fd...

[bors]

☀️ Try build successful - checks-actions
Build commit: 51ad93974f526eaf4ede64932870490e6c96e4fd (51ad93974f526eaf4ede64932870490e6c96e4fd)

[Dylan-DPC]

@craterbot check

[craterbot]

👌 Experiment pr-103293 created and queued.
🤖 Automatically detected try build 51ad93974f526eaf4ede64932870490e6c96e4fd
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-103293 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-103293 is completed!
📊 76 regressed and 9 fixed (246085 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[est31]

So it seems that there is one build failure due to the drop order changes. It occurs in a match guard in spaad_internal-0.4.0.

Relevant log excerpt:

[INFO] [stdout] error[E0716]: temporary value dropped while borrowed
[INFO] [stdout]    --> src/entangle/transform.rs:459:51
[INFO] [stdout]     |
[INFO] [stdout] 459 |         ReturnType::Type(_, ty) if !(is_name(ty, &name.to_string()) || is_name(ty, "Self"))
[INFO] [stdout]     |                                                   ^^^^^^^^^^^^^^^^-    ------- borrow later used here
[INFO] [stdout]     |                                                   |               |
[INFO] [stdout]     |                                                   |               temporary value is freed at the end of this statement
[INFO] [stdout]     |                                                   creates a temporary which is freed while still in use
[INFO] [stdout]     |
[INFO] [stdout] help: consider using a `let` binding to create a longer lived value
[INFO] [stdout]     |
[INFO] [stdout] 457 ~     let binding = name.to_string();
[INFO] [stdout] 458 ~     if matches!(
[INFO] [stdout] 459 |         &sig.output,
[INFO] [stdout] 460 ~         ReturnType::Type(_, ty) if !(is_name(ty, &binding) || is_name(ty, "Self"))
[INFO] [stdout]     |

This is the relevant line on github.

I don't really understand how this change can cause the build failure, as name is a const reference. All the other involved variables are references too. I don't get why the borrow should later be used. It seems like a bug in rustc to me, that I've missed something when writing my PR. It might be interesting to create a minimum reproducing example.

For the other failures, most of them are either in proc macros or build.rs failures. I need to investigate more closely, but a lot of crate failures are due to libc's build.rs failing:

[INFO] [stderr] error: failed to run custom build command for `libc v0.2.93`
[INFO] [stderr] 
[INFO] [stderr] Caused by:
[INFO] [stderr]   process didn't exit successfully: `/opt/rustwide/target/debug/build/libc-249f7f643d38f60c/build-script-build` (exit status: 101)
[INFO] [stderr]   --- stdout
[INFO] [stderr]   cargo:rerun-if-changed=build.rs
[INFO] [stderr] 
[INFO] [stderr]   --- stderr
[INFO] [stderr]   thread 'main' panicked at 'Failed to get rustc version', /opt/rustwide/cargo-home/registry/src/git.luolix.top-1ecc6299db9ec823/libc-0.2.93/build.rs:9:63

The code in libc doesn't use any && or || except for one place where it uses ||.

I'm not sure what is going on here. I think though that many more crates depend on libc than the 44 failed crates detected by crater, so it can't be a general build failure of libc.

[est31]

So a minimum reproducing example is

fn main() {
    fn is_empty(s: &str) -> bool {
        s.is_empty()
    }
    // Doesn't fail if provided with this
    //let check_val: fn(&str) -> bool = |v| is_empty(v);
    let check_val = |v| is_empty(v);
    let n = "Hi";
    if check_val(&n.to_string()) || check_val("Hello") {}
}

error[E0716]: temporary value dropped while borrowed
  --> file.rs:10:19
   |
LL |     if check_val(&n.to_string()) || check_val("Hello") {}
   |                   ^^^^^^^^^^^^^-    --------- borrow later used here
   |                   |            |
   |                   |            temporary value is freed at the end of this statement
   |                   creates a temporary which is freed while still in use
   |
   = note: consider using a `let` binding to create a longer lived value

And yes, on current Rust it fails if you add a false || to this chain, or move the two checks out like:

check_val(&n.to_string());
check_val("Hello");

There seems to be something going on in the auto-generation of check_val's signature that makes it fail: if you provide the signature for check_val yourself then it won't issue the error. It will issue the error if you specify it as fn(_) -> bool but it won't issue it if you specify it as fn(&_) -> bool. Something really interesting is going on here, maybe even a bug, but it's not a bug of my PR.

The other errors I still need to investigate.

[RalfJung]

This is not code I am familiar with, I cannot review this. r? compiler

[nagisa]

@nikomatsakis said:

rfcbot reviewed

The crater run doesn't give evidence that we aren't changing runtime semantics, but it seems equally likely to me that this change fixes latent bugs as causes them.

For some reason your checkbox is unchecked, and seems like the rfcbot never actually checked it. A bug in rfcbot?

[nagisa]

This PR is changing the behaviour of mixed && and || chains as well. Please adjust a test to exercise those scenarios as well.

Especially keep in mind the fact that operator precedence for && and || is different, so a && b || c && d has a different evaluation order compared to e.g. a && (b || c) && d. I believe with this change the difference cannot be observed either way, though.

[nagisa]

For this test to make sense as a run-pass test, I think it would need to assert something at the runtime. However it doesn't and we already have a test that does that (src/test/ui/drop/drop_order.rs), so I think this should be changed to a check-pass test.

[nagisa]

What is the purpose of this print? Is it testing that the operands to the expression above are dropped before the next statement? Could you add a comment to that effect?

[est31]

Is it testing that the operands to the expression above are dropped before the next statement?

Yes. I can add a comment that explains this, but note that printing in the body is done with tests for other constructs in the file too.

[nagisa]

Style nit: it is easier to understand the compound comment if it is split and placed directly above the relevant branches.

This applies to the pre-existing comment at the top of this body as well.

[nagisa]

Perhaps

Suggested change

	// If the LHS is not another binop itself of the same kind as ours,
	// If the LHS is not another binop itself of the same kind as the current expression,

[nagisa]

FWIW there are a couple of implied contexts that aren’t necessarily immediately obvious. For example, why is it that only the LHS matters, and not RHS (because LAnd/LOr binary ops are left-associative.) The double negative makes this somewhat difficult to understand as well.

Something like the following might be easier to grok, perhaps?

We want to make sure that operands in expressions like a && b && c and a || b || c are dropped in order. These operators are left-associative, so in order to achieve the desired behaviour we don’t want to terminate the LHS expression, if LHS is the same kind of binary operation as the current expression.

At least that’s what I understand this code to be doing.

[nagisa]

@bors r+

[bors]

📌 Commit a59a2d3 has been approved by nagisa

It is now in the queue for this repository.

[bors]

⌛ Testing commit a59a2d3 with merge 19c250a...

[bors]

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing 19c250a to master...

[rust-timer]

Finished benchmarking commit (19c250a): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.