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.

Sign up for GitHub

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

Jump to bottom

name async generators something more human friendly in type error diagnostic #81496

Merged
merged 2 commits into from
Feb 19, 2021
Merged

name async generators something more human friendly in type error diagnostic #81496

merged 2 commits into from
Feb 19, 2021

Conversation

guswynn
Copy link
Contributor

@guswynn guswynn commented Jan 29, 2021

fixes #81457

Some details:

  1. I opted to load the generator kind from the hir in TyCategory. I also use 1 impl in the hir for the descr
  2. I named both the source of the future, in addition to the general type (future), not sure what is preferred
  3. I am not sure what is required to make sure "generator" is not referred to anywhere. A brief rg "\"generator\"" showed me that most diagnostics correctly distinguish from generators and async generator, but the descrofDefKind` is pretty general (not sure how thats used)
  4. should the descr impl of AsyncGeneratorKind use its display impl instead of copying the string?

@rust-highfive
Copy link
Collaborator

r? @oli-obk

(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 Jan 29, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@guswynn guswynn force-pushed the expected_async_block branch 2 times, most recently from e3ce9a6 to 8eb484d Compare January 29, 2021 02:39
@rust-log-analyzer

This comment has been minimized.

Sign in to view
oli-obk
oli-obk requested changes Jan 29, 2021
View reviewed changes
Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

What do you think about the change in diagnostics (leaving out the future part?)

src/test/ui/issues/issue-81457.rs Outdated Show resolved Hide resolved
compiler/rustc_hir/src/hir.rs Outdated Show resolved Hide resolved
@JohnCSimon
Copy link
Member

Ping from triage - @guswynn
can you please resolve the build failure?

@rustbot label: -S-waiting-on-review +S-waiting-on-author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 14, 2021
@guswynn
Copy link
Contributor Author

guswynn commented Feb 14, 2021 via email

@guswynn
Copy link
Contributor Author

guswynn commented Feb 15, 2021

Fixed the build issue (apparently ui/issues is too big these days?)

also, @oli-obk It appears that async fn's trigger a different path with arguably better (tho somewhat confusing) wording, and async closures also have better wording

I think the async fn one could be improved, but as a separate pr. I have left it in the test because we should have a test for the 3 main async generator types regardless

@guswynn guswynn mentioned this pull request Feb 16, 2021
Merged
@oli-obk
Copy link
Contributor

oli-obk commented Feb 18, 2021

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 18, 2021

📌 Commit c28d86c has been approved by oli-obk

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 18, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 18, 2021
@Dylan-DPC
Rollup merge of rust-lang#81496 - guswynn:expected_async_block, r=oli…
46b4d8e 
…-obk

name async generators something more human friendly in type error diagnostic

fixes rust-lang#81457

Some details:

1. I opted to load the generator kind from the hir in TyCategory. I also use 1 impl in the hir for the descr
2. I named both the source of the future, in addition to the general type (`future`), not sure what is preferred
3. I am not sure what is required to make sure "generator" is not referred to anywhere. A brief `rg "\"generator\"" showed me that most diagnostics correctly distinguish from generators and async generator, but the `descr` of `DefKind` is pretty general (not sure how thats used)
4. should the descr impl of AsyncGeneratorKind use its display impl instead of copying the string?
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Feb 18, 2021
Closed
@Dylan-DPC-zz
Copy link

failed in rollup

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 18, 2021
@guswynn
Copy link
Contributor Author

guswynn commented Feb 18, 2021

@Dylan-DPC looks like I got unlucky with a merge that made hir.rs larger...I added a commit with the suppression the error suggested

@Dylan-DPC-zz
Copy link

@bors r=oli-obk rollup

@bors
Copy link
Contributor

bors commented Feb 18, 2021

📌 Commit 3e7ea40 has been approved by oli-obk

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 18, 2021
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Feb 19, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 19, 2021
@bors
Auto merge of rust-lang#82281 - Dylan-DPC:rollup-raob2tu, r=Dylan-DPC
8599bff 
Rollup of 10 pull requests

Successful merges:

 - rust-lang#79747 (Add explanations and suggestions to `irrefutable_let_patterns` lint)
 - rust-lang#81496 (name async generators something more human friendly in type error diagnostic)
 - rust-lang#81873 (Add Mutex::unlock)
 - rust-lang#82093 (Add tests for Atomic*::fetch_{min,max})
 - rust-lang#82238 (ast: Keep expansion status for out-of-line module items)
 - rust-lang#82245 (Do not ICE when evaluating locals' types of invalid `yield`)
 - rust-lang#82259 (Fix popping singleton paths in when generating E0433)
 - rust-lang#82261 (rustdoc: Support argument files)
 - rust-lang#82274 (libtest: Fix unwrap panic on duplicate TestDesc)
 - rust-lang#82275 (Update cargo)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f468fd1 into rust-lang:master Feb 19, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk requested changes

Assignees

@oli-obk oli-obk

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.52.0
Development

Successfully merging this pull request may close these issues.

Diagnostic involving the type of an async block refers to generator's
8 participants
@guswynn @rust-highfive @rust-log-analyzer @JohnCSimon @oli-obk @bors @Dylan-DPC-zz @rustbot