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

Revert #138019 after further discussion about how hir-pretty printing should work #138060

Merged
merged 1 commit into from
Mar 7, 2025

Conversation

jdonszelmann
Copy link
Contributor

After some more discussion, #138019 was probably merged a little fast. Though there probably is a real bug in pretty printing, it is not feasible to add similar pretty printing routines for all attributes, and making this specific exception is likely not desired either. For more context, see post-merge comments on #138019

I kept the tests around, but reverted the hir-pretty change.

r? @compiler-errors

@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 5, 2025
@jdonszelmann jdonszelmann changed the title Revert #138019 after further discussion about how hir-pretty printing should work and whether an exception should be added here Revert #138019 after further discussion about how hir-pretty printing should work Mar 5, 2025
@compiler-errors
Copy link
Member

👍
@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 5, 2025

📌 Commit 4200f8e 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 5, 2025
@jdonszelmann
Copy link
Contributor Author

jdonszelmann commented Mar 5, 2025

oops forgot to bless a test, after adding a fixme comment

@jdonszelmann
Copy link
Contributor Author

bors doesn't seem to list this PR as approved/in the queue. I thought it might have been fallout from some of the other problems atm but I think it's just the change I pushed after your r+ possibly

@jdonszelmann
Copy link
Contributor Author

@compiler-errors could you r+ again? The test stderr wasn't properly blessed. Only thing that changed is a comment that the output of deprecated should be fixed in the future, which was in the test but not the corresponding stderr. Your pr addresses that comment anyway.

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 7, 2025

📌 Commit 8391c08 has been approved by compiler-errors

It is now in the queue for this repository.

jhpratt added a commit to jhpratt/rust that referenced this pull request Mar 7, 2025
…iler-errors

Revert rust-lang#138019 after further discussion about how hir-pretty printing should work

After some more discussion, rust-lang#138019 was probably merged a little fast. Though there probably is a real bug in pretty printing, it is not feasible to add similar pretty printing routines for all attributes, and making this specific exception is likely not desired either. For more context, see post-merge comments on rust-lang#138019

I kept the tests around, but reverted the hir-pretty change.

r? `@compiler-errors`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 7, 2025
…iler-errors

Revert rust-lang#138019 after further discussion about how hir-pretty printing should work

After some more discussion, rust-lang#138019 was probably merged a little fast. Though there probably is a real bug in pretty printing, it is not feasible to add similar pretty printing routines for all attributes, and making this specific exception is likely not desired either. For more context, see post-merge comments on rust-lang#138019

I kept the tests around, but reverted the hir-pretty change.

r? ``@compiler-errors``
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 7, 2025
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#137674 (Enable `f16` for LoongArch)
 - rust-lang#138034 (library: Use `size_of` from the prelude instead of imported)
 - rust-lang#138060 (Revert rust-lang#138019 after further discussion about how hir-pretty printing should work)
 - rust-lang#138073 (Break critical edges in inline asm before code generation)
 - rust-lang#138107 (`librustdoc`: clippy fixes)
 - rust-lang#138111 (Use `default_field_values` for `rustc_errors::Context`, `rustc_session::config::NextSolverConfig` and `rustc_session::config::ErrorOutputType`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f42c933 into rust-lang:master Mar 7, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Mar 7, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 7, 2025
Rollup merge of rust-lang#138060 - jdonszelmann:revert-138019, r=compiler-errors

Revert rust-lang#138019 after further discussion about how hir-pretty printing should work

After some more discussion, rust-lang#138019 was probably merged a little fast. Though there probably is a real bug in pretty printing, it is not feasible to add similar pretty printing routines for all attributes, and making this specific exception is likely not desired either. For more context, see post-merge comments on rust-lang#138019

I kept the tests around, but reverted the hir-pretty change.

r? ```@compiler-errors```
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Mar 7, 2025
…tty, r=jdonszelmann

Improve `-Zunpretty=hir` for parsed attrs

