Do not promote values with const drop that need to be dropped#89988

tmiasko · 2021-10-17T20:27:12Z

Changes from #88558 allowed using ~const Drop in constants by
introducing a new NeedsNonConstDrop qualif.

The new qualif was also used for promotion purposes, and allowed
promotion to happen for values that needs to be dropped but which
do have a const drop impl.

Since for promoted the drop implementation is never executed,
this lead to observable change in behaviour. For example:

struct Panic();

impl const Drop for Panic {
    fn drop(&mut self) {
        panic!();
    }
}

fn main() {
    let _ = &Panic();
}

Restore the use of NeedsDrop qualif during promotion to avoid the issue.

rust-highfive · 2021-10-17T20:27:15Z

r? @jackh726

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

tmiasko · 2021-10-17T20:32:57Z

cc @fee1-dead @matthewjasper @oli-obk

compiler/rustc_const_eval/src/transform/check_consts/qualifs.rs

oli-obk · 2021-10-17T20:47:31Z

r? @oli-obk

oli-obk · 2021-10-18T10:09:27Z

cc @rust-lang/wg-const-eval

discussion that spawned this: https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/const.20drop.20and.20promoteds

@bors r+

bors · 2021-10-18T10:09:29Z

📌 Commit f8ea04e3a33eeb29438889e162c977a3fd5314d4 has been approved by oli-obk

compiler/rustc_const_eval/src/transform/check_consts/qualifs.rs

RalfJung · 2021-10-18T15:35:30Z

compiler/rustc_const_eval/src/transform/check_consts/qualifs.rs

Why do we have both NeedsDrop and NeedsNonConrstDrop? That seems rather redundant.

We only want to detect nonconst drops in const items, as const drops can actually get executed just fine.

So why does check_consts have NeedsDrop then? Promotion is a separate pass after all.

oli-obk · 2021-10-18T18:57:51Z

@bors r-

Changes from rust-lang#88558 allowed using `~const Drop` in constants by introducing a new `NeedsNonConstDrop` qualif. The new qualif was also used for promotion purposes, and allowed promotion to happen for values that needs to be dropped but which do have a const drop impl. Since for promoted the drop implementation is never executed, this lead to observable change in behaviour. For example: ```rust struct Panic(); impl const Drop for Panic { fn drop(&mut self) { panic!(); } } fn main() { let _ = &Panic(); } ``` Restore the use of `NeedsDrop` qualif during promotion to avoid the issue.

oli-obk · 2021-10-18T21:10:41Z

@bors r+

bors · 2021-10-18T21:10:43Z

📌 Commit 915a581 has been approved by oli-obk

compiler/rustc_const_eval/src/transform/promote_consts.rs

RalfJung · 2021-10-18T22:19:58Z

compiler/rustc_const_eval/src/transform/check_consts/qualifs.rs

+/// This must be ruled out because implicit promotion would remove side-effects
+/// that occur as part of dropping that value. N.B., the implicit promotion has
+/// to reject const Drop implementations because even if side-effects are ruled
+/// out through other means, the execution of the drop could diverge.


"diverge" means "running into an endless loop", right? Panics seem like a much simpler example to me.

The sentence was intended to encompass panicking. In the context of rustc we generally use diverge to describe a computation that does not return normally, but I can see where you are coming from.

Oh I see. Yeah I think that is not standard terminology, at least not in my peer group. ;)

…askrgr Rollup of 8 pull requests Successful merges: - rust-lang#89766 (RustWrapper: adapt for an LLVM API change) - rust-lang#89867 (Fix macro_rules! duplication when reexported in the same module) - rust-lang#89941 (removing TLS support in x86_64-unknown-none-hermitkernel) - rust-lang#89956 (Suggest a case insensitive match name regardless of levenshtein distance) - rust-lang#89988 (Do not promote values with const drop that need to be dropped) - rust-lang#89997 (Add test for issue rust-lang#84957 - `str.as_bytes()` in a `const` expression) - rust-lang#90002 (:arrow_up: rust-analyzer) - rust-lang#90034 (Tiny tweak to Iterator::unzip() doc comment example.) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned jackh726 Oct 17, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 17, 2021

oli-obk reviewed Oct 17, 2021

View reviewed changes

compiler/rustc_const_eval/src/transform/check_consts/qualifs.rs Outdated Show resolved Hide resolved

rust-highfive assigned oli-obk and unassigned jackh726 Oct 17, 2021

tmiasko force-pushed the unpromote-const-drop branch 2 times, most recently from 11496d4 to f8ea04e Compare October 18, 2021 06:43

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 Oct 18, 2021

RalfJung reviewed Oct 18, 2021

View reviewed changes

compiler/rustc_const_eval/src/transform/check_consts/qualifs.rs Outdated Show resolved Hide resolved

RalfJung reviewed Oct 18, 2021

View reviewed changes

Rename needs_drop to needs_non_const_drop

171cbc0

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 18, 2021

tmiasko force-pushed the unpromote-const-drop branch from f8ea04e to d1b72ef Compare October 18, 2021 19:35

tmiasko force-pushed the unpromote-const-drop branch from d1b72ef to 915a581 Compare October 18, 2021 19:57

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 18, 2021

oli-obk reviewed Oct 18, 2021

View reviewed changes

compiler/rustc_const_eval/src/transform/promote_consts.rs Show resolved Hide resolved

RalfJung reviewed Oct 18, 2021

View reviewed changes

matthiaskrgr mentioned this pull request Oct 19, 2021

Rollup of 8 pull requests #90037

Merged

bors merged commit a0724d7 into rust-lang:master Oct 19, 2021

rustbot added this to the 1.58.0 milestone Oct 19, 2021

tmiasko deleted the unpromote-const-drop branch October 19, 2021 08:15

Uh oh!

Conversation

tmiasko commented Oct 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Oct 17, 2021

Uh oh!

tmiasko commented Oct 17, 2021

Uh oh!

Uh oh!

oli-obk commented Oct 17, 2021

Uh oh!

oli-obk commented Oct 18, 2021

Uh oh!

bors commented Oct 18, 2021

Uh oh!

Uh oh!

RalfJung Oct 18, 2021

Choose a reason for hiding this comment

Uh oh!

oli-obk Oct 18, 2021

Choose a reason for hiding this comment

Uh oh!

RalfJung Oct 18, 2021

Choose a reason for hiding this comment

Uh oh!

oli-obk commented Oct 18, 2021

Uh oh!

oli-obk commented Oct 18, 2021

Uh oh!

bors commented Oct 18, 2021

Uh oh!

Uh oh!

RalfJung Oct 18, 2021

Choose a reason for hiding this comment

Uh oh!

tmiasko Oct 19, 2021

Choose a reason for hiding this comment

Uh oh!

RalfJung Oct 19, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tmiasko commented Oct 17, 2021 •

edited

Loading