Skip to content

Remove -Zunique-is-unique #4307

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

Merged
merged 1 commit into from
May 5, 2025

Conversation

JoJoDeveloping
Copy link
Contributor

I propose we declare this experiment a failure and clean up the code a bit. I don't know who is using it, I fear no one is.

Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

Yeah, I tend to agree -- it is clear that a viable model for Unique requires a permission that completely degenerates when the protector is over, i.e., a very different kind of uniquenss than what we have for &mut. We have rust-lang/unsafe-code-guidelines#384 to track whether we want to do anything here in the future, and to record the outcome of this experiment.

There is an interesting discussion to be had whether we want this for all references -- this is tracked at rust-lang/unsafe-code-guidelines#450.

Copy link
Member

Choose a reason for hiding this comment

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

This test doesn't make much sense any more in the form that you left it in, with all the comments stripped. I think the test can just be removed now (maybe double-check that we have an existing "pass" test equivalent to raw_children_of_refmut_can_alias).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the file and moved the test to the general tree_borrows.rs test file

Copy link
Member

Choose a reason for hiding this comment

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

Isn't this an almost exact duplicate of two_raw?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yes. But we could also make the test here stronger and also do reads directly to/from the reference, which two_raw does not do since it was originally taken from SB where this is not allowed.

Copy link
Member

Choose a reason for hiding this comment

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

That's already pretty much covered by local_addr_of_mut.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, then we can simply remove the test

@RalfJung
Copy link
Member

RalfJung commented May 4, 2025

This looks great, thanks! Please squash the commits, then we can land this. Please use the --keep-base flag when squashing so that the force-push diff is easier to review; do not rebase on the latest master branch. Then write @rustbot ready after you pushed the squashed PR.

@rustbot author

@rustbot rustbot added the S-waiting-on-author Status: Waiting for the PR author to address review comments label May 4, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 4, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@JoJoDeveloping JoJoDeveloping force-pushed the remove-unique-is-unique branch from 85d8304 to 8cbe805 Compare May 4, 2025 16:49
@JoJoDeveloping
Copy link
Contributor Author

@rustbot ready

PS: ./miri squash worked very nicely

@rustbot rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels May 4, 2025
@RalfJung
Copy link
Member

RalfJung commented May 5, 2025

PS: ./miri squash worked very nicely

Thanks for testing it. :) I'll add it to my standard message then.

@RalfJung RalfJung added this pull request to the merge queue May 5, 2025
Merged via the queue into rust-lang:master with commit 333e599 May 5, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Waiting for a review to complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants