Auto merge of rust-lang#6200 - rail-rain:borrowed_box_invalid_sugg, r=phansch

fix the error-causing suggestion of 'borrowed_box'

Fixes rust-lang#3128

Fix the suggestion of 'borrowed_box', which causes a syntax error because it misses necessary parentheses.

---

changelog: Fix the error-causing suggestion of 'borrowed_box'

Author: bors

clippy_lints/src/types.rs

@@ -10,9 +10,9 @@ use rustc_errors::{Applicability, DiagnosticBuilder};
 use rustc_hir as hir;
 use rustc_hir::intravisit::{walk_body, walk_expr, walk_ty, FnKind, NestedVisitorMap, Visitor};
 use rustc_hir::{
-    BinOpKind, Block, Body, Expr, ExprKind, FnDecl, FnRetTy, FnSig, GenericArg, GenericParamKind, HirId, ImplItem,
-    ImplItemKind, Item, ItemKind, Lifetime, Lit, Local, MatchSource, MutTy, Mutability, Node, QPath, Stmt, StmtKind,
-    TraitFn, TraitItem, TraitItemKind, TyKind, UnOp,
+    BinOpKind, Block, Body, Expr, ExprKind, FnDecl, FnRetTy, FnSig, GenericArg, GenericBounds, GenericParamKind, HirId,
+    ImplItem, ImplItemKind, Item, ItemKind, Lifetime, Lit, Local, MatchSource, MutTy, Mutability, Node, QPath, Stmt,
+    StmtKind, SyntheticTyParamKind, TraitFn, TraitItem, TraitItemKind, TyKind, UnOp,
 };
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::hir::map::Map;
@@ -678,17 +678,30 @@ impl Types {
                             // details.
                             return;
                         }
