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

Deprecate static mut #3560

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

dyslexicsteak
Copy link

@dyslexicsteak dyslexicsteak commented Jan 26, 2024

Pre-RFC on IRLO.

Results from research survey to inform discussion with respect to perception of the feature. (JS required)

Rendered

@dyslexicsteak dyslexicsteak marked this pull request as ready for review January 26, 2024 10:01
@ehuss ehuss added T-lang Relevant to the language team, which will review and decide on the RFC. A-edition-2024 Area: The 2024 edition labels Jan 26, 2024
@dyslexicsteak dyslexicsteak changed the title Add "Deprecate then remove static mut" RFC Deprecate then remove static mut Jan 26, 2024
@davidhewitt
Copy link
Contributor

Thanks for the RFC! I see that a key point here is that C FFI is suggested to use SyncUnsafeCell instead of static mut. It might be helpful to add to the guide-level explanation a code block which shows the correct pattern to use for such an FFI definition (at the moment this is only described through comments).

pyo3-ffi necessarily has a lot of static mut C FFI definitions which come from the Python interpreter, so we would need to migrate our code. One factor which might be worth noting here is the backwards-compatibility story. PyO3 and probably many other projects will be supporting old Rust versions which don't have SyncUnsafeCell stable at the same time as compiling for these up-to-date versions. Probably for the old Rust versions PyO3 will have to expose its own version of SyncUnsafeCell to keep the API consistent.

Is there any way that we can make this compatibility / migration easier for everyone?

@dyslexicsteak
Copy link
Author

dyslexicsteak commented Jan 27, 2024

Thanks for the RFC! I see that a key point here is that C FFI is suggested to use SyncUnsafeCell instead of static mut. It might be helpful to add to the guide-level explanation a code block which shows the correct pattern to use for such an FFI definition (at the moment this is only described through comments).

pyo3-ffi necessarily has a lot of static mut C FFI definitions which come from the Python interpreter, so we would need to migrate our code. One factor which might be worth noting here is the backwards-compatibility story. PyO3 and probably many other projects will be supporting old Rust versions which don't have SyncUnsafeCell stable at the same time as compiling for these up-to-date versions. Probably for the old Rust versions PyO3 will have to expose its own version of SyncUnsafeCell to keep the API consistent.

Is there any way that we can make this compatibility / migration easier for everyone?

Hi David,

I personally thought the comments were sufficient, but I'll add a dedicated example when I have time.

Speaking of SyncUnsafeCell, yeah, although easily replicable, this might be a bump of the MSRV on new crate versions because the type isn't there in std on old versions. I haven't thought of backwards compatibility measures for old code using new code really, if you have any ideas feel free to share.

The ultimate tool we have to ease migration is an excellent migration guide, but it's also generally good practice to have a cargo fix. Automatic replication of the exact semantics of static mut with SyncUnsafeCell in all cases is not possible due to its bounds and auto-trait implementations, see AssertThreadSafe in the RFC prior art for a better equivalent. We can migrate code to a type that waives all thread safety and aliasing concerns like static mut does, but intelligently choosing the "safest" type that has the most guarantees is also on the table. It doesn't really fix code that has UB, but at least it compiles and the new generated code makes it easier to find and fix the issue.

The output of cargo fix also will not be the greatest if we don't put significant effort, we'll have to try to use .method() syntax where possible instead of Ty::method(STATIC.get().cast()) or similar, among others.

@Nemo157
Copy link
Member

Nemo157 commented Jan 27, 2024

Given the RFC proposes edition-gating the warning, as long as SUC (or whatever other API is proposed) is stabilized by the first release of the 2024 edition there should be no MSRV impact, you won't be able to have a crate with an MSRV that results in both getting the warning (on any version of the compiler) and not having a way to resolve it.

@davidhewitt
Copy link
Contributor

Ah, that's of course the answer, I had not considered that PyO3's migration pathway will be to not change anything until MSRV is bumped past 2024 edition.

And as you say, as long as SUC or other alternatives are stable by the 2024 edition, then the correct step for PyO3 will be to migrate completely over to std at that point 👍

@dyslexicsteak
Copy link
Author

Given the RFC proposes edition-gating the warning, as long as SUC (or whatever other API is proposed) is stabilized by the first release of the 2024 edition there should be no MSRV impact, you won't be able to have a crate with an MSRV that results in both getting the warning (on any version of the compiler) and not having a way to resolve it.

