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

change BorrowKind::Unique to be a mutating PlaceContext #112070

Merged
merged 3 commits into from
May 31, 2023

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented May 29, 2023

fixes #112056

I believe that BorrowKind::Unique is a footgun in general, so I added a FIXME and opened #112072. This is a bit too involved for this PR though.

@rustbot
Copy link
Collaborator

rustbot commented May 29, 2023

r? @wesleywiser

(rustbot has picked a reviewer for you, use r? to override)

@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 29, 2023
@rustbot
Copy link
Collaborator

rustbot commented May 29, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@lcnr lcnr changed the title BorrowKind::Unique is a mutating PlaceContext change BorrowKind::Unique to a mutating PlaceContext May 29, 2023
@rustbot
Copy link
Collaborator

rustbot commented May 29, 2023

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

@lcnr lcnr added beta-nominated Nominated for backporting to the compiler in the beta channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels May 29, 2023
@lcnr lcnr changed the title change BorrowKind::Unique to a mutating PlaceContext change BorrowKind::Unique to be a mutating PlaceContext May 29, 2023
@rust-log-analyzer

This comment has been minimized.

@lcnr lcnr force-pushed the disjoint-closure-capture-ub branch from 150a4a1 to d01c358 Compare May 29, 2023 15:55
@rust-log-analyzer

This comment has been minimized.

@lcnr lcnr force-pushed the disjoint-closure-capture-ub branch from d01c358 to 25f8f4c Compare May 29, 2023 16:38
@oli-obk
Copy link
Contributor

oli-obk commented May 30, 2023

@bors r+

@bors
Copy link
Contributor

bors commented May 30, 2023

📌 Commit 25f8f4c has been approved by oli-obk

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 May 30, 2023
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request May 30, 2023
…r=oli-obk

change `BorrowKind::Unique` to be a mutating `PlaceContext`

fixes rust-lang#112056

I believe that `BorrowKind::Unique` is a footgun in general, so I added a FIXME and opened rust-lang#112072. This is a bit too involved for this PR though.
@wesleywiser
Copy link
Member

@bors p=10

Beta nominated backport for P-critical issue.

@bors
Copy link
Contributor

bors commented May 31, 2023

⌛ Testing commit 25f8f4c with merge e6e4f7e...

@bors
Copy link
Contributor

bors commented May 31, 2023

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 31, 2023
@bors bors merged commit e6e4f7e into rust-lang:master May 31, 2023
@rustbot rustbot added this to the 1.72.0 milestone May 31, 2023
@cormac-ainc
Copy link

cormac-ainc commented May 31, 2023

Is there any chance of this landing in stable 1.70? It seems to me that it's an easy enough thing to hit with very innocent-looking code (like this example which exploits lifetime elision to really bury the unsoundness) that there could be serious harm done by leaving it in stable for another 6 weeks. There's also the option of a 1.70.1 if it is too late now.

Our team downgraded to 1.68.2 this week, as we're wrangling lifetimes, we already hit the issue by accident, and it's just too big a hole in borrowck.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e6e4f7e): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.9% [-2.9%, -2.9%] 1
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 643.715s -> 643.102s (-0.10%)

@lcnr lcnr deleted the disjoint-closure-capture-ub branch May 31, 2023 08:37
@klensy
Copy link
Contributor

klensy commented May 31, 2023

Is there any chance of this landing in stable 1.70? It seems to me that it's an easy enough thing to hit with very innocent-looking code (like this example which exploits lifetime elision to really bury the unsoundness) that there could be serious harm done by leaving it in stable for another 6 weeks. There's also the option of a 1.70.1 if it is too late now.

Our team downgraded to 1.68.2 this week, as we're wrangling lifetimes, we already hit the issue by accident, and it's just too big a hole in borrowck.

This PR labeled as stable-nominated, so i guess it will be backported to stable.

@pnkfelix
Copy link
Member

pnkfelix commented May 31, 2023

Unilaterally accepting for stable- and beta-backports, based on consensus decision between @wesleywiser and myself as T-compiler co-leads.

@rustbot label: +stable-accepted +beta-accepted

@rustbot rustbot added beta-accepted Accepted for backporting to the compiler in the beta channel. stable-accepted Accepted for backporting to the compiler in the stable channel. labels May 31, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request May 31, 2023
…Simulacrum

Backport of rust-lang#112070

Backports rust-lang#112070 to stable

r? `@Mark-Simulacrum`
@cormac-ainc
Copy link

I want to thank everyone who worked hard on this, under pressure and at a difficult time. I understand it took an unusual effort to get it done, and I really appreciate it. There's a lot to be proud of there.

@cormac-ainc
Copy link

(One last thing to note is that because this was so extraordinarily last-minute, the fix didn't make it into beta. I suppose it will need to be backported before the next stable promotion, otherwise it will sneak back into stable, right?)

@Mark-Simulacrum Mark-Simulacrum removed beta-nominated Nominated for backporting to the compiler in the beta channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels Jun 3, 2023
@Mark-Simulacrum Mark-Simulacrum added stable-nominated Nominated for backporting to the compiler in the stable channel. and removed stable-nominated Nominated for backporting to the compiler in the stable channel. labels Jun 3, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 4, 2023
…mulacrum

[beta] backports + stage0 bump

This bumps the beta branch to use 1.70 as the bootstrap compiler, and backports:

* rust-lang#112070

r? `@Mark-Simulacrum`
@pnkfelix
Copy link
Member

pnkfelix commented Jun 5, 2023

(One last thing to note is that because this was so extraordinarily last-minute, the fix didn't make it into beta. I suppose it will need to be backported before the next stable promotion, otherwise it will sneak back into stable, right?)

I'm hoping PR #112246 means its in beta now?

@Mark-Simulacrum
Copy link
Member

Yeah, this should be in beta now.

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 21, 2023
…w-kind, r=lcnr

Merge `BorrowKind::Unique` into `BorrowKind::Mut`

Fixes rust-lang#112072

Might have conflict with rust-lang#112070

r? `@lcnr`

I'm not sure what's the suitable change in a couple places.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. stable-accepted Accepted for backporting to the compiler in the stable channel. 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.

Evade borrowck by wrapping your lifetime crimes in a simple closure