0. Rename `print_something` to `should_render` to make it distinct from `print_attribute` in that it doesn't print anything, it's just a way to probe if a type renders anything.
1. Fixes a few bugs in the `PrintAttribute` derive. Namely, the `__printed_anything` variable was entangled with the `should_render` call, leading us to always render field names but never render commas.
2. Remove the outermost `""` from the attr.
3. Debug print `Symbol`s. I know that this is redundant for some parsed attributes, but there's no good way to distinguish symbols that are ident-like and symbols which are cooked string literals. We could perhaps *conditionally* to fall back to a debug printing if the symbol doesn't match an ident? But seems like overkill.

Based on rust-lang#138060, only review the commits not in that one.
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Mar 7, 2025
…tty, r=jdonszelmann

Improve `-Zunpretty=hir` for parsed attrs

0. Rename `print_something` to `should_render` to make it distinct from `print_attribute` in that it doesn't print anything, it's just a way to probe if a type renders anything.
1. Fixes a few bugs in the `PrintAttribute` derive. Namely, the `__printed_anything` variable was entangled with the `should_render` call, leading us to always render field names but never render commas.
2. Remove the outermost `""` from the attr.
3. Debug print `Symbol`s. I know that this is redundant for some parsed attributes, but there's no good way to distinguish symbols that are ident-like and symbols which are cooked string literals. We could perhaps *conditionally* to fall back to a debug printing if the symbol doesn't match an ident? But seems like overkill.

Based on rust-lang#138060, only review the commits not in that one.
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Mar 10, 2025
…tty, r=jdonszelmann

Improve `-Zunpretty=hir` for parsed attrs

0. Rename `print_something` to `should_render` to make it distinct from `print_attribute` in that it doesn't print anything, it's just a way to probe if a type renders anything.
1. Fixes a few bugs in the `PrintAttribute` derive. Namely, the `__printed_anything` variable was entangled with the `should_render` call, leading us to always render field names but never render commas.
2. Remove the outermost `""` from the attr.
3. Debug print `Symbol`s. I know that this is redundant for some parsed attributes, but there's no good way to distinguish symbols that are ident-like and symbols which are cooked string literals. We could perhaps *conditionally* to fall back to a debug printing if the symbol doesn't match an ident? But seems like overkill.

Based on rust-lang#138060, only review the commits not in that one.
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Mar 10, 2025
…tty, r=jdonszelmann

Improve `-Zunpretty=hir` for parsed attrs

0. Rename `print_something` to `should_render` to make it distinct from `print_attribute` in that it doesn't print anything, it's just a way to probe if a type renders anything.
1. Fixes a few bugs in the `PrintAttribute` derive. Namely, the `__printed_anything` variable was entangled with the `should_render` call, leading us to always render field names but never render commas.
2. Remove the outermost `""` from the attr.
3. Debug print `Symbol`s. I know that this is redundant for some parsed attributes, but there's no good way to distinguish symbols that are ident-like and symbols which are cooked string literals. We could perhaps *conditionally* to fall back to a debug printing if the symbol doesn't match an ident? But seems like overkill.

Based on rust-lang#138060, only review the commits not in that one.
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Mar 11, 2025
…tty, r=jdonszelmann

Improve `-Zunpretty=hir` for parsed attrs

0. Rename `print_something` to `should_render` to make it distinct from `print_attribute` in that it doesn't print anything, it's just a way to probe if a type renders anything.
1. Fixes a few bugs in the `PrintAttribute` derive. Namely, the `__printed_anything` variable was entangled with the `should_render` call, leading us to always render field names but never render commas.
2. Remove the outermost `""` from the attr.
3. Debug print `Symbol`s. I know that this is redundant for some parsed attributes, but there's no good way to distinguish symbols that are ident-like and symbols which are cooked string literals. We could perhaps *conditionally* to fall back to a debug printing if the symbol doesn't match an ident? But seems like overkill.

Based on rust-lang#138060, only review the commits not in that one.
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Mar 11, 2025
…tty, r=jdonszelmann

Improve `-Zunpretty=hir` for parsed attrs

