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

Suggest async {} for async || {} #76580

Merged
merged 1 commit into from Jan 12, 2021
Merged

Suggest async {} for async || {} #76580

merged 1 commit into from Jan 12, 2021

Conversation

rokob
Copy link
Contributor

@rokob rokob commented Sep 10, 2020

Fixes #76011

This adds support for adding help diagnostics to the feature gating checks and
then uses it for the async_closure gate to add the extra bit of help
information as described in the issue.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(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 Sep 10, 2020
@jyn514 jyn514 added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. labels Sep 10, 2020
@jyn514
Copy link
Member

jyn514 commented Sep 10, 2020

r? @estebank

@estebank
Copy link
Contributor

estebank commented Sep 11, 2020

Can we add a test? It should live somewhere in src/test/ui/** and after the .rs file is created (and test check comments are added in the right place) you can run ./x.py test src/test/ui --stage 1 --bless to autogenerate the .stderr files.

debug!("gate_feature(feature = {:?}, span = {:?}); has? {}", name, span, has_feature);
if !has_feature && !span.allows_unstable($name) {
feature_err_issue(&visitor.sess.parse_sess, name, span, GateIssue::Language, explain)
.span_help(span, help)
Copy link
Contributor

Choose a reason for hiding this comment

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

Because we have a single span, I think this should be a bare help call (without a span) so the output isn't too verbose/redundant. I would not close #76011, as that is asking for a structured suggestion, which we might be able to provide in the future (but this current change is a great mitigation).

Fixes rust-lang#76011

This adds support for adding help diagnostics to the feature gating checks and
then uses it for the async_closure gate to add the extra bit of help
information as described in the issue.
@rokob
Copy link
Contributor Author

rokob commented Sep 22, 2020

I updated the src/test/ui/async-await/feature-async-closure UI test. I also made a change to compiler/rustc_parse/src/parser/expr.rs because otherwise this help would show up in this test src/test/ui/parser/block-no-opening-brace.

I can backout that change if you want and instead have this help show up in that case as well. But looking at that example, it didn't seem right to me that this code:

fn f5() {
    async
        let x = 0;
}

would produce an error about async closures being unstable. You don't know that this is trying to be a closure, it could very well have been an async block which is not unstable. The help message I am adding makes this part even more confusing because it suggests removing the || and replacing it with a { but there is no ||. I think moving the feature gate until after the parsing of the rest of the closure has no semantic impact but makes more sense as you shouldn't assume you have a closure if you don't have |, or ||, or {.

@rokob rokob requested a review from estebank October 5, 2020 21:12
@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 23, 2020
@jyn514
Copy link
Member

jyn514 commented Oct 25, 2020

ping @estebank

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 13, 2020
@jyn514
Copy link
Member

jyn514 commented Nov 19, 2020

ping @estebank - is there a better reviewer for this?

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 11, 2020
@camelid
Copy link
Member

camelid commented Jan 9, 2021

I think estebank is catching up on reviews now.

@estebank
Copy link
Contributor

estebank commented Jan 12, 2021

@bors r+

Noticed the following output

error: expected one of `move`, `|`, or `||`, found keyword `let`
  --> $DIR/block-no-opening-brace.rs:30:9
   |
LL |     async
   |          - expected one of `move`, `|`, or `||`
LL |         let x = 0;
   |         ^^^ unexpected token

should also include { as an expected token, but that's independent of this. Edit: Created #80931.

@bors
Copy link
Contributor

bors commented Jan 12, 2021

📌 Commit 5b475a4 has been approved by estebank

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

bors commented Jan 12, 2021

⌛ Testing commit 5b475a4 with merge 467f5e9...

@bors
Copy link
Contributor

bors commented Jan 12, 2021

☀️ Test successful - checks-actions
Approved by: estebank
Pushing 467f5e9 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 12, 2021
@bors bors merged commit 467f5e9 into rust-lang:master Jan 12, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggest async {} for async || {}
9 participants