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

In pretty_print_type(), print async fn futures' paths instead of spans. #122923

Merged
merged 1 commit into from
Mar 26, 2024

Conversation

kpreid
Copy link
Contributor

@kpreid kpreid commented Mar 23, 2024

This makes -Zprint-type-sizes's output easier to read, because the name of an async fn is more immediately recognizable than its span. This change will also synergize with my other -Zprint-type-sizes PR #122922 which prints the type of child futures being awaited.

I also deleted the comment "FIXME(eddyb) should use def_span." because it appears to have already been fixed by commit 67727aa.

@rustbot
Copy link
Collaborator

rustbot commented Mar 23, 2024

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 23, 2024
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

I don't mind this approach, but I think it needs some sort of delimiter to separate the "async fn body" part and the def path - {async fn body operation} is hard to parse over, e.g. {async fn body @ operation} or {async fn body - operation} something.

@kpreid
Copy link
Contributor Author

kpreid commented Mar 23, 2024

I don't mind this approach, but I think it needs some sort of delimiter to separate the "async fn body" part and the def path - {async fn body operation} is hard to parse over, e.g. {async fn body @ operation} or {async fn body - operation} something.

I was having that concern myself, but I couldn't think of a really good syntax. The "@" seems appropriate to keep only where it is currently used: before a span. How about {async fn body of: operation}? Maybe with function call parens?

@compiler-errors
Copy link
Member

compiler-errors commented Mar 23, 2024

of and : together seems a bit excessive. I think : is probably fine on its own. parens are fine i guess too

@compiler-errors
Copy link
Member

Given this change is a bit more involved, I'm gonna cc @rust-lang/wg-diagnostics to see if anyone has thoughts. Given it's friday, I'm happy to r+ this on like monday or tuesday.

@kpreid
Copy link
Contributor Author

kpreid commented Mar 23, 2024

A colon can have lots of meanings. My intuition about including the actual English word "of" is that new readers really don't know what all this punctuation and terminology means, and including "of" makes it much closer to immediately clear what we are talking about: this type is the "async fn body" of the named function.

@rust-log-analyzer

This comment has been minimized.

@kpreid
Copy link
Contributor Author

kpreid commented Mar 23, 2024

I pushed a new version which prints like {async fn body of name()}, or in a more complex example from tests/ui/async-await/future-sizes/async-awaiting-fut.stdout,

std::mem::MaybeUninit<{async fn body of calls_fut<{async fn body of big_fut()}>()}>

I think that reads pretty well — the lack of turbofish isn't my fault. 😁

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 23, 2024
-Zprint-type-sizes: print the types of awaitees and unnamed coroutine locals.

This should assist comprehending the size of coroutines. In particular, whenever a future is suspended while awaiting another future, the latter is given the special name `__awaitee`, and now the type of the awaited future will be printed, allowing identifying caller/callee — er, I mean, poller/pollee — relationships.

It would be possible to include the type name in more cases, but I thought that that might be overly verbose (`print-type-sizes` is already a lot of text) and ordinary named fields or variables are easier for readers to discover the types of.

This change will also synergize with my other PR rust-lang#122923 which changes type printing to print the path of the `async fn` instead of the span.

Implementation note: I'm not sure if `Symbol::intern` is appropriate for this application, but it was the obvious way to not have to remove the `Copy` implementation from `FieldInfo`, or add a `'tcx` lifetime, while avoiding keeping a lot of possibly redundant strings in memory. I don't know what the proper tradeoff to make here is (though presumably it is not too important for a `-Z` debugging option).
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Mar 23, 2024
-Zprint-type-sizes: print the types of awaitees and unnamed coroutine locals.

This should assist comprehending the size of coroutines. In particular, whenever a future is suspended while awaiting another future, the latter is given the special name `__awaitee`, and now the type of the awaited future will be printed, allowing identifying caller/callee — er, I mean, poller/pollee — relationships.

It would be possible to include the type name in more cases, but I thought that that might be overly verbose (`print-type-sizes` is already a lot of text) and ordinary named fields or variables are easier for readers to discover the types of.

This change will also synergize with my other PR rust-lang#122923 which changes type printing to print the path of the `async fn` instead of the span.

Implementation note: I'm not sure if `Symbol::intern` is appropriate for this application, but it was the obvious way to not have to remove the `Copy` implementation from `FieldInfo`, or add a `'tcx` lifetime, while avoiding keeping a lot of possibly redundant strings in memory. I don't know what the proper tradeoff to make here is (though presumably it is not too important for a `-Z` debugging option).
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 23, 2024
-Zprint-type-sizes: print the types of awaitees and unnamed coroutine locals.

