Skip to content

Shorten drop scope of temporaries in conditions #111725

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

Conversation

azizghuloum
Copy link
Contributor

@azizghuloum azizghuloum commented May 18, 2023

During AST->HIR lowering, instead of transforming

if e1 && e2 ... { --- }

to

if DropTemps(e1' && e2' ...) { --- }

now it gets transformed to

if DropTemps(e1') && DropTemps(e2') ... { --- }

leading to subsequent passes (in MIR) to recognize the && pattern in if e1 && e2 which was previously obscured by unneeded Use markers.

Note that the && itself never constructs temporaries and thus doesn't need to be wrapped by DropTemps.

Fixes #111583
Replaces #111644

@rustbot
Copy link
Collaborator

rustbot commented May 18, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @davidtwco (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 18, 2023
@azizghuloum azizghuloum changed the title pushing DropTemp experessions in conditionals up the && trees optimizing Use expressions inside if condition (take 2) May 18, 2023
@azizghuloum
Copy link
Contributor Author

@cjgillot @dingxiangfei2009 does this address the concerns with the temporaries? thanks in advance.

@rust-log-analyzer

This comment has been minimized.

@azizghuloum azizghuloum marked this pull request as ready for review May 21, 2023 05:56
@rustbot
Copy link
Collaborator

rustbot commented May 21, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@cjgillot cjgillot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Jun 10, 2023
@cjgillot
Copy link
Contributor

Nominating this issue for lang team discussion.

This PR makes 2 user-visible changes to the language:

  1. Temporaries in && conditions are dropped earlier: DropTemps(e1 && e2) becomes DropTemps(e1) && DropTemps(e2). This behaviour already exists with let chains, when one of the conditions is a let.

  2. The MIR built for a if e1 && e2 { is now equivalent to the MIR for if e1 { if e2 {, which changes how borrowck understands that code, see tests/ui/rfc-2497-if-let-chains/chains-without-let.rs.

Point 1 may create unexpected bugs for users that rely on drop order for synchronization (for instance). A crater run may be useful, but I'm not sure it will find such bugs.

@@ -1,7 +1,7 @@
fn and_chain() {
let z;
if true && { z = 3; true} && z == 3 {}
//~^ ERROR E0381
// no longer ERROR E0381
}

fn and_chain_2() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the test in this function unchanged?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry. I don't understand the question. It is changed. I removed the ~^ (meaning, it's no an error) and the output in tests/ui/rfc-2497-if-let-chains/chains-without-let.stderr shows that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or do you mean I need to merge master to resolve the conflict?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean in the other function and_chain_2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because so far I'm only doing the &&s inside the test of if, not && as expression.

I agree that that too should be fixed, along with the || cases.

If this branch gets merged, I will fix the other cases.

@cjgillot cjgillot changed the title optimizing Use expressions inside if condition (take 2) Shorten drop scope of temporaries in conditions Jun 24, 2023
@davidtwco davidtwco assigned cjgillot and unassigned davidtwco Jun 27, 2023
@nikomatsakis
Copy link
Contributor

Discussed in the rust-lang/lang meeting.

We felt this change would require an edition. @dingxiangfei2009 and I are exploring some changes to temporary lifetimes for rust 2024 so perhaps you can reach out on Zulip and we can discuss there.

@tmandry tmandry added the A-maybe-future-edition Something we may consider for a future edition. label Aug 8, 2023
@apiraino apiraino removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Apr 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-maybe-future-edition Something we may consider for a future edition. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
Status: Idea
Development

Successfully merging this pull request may close these issues.

Collapsing two if-statements to a single if statement can result in a large performance decrease
8 participants