Detect more cases of unused_parens around types

Author: Benjamin Schulz

compiler/rustc_ast/src/ast.rs

@@ -1391,6 +1391,7 @@ impl Expr {
                 path.clone(),
                 TraitBoundModifiers::NONE,
                 self.span,
+                Parens::No,
             ))),
             _ => None,
         }
@@ -3360,6 +3361,13 @@ pub struct TraitRef {
     pub ref_id: NodeId,
 }
 
+/// Whether enclosing parentheses are present or not.
+#[derive(Clone, Encodable, Decodable, Debug)]
+pub enum Parens {
+    Yes,
+    No,
+}
+
 #[derive(Clone, Encodable, Decodable, Debug)]
 pub struct PolyTraitRef {
     /// The `'a` in `for<'a> Foo<&'a T>`.
@@ -3372,6 +3380,10 @@ pub struct PolyTraitRef {
     pub trait_ref: TraitRef,
 
     pub span: Span,
+
+    /// When `Yes`, the first and last character of `span` are an opening
+    /// and a closing paren respectively.
+    pub parens: Parens,
 }
 
 impl PolyTraitRef {
@@ -3380,12 +3392,14 @@ impl PolyTraitRef {
         path: Path,
         modifiers: TraitBoundModifiers,
         span: Span,
+        parens: Parens,
     ) -> Self {
         PolyTraitRef {
             bound_generic_params: generic_params,
             modifiers,
             trait_ref: TraitRef { path, ref_id: DUMMY_NODE_ID },
             span,
+            parens,
         }
     }
 }

compiler/rustc_ast/src/visit.rs

@@ -1142,7 +1142,7 @@ macro_rules! common_visitor_and_walkers {
             vis: &mut V,
             p: &$($lt)? $($mut)? PolyTraitRef,
         ) -> V::Result {
-            let PolyTraitRef { bound_generic_params, modifiers, trait_ref, span } = p;
+            let PolyTraitRef { bound_generic_params, modifiers, trait_ref, span, parens: _ } = p;
             try_visit!(visit_modifiers(vis, modifiers));
             try_visit!(visit_generic_params(vis, bound_generic_params));
             try_visit!(vis.visit_trait_ref(trait_ref));

compiler/rustc_ast_lowering/src/lib.rs

@@ -1209,6 +1209,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
                         modifiers: TraitBoundModifiers::NONE,
                         trait_ref: TraitRef { path: path.clone(), ref_id: t.id },
                         span: t.span,
+                        parens: ast::Parens::No,
                     },
                     itctx,
                 );

compiler/rustc_expand/src/build.rs

@@ -195,6 +195,7 @@ impl<'a> ExtCtxt<'a> {
             },
             trait_ref: self.trait_ref(path),
             span,
+            parens: ast::Parens::No,
         }
     }
 

compiler/rustc_lint/src/unused.rs

@@ -3,13 +3,15 @@ use std::iter;
 use rustc_ast::util::{classify, parser};
 use rustc_ast::{self as ast, ExprKind, HasAttrs as _, StmtKind};
 use rustc_attr_data_structures::{AttributeKind, find_attr};
+use rustc_data_structures::fx::FxHashMap;
 use rustc_errors::{MultiSpan, pluralize};
 use rustc_hir::def::{DefKind, Res};
 use rustc_hir::def_id::DefId;
 use rustc_hir::{self as hir, LangItem};
 use rustc_infer::traits::util::elaborate;
 use rustc_middle::ty::{self, Ty, adjustment};
 use rustc_session::{declare_lint, declare_lint_pass, impl_lint_pass};
+use rustc_span::edition::Edition::Edition2015;
 use rustc_span::{BytePos, Span, Symbol, kw, sym};
 use tracing::instrument;
 
@@ -1032,6 +1034,31 @@ pub(crate) struct UnusedParens {
     /// `1 as (i32) < 2` parses to ExprKind::Lt
     /// `1 as i32 < 2` parses to i32::<2[missing angle bracket]
     parens_in_cast_in_lt: Vec<ast::NodeId>,
+    /// Ty nodes in this map are in TypeNoBounds position. Any bounds they
+    /// contain may be ambiguous w/r/t trailing `+` operators.
+    in_no_bounds_pos: FxHashMap<ast::NodeId, NoBoundsException>,
+}
+
+/// Whether parentheses may be omitted from a type without resulting in ambiguity.
+///
+/// ```
+/// type Example = Box<dyn Fn() -> &'static (dyn Send) + Sync>;
+/// ```
+///
+/// Here, `&'static (dyn Send) + Sync` is a `TypeNoBounds`. As such, it may not directly
+/// contain `ImplTraitType` or `TraitObjectType` which is why `(dyn Send)` is parenthesized.
+/// However, an exception is made for `ImplTraitTypeOneBound` and `TraitObjectTypeOneBound`.
+/// The following is accepted because there is no `+`.
+///
+/// ```
+/// type Example = Box<dyn Fn() -> &'static dyn Send>;
+/// ```
+enum NoBoundsException {
+    /// The type must be parenthesized.
+    None,
+    /// The type is the last bound of the containing type expression. If it has exactly one bound,
+    /// parentheses around the type are unnecessary.
+    OneBound,
 }
 
 impl_lint_pass!(UnusedParens => [UNUSED_PARENS]);