Wait, are you not mandated to be able to use future edition crate versions?

@dlight
Copy link

dlight commented Jan 28, 2024

It seems to me that deprecation is relatively uncontroversial but removal may not be. The only thing this RFC has to say about it is:

Deprecation without removal could be done, but after the presumed migration of at least a majority of the ecosystem after the deprecation lint there is no reason to keep the feature in the language.

I think there is a reason: to maintain backwards compatibility. I mean, not even outright bad and error-prone APIs like std::mem::unitialized are removed, and for a reason. I think deprecation is enough.

What about a deprecation lint that in Rust 2024 is warn by default, and in Rust 2027 and later is deny by default? This is practically the same as a removal, but with a escape hatch. Maybe the RFC could list this as an alternative.

This covers the case where someone has perfectly sound and working static mut code, doesn't want to modify it to use SyncUnsafeCell, but would like to upgrade to Rust 2027 (or a later edition) because some other feature entirely unrelated to this issue.

@dyslexicsteak
Copy link
Author

It seems to me that deprecation is relatively uncontroversial but removal may not be. The only thing this RFC has to say about it is:

Deprecation without removal could be done, but after the presumed migration of at least a majority of the ecosystem after the deprecation lint there is no reason to keep the feature in the language.

I think there is a reason: to maintain backwards compatibility. I mean, not even outright bad and error-prone APIs like std::mem::unitialized are removed, and for a reason. I think deprecation is enough.

What about a deprecation lint that in Rust 2024 is warn by default, and in Rust 2027 and later is deny by default? This is practically the same as a removal, but with a escape hatch. Maybe the RFC could list this as an alternative.

This covers the case where someone has perfectly sound and working static mut code, doesn't want to modify it to use SyncUnsafeCell, but would like to upgrade to Rust 2027 (or a later edition) because some other feature entirely unrelated to this issue.

Hi Elias,

I don't understand exactly what it is that you're trying to escape hatch here. Could you please clarify? Also, std::mem::uninitialized is not really a good analogue as it's from the standard library, not the language.

@Lokathor
Copy link
Contributor

I think their final paragraph says exactly the case:

This covers the case where someone has perfectly sound and working static mut code, doesn't want to modify it to use SyncUnsafeCell, but would like to upgrade to Rust 2027 (or a later edition) because some other feature entirely unrelated to this issue.

As to standard library vs language, it actually doesn't matter. A break is a break, and people don't care much about splitting hairs once their code is broken.

@dyslexicsteak
Copy link
Author

I think their final paragraph says exactly the case:

This covers the case where someone has perfectly sound and working static mut code, doesn't want to modify it to use SyncUnsafeCell, but would like to upgrade to Rust 2027 (or a later edition) because some other feature entirely unrelated to this issue.

As to standard library vs language, it actually doesn't matter. A break is a break, and people don't care much about splitting hairs once their code is broken.

Oh, I'd assumed the standard library and the language would have different policies for breaking things.

If the case we want to escape hatch is not wanting to port your code to use interior mutability for any reason then we can add the attribute described as an option in the RFC where:

pub static mut MY_STATIC: Ty = ...;
// becomes
#[legacy_static_mut]
pub static MY_STATIC: Ty = ...;

This is easily cargo fix-able comparing to type changes and means the syntax itself is removed but you're still able to get the same type of behaviour the static mut syntax provided before.

@workingjubilee
Copy link
Contributor

workingjubilee commented Jan 31, 2024

Oh, I'd assumed the standard library and the language would have different policies for breaking things.

Given the RFC proposes edition-gating the warning, as long as SUC (or whatever other API is proposed) is stabilized by the first release of the 2024 edition there should be no MSRV impact, you won't be able to have a crate with an MSRV that results in both getting the warning (on any version of the compiler) and not having a way to resolve it.

Wait, are you not mandated to be able to use future edition crate versions?

I don't understand this question? Each crate defines its own edition.

@nikomatsakis
Copy link
Contributor

I would prefer to simply deprecate it across all editions and leave possible 2027 removal as something to be decided on the next edition. This fits with our general strategy, which is that we try to make behavior across editions as uniform as we can.

