Future deprecation of env::{set, remove}_var

[jhpratt]

At some point in the future it is quite likely that std::env::set_var and std::env::remove_var will be deprecated. There appears to be consensus that this should happen per a poll on IRLO. The only blocker to actually deprecating these methods is agreement on what should replace them. This PR serves two purposes: indicate they will be deprecated in documentation and trigger the deprecated_in_future lint.

@rustbot label +S-waiting-on-review +T-libs-api

[rust-highfive]

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

[rust-log-analyzer]

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

    Checking hashbrown v0.11.0
    Checking object v0.26.2
    Checking miniz_oxide v0.4.0
    Checking addr2line v0.16.0
error: use of function `env::remove_var` that will be deprecated in a future Rust version: method is unsound
   |
46 |                 env::remove_var(k);
   |                      ^^^^^^^^^^
   |
   |
   = note: `-D deprecated-in-future` implied by `-D warnings`

error: use of function `env::set_var` that will be deprecated in a future Rust version: method is unsound
   |
51 |                 env::set_var(key, val);
   |                      ^^^^^^^


error: use of function `env::remove_var` that will be deprecated in a future Rust version: method is unsound
   |
53 |                 env::remove_var(key);
   |                      ^^^^^^^^^^

[jhpratt]

Hmm…not sure how we can avoid triggering the lint. I'm fairly certain rustc itself needs environment variables.

[joshtriplett]

@jhpratt I think you'd have to allow the lint in those cases.

Also, rather than "method is unsound", I'd suggest "method cannot be made thread-safe".

[jhpratt]

How about "method is unsound because it cannot be made thread-safe"? I think any message without "unsound" is not ideal.

[bjorn3]

Can you link to the discussion about why it is unsound?

[jhpratt]

Which discussion is preferred? I'm pretty sure at this point we could assemble a book with the various discussions.

[bjorn3]

Good point. Maybe open a tracking issue to be refenced here and write a recap there together with all discussion you can find?

[leo60228]

#90308?

[briansmith]

Do you want to defer changing the documentation of this function to another PR, or should we do it here? If we're going to do it as part of this PR then I'm happy to suggest changes to the docs.

[joshtriplett]

@jhpratt Perhaps "method is unsound in multithreaded programs"?

EDIT: I like @mathstuf's suggestion even better: "method is unsound in the presence of threads".

[mathstuf]

How about "method is unsound in the presence of threads"? Multi-threaded programs may have periods where no other threads exist. It is also ok post-fork (where no threads exist anymore).

[KamilaBorowska]

It is also ok post-fork (where no threads exist anymore).

EDIT: This is actually incorrect, see later comment.

While technically true, Rust provides Command::{env, env_clear, env_remove, envs} for setting up the environment before spawning a process. I'm aware that fork is technically not only for spawning processes, however I think it's acceptable to require libc::setenv calls in programs that use libc::fork anyway.

[RalfJung]

trigger the deprecated_in_future lint.

I don't think we should trigger that lint without giving some guidance on what people should do instead. As-is, the likely thing people will do is allow that lint, because they do not really have any other good options. So for now I don't think it makes sense to trigger that lint.

[jhpratt]

@RalfJung I presume that would require altering the lint itself?

[RalfJung]

Well, it would require not adding the attribute. It might make sense to have some discussion of the issue in the doc comment and explain that a better replacement will come Eventually (TM), but I don't think we should add the attribute at this point.

[mathstuf]

While technically true, Rust provides Command::{env, env_clear, env_remove, envs} for setting up the environment before spawning a process.

I'm well aware of that, but there are other tasks which are not exposed via Command which may need to be done as well (ulimit modifications, dropping privs, pledge, capability setup, namespace setup, etc. all come to mind at least) which would require an explicit fork in practice. I think explicitly mentioning that the special "twilight zone" of post-fork, pre-exec is fine in the docs at least (it already has numerous restrictions anyways, so one place it is actually allowed to assume more would be useful).

[jhpratt]

So is this something that's currently wanted? I don't particularly mind either way; it's not like this was a difficult PR to put together. Simple 👍/👎 from people that are relevant I suppose?

[leo60228]

Maybe add an unstable copy of today's set_env as set_env_unsafe with a note that it's almost certainly subject to change before stabilizing?

[KamilaBorowska]

How about "method is unsound in the presence of threads"? Multi-threaded programs may have periods where no other threads exist. It is also ok post-fork (where no threads exist anymore).

Okay, I updated my previous comment to note it's incorrect. According to POSIX:

A process shall be created with a single thread. If a multi-threaded process calls fork(), the new process shall contain a replica of the calling thread and its entire address space, possibly including the states of mutexes and other resources. Consequently, to avoid errors, the child process may only execute async-signal-safe operations until such time as one of the exec functions is called.

setenv is not an async-signal-safe function, as such it's not guaranteed to work after fork in a multi-threaded program. As such, for setting up environment variables, you are better of using Command::{env, env_clear, env_remove, envs}. If you need to do additional set-up in addition to setting up environment variables, Rust provides pre_exec function specifically for that purpose, see https://doc.rust-lang.org/stable/std/os/unix/process/trait.CommandExt.html#tymethod.pre_exec.

[mathstuf]

setenv is not an async-signal-safe function, as such it's not guaranteed to work after fork in a multi-threaded program

Oh, indeed. There may be a lock behind that call that is still locked after the fork() I suppose. Fun.

[briansmith]

Hmm…not sure how we can avoid triggering the lint. I'm fairly certain rustc itself needs environment variables.

If rustc is setting or unsetting environment variables itself, either directly or indirectly via its dependencies, we should attempt to change that before or during the process of this deprecation. If the uses in rustc are too hard to remove then what hope do other users have?

