Use `cfg_attr_trace` in AST with a placeholder attribute for accurate suggestion #133823

estebank · 2024-12-03T21:19:56Z

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

estebank · 2024-12-03T21:22:14Z

tests/ui/rust-2018/remove-extern-crate.stderr

+help: remove the unused `extern crate`
+   |
+LL - extern crate core;
+LL +


I thought this was a rendering bug, but it is because of the //~ removed comment.

rustbot · 2024-12-03T23:29:16Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

petrochenkov · 2024-12-10T19:08:54Z

Are the placeholder attributes visible to proc macros?
Or they somehow produce empty token streams (from the proc macro point of view) and exist only in AST?
I'm also interested whether these placeholders mess up the token collection process, because removing false #[cfg(_attr)]s from the token stream representation is probably the most involved part of the cfg expansion.

Something like

#[cfg_eval]
#[my_proc_macro]
#[cfg_attr(all(), my_attr)]
struct S {}

can be used for testing, the cfg_eval macro will eagerly expand all cfg(_attr) attributes so macros like my_proc_macro can see only their results.

If it's possible to introduce placeholders like this (on best effort basis) only in AST, without making them observable in token streams in any way, then they should be able to solve some other problems as well, like removing expanded #[cfg(TRUE)]s from token streams without damaging rustdoc and clippy.

Upd: if these placeholders are visible to proc macros then we cannot add them, because they are internal details that should not be public and stable.

estebank · 2024-12-10T20:08:46Z

With a test

//@ proc-macro: cfg-placeholder.rs
#![feature(cfg_eval)]
#[macro_use] extern crate cfg_placeholder;

#[cfg_eval]
#[my_proc_macro]
#[cfg_attr(FALSE, my_attr1)]
#[cfg_attr(all(), my_attr2)]
struct S {}

and a my_proc_macro and my_attrs that just print what it was passed in, the output was

my_proc_macro:
TokenStream []
TokenStream [
    Punct {
        ch: '#',
        spacing: Alone,
        span: #0 bytes(160..161),
    },
    Group {
        delimiter: Bracket,
        stream: TokenStream [
            Ident {
                ident: "my_attr2",
                span: #0 bytes(178..186),
            },
        ],
        span: #0 bytes(161..188),
    },
    Ident {
        ident: "struct",
        span: #0 bytes(189..195),
    },
    Ident {
        ident: "S",
        span: #0 bytes(196..197),
    },
    Group {
        delimiter: Brace,
        stream: TokenStream [],
        span: #0 bytes(198..200),
    },
]

my_attr2:
TokenStream []
TokenStream [
    Ident {
        ident: "struct",
        span: #0 bytes(189..195),
    },
    Ident {
        ident: "S",
        span: #0 bytes(196..197),
    },
    Group {
        delimiter: Brace,
        stream: TokenStream [],
        span: #0 bytes(198..200),
    },
]

I'm honestly surprised it doesn't seem like we need to do anything else to avoid polluting the TTS. I think it is because of the check for is_cfg_placeholder in Attribute::token_trees, which was needed to be able to build the stdlib (IIRC) in the first place (likely precisely because of this exact problem you anticipated) so I addressed this issue reactively purely trying to get things to a working state to begin with.

petrochenkov · 2025-03-13T18:29:34Z

Sorry for the long delay.
I think the direction here is right, but I need to do some experiments with this.

rustbot · 2025-03-14T19:37:52Z

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

cc @jdonszelmann

estebank · 2025-03-14T19:39:30Z

No problem. Rebased (but kept the separated commits) to hopefully make it easier to review. The recent changes to support cfg attrs on where clauses required yet another tweak, but besides that everything is the same as it was back in December.

petrochenkov · 2025-03-14T19:47:14Z

