Merge `BorrowKind::Unique` into `BorrowKind::Mut` #112119

zirconium-n · 2023-05-30T22:36:22Z

Fixes #112072

Might have conflict with #112070

r? @lcnr

I'm not sure what's the suitable change in a couple places.

rustbot · 2023-05-30T22:36:32Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

This PR changes MIR

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

Some changes might have occurred in exhaustiveness checking

cc @Nadrieril

zirconium-n

I commented on places I'm not sure.

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

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

compiler/rustc_middle/src/mir/mod.rs

compiler/rustc_mir_build/src/check_unsafety.rs

lcnr

quickly skimmed it but need to take some time to think about the places where Unique currently differs from Mut, will only get to that next week probably

compiler/rustc_mir_build/src/thir/cx/expr.rs

zirconium-n · 2023-05-31T12:12:00Z

quickly skimmed it but need to take some time to think about the places where Unique currently differs from Mut, will only get to that next week probably

From my intuition, Unique is just a Mut except it does not require a mutable binding. (Maybe that's oversimplification?)

So here's some personal observation:

Diagnostics obviously should differ.

These ones should unify Unique and Mut. Since they are checking exactly whether it's a mutable borrow.

rust/compiler/rustc_const_eval/src/transform/check_consts/resolver.rs

Lines 103 to 110 in e29821f

    
           fn ref_allows_mutation(&self, kind: mir::BorrowKind, place: mir::Place<'tcx>) -> bool { 
        
               match kind { 
        
                   mir::BorrowKind::Mut { .. } => true, 
        
                   mir::BorrowKind::Shared | mir::BorrowKind::Shallow | mir::BorrowKind::Unique => { 
        
                       self.shared_borrow_allows_mutation(place) 
        
                   } 
        
               } 
        
           }

rust/compiler/rustc_middle/src/mir/mod.rs

Lines 2030 to 2035 in e29821f

    
           pub fn mutability(&self) -> Mutability { 
        
               match *self { 
        
                   BorrowKind::Shared | BorrowKind::Shallow | BorrowKind::Unique => Mutability::Not, 
        
                   BorrowKind::Mut { .. } => Mutability::Mut, 
        
               } 
        
           }

rust/compiler/rustc_mir_build/src/check_unsafety.rs

Lines 438 to 454 in e29821f

    
           ExprKind::Borrow { borrow_kind, arg } => { 
        
               let mut visitor = LayoutConstrainedPlaceVisitor::new(self.thir, self.tcx); 
        
               visit::walk_expr(&mut visitor, expr); 
        
               if visitor.found { 
        
                   match borrow_kind { 
        
                       BorrowKind::Shallow | BorrowKind::Shared | BorrowKind::Unique 
        
                           if !self.thir[arg].ty.is_freeze(self.tcx, self.param_env) => 
        
                       { 
        
                           self.requires_unsafe(expr.span, BorrowOfLayoutConstrainedField) 
        
                       } 
        
                       BorrowKind::Mut { .. } => { 
        
                           self.requires_unsafe(expr.span, MutationOfLayoutConstrainedField) 
        
                       } 
        
                       BorrowKind::Shallow | BorrowKind::Shared | BorrowKind::Unique => {} 
        
                   } 
        
               } 
        
           }

Same to 1. But I think we can't create a ref uniq binding anyway so it probably does not matter?

rust/compiler/rustc_mir_build/src/check_unsafety.rs

Lines 247 to 268 in e29821f

    
           PatKind::Binding { mode: BindingMode::ByRef(borrow_kind), ty, .. } => { 
        
               if self.inside_adt { 
        
                   let ty::Ref(_, ty, _) = ty.kind() else { 
        
                       span_bug!( 
        
                           pat.span, 
        
                           "BindingMode::ByRef in pattern, but found non-reference type {}", 
        
                           ty 
        
                       ); 
        
                   }; 
        
                   match borrow_kind { 
        
                       BorrowKind::Shallow | BorrowKind::Shared | BorrowKind::Unique => { 
        
                           if !ty.is_freeze(self.tcx, self.param_env) { 
        
                               self.requires_unsafe(pat.span, BorrowOfLayoutConstrainedField); 
        
                           } 
        
                       } 
        
                       BorrowKind::Mut { .. } => { 
        
                           self.requires_unsafe(pat.span, MutationOfLayoutConstrainedField); 
        
                       } 
        
                   } 
        
               } 
        
               visit::walk_pat(self, pat); 
        
           }

Same to 1. but this is just for diagnostic I guess.

rust/compiler/rustc_middle/src/mir/mod.rs

Lines 2044 to 2050 in e29821f

    
           // FIXME: won't be used after diagnostic migration 
        
           pub fn describe_mutability(&self) -> &str { 
        
               match *self { 
        
                   BorrowKind::Shared | BorrowKind::Shallow | BorrowKind::Unique => "immutable", 
        
                   BorrowKind::Mut { .. } => "mutable", 
        
               } 
        
           }

This one probably stay different just to be safe.

rust/compiler/rustc_const_eval/src/transform/promote_consts.rs

Lines 456 to 460 in e29821f

    
           fn validate_ref(&mut self, kind: BorrowKind, place: &Place<'tcx>) -> Result<(), Unpromotable> { 
        
               match kind { 
        
                   // Reject these borrow types just to be safe. 
        
                   // FIXME(RalfJung): could we allow them? Should we? No point in it until we have a usecase. 
        
                   BorrowKind::Shallow | BorrowKind::Unique => return Err(Unpromotable),

This one should stay different since this is the motivation to introduce Unique.

rust/compiler/rustc_borrowck/src/lib.rs

Lines 1958 to 1969 in e29821f

    
           match kind { 
        
               Reservation(WriteKind::MutableBorrow( 
        
                   borrow_kind @ (BorrowKind::Unique | BorrowKind::Mut { .. }), 
        
               )) 
        
               | Write(WriteKind::MutableBorrow( 
        
                   borrow_kind @ (BorrowKind::Unique | BorrowKind::Mut { .. }), 
        
               )) => { 
        
                   let is_local_mutation_allowed = match borrow_kind { 
        
                       BorrowKind::Unique => LocalMutationIsAllowed::Yes, 
        
                       BorrowKind::Mut { .. } => is_local_mutation_allowed, 
        
                       BorrowKind::Shared | BorrowKind::Shallow => unreachable!(), 
        
                   };

lcnr

I am not sure whether specialcasing ClosureCapture borrows this much in compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs is necessary or even particularily helpful, but 🤷

after dealing with the comments, r=me

thank you for working on this ❤️

compiler/rustc_borrowck/src/borrow_set.rs

compiler/rustc_borrowck/src/diagnostics/mod.rs

compiler/rustc_borrowck/src/lib.rs

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

compiler/rustc_middle/src/mir/mod.rs

compiler/rustc_middle/src/mir/syntax.rs

compiler/rustc_mir_build/src/check_unsafety.rs

lcnr · 2023-06-06T13:20:25Z

we should also run crater after the remaining changes as I expect the unsafeck and const checks to be theoretically breaking. Don't have the capacity to try and get an actual test for that though

zirconium-n · 2023-06-07T08:14:45Z

@rustbot label -S-waiting-on-author +S-waiting-on-review

we should also run crater after the remaining changes as I expect the unsafeck and const checks to be theoretically breaking. Don't have the capacity to try and get an actual test for that though

Const checks should be ok, since we forbid &mut in const so it's rejected anyway?
Not sure about unsafeck.

compiler/rustc_middle/src/mir/syntax.rs

lcnr · 2023-06-08T09:57:50Z

@bors try

bors · 2023-06-08T09:58:00Z

⌛ Trying commit 9ce76438c3537c38c0614036add7cbe790cbdfd7 with merge d43518feaed04ed9d50bf2337350de8304ca58cf...

lcnr · 2023-06-15T21:43:40Z

@bors try

bors · 2023-06-15T21:44:29Z

⌛ Trying commit d93e93b3c821de824a888eae65a05f9687ad940c with merge ca6d8d30c65b4fcf60d8a40774045a936ec3c78d...

bors · 2023-06-15T23:18:00Z

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

lcnr · 2023-06-16T07:48:57Z

@craterbot check

craterbot · 2023-06-16T07:49:03Z

👌 Experiment pr-112119 created and queued.
🤖 Automatically detected try build ca6d8d30c65b4fcf60d8a40774045a936ec3c78d
🔍 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 · 2023-06-18T19:31:14Z

🚧 Experiment pr-112119 is now running

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

craterbot · 2023-06-19T23:03:26Z

🎉 Experiment pr-112119 is completed!
📊 16 regressed and 4 fixed (298526 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

lcnr · 2023-06-20T09:36:53Z

looked at a bunch and they all looked spurious, didn't get a nice testcase for them

r=me after rebase

lcnr · 2023-06-20T16:44:49Z

@bors r+ rollup=never

bors · 2023-06-20T16:44:50Z

📌 Commit b8a250f has been approved by lcnr

It is now in the queue for this repository.

bors · 2023-06-21T02:06:14Z

⌛ Testing commit b8a250f with merge c55d1ee...

bors · 2023-06-21T04:47:53Z

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing c55d1ee to master...

rust-timer · 2023-06-21T06:08:55Z

Finished benchmarking commit (c55d1ee): 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)	2.3%	[2.3%, 2.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

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)	3.0%	[3.0%, 3.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 657.959s -> 655.662s (-0.35%)

Patches: * rust-lang/rust#112119 * rust-lang/rust#108293 (partially) * rust-lang/rust#112786 * rust-lang/rust#112060 * rust-lang/rust#112163 * rust-lang/rust#112238 * rust-lang/rust#112165

rustbot assigned lcnr May 30, 2023

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 30, 2023

zirconium-n commented May 30, 2023

View reviewed changes

zirconium-n force-pushed the issue-113072-merge-borrow-kind branch from 0ec3a2d to c12feb5 Compare May 31, 2023 09:50

lcnr reviewed May 31, 2023

View reviewed changes

compiler/rustc_mir_build/src/thir/cx/expr.rs Show resolved Hide resolved

lcnr reviewed Jun 6, 2023

View reviewed changes

lcnr 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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 6, 2023

zirconium-n force-pushed the issue-113072-merge-borrow-kind branch 2 times, most recently from e34c364 to ed3d78f Compare June 7, 2023 00:22

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 7, 2023

zirconium-n mentioned this pull request Jun 7, 2023

Spurious E0492 with closure capture #112378

Closed

lcnr reviewed Jun 8, 2023

View reviewed changes

compiler/rustc_middle/src/mir/syntax.rs Outdated Show resolved Hide resolved

compiler/rustc_middle/src/mir/syntax.rs Outdated Show resolved Hide resolved

zirconium-n force-pushed the issue-113072-merge-borrow-kind branch from 9ce7643 to d93e93b Compare June 8, 2023 10:55

craterbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 16, 2023

craterbot added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Jun 16, 2023

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 Jun 19, 2023

zirconium-n added 3 commits June 20, 2023 20:55

merge BorrowKind::Unique into BorrowKind::Mut

8fb4c41

address most easy comments

a52cc0a

update comment on MutBorrowKind::ClosureCapture

b8a250f

zirconium-n force-pushed the issue-113072-merge-borrow-kind branch from d93e93b to b8a250f Compare June 20, 2023 13:09

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 Jun 20, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 21, 2023

bors merged commit c55d1ee into rust-lang:master Jun 21, 2023

rustbot added this to the 1.72.0 milestone Jun 21, 2023

Veykril mentioned this pull request Jun 29, 2023

Merge BorrowKind::Unique into BorrowKind::Mut rust-lang/rust-analyzer#15170

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge `BorrowKind::Unique` into `BorrowKind::Mut` #112119

Merge `BorrowKind::Unique` into `BorrowKind::Mut` #112119

zirconium-n commented May 30, 2023

rustbot commented May 30, 2023

zirconium-n left a comment

lcnr left a comment

zirconium-n commented May 31, 2023

lcnr left a comment

lcnr commented Jun 6, 2023

zirconium-n commented Jun 7, 2023

lcnr commented Jun 8, 2023

bors commented Jun 8, 2023

lcnr commented Jun 15, 2023

bors commented Jun 15, 2023

bors commented Jun 15, 2023

lcnr commented Jun 16, 2023

craterbot commented Jun 16, 2023

craterbot commented Jun 18, 2023

craterbot commented Jun 19, 2023

lcnr commented Jun 20, 2023

lcnr commented Jun 20, 2023

bors commented Jun 20, 2023

bors commented Jun 21, 2023

bors commented Jun 21, 2023

rust-timer commented Jun 21, 2023

Merge BorrowKind::Unique into BorrowKind::Mut #112119

Merge BorrowKind::Unique into BorrowKind::Mut #112119

Conversation

zirconium-n commented May 30, 2023

rustbot commented May 30, 2023

zirconium-n left a comment

Choose a reason for hiding this comment

lcnr left a comment

Choose a reason for hiding this comment

zirconium-n commented May 31, 2023

lcnr left a comment

Choose a reason for hiding this comment

lcnr commented Jun 6, 2023

zirconium-n commented Jun 7, 2023

lcnr commented Jun 8, 2023

bors commented Jun 8, 2023

lcnr commented Jun 15, 2023

bors commented Jun 15, 2023

bors commented Jun 15, 2023

lcnr commented Jun 16, 2023

craterbot commented Jun 16, 2023

craterbot commented Jun 18, 2023

craterbot commented Jun 19, 2023

lcnr commented Jun 20, 2023

lcnr commented Jun 20, 2023

bors commented Jun 20, 2023

bors commented Jun 21, 2023

bors commented Jun 21, 2023

rust-timer commented Jun 21, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Merge `BorrowKind::Unique` into `BorrowKind::Mut` #112119

Merge `BorrowKind::Unique` into `BorrowKind::Mut` #112119