Skip to content

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

Merged
bors merged 3 commits into
rust-lang:masterfrom
lcnr:disjoint-closure-capture-ub
May 31, 2023
Merged

change BorrowKind::Unique to be a mutating PlaceContext#112070
bors merged 3 commits into
rust-lang:masterfrom
lcnr:disjoint-closure-capture-ub

Conversation

@lcnr

@lcnr lcnr commented May 29, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

rustbot commented May 29, 2023

Copy link
Copy Markdown
Collaborator

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

rustbot commented May 29, 2023

Copy link
Copy Markdown
Collaborator

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
@lcnr lcnr mentioned this pull request May 29, 2023
Closed
@rustbot

rustbot commented May 29, 2023

Copy link
Copy Markdown
Collaborator

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.

Sign in to view
@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.

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

oli-obk commented May 30, 2023

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented May 30, 2023

Copy link
Copy Markdown
Collaborator

📌 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 Dylan-DPC mentioned this pull request May 30, 2023
Closed
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request May 30, 2023
@Dylan-DPC
Rollup merge of rust-lang#112070 - lcnr:disjoint-closure-capture-ub, …
f00651c 
…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

wesleywiser commented May 30, 2023

Copy link
Copy Markdown
Member

@bors p=10

Beta nominated backport for P-critical issue.

@zirconium-n zirconium-n mentioned this pull request May 30, 2023
Merged
@bors

bors commented May 31, 2023

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 25f8f4c with merge e6e4f7e...

@bors

bors commented May 31, 2023

Copy link
Copy Markdown
Collaborator

☀️ 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

cormac-ainc commented May 31, 2023 •
edited
Loading

Copy link
Copy Markdown

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

rust-timer commented May 31, 2023

Copy link
Copy Markdown
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

klensy commented May 31, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

pnkfelix commented May 31, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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
@wesleywiser wesleywiser mentioned this pull request May 31, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 31, 2023
@bors
Auto merge of rust-lang#112145 - wesleywiser:backport_112070, r=Mark-…
90c5418 
…Simulacrum

Backport of rust-lang#112070

Backports rust-lang#112070 to stable

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

cormac-ainc commented Jun 2, 2023

Copy link
Copy Markdown

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

cormac-ainc commented Jun 2, 2023

Copy link
Copy Markdown

(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 mentioned this pull request Jun 3, 2023
Merged
@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
@bors
Auto merge of rust-lang#112246 - Mark-Simulacrum:beta-next, r=Mark-Si…
4e8c9e8 
…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

pnkfelix commented Jun 5, 2023

Copy link
Copy Markdown
Contributor

(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

Mark-Simulacrum commented Jun 5, 2023

Copy link
Copy Markdown
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
@bors
Auto merge of rust-lang#112119 - zirconium-n:issue-113072-merge-borro…
c55d1ee 
…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

Reviewers

No reviews

Assignees

@wesleywiser wesleywiser

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

Milestone

1.72.0

Development

Successfully merging this pull request may close these issues.

Evade borrowck by wrapping your lifetime crimes in a simple closure