do not borrow non-Sync data in constants #54424
Conversation
|
@bors try |
WIP: do not borrow non-Sync data in constants We cannot share that data across threads. non-Sync is as bad as non-Freeze in that regard. This is currently WIP because it ignores a test that is broken by #54419. But it is good enough ti get crater going. Fixes #49206. Cc @eddyb @nikomatsakis
| @@ -9,6 +9,7 @@ | |||
| // except according to those terms. | |||
|
|
|||
| // error-pattern:reached recursion limit | |||
| // ignore-test FIXME #54419 | |||
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
💔 Test failed - status-travis |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bors
added
the
S-waiting-on-author
|
Oh great. Not even the full compiler bootstraps with this change.^^ |
|
Seems like Why is it not |
|
I hacked around this, so that we can get a crater run. @bors try |
WIP: do not borrow non-Sync data in constants We cannot share that data across threads. non-Sync is as bad as non-Freeze in that regard. This is currently WIP because it ignores a test that is broken by #54419. But it is good enough to get crater going. Fixes #49206. Cc @eddyb @nikomatsakis
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
☀️ Test successful - status-travis |
|
@craterbot run start=master#1002e404e1ae6b8b907c8655edd41380d0c561cb end=try#4056754e28b7351841378254fef9dbe2f2357c48 mode=check-only |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
@craterbot abort |
|
🗑️ Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-review
|
@craterbot run start=master#1002e404e1ae6b8b907c8655edd41380d0c561cb end=try#4056754e28b7351841378254fef9dbe2f2357c48 mode=check-only |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
|
That didn't go as well as I had hoped. :/ The most common failure pattern is something like: Value-based, this seems fine. However, this is still constructing an instance of a non-Sync type (because of the raw pointer), and there is no way to know that it's non-Sync for no good reason, at least for these values. Basically, if I have a struct containing a non-Sync type, I can rely on it not being Sync. It doesn't matter that all values floating into the constructor are Sync, wrapping a newtype around them can change the game. This breaks ffi_utils, jobserver, libdwfl and is likely also the cause for yargb. Something similar can happen for enums (being inherently non-Sync because some variant is), and that breaks unison, sharing-list and sexpr-rust. seq could be fixed by improving value-based reasoning for enums to ignore lifetime bounds. rustfmt (indirectly) has a Span that it wants to share across thread in a const, which is not Sync. |
TimNN
added
S-waiting-on-author
|
Ping from triage @RalfJung: What is the status of this PR? |
|
Well, good question. I still think we need to do something for soundness, but I do not have a good counterexample (just the contrived one from #49206 (comment)) and I don't know how fix fix this and maintain compatibility -- how to obtain enough "value-based reasoning" that the existing code keeps working. Would this be lang team or compiler team? I'm guessing lang. |
RalfJung
added
the
T-lang
|
Ping from triage @rust-lang/lang: It looks like this PR could use your input. If I understand things correctly, the current issue is that a specific pattern ( This PR also has some other effects I believe, but I'm not sure what exactly they are. @RalfJung: Please verify that my summary is correct / add anything I have missed. It would then probably make sense to |
There was a problem hiding this comment.
I understand that these are weird cases, but the fact that these things can be constructed at compile-time does have some meaning (yes I read the issue).
I think we could reduce the breaking change to runtime promoteds only and keep allowing !Sync but Freeze data in constants.
| /// Concretely, some type parameters get replaced by `()`. We assume that | ||
| /// if a type parameter has no bounds (except for `Sized`), and if the same | ||
| /// type is `Sync` for that type parameter replaced by `()`, then the constructor | ||
| /// valie is also `Sync` at the original type. This is a kind of parametricity |
| /// type is `Sync` for that type parameter replaced by `()`, then the constructor | ||
| /// valie is also `Sync` at the original type. This is a kind of parametricity | ||
| /// assumption. | ||
| /// For now, we only do anything if *no* type parameter has any bound other than |
| // So we can check for that instead of `Freeze`. | ||
| let freeze = Some(def.did) != self.tcx.lang_items().unsafe_cell_type(); | ||
| // For Sync, we generally have to fall back on the type, but have a | ||
| // special exception for 0-argument ADT cosntructors. |
There was a problem hiding this comment.
cosntructors -> constructors
| // special exception for 0-argument ADT cosntructors. | ||
| let sync = { | ||
| let generalized_ty = if args.len() == 0 { | ||
| // A type cosntructor with no arguments is directly a value. |
|
@oli-obk Maybe we can emit lint warnings (from the MIR checking code) for |
The problem is mostly one of not enough information: When a type is not This would all be trivial if we had a notion of |
|
@TimNN: Basically, yes. There is no grounds on which we can allow non- Yet, with the current limitations of I will try to be in the lang team meeting today to discuss this. |
|
Some potential hacks I could imagine:
|
|
Summary of lang team discussion: Nobody objected to this being a soundness issue per se, but for lack of actually exhibited UB we also shouldn't just break code. I am going to try and make this a warning instead of an error, redirecting people to a tracking issue that explains what this is about. I think for promotion we can likely make it a hard error now. I also noticed that almost all of the remaining regressions are due to raw pointers. They are by far the most common |
This seems unlikely to be ideal since unsafe code outside of the compiler/std is likely to use More generally, will you have a chance to update this PR soon? Or maybe lang team needs to come to a decision on it? |
|
I hope to get to it next week. |
|
Ping from triage @RalfJung: What is the status of this PR? |
|
Ah sorry, I didn't have time to get to this. :/ I still want to do this, but it could take a few more weeks. |
TimNN
added
A-allocators
|
ping from triage @RalfJung any updates on this? |
|
Not really, sorry -- the time machine in my basement is still broken. |
|
Ping from triage! If I understand recent comments correctly, this PR won't make any progress for a while (I really wish there was that time-machine 🙄), so I'm closing it as inactive for now. Thanks for your contributions, @RalfJung! |
TimNN
added
S-inactive
We cannot share that data across threads. non-Sync is as bad as non-Freeze in that regard.
This is currently WIP because it ignores a test that is broken by #54419. But it is good enough to get crater going.
Fixes #49206.
Cc @eddyb @nikomatsakis