TL;DR the reason to deprecate in an edition (to me) is if the solution is only present in 1 edition. If the solution applies in all editions (and I think it does), then let's do it uniformly.

The question then is:

  • what is the recommended replacement pattern and how preferable / rustfix-able is it?
  • what kind of fallout do we know about when it comes to e.g. crater runs etc?

@dyslexicsteak
Copy link
Author

dyslexicsteak commented Jan 31, 2024

I would prefer to simply deprecate it across all editions and leave possible 2027 removal as something to be decided on the next edition. This fits with our general strategy, which is that we try to make behavior across editions as uniform as we can.

TL;DR the reason to deprecate in an edition (to me) is if the solution is only present in 1 edition. If the solution applies in all editions (and I think it does), then let's do it uniformly.

The question then is:

  • what is the recommended replacement pattern and how preferable / rustfix-able is it?
  • what kind of fallout do we know about when it comes to e.g. crater runs etc?

Good point about deprecation. I'll make that clear in the RFC. About removal, yeah, we can remove that from this RFC and make another one in 2026 or so, the result is (hopefully) the same.

As discussed in the RFC, SyncUnsafeCell is about the closest thing we have that has an issue open on rust-lang/rust, it's not exact parity because it's bound on T: Sync for its unsafe impl. The actual closest thing is AssertThreadSafe but the issue for that is up on T-libs' repo. As for the recommended replacement it's actually using synchronisation if possible, but if you can't then these types should suffice.

As for cargo fixability it's a bit problematic, I discussed a few options in the RFC but none are that great.

We don't have any actual code yet so no crater runs but I did some code searches and there's a lot of pub static mut but it's not necessarily consequential for Rust code as it's not necessarily accessible from outside the crate. Internal static mut is also quite prolific but it's relatively easy to fix. no_core would break even more as it would have no way to have a mutable global anymore but I don't think anybody uses it because nearly nothing works.

@traviscross
Copy link
Contributor

We discussed this in the T-lang triage meeting today. Our consensus was in favor of what @nikomatsakis described here. We would like to see the proposal modified to specify making this lint fire on all editions subject to first ensuring that we have stabilized and made available the replacement solutions that people will need.

We agreed that Rust 2024 would be too early to make this a hard error and that we might consider it for later editions.

High level, this is something we've discussed for some time and something that we're interested in doing. Thanks @dyslexicsteak for putting together a concrete proposal here.

@dyslexicsteak
Copy link
Author

We discussed this in the T-lang triage meeting today. Our consensus was in favor of what @nikomatsakis described here. We would like to see the proposal modified to specify making this lint fire on all editions subject to first ensuring that we have stabilized and made available the replacement solutions that people will need.

We agreed that Rust 2024 would be too early to make this a hard error and that we might consider it for later editions.

High level, this is something we've discussed for some time and something that we're interested in doing. Thanks @dyslexicsteak for putting together a concrete proposal here.

Alright, understood. Shall I modify the PR name and to reflect a reduction of the RFC's scope i.e. removal left as an encouraged future possibility but not an integral part of the RFC?

@LegionMammal978
Copy link
Contributor

LegionMammal978 commented Jan 31, 2024

Reading the discussion around this RFC, I've been wondering: why can't the already-implemented rust-lang/rust#117556 (deprecating &STATIC_MUT and &mut STATIC_MUT for removal), or an expansion, be considered as an alternative to this? The main threat imposed by static mut is that people could unwittingly create aliasing & or &mut references to the same static, since the syntax for creating a reference is very simple, and analogous to let mut bindings. But with the implementation of rust-lang/rust#117556, that part becomes a non-issue, since users are simply no longer allowed to do that. (Well, mostly a non-issue, since the PR still allows mutable statics to be copied from and assigned to. But it could easily be expanded to cover those cases as well.)

In a world where the only way to reference a static mut is through addr_of_mut!(STATIC_MUT), I don't see what remains so problematic about static muts that they ought to be removed wholly, in favor of a static SuperUnsafeCell<T> that asserts Send and Sync. The syntax is no less tedious: unsafe { &mut *addr_of_mut!(STATIC_MUT) } is even longer than unsafe { &mut *STATIC_CELL.get() }. The step of interior mutability seems redundant here, since there's no reason that a user would need to go through a safe & reference in the first place; the ultimate &*ptr or &mut *ptr step has the exact same rules in either case, distinct from the rules around UnsafeCell. And keeping static mut, allowing it to be accessed through the addr_of_*!() macros, would avoid all the stability issues around replacing it in crates' public APIs.

