Auto merge of #5132 - JohnTitor:fix-fp-in-unwrap-lint, r=flip1995

Do not lint `unnecessary_unwrap` in external macros Fixes #5131 I think we shouldn't lint `{panicking, unnecessary}_unwrap` in macros, not just `assert!`. changelog: Fix false positive in `unnecessary_unwrap`
rust-lang · Feb 6, 2020 · 0c6d55b · 0c6d55b
2 parents a7b3b9f + 19ce66c
commit 0c6d55b
Show file tree

Hide file tree

Showing 3 changed files with 40 additions and 13 deletions.
diff --git a/clippy_lints/src/unwrap.rs b/clippy_lints/src/unwrap.rs
@@ -1,6 +1,7 @@
 use crate::utils::{higher::if_block, match_type, paths, span_lint_and_then, usage::is_potentially_mutated};
 use if_chain::if_chain;
 use rustc::hir::map::Map;
+use rustc::lint::in_external_macro;
 use rustc_hir::intravisit::*;
 use rustc_hir::*;
 use rustc_lint::{LateContext, LateLintPass};
@@ -138,6 +139,10 @@ impl<'a, 'tcx> Visitor<'tcx> for UnwrappableVariablesVisitor<'a, 'tcx> {
     type Map = Map<'tcx>;
 
     fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
+        // Shouldn't lint when `expr` is in macro.
+        if in_external_macro(self.cx.tcx.sess, expr.span) {
+            return;
+        }
         if let Some((cond, then, els)) = if_block(&expr) {
             walk_expr(self, cond);
             self.visit_branch(cond, then, false);

diff --git a/tests/ui/checked_unwrap/simple_conditionals.rs b/tests/ui/checked_unwrap/simple_conditionals.rs
@@ -1,6 +1,14 @@
 #![deny(clippy::panicking_unwrap, clippy::unnecessary_unwrap)]
 #![allow(clippy::if_same_then_else)]
 
+macro_rules! m {
+    ($a:expr) => {
+        if $a.is_some() {
+            $a.unwrap(); // unnecessary
+        }
+    };
+}
+
 fn main() {
     let x = Some(());
     if x.is_some() {
@@ -13,6 +21,7 @@ fn main() {
     } else {
         x.unwrap(); // unnecessary
     }
+    m!(x);
     let mut x: Result<(), ()> = Ok(());
     if x.is_ok() {
         x.unwrap(); // unnecessary
@@ -39,4 +48,6 @@ fn main() {
                         // it will always panic but the lint is not smart enough to see this (it
                         // only checks if conditions).
     }
+
+    assert!(x.is_ok(), "{:?}", x.unwrap_err()); // ok, it's a common test pattern
 }
diff --git a/tests/ui/checked_unwrap/simple_conditionals.stderr b/tests/ui/checked_unwrap/simple_conditionals.stderr
@@ -1,5 +1,5 @@
 error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
-  --> $DIR/simple_conditionals.rs:7:9
+  --> $DIR/simple_conditionals.rs:15:9
    |
 LL |     if x.is_some() {
    |        ----------- the check is happening here
@@ -13,7 +13,7 @@ LL | #![deny(clippy::panicking_unwrap, clippy::unnecessary_unwrap)]
    |                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: This call to `unwrap()` will always panic.
-  --> $DIR/simple_conditionals.rs:9:9
+  --> $DIR/simple_conditionals.rs:17:9
    |
 LL |     if x.is_some() {
    |        ----------- because of this check
@@ -28,15 +28,15 @@ LL | #![deny(clippy::panicking_unwrap, clippy::unnecessary_unwrap)]
    |         ^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: This call to `unwrap()` will always panic.
-  --> $DIR/simple_conditionals.rs:12:9
+  --> $DIR/simple_conditionals.rs:20:9
    |
 LL |     if x.is_none() {
    |        ----------- because of this check
 LL |         x.unwrap(); // will panic
    |         ^^^^^^^^^^
 
 error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
-  --> $DIR/simple_conditionals.rs:14:9
+  --> $DIR/simple_conditionals.rs:22:9
    |
 LL |     if x.is_none() {
    |        ----------- the check is happening here
@@ -45,15 +45,26 @@ LL |         x.unwrap(); // unnecessary
    |         ^^^^^^^^^^
 
 error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
-  --> $DIR/simple_conditionals.rs:18:9
+  --> $DIR/simple_conditionals.rs:7:13
+   |
+LL |         if $a.is_some() {
+   |            ------------ the check is happening here
+LL |             $a.unwrap(); // unnecessary
+   |             ^^^^^^^^^^^
+...
+LL |     m!(x);
+   |     ------ in this macro invocation
+
+error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
+  --> $DIR/simple_conditionals.rs:27:9
    |
 LL |     if x.is_ok() {
    |        --------- the check is happening here
 LL |         x.unwrap(); // unnecessary
    |         ^^^^^^^^^^
 
 error: This call to `unwrap_err()` will always panic.
-  --> $DIR/simple_conditionals.rs:19:9
+  --> $DIR/simple_conditionals.rs:28:9
    |
 LL |     if x.is_ok() {
    |        --------- because of this check
@@ -62,7 +73,7 @@ LL |         x.unwrap_err(); // will panic
    |         ^^^^^^^^^^^^^^
 
 error: This call to `unwrap()` will always panic.
-  --> $DIR/simple_conditionals.rs:21:9
+  --> $DIR/simple_conditionals.rs:30:9
    |
 LL |     if x.is_ok() {
    |        --------- because of this check
@@ -71,7 +82,7 @@ LL |         x.unwrap(); // will panic
    |         ^^^^^^^^^^
 
 error: You checked before that `unwrap_err()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
-  --> $DIR/simple_conditionals.rs:22:9
+  --> $DIR/simple_conditionals.rs:31:9
    |
 LL |     if x.is_ok() {
    |        --------- the check is happening here
@@ -80,15 +91,15 @@ LL |         x.unwrap_err(); // unnecessary
    |         ^^^^^^^^^^^^^^
 
 error: This call to `unwrap()` will always panic.
-  --> $DIR/simple_conditionals.rs:25:9
+  --> $DIR/simple_conditionals.rs:34:9
    |
 LL |     if x.is_err() {
    |        ---------- because of this check
 LL |         x.unwrap(); // will panic
    |         ^^^^^^^^^^
 
 error: You checked before that `unwrap_err()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
-  --> $DIR/simple_conditionals.rs:26:9
+  --> $DIR/simple_conditionals.rs:35:9
    |
 LL |     if x.is_err() {
    |        ---------- the check is happening here
@@ -97,7 +108,7 @@ LL |         x.unwrap_err(); // unnecessary
    |         ^^^^^^^^^^^^^^
 
 error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
-  --> $DIR/simple_conditionals.rs:28:9
+  --> $DIR/simple_conditionals.rs:37:9
    |
 LL |     if x.is_err() {
    |        ---------- the check is happening here
@@ -106,13 +117,13 @@ LL |         x.unwrap(); // unnecessary
    |         ^^^^^^^^^^
 
 error: This call to `unwrap_err()` will always panic.
-  --> $DIR/simple_conditionals.rs:29:9
+  --> $DIR/simple_conditionals.rs:38:9
    |
 LL |     if x.is_err() {
    |        ---------- because of this check
 ...
 LL |         x.unwrap_err(); // will panic
    |         ^^^^^^^^^^^^^^
 
-error: aborting due to 12 previous errors
+error: aborting due to 13 previous errors