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

remove blanket-Freeze for PhantomData #114559

Closed
wants to merge 3 commits into from

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Aug 6, 2023

The exact behavior of types that are only partially covered by UnsafeCell is still up for debate. So we shouldn't rush to the conclusion that PhantomData<UnsafeCell<T>> has no interior mutability. Let's take back that blanket impl. This will need a crater run because it is a breaking change -- but the locations where Freeze makes the difference are few and far between, so the impact is likely to be low. (UnsafeCell is also !Sync and that has the much bigger impact, and is not masked by PhantomData.)

I couldn't find an existing ui test folder that fits, so I created a new one. tidy doesn't seem to like that though, it complains about new files and new folders, which is kind of counterproductive? I had to silence it the hard way.

@rustbot
Copy link
Collaborator

rustbot commented Aug 6, 2023

r? @TaKO8Ki

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

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 6, 2023
@rustbot
Copy link
Collaborator

rustbot commented Aug 6, 2023

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

@RalfJung RalfJung added T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 6, 2023
@RalfJung
Copy link
Member Author

RalfJung commented Aug 6, 2023

@bors try

@bors
Copy link
Contributor

bors commented Aug 6, 2023

⌛ Trying commit 105df08 with merge 8d0c454bea9d9cc2cc7e288388f3b7d1da19c04f...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Aug 6, 2023

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

@RalfJung
Copy link
Member Author

RalfJung commented Aug 6, 2023

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-114559 created and queued.
🤖 Automatically detected try build 8d0c454bea9d9cc2cc7e288388f3b7d1da19c04f
⚠️ Try build based on commit 105df08, but latest commit is 344af673f0a63147411f917245695d11be7d26a8. Did you forget to make a new try build?
🔍 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 craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 6, 2023
@craterbot
Copy link
Collaborator

🚧 Experiment pr-114559 is now running

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

@Noratrieb
Copy link
Member

Regarding the UI test, there's a good chance there are some UnsafeCell tests in the root dir and it would be nice moving those.

@craterbot
Copy link
Collaborator

🎉 Experiment pr-114559 is completed!
📊 721 regressed and 2 fixed (343114 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

@craterbot 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 Aug 9, 2023
@RalfJung
Copy link
Member Author

RalfJung commented Aug 9, 2023

@craterbot
Copy link
Collaborator

👌 Experiment pr-114559-1 created and queued.
🤖 Automatically detected try build 8d0c454bea9d9cc2cc7e288388f3b7d1da19c04f
⚠️ Try build based on commit 105df08, but latest commit is 5f6edef. Did you forget to make a new try build?
🔍 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 craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 9, 2023
@RalfJung
Copy link
Member Author

RalfJung commented Aug 9, 2023

Ah never mind, this needs a rerun with the last commit included.
@craterbot abort name=pr-114559-1

@bors try

@craterbot
Copy link
Collaborator

🗑️ Experiment pr-114559-1 deleted!

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

@craterbot 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 Aug 9, 2023
@bors
Copy link
Contributor

bors commented Aug 9, 2023

⌛ Trying commit 5f6edef with merge 4001eed09e19538fd1a219411bb07c1bce2bbf55...

@bors
Copy link
Contributor

bors commented Aug 9, 2023

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

@RalfJung
Copy link
Member Author

RalfJung commented Aug 9, 2023

@craterbot
Copy link
Collaborator

👌 Experiment pr-114559-1 created and queued.
🤖 Automatically detected try build 4001eed09e19538fd1a219411bb07c1bce2bbf55
🔍 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 craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 9, 2023
@craterbot
Copy link
Collaborator

🚧 Experiment pr-114559-1 is now running

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

@craterbot
Copy link
Collaborator

🎉 Experiment pr-114559-1 is completed!
📊 246 regressed and 0 fixed (721 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

@craterbot 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 Aug 11, 2023
@RalfJung
Copy link
Member Author

The vast majority of these seem to trace back to 2 root causes:

  • code exploiting that types like this in the ff crate have no interior mutability.
  • something deep inside the macro soup that is the abi_stable crate, probably related to types like this and then taking references to them in const.

@RalfJung
Copy link
Member Author

So, this seems unlikely right now, we'd need solid motivation for the breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants