Uplift clippy::option_env_unwrap to rustc

Author: Urgau

compiler/rustc_lint/messages.ftl

@@ -31,6 +31,9 @@ lint_map_unit_fn = `Iterator::map` call that discard the iterator's values
     .map_label = after this call to map, the resulting iterator is `impl Iterator<Item = ()>`, which means the only information carried by the iterator is the number of items
     .suggestion = you might have meant to use `Iterator::for_each`
 
+lint_option_env_unwrap = incorrect usage of `option_env!`, it will panic at run-time if the environment variable doesn't exist at compile-time
+    .suggestion = consider using the `env!` macro instead
+
 lint_non_binding_let_on_sync_lock =
     non-binding let on a synchronization lock
 

compiler/rustc_lint/src/lib.rs

@@ -72,6 +72,7 @@ mod non_fmt_panic;
 mod nonstandard_style;
 mod noop_method_call;
 mod opaque_hidden_inferred_bound;
+mod option_env_unwrap;
 mod pass_by_value;
 mod passes;
 mod redundant_semicolon;
@@ -111,6 +112,7 @@ use non_fmt_panic::NonPanicFmt;
 use nonstandard_style::*;
 use noop_method_call::*;
 use opaque_hidden_inferred_bound::*;
+use option_env_unwrap::*;
 use pass_by_value::*;
 use redundant_semicolon::*;
 use traits::*;
@@ -211,6 +213,7 @@ late_lint_methods!(
             BoxPointers: BoxPointers,
             PathStatements: PathStatements,
             LetUnderscore: LetUnderscore,
+            OptionEnvUnwrap: OptionEnvUnwrap,
             // Depends on referenced function signatures in expressions
             UnusedResults: UnusedResults,
             NonUpperCaseGlobals: NonUpperCaseGlobals,

compiler/rustc_lint/src/lints.rs

@@ -801,6 +801,27 @@ pub struct MappingToUnit {
     pub replace: String,
 }
 
+// option_env_unwrap.rs
+#[derive(LintDiagnostic)]
+#[diag(lint_option_env_unwrap)]
+pub struct OptionEnvUnwrapDiag {
+    #[subdiagnostic]
+    pub suggestion: Option<OptionEnvUnwrapSuggestion>,
+}
+
+#[derive(Subdiagnostic)]
+#[suggestion(
+    lint_suggestion,
+    style = "verbose",
+    code = "env!({replace})",
+    applicability = "machine-applicable"
+)]
+pub struct OptionEnvUnwrapSuggestion {
+    #[primary_span]
+    pub span: Span,
+    pub replace: String,
+}
+
 // internal.rs
 #[derive(LintDiagnostic)]
 #[diag(lint_default_hash_types)]

compiler/rustc_lint/src/option_env_unwrap.rs

@@ -0,0 +1,95 @@
+use rustc_hir::{Expr, ExprKind};
+use rustc_session::{declare_lint, declare_lint_pass};
+use rustc_span::{sym, ExpnKind, MacroKind, Span};
+
+use crate::{
+    lints::{OptionEnvUnwrapDiag, OptionEnvUnwrapSuggestion},
+    LateContext, LateLintPass, LintContext,
+};
+
+declare_lint! {
+    /// The `incorrect_option_env_unwraps` lint checks for usage of
+    /// `option_env!(...).unwrap()` and suggests using the `env!` macro instead.
+    ///
+    /// ### Example
+    ///
+    /// ```rust,no_run
+    /// let _ = option_env!("HOME").unwrap();
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### 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.
+    INCORRECT_OPTION_ENV_UNWRAPS,
+    Warn,
+    "using `option_env!(...).unwrap()` to get environment variable"
+}
+
+declare_lint_pass!(OptionEnvUnwrap => [INCORRECT_OPTION_ENV_UNWRAPS]);
+
+impl<'tcx> LateLintPass<'tcx> for OptionEnvUnwrap {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
+        let ExprKind::MethodCall(_, receiver, _, _) = expr.kind else { return; };
+
+        let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) else { return; };
+
+        if !matches!(
+            cx.tcx.get_diagnostic_name(method_def_id),
+            Some(sym::option_unwrap | sym::option_expect)
+        ) {
+            return;
+        }
+
+        // Handle found environment variable `Option::Some(...)`
+        let caller_span = if let ExprKind::Call(caller, _) = &receiver.kind {
+            caller.span
+        // Handle not found environment variable `Option::None`
+        } else if let ExprKind::Path(qpath) = &receiver.kind {
+            qpath.span()
+        } else {
+            return;
+        };
+
+        if is_direct_expn_of_option_env(caller_span) {
+            cx.emit_spanned_lint(
+                INCORRECT_OPTION_ENV_UNWRAPS,
+                expr.span,
+                OptionEnvUnwrapDiag {
+                    suggestion: extract_inner_macro_call(cx, caller_span)
+                        .map(|replace| OptionEnvUnwrapSuggestion { span: expr.span, replace }),
+                },
+            )
+        }
+    }
+}
+
+fn is_direct_expn_of_option_env(span: Span) -> bool {
+    if span.from_expansion() {
+        let data = span.ctxt().outer_expn_data();
+
+        if let ExpnKind::Macro(MacroKind::Bang, mac_name) = data.kind {
+            return mac_name == sym::option_env;
+        }
+    }
+
+    false
+}
+
+/// Given a Span representing a macro call: `option_env! ( \"j)j\")` get the inner
+/// content, ie. ` \"j)j\"`
+fn extract_inner_macro_call(cx: &LateContext<'_>, span: Span) -> Option<String> {
+    let snippet = cx.sess().parse_sess.source_map().span_to_snippet(span).ok()?;
+
+    let mut inner = snippet.chars().skip_while(|c| !"([{".contains(*c)).collect::<String>();
+
+    // remove last character, ie one of `)]}`
+    inner.pop()?;
+    // remove first character, ie one of `([{`
+    inner.remove(0);
+
+    Some(inner)
+}

