Auto merge of #103293 - est31:untwist_and_drop_order, r=nagisa
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](https://github.com/rust-lang/rust/pull/103293#issuecomment-1285341113). Edit2: the crater run has finished and [results](https://github.com/rust-lang/rust/pull/103293#issuecomment-1292275203) 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 #103107 Status: waiting on [this accepted FCP](https://github.com/rust-lang/rust/pull/103293#issuecomment-1293411354) finishing.
This commit is contained in:
commit
19c250aa12
|
@ -241,17 +241,46 @@ fn resolve_expr<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, expr: &'tcx h
|
|||
// scopes, meaning that temporaries cannot outlive them.
|
||||
// This ensures fixed size stacks.
|
||||
hir::ExprKind::Binary(
|
||||
source_map::Spanned { node: hir::BinOpKind::And, .. },
|
||||
_,
|
||||
ref r,
|
||||
)
|
||||
| hir::ExprKind::Binary(
|
||||
source_map::Spanned { node: hir::BinOpKind::Or, .. },
|
||||
_,
|
||||
source_map::Spanned { node: hir::BinOpKind::And | hir::BinOpKind::Or, .. },
|
||||
ref l,
|
||||
ref r,
|
||||
) => {
|
||||
// For shortcircuiting operators, mark the RHS as a terminating
|
||||
// scope since it only executes conditionally.
|
||||
// expr is a short circuiting operator (|| or &&). As its
|
||||
// functionality can't be overridden by traits, it always
|
||||
// processes bool sub-expressions. bools are Copy and thus we
|
||||
// can drop any temporaries in evaluation (read) order
|
||||
// (with the exception of potentially failing let expressions).
|
||||
// We achieve this by enclosing the operands in a terminating
|
||||
// scope, both the LHS and the RHS.
|
||||
|
||||
// We optimize this a little in the presence of chains.
|
||||
// Chains like a && b && c get lowered to AND(AND(a, b), c).
|
||||
// In here, b and c are RHS, while a is the only LHS operand in
|
||||
// that chain. This holds true for longer chains as well: the
|
||||
// leading operand is always the only LHS operand that is not a
|
||||
// binop itself. Putting a binop like AND(a, b) into a
|
||||
// terminating scope is not useful, thus we only put the LHS
|
||||
// into a terminating scope if it is not a binop.
|
||||
|
||||
let terminate_lhs = match l.kind {
|
||||
// let expressions can create temporaries that live on
|
||||
hir::ExprKind::Let(_) => false,
|
||||
// binops already drop their temporaries, so there is no
|
||||
// need to put them into a terminating scope.
|
||||
// This is purely an optimization to reduce the number of
|
||||
// terminating scopes.
|
||||
hir::ExprKind::Binary(
|
||||
source_map::Spanned {
|
||||
node: hir::BinOpKind::And | hir::BinOpKind::Or, ..
|
||||
},
|
||||
..,
|
||||
) => false,
|
||||
// otherwise: mark it as terminating
|
||||
_ => true,
|
||||
};
|
||||
if terminate_lhs {
|
||||
terminating(l.hir_id.local_id);
|
||||
}
|
||||
|
||||
// `Let` expressions (in a let-chain) shouldn't be terminating, as their temporaries
|
||||
// should live beyond the immediate expression
|
||||
|
|
|
@ -43,7 +43,7 @@ impl DropOrderCollector {
|
|||
}
|
||||
|
||||
if {
|
||||
if self.option_loud_drop(7).is_some() && self.option_loud_drop(6).is_some() {
|
||||
if self.option_loud_drop(6).is_some() && self.option_loud_drop(7).is_some() {
|
||||
self.loud_drop(8);
|
||||
true
|
||||
} else {
|
||||
|
@ -118,17 +118,85 @@ impl DropOrderCollector {
|
|||
}
|
||||
}
|
||||
|
||||
fn and_chain(&self) {
|
||||
// issue-103107
|
||||
if self.option_loud_drop(1).is_some() // 1
|
||||
&& self.option_loud_drop(2).is_some() // 2
|
||||
&& self.option_loud_drop(3).is_some() // 3
|
||||
&& self.option_loud_drop(4).is_some() // 4
|
||||
&& self.option_loud_drop(5).is_some() // 5
|
||||
{
|
||||
self.print(6); // 6
|
||||
}
|
||||
|
||||
let _ = self.option_loud_drop(7).is_some() // 1
|
||||
&& self.option_loud_drop(8).is_some() // 2
|
||||
&& self.option_loud_drop(9).is_some(); // 3
|
||||
self.print(10); // 4
|
||||
|
||||
// Test associativity
|
||||
if self.option_loud_drop(11).is_some() // 1
|
||||
&& (self.option_loud_drop(12).is_some() // 2
|
||||
&& self.option_loud_drop(13).is_some() // 3
|
||||
&& self.option_loud_drop(14).is_some()) // 4
|
||||
&& self.option_loud_drop(15).is_some() // 5
|
||||
{
|
||||
self.print(16); // 6
|
||||
}
|
||||
}
|
||||
|
||||
fn or_chain(&self) {
|
||||
// issue-103107
|
||||
if self.option_loud_drop(1).is_none() // 1
|
||||
|| self.option_loud_drop(2).is_none() // 2
|
||||
|| self.option_loud_drop(3).is_none() // 3
|
||||
|| self.option_loud_drop(4).is_none() // 4
|
||||
|| self.option_loud_drop(5).is_some() // 5
|
||||
{
|
||||
self.print(6); // 6
|
||||
}
|
||||
|
||||
let _ = self.option_loud_drop(7).is_none() // 1
|
||||
|| self.option_loud_drop(8).is_none() // 2
|
||||
|| self.option_loud_drop(9).is_none(); // 3
|
||||
self.print(10); // 4
|
||||
|
||||
// Test associativity
|
||||
if self.option_loud_drop(11).is_none() // 1
|
||||
|| (self.option_loud_drop(12).is_none() // 2
|
||||
|| self.option_loud_drop(13).is_none() // 3
|
||||
|| self.option_loud_drop(14).is_none()) // 4
|
||||
|| self.option_loud_drop(15).is_some() // 5
|
||||
{
|
||||
self.print(16); // 6
|
||||
}
|
||||
}
|
||||
|
||||
fn mixed_and_or_chain(&self) {
|
||||
// issue-103107
|
||||
if self.option_loud_drop(1).is_none() // 1
|
||||
|| self.option_loud_drop(2).is_none() // 2
|
||||
|| self.option_loud_drop(3).is_some() // 3
|
||||
&& self.option_loud_drop(4).is_some() // 4
|
||||
&& self.option_loud_drop(5).is_none() // 5
|
||||
|| self.option_loud_drop(6).is_none() // 6
|
||||
|| self.option_loud_drop(7).is_some() // 7
|
||||
{
|
||||
self.print(8); // 8
|
||||
}
|
||||
}
|
||||
|
||||
fn let_chain(&self) {
|
||||
// take the "then" branch
|
||||
if self.option_loud_drop(2).is_some() // 2
|
||||
&& self.option_loud_drop(1).is_some() // 1
|
||||
if self.option_loud_drop(1).is_some() // 1
|
||||
&& self.option_loud_drop(2).is_some() // 2
|
||||
&& let Some(_d) = self.option_loud_drop(4) { // 4
|
||||
self.print(3); // 3
|
||||
}
|
||||
|
||||
// take the "else" branch
|
||||
if self.option_loud_drop(6).is_some() // 2
|
||||
&& self.option_loud_drop(5).is_some() // 1
|
||||
if self.option_loud_drop(5).is_some() // 1
|
||||
&& self.option_loud_drop(6).is_some() // 2
|
||||
&& let None = self.option_loud_drop(8) { // 4
|
||||
unreachable!();
|
||||
} else {
|
||||
|
@ -152,8 +220,8 @@ impl DropOrderCollector {
|
|||
}
|
||||
|
||||
// let exprs last
|
||||
if self.option_loud_drop(20).is_some() // 2
|
||||
&& self.option_loud_drop(19).is_some() // 1
|
||||
if self.option_loud_drop(19).is_some() // 1
|
||||
&& self.option_loud_drop(20).is_some() // 2
|
||||
&& let Some(_d) = self.option_loud_drop(23) // 5
|
||||
&& let Some(_e) = self.option_loud_drop(22) { // 4
|
||||
self.print(21); // 3
|
||||
|
@ -187,6 +255,21 @@ fn main() {
|
|||
collector.if_();
|
||||
collector.assert_sorted();
|
||||
|
||||
println!("-- and chain --");
|
||||
let collector = DropOrderCollector::default();
|
||||
collector.and_chain();
|
||||
collector.assert_sorted();
|
||||
|
||||
println!("-- or chain --");
|
||||
let collector = DropOrderCollector::default();
|
||||
collector.or_chain();
|
||||
collector.assert_sorted();
|
||||
|
||||
println!("-- mixed and/or chain --");
|
||||
let collector = DropOrderCollector::default();
|
||||
collector.mixed_and_or_chain();
|
||||
collector.assert_sorted();
|
||||
|
||||
println!("-- if let --");
|
||||
let collector = DropOrderCollector::default();
|
||||
collector.if_let();
|
||||
|
|
37
src/test/ui/drop/issue-103107.rs
Normal file
37
src/test/ui/drop/issue-103107.rs
Normal file
|
@ -0,0 +1,37 @@
|
|||
// check-pass
|
||||
// compile-flags: -Z validate-mir
|
||||
|
||||
struct Foo<'a>(&'a mut u32);
|
||||
|
||||
impl<'a> Drop for Foo<'a> {
|
||||
fn drop(&mut self) {
|
||||
*self.0 = 0;
|
||||
}
|
||||
}
|
||||
|
||||
fn and() {
|
||||
let mut foo = 0;
|
||||
// This used to compile also before the fix
|
||||
if true && *Foo(&mut foo).0 == 0 && ({ foo = 0; true}) {}
|
||||
|
||||
// This used to fail before the fix
|
||||
if *Foo(&mut foo).0 == 0 && ({ foo = 0; true}) {}
|
||||
|
||||
println!("{foo}");
|
||||
}
|
||||
|
||||
fn or() {
|
||||
let mut foo = 0;
|
||||
// This used to compile also before the fix
|
||||
if false || *Foo(&mut foo).0 == 1 || ({ foo = 0; true}) {}
|
||||
|
||||
// This used to fail before the fix
|
||||
if *Foo(&mut foo).0 == 1 || ({ foo = 0; true}) {}
|
||||
|
||||
println!("{foo}");
|
||||
}
|
||||
|
||||
fn main() {
|
||||
and();
|
||||
or();
|
||||
}
|
Loading…
Reference in a new issue