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

Add tests for the drop behavior of some control flow constructs #100526

Merged
merged 1 commit into from
Aug 15, 2022

Conversation

Noratrieb
Copy link
Member

@Noratrieb Noratrieb commented Aug 14, 2022

In #100513 it was shown that the drop behaviour of let_chains is not correct currently. Since drop behaviour is something pretty subtle, this adds explicit tests for the drop behavior of if, if let and match to make sure that it does not regress in the future.

The println!s were left in to make debugging easier in case something goes wrong, but they are not required for the test.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 14, 2022
@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 14, 2022
In rust-lang#100513 it was shown that the drop behavior of let_chains is not correct
currently. Since drop behavior is something pretty subtle, this adds
explicit tests for the drop behavior of `if`, `if let` and `match` to
make sure that it does not regress in the future.

The `println!`s were left in to make debugging easier in case something
goes wrong, but they are not required for the test.
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 14, 2022

📌 Commit f6c2816 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 14, 2022
@est31
Copy link
Member

est31 commented Aug 14, 2022

This is basically duplicating my PR #99291 which is more comprehensive (it might not be a true superset, need to check), but has still been open for a month without it being merged. I also think that my approach to use check-run-results and .run.stdout files allows the test to have way less overhead.

@Noratrieb
Copy link
Member Author

Ah, I wasn't aware about check-run-results, that would be good here, but I guess my solution works as well.

@est31
Copy link
Member

est31 commented Aug 14, 2022

Sure, your solution works too. I am just a bit upset that my work was ignored. I'm not sure what I did wrong, but I'm happy that reviewers got around and accepted your PR. If this PR is merged, I might change #99291 to improve the added test.

@Mark-Simulacrum
Copy link
Member

FWIW, I don't mind r+'ing #99291 if you think that's right, @est31 -- I missed #99291 (comment) previously.

The reason I didn't re-assign this PR is that it didn't touch unstable constructs so seems obviously good, whereas #99291 is in a bit more of an interesting place where the expected semantics are intended to change. (But it also seems OK to test them in the meantime).

I personally found this PR easier to review than the println!-based tests there, because I can directly see in one file what the expected order is (vs. comparing across two files). It also seems less likely to get missed in a rebase/large-scale stderr test update.

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 14, 2022
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#100249 (Fix HorizonOS regression in FileTimes)
 - rust-lang#100253 (Recover from mutable variable declaration where `mut` is placed before `let`)
 - rust-lang#100482 (Add Duration rounding change to release note)
 - rust-lang#100523 ([rustdoc] remove Clean trait)
 - rust-lang#100524 (Impl `Debug` for some structs of rustbuild)
 - rust-lang#100526 (Add tests for the drop behavior of some control flow constructs)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@est31
Copy link
Member

est31 commented Aug 14, 2022

@Mark-Simulacrum fair points. I think the area that needs to be tested is pretty large, and both my and the test added by this PR are only scratching on the surface. I tried to architect my test in a way that you specify the construct in one place, and the list of things that get passed to the construct in another, and then the test explores the entire matrix built by the combination of both. Then there is no manual step in having to expand the entire matrix manually, and one can test for many scenarios. In the instance of this PR for example, it's not trivially observable that each construct tested has both the match and mismatch case covered. But the approach of this PR also has advantages because it can also test the "binding created" case. Again, it's a gigantic design space.

You are right by saying that changes in the stdout file are easy to be missed. They are also easy to be included, as one can pass the --bless flag and one might not realize that the change also also changed the drop order test. I thought that stdout files are neat but it seems they don't have many fans (you are not the first one to not like them)... I will reconsider my approach for that PR and how to do matrix based testing in a good way.

@Mark-Simulacrum
Copy link
Member

I think it's worth thinking about how we can expand the testing here to be more complete, for sure. I agree that the coverage added by our tests so far and (in either PR) is likely fairly minimal compared to what we should have -- I wonder if one possibility is some kind of fuzzing here (or other generated test cases), with the intent being to strictly keep current behavior (or at least intentionally change it).

Seems tough to make sure we're being exhaustive though; maybe one avenue is to look at --emit=mir or similar and try to make sure that the dropping thing has ended up in every user-code place somehow?

@bors bors merged commit 59795d0 into rust-lang:master Aug 15, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 15, 2022
@Noratrieb Noratrieb deleted the tests! branch August 15, 2022 04:17
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Oct 15, 2022
…r=eholk

Drop temporaries created in a condition, even if it's a let chain

Fixes rust-lang#100513.

During the lowering from AST to HIR we wrap expressions acting as conditions in a `DropTemps` expression so that any temporaries created in the condition are dropped after the condition is executed. Effectively this means we transform

```rust
if Some(1).is_some() { .. }
```

into (roughly)

```rust
if { let _t = Some(1).is_some(); _t } { .. }
```

so that if we create any temporaries, they're lifted into the new scope surrounding the condition, so for example something along the lines of

```rust
if { let temp = Some(1); let _t = temp.is_some(); _t }.
```

Before this PR, if the condition contained any let expressions we would not introduce that new scope, instead leaving the condition alone. This meant that in a let-chain like

```rust
if get_drop("first").is_some() && let None = get_drop("last") {
        println!("second");
} else { .. }
```

the temporary created for `get_drop("first")` would be lifted into the _surrounding block_, which caused it to be dropped after the execution of the entire `if` expression.

After this PR, we wrap everything but the `let` expression in terminating scopes. The upside to this solution is that it's minimally invasive, but the downside is that in the worst case, an expression with `let` exprs interspersed like

```rust
if get_drop("first").is_some()
    && let Some(_a) = get_drop("fifth")
    && get_drop("second").is_some()
    && let Some(_b) = get_drop("fourth") { .. }
```

gets _multiple_ new scopes, roughly

```rust
if { let _t = get_drop("first").is_some(); _t }
    && let Some(_a) = get_drop("fifth")
    && { let _t = get_drop("second").is_some(); _t }
    && let Some(_b) = get_drop("fourth") { .. }
```

so instead of all of the temporaries being dropped at the end of the entire condition, they will be dropped right after they're evaluated (before the subsequent `let` expr). So while I'd say the drop behavior around let-chains is _less_ surprising after this PR, it still might not exactly match what people might expect.

For tests, I've just extended the drop order tests added in rust-lang#100526. I'm not sure if that's the best way to go about it, though, so suggestions are welcome.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants