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

Conversation

estebank
Copy link
Contributor

@estebank estebank commented May 4, 2021

Fix #84772.

CC @jonhoo

@rust-highfive
Copy link
Collaborator

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 4, 2021
Copy link
Member

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

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

One small comment, but I'm not sure it's worth following up on in the PR

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

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 `,`

error: expected identifier, found `<`
--> $DIR/trait-object-delimiters.rs:12:17
|
LL | fn foo4(_: &dyn <Drop + AsRef<str>>) {}
| ^ expected identifier
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.

@jackh726
Copy link
Member

jackh726 commented May 4, 2021

@bors r+

@bors
Copy link
Contributor

bors commented May 4, 2021

📌 Commit 6b64202 has been approved by jackh726

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 4, 2021
@jackh726
Copy link
Member

jackh726 commented May 4, 2021

@bors rollup

|
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.

--> $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.

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.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 5, 2021
Handle incorrect placement of parentheses in trait bounds more gracefully

Fix rust-lang#84772.

CC `@jonhoo`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 5, 2021
Handle incorrect placement of parentheses in trait bounds more gracefully

Fix rust-lang#84772.

CC ``@jonhoo``
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 6, 2021
Handle incorrect placement of parentheses in trait bounds more gracefully

Fix rust-lang#84772.

CC ```@jonhoo```
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 6, 2021
Handle incorrect placement of parentheses in trait bounds more gracefully

Fix rust-lang#84772.

CC ````@jonhoo````
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 6, 2021
Handle incorrect placement of parentheses in trait bounds more gracefully

Fix rust-lang#84772.

CC `````@jonhoo`````
bors added a commit to rust-lang-ci/rust that referenced this pull request May 7, 2021
Rollup of 11 pull requests

Successful merges:

 - rust-lang#84409 (Ensure TLS destructors run before thread joins in SGX)
 - rust-lang#84500 (Add --run flag to compiletest)
 - rust-lang#84728 (Add test for suggestion to borrow unsized function parameters)
 - rust-lang#84734 (Add `needs-unwind` and beginning of support for testing `panic=abort` std to compiletest)
 - rust-lang#84755 (Allow using `core::` in intra-doc links within core itself)
 - rust-lang#84871 (Disallows `#![feature(no_coverage)]` on stable and beta (using standard crate-level gating))
 - rust-lang#84872 (Wire up tidy dependency checks for cg_clif)
 - rust-lang#84896 (Handle incorrect placement of parentheses in trait bounds more gracefully)
 - rust-lang#84905 (CTFE engine: rename copy → copy_intrinsic, move to intrinsics.rs)
 - rust-lang#84953 (Remove unneeded call to with_default_session_globals in rustdoc highlight)
 - rust-lang#84987 (small nits)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b44e56f into rust-lang:master May 7, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 7, 2021
@estebank estebank deleted the issue-84772 branch November 9, 2023 05:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More helpful output for slightly-wrong dyn with multiple traits.
6 participants