Skip to content

Commit 7d6764a

Browse files
Benjamin Schulzbenschulz
Benjamin Schulz
authored and
benschulz
committed
Detect more cases of unused_parens around types
1 parent e61dd43 commit 7d6764a

File tree

19 files changed

+596
-83
lines changed

19 files changed

+596
-83
lines changed

‎compiler/rustc_ast/src/ast.rs

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1391,6 +1391,7 @@ impl Expr {
13911391
path.clone(),
13921392
TraitBoundModifiers::NONE,
13931393
self.span,
1394+
Parens::No,
13941395
))),
13951396
_ => None,
13961397
}
@@ -3360,6 +3361,13 @@ pub struct TraitRef {
33603361
pub ref_id: NodeId,
33613362
}
33623363

3364+
/// Whether enclosing parentheses are present or not.
3365+
#[derive(Clone, Encodable, Decodable, Debug)]
3366+
pub enum Parens {
3367+
Yes,
3368+
No,
3369+
}
3370+
33633371
#[derive(Clone, Encodable, Decodable, Debug)]
33643372
pub struct PolyTraitRef {
33653373
/// The `'a` in `for<'a> Foo<&'a T>`.
@@ -3372,6 +3380,10 @@ pub struct PolyTraitRef {
33723380
pub trait_ref: TraitRef,
33733381

33743382
pub span: Span,
3383+
3384+
/// When `Yes`, the first and last character of `span` are an opening
3385+
/// and a closing paren respectively.
3386+
pub parens: Parens,
33753387
}
33763388

33773389
impl PolyTraitRef {
@@ -3380,12 +3392,14 @@ impl PolyTraitRef {
33803392
path: Path,
33813393
modifiers: TraitBoundModifiers,
33823394
span: Span,
3395+
parens: Parens,
33833396
) -> Self {
33843397
PolyTraitRef {
33853398
bound_generic_params: generic_params,
33863399
modifiers,
33873400
trait_ref: TraitRef { path, ref_id: DUMMY_NODE_ID },
33883401
span,
3402+
parens,
33893403
}
33903404
}
33913405
}

‎compiler/rustc_ast/src/visit.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1142,7 +1142,7 @@ macro_rules! common_visitor_and_walkers {
11421142
vis: &mut V,
11431143
p: &$($lt)? $($mut)? PolyTraitRef,
11441144
) -> V::Result {
1145-
let PolyTraitRef { bound_generic_params, modifiers, trait_ref, span } = p;
1145+
let PolyTraitRef { bound_generic_params, modifiers, trait_ref, span, parens: _ } = p;
11461146
try_visit!(visit_modifiers(vis, modifiers));
11471147
try_visit!(visit_generic_params(vis, bound_generic_params));
11481148
try_visit!(vis.visit_trait_ref(trait_ref));

‎compiler/rustc_ast_lowering/src/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1209,6 +1209,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
12091209
modifiers: TraitBoundModifiers::NONE,
12101210
trait_ref: TraitRef { path: path.clone(), ref_id: t.id },
12111211
span: t.span,
1212+
parens: ast::Parens::No,
12121213
},
12131214
itctx,
12141215
);

‎compiler/rustc_expand/src/build.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,7 @@ impl<'a> ExtCtxt<'a> {
195195
},
196196
trait_ref: self.trait_ref(path),
197197
span,
198+
parens: ast::Parens::No,
198199
}
199200
}
200201

‎compiler/rustc_lint/src/unused.rs

Lines changed: 118 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,15 @@ use std::iter;
33
use rustc_ast::util::{classify, parser};
44
use rustc_ast::{self as ast, ExprKind, HasAttrs as _, StmtKind};
55
use rustc_attr_data_structures::{AttributeKind, find_attr};
6+
use rustc_data_structures::fx::FxHashMap;
67
use rustc_errors::{MultiSpan, pluralize};
78
use rustc_hir::def::{DefKind, Res};
89
use rustc_hir::def_id::DefId;
910
use rustc_hir::{self as hir, LangItem};
1011
use rustc_infer::traits::util::elaborate;
1112
use rustc_middle::ty::{self, Ty, adjustment};
1213
use rustc_session::{declare_lint, declare_lint_pass, impl_lint_pass};
14+
use rustc_span::edition::Edition::Edition2015;
1315
use rustc_span::{BytePos, Span, Symbol, kw, sym};
1416
use tracing::instrument;
1517