[jhpratt]

Right now it's a future deprecation — it's not actually being deprecated in this PR. The lint is allow by default. Though I do agree that there needs to be a replacement before actually deprecating (and I believe there's consensus on that point).

[leo60228]

The lint is allow by default.

Yes, but rustc and libstd have #![warn(deprecated_in_future)].

[jhpratt]

That's why it's an issue for this PR but not others, generally speaking. Other users don't have worry about builds failing because of a future deprecation unless they've explicitly opted into the lint.

[RalfJung]

So is this something that's currently wanted? I don't particularly mind either way; it's not like this was a difficult PR to put together. Simple +1/-1 from people that are relevant I suppose?

To explain my 👎 : I think we should not add a deprecated attribute until the replacement is in place. I have no objection to documentation changes, though.

[joshtriplett]

This seems to be somewhat stalled.

I do think we should have at least some plan for what we intend to support as a replacement, though that replacement doesn't have to exist in stable Rust for us to add deprecated_in_future. (Such a requirement seems more appropriate for deprecated.)

At the moment, the only proposal I've seen that seems like a viable replacement is to add unsafe functions. Apart from bikeshedding the names of those methods (std::env::set vs std::env::set_var_unsafe for instance) I don't think there's another proposal on the table that would still provide a mechanism to access the environment.

Would someone be interested in submitting a PR adding unsafe get and set and unset functions? I think we could merge that PR (with the functions unstable), then merge this future-deprecation, then consider actual deprecation if and when we stabilize the replacement functions.

[jhpratt]

I can do that tonight. Should just amount to calling the same thing internally.

[RalfJung]

I think the intention is for get to remain safe and unchanged.

Only mutating the environment is unsafe.

[Mark-Simulacrum]

I believe the goal is to only mark env::set_var as unsafe, and to do so in-place (i.e., with a new custom deprecated_safe or similar attribute), based on the latest discussions in libs-api that I recall. My understanding is that we want to leave env::var safe in Rust's standard library, as the retrieval of environment variables is safe in correctly written programs (i.e., those that don't have any setenv/putenv/etc calls in the possibly MT section).

I think the next step before actually deprecating these would be to stabilize #93346, which tracks the RUST_BACKTRACE setting for std::panic. That's the only in-std API stable that relies on the environment, I believe. (Modulo env::home_dir and env::temp_dir, but those are not so interesting).

[jhpratt]

Yeah, get should remain safe. I'll leave that as-is in my PR.

[ghost]

@Mark-Simulacrum, is the implementation of a deprecated_safe attribute up for grabs? I suggested something like that on IRLO and I've been looking for an excuse to work on Rust. 😊 I've been following the thread but wasn't sure if that was the direction things would be going.

[jhpratt]

I'm not him, but to my knowledge no one has worked on such an attribute in any way.

[Mark-Simulacrum]

Yes, I think so, would be great to get someone to implement it. No guarantees on whether it gets used, of course, but I believe several folks are in favor of using it so chances are good.

[RalfJung]

FWIW we already have at least one example of a "deprecated safe" function: https://doc.rust-lang.org/nightly/std/os/unix/process/trait.CommandExt.html#method.before_exec. That one is just deprecated the normal way now. Not sure if it makes sense to also apply the new approach there.

Certainly if we plan to make an edition change here where calling them as safe functions becomes a hard error, we should also do that for before_exec.

[Mark-Simulacrum]

Part of the goal on the 'in-place' deprecation is that there's no nice alternative name (before_exec/pre_exec in the CommandExt case), so we're stuck with either using a nice name like env::set, or something with an awkward extra word -- e.g., set_unsafe_var -- and the in-place deprecation leaves the door open to the nice name being used later down the line for a safe API (but one that targets only some use cases, perhaps, like just affecting Rust code, or Rust code and C code using some new libc interfaces, etc.)

[jhpratt]

PR introducing the unsafe env::set and env::unset: #94619

[ghost]

FYI, I've been discussing work on the potential new #[rustc_deprecated_safe] attribute on Zulip here: https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/deprecated_safe.20attribute

[ghost]

I have some commits pushed here that (very, VERY roughly) implement a new #[rustc_deprecated_safe] attribute. I'm hesitant to post even a draft PR as the code quality is not at all up to snuff, but it'd be good know if the approach I'm taking is even valid before I make things proper. I've never touched rustc before, so I'm not really sure what I'm doing. :)

https://github.com/skippy10110/rust/commits/rustc_deprecated_safe

This covers the following places where making set_var unsafe would break existing code, I'm not sure if there are other places that need to be handled:

env::set_var("TEST", "test");

let set_var_fn_ptr: fn(OsString, OsString) = env::set_var;

let set_var_fn_impl: Box<dyn Fn(OsString, OsString)> = Box::new(env::set_var);
let mut set_var_fnmut_impl: Box<dyn FnMut(OsString, OsString)> = Box::new(env::set_var);
let set_var_fnonce_impl: Box<dyn FnOnce(OsString, OsString)> = Box::new(env::set_var);

P.S. I haven't updated the new THIR unsafety checker yet, I figured I'd wait until I knew if any of this code is even correct first.

[JohnCSimon]

ping from triage:
@jhpratt
Returning to you to build failures

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

@rustbot author

[jhpratt]

This is sort of in limbo. Basically there are no technical blockers, but it shouldn't land if #[deprecated_safe] lands. To my knowledge there hasn't been any progress on that front. It's in the same situation as #94619. Given that it's effectively waiting on something else, I'm going to mark it as blocked so that triage doesn't have to keep coming back to it.

@rustbot label -S-waiting-on-author +S-blocked