I've reimplemented the trace attributes in #138515 in a slightly more principled way, but without including any diagnostic changes from this PR.
@rustbot blocked

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.`

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 · 2025-05-28T15:57:41Z

@bors r=petrochenkov

bors · 2025-05-28T15:57:43Z

📌 Commit ee4596b has been approved by petrochenkov

It is now in the queue for this repository.

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_attr`s (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`

Rollup of 7 pull requests Successful merges: - #125087 (Optimize `Seek::stream_len` impl for `File`) - #133823 (Use `cfg_attr_trace` in AST with a placeholder attribute for accurate suggestion) - #138285 (Stabilize `repr128`) - #139994 (add `CStr::display`) - #141477 (Path::with_extension: show that it adds an extension where one did no…) - #141533 (clean up old rintf leftovers) - #141693 (Subtree update of `rust-analyzer`) r? `@ghost` `@rustbot` modify labels: rollup

tgross35 · 2025-05-28T21:54:34Z

I think this was the failure at #141707 (comment), looks like this is triggering a new failure in Cargo.

@bors r-

estebank · 2025-05-29T10:09:06Z

@tgross35 is it enough to modify src/tools/cargo/tests/testsuite/lints_table.rs from this PR, or do I need to do the dance of changing the test upstream before being able to merge this PR? Edit: confirmed that src/tools/cargo is a submodule :-/
Edit 2: just removed the one change to the main message which is being checked against. We really need a better mechanism for these kind of changes as they've been happening more of late :(

PR 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_attr`s (fix Issue 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; LL + | ```

estebank · 2025-05-29T14:01:29Z

@bors r=petrochenkov

bors · 2025-05-29T14:01:31Z

📌 Commit f80e3ac has been approved by petrochenkov

It is now in the queue for this repository.

Rollup of 8 pull requests Successful merges: - #133823 (Use `cfg_attr_trace` in AST with a placeholder attribute for accurate suggestion) - #141004 (Report text_direction_codepoint_in_literal when parsing) - #141407 (Refactor the two-phase check for impls and impl items) - #141430 (remove `visit_clobber` and move `DummyAstNode` to `rustc_expand`) - #141507 (atomic_load intrinsic: use const generic parameter for ordering) - #141538 (implement `va_arg` for x86_64 systemv) - #141669 (float: Replace some approximate assertions with exact) - #141747 (rustdoc: display doc(cfg(false)) properly) r? `@ghost` `@rustbot` modify labels: rollup

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_attr`s (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

Kobzol · 2025-05-30T15:20:22Z

@rust-timer build dcca579

(Trying perf. due to #141753)

rust-timer · 2025-05-30T17:20:53Z

Finished benchmarking commit (dcca579): comparison URL.

Overall result: ❌ regressions - no action needed

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.

@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
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -1.2%, secondary 0.3%)

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)	1.4%	[0.6%, 3.4%]	4
Improvements ✅ (primary)	-1.2%	[-1.2%, -1.2%]	1
Improvements ✅ (secondary)	-0.7%	[-1.0%, -0.5%]	4
All ❌✅ (primary)	-1.2%	[-1.2%, -1.2%]	1

Cycles

Results (secondary 0.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)	1.0%	[0.6%, 1.6%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.6%	[-0.7%, -0.4%]	4
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: missing data
Artifact size: 370.26 MiB -> 370.26 MiB (-0.00%)

Rollup of 8 pull requests Successful merges: - rust-lang/rust#133823 (Use `cfg_attr_trace` in AST with a placeholder attribute for accurate suggestion) - rust-lang/rust#141004 (Report text_direction_codepoint_in_literal when parsing) - rust-lang/rust#141407 (Refactor the two-phase check for impls and impl items) - rust-lang/rust#141430 (remove `visit_clobber` and move `DummyAstNode` to `rustc_expand`) - rust-lang/rust#141507 (atomic_load intrinsic: use const generic parameter for ordering) - rust-lang/rust#141538 (implement `va_arg` for x86_64 systemv) - rust-lang/rust#141669 (float: Replace some approximate assertions with exact) - rust-lang/rust#141747 (rustdoc: display doc(cfg(false)) properly) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned petrochenkov Dec 3, 2024

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 Dec 3, 2024

estebank force-pushed the issue-56328 branch from 4ff5353 to e30e764 Compare December 3, 2024 21:20

estebank commented Dec 3, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

estebank changed the title ~~Replace "false" cfg_attr in AST with a placeholder attribute for accurate span tracking~~ Replace evaluated cfg_attr in AST with a placeholder attribute for accurate span tracking Dec 3, 2024

This comment has been minimized.

Sign in to view

estebank force-pushed the issue-56328 branch from cd613fd to d2d1ac1 Compare December 4, 2024 06:29

estebank force-pushed the issue-56328 branch from d2d1ac1 to 78b23f6 Compare December 10, 2024 22:01

This comment has been minimized.

Sign in to view

estebank mentioned this pull request Dec 12, 2024

Incorrect clippy::derivable_impls in the presence of conditional compilation rust-lang/rust-clippy#13160

Open

estebank force-pushed the issue-56328 branch from 78b23f6 to 5ebcd19 Compare December 12, 2024 23:11

estebank force-pushed the issue-56328 branch from 5ebcd19 to 59e4c2c Compare March 14, 2025 19:37

rustbot added the A-attributes Area: Attributes (`#[…]`, `#![…]`) label Mar 14, 2025

petrochenkov mentioned this pull request Mar 14, 2025

expand: Leave traces when expanding cfg_attr attributes #138515

Merged

rustbot 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 14, 2025

This comment has been minimized.

Sign in to view

estebank force-pushed the issue-56328 branch from 5412241 to ee4596b Compare May 28, 2025 14:40

estebank changed the title ~~Use cfg_attr in AST with a placeholder attribute for accurate suggestion~~ Use cfg_attr_trace in AST with a placeholder attribute for accurate suggestion May 28, 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 28, 2025

tgross35 mentioned this pull request May 28, 2025

Rollup of 7 pull requests #141707

Closed

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 May 28, 2025

estebank force-pushed the issue-56328 branch from ee4596b to f80e3ac Compare May 29, 2025 10:24

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 May 29, 2025

matthiaskrgr mentioned this pull request May 30, 2025

Rollup of 8 pull requests #141753

Merged

bors merged commit 7aba37d into rust-lang:master May 30, 2025
9 checks passed

rustbot added this to the 1.89.0 milestone May 30, 2025

This comment has been minimized.

Sign in to view

jieyouxu added the L-unused_extern_crates Lint: unused_extern_crates label May 30, 2025

Use cfg_attr_trace in AST with a placeholder attribute for accurate suggestion #133823

Use cfg_attr_trace in AST with a placeholder attribute for accurate suggestion #133823

Uh oh!

Conversation

estebank commented Dec 3, 2024 • edited by tgross35 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

estebank Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

rustbot commented Dec 3, 2024

Uh oh!

This comment has been minimized.

This comment has been minimized.

petrochenkov commented Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

estebank commented Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

petrochenkov commented Mar 13, 2025

Uh oh!

rustbot commented Mar 14, 2025

Uh oh!

estebank commented Mar 14, 2025

Uh oh!

petrochenkov commented Mar 14, 2025

Uh oh!

This comment has been minimized.

estebank commented May 28, 2025

Uh oh!

bors commented May 28, 2025

Uh oh!

tgross35 commented May 28, 2025

Uh oh!

estebank commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

estebank commented May 29, 2025

Uh oh!

bors commented May 29, 2025

Uh oh!

Uh oh!

Kobzol commented May 30, 2025

Uh oh!

This comment has been minimized.

rust-timer commented May 30, 2025

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

Uh oh!

Use `cfg_attr_trace` in AST with a placeholder attribute for accurate suggestion #133823

Use `cfg_attr_trace` in AST with a placeholder attribute for accurate suggestion #133823

estebank commented Dec 3, 2024 •

edited by tgross35

Loading

petrochenkov commented Dec 10, 2024 •

edited

Loading

estebank commented Dec 10, 2024 •

edited

Loading

estebank commented May 29, 2025 •

edited

Loading