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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Dec 4, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8cf521d
Remove drop order twist of && and || and make them associative
est31 Oct 20, 2022
a2076dc
Improve comments
est31 Dec 3, 2022
a59a2d3
Also avoid creating a terminating scope in mixed chains
est31 Dec 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 38 additions & 9 deletions compiler/rustc_hir_analysis/src/check/region.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
97 changes: 90 additions & 7 deletions src/test/ui/drop/drop_order.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// 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 {
Expand All @@ -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
Expand Down Expand Up @@ -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();
Expand Down
37 changes: 37 additions & 0 deletions src/test/ui/drop/issue-103107.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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();
}