tests/ui/lint/option_env_unwrap.rs

@@ -0,0 +1,30 @@
+// check-pass
+
+fn main() {
+    let _ = option_env!("PATH").unwrap();
+    //~^ WARN incorrect usage of `option_env!`
+    let _ = option_env!("PATH").expect("environment variable PATH isn't set");
+    //~^ WARN incorrect usage of `option_env!`
+    let _ = option_env!("NOT_IN_ENV").unwrap();
+    //~^ WARN incorrect usage of `option_env!`
+    let _ = option_env!("NOT_IN_ENV").expect("environment variable NOT_IN_ENV isn't set");
+    //~^ WARN incorrect usage of `option_env!`
+    let _ = assert_ne!(option_env!("PATH").unwrap(), "a");
+    //~^ WARN incorrect usage of `option_env!`
+
+    // just verify suggestion
+    let _ = option_env!("PATH")
+    //~^ WARN incorrect usage of `option_env!`
+        .unwrap();
+    let _ = option_env!(
+    //~^ WARN incorrect usage of `option_env!`
+            "PATH"
+        )
+        . unwrap();
+    let _ = (option_env!("NOT_IN_ENV")).expect("aaa");
+    //~^ WARN incorrect usage of `option_env!`
+
+    // should not lint
+    let _ = option_env!("PATH").unwrap_or("...");
+    let _ = option_env!("PATH").unwrap_or_else(|| "...");
+}

tests/ui/lint/option_env_unwrap.stderr

@@ -0,0 +1,102 @@
+warning: incorrect usage of `option_env!`, it will panic at run-time if the environment variable doesn't exist at compile-time
+  --> $DIR/option_env_unwrap.rs:4:13
+   |
+LL |     let _ = option_env!("PATH").unwrap();
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: `#[warn(incorrect_option_env_unwraps)]` on by default
+help: consider using the `env!` macro instead
+   |
+LL |     let _ = env!("PATH");
+   |             ~~~~~~~~~~~~
+
+warning: incorrect usage of `option_env!`, it will panic at run-time if the environment variable doesn't exist at compile-time
+  --> $DIR/option_env_unwrap.rs:6:13
+   |
+LL |     let _ = option_env!("PATH").expect("environment variable PATH isn't set");
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: consider using the `env!` macro instead
+   |
+LL |     let _ = env!("PATH");
+   |             ~~~~~~~~~~~~
+
+warning: incorrect usage of `option_env!`, it will panic at run-time if the environment variable doesn't exist at compile-time
+  --> $DIR/option_env_unwrap.rs:8:13
+   |
+LL |     let _ = option_env!("NOT_IN_ENV").unwrap();
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: consider using the `env!` macro instead
+   |
+LL |     let _ = env!("NOT_IN_ENV");
+   |             ~~~~~~~~~~~~~~~~~~
+
+warning: incorrect usage of `option_env!`, it will panic at run-time if the environment variable doesn't exist at compile-time
+  --> $DIR/option_env_unwrap.rs:10:13
+   |
+LL |     let _ = option_env!("NOT_IN_ENV").expect("environment variable NOT_IN_ENV isn't set");
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: consider using the `env!` macro instead
+   |
+LL |     let _ = env!("NOT_IN_ENV");
+   |             ~~~~~~~~~~~~~~~~~~
+
+warning: incorrect usage of `option_env!`, it will panic at run-time if the environment variable doesn't exist at compile-time
+  --> $DIR/option_env_unwrap.rs:12:24
+   |
+LL |     let _ = assert_ne!(option_env!("PATH").unwrap(), "a");
+   |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: consider using the `env!` macro instead
+   |
+LL |     let _ = assert_ne!(env!("PATH"), "a");
+   |                        ~~~~~~~~~~~~
+
+warning: incorrect usage of `option_env!`, it will panic at run-time if the environment variable doesn't exist at compile-time
+  --> $DIR/option_env_unwrap.rs:16:13
+   |
+LL |       let _ = option_env!("PATH")
+   |  _____________^
+LL | |
+LL | |         .unwrap();
+   | |_________________^
+   |
+help: consider using the `env!` macro instead
+   |
+LL |     let _ = env!("PATH");
+   |             ~~~~~~~~~~~~
+
+warning: incorrect usage of `option_env!`, it will panic at run-time if the environment variable doesn't exist at compile-time
+  --> $DIR/option_env_unwrap.rs:19:13
+   |
+LL |       let _ = option_env!(
+   |  _____________^
+LL | |
+LL | |             "PATH"
+LL | |         )
+LL | |         . unwrap();
+   | |__________________^
+   |
+help: consider using the `env!` macro instead
+   |
+LL ~     let _ = env!(
+LL +
+LL +             "PATH"
+LL ~         );
+   |
+
+warning: incorrect usage of `option_env!`, it will panic at run-time if the environment variable doesn't exist at compile-time
+  --> $DIR/option_env_unwrap.rs:24:13
+   |
+LL |     let _ = (option_env!("NOT_IN_ENV")).expect("aaa");
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: consider using the `env!` macro instead
+   |
+LL |     let _ = env!("NOT_IN_ENV");
+   |             ~~~~~~~~~~~~~~~~~~
+
+warning: 8 warnings emitted
+