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.

Sign up for GitHub

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

Jump to bottom

Implement Mutation- and BorrowOfLayoutConstrainedField in thir-unsafeck #86665

Merged
merged 1 commit into from
Jul 13, 2021
Merged

Implement Mutation- and BorrowOfLayoutConstrainedField in thir-unsafeck #86665

merged 1 commit into from
Jul 13, 2021

Conversation

FabianWolff
Copy link
Contributor

Since nobody has so far claimed Mutation- and BorrowOfLayoutConstrainedField in rust-lang/project-thir-unsafeck#7, I have taken the liberty of implementing them in thir-unsafeck.

r? @LeSeulArtichaut

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 27, 2021
@LeSeulArtichaut
Copy link
Contributor

Thanks for your help on this! I'll assign this to you in the tracking issue, if you want to implement something else in the future feel free to just drop a comment on the tracking issue, just to make sure that there aren't two people working on the same thing. With that said, the last few unsafe ops to implement are the trickiest, so having multiple eyes on them is a good thing.

I don't have time to do a proper review today, but I think I can already tell that this PR doesn't cover references that arise from patterns (e.g. ref mut patterns)? It doesn't need to be implemented here, but that's something you can implement if you want additional work.

Also cc @nikomatsakis, if you want to take a look at this as well

@LeSeulArtichaut LeSeulArtichaut mentioned this pull request Jun 27, 2021
Closed
13 tasks
@FabianWolff
Copy link
Contributor Author

Thanks for your quick response! I have added some code (and a test case) now that deals with by-reference bindings in match arms.

@goffrie
Copy link
Contributor

goffrie commented Jun 28, 2021

The title of this PR confused me, but you probably didn't mean thir-borrowck :p

@FabianWolff FabianWolff changed the title Implement Mutation- and BorrowOfLayoutConstrainedField in thir-borrowck Implement Mutation- and BorrowOfLayoutConstrainedField in thir-unsafeck Jun 28, 2021
@FabianWolff
Copy link
Contributor Author

The title of this PR confused me, but you probably didn't mean thir-borrowck :p

Fixed, thanks for catching this! Thinking about all the different borrow kinds etc. apparently got me confused...

LeSeulArtichaut
LeSeulArtichaut reviewed Jul 4, 2021
View reviewed changes
Copy link
Contributor

@LeSeulArtichaut LeSeulArtichaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, sorry for the wait, here's a first review. This is certainly trickier than it looks, and I worry that we'll miss edge cases, but it's OK if we miss things in this PR because THIR unsafeck is gated under an explicitely-marked-as-unsound-do-not-enable option.

compiler/rustc_mir_build/src/check_unsafety.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_build/src/check_unsafety.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_build/src/check_unsafety.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_build/src/check_unsafety.rs Outdated Show resolved Hide resolved
@LeSeulArtichaut
Copy link
Contributor

cc @nikomatsakis you might be interested in reviewing if you have enough bandwidth

LeSeulArtichaut
LeSeulArtichaut reviewed Jul 8, 2021
View reviewed changes
Copy link
Contributor

@LeSeulArtichaut LeSeulArtichaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, besides my last comment I haven't found a case where the implementation is wrong. I'll still pass this to someone with better knowledge of the language than me. In the meantime here are a few nits.

compiler/rustc_mir_build/src/check_unsafety.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_build/src/check_unsafety.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_build/src/check_unsafety.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_build/src/check_unsafety.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_build/src/check_unsafety.rs Outdated Show resolved Hide resolved
@FabianWolff
Copy link
Contributor Author

All done, thanks again for your comments!

@LeSeulArtichaut
Copy link
Contributor

r? @oli-obk

@bors
Copy link
Contributor

bors commented Jul 9, 2021

☔ The latest upstream changes (presumably #85263) made this pull request unmergeable. Please resolve the merge conflicts.

oli-obk
oli-obk requested changes Jul 10, 2021
View reviewed changes
compiler/rustc_mir_build/src/check_unsafety.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_build/src/check_unsafety.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_build/src/check_unsafety.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_build/src/check_unsafety.rs Outdated Show resolved Hide resolved
@oli-obk
Copy link
Contributor

oli-obk commented Jul 12, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jul 12, 2021

📌 Commit b088861 has been approved by oli-obk

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 12, 2021
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jul 12, 2021
@bors
Copy link
Contributor

bors commented Jul 13, 2021

⌛ Testing commit b088861 with merge 1f0db5e...

@bors
Copy link
Contributor

bors commented Jul 13, 2021

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 13, 2021
@bors bors merged commit 1f0db5e into rust-lang:master Jul 13, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LeSeulArtichaut LeSeulArtichaut LeSeulArtichaut left review comments

@oli-obk oli-obk oli-obk requested changes

Assignees

@oli-obk oli-obk

Labels
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.
Projects
None yet
Milestone
1.55.0
Development

Successfully merging this pull request may close these issues.
7 participants
@FabianWolff @LeSeulArtichaut @goffrie @bors @oli-obk @rust-highfive @rustbot