remove this weird special case from promotion #78363

RalfJung · 2020-10-25T17:10:50Z

Promotion has a special case to ignore interior mutability under some specific circumstances. The purpose of this PR is to figure out what changes if we remove that. Since Cell::new and friends only get promoted inside const/static initializers these days, it actually is not easy to exploit this case: you need something like

const TEST_INTERIOR_MUT: () = {
    // The "0." case is already ruled out by not permitting any interior mutability in `const`.
    let _val: &'static _ = &(Cell::new(1), 2).1;
};

I assume something like &Some(&(Cell::new(1), 2).1) would hit the nested case inside validate_rvalue... though I am not sure why that would not just trigger nested promotion, first promoting the inner reference and then the outer one?

Fixes #67534 (by simply rejecting that code^^)

r? @oli-obk (but for now this is not meant to be merged!)
Cc @rust-lang/wg-const-eval

RalfJung · 2020-10-25T17:11:05Z

src/test/ui/consts/promote-not.rs

+fn main() {
+    // We must not promote things with interior mutability. Not even if we "project it away".
+    let _val: &'static _ = &(Cell::new(1), 2).0; //~ ERROR temporary value dropped while borrowed
+    let _val: &'static _ = &(Cell::new(1), 2).1; //~ ERROR temporary value dropped while borrowed


These two already failed before.

oli-obk

Is this maybe a leftover of the fact that promotion checking and const checking used to be one huge spaghetti monster? Or is it "just because" our promotion rules in const contexts kind of follow explicit promotion rules?

compiler/rustc_mir/src/transform/promote_consts.rs

RalfJung · 2020-10-25T18:19:00Z

Ah, here is another case that is affected:

const FIVE: Cell<i32> = Cell::new(5);

#[inline(never)]
fn tuple_field() -> &'static u32 {
    // This test is MIR-borrowck-only because the old borrowck
    // doesn't agree that borrows of "frozen" (i.e., without any
    // interior mutability) fields of non-frozen temporaries,
    // should be promoted, while MIR promotion does promote them.
    &(FIVE, 42).1
}

(I wish we would have sticked to what the old borrowck allows.^^)

RalfJung · 2020-10-25T18:21:45Z

Is this maybe a leftover of the fact that promotion checking and const checking used to be one huge spaghetti monster? Or is it "just because" our promotion rules in const contexts kind of follow explicit promotion rules?

I think it is a "because we can" that was added when promotion was ported to MIR (probably it was natural to add given that it was the same code as const checking, where such precision is probably useful and less of a problem).

oli-obk · 2020-10-25T18:23:17Z

a check-only crater run should suffice, please feel free to start it whenever you think the PR is ready. I really hope we can get away with this change

RalfJung · 2020-10-25T18:43:42Z

@bors r-
(trying to de-confuse bors)

RalfJung · 2020-10-25T18:43:47Z

eh...
@bors try

bors · 2020-10-25T18:43:57Z

⌛ Trying commit dfd3ec06104aaaa23cecde2f24fedac2d5fb156b with merge b2c2190fa7bf856bb0ed5e8806266e3a2135c6f8...

bors · 2020-10-25T19:31:19Z

☀️ Try build successful - checks-actions
Build commit: b2c2190fa7bf856bb0ed5e8806266e3a2135c6f8 (b2c2190fa7bf856bb0ed5e8806266e3a2135c6f8)

RalfJung · 2020-10-25T19:49:39Z

@craterbot check

craterbot · 2020-10-25T19:49:43Z

👌 Experiment pr-78363 created and queued.
🤖 Automatically detected try build b2c2190fa7bf856bb0ed5e8806266e3a2135c6f8
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2020-10-28T04:39:06Z

🚧 Experiment pr-78363 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2020-10-31T01:55:59Z

🎉 Experiment pr-78363 is completed!
📊 11 regressed and 10 fixed (127561 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

RalfJung · 2020-10-31T10:11:08Z

All of these regressions are false positives. :)

joshtriplett · 2020-11-15T19:13:08Z

🎉
@rfcbot merge

rfcbot · 2020-11-15T19:13:08Z

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

nikomatsakis · 2020-11-16T14:06:05Z

@rfcbot reviewed

rfcbot · 2020-11-29T04:22:38Z

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot · 2020-12-09T04:27:45Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

RalfJung · 2020-12-09T08:47:13Z

Rebased on newer master.

@oli-obk this PR is now awaiting your review. :)

oli-obk · 2020-12-09T09:07:55Z

@bors r+

