Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle incorrect placement of parentheses in trait bounds more gracefully #84896

Merged
merged 1 commit into from
May 7, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 36 additions & 3 deletions compiler/rustc_parse/src/parser/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,7 @@ impl<'a> Parser<'a> {
/// Is a `dyn B0 + ... + Bn` type allowed here?
fn is_explicit_dyn_type(&mut self) -> bool {
self.check_keyword(kw::Dyn)
&& (self.token.uninterpolated_span().rust_2018()
&& (!self.token.uninterpolated_span().rust_2015()
|| self.look_ahead(1, |t| {
t.can_begin_bound() && !can_continue_type_after_non_fn_ident(t)
}))
Expand Down Expand Up @@ -539,7 +539,21 @@ impl<'a> Parser<'a> {
) -> PResult<'a, GenericBounds> {
let mut bounds = Vec::new();
let mut negative_bounds = Vec::new();
while self.can_begin_bound() {

while self.can_begin_bound() || self.token.is_keyword(kw::Dyn) {
if self.token.is_keyword(kw::Dyn) {
// Account for `&dyn Trait + dyn Other`.
self.struct_span_err(self.token.span, "invalid `dyn` keyword")
.help("`dyn` is only needed at the start of a trait `+`-separated list")
.span_suggestion(
self.token.span,
"remove this keyword",
String::new(),
Applicability::MachineApplicable,
)
.emit();
self.bump();
}
match self.parse_generic_bound()? {
Ok(bound) => bounds.push(bound),
Err(neg_sp) => negative_bounds.push(neg_sp),
Expand Down Expand Up @@ -721,7 +735,26 @@ impl<'a> Parser<'a> {
let lifetime_defs = self.parse_late_bound_lifetime_defs()?;
let path = self.parse_path(PathStyle::Type)?;
if has_parens {
self.expect(&token::CloseDelim(token::Paren))?;
if self.token.is_like_plus() {
// Someone has written something like `&dyn (Trait + Other)`. The correct code
// would be `&(dyn Trait + Other)`, but we don't have access to the appropriate
// span to suggest that. When written as `&dyn Trait + Other`, an appropriate
// suggestion is given.
let bounds = vec![];
self.parse_remaining_bounds(bounds, true)?;
self.expect(&token::CloseDelim(token::Paren))?;
let sp = vec![lo, self.prev_token.span];
let sugg: Vec<_> = sp.iter().map(|sp| (*sp, String::new())).collect();
self.struct_span_err(sp, "incorrect braces around trait bounds")
.multipart_suggestion(
"remove the parentheses",
sugg,
Applicability::MachineApplicable,
)
.emit();
} else {
self.expect(&token::CloseDelim(token::Paren))?;
}
}

let modifier = modifiers.to_trait_bound_modifier();
Expand Down
17 changes: 17 additions & 0 deletions src/test/ui/parser/trait-object-delimiters.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// edition:2018

fn foo1(_: &dyn Drop + AsRef<str>) {} //~ ERROR ambiguous `+` in a type
//~^ ERROR only auto traits can be used as additional traits in a trait object

fn foo2(_: &dyn (Drop + AsRef<str>)) {} //~ ERROR incorrect braces around trait bounds

fn foo3(_: &dyn {Drop + AsRef<str>}) {} //~ ERROR expected parameter name, found `{`
//~^ ERROR expected one of `!`, `(`, `)`, `,`, `?`, `for`, lifetime, or path, found `{`
//~| ERROR at least one trait is required for an object type

fn foo4(_: &dyn <Drop + AsRef<str>>) {} //~ ERROR expected identifier, found `<`

fn foo5(_: &(dyn Drop + dyn AsRef<str>)) {} //~ ERROR invalid `dyn` keyword
//~^ ERROR only auto traits can be used as additional traits in a trait object

fn main() {}
77 changes: 77 additions & 0 deletions src/test/ui/parser/trait-object-delimiters.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
error: ambiguous `+` in a type
--> $DIR/trait-object-delimiters.rs:3:13
|
LL | fn foo1(_: &dyn Drop + AsRef<str>) {}
| ^^^^^^^^^^^^^^^^^^^^^ help: use parentheses to disambiguate: `(dyn Drop + AsRef<str>)`

error: incorrect braces around trait bounds
--> $DIR/trait-object-delimiters.rs:6:17
|
LL | fn foo2(_: &dyn (Drop + AsRef<str>)) {}
| ^ ^
|
help: remove the parentheses
|
LL | fn foo2(_: &dyn Drop + AsRef<str>) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This suggestion seems odd, since if you made this change, you'd then get the error above of "use parentheses to disambiguate". Shouldn't this suggest to use &(dyn Drop + AsRef<str>) instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is less than ideal. But I don't think we have the right information to give that suggestion. This will eventually lead the user to the right solution though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An unwritten rule is that we are ok with giving incomplete suggestions if the resulting code will give you a second correct suggestion. We try to get ahead and give a good direct fix when possible, but the amount of metadata we'd need to carry seems low benefit for the maintenance burden. Particularly in the parser, I have to be very careful not to introduce invalid changes to the grammar by accident.

| -- --

error: expected parameter name, found `{`
--> $DIR/trait-object-delimiters.rs:8:17
|
LL | fn foo3(_: &dyn {Drop + AsRef<str>}) {}
| ^ expected parameter name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really weird to me — it's not expecting a parameter name, but rather a type/trait, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a consequence of the recovery machinery. Making the errors for the {} case better regressed other errors involving trait T: A + B {, so I decided not to address it in this PR.


error: expected one of `!`, `(`, `)`, `,`, `?`, `for`, lifetime, or path, found `{`
--> $DIR/trait-object-delimiters.rs:8:17
|
LL | fn foo3(_: &dyn {Drop + AsRef<str>}) {}
| -^ expected one of 8 possible tokens
| |
| help: missing `,`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also misleading.


error: expected identifier, found `<`
--> $DIR/trait-object-delimiters.rs:12:17
|
LL | fn foo4(_: &dyn <Drop + AsRef<str>>) {}
| ^ expected identifier
Comment on lines +18 to +36
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that these two give different errors is a bit confusing. One expects a parameter and the other an identifier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree. I had a fix that improved this case, but it regressed other cases on malformed super-traits badly, so I pulled it of. I still left the test in in case we ever revisit this.


error: invalid `dyn` keyword
--> $DIR/trait-object-delimiters.rs:14:25
|
LL | fn foo5(_: &(dyn Drop + dyn AsRef<str>)) {}
| ^^^ help: remove this keyword
|
= help: `dyn` is only needed at the start of a trait `+`-separated list

error[E0225]: only auto traits can be used as additional traits in a trait object
--> $DIR/trait-object-delimiters.rs:3:24
|
LL | fn foo1(_: &dyn Drop + AsRef<str>) {}
| ---- ^^^^^^^^^^ additional non-auto trait
| |
| first non-auto trait
|
= help: consider creating a new trait with all of these as super-traits and using that trait here instead: `trait NewTrait: Drop + AsRef<str> {}`
= note: auto-traits like `Send` and `Sync` are traits that have special properties; for more information on them, visit <https://doc.rust-lang.org/reference/special-types-and-traits.html#auto-traits>

error[E0224]: at least one trait is required for an object type
--> $DIR/trait-object-delimiters.rs:8:13
|
LL | fn foo3(_: &dyn {Drop + AsRef<str>}) {}
| ^^^

error[E0225]: only auto traits can be used as additional traits in a trait object
--> $DIR/trait-object-delimiters.rs:14:29
|
LL | fn foo5(_: &(dyn Drop + dyn AsRef<str>)) {}
| ---- ^^^^^^^^^^ additional non-auto trait
| |
| first non-auto trait
|
= help: consider creating a new trait with all of these as super-traits and using that trait here instead: `trait NewTrait: Drop + AsRef<str> {}`
= note: auto-traits like `Send` and `Sync` are traits that have special properties; for more information on them, visit <https://doc.rust-lang.org/reference/special-types-and-traits.html#auto-traits>

error: aborting due to 9 previous errors

Some errors have detailed explanations: E0224, E0225.
For more information about an error, try `rustc --explain E0224`.