0. Rename `print_something` to `should_render` to make it distinct from `print_attribute` in that it doesn't print anything, it's just a way to probe if a type renders anything.
1. Fixes a few bugs in the `PrintAttribute` derive. Namely, the `__printed_anything` variable was entangled with the `should_render` call, leading us to always render field names but never render commas.
2. Remove the outermost `""` from the attr.
3. Debug print `Symbol`s. I know that this is redundant for some parsed attributes, but there's no good way to distinguish symbols that are ident-like and symbols which are cooked string literals. We could perhaps *conditionally* to fall back to a debug printing if the symbol doesn't match an ident? But seems like overkill.

Based on rust-lang#138060, only review the commits not in that one.
Kobzol added a commit to Kobzol/rust that referenced this pull request Mar 11, 2025
…tty, r=jdonszelmann

Improve `-Zunpretty=hir` for parsed attrs

0. Rename `print_something` to `should_render` to make it distinct from `print_attribute` in that it doesn't print anything, it's just a way to probe if a type renders anything.
1. Fixes a few bugs in the `PrintAttribute` derive. Namely, the `__printed_anything` variable was entangled with the `should_render` call, leading us to always render field names but never render commas.
2. Remove the outermost `""` from the attr.
3. Debug print `Symbol`s. I know that this is redundant for some parsed attributes, but there's no good way to distinguish symbols that are ident-like and symbols which are cooked string literals. We could perhaps *conditionally* to fall back to a debug printing if the symbol doesn't match an ident? But seems like overkill.

Based on rust-lang#138060, only review the commits not in that one.
Kobzol added a commit to Kobzol/rust that referenced this pull request Mar 11, 2025
…tty, r=jdonszelmann

Improve `-Zunpretty=hir` for parsed attrs

0. Rename `print_something` to `should_render` to make it distinct from `print_attribute` in that it doesn't print anything, it's just a way to probe if a type renders anything.
1. Fixes a few bugs in the `PrintAttribute` derive. Namely, the `__printed_anything` variable was entangled with the `should_render` call, leading us to always render field names but never render commas.
2. Remove the outermost `""` from the attr.
3. Debug print `Symbol`s. I know that this is redundant for some parsed attributes, but there's no good way to distinguish symbols that are ident-like and symbols which are cooked string literals. We could perhaps *conditionally* to fall back to a debug printing if the symbol doesn't match an ident? But seems like overkill.

Based on rust-lang#138060, only review the commits not in that one.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 11, 2025
Rollup merge of rust-lang#138063 - compiler-errors:improve-attr-unpretty, r=jdonszelmann

Improve `-Zunpretty=hir` for parsed attrs

0. Rename `print_something` to `should_render` to make it distinct from `print_attribute` in that it doesn't print anything, it's just a way to probe if a type renders anything.
1. Fixes a few bugs in the `PrintAttribute` derive. Namely, the `__printed_anything` variable was entangled with the `should_render` call, leading us to always render field names but never render commas.
2. Remove the outermost `""` from the attr.
3. Debug print `Symbol`s. I know that this is redundant for some parsed attributes, but there's no good way to distinguish symbols that are ident-like and symbols which are cooked string literals. We could perhaps *conditionally* to fall back to a debug printing if the symbol doesn't match an ident? But seems like overkill.

Based on rust-lang#138060, only review the commits not in that one.
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request Mar 14, 2025
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#137674 (Enable `f16` for LoongArch)
 - rust-lang#138034 (library: Use `size_of` from the prelude instead of imported)
 - rust-lang#138060 (Revert rust-lang#138019 after further discussion about how hir-pretty printing should work)
 - rust-lang#138073 (Break critical edges in inline asm before code generation)
 - rust-lang#138107 (`librustdoc`: clippy fixes)
 - rust-lang#138111 (Use `default_field_values` for `rustc_errors::Context`, `rustc_session::config::NextSolverConfig` and `rustc_session::config::ErrorOutputType`)

r? `@ghost`
`@rustbot` modify labels: rollup
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.

None yet

4 participants