@@ -1275,23 +1302,100 @@ impl EarlyLintPass for UnusedParens {
                 );
             }
             ast::TyKind::Paren(r) => {
-                match &r.kind {
-                    ast::TyKind::TraitObject(..) => {}
-                    ast::TyKind::BareFn(b)
-                        if self.with_self_ty_parens && b.generic_params.len() > 0 => {}
-                    ast::TyKind::ImplTrait(_, bounds) if bounds.len() > 1 => {}
-                    _ => {
-                        let spans = if !ty.span.from_expansion() {
+                let unused_parens = match &r.kind {
+                    ast::TyKind::ImplTrait(_, bounds) | ast::TyKind::TraitObject(bounds, _) => {
+                        match self.in_no_bounds_pos.get(&ty.id) {
+                            Some(NoBoundsException::None) => false,
+                            Some(NoBoundsException::OneBound) => bounds.len() <= 1,
+                            None => true,
+                        }
+                    }
+                    ast::TyKind::BareFn(b) => {
+                        !self.with_self_ty_parens || b.generic_params.is_empty()
+                    }
+                    _ => true,
+                };
+
+                if unused_parens {
+                    let spans = (!ty.span.from_expansion())
+                        .then(|| {
                             r.span
                                 .find_ancestor_inside(ty.span)
                                 .map(|r| (ty.span.with_hi(r.lo()), ty.span.with_lo(r.hi())))
+                        })
+                        .flatten();
+
+                    self.emit_unused_delims(cx, ty.span, spans, "type", (false, false), false);
+                }
+
+                self.with_self_ty_parens = false;
+            }
+            ast::TyKind::Ref(_, mut_ty) | ast::TyKind::Ptr(mut_ty) => {
+                self.in_no_bounds_pos.insert(mut_ty.ty.id, NoBoundsException::OneBound);
+            }
+            ast::TyKind::TraitObject(bounds, _) | ast::TyKind::ImplTrait(_, bounds) => {
+                for i in 0..bounds.len() {
+                    let is_last = i == bounds.len() - 1;
+
+                    if let ast::GenericBound::Trait(poly_trait_ref) = &bounds[i] {
+                        let fn_with_explicit_ret_ty = if let [.., segment] =
+                            &*poly_trait_ref.trait_ref.path.segments
+                            && let Some(args) = segment.args.as_ref()
+                            && let ast::GenericArgs::Parenthesized(paren_args) = &**args
+                            && let ast::FnRetTy::Ty(ret_ty) = &paren_args.output
+                        {
+                            self.in_no_bounds_pos.insert(
+                                ret_ty.id,
+                                if is_last {
+                                    NoBoundsException::OneBound
+                                } else {
+                                    NoBoundsException::None
+                                },
+                            );
+
+                            true
                         } else {
-                            None
+                            false
                         };
-                        self.emit_unused_delims(cx, ty.span, spans, "type", (false, false), false);
+
+                        // In edition 2015, dyn is a contextual keyword and `dyn::foo::Bar` is
+                        // parsed as a path, so parens are necessary to disambiguate. See
+                        //  - tests/ui/lint/unused/unused-parens-trait-obj-e2015.rs and
+                        //  - https://doc.rust-lang.org/reference/types/trait-object.html#r-type.trait-object.syntax-edition2018
+                        let dyn2015_exception = cx.sess().psess.edition == Edition2015
+                            && matches!(ty.kind, ast::TyKind::TraitObject(..))
+                            && i == 0
+                            && poly_trait_ref
+                                .trait_ref
+                                .path
+                                .segments
+                                .first()
+                                .map(|s| s.ident.name == kw::PathRoot)
+                                .unwrap_or(false);
+
+                        if let ast::Parens::Yes = poly_trait_ref.parens
+                            && (is_last || !fn_with_explicit_ret_ty)
+                            && !dyn2015_exception
+                        {
+                            let s = poly_trait_ref.span;
+                            let spans = (!s.from_expansion()).then(|| {
+                                (
+                                    s.with_hi(s.lo() + rustc_span::BytePos(1)),
+                                    s.with_lo(s.hi() - rustc_span::BytePos(1)),
+                                )
+                            });
+
+                            self.emit_unused_delims(
+                                cx,
+                                poly_trait_ref.span,
+                                spans,
+                                "type",
+                                (false, false),
+                                false,
+                            );
+                        }
                     }
                 }
-                self.with_self_ty_parens = false;
             }
             _ => {}
         }
@@ -1301,6 +1405,10 @@ impl EarlyLintPass for UnusedParens {
         <Self as UnusedDelimLint>::check_item(self, cx, item)
     }
 
+    fn check_item_post(&mut self, _: &EarlyContext<'_>, _: &rustc_ast::Item) {
+        self.in_no_bounds_pos.clear();
+    }
+
     fn enter_where_predicate(&mut self, _: &EarlyContext<'_>, pred: &ast::WherePredicate) {
         use rustc_ast::{WhereBoundPredicate, WherePredicateKind};
         if let WherePredicateKind::BoundPredicate(WhereBoundPredicate {

compiler/rustc_parse/src/parser/ty.rs

@@ -305,8 +305,13 @@ impl<'a> Parser<'a> {
                     let removal_span = kw.span.with_hi(self.token.span.lo());
                     let path = self.parse_path(PathStyle::Type)?;
                     let parse_plus = allow_plus == AllowPlus::Yes && self.check_plus();
-                    let kind =
-                        self.parse_remaining_bounds_path(lifetime_defs, path, lo, parse_plus)?;
+                    let kind = self.parse_remaining_bounds_path(
+                        lifetime_defs,
+                        path,
+                        lo,
+                        parse_plus,
+                        ast::Parens::No,
+                    )?;
                     let err = self.dcx().create_err(errors::TransposeDynOrImpl {
                         span: kw.span,
                         kw: kw.name.as_str(),
@@ -333,7 +338,13 @@ impl<'a> Parser<'a> {
                 } else {
                     let path = self.parse_path(PathStyle::Type)?;
                     let parse_plus = allow_plus == AllowPlus::Yes && self.check_plus();
-                    self.parse_remaining_bounds_path(lifetime_defs, path, lo, parse_plus)?
+                    self.parse_remaining_bounds_path(
+                        lifetime_defs,
+                        path,
+                        lo,
+                        parse_plus,
+                        ast::Parens::No,
+                    )?
                 }
             }
         } else if self.eat_keyword(exp!(Impl)) {
@@ -413,9 +424,13 @@ impl<'a> Parser<'a> {
             let maybe_bounds = allow_plus == AllowPlus::Yes && self.token.is_like_plus();
             match ty.kind {
                 // `(TY_BOUND_NOPAREN) + BOUND + ...`.
-                TyKind::Path(None, path) if maybe_bounds => {
-                    self.parse_remaining_bounds_path(ThinVec::new(), path, lo, true)
-                }
+                TyKind::Path(None, path) if maybe_bounds => self.parse_remaining_bounds_path(
+                    ThinVec::new(),
+                    path,
+                    lo,
+                    true,
+                    ast::Parens::Yes,
+                ),
                 // For `('a) + …`, we know that `'a` in type position already lead to an error being
                 // emitted. To reduce output, let's indirectly suppress E0178 (bad `+` in type) and
                 // other irrelevant consequential errors.
@@ -495,12 +510,14 @@ impl<'a> Parser<'a> {
         path: ast::Path,
         lo: Span,
         parse_plus: bool,
+        parens: ast::Parens,
     ) -> PResult<'a, TyKind> {
         let poly_trait_ref = PolyTraitRef::new(
             generic_params,
             path,
             TraitBoundModifiers::NONE,
             lo.to(self.prev_token.span),
+            parens,
         );
         let bounds = vec![GenericBound::Trait(poly_trait_ref)];
         self.parse_remaining_bounds(bounds, parse_plus)
@@ -832,7 +849,7 @@ impl<'a> Parser<'a> {
             Ok(TyKind::MacCall(P(MacCall { path, args: self.parse_delim_args()? })))
         } else if allow_plus == AllowPlus::Yes && self.check_plus() {
             // `Trait1 + Trait2 + 'a`
-            self.parse_remaining_bounds_path(ThinVec::new(), path, lo, true)
+            self.parse_remaining_bounds_path(ThinVec::new(), path, lo, true, ast::Parens::No)
         } else {
             // Just a type path.
             Ok(TyKind::Path(None, path))
@@ -897,10 +914,10 @@ impl<'a> Parser<'a> {
     fn parse_generic_bound(&mut self) -> PResult<'a, GenericBound> {
         let lo = self.token.span;
         let leading_token = self.prev_token;
-        let has_parens = self.eat(exp!(OpenParen));
+        let parens = if self.eat(exp!(OpenParen)) { ast::Parens::Yes } else { ast::Parens::No };
 
         let bound = if self.token.is_lifetime() {
-            self.parse_generic_lt_bound(lo, has_parens)?
+            self.parse_generic_lt_bound(lo, parens)?
         } else if self.eat_keyword(exp!(Use)) {
             // parse precise captures, if any. This is `use<'lt, 'lt, P, P>`; a list of
             // lifetimes and ident params (including SelfUpper). These are validated later
@@ -909,7 +926,7 @@ impl<'a> Parser<'a> {
             let (args, args_span) = self.parse_precise_capturing_args()?;
             GenericBound::Use(args, use_span.to(args_span))
         } else {
-            self.parse_generic_ty_bound(lo, has_parens, &leading_token)?
+            self.parse_generic_ty_bound(lo, parens, &leading_token)?
         };
 
         Ok(bound)
@@ -919,10 +936,14 @@ impl<'a> Parser<'a> {
     /// ```ebnf
     /// LT_BOUND = LIFETIME
     /// ```
-    fn parse_generic_lt_bound(&mut self, lo: Span, has_parens: bool) -> PResult<'a, GenericBound> {
+    fn parse_generic_lt_bound(
+        &mut self,
+        lo: Span,
+        parens: ast::Parens,
+    ) -> PResult<'a, GenericBound> {
         let lt = self.expect_lifetime();
         let bound = GenericBound::Outlives(lt);
-        if has_parens {
+        if let ast::Parens::Yes = parens {
             // FIXME(Centril): Consider not erroring here and accepting `('lt)` instead,
             // possibly introducing `GenericBound::Paren(P<GenericBound>)`?
             self.recover_paren_lifetime(lo)?;
@@ -1078,7 +1099,7 @@ impl<'a> Parser<'a> {
     fn parse_generic_ty_bound(
         &mut self,
         lo: Span,
-        has_parens: bool,
+        parens: ast::Parens,
         leading_token: &Token,
     ) -> PResult<'a, GenericBound> {
         let (mut lifetime_defs, binder_span) = self.parse_late_bound_lifetime_defs()?;
@@ -1104,7 +1125,7 @@ impl<'a> Parser<'a> {
         // e.g. `T: for<'a> 'a` or `T: ~const 'a`.
         if self.token.is_lifetime() {
             let _: ErrorGuaranteed = self.error_lt_bound_with_modifiers(modifiers, binder_span);
-            return self.parse_generic_lt_bound(lo, has_parens);
+            return self.parse_generic_lt_bound(lo, parens);
         }
 
         if let (more_lifetime_defs, Some(binder_span)) = self.parse_late_bound_lifetime_defs()? {
@@ -1171,7 +1192,7 @@ impl<'a> Parser<'a> {
             self.recover_fn_trait_with_lifetime_params(&mut path, &mut lifetime_defs)?;
         }
 
-        if has_parens {
+        if let ast::Parens::Yes = parens {
             // Someone has written something like `&dyn (Trait + Other)`. The correct code
             // would be `&(dyn Trait + Other)`
             if self.token.is_like_plus() && leading_token.is_keyword(kw::Dyn) {
@@ -1191,7 +1212,7 @@ impl<'a> Parser<'a> {
         }
 
         let poly_trait =
-            PolyTraitRef::new(lifetime_defs, path, modifiers, lo.to(self.prev_token.span));
+            PolyTraitRef::new(lifetime_defs, path, modifiers, lo.to(self.prev_token.span), parens);
         Ok(GenericBound::Trait(poly_trait))
     }
 

compiler/rustc_resolve/src/late/diagnostics.rs

@@ -3832,6 +3832,7 @@ fn mk_where_bound_predicate(
                 ref_id: DUMMY_NODE_ID,
             },
             span: DUMMY_SP,
+            parens: ast::Parens::No,
         })],
     };