+
+                        // When trait objects or opaque types have lifetime or auto-trait bounds,
+                        // we need to add parentheses to avoid a syntax error due to its ambiguity.
+                        // Originally reported as the issue #3128.
+                        let inner_snippet = snippet(cx, inner.span, "..");
+                        let suggestion = match &inner.kind {
+                            TyKind::TraitObject(bounds, lt_bound) if bounds.len() > 1 || !lt_bound.is_elided() => {
+                                format!("&{}({})", ltopt, &inner_snippet)
+                            },
+                            TyKind::Path(qpath)
+                                if get_bounds_if_impl_trait(cx, qpath, inner.hir_id)
+                                    .map_or(false, |bounds| bounds.len() > 1) =>
+                            {
+                                format!("&{}({})", ltopt, &inner_snippet)
+                            },
+                            _ => format!("&{}{}", ltopt, &inner_snippet),
+                        };
                         span_lint_and_sugg(
                             cx,
                             BORROWED_BOX,
                             hir_ty.span,
                             "you seem to be trying to use `&Box<T>`. Consider using just `&T`",
                             "try",
-                            format!(
-                                "&{}{}",
-                                ltopt,
-                                &snippet(cx, inner.span, "..")
-                            ),
+                            suggestion,
                             // To make this `MachineApplicable`, at least one needs to check if it isn't a trait item
                             // because the trait impls of it will break otherwise;
                             // and there may be other cases that result in invalid code.
@@ -721,6 +734,21 @@ fn is_any_trait(t: &hir::Ty<'_>) -> bool {
     false
 }
 
+fn get_bounds_if_impl_trait<'tcx>(cx: &LateContext<'tcx>, qpath: &QPath<'_>, id: HirId) -> Option<GenericBounds<'tcx>> {
+    if_chain! {
+        if let Some(did) = qpath_res(cx, qpath, id).opt_def_id();
+        if let Some(node) = cx.tcx.hir().get_if_local(did);
+        if let Node::GenericParam(generic_param) = node;
+        if let GenericParamKind::Type { synthetic, .. } = generic_param.kind;
+        if synthetic == Some(SyntheticTyParamKind::ImplTrait);
+        then {
+            Some(generic_param.bounds)
+        } else {
+            None
+        }
+    }
+}
+
 declare_clippy_lint! {
     /// **What it does:** Checks for binding a unit value.
     ///

tests/ui/borrow_box.rs

@@ -3,6 +3,8 @@
 #![allow(unused_variables)]
 #![allow(dead_code)]
 
+use std::fmt::Display;
+
 pub fn test1(foo: &mut Box<bool>) {
     // Although this function could be changed to "&mut bool",
     // avoiding the Box, mutable references to boxes are not
@@ -89,6 +91,20 @@ pub fn test13(boxed_slice: &mut Box<[i32]>) {
     *boxed_slice = data.into_boxed_slice();
 }
 
+// The suggestion should include proper parentheses to avoid a syntax error.
+pub fn test14(_display: &Box<dyn Display>) {}
+pub fn test15(_display: &Box<dyn Display + Send>) {}
+pub fn test16<'a>(_display: &'a Box<dyn Display + 'a>) {}
+
+pub fn test17(_display: &Box<impl Display>) {}
+pub fn test18(_display: &Box<impl Display + Send>) {}
+pub fn test19<'a>(_display: &'a Box<impl Display + 'a>) {}
+
+// This exists only to check what happens when parentheses are already present.
+// Even though the current implementation doesn't put extra parentheses,
+// it's fine that unnecessary parentheses appear in the future for some reason.
+pub fn test20(_display: &Box<(dyn Display + Send)>) {}
+
 fn main() {
     test1(&mut Box::new(false));
     test2();

tests/ui/borrow_box.stderr

@@ -1,5 +1,5 @@
 error: you seem to be trying to use `&Box<T>`. Consider using just `&T`
-  --> $DIR/borrow_box.rs:19:14
+  --> $DIR/borrow_box.rs:21:14
    |
 LL |     let foo: &Box<bool>;
    |              ^^^^^^^^^^ help: try: `&bool`
@@ -11,16 +11,58 @@ LL | #![deny(clippy::borrowed_box)]
    |         ^^^^^^^^^^^^^^^^^^^^
 
 error: you seem to be trying to use `&Box<T>`. Consider using just `&T`
-  --> $DIR/borrow_box.rs:23:10
+  --> $DIR/borrow_box.rs:25:10
    |
 LL |     foo: &'a Box<bool>,
    |          ^^^^^^^^^^^^^ help: try: `&'a bool`
 
 error: you seem to be trying to use `&Box<T>`. Consider using just `&T`
-  --> $DIR/borrow_box.rs:27:17
+  --> $DIR/borrow_box.rs:29:17
    |
 LL |     fn test4(a: &Box<bool>);
    |                 ^^^^^^^^^^ help: try: `&bool`
 
-error: aborting due to 3 previous errors
+error: you seem to be trying to use `&Box<T>`. Consider using just `&T`
+  --> $DIR/borrow_box.rs:95:25
+   |
+LL | pub fn test14(_display: &Box<dyn Display>) {}
+   |                         ^^^^^^^^^^^^^^^^^ help: try: `&dyn Display`
+
+error: you seem to be trying to use `&Box<T>`. Consider using just `&T`
+  --> $DIR/borrow_box.rs:96:25
+   |
+LL | pub fn test15(_display: &Box<dyn Display + Send>) {}
+   |                         ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `&(dyn Display + Send)`
+
+error: you seem to be trying to use `&Box<T>`. Consider using just `&T`
+  --> $DIR/borrow_box.rs:97:29
+   |
+LL | pub fn test16<'a>(_display: &'a Box<dyn Display + 'a>) {}
+   |                             ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `&'a (dyn Display + 'a)`
+
+error: you seem to be trying to use `&Box<T>`. Consider using just `&T`
+  --> $DIR/borrow_box.rs:99:25
+   |
+LL | pub fn test17(_display: &Box<impl Display>) {}
+   |                         ^^^^^^^^^^^^^^^^^^ help: try: `&impl Display`
+
+error: you seem to be trying to use `&Box<T>`. Consider using just `&T`
+  --> $DIR/borrow_box.rs:100:25
+   |
+LL | pub fn test18(_display: &Box<impl Display + Send>) {}
+   |                         ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `&(impl Display + Send)`
+
+error: you seem to be trying to use `&Box<T>`. Consider using just `&T`
+  --> $DIR/borrow_box.rs:101:29
+   |
+LL | pub fn test19<'a>(_display: &'a Box<impl Display + 'a>) {}
+   |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `&'a (impl Display + 'a)`
+
+error: you seem to be trying to use `&Box<T>`. Consider using just `&T`
+  --> $DIR/borrow_box.rs:106:25
+   |
+LL | pub fn test20(_display: &Box<(dyn Display + Send)>) {}
+   |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `&(dyn Display + Send)`
+
+error: aborting due to 10 previous errors