diff --git a/CHANGELOG.md b/CHANGELOG.md index 0953432bf..3658df88d 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index 96584dfef..fd7f33f91 100644 --- a/README.md +++ b/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: diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 5084e4e59..f900a99f3 100644 --- a/clippy_lints/src/lib.rs +++ b/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(®ex::INVALID_REGEX), LintId::of(&serde_api::SERDE_API_MISUSE), diff --git a/clippy_lints/src/option_env_unwrap.rs b/clippy_lints/src/option_env_unwrap.rs new file mode 100644 index 000000000..1af779349 --- /dev/null +++ b/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" + ); + } + } + } +} diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index f725ce256..d4f94a9b6 100644 --- a/src/lintlist/mod.rs +++ b/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", diff --git a/tests/ui/auxiliary/macro_rules.rs b/tests/ui/auxiliary/macro_rules.rs index eafc68bd6..0bbb95349 100644 --- a/tests/ui/auxiliary/macro_rules.rs +++ b/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) + }; +} diff --git a/tests/ui/option_env_unwrap.rs b/tests/ui/option_env_unwrap.rs new file mode 100644 index 000000000..642c77460 --- /dev/null +++ b/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"); +} diff --git a/tests/ui/option_env_unwrap.stderr b/tests/ui/option_env_unwrap.stderr new file mode 100644 index 000000000..8de9c8a9d --- /dev/null +++ b/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 +