Uplift clippy::option_env_unwrap lint

[Urgau]

This PR aims at uplifting the clippy::option_env_unwrap lint into rustc.

incorrect_option_env_unwraps

(warn-by-default)

The incorrect_option_env_unwraps lint checks for usage of option_env!(...).unwrap() and suggests using the env! macro instead.

Example

let _ = option_env!("HOME").unwrap();

Explanation

Unwrapping the result of option_env! will panic at run-time if the environment variable doesn't exist, whereas env! catches it at compile-time.

---

Mostly followed the instructions for uplifting a clippy lint described here: #99696 (review)

@rustbot label: +I-lang-nominated
r? compiler

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[bors]

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

[bors]

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

[nikomatsakis]

@rfcbot fcp close

We discussed this in the lang-team meeting and felt it did not meet the rustc bar of preventing bugs or helping to shape ecosystem wide consistency (like naming conventions).

We would be interested in some kind of "custom lint" mechanism. Ideally this would be a pattern-matching scheme that would ecosystem crates provide this sort of lint. A more limited thing might be something like #[must_use] (e.g., #[prefer_on_unwrap("env!")], but some members of the team were skeptical of such a narrow purpose attribute. Regardless that would be a separate proposal that would ultimately require an RFC.

The motivation here is that we think "usage lints" like this add value, but to do so, you need an awful lot of them, and we think the best way to get that is to let people add them themselves. Otherwise, clippy is a better home.

[nikomatsakis]

@rfcbot fcp cancel

@rustbot labels +T-lang -T-compiler

[rfcbot]

@nikomatsakis proposal cancelled.

[nikomatsakis]

@rfcbot fcp close

We discussed this in the lang-team meeting and felt it did not meet the rustc bar of preventing bugs or helping to shape ecosystem wide consistency (like naming conventions).

We would be interested in some kind of "custom lint" mechanism. Ideally this would be a pattern-matching scheme that would ecosystem crates provide this sort of lint. A more limited thing might be something like #[must_use] (e.g., #[prefer_on_unwrap("env!")], but some members of the team were skeptical of such a narrow purpose attribute. Regardless that would be a separate proposal that would ultimately require an RFC.

The motivation here is that we think "usage lints" like this add value, but to do so, you need an awful lot of them, and we think the best way to get that is to let people add them themselves. Otherwise, clippy is a better home.

[rfcbot]

Team member @nikomatsakis has proposed to close this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @tmandry

Concerns:

• move-errors-left (Uplift clippy::option_env_unwrap lint #111738 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[scottmcm]

Agreed that "you could write this in a better way" lints -- particularly about specific functions rather than language constructs -- are a better fit for a more stylistically inclined tool (like clippy!) rather than natively in rustc itself.

@rfcbot reviewed

Clippy has lots of helpful lints about specific things that I don't think I'd want to bring into rustc. My go-to example is .step_by(0), which will always panic, so clippy helpfully warns about it. But the set of such things is essentially never-ending, and thus I'd much rather see them move to some general feature that libraries (including the standard library!) use to get those warnings, rather than needing to manually write out the thir-matching code for all of them. And ideally that would let r-a pick them up too!

[Urgau]

While I agree with the arguments presented, I would just like to mention that the changes proposed by this lint are not stylistics!

Using env!(..) instead of option_env!(..).unwrap() makes the compiler throw an compilation error for the former and a run-time exception for the later. Which can make a huge difference in build script, having a compiler error is much nicer than a exception, the situation is even worse when a binary is installed via cargo install and expect a env at compile time as the binary may crash with some options leading to a sub-par experience for everyone.

[joshtriplett]

Given that @nikomatsakis originally started the FCP to close, with the motivation "did not meet the rustc bar of preventing bugs", and given the point @Urgau made and @nikomatsakis supported that this is in fact catching something that's almost always a bug, I'm going to cancel the FCP to close, and start an FCP to merge to see if we have consensus in that direction.

(Eagerly awaiting the rustbot or rfcbot functionality for tracking statuses by person and not predisposing in one direction or the other.)

@rfcbot cancel
@rfcbot merge

[rfcbot]

@joshtriplett proposal cancelled.

[rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @tmandry

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[RalfJung]

FWIW, there are rare situations where this lint is a false positive. So maybe this shouldn't be called incorrect_X since rustc can't be sure that this is incorrect?

[Urgau]

FWIW, there are rare situations where this lint is a false positive.

Do you have an example where someone might legitimately prefer to panic at run-time instead of having a compile-time error for a compile-time environment variable?

[RalfJung]

Yes, we used to have such code in Miri. Basically for the same reason that people use panic! instead of compile_error! one might also want option_env!(...).unwrap() instead of env!.

In the Miri case the situation was of the sort "if env var A is not set at build time then env var B must be set", which then can look like

if let Some(a) = option_env!("A") {
  // ...
} else {
  let b = option_env!("B").unwrap();
  // ...
}

Using env!("B") would clearly be wrong as then the build would fail if A is set and B is not.

[Urgau]

Interesting example. The minimized code is indeed an issue, and the wording should be soften.
It's worth noting that the original code from Miri would not be linted against since it's using unwrap_or_else (and not unwrap/expect).

Concerning the lint name, what about suspicious_option_env_unwraps?

[RalfJung]

suspicious works for me. Do we have precedent in other lints that we can follow?

[alexcrichton]

As a drive-by comment I personally was thinking of the same situation that @RalfJung brought up and given that the lint can have false positives I would personally say it shouldn't be an on-by-default lint. I know I feel differently about lints than many others, though, but my thinking is that rustc lints should never have false positives in general.

[joshtriplett]

Based on discussion in today's lang meeting, we reluctantly agreed that since this does have the potential for false positives it probably shouldn't be a rustc lint.

@rfcbot cancel
@rfcbot close

[rfcbot]

@joshtriplett proposal cancelled.

[rfcbot]

Team member @joshtriplett has proposed to close this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @tmandry

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[nagisa]

Using env!(..) instead of option_env!(..).unwrap() makes the compiler throw an compilation error for the former and a run-time exception for the later.

That's not necessarily the case. Panics in consteval contexts still become compilation errors, and in some of codebases I work on I and my team had converged towards using the pattern like try_fallible_op().unwrap() or try_fallible_op().expect() rather than the panicking_op() if only to have an easier time grepping for these panic locations. Adding the two ideas together we’d lose nothing over plain env! while retaining general consistency.

(But my comment is just a more specific example of “this is a style lint” that you were responding to.)

[rfcbot]

The final comment period, with a disposition to close, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.