Auto merge of rust-lang#6212 - ThibsG:MacroTopLevelRefArg, r=flip1995

No lint in macro for `toplevel_ref_arg` Do not lint when the span is from a macro. Question: shouldn't we extend this for external macros also ? Fixes: rust-lang#5849 changelog: none
flip1995 · Oct 29, 2020 · e8de57c · e8de57c
2 parents 6d5cd6e + bab3386
commit e8de57c
Show file tree

Hide file tree

Showing 7 changed files with 119 additions and 12 deletions.
diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs
@@ -7,6 +7,7 @@ use rustc_hir::{
     StmtKind, TyKind, UnOp,
 };
 use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::lint::in_external_macro;
 use rustc_middle::ty::{self, Ty};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::hygiene::DesugaringKind;
@@ -271,13 +272,16 @@ impl<'tcx> LateLintPass<'tcx> for MiscLints {
         k: FnKind<'tcx>,
         decl: &'tcx FnDecl<'_>,
         body: &'tcx Body<'_>,
-        _: Span,
+        span: Span,
         _: HirId,
     ) {
         if let FnKind::Closure(_) = k {
             // Does not apply to closures
             return;
         }
+        if in_external_macro(cx.tcx.sess, span) {
+            return;
+        }
         for arg in iter_input_pats(decl, body) {
             if let PatKind::Binding(BindingAnnotation::Ref | BindingAnnotation::RefMut, ..) = arg.pat.kind {
                 span_lint(
@@ -293,13 +297,16 @@ impl<'tcx> LateLintPass<'tcx> for MiscLints {
 
     fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) {
         if_chain! {
+            if !in_external_macro(cx.tcx.sess, stmt.span);
             if let StmtKind::Local(ref local) = stmt.kind;
             if let PatKind::Binding(an, .., name, None) = local.pat.kind;
             if let Some(ref init) = local.init;
             if !higher::is_from_for_desugar(local);
             then {
                 if an == BindingAnnotation::Ref || an == BindingAnnotation::RefMut {
-                    let sugg_init = if init.span.from_expansion() {
+                    // use the macro callsite when the init span (but not the whole local span)
+                    // comes from an expansion like `vec![1, 2, 3]` in `let ref _ = vec![1, 2, 3];`
+                    let sugg_init = if init.span.from_expansion() && !local.span.from_expansion() {
                         Sugg::hir_with_macro_callsite(cx, init, "..")
                     } else {
                         Sugg::hir(cx, init, "..")
@@ -310,7 +317,7 @@ impl<'tcx> LateLintPass<'tcx> for MiscLints {
                         ("", sugg_init.addr())
                     };
                     let tyopt = if let Some(ref ty) = local.ty {
-                        format!(": &{mutopt}{ty}", mutopt=mutopt, ty=snippet(cx, ty.span, "_"))
+                        format!(": &{mutopt}{ty}", mutopt=mutopt, ty=snippet(cx, ty.span, ".."))
                     } else {
                         String::new()
                     };
@@ -326,7 +333,7 @@ impl<'tcx> LateLintPass<'tcx> for MiscLints {
                                 "try",
                                 format!(
                                     "let {name}{tyopt} = {initref};",
-                                    name=snippet(cx, name.span, "_"),
+                                    name=snippet(cx, name.span, ".."),
                                     tyopt=tyopt,
                                     initref=initref,
                                 ),

diff --git a/tests/ui/auxiliary/macro_rules.rs b/tests/ui/auxiliary/macro_rules.rs
@@ -56,3 +56,17 @@ macro_rules! option_env_unwrap_external {
         option_env!($env).expect($message)
     };
 }
+
+#[macro_export]
+macro_rules! ref_arg_binding {
+    () => {
+        let ref _y = 42;
+    };
+}
+
+#[macro_export]
+macro_rules! ref_arg_function {
+    () => {
+        fn fun_example(ref _x: usize) {}
+    };
+}
diff --git a/tests/ui/toplevel_ref_arg.fixed b/tests/ui/toplevel_ref_arg.fixed
@@ -1,7 +1,17 @@
 // run-rustfix
+// aux-build:macro_rules.rs
 
 #![warn(clippy::toplevel_ref_arg)]
 
+#[macro_use]
+extern crate macro_rules;
+
+macro_rules! gen_binding {
+    () => {
+        let _y = &42;
+    };
+}
+
 fn main() {
     // Closures should not warn
     let y = |ref x| println!("{:?}", x);
@@ -26,4 +36,15 @@ fn main() {
 
     // ok
     for ref _x in 0..10 {}
+
+    // lint in macro
+    #[allow(unused)]
+    {
+        gen_binding!();
+    }
+
+    // do not lint in external macro
+    {
+        ref_arg_binding!();
+    }
 }
diff --git a/tests/ui/toplevel_ref_arg.rs b/tests/ui/toplevel_ref_arg.rs
@@ -1,7 +1,17 @@
 // run-rustfix
+// aux-build:macro_rules.rs
 
 #![warn(clippy::toplevel_ref_arg)]
 
+#[macro_use]
+extern crate macro_rules;
+
+macro_rules! gen_binding {
+    () => {
+        let ref _y = 42;
+    };
+}
+
 fn main() {
     // Closures should not warn
     let y = |ref x| println!("{:?}", x);
@@ -26,4 +36,15 @@ fn main() {
 
     // ok
     for ref _x in 0..10 {}
+
+    // lint in macro
+    #[allow(unused)]
+    {
+        gen_binding!();
+    }
+
+    // do not lint in external macro
+    {
+        ref_arg_binding!();
+    }
 }
diff --git a/tests/ui/toplevel_ref_arg.stderr b/tests/ui/toplevel_ref_arg.stderr
@@ -1,34 +1,45 @@
 error: `ref` on an entire `let` pattern is discouraged, take a reference with `&` instead
-  --> $DIR/toplevel_ref_arg.rs:10:9
+  --> $DIR/toplevel_ref_arg.rs:20:9
    |
 LL |     let ref _x = 1;
    |     ----^^^^^^----- help: try: `let _x = &1;`
    |
    = note: `-D clippy::toplevel-ref-arg` implied by `-D warnings`
 
 error: `ref` on an entire `let` pattern is discouraged, take a reference with `&` instead
-  --> $DIR/toplevel_ref_arg.rs:12:9
+  --> $DIR/toplevel_ref_arg.rs:22:9
    |
 LL |     let ref _y: (&_, u8) = (&1, 2);
    |     ----^^^^^^--------------------- help: try: `let _y: &(&_, u8) = &(&1, 2);`
 
 error: `ref` on an entire `let` pattern is discouraged, take a reference with `&` instead
-  --> $DIR/toplevel_ref_arg.rs:14:9
+  --> $DIR/toplevel_ref_arg.rs:24:9
    |
 LL |     let ref _z = 1 + 2;
    |     ----^^^^^^--------- help: try: `let _z = &(1 + 2);`
 
 error: `ref` on an entire `let` pattern is discouraged, take a reference with `&` instead
-  --> $DIR/toplevel_ref_arg.rs:16:9
+  --> $DIR/toplevel_ref_arg.rs:26:9
    |
 LL |     let ref mut _z = 1 + 2;
    |     ----^^^^^^^^^^--------- help: try: `let _z = &mut (1 + 2);`
 
 error: `ref` on an entire `let` pattern is discouraged, take a reference with `&` instead
-  --> $DIR/toplevel_ref_arg.rs:21:9
+  --> $DIR/toplevel_ref_arg.rs:31:9
    |
 LL |     let ref _x = vec![1, 2, 3];
    |     ----^^^^^^----------------- help: try: `let _x = &vec![1, 2, 3];`
 
-error: aborting due to 5 previous errors
+error: `ref` on an entire `let` pattern is discouraged, take a reference with `&` instead
+  --> $DIR/toplevel_ref_arg.rs:11:13
+   |
+LL |         let ref _y = 42;
+   |         ----^^^^^^------ help: try: `let _y = &42;`
+...
+LL |         gen_binding!();
+   |         --------------- 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 6 previous errors
 
diff --git a/tests/ui/toplevel_ref_arg_non_rustfix.rs b/tests/ui/toplevel_ref_arg_non_rustfix.rs
@@ -1,11 +1,33 @@
+// aux-build:macro_rules.rs
+
 #![warn(clippy::toplevel_ref_arg)]
 #![allow(unused)]
 
+#[macro_use]
+extern crate macro_rules;
+
 fn the_answer(ref mut x: u8) {
     *x = 42;
 }
 
+macro_rules! gen_function {
+    () => {
+        fn fun_example(ref _x: usize) {}
+    };
+}
+
 fn main() {
     let mut x = 0;
     the_answer(x);
+
+    // lint in macro
+    #[allow(unused)]
+    {
+        gen_function!();
+    }
+
+    // do not lint in external macro
+    {
+        ref_arg_function!();
+    }
 }
diff --git a/tests/ui/toplevel_ref_arg_non_rustfix.stderr b/tests/ui/toplevel_ref_arg_non_rustfix.stderr
@@ -1,10 +1,21 @@
 error: `ref` directly on a function argument is ignored. Consider using a reference type instead.
-  --> $DIR/toplevel_ref_arg_non_rustfix.rs:4:15
+  --> $DIR/toplevel_ref_arg_non_rustfix.rs:9:15
    |
 LL | fn the_answer(ref mut x: u8) {
    |               ^^^^^^^^^
    |
    = note: `-D clippy::toplevel-ref-arg` implied by `-D warnings`
 
-error: aborting due to previous error
+error: `ref` directly on a function argument is ignored. Consider using a reference type instead.
+  --> $DIR/toplevel_ref_arg_non_rustfix.rs:15:24
+   |
+LL |         fn fun_example(ref _x: usize) {}
+   |                        ^^^^^^
+...
+LL |         gen_function!();
+   |         ---------------- 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 2 previous errors