This should assist comprehending the size of coroutines. In particular, whenever a future is suspended while awaiting another future, the latter is given the special name `__awaitee`, and now the type of the awaited future will be printed, allowing identifying caller/callee — er, I mean, poller/pollee — relationships.

It would be possible to include the type name in more cases, but I thought that that might be overly verbose (`print-type-sizes` is already a lot of text) and ordinary named fields or variables are easier for readers to discover the types of.

This change will also synergize with my other PR rust-lang#122923 which changes type printing to print the path of the `async fn` instead of the span.

Implementation note: I'm not sure if `Symbol::intern` is appropriate for this application, but it was the obvious way to not have to remove the `Copy` implementation from `FieldInfo`, or add a `'tcx` lifetime, while avoiding keeping a lot of possibly redundant strings in memory. I don't know what the proper tradeoff to make here is (though presumably it is not too important for a `-Z` debugging option).
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 23, 2024
-Zprint-type-sizes: print the types of awaitees and unnamed coroutine locals.

This should assist comprehending the size of coroutines. In particular, whenever a future is suspended while awaiting another future, the latter is given the special name `__awaitee`, and now the type of the awaited future will be printed, allowing identifying caller/callee — er, I mean, poller/pollee — relationships.

It would be possible to include the type name in more cases, but I thought that that might be overly verbose (`print-type-sizes` is already a lot of text) and ordinary named fields or variables are easier for readers to discover the types of.

This change will also synergize with my other PR rust-lang#122923 which changes type printing to print the path of the `async fn` instead of the span.

Implementation note: I'm not sure if `Symbol::intern` is appropriate for this application, but it was the obvious way to not have to remove the `Copy` implementation from `FieldInfo`, or add a `'tcx` lifetime, while avoiding keeping a lot of possibly redundant strings in memory. I don't know what the proper tradeoff to make here is (though presumably it is not too important for a `-Z` debugging option).
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 23, 2024
Rollup merge of rust-lang#122922 - kpreid:print-async, r=compiler-errors

-Zprint-type-sizes: print the types of awaitees and unnamed coroutine locals.

This should assist comprehending the size of coroutines. In particular, whenever a future is suspended while awaiting another future, the latter is given the special name `__awaitee`, and now the type of the awaited future will be printed, allowing identifying caller/callee — er, I mean, poller/pollee — relationships.

It would be possible to include the type name in more cases, but I thought that that might be overly verbose (`print-type-sizes` is already a lot of text) and ordinary named fields or variables are easier for readers to discover the types of.

This change will also synergize with my other PR rust-lang#122923 which changes type printing to print the path of the `async fn` instead of the span.