What makes raw-pointer-only static muts so dangerous that they must be removed?

@dyslexicsteak
Copy link
Author

dyslexicsteak commented Jan 31, 2024

Reading the discussion around this RFC, I've been wondering: why can't the already-implemented rust-lang/rust#117556 (deprecating &STATIC_MUT and &mut STATIC_MUT for removal), or an expansion, be considered as an alternative to this? The main threat imposed by static mut is that people could unwittingly create aliasing & or &mut references to the same static, since the syntax for creating a reference is very simple, and analogous to let mut bindings. But with the implementation of rust-lang/rust#117556, that part becomes a non-issue, since users are simply no longer allowed to do that. (Well, mostly a non-issue, since the PR still allows mutable statics to be copied from and assigned to. But it could easily be expanded to cover those cases as well.)

In a world where the only way to reference a static mut is through addr_of_mut!(STATIC_MUT), I don't see what remains so problematic about static muts that they ought to be removed wholly in favor of a static SuperUnsafeCell<T> that asserts Send and Sync. The syntax is no less tedious: unsafe { &mut *addr_of_mut!(STATIC_MUT) } is even longer than unsafe { &mut *STATIC_CELL.get() }. The step of interior mutability seems redundant here, since there's no reason that a user would need to go through a safe & reference in the first place; the ultimate &*ptr or &mut *ptr step has the exact same rules in either case, distinct from the rules around UnsafeCell. And keeping static mut, allowing it to be accessed through the addr_of_*!() macros, would avoid all the stability issues around replacing it in crates' public APIs.

What makes raw-pointer-only static muts so dangerous that they must be removed?

My take is that although after reducing its potency by deprecating references to it it becomes similar to SUC, it's still essentially the nuclear option as a language feature. I don't think it has to be worse than SUC in a vacuum to be deprecated; in the context of the plethora of other interior mutability types that are much more specific on intent, there isn't a reason to continue to support static mut anymore besides compatibility.

Removing wholly can be discussed later on, as I'm most likely reducing the scope of this RFC to exclude that and relegate it to a future possibility.

@traviscross traviscross changed the title Deprecate then remove static mut Deprecate static mut Jan 31, 2024
@traviscross
Copy link
Contributor

Shall I modify the PR name and to reflect a reduction of the RFC's scope i.e. removal left as an encouraged future possibility but not an integral part of the RFC?

Yes. I went ahead and removed the "remove" part. Feel free to edit further if we can be more clear.

Reading the discussion around this RFC, I've been wondering: why can't the already-implemented rust-lang/rust#117556 (deprecating &STATIC_MUT and &mut STATIC_MUT for removal), or an expansion, be considered as an alternative to this?

...

What makes raw-pointer-only static muts so dangerous that they must be removed?

It seems likely to me that this question will come up in further lang team discussion. It would be valuable, I would suggest @dyslexicsteak, for this proposed RFC to discuss and analyze this point in considerable detail (i.e. extending further your brief remarks above).

@VitWW
Copy link

VitWW commented Feb 2, 2024

I went ahead and removed the "remove" part. Feel free to edit further if we can be more clear.

You still need to clean the body of RFC from "removal part" and maybe insert part of them into "future possibilities"

@dyslexicsteak
Copy link
Author

I went ahead and removed the "remove" part. Feel free to edit further if we can be more clear.

You still need to clean the body of RFC from "removal part" and maybe insert part of them into "future possibilities"

No, I'm working on it.

Rectify a quoting error
@traviscross
Copy link
Contributor

@rustbot labels -A-edition-2024 +A-maybe-next-edition

We discussed the timeline of edition items in the lang meeting today, and this didn't make the list, so it seems unlikely this will happen for Rust 2024. We'll keep this in mind for future editions. Thanks to everyone working on this and providing helpful feedback here.

@rustbot rustbot added A-maybe-future-edition Changes of edition-relevance not targeted for a specific edition. and removed A-edition-2024 Area: The 2024 edition labels Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-maybe-future-edition Changes of edition-relevance not targeted for a specific edition. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
Status: Idea
Development

Successfully merging this pull request may close these issues.

None yet