@@ -1032,6 +1034,31 @@ pub(crate) struct UnusedParens {
10321034
/// `1 as (i32) < 2` parses to ExprKind::Lt
10331035
/// `1 as i32 < 2` parses to i32::<2[missing angle bracket]
10341036
parens_in_cast_in_lt: Vec<ast::NodeId>,
1037+
/// Ty nodes in this map are in TypeNoBounds position. Any bounds they
1038+
/// contain may be ambiguous w/r/t trailing `+` operators.
1039+
in_no_bounds_pos: FxHashMap<ast::NodeId, NoBoundsException>,
1040+
}
1041+
1042+
/// Whether parentheses may be omitted from a type without resulting in ambiguity.
1043+
///
1044+
/// ```
1045+
/// type Example = Box<dyn Fn() -> &'static (dyn Send) + Sync>;
1046+
/// ```
1047+
///
1048+
/// Here, `&'static (dyn Send) + Sync` is a `TypeNoBounds`. As such, it may not directly
1049+
/// contain `ImplTraitType` or `TraitObjectType` which is why `(dyn Send)` is parenthesized.
1050+
/// However, an exception is made for `ImplTraitTypeOneBound` and `TraitObjectTypeOneBound`.
1051+
/// The following is accepted because there is no `+`.
1052+
///
1053+
/// ```
1054+
/// type Example = Box<dyn Fn() -> &'static dyn Send>;
1055+
/// ```
1056+
enum NoBoundsException {
1057+
/// The type must be parenthesized.
1058+
None,
1059+
/// The type is the last bound of the containing type expression. If it has exactly one bound,
1060+
/// parentheses around the type are unnecessary.
1061+
OneBound,
10351062
}
10361063

