Auto merge of #5148 - krishna-veerareddy:issue-5147-option-env-unwrap, r=flip1995

Add `option-env-unwrap` lint

changelog: Add `option-env-unwrap` lint

Fixes #5147

CHANGELOG.md

@@ -1276,6 +1276,7 @@ Released 2018-09-13
 [`op_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#op_ref
 [`option_and_then_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_and_then_some
 [`option_as_ref_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_as_ref_deref
+[`option_env_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_env_unwrap
 [`option_expect_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_expect_used
 [`option_map_or_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_or_none
 [`option_map_unit_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_unit_fn

README.md

@@ -6,7 +6,7 @@
 
 A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
 
-[There are 354 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are 355 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
 
 We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:
 

clippy_lints/src/lib.rs

@@ -267,6 +267,7 @@ pub mod no_effect;
 pub mod non_copy_const;
 pub mod non_expressive_names;
 pub mod open_options;
+pub mod option_env_unwrap;
 pub mod overflow_check_conditional;
 pub mod panic_unimplemented;
 pub mod partialeq_ne_impl;
@@ -713,6 +714,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &non_expressive_names::MANY_SINGLE_CHAR_NAMES,
         &non_expressive_names::SIMILAR_NAMES,
         &open_options::NONSENSICAL_OPEN_OPTIONS,
+        &option_env_unwrap::OPTION_ENV_UNWRAP,
         &overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL,
         &panic_unimplemented::PANIC,
         &panic_unimplemented::PANIC_PARAMS,
@@ -1003,6 +1005,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     let max_fn_params_bools = conf.max_fn_params_bools;
     let max_struct_bools = conf.max_struct_bools;
     store.register_early_pass(move || box excessive_bools::ExcessiveBools::new(max_struct_bools, max_fn_params_bools));
+    store.register_early_pass(|| box option_env_unwrap::OptionEnvUnwrap);
 
     store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
         LintId::of(&arithmetic::FLOAT_ARITHMETIC),
@@ -1285,6 +1288,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&non_expressive_names::JUST_UNDERSCORES_AND_DIGITS),
         LintId::of(&non_expressive_names::MANY_SINGLE_CHAR_NAMES),
         LintId::of(&open_options::NONSENSICAL_OPEN_OPTIONS),
+        LintId::of(&option_env_unwrap::OPTION_ENV_UNWRAP),
         LintId::of(&overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL),
         LintId::of(&panic_unimplemented::PANIC_PARAMS),
         LintId::of(&partialeq_ne_impl::PARTIALEQ_NE_IMPL),
@@ -1590,6 +1594,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&non_copy_const::BORROW_INTERIOR_MUTABLE_CONST),
         LintId::of(&non_copy_const::DECLARE_INTERIOR_MUTABLE_CONST),
         LintId::of(&open_options::NONSENSICAL_OPEN_OPTIONS),
+        LintId::of(&option_env_unwrap::OPTION_ENV_UNWRAP),
         LintId::of(&ptr::MUT_FROM_REF),
         LintId::of(&regex::INVALID_REGEX),
         LintId::of(&serde_api::SERDE_API_MISUSE),

clippy_lints/src/option_env_unwrap.rs

@@ -0,0 +1,54 @@
+use crate::utils::{is_direct_expn_of, span_lint_and_help};
+use if_chain::if_chain;
+use rustc_lint::{EarlyContext, EarlyLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use syntax::ast::*;
+
+declare_clippy_lint! {
+    /// **What it does:** Checks for usage of `option_env!(...).unwrap()` and
+    /// suggests usage of the `env!` macro.
+    ///
+    /// **Why is this bad?** 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.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    ///
+    /// ```rust,no_run
+    /// let _ = option_env!("HOME").unwrap();
+    /// ```
+    ///
+    /// Is better expressed as:
+    ///
+    /// ```rust,no_run
+    /// let _ = env!("HOME");
+    /// ```
+    pub OPTION_ENV_UNWRAP,
+    correctness,
+    "using `option_env!(...).unwrap()` to get environment variable"
+}
+
+declare_lint_pass!(OptionEnvUnwrap => [OPTION_ENV_UNWRAP]);
+
+impl EarlyLintPass for OptionEnvUnwrap {
+    fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) {
+        if_chain! {
+            if let ExprKind::MethodCall(path_segment, args) = &expr.kind;
+            let method_name = path_segment.ident.as_str();
+            if method_name == "expect" || method_name == "unwrap";
+            if let ExprKind::Call(caller, _) = &args[0].kind;
+            if is_direct_expn_of(caller.span, "option_env").is_some();
+            then {
+                span_lint_and_help(
+                    cx,
+                    OPTION_ENV_UNWRAP,
+                    expr.span,
+                    "this will panic at run-time if the environment variable doesn't exist at compile-time",
+                    "consider using the `env!` macro instead"
+                );
+            }
+        }
+    }
+}

src/lintlist/mod.rs

@@ -6,7 +6,7 @@ pub use lint::Lint;
 pub use lint::LINT_LEVELS;
 
 // begin lint list, do not remove this comment, it’s used in `update_lints`
-pub const ALL_LINTS: [Lint; 354] = [
+pub const ALL_LINTS: [Lint; 355] = [
     Lint {
         name: "absurd_extreme_comparisons",
         group: "correctness",
@@ -1498,6 +1498,13 @@ pub const ALL_LINTS: [Lint; 354] = [
         deprecation: None,
         module: "methods",
     },
+    Lint {
+        name: "option_env_unwrap",
+        group: "correctness",
+        desc: "using `option_env!(...).unwrap()` to get environment variable",
+        deprecation: None,
+        module: "option_env_unwrap",
+    },
     Lint {
         name: "option_expect_used",
         group: "restriction",

tests/ui/auxiliary/macro_rules.rs

@@ -46,3 +46,13 @@ macro_rules! take_external {
         std::mem::replace($s, Default::default())
     };
 }
+
+#[macro_export]
+macro_rules! option_env_unwrap_external {
+    ($env: expr) => {
+        option_env!($env).unwrap()
+    };
+    ($env: expr, $message: expr) => {
+        option_env!($env).expect($message)
+    };
+}

tests/ui/option_env_unwrap.rs

@@ -0,0 +1,23 @@
+// aux-build:macro_rules.rs
+#![warn(clippy::option_env_unwrap)]
+
+#[macro_use]
+extern crate macro_rules;
+
+macro_rules! option_env_unwrap {
+    ($env: expr) => {
+        option_env!($env).unwrap()
+    };
+    ($env: expr, $message: expr) => {
+        option_env!($env).expect($message)
+    };
+}
+
+fn main() {
+    let _ = option_env!("PATH").unwrap();
+    let _ = option_env!("PATH").expect("environment variable PATH isn't set");
+    let _ = option_env_unwrap!("PATH");
+    let _ = option_env_unwrap!("PATH", "environment variable PATH isn't set");
+    let _ = option_env_unwrap_external!("PATH");
+    let _ = option_env_unwrap_external!("PATH", "environment variable PATH isn't set");
+}

tests/ui/option_env_unwrap.stderr

@@ -0,0 +1,61 @@
+error: this will panic at run-time if the environment variable doesn't exist at compile-time
+  --> $DIR/option_env_unwrap.rs:17:13
+   |
+LL |     let _ = option_env!("PATH").unwrap();
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::option-env-unwrap` implied by `-D warnings`
+   = help: consider using the `env!` macro instead
+
+error: this will panic at run-time if the environment variable doesn't exist at compile-time
+  --> $DIR/option_env_unwrap.rs:18:13
+   |
+LL |     let _ = option_env!("PATH").expect("environment variable PATH isn't set");
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using the `env!` macro instead
+
+error: this will panic at run-time if the environment variable doesn't exist at compile-time
+  --> $DIR/option_env_unwrap.rs:9:9
+   |
+LL |         option_env!($env).unwrap()
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^
+...
+LL |     let _ = option_env_unwrap!("PATH");
+   |             -------------------------- in this macro invocation
+   |
+   = help: consider using the `env!` macro instead
+   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: this will panic at run-time if the environment variable doesn't exist at compile-time
+  --> $DIR/option_env_unwrap.rs:12:9
+   |
+LL |         option_env!($env).expect($message)
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+...
+LL |     let _ = option_env_unwrap!("PATH", "environment variable PATH isn't set");
+   |             ----------------------------------------------------------------- in this macro invocation
+   |
+   = help: consider using the `env!` macro instead
+   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: this will panic at run-time if the environment variable doesn't exist at compile-time
+  --> $DIR/option_env_unwrap.rs:21:13
+   |
+LL |     let _ = option_env_unwrap_external!("PATH");
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using the `env!` macro instead
+   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: this will panic at run-time if the environment variable doesn't exist at compile-time
+  --> $DIR/option_env_unwrap.rs:22:13
+   |
+LL |     let _ = option_env_unwrap_external!("PATH", "environment variable PATH isn't set");
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using the `env!` macro instead
+   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: aborting due to 6 previous errors
+