Implementation note: I'm not sure if `Symbol::intern` is appropriate for this application, but it was the obvious way to not have to remove the `Copy` implementation from `FieldInfo`, or add a `'tcx` lifetime, while avoiding keeping a lot of possibly redundant strings in memory. I don't know what the proper tradeoff to make here is (though presumably it is not too important for a `-Z` debugging option).
@compiler-errors
Copy link
Member

ok we'll that's basically consensus

@bors r+

@bors
Copy link
Contributor

bors commented Mar 23, 2024

📌 Commit 3743e3e has been approved by compiler-errors

It is now in the queue for this repository.

@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 Mar 23, 2024
@kpreid
Copy link
Contributor Author

kpreid commented Mar 23, 2024

Rebased and re-blessed on top of #122922 which uses this printing routine more often.

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 23, 2024

📌 Commit b6b5745 has been approved by compiler-errors

It is now in the queue for this repository.

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Mar 24, 2024
…-errors

In `pretty_print_type()`, print `async fn` futures' paths instead of spans.

This makes `-Zprint-type-sizes`'s output easier to read, because the name of an `async fn` is more immediately recognizable than its span. This change will also synergize with my other `-Zprint-type-sizes` PR rust-lang#122922 which prints the type of child futures being awaited.

I also deleted the comment "FIXME(eddyb) should use `def_span`." because it appears to have already been fixed by commit 67727aa.
@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 Mar 24, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 24, 2024
…-errors

In `pretty_print_type()`, print `async fn` futures' paths instead of spans.

This makes `-Zprint-type-sizes`'s output easier to read, because the name of an `async fn` is more immediately recognizable than its span. This change will also synergize with my other `-Zprint-type-sizes` PR rust-lang#122922 which prints the type of child futures being awaited.

I also deleted the comment "FIXME(eddyb) should use `def_span`." because it appears to have already been fixed by commit 67727aa.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 24, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#120557 (Add rust-lldb pretty printing for Path and PathBuf)
 - rust-lang#121051 (Introduce infrastructure for generating target docs)
 - rust-lang#122892 (fix(bootstrap/dist): use versioned dirs when vendoring)
 - rust-lang#122896 (Update stdarch submodule)
 - rust-lang#122923 (In `pretty_print_type()`, print `async fn` futures' paths instead of spans.)
 - rust-lang#122970 (Use `chunk_by` when building `ReverseSccGraph`)
 - rust-lang#123003 (CFI: Handle dyn with no principal)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 25, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#120557 (Add rust-lldb pretty printing for Path and PathBuf)
 - rust-lang#121051 (Introduce infrastructure for generating target docs)
 - rust-lang#122892 (fix(bootstrap/dist): use versioned dirs when vendoring)
 - rust-lang#122896 (Update stdarch submodule)
 - rust-lang#122923 (In `pretty_print_type()`, print `async fn` futures' paths instead of spans.)
 - rust-lang#122970 (Use `chunk_by` when building `ReverseSccGraph`)
 - rust-lang#123003 (CFI: Handle dyn with no principal)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 25, 2024
…-errors

In `pretty_print_type()`, print `async fn` futures' paths instead of spans.

This makes `-Zprint-type-sizes`'s output easier to read, because the name of an `async fn` is more immediately recognizable than its span. This change will also synergize with my other `-Zprint-type-sizes` PR rust-lang#122922 which prints the type of child futures being awaited.

I also deleted the comment "FIXME(eddyb) should use `def_span`." because it appears to have already been fixed by commit 67727aa.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 25, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#122802 (Provide structured suggestion for unconstrained generic constant)
 - rust-lang#122858 (Tweak `parse_dot_suffix_expr`)
 - rust-lang#122923 (In `pretty_print_type()`, print `async fn` futures' paths instead of spans.)
 - rust-lang#122990 (Clarify transmute example)
 - rust-lang#122995 (Clean up unnecessary headers/flags in coverage mir-opt tests)
 - rust-lang#123003 (CFI: Handle dyn with no principal)
 - rust-lang#123005 (CFI: Support complex receivers)
 - rust-lang#123020 (Temporarily remove nnethercote from the review rotation.)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr
Copy link
Member

needs rebase I think #123023 (comment)
@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 Mar 25, 2024
RenjiSann pushed a commit to RenjiSann/rust that referenced this pull request Mar 25, 2024
-Zprint-type-sizes: print the types of awaitees and unnamed coroutine locals.

This should assist comprehending the size of coroutines. In particular, whenever a future is suspended while awaiting another future, the latter is given the special name `__awaitee`, and now the type of the awaited future will be printed, allowing identifying caller/callee — er, I mean, poller/pollee — relationships.

It would be possible to include the type name in more cases, but I thought that that might be overly verbose (`print-type-sizes` is already a lot of text) and ordinary named fields or variables are easier for readers to discover the types of.

This change will also synergize with my other PR rust-lang#122923 which changes type printing to print the path of the `async fn` instead of the span.

Implementation note: I'm not sure if `Symbol::intern` is appropriate for this application, but it was the obvious way to not have to remove the `Copy` implementation from `FieldInfo`, or add a `'tcx` lifetime, while avoiding keeping a lot of possibly redundant strings in memory. I don't know what the proper tradeoff to make here is (though presumably it is not too important for a `-Z` debugging option).
…spans.

This makes `-Zprint-type-sizes`'s output easier to read, because the
name of an `async fn` is more immediately recognizable than its span.

I also deleted the comment "FIXME(eddyb) should use `def_span`." because
it appears to have already been fixed by commit 67727aa.
@kpreid
Copy link
Contributor Author

kpreid commented Mar 25, 2024

needs rebase I think #123023 (comment)

Not actually a conflict (I think) but there was a file that hadn't gotten blessed before because I was having an unrelated problem actually finishing the run of tests/mir-opt. Sorry for the trouble. Fixed.

