Skip to content

Commit

Permalink
Fix unwrap not to lint the #[test]ing code
Browse files Browse the repository at this point in the history
  • Loading branch information
chansuke committed Oct 4, 2020
1 parent 9408c68 commit d38fe0f
Show file tree
Hide file tree
Showing 6 changed files with 67 additions and 25 deletions.
2 changes: 1 addition & 1 deletion clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1021,7 +1021,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
store.register_late_pass(|| box map_unit_fn::MapUnit);
store.register_late_pass(|| box inherent_impl::MultipleInherentImpl::default());
store.register_late_pass(|| box neg_cmp_op_on_partial_ord::NoNegCompOpForPartialOrd);
store.register_late_pass(|| box unwrap::Unwrap);
store.register_late_pass(|| box unwrap::Unwrap::new());
store.register_late_pass(|| box duration_subsec::DurationSubsec);
store.register_late_pass(|| box default_trait_access::DefaultTraitAccess);
store.register_late_pass(|| box indexing_slicing::IndexingSlicing);
Expand Down
51 changes: 38 additions & 13 deletions clippy_lints/src/unwrap.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
use crate::utils::{
differing_macro_contexts, higher::if_block, is_type_diagnostic_item, span_lint_and_then,
usage::is_potentially_mutated,
differing_macro_contexts, higher::if_block, is_test_module_or_function, is_type_diagnostic_item,
span_lint_and_then, usage::is_potentially_mutated,
};
use if_chain::if_chain;
use rustc_hir::intravisit::{walk_expr, walk_fn, FnKind, NestedVisitorMap, Visitor};
use rustc_hir::{BinOpKind, Body, Expr, ExprKind, FnDecl, HirId, Path, QPath, UnOp};
use rustc_hir::{BinOpKind, Body, Expr, ExprKind, FnDecl, HirId, Item, Path, QPath, UnOp};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::hir::map::Map;
use rustc_middle::lint::in_external_macro;
use rustc_middle::ty::Ty;
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_session::{declare_tool_lint, impl_lint_pass};
use rustc_span::source_map::Span;

declare_clippy_lint! {
Expand Down Expand Up @@ -207,9 +207,25 @@ impl<'a, 'tcx> Visitor<'tcx> for UnwrappableVariablesVisitor<'a, 'tcx> {
}
}

declare_lint_pass!(Unwrap => [PANICKING_UNWRAP, UNNECESSARY_UNWRAP]);
#[derive(Default)]
pub struct Unwrap {
test_modules_deep: u32,
}

impl Unwrap {
pub fn new() -> Self {
Self { test_modules_deep: 0 }
}
}

impl_lint_pass!(Unwrap => [PANICKING_UNWRAP, UNNECESSARY_UNWRAP]);

impl<'tcx> LateLintPass<'tcx> for Unwrap {
fn check_item(&mut self, _: &LateContext<'_>, item: &Item<'_>) {
if is_test_module_or_function(item) {
self.test_modules_deep = self.test_modules_deep.saturating_add(1);
}
}
fn check_fn(
&mut self,
cx: &LateContext<'tcx>,
Expand All @@ -219,15 +235,24 @@ impl<'tcx> LateLintPass<'tcx> for Unwrap {
span: Span,
fn_id: HirId,
) {
if span.from_expansion() {
return;
if_chain! {
if !span.from_expansion();
if !self.check_test_module();
if let mut v = UnwrappableVariablesVisitor { cx, unwrappables: Vec::new(), };
then {
walk_fn(&mut v, kind, decl, body.id(), span, fn_id);
}
}
}
fn check_item_post(&mut self, _: &LateContext<'_>, item: &Item<'_>) {
if is_test_module_or_function(item) {
self.test_modules_deep = self.test_modules_deep.saturating_sub(1);
}
}
}

let mut v = UnwrappableVariablesVisitor {
cx,
unwrappables: Vec::new(),
};

walk_fn(&mut v, kind, decl, body.id(), span, fn_id);
impl Unwrap {
fn check_test_module(&self) -> bool {
self.test_modules_deep > 0
}
}
5 changes: 5 additions & 0 deletions clippy_lints/src/utils/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1440,6 +1440,11 @@ pub fn is_slice_of_primitives(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<S
None
}

// Check if item is #[test]ing code.
pub fn is_test_module_or_function(item: &Item<'_>) -> bool {
matches!(item.kind, ItemKind::Mod(..)) && item.ident.name.as_str().contains("test")
}

#[macro_export]
macro_rules! unwrap_cargo_metadata {
($cx: ident, $lint: ident, $deps: expr) => {{
Expand Down
6 changes: 1 addition & 5 deletions clippy_lints/src/wildcard_imports.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::utils::{in_macro, snippet, snippet_with_applicability, span_lint_and_sugg};
use crate::utils::{in_macro, is_test_module_or_function, snippet, snippet_with_applicability, span_lint_and_sugg};
use if_chain::if_chain;
use rustc_errors::Applicability;
use rustc_hir::{
Expand Down Expand Up @@ -205,7 +205,3 @@ fn is_prelude_import(segments: &[PathSegment<'_>]) -> bool {
fn is_super_only_import(segments: &[PathSegment<'_>]) -> bool {
segments.len() == 1 && segments[0].ident.as_str() == "super"
}

fn is_test_module_or_function(item: &Item<'_>) -> bool {
matches!(item.kind, ItemKind::Mod(..)) && item.ident.name.as_str().contains("test")
}
14 changes: 11 additions & 3 deletions tests/ui/unwrap.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
// compile-flags: --test

#![warn(clippy::unwrap_used)]

fn unwrap_option() {
Expand All @@ -10,7 +12,13 @@ fn unwrap_result() {
let _ = res.unwrap();
}

fn main() {
unwrap_option();
unwrap_result();
#[cfg(test)]
mod test {
#[test]
fn test_flag() {
let opt = Some(0);
let _ = opt.unwrap();
}
}

fn main() {}
14 changes: 11 additions & 3 deletions tests/ui/unwrap.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: used `unwrap()` on `an Option` value
--> $DIR/unwrap.rs:5:13
--> $DIR/unwrap.rs:7:13
|
LL | let _ = opt.unwrap();
| ^^^^^^^^^^^^
Expand All @@ -8,12 +8,20 @@ LL | let _ = opt.unwrap();
= help: if you don't want to handle the `None` case gracefully, consider using `expect()` to provide a better panic message

error: used `unwrap()` on `a Result` value
--> $DIR/unwrap.rs:10:13
--> $DIR/unwrap.rs:12:13
|
LL | let _ = res.unwrap();
| ^^^^^^^^^^^^
|
= help: if you don't want to handle the `Err` case gracefully, consider using `expect()` to provide a better panic message

error: aborting due to 2 previous errors
error: used `unwrap()` on `an Option` value
--> $DIR/unwrap.rs:20:17
|
LL | let _ = opt.unwrap();
| ^^^^^^^^^^^^
|
= help: if you don't want to handle the `None` case gracefully, consider using `expect()` to provide a better panic message

error: aborting due to 3 previous errors

0 comments on commit d38fe0f

Please sign in to comment.