Auto merge of #10736 - NotAPenguin0:master, r=Alexendoo

initial clippy::ref_pattern implementation

This implements a new lint that discourages the use of the `ref` keyword as outlined in #9010. I think there are still some things to improve about this lint, but I need some feedback before I can implement those.

- [x] ~~Maybe it's desirable that a quick fix is listed too, instead of a generic `avoid using the ref keyword` lint.~~
- [x] `let ref x = y` already has a lint (`clippy::toplevel_ref_arg`). This implementation will report this too, so you get two lints for the same issue, which is not great. I don't really know a way around this though.
- [X] The dogfood test is currently failing locally, though I ran `cargo clippy -- -D clippy::all -D clippy::pedantic` and got no output, so I'm not sure why this is.

Any help with these would be greatly appreciated.

fixes #9010
changelog: [`ref_pattern`]: newly added lint

Author: bors

CHANGELOG.md

@@ -4978,6 +4978,7 @@ Released 2018-09-13
 [`ref_binding_to_reference`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_binding_to_reference
 [`ref_in_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_in_deref
 [`ref_option_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_option_ref
+[`ref_patterns`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_patterns
 [`regex_macro`]: https://rust-lang.github.io/rust-clippy/master/index.html#regex_macro
 [`repeat_once`]: https://rust-lang.github.io/rust-clippy/master/index.html#repeat_once
 [`replace_consts`]: https://rust-lang.github.io/rust-clippy/master/index.html#replace_consts

clippy_lints/src/declared_lints.rs

@@ -539,6 +539,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::redundant_slicing::REDUNDANT_SLICING_INFO,
     crate::redundant_static_lifetimes::REDUNDANT_STATIC_LIFETIMES_INFO,
     crate::ref_option_ref::REF_OPTION_REF_INFO,
+    crate::ref_patterns::REF_PATTERNS_INFO,
     crate::reference::DEREF_ADDROF_INFO,
     crate::regex::INVALID_REGEX_INFO,
     crate::regex::TRIVIAL_REGEX_INFO,

clippy_lints/src/lib.rs

@@ -266,6 +266,7 @@ mod redundant_pub_crate;
 mod redundant_slicing;
 mod redundant_static_lifetimes;
 mod ref_option_ref;
+mod ref_patterns;
 mod reference;
 mod regex;
 mod return_self_not_must_use;
@@ -971,6 +972,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|_| Box::new(manual_slice_size_calculation::ManualSliceSizeCalculation));
     store.register_early_pass(|| Box::new(suspicious_doc_comments::SuspiciousDocComments));
     store.register_late_pass(|_| Box::new(items_after_test_module::ItemsAfterTestModule));
+    store.register_early_pass(|| Box::new(ref_patterns::RefPatterns));
     store.register_late_pass(|_| Box::new(default_constructed_unit_structs::DefaultConstructedUnitStructs));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }

clippy_lints/src/misc.rs

@@ -16,9 +16,12 @@ use rustc_span::source_map::{ExpnKind, Span};
 
 use clippy_utils::sugg::Sugg;
 use clippy_utils::{
-    get_parent_expr, in_constant, is_integer_literal, is_no_std_crate, iter_input_pats, last_path_segment, SpanlessEq,
+    get_parent_expr, in_constant, is_integer_literal, is_lint_allowed, is_no_std_crate, iter_input_pats,
+    last_path_segment, SpanlessEq,
 };
 
+use crate::ref_patterns::REF_PATTERNS;
+
 declare_clippy_lint! {
     /// ### What it does
     /// Checks for function arguments and let bindings denoted as
@@ -162,6 +165,10 @@ impl<'tcx> LateLintPass<'tcx> for LintPass {
             return;
         }
         for arg in iter_input_pats(decl, body) {
+            // Do not emit if clippy::ref_patterns is not allowed to avoid having two lints for the same issue.
+            if !is_lint_allowed(cx, REF_PATTERNS, arg.pat.hir_id) {
+                return;
+            }
             if let PatKind::Binding(BindingAnnotation(ByRef::Yes, _), ..) = arg.pat.kind {
                 span_lint(
                     cx,
@@ -180,6 +187,8 @@ impl<'tcx> LateLintPass<'tcx> for LintPass {
             if let StmtKind::Local(local) = stmt.kind;
             if let PatKind::Binding(BindingAnnotation(ByRef::Yes, mutabl), .., name, None) = local.pat.kind;
             if let Some(init) = local.init;
+            // Do not emit if clippy::ref_patterns is not allowed to avoid having two lints for the same issue.
+            if is_lint_allowed(cx, REF_PATTERNS, local.pat.hir_id);
             then {
                 let ctxt = local.span.ctxt();
                 let mut app = Applicability::MachineApplicable;

clippy_lints/src/ref_patterns.rs

@@ -0,0 +1,44 @@
+use clippy_utils::diagnostics::span_lint_and_help;
+use rustc_ast::ast::{BindingAnnotation, Pat, PatKind};
+use rustc_lint::{EarlyContext, EarlyLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for usages of the `ref` keyword.
+    /// ### Why is this bad?
+    /// The `ref` keyword can be confusing for people unfamiliar with it, and often
+    /// it is more concise to use `&` instead.
+    /// ### Example
+    /// ```rust
+    /// let opt = Some(5);
+    /// if let Some(ref foo) = opt {}
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// let opt = Some(5);
+    /// if let Some(foo) = &opt {}
+    /// ```
+    #[clippy::version = "1.71.0"]
+    pub REF_PATTERNS,
+    restriction,
+    "use of a ref pattern, e.g. Some(ref value)"
+}
+declare_lint_pass!(RefPatterns => [REF_PATTERNS]);
+
+impl EarlyLintPass for RefPatterns {
+    fn check_pat(&mut self, cx: &EarlyContext<'_>, pat: &Pat) {
+        if let PatKind::Ident(BindingAnnotation::REF, _, _) = pat.kind
+                && !pat.span.from_expansion()
+        {
+            span_lint_and_help(
+                cx,
+                REF_PATTERNS,
+                pat.span,
+                "usage of ref pattern",
+                None,
+                "consider using `&` for clarity instead",
+            );
+        }
+    }
+}

tests/ui/ref_patterns.rs

@@ -0,0 +1,19 @@
+#![allow(unused)]
+#![warn(clippy::ref_patterns)]
+
+fn use_in_pattern() {
+    let opt = Some(5);
+    match opt {
+        None => {},
+        Some(ref opt) => {},
+    }
+}
+
+fn use_in_binding() {
+    let x = 5;
+    let ref y = x;
+}
+
+fn use_in_parameter(ref x: i32) {}
+
+fn main() {}

tests/ui/ref_patterns.stderr

@@ -0,0 +1,27 @@
+error: usage of ref pattern
+  --> $DIR/ref_patterns.rs:8:14
+   |
+LL |         Some(ref opt) => {},
+   |              ^^^^^^^
+   |
+   = help: consider using `&` for clarity instead
+   = note: `-D clippy::ref-patterns` implied by `-D warnings`
+
+error: usage of ref pattern
+  --> $DIR/ref_patterns.rs:14:9
+   |
+LL |     let ref y = x;
+   |         ^^^^^
+   |
+   = help: consider using `&` for clarity instead
+
+error: usage of ref pattern
+  --> $DIR/ref_patterns.rs:17:21
+   |
+LL | fn use_in_parameter(ref x: i32) {}
+   |                     ^^^^^
+   |
+   = help: consider using `&` for clarity instead
+
+error: aborting due to 3 previous errors
+