@Noratrieb
Copy link
Member

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Mar 25, 2024

📌 Commit 3010fa9 has been approved by compiler-errors

It is now in the queue for this repository.

@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 Mar 25, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Mar 25, 2024
…-errors

In `pretty_print_type()`, print `async fn` futures' paths instead of spans.

This makes `-Zprint-type-sizes`'s output easier to read, because the name of an `async fn` is more immediately recognizable than its span. This change will also synergize with my other `-Zprint-type-sizes` PR rust-lang#122922 which prints the type of child futures being awaited.

I also deleted the comment "FIXME(eddyb) should use `def_span`." because it appears to have already been fixed by commit 67727aa.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 25, 2024
…kingjubilee

Rollup of 10 pull requests

Successful merges:

 - rust-lang#122707 (Fix a typo in the alloc::string::String docs)
 - rust-lang#122769 (extend comments for reachability set computation)
 - rust-lang#122892 (fix(bootstrap/dist): use versioned dirs when vendoring)
 - rust-lang#122896 (Update stdarch submodule)
 - rust-lang#122923 (In `pretty_print_type()`, print `async fn` futures' paths instead of spans.)
 - rust-lang#122950 (Add regression tests for rust-lang#101903)
 - rust-lang#122958 (Port backtrace dylib-dep test to a ui test)
 - rust-lang#123039 (Update books)
 - rust-lang#123044 (`Instance` is `Copy`)
 - rust-lang#123051 (did I mention that tests are super cool? )

r? `@ghost`
`@rustbot` modify labels: rollup
let _24: ();
let mut _25: std::task::Poll<()>;
let mut _26: std::pin::Pin<&mut {async fn body@$DIR/async_await.rs:12:14: 12:16}>;
let mut _27: &mut {async fn body@$DIR/async_await.rs:12:14: 12:16};
let mut _28: &mut {async fn body@$DIR/async_await.rs:12:14: 12:16};
Copy link
Member

Choose a reason for hiding this comment

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

🎉 for anything that gets line numbers out of mir-opt tests :D

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 25, 2024
…kingjubilee

Rollup of 10 pull requests

Successful merges:

 - rust-lang#122707 (Fix a typo in the alloc::string::String docs)
 - rust-lang#122769 (extend comments for reachability set computation)
 - rust-lang#122892 (fix(bootstrap/dist): use versioned dirs when vendoring)
 - rust-lang#122896 (Update stdarch submodule)
 - rust-lang#122923 (In `pretty_print_type()`, print `async fn` futures' paths instead of spans.)
 - rust-lang#122950 (Add regression tests for rust-lang#101903)
 - rust-lang#123039 (Update books)
 - rust-lang#123042 (Import the 2021 prelude in the core crate)
 - rust-lang#123044 (`Instance` is `Copy`)
 - rust-lang#123051 (did I mention that tests are super cool? )

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 26, 2024
…kingjubilee

Rollup of 10 pull requests

Successful merges:

 - rust-lang#122707 (Fix a typo in the alloc::string::String docs)
 - rust-lang#122769 (extend comments for reachability set computation)
 - rust-lang#122892 (fix(bootstrap/dist): use versioned dirs when vendoring)
 - rust-lang#122896 (Update stdarch submodule)
 - rust-lang#122923 (In `pretty_print_type()`, print `async fn` futures' paths instead of spans.)
 - rust-lang#122950 (Add regression tests for rust-lang#101903)
 - rust-lang#123039 (Update books)
 - rust-lang#123042 (Import the 2021 prelude in the core crate)
 - rust-lang#123044 (`Instance` is `Copy`)
 - rust-lang#123051 (did I mention that tests are super cool? )

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9775296 into rust-lang:master Mar 26, 2024
11 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Mar 26, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 26, 2024
Rollup merge of rust-lang#122923 - kpreid:print-async-def, r=compiler-errors

In `pretty_print_type()`, print `async fn` futures' paths instead of spans.

This makes `-Zprint-type-sizes`'s output easier to read, because the name of an `async fn` is more immediately recognizable than its span. This change will also synergize with my other `-Zprint-type-sizes` PR rust-lang#122922 which prints the type of child futures being awaited.

I also deleted the comment "FIXME(eddyb) should use `def_span`." because it appears to have already been fixed by commit 67727aa.
@kpreid kpreid deleted the print-async-def branch March 26, 2024 18:36
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. 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.