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

Improve -Zunpretty=hir for parsed attrs #138063

Merged
merged 2 commits into from
Mar 11, 2025

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Mar 5, 2025

  1. 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.
  2. 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.
  3. Remove the outermost "" from the attr.
  4. Debug print Symbols. 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 #138060, only review the commits not in that one.

@rustbot
Copy link
Collaborator

rustbot commented Mar 5, 2025

r? @lcnr

rustbot has assigned @lcnr.
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 A-attributes Area: Attributes (`#[…]`, `#![…]`) 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
@rustbot
Copy link
Collaborator

rustbot commented Mar 5, 2025

Some changes occurred in compiler/rustc_attr_data_structures

cc @jdonszelmann

@compiler-errors
Copy link
Member Author

@bors delegate=jdonszelmann

@bors
Copy link
Contributor

bors commented Mar 5, 2025

✌️ @jdonszelmann, you can now approve this pull request!

If @compiler-errors told you to "r=me" after making some further change, please make that change, then do @bors r=@compiler-errors

@@ -5,17 +5,21 @@ extern crate std;
//@ compile-flags: -Zunpretty=hir
//@ check-pass

#[deprecated]
#[attr = Deprecation {deprecation: Deprecation {since: Unspecified}}]
Copy link
Member Author

@compiler-errors compiler-errors Mar 5, 2025

Choose a reason for hiding this comment

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

some of the spacing is a bit tighter than i'd like, but the pp_hir machinery is annoying lol

