expand: Leave traces when expanding `cfg_attr` attributes by petrochenkov · Pull Request #138515 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation35 Commits1 Checks6 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Currently cfg_trace just disappears during expansion, but after this PR #[cfg_attr(some tokens)] will leave a #[cfg_attr_trace(some tokens)] attribute instead of itself in AST after expansion (the new attribute is built-in and inert, its inner tokens are the same as in the original attribute).
This trace attribute can then be used by lints or other diagnostics, #133823 has some examples.

Tokens in these trace attributes are set to an empty token stream, so the traces are non-existent for proc macros and cannot affect any user-observable behavior.
This is also a weakness, because if a proc macro processes some code with the trace attributes, they will be lost, so the traces are best effort rather than precise.

The next step is to do the same thing with cfg attributes (#[cfg(TRUE)] currently remains in both AST and tokens after expanding, it should be replaced with a trace instead).

The idea belongs to @estebank.

r? @nnethercote

rustbot has assigned @nnethercote.
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 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 14, 2025

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

This comment has been minimized.

bors added a commit to rust-lang-ci/rust that referenced this pull request

Mar 14, 2025

expand: Leave traces when expanding cfg_attr attributes

Currently cfg_trace just disappears during expansion, but after this PR #[cfg_attr(some tokens)] will leave a #[cfg_attr_trace(some tokens)] attribute instead of itself in AST after expansion (the new attribute is built-in and inert, its inner tokens are the same as in the original attribute). This trace attribute can then be used by lints or other diagnostics, rust-lang#133823 has some examples.

Tokens in these trace attributes are set to an empty token stream, so the traces are non-existent for proc macros and cannot affect any user-observable behavior. This is also a weakness, because if a proc macro processes some code with the trace attributes, they will be lost, so the traces are best effort rather than precise.

The idea belongs to @estebank.

☀️ Try build successful - checks-actions
Build commit: c78b933 (c78b933a4e7ef33fae2a0fcf7f38b7bc7ecf71cc)

This comment has been minimized.

TODO: ignore the trace attributes during AST pretty-printing, we shouldn't break people parsing pretty-printed code, if possible.

Finished benchmarking commit (c78b933): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean	range	count
Regressions ❌ (primary)	0.5%	[0.2%, 1.8%]	26
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	1
Improvements ✅ (primary)	-1.7%	[-1.7%, -1.7%]	1
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.3%]	1
All ❌✅ (primary)	0.4%	[-1.7%, 1.8%]	27

Max RSS (memory usage)

Results (primary 1.1%, secondary 3.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean	range	count
Regressions ❌ (primary)	1.1%	[1.1%, 1.1%]	1
Regressions ❌ (secondary)	3.2%	[2.5%, 3.8%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.1%	[1.1%, 1.1%]	1

Cycles

Results (primary -1.7%, secondary 3.4%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.4%	[3.2%, 3.7%]	3
Improvements ✅ (primary)	-1.7%	[-1.7%, -1.7%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.7%	[-1.7%, -1.7%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 773.132s -> 772.081s (-0.14%)
Artifact size: 365.00 MiB -> 365.01 MiB (0.00%)

The slight regressions are expected, because we actually add more information to AST.
The regression in libc in doc mode is larger because libc has a lot of cfg_attrs and the propagate-doc-cfg pass performs a lot of inefficient operations with attributes.

rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

and removed S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Mar 15, 2025

Another idea - cfg_attr can be its own trace, if we put it into expanded_inert_attrs.
Let me try.
@rustbot author

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

Mar 16, 2025

Another idea - cfg_attr can be its own trace, if we put it into expanded_inert_attrs. Let me try. @rustbot author

Let's keep a separate attribute, with cfg_attr reused the pretty printing part is difficult, and you need to pass expanded_inert_attrs to cfg expander in various places.
@rustbot ready

rustbot removed the S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

label

Mar 16, 2025

bors added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

and removed S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

labels

Mar 18, 2025

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 18, 2025

What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing d8e44b7 (parent) -> 78948ac (this PR)

Test differences

Show 6 test diffs

[ui] tests/ui/proc-macro/cfg-attr-trace.rs (stage 2): [missing] -> pass (J0)
[ui] tests/ui/proc-macro/cfg-attr-trace.rs (stage 1): [missing] -> pass (J1)

Additionally, 4 doctest diffs were found. These are ignored, as they are noisy.

Job group index

J0: aarch64-apple, aarch64-gnu, arm-android, armhf-gnu, dist-i586-gnu-i586-i686-musl, i686-gnu-1, i686-gnu-nopt-1, i686-msvc-1, test-various, x86_64-apple-2, x86_64-gnu, x86_64-gnu-llvm-18-1, x86_64-gnu-llvm-18-2, x86_64-gnu-llvm-19-1, x86_64-gnu-llvm-19-2, x86_64-gnu-nopt, x86_64-gnu-stable, x86_64-mingw-1, x86_64-msvc-1
J1: x86_64-gnu-llvm-18-3, x86_64-gnu-llvm-19-3

Finished benchmarking commit (78948ac): comparison URL.

Overall result: ❌ regressions - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

If the regression was expected or you think it can be justified,
please write a comment with sufficient written justification, and add
@rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
If you think that you know of a way to resolve the regression, try to create
a new PR with a fix for the regression.
If you do not understand the regression or you think that it is just noise,
you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean	range	count
Regressions ❌ (primary)	0.4%	[0.1%, 1.8%]	29
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[0.1%, 1.8%]	29

Max RSS (memory usage)

Results (primary -6.9%, secondary -0.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.4%	[2.4%, 2.4%]	1
Improvements ✅ (primary)	-6.9%	[-6.9%, -6.9%]	1
Improvements ✅ (secondary)	-2.9%	[-2.9%, -2.9%]	1
All ❌✅ (primary)	-6.9%	[-6.9%, -6.9%]	1

Cycles

Results (secondary -1.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.8%	[-2.2%, -1.4%]	3
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 775.297s -> 774.951s (-0.04%)
Artifact size: 365.52 MiB -> 365.52 MiB (0.00%)

After this change, we see an unused_attributes lint on this line: https://github.com/aya-rs/aya/blob/9eecbe9d0e9dc1fdbbc87d41512d4202e26d4687/aya/src/maps/mod.rs#L493

It isn't clear to me if this is a true or false positive, since it seems to render correctly here: https://docs.aya-rs.dev/aya/maps/perf/struct.asyncperfeventarray.

  error: unused attribute `<cfg_attr>`
     --> aya/src/maps/mod.rs:493:1
      |
  493 | #[cfg_attr(docsrs, doc(cfg(any(feature = "async_tokio", feature = "async_std"))))]
      | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      |
  note: the built-in attribute `<cfg_attr>` will be ignored, since it's applied to the macro invocation `impl_try_from_map`
     --> aya/src/maps/mod.rs:494:1
      |
  494 | impl_try_from_map!(() {
      | ^^^^^^^^^^^^^^^^^
      = note: `-D unused-attributes` implied by `-D warnings`
      = help: to override `-D warnings` add `#[allow(unused_attributes)]`

@tamird I'll fix it today or tomorrow.

bors added a commit to rust-lang-ci/rust that referenced this pull request

Mar 22, 2025

expand: Leave traces when expanding cfg attributes

This is the same as rust-lang#138515, but for cfg(true) instead of cfg_attr.

The difference is that cfg(true)s already left "traces" after themselves - the cfg attributes themselves, with expanded_inert_attrs set to true, with full tokens, available to proc macros. This is not a reasonably expected behavior, but it could not be removed without a replacement, because a major rustdoc feature and a number of clippy lints rely on it. This PR implements a replacement.

This needs a crater run, because it changes observable behavior (in an intended way) - proc macros can no longer see expanded cfg(true) attributes.

(Some minor unnecessary special casing for sym::cfg_attr is also removed in this PR.)

r? @nnethercote

jhpratt added a commit to jhpratt/rust that referenced this pull request

Mar 27, 2025

expand: Leave traces when expanding cfg attributes

This is the same as rust-lang#138515, but for cfg(true) instead of cfg_attr.

This needs a crater run, because it changes observable behavior (in an intended way) - proc macros can no longer see expanded cfg(true) attributes.

(Some minor unnecessary special casing for sym::cfg_attr is also removed in this PR.)

r? @nnethercote

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Mar 27, 2025

Rollup merge of rust-lang#138844 - petrochenkov:cfgtrace2, r=nnethercote

expand: Leave traces when expanding cfg attributes

This is the same as rust-lang#138515, but for cfg(true) instead of cfg_attr.

This needs a crater run, because it changes observable behavior (in an intended way) - proc macros can no longer see expanded cfg(true) attributes.

(Some minor unnecessary special casing for sym::cfg_attr is also removed in this PR.)

r? @nnethercote

flip1995 pushed a commit to flip1995/rust that referenced this pull request

Apr 3, 2025

expand: Leave traces when expanding cfg_attr attributes

Currently cfg_trace just disappears during expansion, but after this PR #[cfg_attr(some tokens)] will leave a #[cfg_attr_trace(some tokens)] attribute instead of itself in AST after expansion (the new attribute is built-in and inert, its inner tokens are the same as in the original attribute). This trace attribute can then be used by lints or other diagnostics, rust-lang#133823 has some examples.

Tokens in these trace attributes are set to an empty token stream, so the traces are non-existent for proc macros and cannot affect any user-observable behavior. This is also a weakness, because if a proc macro processes some code with the trace attributes, they will be lost, so the traces are best effort rather than precise.

The next step is to do the same thing with cfg attributes (#[cfg(TRUE)] currently remains in both AST and tokens after expanding, it should be replaced with a trace instead).

The idea belongs to @estebank.

estebank added a commit to estebank/rust that referenced this pull request

May 22, 2025

Previously, when evaluating a #[cfg_attr(..)] to false, the entire attribute was removed from the AST. Afterwards in rust-lang#138515, we insert in its place a placeholder attribute so that checks for attributes can still know about their placement. This is particularly relevant when we suggest removing items with cfg_attrs (fix rust-lang#56328). We use the placeholder as it is an ident that can't be written by the end user to begin with. We tweak the wording of the existing "unused extern crate" lint.

warning: unused `extern crate`
  --> $DIR/removing-extern-crate.rs:9:1
   |
LL | extern crate removing_extern_crate as foo;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unused
   |
note: the lint level is defined here
  --> $DIR/removing-extern-crate.rs:6:9
   |
LL | #![warn(rust_2018_idioms)]
   |         ^^^^^^^^^^^^^^^^
   = note: `#[warn(unused_extern_crates)]` implied by `#[warn(rust_2018_idioms)]`
help: remove the unused `extern crate`
   |
LL - #[cfg_attr(test, macro_use)]
LL - extern crate removing_extern_crate as foo;
LL +
   |

estebank added a commit to estebank/rust that referenced this pull request

May 28, 2025

warning: unused `extern crate`
  --> $DIR/removing-extern-crate.rs:9:1
   |
LL | extern crate removing_extern_crate as foo;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unused
   |
note: the lint level is defined here
  --> $DIR/removing-extern-crate.rs:6:9
   |
LL | #![warn(rust_2018_idioms)]
   |         ^^^^^^^^^^^^^^^^
   = note: `#[warn(unused_extern_crates)]` implied by `#[warn(rust_2018_idioms)]`
help: remove the unused `extern crate`
   |
LL - #[cfg_attr(test, macro_use)]
LL - extern crate removing_extern_crate as foo;
LL +
   |

tgross35 added a commit to tgross35/rust that referenced this pull request

May 28, 2025

Use cfg_attr_trace in AST with a placeholder attribute for accurate suggestion

In rust-lang#138515, we insert a placeholder attribute so that checks for attributes can still know about the placement of cfg attributes. When we suggest removing items with cfg_attrs (fix rust-lang#56328) and make them verbose. We tweak the wording of the existing "unused extern crate" lint.

warning: unused `extern crate`
  --> $DIR/removing-extern-crate.rs:9:1
   |
LL | extern crate removing_extern_crate as foo;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unused
   |
note: the lint level is defined here
  --> $DIR/removing-extern-crate.rs:6:9
   |
LL | #![warn(rust_2018_idioms)]
   |         ^^^^^^^^^^^^^^^^
   = note: `#[warn(unused_extern_crates)]` implied by `#[warn(rust_2018_idioms)]`
help: remove the unused `extern crate`
   |
LL - #[cfg_attr(test, macro_use)]
LL - extern crate removing_extern_crate as foo;
   |

r? @petrochenkov

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

May 30, 2025

Use cfg_attr_trace in AST with a placeholder attribute for accurate suggestion

warning: unused `extern crate`
  --> $DIR/removing-extern-crate.rs:9:1
   |
LL | extern crate removing_extern_crate as foo;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unused
   |
note: the lint level is defined here
  --> $DIR/removing-extern-crate.rs:6:9
   |
LL | #![warn(rust_2018_idioms)]
   |         ^^^^^^^^^^^^^^^^
   = note: `#[warn(unused_extern_crates)]` implied by `#[warn(rust_2018_idioms)]`
help: remove the unused `extern crate`
   |
LL - #[cfg_attr(test, macro_use)]
LL - extern crate removing_extern_crate as foo;
   |

r? @petrochenkov

try-job: x86_64-gnu-aux

rust-timer added a commit that referenced this pull request

May 30, 2025

Rollup merge of #133823 - estebank:issue-56328, r=petrochenkov

Use cfg_attr_trace in AST with a placeholder attribute for accurate suggestion

In #138515, we insert a placeholder attribute so that checks for attributes can still know about the placement of cfg attributes. When we suggest removing items with cfg_attrs (fix #56328) and make them verbose. We tweak the wording of the existing "unused extern crate" lint.

warning: unused `extern crate`
  --> $DIR/removing-extern-crate.rs:9:1
   |
LL | extern crate removing_extern_crate as foo;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unused
   |
note: the lint level is defined here
  --> $DIR/removing-extern-crate.rs:6:9
   |
LL | #![warn(rust_2018_idioms)]
   |         ^^^^^^^^^^^^^^^^
   = note: `#[warn(unused_extern_crates)]` implied by `#[warn(rust_2018_idioms)]`
help: remove the unused `extern crate`
   |
LL - #[cfg_attr(test, macro_use)]
LL - extern crate removing_extern_crate as foo;
   |

r? @petrochenkov

try-job: x86_64-gnu-aux

github-actions bot pushed a commit to rust-lang/miri that referenced this pull request

May 31, 2025

Use cfg_attr_trace in AST with a placeholder attribute for accurate suggestion

In rust-lang/rust#138515, we insert a placeholder attribute so that checks for attributes can still know about the placement of cfg attributes. When we suggest removing items with cfg_attrs (fix rust-lang/rust#56328) and make them verbose. We tweak the wording of the existing "unused extern crate" lint.

warning: unused `extern crate`
  --> $DIR/removing-extern-crate.rs:9:1
   |
LL | extern crate removing_extern_crate as foo;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unused
   |
note: the lint level is defined here
  --> $DIR/removing-extern-crate.rs:6:9
   |
LL | #![warn(rust_2018_idioms)]
   |         ^^^^^^^^^^^^^^^^
   = note: `#[warn(unused_extern_crates)]` implied by `#[warn(rust_2018_idioms)]`
help: remove the unused `extern crate`
   |
LL - #[cfg_attr(test, macro_use)]
LL - extern crate removing_extern_crate as foo;
   |

r? @petrochenkov

try-job: x86_64-gnu-aux

expand: Leave traces when expanding cfg_attr attributes by petrochenkov · Pull Request #138515 · rust-lang/rust (original) (raw)

Conversation

This comment has been minimized.

This comment has been minimized.

Overall result: ❌✅ regressions and improvements - please read the text below

Test differences

Overall result: ❌ regressions - please read the text below

expand: Leave traces when expanding `cfg_attr` attributes by petrochenkov · Pull Request #138515 · rust-lang/rust (original) (raw)