bors · 2020-12-09T09:07:56Z

📌 Commit d057a93 has been approved by oli-obk

bors · 2020-12-09T09:14:00Z

⌛ Testing commit d057a93 with merge c0bfe34...

bors · 2020-12-09T11:31:26Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing c0bfe34 to master...

rust-highfive assigned oli-obk Oct 25, 2020

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

RalfJung commented Oct 25, 2020

View reviewed changes

RalfJung force-pushed the promotion branch from 79e6f60 to 6abc857 Compare October 25, 2020 17:38

oli-obk reviewed Oct 25, 2020

View reviewed changes

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

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

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

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 25, 2020

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 25, 2020

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 25, 2020

jyn514 changed the title ~~Crater expeximent: can we remove this weird special case from promotion?~~ Crater experiment: can we remove this weird special case from promotion? Oct 25, 2020

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Oct 31, 2020

oli-obk added T-lang Relevant to the language team, which will review and decide on the PR/issue. I-nominated and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 1, 2020

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Nov 15, 2020

Mark-Simulacrum removed the I-nominated label Nov 17, 2020

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Nov 29, 2020

rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Dec 9, 2020

RalfJung added 3 commits December 9, 2020 09:46

make sure we do not promote things with interior mutability

bd837e8

remove a hack that seems to only benefit a few very special cases

99a44ed

remove a test that does not work any more

d057a93

RalfJung force-pushed the promotion branch from 9924bc3 to d057a93 Compare December 9, 2020 08:46

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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Dec 9, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 9, 2020

bors merged commit c0bfe34 into rust-lang:master Dec 9, 2020

rustbot added this to the 1.50.0 milestone Dec 9, 2020

RalfJung deleted the promotion branch December 9, 2020 15:56

spastorino removed the to-announce Announce this issue on triage meeting label Dec 10, 2020

RalfJung mentioned this pull request Jan 2, 2021

Tracking Issue for infallible promotion #80619

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove this weird special case from promotion #78363

remove this weird special case from promotion #78363

RalfJung commented Oct 25, 2020 •

edited

Loading

RalfJung Oct 25, 2020

oli-obk left a comment

RalfJung commented Oct 25, 2020

RalfJung commented Oct 25, 2020

oli-obk commented Oct 25, 2020

RalfJung commented Oct 25, 2020 •

edited

Loading

RalfJung commented Oct 25, 2020

bors commented Oct 25, 2020

bors commented Oct 25, 2020

RalfJung commented Oct 25, 2020

craterbot commented Oct 25, 2020

craterbot commented Oct 28, 2020

craterbot commented Oct 31, 2020

RalfJung commented Oct 31, 2020

joshtriplett commented Nov 15, 2020

rfcbot commented Nov 15, 2020 •

edited by cramertj

Loading

nikomatsakis commented Nov 16, 2020

rfcbot commented Nov 29, 2020

rfcbot commented Dec 9, 2020

RalfJung commented Dec 9, 2020

oli-obk commented Dec 9, 2020

bors commented Dec 9, 2020

bors commented Dec 9, 2020

bors commented Dec 9, 2020

remove this weird special case from promotion #78363

remove this weird special case from promotion #78363

Conversation

RalfJung commented Oct 25, 2020 • edited Loading

RalfJung Oct 25, 2020

Choose a reason for hiding this comment

oli-obk left a comment

Choose a reason for hiding this comment

RalfJung commented Oct 25, 2020

RalfJung commented Oct 25, 2020

oli-obk commented Oct 25, 2020

RalfJung commented Oct 25, 2020 • edited Loading

RalfJung commented Oct 25, 2020

bors commented Oct 25, 2020

bors commented Oct 25, 2020

RalfJung commented Oct 25, 2020

craterbot commented Oct 25, 2020

craterbot commented Oct 28, 2020

craterbot commented Oct 31, 2020

RalfJung commented Oct 31, 2020

joshtriplett commented Nov 15, 2020

rfcbot commented Nov 15, 2020 • edited by cramertj Loading

nikomatsakis commented Nov 16, 2020

rfcbot commented Nov 29, 2020

rfcbot commented Dec 9, 2020

RalfJung commented Dec 9, 2020

oli-obk commented Dec 9, 2020

bors commented Dec 9, 2020

bors commented Dec 9, 2020

bors commented Dec 9, 2020

RalfJung commented Oct 25, 2020 •

edited

Loading

RalfJung commented Oct 25, 2020 •

edited

Loading

rfcbot commented Nov 15, 2020 •

edited by cramertj

Loading