hir::Attribute::Parsed(pa) => {
self.word("#[attr=\"");
self.word("#[attr = ");
Copy link
Member Author

Choose a reason for hiding this comment

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

this isn't grammatical (#[attr = {}]) but renders better than a string :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I do prefer it though, so nice change :)

@compiler-errors compiler-errors added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 5, 2025
if parens {
p.word("(");
}

let mut printed_anything = $t.print_something();
let mut printed_anything = $t.should_render();
Copy link
Contributor

Choose a reason for hiding this comment

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

is printed_anything doing anything here? I may have misremembered my own code but can't find a usage atm


$t.print_attribute(p);

$(
if printed_anything && $ts.print_something() {
if $ts.should_render() {
Copy link
Contributor

Choose a reason for hiding this comment

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

the check isn't here anymore, which could be correct

Copy link
Member Author

Choose a reason for hiding this comment

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

oh yes, that is necessary, but we need to push the if printed_anything into the block like the other fixes.

@@ -126,7 +126,7 @@ macro_rules! print_tup {
let ($t, $($ts),*) = self;
let parens = print_tup!(num_should_render $t $($ts)*) > 1;
if parens {
p.word("(");
p.popen();
Copy link
Contributor

Choose a reason for hiding this comment

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

oh didn't know these existed, neat :3

hir::Attribute::Parsed(pa) => {
self.word("#[attr=\"");
self.word("#[attr = ");
Copy link
Contributor

Choose a reason for hiding this comment

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

I do prefer it though, so nice change :)

@@ -35,6 +37,7 @@ fn print_fields(name: &Ident, fields: &Fields) -> (TokenStream, TokenStream, Tok
return;
}

__p.nbsp();
__p.word("{");
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we could add some spaces (when #disps.len() > 0) here at the braces to make things slightly less tight?

Copy link
Member Author

Choose a reason for hiding this comment

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

It may lead to other spacing awkwardness, so let's keep it as is.

@bors
Copy link
Contributor

bors commented Mar 7, 2025

☔ The latest upstream changes (presumably #138155) made this pull request unmergeable. Please resolve the merge conflicts.

@compiler-errors compiler-errors force-pushed the improve-attr-unpretty branch from 914ad9e to 9ae9453 Compare March 7, 2025 18:42
@jdonszelmann
Copy link
Contributor

I see ya did the spaces change anyway, nice! Looks like that addresses everything @bors r+

@bors
Copy link
Contributor

bors commented Mar 7, 2025

📌 Commit 9ae9453 has been approved by jdonszelmann

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-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Mar 7, 2025
@compiler-errors
Copy link
Member Author

@bors rollup since this does not really affect stable code

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.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 7, 2025
…mpiler-errors

Rollup of 10 pull requests

Successful merges:

 - rust-lang#135651 (Support for `wasm32-wali-linux-musl` Tier-3 target)
 - rust-lang#136642 (Put the alloc unit tests in a separate alloctests package)
 - rust-lang#137337 (Add verbatim linker to AIXLinker)
 - rust-lang#137549 (Clean up various LLVM FFI things in codegen_llvm)
 - rust-lang#137957 (Remove i586-pc-windows-msvc)
 - rust-lang#138063 (Improve `-Zunpretty=hir` for parsed attrs)
 - rust-lang#138137 (setTargetTriple now accepts Triple rather than string)
 - rust-lang#138141 (tests: fix some typos in comment)
 - rust-lang#138150 (Streamline HIR intravisit `visit_id` calls for items)
 - rust-lang#138173 (Delay bug for negative auto trait rather than ICEing)

r? `@ghost`
`@rustbot` modify labels: rollup
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.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 7, 2025
…mpiler-errors

Rollup of 8 pull requests

Successful merges:

 - rust-lang#136642 (Put the alloc unit tests in a separate alloctests package)
 - rust-lang#137337 (Add verbatim linker to AIXLinker)
 - rust-lang#137363 (compiler: factor Windows x86-32 ABI impl into its own file)
 - rust-lang#137685 (self-contained linker: conservatively default to `-znostart-stop-gc` on x64 linux)
 - rust-lang#138000 (atomic: clarify that failing conditional RMW operations are not 'writes')
 - rust-lang#138063 (Improve `-Zunpretty=hir` for parsed attrs)
 - rust-lang#138137 (setTargetTriple now accepts Triple rather than string)
 - rust-lang#138173 (Delay bug for negative auto trait rather than ICEing)

r? `@ghost`
`@rustbot` modify labels: rollup
@aDotInTheVoid aDotInTheVoid removed A-rustdoc-search Area: Rustdoc's search feature T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. labels Mar 9, 2025
@rustbot rustbot added A-rustdoc-search Area: Rustdoc's search feature T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. labels Mar 10, 2025
@jdonszelmann
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 10, 2025

📌 Commit 279377f has been approved by jdonszelmann

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 10, 2025
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.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 11, 2025
Rollup of 16 pull requests

Successful merges:

 - rust-lang#126856 (remove deprecated tool `rls`)
 - rust-lang#136932 (Reduce formatting `width` and `precision` to 16 bits)
 - rust-lang#137314 (change definitely unproductive cycles to error)
 - rust-lang#137612 (Update bootstrap to edition 2024)
 - rust-lang#138002 (Disable CFI for weakly linked syscalls)
 - rust-lang#138052 (strip `-Wlinker-messages` wrappers from `rust-lld` rmake test)
 - rust-lang#138063 (Improve `-Zunpretty=hir` for parsed attrs)
 - rust-lang#138109 (make precise capturing args in rustdoc Json typed)
 - rust-lang#138147 (Add maintainers for powerpc64le-unknown-linux-gnu)
 - rust-lang#138245 (stabilize `ci_rustc_if_unchanged_logic` test for local environments)
 - rust-lang#138296 (Remove `AdtFlags::IS_ANONYMOUS` and `Copy`/`Clone` condition for anonymous ADT)
 - rust-lang#138300 (add tracking issue for unqualified_local_imports)
 - rust-lang#138307 (Allow specifying glob patterns for try jobs)
 - rust-lang#138313 (Update books)
 - rust-lang#138315 (use next_back() instead of last() on DoubleEndedIterator)
 - rust-lang#138318 (Rustdoc: remove a bunch of `@ts-expect-error` from main.js)

r? `@ghost`
`@rustbot` modify labels: rollup
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.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 11, 2025
Rollup of 18 pull requests

Successful merges:

 - rust-lang#126856 (remove deprecated tool `rls`)
 - rust-lang#137314 (change definitely unproductive cycles to error)
 - rust-lang#137504 (Move methods from Map to TyCtxt, part 4.)
 - rust-lang#137701 (Convert `ShardedHashMap` to use `hashbrown::HashTable`)
 - rust-lang#137967 ([AIX] Fix hangs during testing)
 - rust-lang#138002 (Disable CFI for weakly linked syscalls)
 - rust-lang#138052 (strip `-Wlinker-messages` wrappers from `rust-lld` rmake test)
 - rust-lang#138063 (Improve `-Zunpretty=hir` for parsed attrs)
 - rust-lang#138109 (make precise capturing args in rustdoc Json typed)
 - rust-lang#138147 (Add maintainers for powerpc64le-unknown-linux-gnu)
 - rust-lang#138245 (stabilize `ci_rustc_if_unchanged_logic` test for local environments)
 - rust-lang#138296 (Remove `AdtFlags::IS_ANONYMOUS` and `Copy`/`Clone` condition for anonymous ADT)
 - rust-lang#138300 (add tracking issue for unqualified_local_imports)
 - rust-lang#138307 (Allow specifying glob patterns for try jobs)
 - rust-lang#138313 (Update books)
 - rust-lang#138315 (use next_back() instead of last() on DoubleEndedIterator)
 - rust-lang#138318 (Rustdoc: remove a bunch of `@ts-expect-error` from main.js)
 - rust-lang#138330 (Remove unnecessary `[lints.rust]` sections.)

Failed merges:

 - rust-lang#137147 (Add exclude to config.toml)

r? `@ghost`
`@rustbot` modify labels: rollup
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.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 11, 2025
Rollup of 11 pull requests

Successful merges:

 - rust-lang#135987 (Clarify iterator by_ref docs)
 - rust-lang#137967 ([AIX] Fix hangs during testing)
 - rust-lang#138063 (Improve `-Zunpretty=hir` for parsed attrs)
 - rust-lang#138147 (Add maintainers for powerpc64le-unknown-linux-gnu)
 - rust-lang#138288 (Document -Z crate-attr)
 - rust-lang#138300 (add tracking issue for unqualified_local_imports)
 - rust-lang#138307 (Allow specifying glob patterns for try jobs)
 - rust-lang#138315 (use next_back() instead of last() on DoubleEndedIterator)
 - rust-lang#138330 (Remove unnecessary `[lints.rust]` sections.)
 - rust-lang#138335 (Fix post-merge workflow)
 - rust-lang#138343 (Enable `f16` tests for `powf`)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 11, 2025
Rollup of 10 pull requests

Successful merges:

 - rust-lang#135987 (Clarify iterator by_ref docs)
 - rust-lang#137967 ([AIX] Fix hangs during testing)
 - rust-lang#138063 (Improve `-Zunpretty=hir` for parsed attrs)
 - rust-lang#138147 (Add maintainers for powerpc64le-unknown-linux-gnu)
 - rust-lang#138288 (Document -Z crate-attr)
 - rust-lang#138300 (add tracking issue for unqualified_local_imports)
 - rust-lang#138307 (Allow specifying glob patterns for try jobs)
 - rust-lang#138315 (use next_back() instead of last() on DoubleEndedIterator)
 - rust-lang#138330 (Remove unnecessary `[lints.rust]` sections.)
 - rust-lang#138335 (Fix post-merge workflow)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c054bac into rust-lang:master Mar 11, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Mar 11, 2025
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-rustdoc-json Area: Rustdoc JSON backend A-rustdoc-search Area: Rustdoc's search feature 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. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants