Auto merge of rust-lang#6278 - ThibsG:DerefAddrOf, r=llogiq

Fix bad suggestions for `deref_addrof` and `try_err` lints Fix bad suggestions when in macro expansion for `deref_addrof` and `try_err` lints. Fixes: rust-lang#6234 Fixes: rust-lang#6242 Fixes: rust-lang#6237 changelog: none r? `@llogiq`
flip1995 · Nov 9, 2020 · d212c38 · d212c38
2 parents 2067a01 + 83e75f9
commit d212c38
Show file tree

Hide file tree

Showing 8 changed files with 207 additions and 15 deletions.
diff --git a/clippy_lints/src/reference.rs b/clippy_lints/src/reference.rs
@@ -1,9 +1,10 @@
-use crate::utils::{in_macro, snippet_with_applicability, span_lint_and_sugg};
+use crate::utils::{in_macro, snippet_opt, snippet_with_applicability, span_lint_and_sugg};
 use if_chain::if_chain;
-use rustc_ast::ast::{Expr, ExprKind, UnOp};
+use rustc_ast::ast::{Expr, ExprKind, Mutability, UnOp};
 use rustc_errors::Applicability;
 use rustc_lint::{EarlyContext, EarlyLintPass};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::BytePos;
 
 declare_clippy_lint! {
     /// **What it does:** Checks for usage of `*&` and `*&mut` in expressions.
@@ -42,17 +43,45 @@ impl EarlyLintPass for DerefAddrOf {
     fn check_expr(&mut self, cx: &EarlyContext<'_>, e: &Expr) {
         if_chain! {
             if let ExprKind::Unary(UnOp::Deref, ref deref_target) = e.kind;
-            if let ExprKind::AddrOf(_, _, ref addrof_target) = without_parens(deref_target).kind;
+            if let ExprKind::AddrOf(_, ref mutability, ref addrof_target) = without_parens(deref_target).kind;
             if !in_macro(addrof_target.span);
             then {
                 let mut applicability = Applicability::MachineApplicable;
+                let sugg = if e.span.from_expansion() {
+                    if let Ok(macro_source) = cx.sess.source_map().span_to_snippet(e.span) {
+                        // Remove leading whitespace from the given span
+                        // e.g: ` $visitor` turns into `$visitor`
+                        let trim_leading_whitespaces = |span| {
+                            snippet_opt(cx, span).and_then(|snip| {
+                                #[allow(clippy::cast_possible_truncation)]
+                                snip.find(|c: char| !c.is_whitespace()).map(|pos| {
+                                    span.lo() + BytePos(pos as u32)
+                                })
+                            }).map_or(span, |start_no_whitespace| e.span.with_lo(start_no_whitespace))
+                        };
+
+                        let rpos = if *mutability == Mutability::Mut {
+                            macro_source.rfind("mut").expect("already checked this is a mutable reference") + "mut".len()
+                        } else {
+                            macro_source.rfind('&').expect("already checked this is a reference") + "&".len()
+                        };
+                        #[allow(clippy::cast_possible_truncation)]
+                        let span_after_ref = e.span.with_lo(BytePos(e.span.lo().0 + rpos as u32));
+                        let span = trim_leading_whitespaces(span_after_ref);
+                        snippet_with_applicability(cx, span, "_", &mut applicability)
+                    } else {
+                        snippet_with_applicability(cx, e.span, "_", &mut applicability)
+                    }
+                } else {
+                    snippet_with_applicability(cx, addrof_target.span, "_", &mut applicability)
+                }.to_string();
                 span_lint_and_sugg(
                     cx,
                     DEREF_ADDROF,
                     e.span,
                     "immediately dereferencing a reference",
                     "try this",
-                    format!("{}", snippet_with_applicability(cx, addrof_target.span, "_", &mut applicability)),
+                    sugg,
                     applicability,
                 );
             }

diff --git a/clippy_lints/src/try_err.rs b/clippy_lints/src/try_err.rs
@@ -1,6 +1,6 @@
 use crate::utils::{
-    is_type_diagnostic_item, match_def_path, match_qpath, paths, snippet, snippet_with_macro_callsite,
-    span_lint_and_sugg,
+    differing_macro_contexts, in_macro, is_type_diagnostic_item, match_def_path, match_qpath, paths, snippet,
+    snippet_with_macro_callsite, span_lint_and_sugg,
 };
 use if_chain::if_chain;
 use rustc_errors::Applicability;
@@ -92,8 +92,11 @@ impl<'tcx> LateLintPass<'tcx> for TryErr {
                 };
 
                 let expr_err_ty = cx.typeck_results().expr_ty(err_arg);
+                let differing_contexts = differing_macro_contexts(expr.span, err_arg.span);
 
-                let origin_snippet = if err_arg.span.from_expansion() {
+                let origin_snippet = if in_macro(expr.span) && in_macro(err_arg.span) && differing_contexts {
+                    snippet(cx, err_arg.span.ctxt().outer_expn_data().call_site, "_")
+                } else if err_arg.span.from_expansion() && !in_macro(expr.span) {
                     snippet_with_macro_callsite(cx, err_arg.span, "_")
                 } else {
                     snippet(cx, err_arg.span, "_")

diff --git a/tests/ui/deref_addrof.fixed b/tests/ui/deref_addrof.fixed
@@ -1,4 +1,5 @@
 // run-rustfix
+#![warn(clippy::deref_addrof)]
 
 fn get_number() -> usize {
     10
@@ -10,7 +11,6 @@ fn get_reference(n: &usize) -> &usize {
 
 #[allow(clippy::many_single_char_names, clippy::double_parens)]
 #[allow(unused_variables, unused_parens)]
-#[warn(clippy::deref_addrof)]
 fn main() {
     let a = 10;
     let aref = &a;
@@ -37,3 +37,27 @@ fn main() {
 
     let b = *aref;
 }
+
+#[rustfmt::skip]
+macro_rules! m {
+    ($visitor: expr) => {
+        $visitor
+    };
+}
+
+#[rustfmt::skip]
+macro_rules! m_mut {
+    ($visitor: expr) => {
+        $visitor
+    };
+}
+
+pub struct S;
+impl S {
+    pub fn f(&self) -> &Self {
+        m!(self)
+    }
+    pub fn f_mut(&self) -> &Self {
+        m_mut!(self)
+    }
+}
diff --git a/tests/ui/deref_addrof.rs b/tests/ui/deref_addrof.rs
@@ -1,4 +1,5 @@
 // run-rustfix
+#![warn(clippy::deref_addrof)]
 
 fn get_number() -> usize {
     10
@@ -10,7 +11,6 @@ fn get_reference(n: &usize) -> &usize {
 
 #[allow(clippy::many_single_char_names, clippy::double_parens)]
 #[allow(unused_variables, unused_parens)]
-#[warn(clippy::deref_addrof)]
 fn main() {
     let a = 10;
     let aref = &a;
@@ -37,3 +37,27 @@ fn main() {
 
     let b = **&aref;
 }
+
+#[rustfmt::skip]
+macro_rules! m {
+    ($visitor: expr) => {
+        *& $visitor
+    };
+}
+
+#[rustfmt::skip]
+macro_rules! m_mut {
+    ($visitor: expr) => {
+        *& mut $visitor
+    };
+}
+
+pub struct S;
+impl S {
+    pub fn f(&self) -> &Self {
+        m!(self)
+    }
+    pub fn f_mut(&self) -> &Self {
+        m_mut!(self)
+    }
+}
diff --git a/tests/ui/deref_addrof.stderr b/tests/ui/deref_addrof.stderr
@@ -48,5 +48,27 @@ error: immediately dereferencing a reference
 LL |     let b = **&aref;
    |              ^^^^^^ help: try this: `aref`
 
-error: aborting due to 8 previous errors
+error: immediately dereferencing a reference
+  --> $DIR/deref_addrof.rs:44:9
+   |
+LL |         *& $visitor
+   |         ^^^^^^^^^^^ help: try this: `$visitor`
+...
+LL |         m!(self)
+   |         -------- in this macro invocation
+   |
+   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: immediately dereferencing a reference
+  --> $DIR/deref_addrof.rs:51:9
+   |
+LL |         *& mut $visitor
+   |         ^^^^^^^^^^^^^^^ help: try this: `$visitor`
+...
+LL |         m_mut!(self)
+   |         ------------ in this macro invocation
+   |
+   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: aborting due to 10 previous errors
 
diff --git a/tests/ui/try_err.fixed b/tests/ui/try_err.fixed
@@ -78,12 +78,46 @@ fn nested_error() -> Result<i32, i32> {
     Ok(1)
 }
 
+// Bad suggestion when in macro (see #6242)
+macro_rules! try_validation {
+    ($e: expr) => {{
+        match $e {
+            Ok(_) => 0,
+            Err(_) => return Err(1),
+        }
+    }};
+}
+
+macro_rules! ret_one {
+    () => {
+        1
+    };
+}
+
+macro_rules! try_validation_in_macro {
+    ($e: expr) => {{
+        match $e {
+            Ok(_) => 0,
+            Err(_) => return Err(ret_one!()),
+        }
+    }};
+}
+
+fn calling_macro() -> Result<i32, i32> {
+    // macro
+    try_validation!(Ok::<_, i32>(5));
+    // `Err` arg is another macro
+    try_validation_in_macro!(Ok::<_, i32>(5));
+    Ok(5)
+}
+
 fn main() {
     basic_test().unwrap();
     into_test().unwrap();
     negative_test().unwrap();
     closure_matches_test().unwrap();
     closure_into_test().unwrap();
+    calling_macro().unwrap();
 
     // We don't want to lint in external macros
     try_err!();

diff --git a/tests/ui/try_err.rs b/tests/ui/try_err.rs
@@ -78,12 +78,46 @@ fn nested_error() -> Result<i32, i32> {
     Ok(1)
 }
 
+// Bad suggestion when in macro (see #6242)
+macro_rules! try_validation {
+    ($e: expr) => {{
+        match $e {
+            Ok(_) => 0,
+            Err(_) => Err(1)?,
+        }
+    }};
+}
+
+macro_rules! ret_one {
+    () => {
+        1
+    };
+}
+
+macro_rules! try_validation_in_macro {
+    ($e: expr) => {{
+        match $e {
+            Ok(_) => 0,
+            Err(_) => Err(ret_one!())?,
+        }
+    }};
+}
+
+fn calling_macro() -> Result<i32, i32> {
+    // macro
+    try_validation!(Ok::<_, i32>(5));
+    // `Err` arg is another macro
+    try_validation_in_macro!(Ok::<_, i32>(5));
+    Ok(5)
+}
+
 fn main() {
     basic_test().unwrap();
     into_test().unwrap();
     negative_test().unwrap();
     closure_matches_test().unwrap();
     closure_into_test().unwrap();
+    calling_macro().unwrap();
 
     // We don't want to lint in external macros
     try_err!();

diff --git a/tests/ui/try_err.stderr b/tests/ui/try_err.stderr
@@ -29,28 +29,50 @@ LL |                 Err(err)?;
    |                 ^^^^^^^^^ help: try this: `return Err(err.into())`
 
 error: returning an `Err(_)` with the `?` operator
-  --> $DIR/try_err.rs:106:9
+  --> $DIR/try_err.rs:86:23
+   |
+LL |             Err(_) => Err(1)?,
+   |                       ^^^^^^^ help: try this: `return Err(1)`
+...
+LL |     try_validation!(Ok::<_, i32>(5));
+   |     --------------------------------- in this macro invocation
+   |
+   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: returning an `Err(_)` with the `?` operator
+  --> $DIR/try_err.rs:101:23
+   |
+LL |             Err(_) => Err(ret_one!())?,
+   |                       ^^^^^^^^^^^^^^^^ help: try this: `return Err(ret_one!())`
+...
+LL |     try_validation_in_macro!(Ok::<_, i32>(5));
+   |     ------------------------------------------ in this macro invocation
+   |
+   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: returning an `Err(_)` with the `?` operator
+  --> $DIR/try_err.rs:140:9
    |
 LL |         Err(foo!())?;
    |         ^^^^^^^^^^^^ help: try this: `return Err(foo!())`
 
 error: returning an `Err(_)` with the `?` operator
-  --> $DIR/try_err.rs:113:9
+  --> $DIR/try_err.rs:147:9
    |
 LL |         Err(io::ErrorKind::WriteZero)?
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `return Poll::Ready(Err(io::ErrorKind::WriteZero.into()))`
 
 error: returning an `Err(_)` with the `?` operator
-  --> $DIR/try_err.rs:115:9
+  --> $DIR/try_err.rs:149:9
    |
 LL |         Err(io::Error::new(io::ErrorKind::InvalidInput, "error"))?
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `return Poll::Ready(Err(io::Error::new(io::ErrorKind::InvalidInput, "error")))`
 
 error: returning an `Err(_)` with the `?` operator
-  --> $DIR/try_err.rs:123:9
+  --> $DIR/try_err.rs:157:9
    |
 LL |         Err(io::ErrorKind::NotFound)?
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `return Poll::Ready(Some(Err(io::ErrorKind::NotFound.into())))`
 
-error: aborting due to 8 previous errors
+error: aborting due to 10 previous errors