Auto merge of #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: #6234
Fixes: #6242
Fixes: #6237

changelog: none

r? `@llogiq`

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,
  );
  }

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, "_")

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)
+ }
+}

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)
+ }
+}

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
 

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!();

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!();

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