10371064
impl_lint_pass!(UnusedParens => [UNUSED_PARENS]);
@@ -1275,23 +1302,100 @@ impl EarlyLintPass for UnusedParens {
12751302
);
12761303
}
12771304
ast::TyKind::Paren(r) => {
1278-
match &r.kind {
1279-
ast::TyKind::TraitObject(..) => {}
1280-
ast::TyKind::BareFn(b)
1281-
if self.with_self_ty_parens && b.generic_params.len() > 0 => {}
1282-
ast::TyKind::ImplTrait(_, bounds) if bounds.len() > 1 => {}
1283-
_ => {
1284-
let spans = if !ty.span.from_expansion() {
1305+
let unused_parens = match &r.kind {
1306+
ast::TyKind::ImplTrait(_, bounds) | ast::TyKind::TraitObject(bounds, _) => {
1307+
match self.in_no_bounds_pos.get(&ty.id) {
1308+
Some(NoBoundsException::None) => false,
1309+
Some(NoBoundsException::OneBound) => bounds.len() <= 1,
1310+
None => true,
1311+
}
1312+
}
1313+
ast::TyKind::BareFn(b) => {
1314+
!self.with_self_ty_parens || b.generic_params.is_empty()
1315+
}
1316+
_ => true,
1317+
};
1318+
1319+
if unused_parens {
1320+
let spans = (!ty.span.from_expansion())
1321+
.then(|| {
12851322
r.span
12861323
.find_ancestor_inside(ty.span)
12871324
.map(|r| (ty.span.with_hi(r.lo()), ty.span.with_lo(r.hi())))
1325+
})
1326+
.flatten();
1327+
1328+
self.emit_unused_delims(cx, ty.span, spans, "type", (false, false), false);
1329+
}
1330+
1331+
self.with_self_ty_parens = false;
1332+
}
1333+
ast::TyKind::Ref(_, mut_ty) | ast::TyKind::Ptr(mut_ty) => {
1334+
self.in_no_bounds_pos.insert(mut_ty.ty.id, NoBoundsException::OneBound);
1335+
}
1336+
ast::TyKind::TraitObject(bounds, _) | ast::TyKind::ImplTrait(_, bounds) => {
1337+
for i in 0..bounds.len() {
1338+
let is_last = i == bounds.len() - 1;
1339+
1340+
if let ast::GenericBound::Trait(poly_trait_ref) = &bounds[i] {
1341+
let fn_with_explicit_ret_ty = if let [.., segment] =
1342+
&*poly_trait_ref.trait_ref.path.segments
1343+
&& let Some(args) = segment.args.as_ref()
1344+
&& let ast::GenericArgs::Parenthesized(paren_args) = &**args
1345+
&& let ast::FnRetTy::Ty(ret_ty) = &paren_args.output
1346+
{
1347+
self.in_no_bounds_pos.insert(
1348+
ret_ty.id,
1349+
if is_last {
1350+
NoBoundsException::OneBound
1351+
} else {
1352+
NoBoundsException::None
1353+
},
1354+
);
1355+
1356+
true
12881357
} else {
1289-
None
1358+
false
12901359
};
1291-
self.emit_unused_delims(cx, ty.span, spans, "type", (false, false), false);
1360+
1361+
// In edition 2015, dyn is a contextual keyword and `dyn::foo::Bar` is
1362+
// parsed as a path, so parens are necessary to disambiguate. See
1363+
// - tests/ui/lint/unused/unused-parens-trait-obj-e2015.rs and
1364+
// - https://doc.rust-lang.org/reference/types/trait-object.html#r-type.trait-object.syntax-edition2018
1365+
let dyn2015_exception = cx.sess().psess.edition == Edition2015
1366+
&& matches!(ty.kind, ast::TyKind::TraitObject(..))
1367+
&& i == 0
1368+
&& poly_trait_ref
1369+
.trait_ref
1370+
.path
1371+
.segments
1372+
.first()
1373+
.map(|s| s.ident.name == kw::PathRoot)
1374+
.unwrap_or(false);
1375+
1376+
if let ast::Parens::Yes = poly_trait_ref.parens
1377+
&& (is_last || !fn_with_explicit_ret_ty)
1378+
&& !dyn2015_exception
1379+
{
1380+
let s = poly_trait_ref.span;
1381+
let spans = (!s.from_expansion()).then(|| {
1382+
(
1383+
s.with_hi(s.lo() + rustc_span::BytePos(1)),
1384+
s.with_lo(s.hi() - rustc_span::BytePos(1)),
1385+
)
1386+
});
1387+
1388+
self.emit_unused_delims(
1389+
cx,
1390+
poly_trait_ref.span,
1391+
spans,
1392+
"type",
1393+
(false, false),
1394+
false,
1395+
);
1396+
}
12921397
}
12931398
}
1294-
self.with_self_ty_parens = false;
12951399
}
12961400
_ => {}
12971401
}
@@ -1301,6 +1405,10 @@ impl EarlyLintPass for UnusedParens {
13011405
<Self as UnusedDelimLint>::check_item(self, cx, item)
13021406
}
13031407

1408+
fn check_item_post(&mut self, _: &EarlyContext<'_>, _: &rustc_ast::Item) {
1409+
self.in_no_bounds_pos.clear();
1410+
}
1411+
13041412
fn enter_where_predicate(&mut self, _: &EarlyContext<'_>, pred: &ast::WherePredicate) {
13051413
use rustc_ast::{WhereBoundPredicate, WherePredicateKind};
13061414
if let WherePredicateKind::BoundPredicate(WhereBoundPredicate {

‎compiler/rustc_parse/src/parser/ty.rs

Lines changed: 37 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -305,8 +305,13 @@ impl<'a> Parser<'a> {
305305
let removal_span = kw.span.with_hi(self.token.span.lo());
306306
let path = self.parse_path(PathStyle::Type)?;
307307
let parse_plus = allow_plus == AllowPlus::Yes && self.check_plus();
308-
let kind =
309-
self.parse_remaining_bounds_path(lifetime_defs, path, lo, parse_plus)?;
308+
let kind = self.parse_remaining_bounds_path(
309+
lifetime_defs,
310+
path,
311+
lo,
312+
parse_plus,
313+
ast::Parens::No,
314+
)?;
310315
let err = self.dcx().create_err(errors::TransposeDynOrImpl {
311316
span: kw.span,
312317
kw: kw.name.as_str(),
@@ -333,7 +338,13 @@ impl<'a> Parser<'a> {
333338
} else {
334339
let path = self.parse_path(PathStyle::Type)?;
335340
let parse_plus = allow_plus == AllowPlus::Yes && self.check_plus();
336-
self.parse_remaining_bounds_path(lifetime_defs, path, lo, parse_plus)?
341+
self.parse_remaining_bounds_path(
342+
lifetime_defs,
343+
path,
344+
lo,
345+
parse_plus,
346+
ast::Parens::No,
347+
)?
337348
}
338349
}
339350
} else if self.eat_keyword(exp!(Impl)) {
@@ -413,9 +424,13 @@ impl<'a> Parser<'a> {
413424
let maybe_bounds = allow_plus == AllowPlus::Yes && self.token.is_like_plus();
414425
match ty.kind {
415426
// `(TY_BOUND_NOPAREN) + BOUND + ...`.
416-
TyKind::Path(None, path) if maybe_bounds => {
417-
self.parse_remaining_bounds_path(ThinVec::new(), path, lo, true)
418-
}
427+
TyKind::Path(None, path) if maybe_bounds => self.parse_remaining_bounds_path(
428+
ThinVec::new(),
429+
path,
430+
lo,
431+
true,
432+
ast::Parens::Yes,
433+
),
419434
// For `('a) + …`, we know that `'a` in type position already lead to an error being
420435
// emitted. To reduce output, let's indirectly suppress E0178 (bad `+` in type) and
421436
// other irrelevant consequential errors.
@@ -495,12 +510,14 @@ impl<'a> Parser<'a> {
495510
path: ast::Path,
496511
lo: Span,
497512
parse_plus: bool,
513+
parens: ast::Parens,
498514
) -> PResult<'a, TyKind> {
499515
let poly_trait_ref = PolyTraitRef::new(
500516
generic_params,
501517
path,
502518
TraitBoundModifiers::NONE,
503519
lo.to(self.prev_token.span),
520+
parens,
504521
);
505522
let bounds = vec![GenericBound::Trait(poly_trait_ref)];
506523
self.parse_remaining_bounds(bounds, parse_plus)
@@ -832,7 +849,7 @@ impl<'a> Parser<'a> {
832849
Ok(TyKind::MacCall(P(MacCall { path, args: self.parse_delim_args()? })))
833850
} else if allow_plus == AllowPlus::Yes && self.check_plus() {
834851
// `Trait1 + Trait2 + 'a`
835-
self.parse_remaining_bounds_path(ThinVec::new(), path, lo, true)
852+
self.parse_remaining_bounds_path(ThinVec::new(), path, lo, true, ast::Parens::No)
836853
} else {
837854
// Just a type path.
838855
Ok(TyKind::Path(None, path))
@@ -897,10 +914,10 @@ impl<'a> Parser<'a> {
897914
fn parse_generic_bound(&mut self) -> PResult<'a, GenericBound> {
898915
let lo = self.token.span;
899916
let leading_token = self.prev_token;
900-
let has_parens = self.eat(exp!(OpenParen));
917+
let parens = if self.eat(exp!(OpenParen)) { ast::Parens::Yes } else { ast::Parens::No };
901918

902919
let bound = if self.token.is_lifetime() {
903-
self.parse_generic_lt_bound(lo, has_parens)?
920+
self.parse_generic_lt_bound(lo, parens)?
904921
} else if self.eat_keyword(exp!(Use)) {
905922
// parse precise captures, if any. This is `use<'lt, 'lt, P, P>`; a list of
906923
// lifetimes and ident params (including SelfUpper). These are validated later
@@ -909,7 +926,7 @@ impl<'a> Parser<'a> {
909926
let (args, args_span) = self.parse_precise_capturing_args()?;
910927
GenericBound::Use(args, use_span.to(args_span))
911928
} else {
912-
self.parse_generic_ty_bound(lo, has_parens, &leading_token)?
929+
self.parse_generic_ty_bound(lo, parens, &leading_token)?
913930
};
914931

915932
Ok(bound)
@@ -919,10 +936,14 @@ impl<'a> Parser<'a> {
919936
/// ```ebnf
920937
/// LT_BOUND = LIFETIME
921938
/// ```
922-
fn parse_generic_lt_bound(&mut self, lo: Span, has_parens: bool) -> PResult<'a, GenericBound> {
939+
fn parse_generic_lt_bound(
940+
&mut self,
941+
lo: Span,
942+
parens: ast::Parens,
943+
) -> PResult<'a, GenericBound> {
923944
let lt = self.expect_lifetime();
924945
let bound = GenericBound::Outlives(lt);
925-
if has_parens {
946+
if let ast::Parens::Yes = parens {
926947
// FIXME(Centril): Consider not erroring here and accepting `('lt)` instead,
927948
// possibly introducing `GenericBound::Paren(P<GenericBound>)`?
928949
self.recover_paren_lifetime(lo)?;
@@ -1078,7 +1099,7 @@ impl<'a> Parser<'a> {
10781099
fn parse_generic_ty_bound(
10791100
&mut self,
10801101
lo: Span,
1081-
has_parens: bool,
1102+
parens: ast::Parens,
10821103
leading_token: &Token,
10831104
) -> PResult<'a, GenericBound> {
10841105
let (mut lifetime_defs, binder_span) = self.parse_late_bound_lifetime_defs()?;
@@ -1104,7 +1125,7 @@ impl<'a> Parser<'a> {
11041125
// e.g. `T: for<'a> 'a` or `T: ~const 'a`.
11051126
if self.token.is_lifetime() {
11061127
let _: ErrorGuaranteed = self.error_lt_bound_with_modifiers(modifiers, binder_span);
1107-
return self.parse_generic_lt_bound(lo, has_parens);
1128+
return self.parse_generic_lt_bound(lo, parens);
11081129
}
11091130

11101131
if let (more_lifetime_defs, Some(binder_span)) = self.parse_late_bound_lifetime_defs()? {
@@ -1171,7 +1192,7 @@ impl<'a> Parser<'a> {
11711192
self.recover_fn_trait_with_lifetime_params(&mut path, &mut lifetime_defs)?;
11721193
}
11731194

1174-
if has_parens {
1195+
if let ast::Parens::Yes = parens {
11751196
// Someone has written something like `&dyn (Trait + Other)`. The correct code
11761197
// would be `&(dyn Trait + Other)`
11771198
if self.token.is_like_plus() && leading_token.is_keyword(kw::Dyn) {
@@ -1191,7 +1212,7 @@ impl<'a> Parser<'a> {
11911212
}
11921213

11931214
let poly_trait =
1194-
PolyTraitRef::new(lifetime_defs, path, modifiers, lo.to(self.prev_token.span));
1215+
PolyTraitRef::new(lifetime_defs, path, modifiers, lo.to(self.prev_token.span), parens);
11951216
Ok(GenericBound::Trait(poly_trait))
11961217
}
11971218

‎compiler/rustc_resolve/src/late/diagnostics.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3832,6 +3832,7 @@ fn mk_where_bound_predicate(
38323832
ref_id: DUMMY_NODE_ID,
38333833
},
38343834
span: DUMMY_SP,
3835+
parens: ast::Parens::No,
38353836
})],
38363837
};
38373838

0 commit comments

Comments
 (0)