Make std::env::{set_var, remove_var} unsafe in edition 2024

[tbu-]

Allow calling these functions without unsafe blocks in editions up until 2021, but don't trigger the unused_unsafe lint for unsafe blocks containing these functions.

Fixes #27970.
Fixes #90308.
CC #124866.

[rustbot]

r? @joboet

rustbot has assigned @joboet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

Some changes occurred in src/tools/compiletest

cc @jieyouxu

The Miri subtree was changed

cc @rust-lang/miri

[rustbot]

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in src/tools/compiletest

cc @jieyouxu

[RalfJung]

Should there be a warning on older editions, to make this not come entirely out of the blue when doing edition migration?

[tbu-]

Should there be a warning on older editions, to make this not come entirely out of the blue when doing edition migration?

I agree that this would be quite useful. It seems hard to do right now, though, because a stage 0 compiler will detect a lot of unused unsafe blocks because it doesn't know about the std::env::set_var unsafety. Perhaps this could be postponed until after one bootstrap cycle?

[RalfJung]

I agree that this would be quite useful. It seems hard to do right now, though, because a stage 0 compiler will detect a lot of unused unsafe blocks because it doesn't know about the std::env::set_var unsafety. Perhaps this could be postponed until after one bootstrap cycle?

I wasn't talking about unused_unsafe, but a lint that warns against using set_var/remove_var outside an unsafe block on old editions.

Also, I am not sure if it's the best idea to add new lang items for these functions. In the past the proposal was to add an attribute for "deprecating safety". Then we could e.g. also add that attribute to before_exec.

[BoxyUwU]

This had the relnotes label added but as far as I can tell this only affects 2024 edition. Is that understanding correct or is there a subtle stable affecting change here :3

[beetrees]

There is a change affecting other editions: the std::env::{set_var,remove_var} functions can no longer be converted to safe function pointers in any edition. For example, both of the following examples compile on current stable but not on current nightly:

use std::ffi::OsString;

fn main() {
	let x: fn(OsString, OsString) = std::env::set_var;
}

and

use std::ffi::OsString;

fn f(x: impl Fn(OsString, OsString)) {}

fn main() {
	f(std::env::set_var);
}

[joshtriplett]

This will be incorrect if we mark any other functions with this attribute where the unsafety is for a different reason.

[tbu-]

Yes, it'll be fixed before #125970 is merged. This PR was intentionally minimal so it could definitely make it into the Rust 2024 edition.

[joshtriplett]

@beetrees Yikes, that doesn't seem right. That sounds like a bug in the implementation of rustc_deprecated_safe_2024; If you're allowed to treat the function as safe, you should be allowed to do anything with it you could have with a safe function, including passing it as an argument.

[RalfJung]

That was a deliberate choice evaluated with crater. From my understanding, it is quite tricky to hook into the type system to achieve this, and it is what stalled the previous attempt at implementing this.

[cuviper]

For safe function pointers, a workaround is that you can wrap them in a safe closure.

let x: fn(OsString, OsString) = |name, value| std::env::set_var(name, value);

It's unfortunate, but maybe someone will figure out how to make this unnecessary later. Or it will just become moot after everyone migrates to 2024+ and they really are unsafe.

[marcospb19]

so the only safe option is to not use set_var or remove_var in multi-threaded programs at all

Can't we use a static Mutex to guard env reads and writes? If so, saying that "the only option is not to use this" sounds misleading.

[ChrisDenton]

No we cannot. There's a lot of discussion about that on the linked issues but in short that would only make it safe for people sharing the same Mutex and then only if all FFI was guarded by that Mutex (because any libc function is allowed to read the environment).

[RalfJung]

Would have been good to add a comment explaining why this is no_run. Are you worried about UB in the doctest?

View changes since the review

[tbu-]

Yes, I'm worried about that. The doctest has no safety documentation, and I wouldn't know how I could prove that no other thread is running concurrently to it.

[ChrisDenton]

It is not safe to use in a doctest. We would need to use an integration test.

A doctest could work on platforms where set_var is thread safe but I'm not sure there's much utility in a test only for those platforms if we can have something that works cross-platform.