-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
name async generators something more human friendly in type error diagnostic #81496
Conversation
r? @oli-obk (rust-highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
e3ce9a6
to
8eb484d
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this 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?)
Yah, I will have time for this tomorrow!
…On Sun, Feb 14, 2021, 9:07 AM John Simon ***@***.***> wrote:
Ping from triage - @guswynn <https://github.com/guswynn>
can you please resolve the build failure?
@rustbot <https://github.com/rustbot> label: -S-waiting-on-review
+S-waiting-on-author
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#81496 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABJHND24ZTNHLER6RHQF65LS677MJANCNFSM4WYDKGHA>
.
|
8eb484d
to
0019a00
Compare
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 |
0019a00
to
e4bdeef
Compare
e4bdeef
to
c28d86c
Compare
@bors r+ rollup |
📌 Commit c28d86c has been approved by |
…-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 looks like I got unlucky with a merge that made hir.rs larger...I added a commit with the suppression the error suggested |
@bors r=oli-obk rollup |
📌 Commit 3e7ea40 has been approved by |
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
fixes #81457
Some details:
future
), not sure what is preferredrg "\"generator\"" showed me that most diagnostics correctly distinguish from generators and async generator, but the
descrof
DefKind` is pretty general (not sure how thats used)