Deprecate set_var and remove_var in 1.58.0

[leo60228]

This is a stopgap solution for #90308 while a decision is made on how the underlying unsoundness should be resolved.

[rust-highfive]

r? @Mark-Simulacrum

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

[leo60228]

Seriously? I ran ./x.py test locally and it was fine...

[joshtriplett]

While I'm in support of such a deprecation, I'd prefer to 1) make a decision on whether to add an unsafe version to the standard library and 2) do so and stabilize it, so that people have a replacement that's in the standard library.

[Mark-Simulacrum]

I also think it's not necessarily the right approach to rush a deprecation out here; my impression is that the risk of segfaults (or UB) is real, but does not in practice occur that often.

I would also suggest that just providing a non-deprecated unsafe version is likely to not really motivate people to migrate away from env variables -- I expect almost all code that currently uses set_var probably did so "reluctantly" anyway (for lack of better options) and would likely end up either just sticking with an allow(deprecated) or an unsafe call with a "hope for the best" comment.

At minimum I would suggest we try to assess any std-used env variables and provide alternatives for setting them safely (e.g., RUST_BACKTRACE) from within the standard library interface. It's probably worthwhile to do a simple grep of crates.io at least (or all Crater-seen code) to see if there are other similar variables that would be good to suggest direct setters for (e.g., in other Rust libraries, perhaps even some popular C libraries with Rust bindings).

IOW, I think if we deprecate before there's an effective "what else to do", we don't actually win that much and only hurt ourselves by forcing people to push the problem under the covers (allow, unsafe, etc.) rather than actually fixing it if at all possible.

[leo60228]

I think this is fair in general, but I *think* the chrono issue was found when someone hit it in production.

…

On Mon, Nov 15, 2021, 7:08 PM Mark Rousskov ***@***.***> wrote: I also think it's not necessarily the right approach to rush a deprecation out here; my impression is that the risk of segfaults (or UB) is real, but does not in practice occur that often. I would also suggest that just providing a non-deprecated unsafe version is likely to not really motivate people to migrate away from env variables -- I expect almost all code that currently uses set_var probably did so "reluctantly" anyway (for lack of better options) and would likely end up either just sticking with an allow(deprecated) or an unsafe call with a "hope for the best" comment. At minimum I would suggest we try to assess any std-used env variables and provide alternatives for setting them safely (e.g., RUST_BACKTRACE) from within the standard library interface. It's probably worthwhile to do a simple grep of crates.io at least (or all Crater-seen code) to see if there are other similar variables that would be good to suggest direct setters for (e.g., in other Rust libraries, perhaps even some popular C libraries with Rust bindings). IOW, I think if we deprecate before there's an effective "what else to do", we don't actually win that much and only hurt ourselves by forcing people to push the problem under the covers (allow, unsafe, etc.) rather than actually fixing it if at all possible. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#90830 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB7X32PS253QAJCSJAY4H2DUMGOIHANCNFSM5H46EPPA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[Amanieu]

We discussed this in the libs-api meeting today.

We feel that the standard library should first offer an alternative (unsafe) API for manipulating the environment variables before deprecating the existing API. This is also a good opportunity to improve the API of set_var by making it return a Result (see #63456).

The standard library should also provide a proper API for controlling behavior that is currently controlled with environment variables. The main culprit here is RUST_BACKTRACE which should have an API added to control it at run-time.

[Mark-Simulacrum]

I'm going to mark this as waiting on author, though it may also make sense to mark it as S-blocked as there's several APIs which need to at least land on nightly (and probably ~stabilize) before we can move ahead here, per the libs-api discussion.

[leo60228]

Should I close this in favor of #92365?

[bors]

☔ The latest upstream changes (presumably #92396) made this pull request unmergeable. Please resolve the merge conflicts.